801576 - Align Gecko and the spec on cross-origin access to window.history

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The only two objects that are cross-origin accessible per spec are Window and Location. However, Gecko currently also allows access to the back(), forward(), and go() methods on window.history. jst says this is probably just heritage from the netscape 4 days.

Cross-origin security policy is pretty important, so I think we should either change Gecko or change the spec. In particular, we should figure out what other UAs do and whether restricting access to window.history is web-compatible.

Comment 1

[Bobby Holley (:bholley)]

So I put together the following testcase, and evaluated it with hixie's live DOM viewer:

http://pastebin.mozilla.org/1867437

It looks like Gecko navigates the iframe back, Chrome/Safari naviate the entire top-level window back, and Opera and IE deny access to the history object entirely.

Given that, I think it makes the most sense for us to kill cross-origin access to the history object and align with the spec here.

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=25043303c42f

Comment 3

[Bobby Holley (:bholley)]

CCing smaug in case he has any thoughts about the weird WebKit navigation behavior or my proposed behavior change here.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Note, Webkit's session history is very broken even without cross-origin frames or anything like that,
so we should not align our behavior with that.

But yes, killing cross-origin access to the the history object sounds ok, especially
if IE and Opera have that behavior.

Comment 5

[Bobby Holley (:bholley)]

Fixed a few tests and pushed again to try:
https://tbpl.mozilla.org/?tree=Try&rev=e2bfdf119790

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #1)
> and Opera and IE deny access to the history
> object entirely.

What does this actually mean? Do they throw an exception?  or what?
Changing this all is very regression risky, so if we decide to go with the behavior IE has,
better to make sure we behave the same way in all the cases.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #6)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > and Opera and IE deny access to the history
> > object entirely.
> 
> What does this actually mean? Do they throw an exception?  or what?

Yes. The throw an exception when touching the property, just like any cross-origin access to non cross-origin-accessible properties. That's why my patch does here.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Forbid cross-origin access to the History object. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Fix sandbox test. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Fix {push,pop}State tests. v1

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 671843 [details] [diff] [review]
Fix {push,pop}State tests. v1

Yay for this bug, but r- because this patch removes important tests.  You'll need to fix this (e.g. by s/example.com/mochi.test:8000/).

Comment 12

[Bobby Holley (:bholley)]

Attachment: Fix {push,pop}State tests. v2

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

> +  tryBadPushAndReplaceState("http://foo.mochitest:8000");

foo.mochi.test, right?

I wish I knew why I went through all this effort to run the tests at example.com.  Maybe it was because I didn't want to rely on the default mochitest domain not changing (it did change, I believe!).

Anyway, this looks good to me.  Thanks.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Add tests for the same-origin policy. v1

Given our terrible track record of testing very basic stuff like this, I think this is worthwhile.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

Doh, it's actually mochi.test:8888, not mochi.test:8000. Canceling review so that I remember to fix this before landing.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 671886 [details] [diff] [review]
Add tests for the same-origin policy. v1

Review of attachment 671886 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. It would be good to test that we correctly distinguish between setting/getting for the props that are sensitive to that distinction (and checking that we throw correctly if someone tries to set a property that's only gettable.

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e9109dc7da37
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ace0976ace07
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/95806d8419f9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6973d363e3e1

Comment 20

[Bobby Holley (:bholley)]

Not sure if this is relnote-worthy, but I'll set the flag and let other folks decide.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

I fixed the :8000 thing. Reapplying review for posterity.

Comment 22

[Bobby Holley (:bholley)]

Quick bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab316048d5e1

Comment 23

[Bobby Holley (:bholley)]

Annnnd, another bustage fix for the inexplicable android issue:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3b672c303a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9109dc7da37
https://hg.mozilla.org/mozilla-central/rev/ace0976ace07
https://hg.mozilla.org/mozilla-central/rev/95806d8419f9
https://hg.mozilla.org/mozilla-central/rev/6973d363e3e1
https://hg.mozilla.org/mozilla-central/rev/ab316048d5e1
https://hg.mozilla.org/mozilla-central/rev/5e3b672c303a

Comment 25

[Jesper Kristensen]

I have recently done some research on the same origin policy. I have tried to document what I found at https://developer.mozilla.org/en-US/docs/JavaScript/Same_origin_policy_for_JavaScript . I just came across this bug today, and I added a link on the documentation page to the source file defining the policy.

It looks like there are still a number of properties, which are allowed cross-origin in Firefox, but not according to the spec. Namely location.hash, window.blur, window.close, window.closed, window.focus and window.opener. Are there any plans on aligning Firefox and the HTML spec on these additional properties?

Comment 26

[Bobby Holley (:bholley)]

(In reply to Jesper Kristensen from comment #25)
> It looks like there are still a number of properties, which are allowed
> cross-origin in Firefox, but not according to the spec. Namely
> location.hash, window.blur, window.close, window.closed, window.focus and
> window.opener. Are there any plans on aligning Firefox and the HTML spec on
> these additional properties?

I would love to. Making that call, however, requires doing research into what other UAs do, which isn't something I've had the time to do. If you're interested in helping out here, it would be _very_ helpful to make a table listing which of these non-standard properties are available in which UAs (we'd want to test Safari, Chrome, Opera, IE9, and IE10 (in the various compat modes)). If the data looks compelling, I'm happy to land a fix to bring us more in line with the spec.

Comment 27

[Jesper Kristensen]

(In reply to Bobby Holley (:bholley) from comment #26)
I have filed bug 839867

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
blocks bug 803870.
User impact if declined: bug 803870. 
Fix Landed on Version: mozilla19
Risk to taking this patch (and alternatives if risky): Somewhat risky in that this is a web-visible change. However, the old behavior was gecko-specific anyway, so it's not clear that sites would have been depending on it. And the release of this bug in mozilla19 should force sites to adapt anyway (though there's a risk for internal intranet apps).
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

When given the choice of not fixing bug 803870, investigating a new fix (without testing elsewhere), or taking this patch, triage thinks this is the best route.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Sorry, I didn't see your whiteboard comment after I'd pushed. My fault entirely :(

It applied cleanly (just one makefile fixup for the tests was required) and it seems to have landed OK, though. If you want me to backout, I will of course.

FWIW, I rolled the tests and bustage fixes into one commit for esr17. Also, is this something we should consider for b2g18 uplift as well?

https://hg.mozilla.org/releases/mozilla-esr17/rev/d286e7431abb

Comment 31

[Bobby Holley (:bholley)]

No, that's fine. I was just worried that it might need some rebasing and didn't want to waste your time. If the test passed, we should be ok. :-)

Whether or not we uplift to b2g18 depends on what we decide for bug 803870.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

Nominating for b2g18 per comment 28.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

approving for uplift to b2g18 since we do want the bug it's blocking.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ef5bc6848172

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a1a5c516b32f

Comment 36

[Will Bamberg [:wbamberg]]

I'm removing dev-doc-needed, because I think the docs added in bug 965898, comment 48, cover this, too. If anyone disagrees please reinstate dev-doc-needed and explain what else is needed.