Bug 194271 – Allow pages to trigger programmatic paste from script on iOS

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

i.e. using document.execCommand('paste');

[Wenson Hsieh]

More context: WKWebView (by default) currently doesn't allow pages to trigger programmatic paste as a user privacy measure, such that untrusted web content can't sniff the contents of the user's pasteboard.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #1)
> More context: WKWebView (by default) …

Of course, this policy extends to Safari as well.

[Radar WebKit Bug Importer]

<rdar://problem/47808810>

[Wenson Hsieh]

Created attachment 361202 [details]
WIP only, no tests yet

[Wenson Hsieh]

Created attachment 361212 [details]
WIP (Fix WPE/GTK/Win builds)

[Wenson Hsieh]

Created attachment 361252 [details]
WIP (with ChangeLog; tests to come)

[Wenson Hsieh]

Created attachment 361305 [details]
Part 1

[Wenson Hsieh]

Created attachment 361306 [details]
Part 2

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #8)
> Created attachment 361306 [details]
> Part 2

I passed --no-ews when uploading this, and yet it somehow applied and is going through EWS anyways. I would expect two of the new tests I added to fail without the actual source changes in Part 1. So I suppose EWS might be useful after all!

[EWS Watchlist]

Comment on attachment 361306 [details]
Part 2

Attachment 361306 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11055495

New failing tests:
editing/pasteboard/ios/programmatic-paste-confirmation.html
editing/pasteboard/ios/programmatic-paste-rejection.html

[EWS Watchlist]

Created attachment 361316 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Wenson Hsieh]

Created attachment 361350 [details]
Part 1

[Wenson Hsieh]

Created attachment 361351 [details]
Part 2

[Ryosuke Niwa]

Comment on attachment 361350 [details]
Part 1

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

> Source/WebCore/dom/Document.cpp:538
> +    , m_programmaticClipboardAccessResetTimer(*this, &Document::programmaticClipboardAccessTimerFired)

Let's not event a new terminology like "programmaticClipboardAccess".
We already have javaScriptCanAccessClipboard / DOMPasteAllowed / clipboardAccessPolicy.

Maybe DOMPasteAccessPolicy?

> Source/WebCore/dom/Document.cpp:5512
> +    if (!m_programmaticClipboardAccessResetTimer.isActive())
> +        m_programmaticClipboardAccessResetTimer.startOneShot(0_s);

This doesn't seem right.
This code would allow any timer or micro tasks scheduled prior to this code to trigger a paste.

> Source/WebCore/editing/EditorCommand.cpp:1245
> +    bool defaultValue = (settings.javaScriptCanAccessClipboard() && settings.DOMPasteAllowed()) || RuntimeEnabledFeatures::sharedFeatures().programmaticClipboardAccessRequestEnabled();

Why isn't this feature simply a setting? It doesn't seem like it needs to be a RuntimeEnabledFeature.

> Source/WebCore/page/Frame.cpp:677
> +    if (m_doc->hasGrantedProgrammaticClipboardAccess())
> +        return true;
> +
> +    if (m_doc->hasDeniedProgrammaticClipboardAccess())
> +        return false;

Instead of having two member functions like this,
why don't we have a function that returns an enum like
DOMPastePolicy {
    Granted,
    Denied,
    Pending,
}

[Wenson Hsieh]

Comment on attachment 361350 [details]
Part 1

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

>> Source/WebCore/dom/Document.cpp:538
>> +    , m_programmaticClipboardAccessResetTimer(*this, &Document::programmaticClipboardAccessTimerFired)
> 
> Let's not event a new terminology like "programmaticClipboardAccess".
> We already have javaScriptCanAccessClipboard / DOMPasteAllowed / clipboardAccessPolicy.
> 
> Maybe DOMPasteAccessPolicy?

Sure, DOMPasteAccessPolicy sounds like a good type name to me.

