Bug 60440 – [Qt] Redirection of HTTP POST (3xx) incorrectly includes original POST data

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dawit A.]

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.

[Dawit A.]

Created attachment 92694 [details]
QtTestBrowser HTTP header output...

[Holger Freyther]

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.

[Dawit A.]

Created attachment 92695 [details]
proposed patch v1

[Holger Freyther]

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.

[Dawit A.]

(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.

[Alexey Proskuryakov]

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.

[Dawit A.]

(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.

[Benjamin Poulain]

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?

[Luiz Agostini]

(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.

[Luiz Agostini]

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.

[Luiz Agostini]

Created attachment 92898 [details]
patch

[Alexey Proskuryakov]

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.

[Luiz Agostini]

(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.

[Alexey Proskuryakov]

> 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.

[Luiz Agostini]

Created attachment 92980 [details]
patch

[Luiz Agostini]

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.

[Alexey Proskuryakov]

Absolutely!

Please let me know if I should r+ this - I was expecting someone working on Qt port to do it.

[Benjamin Poulain]

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.

[Kenneth Rohde Christiansen]

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?

[Benjamin Poulain]

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?

[Luiz Agostini]

(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*)

[Luiz Agostini]

Created attachment 93726 [details]
patch

[Kenneth Rohde Christiansen]

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 ...

[Luiz Agostini]

	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

[Ademar Reis]

Revision r86693 cherry-picked into qtwebkit-2.2 with commit 6eee7bc <http://gitorious.org/webkit/qtwebkit/commit/6eee7bc>