Feedback from potential clients of the Storage Access API pointed out the following problem: If a user grants storage access permission to a third party site, but that third party content does not have its expected local state (e.g., local login cookies or other important state), they will not gain access to their expected data. Without Storage Access API use, they could use the user gesture (generated by clicking in the iframe) to trigger a pop-up window to perform a login. With Storage Access API, the user gesture is consumed, and they lose this ability. We should revise the Storage Access API so that a successful granting of Storage Access API permissions re-enables the UserGestureIndicator state so that the storage access API client can perform a login if they need to. To avoid abuse, we should make this a one-time gesture, so that abusers cannot chain a series of events within the context of the Storage Access API grant.
<rdar://problem/39105791>
Created attachment 340346 [details] WIP Patch
Comment on attachment 340346 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340346&action=review bots seem angry at you. > Source/WebCore/dom/Document.cpp:7623 > + JSC::JSLockHolder lock(vm); I don't think you need this you should have the lock already. > Source/WebCore/dom/Document.cpp:7624 > + vm.drainMicrotasks(); You shouldn't need this line. IIUC, this function should not have any queued micro tasks when it's called. > Source/WebCore/page/DOMWindow.cpp:2385 > + return &newFrame->windowProxy() ; here.
Created attachment 340353 [details] Patch
Comment on attachment 340353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340353&action=review From the JSC side I think this looks good to me. You might want to get someone else to take a look at the UserGestureIndicator code though. It seems fine to me but I'm not an expert in that area. > Source/WebCore/dom/Document.cpp:7625 > + document->vm().drainMicrotasks(); Nit: I would put a comment explaining why we are draining micro tasks here since that's normally pretty weird.
Comment on attachment 340353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340353&action=review > Source/WebCore/page/DOMWindow.cpp:2384 > + UserGestureIndicator::didConsumeOneTimeUserGesture(); This seems odd. Even though it is a one-time user gesture, you are use it as many times as you want, except to open a window? I understand this matches the use-case you have in mind but if I understand this correctly, the OneTimeUserGesture is a lie.
Comment on attachment 340353 [details] Patch Attachment 340353 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7693697 New failing tests: http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
Created attachment 340462 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 340469 [details] Patch
(In reply to Keith Miller from comment #3) > Comment on attachment 340346 [details] > WIP Patch > > Source/WebCore/dom/Document.cpp:7624 > > + vm.drainMicrotasks(); > Unfortunately, testing indicates that the drainMicrotasks idea doesn't work. The Promise resolution doesn't seem to be scheduled at the time we hit this code, so there are no micro tasks to execute, and the UserGestureIndicator falls out of scope before the necessary JavaScript executes. Instead, I've added state to Document to represent that we are in this code path, and added a Task to fire to clear the state if it isn't used in the current call stack.
Created attachment 340518 [details] Patch
Comment on attachment 340518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340518&action=review > Source/WebCore/dom/Document.cpp:7619 > + document->enableOneTimeUserGesture(); This enables the user gesture way too early. A lot of things may get to use the user gesture before the promise is actually resolved. This is because promise::resolve() schedules a MicroTask. I think this should schedule a micro task to enable the user gesture. See comment below for syntax. > Source/WebCore/dom/Document.cpp:7622 > + document->postTask([documentReference = WTFMove(documentReference)] (auto&) { resolve() schedules a MicroTask, not a Task. Therefore, your order of execution here is not necessarily guaranteed and this may well consume the user gesture before we've actually run the JS as a result of resolving the promise. I think this should be a MicrotaskQueue::mainThreadQueue().append(VoidMicrotask([] { // do your thing. }); > Source/WebCore/dom/Document.cpp:7624 > + document->consumeOneTimeUserGesture(); This is truly not a one time user gesture. I would suggest we either make it truly one-time, or we rename it to "temporary" user gesture. There are several ways we could fix this: - Rename to "temporary", do not clear the user gesture on window.open(). Let this micro task clear the user gesture. - Keep calling it one time but then make it truly one time (every user gesture check would clear the user gesture, not just the window.open API). - Call it "OneTimeWindowOpen" and keep clearing it on window.open. > Source/WebCore/page/DOMWindow.cpp:2385 > + document()->consumeOneTimeUserGesture(); Same issue as with the previous patch, this specializes window.open() for some reason. Why would window.open() clear the one-time user gesture but not other operations that require a user gesture? Also note that the user gesture check is in DOMWindow::allowPopUp() I believe. It is weird to do the check in one place but clear the user gesture in another place.
Comment on attachment 340518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340518&action=review > Source/WebCore/dom/Document.h:1432 > + WEBCORE_EXPORT void consumeOneTimeUserGesture(); WEBCORE_EXPORT should not be needed here?
Comment on attachment 340518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340518&action=review >> Source/WebCore/dom/Document.cpp:7619 >> + document->enableOneTimeUserGesture(); > > This enables the user gesture way too early. A lot of things may get to use the user gesture before the promise is actually resolved. This is because promise::resolve() schedules a MicroTask. I think this should schedule a micro task to enable the user gesture. See comment below for syntax. Oh, cool! I'll do that. >> Source/WebCore/dom/Document.cpp:7624 >> + document->consumeOneTimeUserGesture(); > > This is truly not a one time user gesture. I would suggest we either make it truly one-time, or we rename it to "temporary" user gesture. > > There are several ways we could fix this: > - Rename to "temporary", do not clear the user gesture on window.open(). Let this micro task clear the user gesture. > - Keep calling it one time but then make it truly one time (every user gesture check would clear the user gesture, not just the window.open API). > - Call it "OneTimeWindowOpen" and keep clearing it on window.open. In debugging, I found that the test for user interaction happens quite frequently in the code path leading up to navigation or opening a new window. I don't believe that cleaning the state on a test for user interaction will work. I'll try enqueuing a microtask to clear the user interaction. My goal would be: (1) Enqueue a task that adds the temporary user interaction to the Document. (2) Enqueue the promise resolution (3) Enqueue a task to clear the temporary user interaction from the Document. If that works, I won't need to clear it in Window.open. >> Source/WebCore/dom/Document.h:1432 >> + WEBCORE_EXPORT void consumeOneTimeUserGesture(); > > WEBCORE_EXPORT should not be needed here? Yes, right, >> Source/WebCore/page/DOMWindow.cpp:2385 >> + document()->consumeOneTimeUserGesture(); > > Same issue as with the previous patch, this specializes window.open() for some reason. Why would window.open() clear the one-time user gesture but not other operations that require a user gesture? > > Also note that the user gesture check is in DOMWindow::allowPopUp() I believe. It is weird to do the check in one place but clear the user gesture in another place. See my earlier comment. Hopefully I can get rid of this.
Created attachment 340536 [details] Patch
Comment on attachment 340536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340536&action=review r=me > Source/WebKit/ChangeLog:3 > + Allow the WebContent process to read global ViewBridge preferences Bad change log. > LayoutTests/http/tests/storageAccess/request-and-grant-storage-access-cross-origin-non-sandboxed-iframe-pop-window.html:81 > + <iframe onload="runTest()" id="theIframe" src="http://localhost:8000/storageAccess/resources/request-storage-access-iframe-and-pop-window.html#userShouldGrantAccess,userShouldBeConsulted,policyShouldGrantAccess,isNotSameOriginIframe"></iframe> I would have used regular GET (search) parameters rather than hash but OK. Note that search parameters are easy parsable using 'new URLSearchParams(location.search).get("myParam")'. > LayoutTests/http/tests/storageAccess/resources/request-storage-access-iframe-and-pop-window.html:11 > + if (window.internals) { nit: unnecessary curly brackets > LayoutTests/http/tests/storageAccess/resources/request-storage-access-iframe-and-pop-window.html:50 > + var win = window.open("http://localhost:8000/storageAccess/resources/request-storage-access-second-window.html", "test window"); You could have omitted "http://localhost:8000/storageAccess/resources/" I believe since it is same origin as this frame.
Comment on attachment 340536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340536&action=review Thanks for the review! >> Source/WebKit/ChangeLog:3 >> + Allow the WebContent process to read global ViewBridge preferences > > Bad change log. Doh! Fixed. >> LayoutTests/http/tests/storageAccess/resources/request-storage-access-iframe-and-pop-window.html:11 >> + if (window.internals) { > > nit: unnecessary curly brackets Fixed. >> LayoutTests/http/tests/storageAccess/resources/request-storage-access-iframe-and-pop-window.html:50 >> + var win = window.open("http://localhost:8000/storageAccess/resources/request-storage-access-second-window.html", "test window"); > > You could have omitted "http://localhost:8000/storageAccess/resources/" I believe since it is same origin as this frame. That seems to work, so I'll switch to that.
Created attachment 340596 [details] Patch for landing
Comment on attachment 340596 [details] Patch for landing Clearing flags on attachment: 340596 Committed r231910: <https://trac.webkit.org/changeset/231910>
All reviewed patches have been landed. Closing bug.