Closed Bug 553888 Opened 10 years ago Closed 9 years ago

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

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: agresso, Assigned: jdm)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound][Doc][This bug required people to hack their own libraries/frameworks See comments 17, 23, 29, 30])

Attachments

(1 file, 5 obsolete files)

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
Component: JavaScript Engine → DOM
QA Contact: general → general
Attached patch Failing xpcshell test (obsolete) — Splinter Review
I've whipped up an xpcshell testcase demonstrating this behaviour.
Attached patch Patch + test (obsolete) — Splinter Review
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).
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attached patch Patch v3 (obsolete) — Splinter Review
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 385035
Duplicate of this bug: 570271
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".
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.
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
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #433865 - Attachment is obsolete: true
Attachment #535731 - Flags: review?(cbiesinger)
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)
Attachment #535731 - Attachment is obsolete: true
Attachment #535731 - Flags: review?(cbiesinger)
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
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)
Keywords: dev-doc-needed
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+
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
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
Merged:
http://hg.mozilla.org/mozilla-central/rev/52292b725188
Status: ASSIGNED → RESOLVED
Closed: 9 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
Depends on: 674048
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.