The default bug view has changed. See this FAQ.

XMLHttpRequest from chrome content clears Referer header

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
XML
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Manish Singh, Assigned: Manish Singh)

Tracking

({fixed1.8.1.14, fixed1.8.1.15, regression})

Trunk
mozilla1.9beta5
fixed1.8.1.14, fixed1.8.1.15, regression
Points:
---
Bug Flags:
blocking1.8.1.13 -
blocking1.8.1.14 +
blocking1.8.1.15 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 308077 [details] [diff] [review]
Don't set referrer for code from the system principal

When using setRequestHeader to set the Referer header on an XHR from a JS component, it doesn't work because the code in send() unconditionally resets it. This used to work in Gecko 1.8.1 at least, so this is a regression.

Attached patch fixes this, by changing to code to not touch the Referer header if called from chromeland.
Attachment #308077 - Flags: review?(jonas)
(Assignee)

Comment 1

9 years ago
And this is now regressed on 1.8.1 branch, due to what landed for bug #403168, at least according to CVS logs.
Adding regression keyword and requesting b1.8.1.13? to at least get this looked at.
Flags: blocking1.8.1.13?
Keywords: regression
Attachment #308077 - Flags: superreview+
Attachment #308077 - Flags: review?(jonas)
Attachment #308077 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #308077 - Flags: approval1.9?
Comment on attachment 308077 [details] [diff] [review]
Don't set referrer for code from the system principal

a1.9=beltzner
Attachment #308077 - Flags: approval1.9? → approval1.9+
Patch checked in on trunk.
new revision: 1.226; previous revision: 1.225

Awaiting branch version of patch.
Blocks: 403168
We already have 2.0.0.13 release candidate builds. What does this regression mean in practical terms -- is it a stop-ship bug? Seems like a glitch we can just fix in the next version.

Does this patch work on the branch? We can't even consider it for a respin if we don't have a patch ready to go.
Flags: blocking1.8.1.14?
Fixed on trunk means this is FIXED.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
(Assignee)

Comment 7

9 years ago
(In reply to comment #5)
> We already have 2.0.0.13 release candidate builds. What does this regression
> mean in practical terms -- is it a stop-ship bug? Seems like a glitch we can
> just fix in the next version.

There may be extensions that rely on be able to set the referer header. Flock code does, which is why we noticed this regression, so this isn't necessarily theoretical.

> Does this patch work on the branch? We can't even consider it for a respin if
> we don't have a patch ready to go.

This patch doesn't apply to branch. I have a patch for branch, which I will attach shortly, after some testing.
(Assignee)

Comment 8

9 years ago
Verified working on latest tinderbuild.
(Assignee)

Comment 9

9 years ago
Created attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

This is the patch for branch. I had to implement IsSystemPrincipal here, in terms of the 1.8.1 nsIScriptSecurityManager interface, which means getting the system principal and comparing the pointers.
Attachment #309196 - Flags: review?(jonas)
Attachment #309196 - Flags: review?(jonas) → review+
Comment on attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

Requesting a1.8.1.13?

If you're not respinning 13, then please assume a1.8.1.14?
Attachment #309196 - Flags: approval1.8.1.13?
Attachment #309196 - Flags: approval1.8.1.14?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Why did you reopen this bug? It's fixed on trunk.
Branch fixes are tracked with keywords, not bug resolution. Once a bug has been fixed on the trunk, it is resolved as FIXED and not left open pending branch landing.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Olli, do you have any comment as to whether we should take this for 1.8.1.13? Do you think this is a serious enough regression from your patch?
(Assignee)

Comment 14

9 years ago
Created attachment 309706 [details] [diff] [review]
Mochitest testcase
IMO we really should try to avoid all the regressions, so if we now have the
fix, we should take it. Even possibly breaking extensions is bad.
(Assignee)

Comment 16

9 years ago
Created attachment 310112 [details] [diff] [review]
Incorporated suggestions from Waldo for testcase code
Attachment #309706 - Attachment is obsolete: true
Created attachment 310122 [details] [diff] [review]
rev0 - add additional makefiles to toolkit-makefiles.sh

The test code adds a Makefile, and the content/base/test dir was missing
Attachment #310122 - Flags: review?
Attachment #310122 - Flags: review? → review?(benjamin)
Comment on attachment 310122 [details] [diff] [review]
rev0 - add additional makefiles to toolkit-makefiles.sh

trivial changes to *makefiles.sh do not need review
Attachment #310122 - Flags: review?(benjamin) → review+
Testcase and toolkit-makefiles.sh landed on trunk
Flags: in-testsuite? → in-testsuite+
1.8.1.13 is shipped.
Flags: blocking1.8.1.13? → blocking1.8.1.13-
Attachment #309196 - Flags: approval1.8.1.13? → approval1.8.1.13-
Comment on attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

approved for 1.8.1.14, a=dveditz for release-drivers
Attachment #309196 - Flags: approval1.8.1.14? → approval1.8.1.14+
Flags: blocking1.8.1.14? → blocking1.8.1.14+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: 1.8.1 branch patch needs to be checked in
MOZILLA_1_8_BRANCH:

Checking in content/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v  <--  nsXMLHttpRequest.cpp
new revision: 1.156.2.13; previous revision: 1.156.2.12
done
Keywords: checkin-needed → fixed1.8.1.14
Whiteboard: 1.8.1 branch patch needs to be checked in
Nominating to consider for the inserted 1.8.1.14 regression-fix release
Flags: blocking1.8.1.14?
Since this can break extension compatibility, and we were willing to put it on the "will take if we need another RC" list for 1.8.1.13, this should go in the 1.8.1.14 firedrill release.
Flags: blocking1.8.1.14? → blocking1.8.1.14+
Comment on attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

approved for 1.8.1.14, a=dveditz for release-drivers.

For the 1.8.1.14 release please land on the GECKO181_20080311_RELBRANCH
Attachment #309196 - Flags: approval1.8.1.14+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: Needs landing on GECKO181_20080311_RELBRANCH
Checked into GECKO181_20080311_RELBRANCH

Checking in content/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v  <-- 
nsXMLHttpRequest.cpp
new revision: 1.156.2.12.4.1; previous revision: 1.156.2.12
done
Keywords: fixed1.8.1.14
Keywords: checkin-needed
Whiteboard: Needs landing on GECKO181_20080311_RELBRANCH
Is there an easy way to verify this fix in the 1.8.1.14 build?
You need to log in before you can comment on or make changes to this bug.