Bug 60440 - [Qt] Redirection of HTTP POST (3xx) incorrectly includes original POST data
Summary: [Qt] Redirection of HTTP POST (3xx) incorrectly includes original POST data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Luiz Agostini
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-07 10:24 PDT by Dawit A.
Modified: 2011-05-19 13:34 PDT (History)
10 users (show)

See Also:


Attachments
QtTestBrowser HTTP header output... (5.19 KB, text/plain)
2011-05-07 10:24 PDT, Dawit A.
no flags Details
proposed patch v1 (3.55 KB, patch)
2011-05-07 12:49 PDT, Dawit A.
ap: review-
Details | Formatted Diff | Diff
patch (14.61 KB, patch)
2011-05-09 18:03 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (14.24 KB, patch)
2011-05-10 10:49 PDT, Luiz Agostini
benjamin: review-
Details | Formatted Diff | Diff
patch (14.24 KB, patch)
2011-05-16 18:32 PDT, Luiz Agostini
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dawit A. 2011-05-07 10:24:13 PDT
When a HTTP POST operation is redirected (3xx response code), then the subsequent GET operation incorrectly sends the Content-Type header  and the POST data from the previous POST operation! This bug was originally reported downstream against konqueror + kwebkitpart, but has successfully been reproduced with QtTestBrowser. The HTTP header output from QtTestBrowser it attached and the original downstream bug report can be found at https://bugs.kde.org/show_bug.cgi?id=268694.
Comment 1 Dawit A. 2011-05-07 10:24:53 PDT
Created attachment 92694 [details]
QtTestBrowser HTTP header output...
Comment 2 Holger Freyther 2011-05-07 11:23:04 PDT
It would be nice if you could produce a LayoutTest. Please see http://trac.webkit.org/browser/trunk/LayoutTests/http/tests for a pool of tests that involve the networking stack.
Comment 3 Dawit A. 2011-05-07 12:49:43 PDT
Created attachment 92695 [details]
proposed patch v1
Comment 4 Holger Freyther 2011-05-07 13:18:28 PDT
Comment on attachment 92695 [details]
proposed patch v1

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

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

