Bug 29299

Summary: [Soup] Should not ignore body for redirection responses
Product: WebKit Reporter: Gustavo Noronha (kov) <gustavo>
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

Gustavo Noronha (kov)
Reported 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>
Attachments
Do not ignore body on 302 for redirect purpouses (6.97 KB, patch)
2010-09-09 03:23 PDT, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Do not ignore body on 302 for redirect purpouses (SoupURILoader version) (6.70 KB, patch)
2010-09-09 08:58 PDT, Sergio Villar Senin 		mrobinson: review-
DetailsFormatted DiffDiff
Do not ignore body on 302 for redirect purpouses (SoupURILoader version) (6.86 KB, patch)
2010-10-06 03:46 PDT, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Do not ignore body on 302 for redirect purpouses (SoupURILoader version) (7.23 KB, patch)
2010-10-15 04:41 PDT, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 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.
Sergio Villar Senin
Comment 2 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
Sergio Villar Senin
Comment 3 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
Gustavo Noronha (kov)
Comment 4 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.
Martin Robinson
Comment 5 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.
Sergio Villar Senin
Comment 6 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
Gustavo Noronha (kov)
Comment 7 2010-10-06 06:17:17 PDT
Comment on attachment 69917 [details] Do not ignore body on 302 for redirect purpouses (SoupURILoader version) Oki!
Sergio Villar Senin
Comment 8 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.
Eric Seidel (no email)
Comment 9 2010-10-14 11:37:54 PDT
Seems bug 44261 landed. What should be done here?
Martin Robinson
Comment 10 2010-10-14 11:39:45 PDT
Can we land this now?
Sergio Villar Senin
Comment 11 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)
WebKit Commit Bot
Comment 12 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
Sergio Villar Senin
Comment 13 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
Eric Seidel (no email)
Comment 14 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.
Eric Seidel (no email)
Comment 15 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.
WebKit Commit Bot
Comment 16 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>
WebKit Commit Bot
Comment 17 2010-10-15 10:13:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.