Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 553888 - Additional XHR request after Redirect response doesn't forward non standard headers
: Additional XHR request after Redirect response doesn't forward non standard h...
[inbound][Doc][This bug required peop...
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major with 12 votes (vote)
: mozilla7
Assigned To: Josh Matthews [:jdm]
: Andrew Overholt [:overholt]
: 385035 (view as bug list)
Depends on: 674048
  Show dependency treegraph
Reported: 2010-03-21 02:44 PDT by Bogdan Gusiev
Modified: 2014-06-12 09:56 PDT (History)
37 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Failing xpcshell test (3.63 KB, patch)
2010-03-21 06:43 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Patch + test (5.91 KB, patch)
2010-03-21 08:02 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Patch v2 (7.03 KB, patch)
2010-03-21 17:19 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Patch v3 (6.10 KB, patch)
2010-03-21 18:58 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Patch v4 (7.29 KB, patch)
2011-05-27 13:19 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Duplicate XHR request headers when following redirect. (9.51 KB, patch)
2011-05-30 11:18 PDT, Josh Matthews [:jdm]
jonas: review+
Details | Diff | Splinter Review

Description Bogdan Gusiev 2010-03-21 02:44:19 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/533.3 (KHTML, like Gecko) Chrome/5.0.352.0 Safari/533.3
Build Identifier: version 3.5.8

Suppose we have the xhr 'POST' request that returns 'redirect' status code. In that case browser is sending additional xhr 'GET' by the given URL. 

The problem is that the second 'GET' request is not recognized as xhr by the server: It doesn't have "X-Requested-With" header contains "XMLHttpRequest"

The problem appears only on Firefox, but not on Webkit. So, believe it is not related to js library bug.

Reproducible: Always

Steps to Reproduce:
1. Implement the server access point that response with redirect on post requests.
2. Implement the server access point that do some conditional actions when "X-Requested-With" is set and not set.
2. Send xhr request to server with header "X-Requested-With" set to "XMLHttpRequest".

Actual Results:  
Browser sent additional xhr to the redirected url without specified header.

Expected Results:  
Second ajax request should have specified header

Forward all non-standard headers to second XHR request specified in first XHR request.
Comment 1 Josh Matthews [:jdm] 2010-03-21 04:32:46 PDT
I've got a simple set of PHP and HTML that reproduces this.
Comment 2 Josh Matthews [:jdm] 2010-03-21 06:43:20 PDT
Created attachment 433797 [details] [diff] [review]
Failing xpcshell test

I've whipped up an xpcshell testcase demonstrating this behaviour.
Comment 3 Josh Matthews [:jdm] 2010-03-21 08:02:22 PDT
Created attachment 433801 [details] [diff] [review]
Patch + test

Well, I got my hands dirty in a whole new area of the project, so this patch may be completely off base.  However, it makes the test pass, which is a start.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2010-03-21 12:03:03 PDT
Why would you only do this for some sorts of replacement channels but not others?

Note that copying all request headers from a POST to a GET is certainly wrong (e.g. it will copy the bogus Content-Length header, etc).
Comment 5 Josh Matthews [:jdm] 2010-03-21 17:19:34 PDT
Created attachment 433860 [details] [diff] [review]
Patch v2

biesi stated that he would be more comfortable with this behaviour being contained in nsXMLHttpRequest, and only copying headers that were specifically set via SetRequestHeader.  The actual implementation is basically unchanged, though.
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-21 17:54:39 PDT
Wouldn't it be more elegant to store the header name/value pairs in the member variable and just iterate over that array, instead of this O(n^2) loop?
Comment 7 Josh Matthews [:jdm] 2010-03-21 18:58:33 PDT
Created attachment 433865 [details] [diff] [review]
Patch v3

Why yes, you are correct.  Is the space tradeoff of storing the values worth it vs. looking up the current value for each name?
Comment 8 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-22 05:57:09 PDT
Comment on attachment 433865 [details] [diff] [review]
Patch v3

I think this is worth the space tradeoff, it's not like this requires a lot of memory.

Also, the behaviour is not equivalent anyway, e.g. for the cookie header.

+    nsCAutoString(header), nsCAutoString(value)

use nsCString instead to avoid an additional copy

+  nsCOMPtr<nsIHttpChannel> newHttpChannel(do_QueryInterface(aNewChannel));

there is no guarantee that the new channel is an http channel
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-24 07:33:06 PDT
I've also sent an email to public-webapps to clarify this in the xmlhttprequest spec:
Comment 10 Josh Matthews [:jdm] 2010-05-07 23:33:06 PDT
So, I don't believe there was any feedback on the mailing list.  Who makes the call here?
Comment 11 Josh Matthews [:jdm] 2010-05-17 10:13:14 PDT
*** Bug 385035 has been marked as a duplicate of this bug. ***
Comment 12 Josh Matthews [:jdm] 2010-06-04 19:52:52 PDT
*** Bug 570271 has been marked as a duplicate of this bug. ***
Comment 13 Jason Duell [:jduell] (needinfo me) 2010-07-28 23:24:35 PDT
Note that if we do this it will take additional logic for electrolysis.  We don't currently copy any HTTP headers from the new parent redirect channel to the HttpChannelChild, and we'd need to do so for any whitelisted XHR headers.
Comment 14 Johannes H. Jensen 2010-08-03 09:02:01 PDT
Headers set via setRequestHeader are not forwarded even when the initial request is GET and the header is a standard one like 'Accept'.

This is particularly frustrating when setting the Accept request header to have the server produce a response in a particular format (e.g. JSON etc). Instead it's ignored completely and Accept: text/html,... is sent instead in the second request. Opera also seems to forward the header properly.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2010-08-03 09:24:26 PDT
That sounds like a separate issue; I think we should be copying the Accept header on the necko level.
Comment 16 James Leigh 2010-08-03 11:09:50 PDT
I created Bug 570271 previously about the Accept header reset. Mozilla seems to be the only browser (that I have tested) that resets the Accept header on redirect.
Comment 17 Stephen Kraushaar 2010-09-16 02:59:24 PDT
Our application uses custom headers to pass authentication information, and this is blocking a bug to remove a dependency on 3rd party cookies. I'm aware that this could likely be handled via GET variables, but is there a possibility of getting a time line on this fix? Should we expect it in FF4?
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-16 03:04:20 PDT
This is unfortunately definitely not getting fixed in FF4. It's too late in the release cycle for that.

What is the status of the patch in the patch in this bug?
Comment 19 Josh Matthews [:jdm] 2010-09-16 08:31:53 PDT
The patch has probably bitrotted, but I don't know how severely.  I could take a a look to see how cleanly it applies today, if desired.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-16 09:58:16 PDT
It's too late for this to make FF4, so take your time.

Also need feedback regarding electrolysis.
Comment 21 rubs00 2010-09-17 15:19:51 PDT
Guys, please fix it. This extremely important issue and creates a lot of headaches. Currently, there is a problem with ASP.NET auth due to this bug.
Comment 22 Bence Balint 2011-01-19 16:37:12 PST
This is really disappointing. I've just ran into this bug, caused me quite some extra work, and it's been 10 months since the report, and the status is still "NEW".
Comment 23 Michael Hasenstein 2011-02-14 01:58:57 PST
I have nothing to contribute but "me too" - although I would like to point out to the other reporters that unfortunately there is little use in getting this (or any) browser bug fixed. It is going to be out there for the next 5 years at least. What I'm going to do is to make sure - on the server - there are no redirects ("Location:..." headers) in HTTP responses when the request is an xhr one. Instead, tell the client-side Javascript code via a "soft" method, e.g. in JSON response data, the redirect URL. Redirects are not all that useful IMHO for AJAX requests anyway, but belong to the realm of normal HTML page loads.
Comment 24 Josh Matthews [:jdm] 2011-05-26 22:53:35 PDT
Jonas, is this (still) something we want? I can pick it up again if it is.
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-26 23:03:18 PDT
i would say that we still want this yes
Comment 26 Josh Matthews [:jdm] 2011-05-27 13:19:23 PDT
Created attachment 535731 [details] [diff] [review]
Patch v4

Unbitrotted, and with an IPC test added. Turns out that there don't appear to be any changes needed for e10s, as the test passes. Reading the code, I believe this is sensible - in nsXMLHttpRequest::OnRedirectVerifyCallback we set the modified headers on the new channel, then trigger the callback that ends up instructing the child channel to SendRedirect2Verify and includes the headers.
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-27 14:21:16 PDT
Hmm.. We'll want to make sure that this doesn't make it possible to add headers half-way through a request.

I.e. if you've started a request and then then call SetRequestHeader, and then the channel is redirected, we want to make sure that the new header isn't added to the post-redirect channel.

This is especially important in the case of cross-site requests since that could be after we've done security checks based on the original set of headers.

I believe that the current patch fails that since headers are added to the array even if adding them to the channel fails.

I think we should fix this in two ways. First of all we should add a state-check at the start of the setRequestHeader method to make sure that we are in the correct state. I.e. that we're in the OPENED state.

Second, as a belts and suspenders thing, we should only add to the array if the httpChannel->SetRequestHeader call succeeds.
Comment 28 Josh Matthews [:jdm] 2011-05-30 11:18:40 PDT
Created attachment 536132 [details] [diff] [review]
Duplicate XHR request headers when following redirect.

Updated in accordance with Jonas' concerns.
Comment 29 Alex Ford 2011-06-08 22:29:19 PDT
Wow, this was reported over a year ago and the status is still new? This issue has caused me great headaches in ASP.NET MVC. Javascript libraries like jQuery pass a custom request header "X-Requested-With" to identify that a request was made with AJAX. When my server does a redirect this request header is lost in Firefox. All other browsers (even IE) correctly forwards this request header along with the next request. Please fix this. I currently have to make my entire MVC controller inherit from a base class that does some nasty hack to store this value in session and pull it out again after the redirect. It's an ugly solution to what should be a non-issue.

See this post for more details:
Comment 30 Eugenio Lattanzio 2011-06-19 17:53:34 PDT
After several headaches thinking grails framework was doing something wrong i've notice is firefox who doesn't pass X-Requested-With header when receive an Status 302, do you plan to fix it?
Comment 31 Josh Matthews [:jdm] 2011-06-20 07:51:11 PDT
Comment on attachment 536132 [details] [diff] [review]
Duplicate XHR request headers when following redirect.

Biesi made the valid point that none of this code is in netwerk.
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-23 13:51:43 PDT
Comment on attachment 536132 [details] [diff] [review]
Duplicate XHR request headers when following redirect.

Review of attachment 536132 [details] [diff] [review]:

Comment 33 :Ms2ger (⌚ UTC+1/+2) 2011-06-25 07:19:22 PDT

I almost missed the fact that the r=biesi is the patch was wrong...
Comment 34 Mounir Lamouri (:mounir) 2011-06-27 02:10:25 PDT
Comment 35 Jesse Ruderman 2011-07-02 18:17:13 PDT
This patch really should have included tests to ensure:
* headers can't be added between the requests (comment 27)
* redirects from same-origin to cross-origin do the right CORS checks
Comment 37 Trif Andrei-Alin[:AlinT] 2011-08-24 02:47:16 PDT
Could anyone provide a testcase or steps on how can this issue get verified?
Comment 38 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-24 04:37:21 PDT
(In reply to Trif Andrei-Alin from comment #37)
> Could anyone provide a testcase or steps on how can this issue get verified?
> Thanks.

There's a test as the first attachment to this bug ...
Comment 39 andrew.luetgers 2014-06-12 09:49:31 PDT
Just ran into this bug on Mac FF v 30.0, a custom header was visible in the network traffic inspector but not in js after a 307 redirect.

Is this really fixed or is there a regression?
Comment 40 Josh Matthews [:jdm] 2014-06-12 09:56:48 PDT
It was fixed at the time. Please file a new bug with a testcase so we can quickly confirm the problem you're experiencing.

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