Bug 330795 - XmlHttpRequest HTTP method redirect handling
Summary: XmlHttpRequest HTTP method redirect handling
Status: RESOLVED FIXED
Alias: None
Product: kio
Classification: Unmaintained
Component: http (show other bugs)
Version: 4.11.3
Platform: openSUSE Linux
: NOR normal
Target Milestone: ---
Assignee: kdelibs bugs
URL: http://greenbytes.de/tech/tc/httpredi...
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-05 11:20 UTC by Julian Reschke
Modified: 2014-03-12 21:26 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.12.3
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Reschke 2014-02-05 11:20:23 UTC
a) it appears that HEAD requests are slow (for some value of slow); maybe they time out because something is waiting for the payload?

b) when following a redirect, the method name is never rewritten. However, it needs to be rewritten for status code 303 (to GET, unless the request was HEAD). It probably also should be rewritten for 301 and 302 (POST->GET) for compatibility with other browsers.

Full test suite: http://greenbytes.de/tech/tc/httpredirects/

Reproducible: Always

Steps to Reproduce:
1. Run tests at http://greenbytes.de/tech/tc/httpredirects/t301methods.html and http://greenbytes.de/tech/tc/httpredirects/t303methods.html

Actual Results:  
HEAD slow. 

When following a redirect, the method name is never rewritten. However, it needs to be rewritten for status code 303 (to GET, unless the request was HEAD). It probably also should be rewritten for 301 and 302 (POST->GET) for compatibility with other browsers.


Expected Results:  
HEAD should not time out.

Method rewriting for 303 needs to follow the HTTP spec.
Comment 1 Dawit Alemayehu 2014-02-11 00:43:11 UTC
a) is only true for the khtml browser engine.
b) applies to the kio_http ioslave and hence anything that uses it.

Can you please open a separate report against the KHTML browser engine for a. I will fix b with this ticket.
Comment 2 Dawit Alemayehu 2014-02-11 10:30:00 UTC
Fix pending review: https://git.reviewboard.kde.org/r/115651/
Comment 3 Dawit Alemayehu 2014-02-11 10:32:08 UTC
Note that the above fix is tested using both KHTML and webkit engines. For some reason HEAD XHR requests are not only slow, but also fail the redirection request with this fix.
Comment 4 Dawit Alemayehu 2014-02-14 15:50:27 UTC
Git commit 944eb352595a912fc829b1fca4edfca7d00c4de1 by Dawit Alemayehu.
Committed on 11/02/2014 at 10:11.
Pushed by adawit into branch 'KDE/4.12'.

Fix redirection handling in kio_http.
REVIEW: 115651
FIXED-IN: 4.12.3

M  +1    -0    kio/DESIGN.metadata
M  +4    -2    kio/kio/accessmanager.cpp
M  +3    -2    kio/kio/accessmanagerreply_p.cpp
M  +7    -1    kio/kio/job.cpp
M  +29   -13   kioslave/http/http.cpp

http://commits.kde.org/kdelibs/944eb352595a912fc829b1fca4edfca7d00c4de1
Comment 5 Julian Reschke 2014-02-14 16:04:04 UTC
The fix looks correct to me, but the code comments are somewhat misleading. You may want to cite <http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-26.html#status.3xx> instead.
Comment 6 Dawit Alemayehu 2014-02-14 20:53:14 UTC
(In reply to comment #5)
> The fix looks correct to me, but the code comments are somewhat misleading.
> You may want to cite
> <http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-26.
> html#status.3xx> instead.

Dunno why you think the code comment is misleading. It was exactly what was there before and the cited sections of the actual RFC we implemented 2616 clearly stated that changing a POST request to a GET as a result of a 301 response is wrong:

10.3.2  301 Moved Temporarily
....
   If the 301 status code is received in response to a request other
   than GET or HEAD, the user agent MUST NOT automatically redirect the
   request unless it can be confirmed by the user, since this might
   change the conditions under which the request was issued.

      Note: When automatically redirecting a POST request after
      receiving a 301 status code, some existing HTTP/1.0 user agents
      will erroneously change it into a GET request.

10.3.3   302 Found

If the 302 status code is received in response to a request other
   than GET or HEAD, the user agent MUST NOT automatically redirect the
   request unless it can be confirmed by the user, since this might
   change the conditions under which the request was issued.

      Note: RFC 1945 and RFC 2068 specify that the client is not allowed
      to change the method on the redirected request.  However, most
      existing user agent implementations treat 302 as if it were a 303
      response, performing a GET on the Location field-value regardless
      of the original request method. The status codes 303 and 307 have
      been added for servers that wish to make unambiguously clear which
      kind of reaction is expected of the client.

So I fail to see why the comment would be wrong regardless of which spec it pointed to.
KDE's KIO completely ignores the first part, "must not automatically redirect without prompting the user",  purposefully violates the second to conform to prevailing implementations. BTW, even after the fixes for this bug and #331007 are merged, we will not be fully compliant because we cannot easily fix the POST->POST redirection that is expected for 307 and 308 responses. IOW, POST redirection for 307/308 response will still be converted to a GET request.
Comment 7 Julian Reschke 2014-02-14 22:26:21 UTC
The code comment is misleading because RFC 2616 has been obsoleted by the document that I referenced.
Comment 8 Andrea Iacovitti 2014-03-12 21:26:19 UTC
Git commit 863f775bacf0a8f7a8ec34f558cc46ceabeacdf8 by Andrea Iacovitti.
Committed on 12/03/2014 at 21:17.
Pushed by aiacovitti into branch 'master'.

Fix redirection handling in kio_http.
REVIEW: 115651
FIXED-IN: 4.12.3

(Forwardport kdelibs commit 944eb35)
CCMAIL: adawit@kde.org

M  +1    -0    docs/metadata.txt
M  +7    -1    src/core/transferjob.cpp
M  +29   -13   src/ioslaves/http/http.cpp
M  +4    -2    src/widgets/accessmanager.cpp
M  +3    -2    src/widgets/accessmanagerreply_p.cpp

http://commits.kde.org/kio/863f775bacf0a8f7a8ec34f558cc46ceabeacdf8