| Summary: | Sandbox flags do not support document.domain control | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||
| Component: | DOM | Assignee: | Brent Fulgham <bfulgham> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, cdumez, dbates, esprehn+autocc, kangil.han, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=175300 | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 175300 | ||||||||
| Attachments: |
|
||||||||
|
Description
Brent Fulgham
2017-08-07 13:12:09 PDT
It seems like this might just be a matter of merging this Blink revision: https://crrev.com/162703 Whoops. I meant: https://bugs.chromium.org/p/chromium/issues/detail?id=277084 Created attachment 317594 [details]
Patch
Comment on attachment 317594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317594&action=review > Source/WebCore/dom/Document.cpp:4490 > + if (isSandboxed(SandboxDocumentDomain)) Eh.. Would you mind removing the "// FIXME(175281): Check for 'document.domain' sandbox flag and return an exception if present." comment a few lines below? :) > LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:6 > +Denied: SecurityError: Assignment is forbidden for sandboxed iframes. I think this is going to be flaky. Given how the test is written, there is currently no guarantee the script in one particular iframe will run before the other. Therefore, those "Denied" lines may be switched. I think the test need to be written in a way that first inserts the first iframe, tests it and then does so for a second iframe. > LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:7 > +Denied: SecurityError: The document has a null effectiveDomain. Would this actually succeed if we moved the test to http/tests? If so, I'd suggest we do this. > LayoutTests/fast/frames/sandboxed-iframe-domain.html:6 > + testRunner.dumpAsText(); Not needed with js-test. > LayoutTests/fast/frames/sandboxed-iframe-domain.html:23 > + window.testRunner.notifyDone(); finishJSTest() since you're using js-test. > LayoutTests/fast/frames/sandboxed-iframe-domain.html:27 > +description("This test verifies that a sandboxed iframe does not have permission to modify the document.domain property."); I think it would look better if the description was at the beginning of the script. As an FYI, there is some discussion about removing document.origin from the spec: https://github.com/whatwg/dom/issues/410 Created attachment 317627 [details]
Patch
Comment on attachment 317594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317594&action=review >> Source/WebCore/dom/Document.cpp:4490 >> + if (isSandboxed(SandboxDocumentDomain)) > > Eh.. Would you mind removing the "// FIXME(175281): Check for 'document.domain' sandbox flag and return an exception if present." comment a few lines below? :) Oh gosh! Doh! >> LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:6 >> +Denied: SecurityError: Assignment is forbidden for sandboxed iframes. > > I think this is going to be flaky. Given how the test is written, there is currently no guarantee the script in one particular iframe will run before the other. Therefore, those "Denied" lines may be switched. > I think the test need to be written in a way that first inserts the first iframe, tests it and then does so for a second iframe. Good point. I had originally hoped to be able to have one "succeeded" and one "denied", but the nature of our test environment prevents us from setting domain to anything legal. So another option would just be to test the sandbox case, but I wanted to show that with the sandbox property, you get a sandbox error, and without you get different behavior. >> LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:7 >> +Denied: SecurityError: The document has a null effectiveDomain. > > Would this actually succeed if we moved the test to http/tests? If so, I'd suggest we do this. I just gave up on having a pass and a fail case. I can't get our test system (which runs on 127.0.0.1:8000 to play nice with CORS and the various rules for setting a domain as a subdomain of the current domain. It's just not feasible. >> LayoutTests/fast/frames/sandboxed-iframe-domain.html:6 >> + testRunner.dumpAsText(); > > Not needed with js-test. OK! >> LayoutTests/fast/frames/sandboxed-iframe-domain.html:23 >> + window.testRunner.notifyDone(); > > finishJSTest() since you're using js-test. OK! >> LayoutTests/fast/frames/sandboxed-iframe-domain.html:27 >> +description("This test verifies that a sandboxed iframe does not have permission to modify the document.domain property."); > > I think it would look better if the description was at the beginning of the script. OK! Committed r220427: <http://trac.webkit.org/changeset/220427> |