Bug 171321 – Add flag allow-modals to iframe sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Frédéric Wang (:fredw)]

I'm extracting this from bug 158875.

https://html.spec.whatwg.org/multipage/browsers.html#attr-iframe-sandbox-allow-modals
https://googlechrome.github.io/samples/block-modal-dialogs-sandboxed-iframe/

[Frédéric Wang (:fredw)]

I did a quick check of the WebKit and Chromium code and I believe this won't be too difficult to implement.

@Bin Lu, Rick:

So if I understand correctly, WebKit currently always allows modal dialogs in sandboxed frames so implementing the allow-modals flag won't change anything for that case, right? The relevant benefit is when allow-modals is not specified: Modals dialogs would then be blocked, providing better security.

This is however a behavior change that might (in theory) break existing pages using dialogs in sandboxed frame. IIUC, only Gecko and Chromium implements that flag for now. What was your experience with that change at Google? I see that the Chromium counters give very low usage for dialogs in sandboxed frame: https://www.chromestatus.com/metrics/feature/timeline/popularity/767

[Bin Lu]

Yes, I think your understanding is right. In terms of the breakage of dialogs used in sandboxed iframe, the usage was quite low and we didn't get any developer complaints about the blocking in Chrome. So I'd guess it's pretty safe for WebKit to do the same blocking, and the web developer can add 'allow-modals' if they really want it. 

Thanks Frederic for working on this.

[Rick Byers]

It looks like there was some discussion here: https://groups.google.com/a/chromium.org/d/msg/blink-dev/wXbgxLu63Fo/YtsqkySmTWcJ

But yeah given that it's been in Chrome now for ages I'd say it's pretty clear that it's web compatible.  I found a couple reports of site breakage when Chrome made the change (eg. https://bugs.chromium.org/p/chromium/issues/detail?id=543485) but it sounds like those the user might actually want to preserve were fixed by adding allow-modals.

+mkwst who drove this effort in Chromium and can probably provide more data on the web compat risk.

[Frédéric Wang (:fredw)]

Created attachment 316282 [details]
Patch

Minimal changes to implement the behavior. Let's see if it breaks any existing tests.

[Build Bot]

Comment on attachment 316282 [details]
Patch

Attachment 316282 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4178135

New failing tests:
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header2.php
fast/frames/sandboxed-iframe-navigation-top-by-name-denied.html
http/tests/security/no-popup-from-sandbox-top.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame.html
http/tests/security/popup-allowed-by-sandbox-when-allowed.html
http/tests/security/xss-DENIED-window-name-alert.html
fast/forms/autofocus-in-sandbox-with-allow-scripts.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header-control.html
http/tests/security/no-popup-from-sandbox.html
http/tests/security/popup-allowed-by-sandbox-is-sandboxed-control.html
fast/frames/sandboxed-iframe-parsing-space-characters.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame2.html
fast/frames/sandboxed-iframe-scripting-02.html
http/tests/security/contentSecurityPolicy/iframe-inside-csp.html
http/tests/security/no-indexeddb-from-sandbox.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header.html
http/tests/security/drag-drop-same-unique-origin.html

[Build Bot]

Created attachment 316283 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316282 [details]
Patch

Attachment 316282 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4178150

New failing tests:
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header2.php
fast/frames/sandboxed-iframe-navigation-top-by-name-denied.html
http/tests/security/no-popup-from-sandbox-top.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame.html
http/tests/security/popup-allowed-by-sandbox-when-allowed.html
http/tests/security/xss-DENIED-window-name-alert.html
fast/forms/autofocus-in-sandbox-with-allow-scripts.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header-control.html
http/tests/security/no-popup-from-sandbox.html
http/tests/security/popup-allowed-by-sandbox-is-sandboxed-control.html
fast/frames/sandboxed-iframe-parsing-space-characters.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame2.html
fast/frames/sandboxed-iframe-scripting-02.html
http/tests/security/contentSecurityPolicy/iframe-inside-csp.html
http/tests/security/no-indexeddb-from-sandbox.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header.html

[Build Bot]

Created attachment 316285 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316282 [details]
Patch

Attachment 316282 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4178167

New failing tests:
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header2.php
fast/frames/sandboxed-iframe-navigation-top-by-name-denied.html
http/tests/security/no-popup-from-sandbox-top.html
imported/w3c/IndexedDB-private-browsing/idbfactory_open.html
http/tests/security/popup-allowed-by-sandbox-when-allowed.html
http/tests/security/xss-DENIED-window-name-alert.html
fast/forms/autofocus-in-sandbox-with-allow-scripts.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header-control.html
http/tests/security/no-popup-from-sandbox.html
http/tests/security/popup-allowed-by-sandbox-is-sandboxed-control.html
fast/frames/sandboxed-iframe-parsing-space-characters.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame2.html
fast/frames/sandboxed-iframe-scripting-02.html
http/tests/security/contentSecurityPolicy/iframe-inside-csp.html
http/tests/security/no-indexeddb-from-sandbox.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame.html

[Build Bot]

Created attachment 316286 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 316282 [details]
Patch

Attachment 316282 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4178191

New failing tests:
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header2.php
fast/frames/sandboxed-iframe-navigation-top-by-name-denied.html
http/tests/security/no-popup-from-sandbox-top.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame.html
http/tests/security/popup-allowed-by-sandbox-when-allowed.html
http/tests/security/xss-DENIED-window-name-alert.html
fast/forms/autofocus-in-sandbox-with-allow-scripts.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header-control.html
http/tests/security/no-popup-from-sandbox.html
http/tests/security/popup-allowed-by-sandbox-is-sandboxed-control.html
fast/frames/sandboxed-iframe-parsing-space-characters.html
fast/events/popup-blocked-from-sandboxed-frame-via-window-open-named-sibling-frame2.html
fast/frames/sandboxed-iframe-scripting-02.html
http/tests/security/contentSecurityPolicy/iframe-inside-csp.html
http/tests/security/no-indexeddb-from-sandbox.html
http/tests/security/contentSecurityPolicy/sandbox-allow-scripts-in-http-header.html
http/tests/security/drag-drop-same-unique-origin.html

[Build Bot]

Created attachment 316287 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Frédéric Wang (:fredw)]

