DevHeads.net

Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests

Review request for kdelibs and Andrea Iacovitti.

Bugs: 331007
<a href="http://bugs.kde.org/show_bug.cgi?id=331007" title="http://bugs.kde.org/show_bug.cgi?id=331007">http://bugs.kde.org/show_bug.cgi?id=331007</a>

Repository: kdelibs

Description
Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD.

Diffs

Diff: <a href="https://git.reviewboard.kde.org/r/115689/diff/" title="https://git.reviewboard.kde.org/r/115689/diff/">https://git.reviewboard.kde.org/r/115689/diff/</a>

Testing
Tested HEAD redirection with <a href="http://greenbytes.de/tech/tc/httpredirects/" title="http://greenbytes.de/tech/tc/httpredirects/">http://greenbytes.de/tech/tc/httpredirects/</a>

Thanks,

Dawit Alemayehu

Comments

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Dawit A at 02/12/2014 - 05:56

(Updated Feb. 12, 2014, 9:56 a.m.)

Review request for kdelibs and Andrea Iacovitti.

Changes
Uploaded the patch.

Please note that the relevant patch to this pull request is the one that changes khtml/ecma/xmlhttprequest.cpp and adds "CMD_STAT" to TransferJob::slotFinished's switch statement. Unfortunately, this patch depends on another one that is currently in review, <a href="https://git.reviewboard.kde.org/r/115651/" title="https://git.reviewboard.kde.org/r/115651/">https://git.reviewboard.kde.org/r/115651/</a>, hence the posted patch here contains the changes from that review request as well.

Bugs: 331007
<a href="http://bugs.kde.org/show_bug.cgi?id=331007" title="http://bugs.kde.org/show_bug.cgi?id=331007">http://bugs.kde.org/show_bug.cgi?id=331007</a>

Repository: kdelibs

Description
Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD.

Diffs (updated)
khtml/ecma/xmlhttprequest.cpp b3707e7
kio/DESIGN.metadata 1351119
kio/kio/accessmanager.cpp 7a806e8
kio/kio/job.cpp 13107c2
kioslave/http/http.cpp b13eed1

Diff: <a href="https://git.reviewboard.kde.org/r/115689/diff/" title="https://git.reviewboard.kde.org/r/115689/diff/">https://git.reviewboard.kde.org/r/115689/diff/</a>

Testing
Tested HEAD redirection with <a href="http://greenbytes.de/tech/tc/httpredirects/" title="http://greenbytes.de/tech/tc/httpredirects/">http://greenbytes.de/tech/tc/httpredirects/</a>

Thanks,

Dawit Alemayehu

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Dawit A at 02/14/2014 - 12:00

(Updated Feb. 14, 2014, 4 p.m.)

Review request for kdelibs and Andrea Iacovitti.

Changes
Updated the patch because I discovered more issues with the way khtml does its xmlhttp requests.

Andrea please review and see if you have any objections. As stated in my comments, setting a custom HTTP header blindly has unintended consequences. It pretends to correctly redirect a POST request to another POST request for 307/308 HTTP response codes when it really does not. IOW, it simply changes the method from GET to POST, because of the presence of the custom HTTP method, but does NOT send any content to the redirected resource like a real POST->POST redirection should.

Bugs: 331007
<a href="http://bugs.kde.org/show_bug.cgi?id=331007" title="http://bugs.kde.org/show_bug.cgi?id=331007">http://bugs.kde.org/show_bug.cgi?id=331007</a>

Repository: kdelibs

Description
Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD.

Diffs (updated)
khtml/ecma/xmlhttprequest.cpp b3707e7
kio/kio/job.cpp abb3dfd

Diff: <a href="https://git.reviewboard.kde.org/r/115689/diff/" title="https://git.reviewboard.kde.org/r/115689/diff/">https://git.reviewboard.kde.org/r/115689/diff/</a>

Testing
Tested HEAD redirection with <a href="http://greenbytes.de/tech/tc/httpredirects/" title="http://greenbytes.de/tech/tc/httpredirects/">http://greenbytes.de/tech/tc/httpredirects/</a>

Thanks,

Dawit Alemayehu

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Dawit A at 02/16/2014 - 15:10

(Updated Feb. 16, 2014, 7:10 p.m.)

Review request for kdelibs and Andrea Iacovitti.

Changes
Always send the "CustomHTTPmethod" meta data.

Bugs: 331007
<a href="http://bugs.kde.org/show_bug.cgi?id=331007" title="http://bugs.kde.org/show_bug.cgi?id=331007">http://bugs.kde.org/show_bug.cgi?id=331007</a>

Repository: kdelibs

Description
Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD.

Diffs (updated)
khtml/ecma/xmlhttprequest.cpp b3707e7
kio/kio/job.cpp abb3dfd

Diff: <a href="https://git.reviewboard.kde.org/r/115689/diff/" title="https://git.reviewboard.kde.org/r/115689/diff/">https://git.reviewboard.kde.org/r/115689/diff/</a>

Testing
Tested HEAD redirection with <a href="http://greenbytes.de/tech/tc/httpredirects/" title="http://greenbytes.de/tech/tc/httpredirects/">http://greenbytes.de/tech/tc/httpredirects/</a>

Thanks,

Dawit Alemayehu

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Dawit A at 02/17/2014 - 13:29

(Updated Feb. 17, 2014, 5:29 p.m.)

