Bug 185615 – Storage Access API: Allow documents that have been granted storage access to also do a popup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/39105791>

[Brent Fulgham]

Created attachment 340346 [details]
WIP Patch

[Keith Miller]

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.

[Brent Fulgham]

Created attachment 340353 [details]
Patch

[Keith Miller]

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.

[Chris Dumez]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

Created attachment 340469 [details]
Patch

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 340518 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 340536 [details]
Patch

[Chris Dumez]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 340596 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 340596 [details]
Patch for landing

Clearing flags on attachment: 340596

Committed r231910: <https://trac.webkit.org/changeset/231910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.