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

RESOLVED FIXED in mozilla7

Status

()

Core
DOM
--
major
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Bogdan Gusiev, Assigned: jdm)

Tracking

({dev-doc-complete})

Trunk
mozilla7
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
I've got a simple set of PHP and HTML that reproduces this.
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
(Assignee)

Comment 2

8 years ago
Created attachment 433797 [details] [diff] [review]
Failing xpcshell test

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

Comment 3

8 years ago
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.
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).
(Assignee)

Comment 5

8 years ago
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.
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?
(Assignee)

Comment 7

8 years ago
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?
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
(Assignee)

Comment 10

7 years ago
So, I don't believe there was any feedback on the mailing list.  Who makes the call here?
(Assignee)

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

7 years ago
Duplicate of this bug: 385035
(Assignee)

Updated

7 years ago
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.

Comment 14

7 years ago
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.

Comment 16

7 years ago
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

7 years ago
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?
(Assignee)

Comment 19

7 years ago
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.

Comment 21

7 years ago
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

7 years ago
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

7 years ago
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.
(Assignee)

Comment 24

6 years ago
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
(Assignee)

Comment 26

6 years ago
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.
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.
(Assignee)

Comment 28

6 years ago
Created attachment 536132 [details] [diff] [review]
Duplicate XHR request headers when following redirect.

Updated in accordance with Jonas' concerns.
Attachment #536132 - Flags: review?(cbiesinger)
(Assignee)

Updated

6 years ago
Attachment #535731 - Attachment is obsolete: true
Attachment #535731 - Flags: review?(cbiesinger)

Comment 29

6 years ago
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

6 years ago
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?
(Assignee)

Comment 31

6 years ago
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)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 35

6 years ago
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
(Assignee)

Updated

6 years ago
Depends on: 674048
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
Keywords: dev-doc-needed → dev-doc-complete
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 ...

Comment 39

3 years ago
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?
(Assignee)

Comment 40

3 years ago
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.