29299 – [Soup] Should not ignore body for redirection responses

RESOLVED FIXED 29299

[Soup] Should not ignore body for redirection responses

https://bugs.webkit.org/show_bug.cgi?id=29299

Summary [Soup] Should not ignore body for redirection responses

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	Details Formatted Diff Diff
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-	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS Linux

Product WebKit

Component WebKitGTK

Assignee

Nobody

Reported

2009-09-16 06:34 PDT

Modified

2010-10-15 10:13 PDT History

CC List

5 users Show

URL

Keywords Gtk, Soup

Depends on

44261

Blocks

Dependencies

tree graph