Closed Bug 479521 Opened 13 years ago Closed 13 years ago

Don't follow unsafe same-site to cross-site redirects

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files)

Attached patch Patch to fixSplinter Review
We don't implement redirects for preflighted requests. However the current code erroneously follow same-site -> cross-site redirects.

This patch fixes this. It also makes sure that when we block such a redirect, we do it in such a way that bug 458248 is fixed.

I also fixed a bug where I noticed that we're not resetting enough access-control state when an XHR object is reused. This could cause us to deny same-site requests which should have succeeded.


In order to test all this I needed to use 307 redirects. Other redirect types change POST is into GET. However we currently prompt the user for 307 redirects. This of course is not acceptable during mochitest. So I added a pref to allow mochitest to disable this prompt (we might actually want to disable this prompt for other reasons too, but that's a separate bug).
Flags: blocking1.9.1+
Attached patch Patch -wSplinter Review
Attachment #363405 - Flags: superreview?(jst)
Attachment #363405 - Flags: review?(jst)
RFC 2616 section 5.1.1 says methods are case-sensitive.  Does the RFC lie?
It doesn't fully correspond to the reality that XMLHttpRequest lives in, no.

For example

xhr.open("POST", ...);
xhr.open("post", ...);
xhr.open("PosT", ...);

are all equivalent, they all send the method "POST" to the server.

This applies to all 'common' methods, such as 'get' and 'delete'. There has even been discussions about applying it to all unknown methods. Not sure where that discussion landed.
Attachment #363405 - Flags: superreview?(jst)
Attachment #363405 - Flags: superreview+
Attachment #363405 - Flags: review?(jst)
Attachment #363405 - Flags: review+
Comment on attachment 363405 [details] [diff] [review]
Patch -w

Looks good.
Whiteboard: needs landing
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
This landed made a bunch of the cross-site XHR tests fail on the Linux tinderboxes, so I'm going to back it out.
Er, looks like you beat me to it.
There was a missing |rv =| in the code that was moved. Didn't matter before since in the old place rv had already been used, in the new it ended up uninitialized.

Landed with that fixed on mozilla-central and 1.9.1 branch.

http://hg.mozilla.org/mozilla-central/rev/5d0ecd2ac88e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/223c4d40bb76
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Looks like tests are orange on 1.9.1.
Backed out from branch
Keywords: fixed1.9.1
So this is working on trunk but breaking tests on the branch? Can we figure out why?
Whiteboard: needs landing → [needs 1.9.1 landing][caused orange on branch?]
Landed on branch. Bug 468443 is what caused the orange on branch since the fix hasn't been ported there.
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing][caused orange on branch?]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.