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

RESOLVED FIXED in Firefox 19

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({dev-doc-complete, relnote})

unspecified
mozilla19
dev-doc-complete, relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 fixed, firefox-esr17 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
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.
https://tbpl.mozilla.org/?tree=Try&rev=25043303c42f
CCing smaug in case he has any thoughts about the weird WebKit navigation behavior or my proposed behavior change here.

Comment 4

5 years ago
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.
Fixed a few tests and pushed again to try:
https://tbpl.mozilla.org/?tree=Try&rev=e2bfdf119790

Comment 6

5 years ago
(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.
(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.
Created attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1
Attachment #671841 - Flags: review?(mrbkap)
Created attachment 671842 [details] [diff] [review]
Fix sandbox test. v1
Attachment #671842 - Flags: review?(mrbkap)
Created attachment 671843 [details] [diff] [review]
Fix {push,pop}State tests. v1
Attachment #671843 - Flags: review?(justin.lebar+bug)
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/).
Attachment #671843 - Flags: review?(justin.lebar+bug) → review-
Created attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2
Attachment #671843 - Attachment is obsolete: true
Attachment #671855 - Flags: review?(justin.lebar+bug)
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.
Attachment #671855 - Flags: review?(justin.lebar+bug) → review+
Created attachment 671886 [details] [diff] [review]
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.
Attachment #671886 - Flags: review?(mrbkap)
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.
Attachment #671855 - Flags: review+

Updated

5 years ago
Attachment #671841 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #671842 - Flags: review?(mrbkap) → review+
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.
Attachment #671886 - Flags: review?(mrbkap) → review+
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
Flags: in-testsuite+
Not sure if this is relnote-worthy, but I'll set the flag and let other folks decide.
Keywords: relnote
Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

I fixed the :8000 thing. Reapplying review for posterity.
Attachment #671855 - Flags: review+
Quick bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab316048d5e1
Annnnd, another bustage fix for the inexplicable android issue:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3b672c303a
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Assignee: nobody → bobbyholley+bmo
Depends on: 809318
Keywords: dev-doc-needed

Comment 25

5 years ago
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?
(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

4 years ago
(In reply to Bobby Holley (:bholley) from comment #26)
I have filed bug 839867
Blocks: 803870
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.
Attachment #671841 - Flags: approval-mozilla-esr17?
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.
Attachment #671841 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Whiteboard: [leave uplift for bholley]
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
status-firefox19: --- → fixed
status-firefox-esr17: --- → fixed
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.
Whiteboard: [leave uplift for bholley]
Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

Nominating for b2g18 per comment 28.
Attachment #671841 - Flags: approval-mozilla-b2g18?
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.
Attachment #671841 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ef5bc6848172
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a1a5c516b32f
status-b2g18-v1.0.1: wontfix → fixed
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.