Summary: | Add flag allow-modals to iframe sandbox | ||
---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> |
Component: | Frames | Assignee: | Frédéric Wang (:fredw) <fred.wang> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | bfulgham, binlu, buildbot, cdumez, dbates, dpaddock, dvoytenko, esprehn+autocc, fred.wang, jond, kangil.han, malte, mkwst, rbyers, rniwa, webkit-bug-importer |
Priority: | P2 | Keywords: | InRadar |
Version: | Safari Technology Preview | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
URL: | https://html.spec.whatwg.org/multipage/origin.html#sandboxed-modals-flag | ||
Bug Depends on: | |||
Bug Blocks: | 175300, 183568 | ||
Attachments: |
Description
Frédéric Wang (:fredw)
2017-04-26 04:40:58 PDT
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 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. 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. Created attachment 316282 [details]
Patch
Minimal changes to implement the behavior. Let's see if it breaks any existing tests.
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 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
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 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
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 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
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 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
Created attachment 316294 [details]
Patch
Created attachment 316369 [details]
Patch
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 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
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 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
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 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
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 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
Created attachment 316387 [details]
Patch
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? Created attachment 319084 [details]
Patch
Committed r221193: <http://trac.webkit.org/changeset/221193> This was actually: <rdar://problem/26079109> |