Bug 29299

Summary: [Soup] Should not ignore body for redirection responses
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, jmalonzo, mrobinson, svillar
Priority: P2 Keywords: Gtk, Soup
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 44261    
Bug Blocks:    
Attachments:
Description Flags
Do not ignore body on 302 for redirect purpouses
none
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)
mrobinson: review-
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)
none
Do not ignore body on 302 for redirect purpouses (SoupURILoader version) none

Description Gustavo Noronha (kov) 2009-09-16 06:34:37 PDT
We currently fail to redirect when braindead techniques like this are used:

> curl -v http://flipper.googlelabs.com/
* About to connect() to flipper.googlelabs.com port 80 (#0)
*   Trying 74.125.93.141... connected
* Connected to flipper.googlelabs.com (74.125.93.141) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.19.5 (x86_64-pc-linux-gnu) libcurl/7.19.5 OpenSSL/0.9.8k zlib/1.2.3.3 libidn/1.15 libssh2/1.2
> Host: flipper.googlelabs.com
> Accept: */*
> 
< HTTP/1.1 302 Found
< Content-Type: text/html; charset=utf-8
< Expires: Fri, 01 Jan 1990 00:00:00 GMT
< Pragma: no-cache
< Cache-control: no-cache, must-revalidate
< Set-Cookie: FLIPS=7c1545bffe118b96; path=/
< Date: Wed, 16 Sep 2009 13:34:01 GMT
< Server: Google Frontend
< Transfer-Encoding: chunked
< 
* Connection #0 to host flipper.googlelabs.com left intact
* Closing connection #0
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN"><html><head><title>Google Fast Flip</title><meta http-equiv="REFRESH" content="0;url=http://fastflip.googlelabs.com"></HEAD><BODY></BODY></HTML>
Comment 1 Sergio Villar Senin 2010-09-08 09:59:32 PDT
I created https://bugzilla.gnome.org/show_bug.cgi?id=629088 in gnome bugzilla as this could be considered as a pure libsoup bug.
Comment 2 Sergio Villar Senin 2010-09-09 03:23:32 PDT
Created attachment 67015 [details]
Do not ignore body on 302 for redirect purpouses

danw suggested to fix that in WebKitGtk+ as it's not libsoup's business according to his words.

I added also a LayoutTest to check this behaviour
Comment 3 Sergio Villar Senin 2010-09-09 08:58:41 PDT
Created attachment 67041 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

Just in case #44261 and all the SoupURILoader stuff lands before this one
Comment 4 Gustavo Noronha (kov) 2010-09-13 01:49:16 PDT
Comment on attachment 67015 [details]
Do not ignore body on 302 for redirect purpouses

Looks right to me. I think it would be cleaner to tell layoutTestController to waitUntilDone in the first page that is loaded, though. Not opposed to keeping it as is if there's a reason I missed, though.
Comment 5 Martin Robinson 2010-10-04 09:00:50 PDT
Comment on attachment 67041 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

View in context: https://bugs.webkit.org/attachment.cgi?id=67041&action=review

This looks very reasonable to me, but because of the funky ChangeLogs and Gustavo's comment, I'll r-.

> LayoutTests/ChangeLog:13
> +        Tests that 302 redirections without Location do not ignore the
> +        body for redirection purpouses (the body could have a meta
> +        http-equiv=refresh)
> +
> +        [Soup] Should not ignore body for redirection responses
> +        https://bugs.webkit.org/show_bug.cgi?id=29299
> +
> +        * http/tests/navigation/redirect302-metaredirect-expected.txt: Added.
> +        * http/tests/navigation/redirect302-metaredirect.html: Added.

I think the order of these is typically reversed with the bug link coming first.

> WebCore/ChangeLog:11
> +        Body is now provided to WebKitGtk+ in some redirections (like 302)
> +        because it could be used by servers to perform clunky redirections
> +        for example using http-equiv=REFRESH
> +
> +        [Soup] Should not ignore body for redirection responses
> +        https://bugs.webkit.org/show_bug.cgi?id=29299
> +

Same thing here.
Comment 6 Sergio Villar Senin 2010-10-06 03:46:56 PDT
Created attachment 69917 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

New version
   * fixed ChangeLogs
   * Updated the patch as now m_msg was replaced by m_soupMessage

Regarding the waitUntilDone the main reason to design the test like that is that all the http/test/redirect* tests follow the same structure. I think in general it's a good idea to harmonize the way tests works
Comment 7 Gustavo Noronha (kov) 2010-10-06 06:17:17 PDT
Comment on attachment 69917 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

Oki!
Comment 8 Sergio Villar Senin 2010-10-06 08:10:53 PDT
(In reply to comment #7)
> (From update of attachment 69917 [details])
> Oki!

Great! We need #44261 first before landing this one.
Comment 9 Eric Seidel (no email) 2010-10-14 11:37:54 PDT
Seems bug 44261 landed.  What should be done here?
Comment 10 Martin Robinson 2010-10-14 11:39:45 PDT
Can we land this now?
Comment 11 Sergio Villar Senin 2010-10-14 11:50:14 PDT
(In reply to comment #10)
> Can we land this now?

Yeah we can land the SoupURILoader version (ignore the other one)
Comment 12 WebKit Commit Bot 2010-10-14 12:24:05 PDT
Comment on attachment 69917 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

Rejecting patch 69917 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69917]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69917&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=29299&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69917 from bug 29299.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Gustavo Noronha Silva', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4427032
Comment 13 Sergio Villar Senin 2010-10-15 04:41:54 PDT
Created attachment 70851 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

New version of the fix properly rebased against current master
Comment 14 Eric Seidel (no email) 2010-10-15 07:04:53 PDT
Comment on attachment 67015 [details]
Do not ignore body on 302 for redirect purpouses

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 67015 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 15 Eric Seidel (no email) 2010-10-15 07:04:57 PDT
Comment on attachment 69917 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

Cleared Gustavo Noronha Silva's review+ from obsolete attachment 69917 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 WebKit Commit Bot 2010-10-15 10:12:56 PDT
Comment on attachment 70851 [details]
Do not ignore body on 302 for redirect purpouses (SoupURILoader version)

Clearing flags on attachment: 70851

Committed r69861: <http://trac.webkit.org/changeset/69861>
Comment 17 WebKit Commit Bot 2010-10-15 10:13:02 PDT
All reviewed patches have been landed.  Closing bug.