Closed Bug 553888 Opened 12 years ago Closed 10 years ago
Additional XHR request after Redirect response doesn't forward non standard headers
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.
I've got a simple set of PHP and HTML that reproduces this.
Assignee: general → nobody
QA Contact: general → general
I've whipped up an xpcshell testcase demonstrating this behaviour.
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.
Assignee: nobody → josh
Attachment #433797 - Attachment is obsolete: true
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).
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.
Attachment #433801 - Attachment is obsolete: true
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?
Why yes, you are correct. Is the space tradeoff of storing the values worth it vs. looking up the current value for each name?
Attachment #433860 - Attachment is obsolete: true
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
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
So, I don't believe there was any feedback on the mailing list. Who makes the call here?
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.
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.
That sounds like a separate issue; I think we should be copying the Accept header on the necko level.
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.
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?
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?
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.
It's too late for this to make FF4, so take your time. Also need feedback regarding electrolysis.
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.
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".
Jonas, is this (still) something we want? I can pick it up again if it is.
i would say that we still want this yes
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.
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.
Updated in accordance with Jonas' concerns.
Attachment #536132 - Flags: review?(cbiesinger)
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 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.
Attachment #536132 - Flags: review?(cbiesinger) → review?(jonas)
Whiteboard: [Doc] This bug required people to hack their own libraries/frameworks See comments 17, 23, 29, 30
Comment on attachment 536132 [details] [diff] [review] Duplicate XHR request headers when following redirect. Review of attachment 536132 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #536132 - Flags: review?(jonas) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/52292b725188 I almost missed the fact that the r=biesi is the patch was wrong...
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Whiteboard: [Doc] This bug required people to hack their own libraries/frameworks See comments 17, 23, 29, 30 → [inbound][Doc][This bug required people to hack their own libraries/frameworks See comments 17, 23, 29, 30]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Could anyone provide a testcase or steps on how can this issue get verified? Thanks.
(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 ...
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?
It was fixed at the time. Please file a new bug with a testcase so we can quickly confirm the problem you're experiencing.
You need to log in before you can comment on or make changes to this bug.