Status
This change has been marked as submitted.

Review request for kdelibs and Andrea Iacovitti.

Bugs: 331007
<a href="http://bugs.kde.org/show_bug.cgi?id=331007" title="http://bugs.kde.org/show_bug.cgi?id=331007">http://bugs.kde.org/show_bug.cgi?id=331007</a>

Repository: kdelibs

Description
Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD.

Diffs
khtml/ecma/xmlhttprequest.cpp b3707e7
kio/kio/job.cpp abb3dfd

Diff: <a href="https://git.reviewboard.kde.org/r/115689/diff/" title="https://git.reviewboard.kde.org/r/115689/diff/">https://git.reviewboard.kde.org/r/115689/diff/</a>

Testing
Tested HEAD redirection with <a href="http://greenbytes.de/tech/tc/httpredirects/" title="http://greenbytes.de/tech/tc/httpredirects/">http://greenbytes.de/tech/tc/httpredirects/</a>

Thanks,

Dawit Alemayehu

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Commit Hook at 03/12/2014 - 17:26

This review has been submitted with commit 3d857d2171d4182954a2f4e90a2bc9a862b5c560 by Andrea Iacovitti to branch master.

- Commit Hook

On Feb. 17, 2014, 5:29 p.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Commit Hook at 02/17/2014 - 13:29

This review has been submitted with commit 24d780debd65e5c2e957a0ab3dd1ba5b9885a42b by Dawit Alemayehu to branch KDE/4.12.

- Commit Hook

On Feb. 16, 2014, 7:10 p.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Andrea Iacovitti at 02/16/2014 - 17:37

Ship it!

- Andrea Iacovitti

On Feb. 16, 2014, 7:10 p.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Andrea Iacovitti at 02/15/2014 - 20:38

khtml/ecma/xmlhttprequest.cpp
<https://git.reviewboard.kde.org/r/115689/#comment35055>

This does not seems to fix the "POST->POST redirection with no content on 307 response" issue, instead it has the side effect that POST method is rewritten to GET in 307 redirection.Also it doesn't make possible to do an XHR PUT request with content (!_body.isEmpty()) as actually a POST request is sent to the origin server and not a PUT.
We use KIO::http_post (regardless of method) whenever we need to send a request that includes content, even for methods that do not have a defined semantics for entity-body (e.g. DELETE).
So setting custom method it's always needed.

- Andrea Iacovitti

On Feb. 14, 2014, 4 p.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Dawit A at 02/16/2014 - 15:09

The reason a POST method is rewritten to GET in a 307 redirection has to do with how we originally chose to deal with POST redirection in KIO::TransferJob. Right now all POST redirections are converted to GET regardless of the HTTP status code in TransferJob::slotFinished in job.cpp. See the CMD_SPECIAL section of the switch statement in that function.

As far as then issue with the PUT request and an entity-body, kio_http was designed only to send the entity-body to the server for POST and other webdav requests. Anything that is done to skirt around that to me is a hack by definition. What would be the purpose of allowing entity-body for methods that are not supposed to have such thing, e.g. DELETE?

Anyhow, I do not want to cause regression ; so I will roll back this patch to the way things were. However, those POST->POST redirection tests it seems to pass are wrong because it never sends the entity-body to the redirected resource.

- Dawit

On Feb. 14, 2014, 4 p.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Andrea Iacovitti at 02/15/2014 - 20:42

Because of CustomHTTPMethod i think changes are also needed in kio_http's POST to GET redirection code:

--- kdelibs/kioslave/http/http.cpp
+++ kdelibs/kioslave/http/http.cpp
@@ -3099,16 +3099,17 @@
// this way of doing things! Thus, we are forced to do the same
// thing here. Otherwise, we loose compatibility and might not be
// able to correctly retrieve sites that redirect.
+ const QByteArray method = m_request.methodString();
switch (m_request.responseCode) {
case 301: // Moved Permanently
setMetaData(QLatin1String("permanent-redirect"), QLatin1String("true"));
- if (m_request.method == HTTP_POST) {
+ if (method == "POST ") {
m_request.method = HTTP_GET; // FORCE a GET
setMetaData(QLatin1String("redirect-to-get"), QLatin1String("true"));
}
break;
case 302: // Found
- if (m_request.method == HTTP_POST) {
+ if (method == "POST ") {
m_request.method = HTTP_GET; // FORCE a GET
setMetaData(QLatin1String("redirect-to-get"), QLatin1String("true"));
}

- Andrea Iacovitti

On Feb. 14, 2014, 4 p.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Andrea Iacovitti at 02/12/2014 - 13:34

Ship it!

Looks good from my side.

I added some comments in <a href="https://git.reviewboard.kde.org/r/115651" title="https://git.reviewboard.kde.org/r/115651">https://git.reviewboard.kde.org/r/115651</a> about http.cpp's changes.

- Andrea Iacovitti

On Feb. 12, 2014, 9:56 a.m., Dawit Alemayehu wrote:

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Rolf Eike Beer at 02/12/2014 - 08:58

Am 12.02.2014 10:56, schrieb Dawit Alemayehu:
It looks to me as if the patch also contains the stuff for RR 115651.

Eike

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to

By Bernd Buschinski at 02/12/2014 - 03:20

no diff?

- Bernd Buschinski

On Feb. 12, 2014, 4:29 a.m., Dawit Alemayehu wrote: