Bug 171321

Summary: Add flag allow-modals to iframe sandbox
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: FramesAssignee: 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, malteubl, 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 Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Patch none

Comment 1 Frédéric Wang (:fredw) 2017-07-12 01:23:30 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
Comment 2 Bin Lu 2017-07-12 07:13:59 PDT
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.
Comment 3 Rick Byers 2017-07-18 12:45:32 PDT
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.
Comment 4 Frédéric Wang (:fredw) 2017-07-24 04:23:34 PDT
Created attachment 316282 [details]
Patch

Minimal changes to implement the behavior. Let's see if it breaks any existing tests.
Comment 5 Build Bot 2017-07-24 05:30:54 PDT
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
Comment 6 Build Bot 2017-07-24 05:30:55 PDT
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 7 Build Bot 2017-07-24 05:37:09 PDT
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
Comment 8 Build Bot 2017-07-24 05:37:10 PDT
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 9 Build Bot 2017-07-24 05:56:53 PDT
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
Comment 10 Build Bot 2017-07-24 05:56:55 PDT
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 11 Build Bot 2017-07-24 06:10:22 PDT
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
Comment 12 Build Bot 2017-07-24 06:10:24 PDT
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
Comment 13 Frédéric Wang (:fredw) 2017-07-24 09:11:11 PDT
Created attachment 316294 [details]
Patch
Comment 14 Frédéric Wang (:fredw) 2017-07-25 09:48:32 PDT
Created attachment 316369 [details]
Patch
Comment 15 Build Bot 2017-07-25 10:56:57 PDT
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
Comment 16 Build Bot 2017-07-25 10:56:59 PDT
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 17 Build Bot 2017-07-25 11:02:41 PDT
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
Comment 18 Build Bot 2017-07-25 11:02:43 PDT
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 19 Build Bot 2017-07-25 11:18:01 PDT
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
Comment 20 Build Bot 2017-07-25 11:18:02 PDT
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 21 Build Bot 2017-07-25 11:29:39 PDT
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
Comment 22 Build Bot 2017-07-25 11:29:40 PDT
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
Comment 23 Frédéric Wang (:fredw) 2017-07-25 13:24:59 PDT
Created attachment 316387 [details]
Patch
Comment 24 Brent Fulgham 2017-08-18 09:44:02 PDT
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?
Comment 25 Frédéric Wang (:fredw) 2017-08-25 09:33:47 PDT
Created attachment 319084 [details]
Patch
Comment 26 Frédéric Wang (:fredw) 2017-08-25 10:29:05 PDT
Committed r221193: <http://trac.webkit.org/changeset/221193>
Comment 27 Radar WebKit Bug Importer 2017-08-25 10:29:59 PDT
<rdar://problem/34083930>
Comment 28 Brent Fulgham 2017-08-31 12:41:51 PDT
This was actually:
<rdar://problem/26079109>