Last Comment Bug 401564 - HTTP ACCEPT header not preserved after GET redirect (HTTP 302, HTTP 303, HTTP 307)
: HTTP ACCEPT header not preserved after GET redirect (HTTP 302, HTTP 303, HTTP...
Status: RESOLVED FIXED
[mentor=jdm][lang=c++]
: helpwanted, student-project
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: All All
: -- major with 8 votes (vote)
: mozilla30
Assigned To: Scott West
:
Mentors:
: 414543 570271 (view as bug list)
Depends on: 216828
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-29 08:02 PDT by Alejandro
Modified: 2014-03-01 00:31 PST (History)
35 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test and fix for losing Accept header information after redirect (3.65 KB, patch)
2014-02-16 11:21 PST, Scott West
no flags Details | Diff | Splinter Review
bug-401564.patch (4.03 KB, patch)
2014-02-25 13:02 PST, Scott West
mcmanus: review+
Details | Diff | Splinter Review
bug-401564.patch (4.04 KB, patch)
2014-02-25 18:56 PST, Scott West
no flags Details | Diff | Splinter Review

Description Alejandro 2007-10-29 08:02:04 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8

The HTTP ACCEPT header is not preserved when after a GET redirect. That leads that the second GET is not using the original HTTP ACCEPT header, but is reset to the standard.

This is a really important feature for the proper implementation of RESTful architectures. I'm currently developing some RESTful prototypes and this feature is a must when dealing with aliasing of resources, for instance in order to access resources by their ID or any other resource key field (as its unique name). The problem is that the http accept header in particular is very important to determine the proper format to de-reference to the resource to (as when using Ruby on Rails respond_to in order to determine the content-type to respond with).



Reproducible: Always

Steps to Reproduce:
http://localhost:3034/elements/ApplicantDetails

GET /elements/ApplicantDetails HTTP/1.1
Host: localhost:3034
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Accept: application/json
Accept-Language: en-gb,es-es;q=0.7,en;q=0.3
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
X-Requested-With: XMLHttpRequest
X-Prototype-Version: 1.5.0
Referer: http://localhost:3034/elements/
Cookie: _XMLDictionaryREST_session_id=BAh7CDoOcmV0dXJuX3RvIg8vZWxlbWVudHMvOgtsb2NhbGUiCmVuLWdiIgpm%250AbGFzaElDOidBY3Rpb25Db250cm9sbGVyOjpGbGFzaDo6Rmxhc2hIYXNoewAG%250AOgpAdXNlZHsA--3392c9915ea7c440fd48aca0423dedd0e26f5595

HTTP/1.x 302 Moved Temporarily
Content-Length: 99
Connection: close
Date: Mon, 29 Oct 2007 14:55:47 GMT
Status: 302 Found
Location: http://localhost:3034/elements/96
X-Runtime: 0.01500
Cache-Control: no-cache
Server: Mongrel 0.3.13.3
Content-Type: text/html; charset=utf-8
----------------------------------------------------------
http://localhost:3034/elements/96

GET /elements/96 HTTP/1.1
Host: localhost:3034
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Accept-Language: en-gb,es-es;q=0.7,en;q=0.3
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Referer: http://localhost:3034/elements/
Cookie: _XMLDictionaryREST_session_id=BAh7CDoOcmV0dXJuX3RvIg8vZWxlbWVudHMvOgtsb2NhbGUiCmVuLWdiIgpm%250AbGFzaElDOidBY3Rpb25Db250cm9sbGVyOjpGbGFzaDo6Rmxhc2hIYXNoewAG%250AOgpAdXNlZHsA--3392c9915ea7c440fd48aca0423dedd0e26f5595
Comment 1 Alejandro 2007-10-29 08:07:49 PDT
Another paste just to make clear that the Content-Type returned by the redirect does affect the HTTP ACCEPT header used by FF when redirecting :)

http://localhost:3034/elements/ApplicantIdentifier?_dc=1193670352239

GET /elements/ApplicantIdentifier?_dc=1193670352239 HTTP/1.1
Host: localhost:3034
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Accept: application/json
Accept-Language: en-gb,es-es;q=0.7,en;q=0.3
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
X-Requested-With: XMLHttpRequest
X-Prototype-Version: 1.5.0
Referer: http://localhost:3034/elements/
Cookie: _XMLDictionaryREST_session_id=BAh7CDoOcmV0dXJuX3RvIhEvZWxlbWVudHMvOTY6C2xvY2FsZSIKZW4tZ2Ii%250ACmZsYXNoSUM6J0FjdGlvbkNvbnRyb2xsZXI6OkZsYXNoOjpGbGFzaEhhc2h7%250AAAY6CkB1c2VkewA%253D--945a7380007bab5ac06eceb4a235805531881f47

