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

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

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

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch -w

Comment 2

[Jeff Walden [:Waldo]]

RFC 2616 section 5.1.1 says methods are case-sensitive.  Does the RFC lie?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 363405 [details] [diff] [review]
Patch -w

Looks good.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This landed made a bunch of the cross-site XHR tests fail on the Linux tinderboxes, so I'm going to back it out.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, looks like you beat me to it.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like tests are orange on 1.9.1.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Backed out from branch

Comment 10

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

So this is working on trunk but breaking tests on the branch? Can we figure out why?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed on branch. Bug 468443 is what caused the orange on branch since the fix hasn't been ported there.