For the verb form of this (i.e. the methods that I've currently named requestProgrammaticClipboardAccess()), what do you think about requestDOMPasteAccess() instead? Or requestAccessToDOMPaste()?

>> Source/WebCore/dom/Document.cpp:5512
>> +        m_programmaticClipboardAccessResetTimer.startOneShot(0_s);
> 
> This doesn't seem right.
> This code would allow any timer or micro tasks scheduled prior to this code to trigger a paste.

Good point! As we discussed, I'll move this to the user gesture token, such that we won't need a timer.

>> Source/WebCore/editing/EditorCommand.cpp:1245
>> +    bool defaultValue = (settings.javaScriptCanAccessClipboard() && settings.DOMPasteAllowed()) || RuntimeEnabledFeatures::sharedFeatures().programmaticClipboardAccessRequestEnabled();
> 
> Why isn't this feature simply a setting? It doesn't seem like it needs to be a RuntimeEnabledFeature.

Ok! Changed to a setting.

>> Source/WebCore/page/Frame.cpp:677
>> +        return false;
> 
> Instead of having two member functions like this,
> why don't we have a function that returns an enum like
> DOMPastePolicy {
>     Granted,
>     Denied,
>     Pending,
> }

Sure! I was debating between exposing two getters and keeping the enum type private, or exposing a single getter with the enum type. I'll change it to do the latter.

[Wenson Hsieh]

Created attachment 361397 [details]
Part 1

[Wenson Hsieh]

Created attachment 361398 [details]
Part 1 (v2)

[Ryosuke Niwa]

Comment on attachment 361398 [details]
Part 1 (v2)

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

> Source/WebCore/page/Frame.cpp:679
> +    case DOMPasteAccessPolicy::Pending: {

It's odd that this state is called pending because there was no pending request.
How about something like DOMPasteAccessPolicy::NotRequestedYet or DOMPasteAccessPolicy::None?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4866
> +    const CGFloat maximumElementWidth = 300;
> +    const CGFloat maximumElementHeight = 120;
> +    if (elementRect.isEmpty() || elementRect.width() > maximumElementWidth || elementRect.height() > maximumElementHeight) {
> +        const CGFloat interactionLocationMargin = 10;

These values seems somewhat arbitrary. Why are these values picked?
We should probably explain it in a comment or in the change log.

[Ryosuke Niwa]

Comment on attachment 361398 [details]
Part 1 (v2)

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

> Source/WebCore/page/Frame.cpp:673
> +    auto gestureToken = UserGestureIndicator::currentUserGesture();

Maybe you should just check the nullity of this instead of checking UserGestureIndicator::processingUserGesture() earlier.

[Wenson Hsieh]

Comment on attachment 361398 [details]
Part 1 (v2)

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

>> Source/WebCore/page/Frame.cpp:673
>> +    auto gestureToken = UserGestureIndicator::currentUserGesture();
> 
> Maybe you should just check the nullity of this instead of checking UserGestureIndicator::processingUserGesture() earlier.

So UserGestureIndicator::processingUserGesture actually checks a tiny bit more than a null check:

    bool UserGestureIndicator::processingUserGesture()
    {
        if (!isMainThread())
            return false;

        return currentToken() ? currentToken()->processingUserGesture() : false;
    }

I think we can assume we're always on the main thread here (since Much Worse Things™️ would be happening if we were on a background thread while evaluating script that calls execCommand on document). However, there's still the check for processingUserGesture() on the token. I can change this to something like:

    auto gestureToken = UserGestureIndicator::currentUserGesture();
    if (!gestureToken->processingUserGesture())
        return false;

...instead.

>> Source/WebCore/page/Frame.cpp:679
>> +    case DOMPasteAccessPolicy::Pending: {
> 
> It's odd that this state is called pending because there was no pending request.
> How about something like DOMPasteAccessPolicy::NotRequestedYet or DOMPasteAccessPolicy::None?

Sounds good — I quite like DOMPasteAccessPolicy::NotRequestedYet.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4866
>> +        const CGFloat interactionLocationMargin = 10;
> 
> These values seems somewhat arbitrary. Why are these values picked?
> We should probably explain it in a comment or in the change log.

It's true that these are arbitrary values — I picked these after trying this new behavior out on a few relevant websites. The idea is that if the element that the user has interacted with is small enough, the callout looks quite natural if we just use the element rect as the menu's target rect; but if the element is too large, it makes more sense to fall back to using the touch location (with a small amount of margin, so that the callout action isn't obscured by the user's touch).

I'll elaborate on this in the ChangeLog.

[Wenson Hsieh]

>     auto gestureToken = UserGestureIndicator::currentUserGesture();
>     if (!gestureToken->processingUserGesture())
>         return false;

...of course, I also meant to check !gestureToken here as well 😅

[Wenson Hsieh]

Created attachment 361537 [details]
Part 2 (tests & minor tweaks)

[Wenson Hsieh]

Created attachment 361547 [details]
Combined patch

[Wenson Hsieh]

Created attachment 361582 [details]
Combined patch (v2)

[Wenson Hsieh]

Created attachment 361702 [details]
Part 2 (tests)

[Tim Horton]

Comment on attachment 361702 [details]
Part 2 (tests)

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

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:859
> +            if (CFStringCompare((__bridge CFStringRef)buttonTitle, action.get(), 0) != kCFCompareEqualTo)

Why no ObjC

[Wenson Hsieh]

Comment on attachment 361702 [details]
Part 2 (tests)

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

>> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:859
>> +            if (CFStringCompare((__bridge CFStringRef)buttonTitle, action.get(), 0) != kCFCompareEqualTo)
> 
> Why no ObjC

👍 changed to -[NSString isEqualToString:]

[Wenson Hsieh]

Created attachment 361837 [details]
Part 1 (for landing)

[WebKit Commit Bot]

Comment on attachment 361837 [details]
Part 1 (for landing)

Clearing flags on attachment: 361837

Committed r241320: <https://trac.webkit.org/changeset/241320>

[Wenson Hsieh]

Created attachment 361847 [details]
Part 2 (for landing)

[WebKit Commit Bot]

Comment on attachment 361847 [details]
Part 2 (for landing)

Clearing flags on attachment: 361847

Committed r241322: <https://trac.webkit.org/changeset/241322>

[Alex Christensen]

http://trac.webkit.org/r241331