Closed
Bug 801576
Opened 12 years ago
Closed 12 years ago
Align Gecko and the spec on cross-origin access to window.history
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: dev-doc-complete, relnote)
Attachments
(4 files, 1 obsolete file)
1.35 KB,
patch
|
mrbkap
:
review+
akeybl
:
approval-mozilla-esr17+
lsblakk
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
1020 bytes,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
2.72 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
3.13 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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•12 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 2•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=25043303c42f
Assignee | ||
Comment 3•12 years ago
|
||
CCing smaug in case he has any thoughts about the weird WebKit navigation behavior or my proposed behavior change here.
Comment 4•12 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.
Assignee | ||
Comment 5•12 years ago
|
||
Fixed a few tests and pushed again to try: https://tbpl.mozilla.org/?tree=Try&rev=e2bfdf119790
Comment 6•12 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.
Assignee | ||
Comment 7•12 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 8•12 years ago
|
||
Attachment #671841 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #671842 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #671843 -
Flags: review?(justin.lebar+bug)
Comment 11•12 years ago
|
||
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•12 years ago
|
||
Attachment #671843 -
Attachment is obsolete: true
Attachment #671855 -
Flags: review?(justin.lebar+bug)
Comment 13•12 years ago
|
||
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•12 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•12 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+
Updated•12 years ago
|
Attachment #671841 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #671842 -
Flags: review?(mrbkap) → review+
Comment 18•12 years ago
|
||
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 19•12 years ago
|
||
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+
Assignee | ||
Comment 20•12 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•12 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 22•12 years ago
|
||
Quick bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab316048d5e1
Assignee | ||
Comment 23•12 years ago
|
||
Annnnd, another bustage fix for the inexplicable android issue: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3b672c303a
Comment 24•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
Assignee: nobody → bobbyholley+bmo
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 25•11 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?
Assignee | ||
Comment 26•11 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.
Comment 27•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #26) I have filed bug 839867
Updated•11 years ago
|
Blocks: CVE-2013-0793
Assignee | ||
Comment 28•11 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 29•11 years ago
|
||
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•11 years ago
|
Whiteboard: [leave uplift for bholley]
Comment 30•11 years ago
|
||
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
Assignee | ||
Comment 31•11 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.
Updated•11 years ago
|
Whiteboard: [leave uplift for bholley]
Assignee | ||
Comment 32•11 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 33•11 years ago
|
||
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+
Comment 36•10 years ago
|
||
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.
Description
•