553888 - Additional XHR request after Redirect response doesn't forward non standard headers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bogdan Gusiev]

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

I've got a simple set of PHP and HTML that reproduces this.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Failing xpcshell test

I've whipped up an xpcshell testcase demonstrating this behaviour.

Comment 3

[Josh Matthews [:jdm]]

Attachment: 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 [:bzbarsky]]

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

Attachment: 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)]

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

Attachment: 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)]

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)]

I've also sent an email to public-webapps to clarify this in the xmlhttprequest spec:
http://lists.w3.org/Archives/Public/public-webapps/2010JanMar/0881.html

Comment 10

[Josh Matthews [:jdm]]

So, I don't believe there was any feedback on the mailing list.  Who makes the call here?

Comment 13

[Jason Duell]

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]

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 [:bzbarsky]]

That sounds like a separate issue; I think we should be copying the Accept header on the necko level.

Comment 16

[James Leigh]

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]

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]

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

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]

It's too late for this to make FF4, so take your time.

Also need feedback regarding electrolysis.

Comment 21

[rubs00]

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]

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]

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

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]

i would say that we still want this yes

Comment 26

[Josh Matthews [:jdm]]

Attachment: 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]

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

Attachment: Duplicate XHR request headers when following redirect.

Updated in accordance with Jonas' concerns.

Comment 29

[Alex Ford]

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: http://www.codetunnel.com/blog/post/21/firefox-doesnt-include-custom-request-headers-with-302-redirects

Comment 30

[Eugenio Lattanzio]

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

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]

Comment on attachment 536132 [details] [diff] [review]
Duplicate XHR request headers when following redirect.

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

r=me

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/52292b725188

I almost missed the fact that the r=biesi is the patch was wrong...

Comment 34

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/52292b725188

Comment 35

[Jesse Ruderman]

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 36

[Eric Shepherd [:sheppy]]

Trevor updated these articles:

https://developer.mozilla.org/en/XMLHttpRequest
https://developer.mozilla.org/en/nsIXMLHttpRequest
https://developer.mozilla.org/en/Firefox_7_for_developers

Comment 37

[Trif Andrei-Alin[:AlinT]]

Could anyone provide a testcase or steps on how can this issue get verified?
Thanks.

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

It was fixed at the time. Please file a new bug with a testcase so we can quickly confirm the problem you're experiencing.