Bug 175281 – Sandbox flags do not support document.domain control

• Component: DOM
• Status: RESOLVED FIXED

[Brent Fulgham]

The SandboxingFlag enumeration is missing an option for "Document.Domain" access. We should add this so that websites can block frames from manipulating the frame's domain.

[Brent Fulgham]

It seems like this might just be a matter of merging this Blink revision:

https://crrev.com/162703

[Brent Fulgham]

Whoops. I meant:

https://bugs.chromium.org/p/chromium/issues/detail?id=277084

[Radar WebKit Bug Importer]

<rdar://problem/33778936>

[Brent Fulgham]

Created attachment 317594 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

As an FYI, there is some discussion about removing document.origin from the spec:
https://github.com/whatwg/dom/issues/410

[Brent Fulgham]

Created attachment 317627 [details]
Patch

[Brent Fulgham]

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!

[Brent Fulgham]

Committed r220427: <http://trac.webkit.org/changeset/220427>