Please provide a layout test, it is really quite easy to do and makes sure this bug stays fixed.
Comment 5 Dawit A. 2011-05-07 13:47:09 PDT
(In reply to comment #4)
> (From update of attachment 92695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92695&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests. (OOPS!)
> 
> Please provide a layout test, it is really quite easy to do and makes sure this bug stays fixed.

Sorry but the last time I attempted to provide a layout test it was nothing but a nightmare. See BR# 50521.

Even if it was not the case, I do not even know how to provide one for this bug since it involves redirection of a HTTP POST request. Either way, I honestly do not have the time to write a layout test for this. Way too many things on my own plate. 

The site that is mentioned in the downstream bug report is a good test site, which is what I used to test QtTestBrowser before and after applying the fix. Moreover, the changes are very very obvious since the old request gets cloned when handling a redirection and the data from the POST op never gets cleaned.
Comment 6 Alexey Proskuryakov 2011-05-07 14:24:01 PDT
Comment on attachment 92695 [details]
proposed patch v1

This is definitely testable, so the fix can't be accepted without a regression test. There is a chance that we already have one, just disabled for Qt.
Comment 7 Dawit A. 2011-05-07 18:35:41 PDT
(In reply to comment #6)
> (From update of attachment 92695 [details])
> This is definitely testable, so the fix can't be accepted without a regression test. 
> There is a chance that we already have one, just disabled for Qt.

There seems to be some stuff under LayoutTests/http/tests/loading/redirect-* that I quite do not understand how it works. Though in general I agree with the the idea behind these layout tests to prevent future regressions, I neither have the time nor the inclination to properly setup a layout test environment to test this fix and generate the proper test files. 

As such I will leave this up to whomever wants to get this bug fixed in QtWebkit and has the ability to create the required layout test. I have already worked around this issue downstream in the stuff I maintain ; so my work is done.
Comment 8 Benjamin Poulain 2011-05-09 01:56:50 PDT
I set P1 since this could potentially be used to steal data if the server is not careful about where it redirects.

Luiz, interested in finishing this?
Comment 9 Luiz Agostini 2011-05-09 09:04:11 PDT
(In reply to comment #8)
> I set P1 since this could potentially be used to steal data if the server is not careful about where it redirects.
> 
> Luiz, interested in finishing this?

I will take a look at it today.
Comment 10 Luiz Agostini 2011-05-09 18:01:43 PDT
It turned out that only the header Content-type was been sent after receiving 301, 302 or 303 HTTP status codes. 307 was been properly handled.
Comment 11 Luiz Agostini 2011-05-09 18:03:34 PDT
Created attachment 92898 [details]
patch
Comment 12 Alexey Proskuryakov 2011-05-09 19:27:46 PDT
Comment on attachment 92898 [details]
patch

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

It's a pity that redirection tests are scattered across many http/tests subdirectories. See e.g. closely related loading/redirect-methods.html, loading/307-after-303-after-post.html, misc/redirect.php, navigation/redirect302-basic.html. That said, the placement you chose seems fine.

Looks good to me, leaving for someone working on Qt to give official review.

> LayoutTests/http/tests/navigation/post-301-response.html:6
> +    if (!window.layoutTestController)
> +        return;

These tests don't seem to really require layoutTestController. Please fix them to work in browser, and confirm that Firefox passes.

> LayoutTests/http/tests/navigation/post-301-response.html:12
> +    if (form)
> +        form.submit();

Why are these checks necessary?  Please remove them if they are not.
Comment 13 Luiz Agostini 2011-05-10 09:33:23 PDT
(In reply to comment #12)
> (From update of attachment 92898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92898&action=review
> 
> It's a pity that redirection tests are scattered across many http/tests subdirectories. See e.g. closely related loading/redirect-methods.html, loading/307-after-303-after-post.html, misc/redirect.php, navigation/redirect302-basic.html. That said, the placement you chose seems fine.
> 
> Looks good to me, leaving for someone working on Qt to give official review.
> 
> > LayoutTests/http/tests/navigation/post-301-response.html:6
> > +    if (!window.layoutTestController)
> > +        return;
> 
> These tests don't seem to really require layoutTestController. Please fix them to work in browser, and confirm that Firefox passes.

The tests as they are work in browser. The form submission is not automated but I left in the form a submit button that may be used in the browser.

layoutTestController is used for dumpAsText, waitUntilDone and notifyDone.

Yes, Firefox passes. The only difference is regarding 307 because in Firefox a confirmation is asked from the user before resending the content to the new URL. The confirmation is needed according to the specification but nor Qt nor Chromium nor Safari do it.

> 
> > LayoutTests/http/tests/navigation/post-301-response.html:12
> > +    if (form)
> > +        form.submit();
> 
> Why are these checks necessary?  Please remove them if they are not.

They are not. I will remove them.
Comment 14 Alexey Proskuryakov 2011-05-10 10:32:04 PDT
> The form submission is not automated but I left in the form a submit button that may be used in the browser.

Ideally, it should either work automatically even in browser, or there should be instructions for the user on how to run the test manually.

> layoutTestController is used for dumpAsText, waitUntilDone and notifyDone.

Sure, but none of these are necessary when running in browser.
Comment 15 Luiz Agostini 2011-05-10 10:49:59 PDT
Created attachment 92980 [details]
patch
Comment 16 Luiz Agostini 2011-05-12 11:11:37 PDT
ap, I have changed the tests to work automatically in browser and removed the unnecessary checks. I hope that all your comments have been addressed.
Comment 17 Alexey Proskuryakov 2011-05-12 11:33:12 PDT
Absolutely!

Please let me know if I should r+ this - I was expecting someone working on Qt port to do it.
Comment 18 Benjamin Poulain 2011-05-12 12:12:15 PDT
Comment on attachment 92980 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +       Makes sure that the HTTP headers Content-type and Content-length are not included in
> +       the requests that do not have any content.

Indent issue?

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:618
> +    if (m_method != QNetworkAccessManager::PostOperation && m_method != QNetworkAccessManager::PutOperation) {

Looks like OPTION can also contain an entity-body: http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.2

Would you mind checking OPTION?

Maybe the tests could loop over all the standards http method definitions?

This is a bit sidetrack from the original issue. If you prefer to do such things in a separate bug report that is fine by me.
Comment 19 Kenneth Rohde Christiansen 2011-05-12 12:13:50 PDT
Let me have a stab at it! :-)

> LayoutTests/http/tests/navigation/post-302-response-expected.txt:7
> +headers CONTENT_TYPE and CONTENT_LENGTH should not be present.
> +headers CONTENT_TYPE and CONTENT_LENGTH are not present.
> +
> +
> +
> +no POST data should be present.
> +no POST data is present.

These results have a lot of newlines, but I guess that is not your fault. Ah actually you did write the redirected-post-request-contents.php so maybe that is the culprit

> LayoutTests/http/tests/navigation/resources/redirected-post-request-contents.php:4
> +        echo $header . " is present. its value is: " . $_SERVER[$header] . "<br>";

I would write "Its"

> LayoutTests/http/tests/navigation/resources/redirected-post-request-contents.php:23
> +echo "<br><br><br>";

This seems a bit too much. Could you fix that?
Comment 20 Benjamin Poulain 2011-05-12 12:16:37 PDT
And to understand the thing correctly... how do the attributes from the original POST get into the following GET of the rediction? Do we reuse the QNetworkRequest or something like that?
Comment 21 Luiz Agostini 2011-05-12 12:23:37 PDT
(In reply to comment #20)
> And to understand the thing correctly... how do the attributes from the original POST get into the following GET of the rediction? Do we reuse the QNetworkRequest or something like that?

No. They come through ResourceRequest::toNetworkRequest(QObject*)
Comment 22 Luiz Agostini 2011-05-16 18:32:29 PDT
Created attachment 93726 [details]
patch
Comment 23 Kenneth Rohde Christiansen 2011-05-17 01:50:54 PDT
Comment on attachment 93726 [details]
patch

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:619
> +    if (m_method != QNetworkAccessManager::PostOperation && m_method != QNetworkAccessManager::PutOperation) {
> +        m_request.setHeader(QNetworkRequest::ContentTypeHeader, QVariant());

I would add a comment here like: clearing Contents-length and Contents-type here, so make sure they are ...
Comment 24 Luiz Agostini 2011-05-17 12:03:13 PDT
	M	LayoutTests/ChangeLog
	A	LayoutTests/http/tests/navigation/post-301-response-expected.txt
	A	LayoutTests/http/tests/navigation/post-301-response.html
	A	LayoutTests/http/tests/navigation/post-302-response-expected.txt
	A	LayoutTests/http/tests/navigation/post-302-response.html
	A	LayoutTests/http/tests/navigation/post-303-response-expected.txt
	A	LayoutTests/http/tests/navigation/post-303-response.html
	A	LayoutTests/http/tests/navigation/post-307-response-expected.txt
	A	LayoutTests/http/tests/navigation/post-307-response.html
	A	LayoutTests/http/tests/navigation/resources/redirected-post-request-contents.php
	A	LayoutTests/http/tests/navigation/resources/redirection-response.php
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp
Committed r86693
Comment 25 Ademar Reis 2011-05-19 13:34:33 PDT
Revision r86693 cherry-picked into qtwebkit-2.2 with commit 6eee7bc <http://gitorious.org/webkit/qtwebkit/commit/6eee7bc>