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

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({dev-doc-complete, relnote})

unspecified
mozilla19
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)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 3

7 years ago
CCing smaug in case he has any thoughts about the weird WebKit navigation behavior or my proposed behavior change here.
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.
(Assignee)

Comment 5

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

Comment 7

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
Attachment #671842 - Flags: review?(mrbkap)
(Assignee)

Comment 10

7 years ago
Posted patch Fix {push,pop}State tests. v1 (obsolete) — Splinter Review
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-
(Assignee)

Comment 12

7 years ago
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+
(Assignee)

Comment 14

7 years ago
Given our terrible track record of testing very basic stuff like this, I think this is worthwhile.
Attachment #671886 - Flags: review?(mrbkap)
(Assignee)

Comment 15

7 years ago
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+
Attachment #671841 - Flags: review?(mrbkap) → review+
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+
(Assignee)

Comment 20

7 years ago
Not sure if this is relnote-worthy, but I'll set the flag and let other folks decide.
Keywords: relnote
(Assignee)

Comment 21

7 years ago
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+
(Assignee)

Comment 23

7 years ago
Annnnd, another bustage fix for the inexplicable android issue:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3b672c303a
Assignee: nobody → bobbyholley+bmo
Depends on: 809318
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?
(Assignee)

Comment 26

6 years ago
(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.
(In reply to Bobby Holley (:bholley) from comment #26)
I have filed bug 839867
(Assignee)

Comment 28

6 years ago
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+
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 31

6 years ago
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]
(Assignee)

Comment 32

6 years ago
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+
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.
You need to log in before you can comment on or make changes to this bug.