HTTP/1.x 302 Moved Temporarily
Content-Length: 99
Connection: close
Date: Mon, 29 Oct 2007 15:05:52 GMT
Status: 302 Found
Location: http://localhost:3034/elements/97
X-Runtime: 0.06200
Cache-Control: no-cache
Server: Mongrel 0.3.13.3
Content-Type: application/json; charset=utf-8
----------------------------------------------------------
http://localhost:3034/elements/97

GET /elements/97 HTTP/1.1
Host: localhost:3034
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5
Accept-Language: en-gb,es-es;q=0.7,en;q=0.3
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Referer: http://localhost:3034/elements/
Cookie: _XMLDictionaryREST_session_id=BAh7CDoOcmV0dXJuX3RvIhEvZWxlbWVudHMvOTY6C2xvY2FsZSIKZW4tZ2Ii%250ACmZsYXNoSUM6J0FjdGlvbkNvbnRyb2xsZXI6OkZsYXNoOjpGbGFzaEhhc2h7%250AAAY6CkB1c2VkewA%253D--945a7380007bab5ac06eceb4a235805531881f47
Comment 2 Myk Melez [:myk] [@mykmelez] 2009-03-03 01:34:15 PST
I see this too, and not just with the Accept header; my If-Modified-Since custom header was dropped from a redirected request in a recent coding project.

In my case I was able to work around the problem by updating the URL I was loading to the new one, but that's not always possible (hence the whole point of having a mechanism for automatically redirecting old URLs to new ones).

XMLHttpRequest should really be preserving these custom headers across redirect requests.
Comment 3 Myk Melez [:myk] [@mykmelez] 2009-03-03 01:35:53 PST
*** Bug 414543 has been marked as a duplicate of this bug. ***
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-03-09 15:01:42 PDT
Yeah, I think all custom headers are dropped. Looking at the redirect code the other day I didn't see code that copied any headers over, other than referrer since that is stored separately from other headers.

