Bug 60440

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

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.

Created attachment 92695 [details] proposed patch v1

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.

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

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

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?

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

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.

Created attachment 92898 [details] patch

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.

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

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

Created attachment 92980 [details] patch

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.

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

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.

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?

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?

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

Created attachment 93726 [details] patch

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

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

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