Created attachment 316294 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 316369 [details]
Patch

[Build Bot]

Comment on attachment 316369 [details]
Patch

Attachment 316369 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4185344

New failing tests:
http/tests/security/sandboxed-iframe-ALLOWED-modals.html
fast/frames/sandboxed-iframe-parsing-space-characters.html

[Build Bot]

Created attachment 316377 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316369 [details]
Patch

Attachment 316369 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4185347

New failing tests:
fast/frames/sandboxed-iframe-parsing-space-characters.html

[Build Bot]

Created attachment 316378 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316369 [details]
Patch

Attachment 316369 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4185355

New failing tests:
http/tests/security/sandboxed-iframe-ALLOWED-modals.html
fast/frames/sandboxed-iframe-parsing-space-characters.html

[Build Bot]

Created attachment 316379 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316369 [details]
Patch

Attachment 316369 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4185396

New failing tests:
fast/frames/sandboxed-iframe-parsing-space-characters.html

[Build Bot]

Created attachment 316381 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Frédéric Wang (:fredw)]

Created attachment 316387 [details]
Patch

[Brent Fulgham]

Comment on attachment 316387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316387&action=review

Thank you for implementing this! r=me, but please update the patch to reflect that bit 11 is already used.

> Source/WebCore/dom/SecurityContext.h:54
> +    SandboxModals               = 1 << 11,

Bit 11 is already taken by SandboxDocumentDomain. Can you please change this to 12?

[Frédéric Wang (:fredw)]

Created attachment 319084 [details]
Patch

[Frédéric Wang (:fredw)]

Committed r221193: <http://trac.webkit.org/changeset/221193>

[Radar WebKit Bug Importer]

<rdar://problem/34083930>

[Brent Fulgham]

This was actually:
<rdar://problem/26079109>