421622 - XMLHttpRequest from chrome content clears Referer header

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Manish Singh]

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

Comment 1

[Manish Singh]

And this is now regressed on 1.8.1 branch, due to what landed for bug #403168, at least according to CVS logs.

Comment 2

[Matthew (lilmatt) Willis]

Adding regression keyword and requesting b1.8.1.13? to at least get this looked at.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 308077 [details] [diff] [review]
Don't set referrer for code from the system principal

a1.9=beltzner

Comment 4

[Matthew (lilmatt) Willis]

Patch checked in on trunk.
new revision: 1.226; previous revision: 1.225

Awaiting branch version of patch.

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Reed Loden [:reed]]

Fixed on trunk means this is FIXED.

Comment 7

[Manish Singh]

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

Comment 8

[Manish Singh]

Verified working on latest tinderbuild.

Comment 9

[Manish Singh]

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

Comment 10

[Matthew (lilmatt) Willis]

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?

Comment 11

[Reed Loden [:reed]]

Why did you reopen this bug? It's fixed on trunk.

Comment 12

[Reed Loden [:reed]]

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.

Comment 13

[Samuel Sidler (old account; do not CC)]

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?

Comment 14

[Manish Singh]

Attachment: Mochitest testcase

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

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.

Comment 16

[Manish Singh]

Attachment: Incorporated suggestions from Waldo for testcase code

Comment 17

[Matthew (lilmatt) Willis]

Attachment: rev0 - add additional makefiles to toolkit-makefiles.sh

The test code adds a Makefile, and the content/base/test dir was missing

Comment 18

[Benjamin Smedberg]

Comment on attachment 310122 [details] [diff] [review]
rev0 - add additional makefiles to toolkit-makefiles.sh

trivial changes to *makefiles.sh do not need review

Comment 19

[Matthew (lilmatt) Willis]

Testcase and toolkit-makefiles.sh landed on trunk

Comment 20

[Samuel Sidler (old account; do not CC)]

1.8.1.13 is shipped.

Comment 21

[Daniel Veditz [:dveditz]]

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

Comment 22

[Reed Loden [:reed]]

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

Comment 23

[Daniel Veditz [:dveditz]]

Nominating to consider for the inserted 1.8.1.14 regression-fix release

Comment 24

[Daniel Veditz [:dveditz]]

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.

Comment 25

[Daniel Veditz [:dveditz]]

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

Comment 26

[Daniel Veditz [:dveditz]]

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

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Is there an easy way to verify this fix in the 1.8.1.14 build?