What does the HTTP spec say on the subject?
Comment 5 Myk Melez [:myk] [@mykmelez] 2009-03-09 15:23:16 PDT
RFC 2616 (Hypertext Transfer Protocol -- HTTP/1.1) <http://www.ietf.org/rfc/rfc2616.txt> section 10.3 (Redirection 3xx) doesn't say anything one way or the other about header preservation across redirects.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-03-09 16:12:32 PDT
What do other browsers do? One way to try is with XMLHttpRequest, though checking with <script> and <img> might also be possible.
Comment 7 Myk Melez [:myk] [@mykmelez] 2009-03-09 18:47:10 PDT
(In reply to comment #6)
> What do other browsers do? One way to try is with XMLHttpRequest, though
> checking with <script> and <img> might also be possible.

I wrote a testcase <http://www.melez.com/mozilla/bug401564.html> that retrieves http://www.melez.com/foo (which redirects to http://www.melez.com/bar), setting an If-Modified-Since header that causes the server to return 304 (not modified) if the header is preserved.

The testcase then outputs the response status code, which is either 304 (if the header is preserved) or 200 (if the header is not preserved).

Here are the results for a few common browsers:

Firefox 3: 200
Safari 3: 304
Chrome 1: 304
IE 7: 200
Opera 9.64: 200
Comment 8 Pelle Wessman 2009-03-11 01:14:51 PDT
RFC 2616 (Hypertext Transfer Protocol -- HTTP/1.1) <http://www.ietf.org/rfc/rfc2616.txt> section 10.3 says that "This class of status code indicates that further action needs to be taken by the user agent in order to fulfill the request." - I interpret that as if the second redirected request should be considered to be the same as the first request, only to another location, something that 10.3.4 also suggest when saying that the 303 See Other does not contain "a substitute reference for the originally requested resource".

An XMLHttpRequest should contain the same headers for all redirected requests except those of 201 and 303 as far as I understand - because those two are not considered as substitute requests.

Although there needs to be a way to specify headers for 201 and 303 redirects as well by perhaps being able to disable XMLHttpRequest automatically redirecting requests so that we manually can specify the correct headers.

I would also like to point out that this applies to all HTTP methods - POST as well as GET and probably PUT and the others as well.
Comment 9 Myk Melez [:myk] [@mykmelez] 2009-03-11 22:31:31 PDT
Although I don't think the HTTP spec says or implies anything about what to do in this situation, I do agree with Pelle that it would make sense for XMLHttpRequest to persist these headers or, if it's not safe to automatically persist headers, then also not automatically follow the redirect.

It's not safe to simply automatically follow the redirect but without the headers that the caller expects the request to contain, as currently occurs.
Comment 10 Boris Zbarsky [:bz] 2010-09-08 11:43:46 PDT
*** Bug 570271 has been marked as a duplicate of this bug. ***
Comment 11 Kaz Nishimura 2011-04-29 04:56:13 PDT
A redirected IMG request also has similar problem too, even on Firefox 4.0.1.

My simple experiment showed the first IMG request was sent with "Accept: image/png,image/*;q=0.8,*/*;q=0.5", but the redirected one was with "Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", which is clearly wrong as Firefox cannot display text/html as an IMG tag.
Comment 12 Josh Matthews [:jdm] (away until 9/3) 2011-05-26 22:54:51 PDT
Worth noting is also bug 553888, which is scoped to redirected XMLHttpRequests retaining custom headers.
Comment 13 Josh Matthews [:jdm] (away until 9/3) 2011-05-31 04:21:16 PDT
Jonas, there's a contributor who's interested in working on this bug. Do we have a firm decision one way or the other as to whether it's desired?
Comment 14 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-06-28 11:46:17 PDT
I will help out the contributor that is interested in working on this. Who is he/she?
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-28 12:16:38 PDT
The one thing I'd be concerned about is that we don't cause scary headers to be forwarded across cross-origin redirects. Especially in requests that originate from plugins which generally means that the plugin doesn't get to participate in the redirect.

Here's the type of plugin-initiated request redirects we currently block:
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginStreamListenerPeer.cpp#1394

I suspect that code need to block more redirects
Comment 16 Gili 2012-06-20 13:50:47 PDT
It looks like FireFox redirects on HTTP 201 as well, which is "Really Bad" (tm). Please fix this soon!
Comment 17 Patrick McManus [:mcmanus] PTO until Sep 6 2012-06-20 14:04:49 PDT
(In reply to Gili from comment #16)
> It looks like FireFox redirects on HTTP 201 as well, which is "Really Bad"
> (tm). Please fix this soon!

firefox does not generally redirect on 201 - test
http://www.ducksong.com/cgi-bin/nph-redir-201

can you provide a test case that illustrates your comment?
Comment 18 Gili 2012-06-20 15:10:31 PDT
Hi Patrick,

My mistake. I am invoking http://api.jquery.com/jQuery.ajax/ against a URI that returns HTTP 201. I was mislead by the fact that "data" returned a Document instead of an empty object if the resource returns an empty response body. I can confirm that xhr.status and xhr.getResponseHeader("Location") return the values I was looking for. Sorry for the false alarm.
Comment 19 ankouc 2012-11-01 19:33:11 PDT
i would like to work on this bug.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-11-01 20:44:12 PDT
Like the assignee field currently says, it's ok to simply take this bug. I.e. assign the bug to yourself and attach a patch once you have one :)
Comment 21 Josh Matthews [:jdm] (away until 9/3) 2012-11-07 11:24:29 PST
Aniss, I heard you were asking about how to create a test for this. You should use the xpcshell harness (https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests); there are many examples in netwerk/test/unit to learn from. In particular, you can create an nsHttpServer with two handlers - one causes a redirect to the second, and the second checks for the presence of the header. The test_307_redirect.js test might be a good reference.
Comment 22 ankouc 2012-11-07 11:36:44 PST
i was thinking of adding this line of code in the line 1563 of nsHttpChannel.cpp: 
newchannel.setrequestheader("accept",mRequestHead.peekheader(accept),0);

what do yoou think about that??
Comment 23 Josh Matthews [:jdm] (away until 9/3) 2012-11-07 11:39:42 PST
I think it's in your interest to write a test that doesn't pass, then try your patch and see if it works.
Comment 24 Scott West 2014-02-16 11:20:21 PST
I took a shot at this bug, following the advice in this thread.

I'm a bit concerned that the solution is a bit too specialized, and it should try to replicate all the additional headers (are they easily found?).
Comment 25 Scott West 2014-02-16 11:21:41 PST
Created attachment 8376849 [details] [diff] [review]
Test and fix for losing Accept header information after redirect

This patch only copies the Accept header, and leaves all others. Is it preferable to handle all additional headers that were in the original request?
Comment 26 Josh Matthews [:jdm] (away until 9/3) 2014-02-16 19:10:20 PST
Woo, there's a test and everything! Scott, bug 216828 covers general headers for redirected requests, so this one can probably stay focused on just Accept. You'll want to set the review flag on your patch (click Details) to '?' and target it at :sicking, I think; this will kick off our code review process.
Comment 27 Scott West 2014-02-16 21:55:34 PST
Josh, thanks for taking the time to respond (on your vacation no less!), much appreciated!
Comment 28 Scott West 2014-02-25 11:54:55 PST
Is there anything else I should be doing to/for/with this bug?
Comment 29 Patrick McManus [:mcmanus] PTO until Sep 6 2014-02-25 12:21:10 PST
Comment on attachment 8376849 [details] [diff] [review]
Test and fix for losing Accept header information after redirect

Review of attachment 8376849 [details] [diff] [review]:
-----------------------------------------------------------------

Scott - thanks for doing this.

Please make the following changes and request review again from me with the new patch. After that I'll give it a r+ and you're ready to check it in. You can do that with the checkin-needed keyword documented https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1811,5 @@
>    httpChannel->SetRedirectionLimit(mRedirectionLimit - 1);
>  
> +  // convey the Accept header value
> +  {
> +    nsCString oldAcceptValue;

nsAutoCString

@@ +1813,5 @@
> +  // convey the Accept header value
> +  {
> +    nsCString oldAcceptValue;
> +    nsresult hasHeader = mRequestHead.GetHeader(nsHttp::Accept, oldAcceptValue);
> +    if (hasHeader == NS_OK) {

if (NS_SUCCEEDED(hasHeader))

::: netwerk/test/unit/test_bug401564.js
@@ +2,5 @@
> +"use strict";
> +Cu.import("resource://testing-common/httpd.js");
> +
> +var httpserver = null;
> +const URL = "http://localhost:8888";

you don't need URL

@@ +3,5 @@
> +Cu.import("resource://testing-common/httpd.js");
> +
> +var httpserver = null;
> +const URL = "http://localhost:8888";
> +const uri = URL + "/redirect";

you don't need uri

@@ +4,5 @@
> +
> +var httpserver = null;
> +const URL = "http://localhost:8888";
> +const uri = URL + "/redirect";
> +const noRedirectURI = URL + "/content";

you can just use "/content".. technically http/1 requires an absolute uri location, but gecko allows relative ones (all browsers do). this saves you the trouble of looking up the port number.

@@ +29,5 @@
> +{
> +  httpserver = new HttpServer();
> +  httpserver.registerPathHandler("/redirect", redirectHandler);
> +  httpserver.registerPathHandler("/content", contentHandler);
> +  httpserver.start(8888);

httpserver.start(-1)
Comment 30 Scott West 2014-02-25 13:02:49 PST
Created attachment 8381655 [details] [diff] [review]
bug-401564.patch

Updates to the test and patch after review.
Comment 31 Patrick McManus [:mcmanus] PTO until Sep 6 2014-02-25 13:07:32 PST
Comment on attachment 8381655 [details] [diff] [review]
bug-401564.patch

Review of attachment 8381655 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!
Comment 32 Josh Matthews [:jdm] (away until 9/3) 2014-02-25 14:59:59 PST
If you attach a patch with "r=mcmanus" at the end of the commit message summary, you can add the checkin-needed keyword (see the Keywords field up top) and it will be committed.
Comment 33 Scott West 2014-02-25 18:56:34 PST
Created attachment 8381860 [details] [diff] [review]
bug-401564.patch

Same as before, just changing commit message to have r=mcmanus
Comment 34 Ryan VanderMeulen [:RyanVM] 2014-02-26 06:09:56 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a737c64fa74
Comment 35 Ryan VanderMeulen [:RyanVM] 2014-02-26 13:28:04 PST
https://hg.mozilla.org/mozilla-central/rev/3a737c64fa74

Note You need to log in before you can comment on or make changes to this bug.