Bug 52417 - Add EditorClient callbacks to override isDOMPasteAllowed and javaScriptCanAccessClipboard
Summary: Add EditorClient callbacks to override isDOMPasteAllowed and javaScriptCanAcc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 59153
  Show dependency treegraph
 
Reported: 2011-01-13 17:51 PST by Ryosuke Niwa
Modified: 2011-04-21 16:19 PDT (History)
10 users (show)

See Also:


Attachments
proof of concept on Mac port (16.16 KB, patch)
2011-01-13 18:12 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (23.79 KB, patch)
2011-02-07 23:07 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per Adam's comment (24.99 KB, patch)
2011-02-14 22:29 PST, Ryosuke Niwa
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-01-13 17:51:07 PST
Right now, the permission to execute copy, cut, paste is dictated by isDOMPasteAllowed and javaScriptCanAccessClipboard, which are called by supportedCopyCut and supportedPaste.  However, this won't allow fine-grained control such as allowing copy, cut, paste for certain origins.

I'd like to make these EditorClient callbacks so that EditorClient can decide whether or not copy, cut, & paste is supported or not per frame.
Comment 1 Ryosuke Niwa 2011-01-13 18:12:28 PST
Created attachment 78881 [details]
proof of concept on Mac port
Comment 2 Ryosuke Niwa 2011-01-13 21:46:43 PST
Comment on attachment 78881 [details]
proof of concept on Mac port

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

> WebKit2/WebProcess/WebPage/WebPage.cpp:1189
> -    settings->setDOMPasteAllowed(store.getBoolValueForKey(WebPreferencesKey::domPasteAllowedKey()));
> +
> +    m_isCopyCutEnabled = store.getBoolValueForKey(WebPreferencesKey::domPasteAllowedKey());

I don't know what's the correct thing to do for WebKit2. Could someone familiar with WebKit2 comment on what I should be doing here?  Enrica?
Comment 3 Alexey Proskuryakov 2011-01-13 23:24:06 PST
> +    m_isCopyCutEnabled = store.getBoolValueForKey(WebPreferencesKey::domPasteAllowedKey());

This looks good WebKit2-wise, but not so good logically. Why does m_isCopyCutEnabled have to do with domPasteAllowed?

These new client calls have extremely generic names that hide their meaning. Compare to javaScriptCanAccessClipboard() and isDOMPasteAllowed().

+    WebPreferences* pref = [m_webView preferences];

No abbr.?

I'm not sure if giving some frames extra privileges is good security architecture. Adam will hopefully tell us if it's not! Perhaps this should be a page group option instead.
Comment 4 Adam Barth 2011-01-14 23:48:18 PST
We discussed this on IRC.  Our current thinking is to add SecurityOrigin::canPaste style functions and use a whitelist.
Comment 5 Ryosuke Niwa 2011-02-07 21:29:11 PST
After another discussion with abarth and othermaciej, I think we're going back to the original plan of adding an method to EditorClient.
Comment 6 Ryosuke Niwa 2011-02-07 23:07:20 PST
Created attachment 81592 [details]
Patch
Comment 7 Ryosuke Niwa 2011-02-14 18:30:04 PST
Hi, could someone review my patch?  This patch or some alternative mechanism to control copy & paste per origin is required for a Chromium feature.
Comment 8 Adam Barth 2011-02-14 18:50:08 PST
Comment on attachment 81592 [details]
Patch

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

> Source/WebCore/editing/EditorCommand.cpp:1129
> +    EditorClient* client = frame->editor()->client();
> +    if (client && client->canCopyCut())
> +        return true;

It's strange that if the client returns false from canCopyCut, the web page might still be able to copy or cut.  That's why I recommended the design we use for allowPlugins where we pass the default as a parameter.  That way its easy for the client to accept the default and also easy for the client to have the final say.
Comment 9 Ryosuke Niwa 2011-02-14 22:29:34 PST
Created attachment 82415 [details]
Fixed per Adam's comment
Comment 10 Adam Barth 2011-02-14 22:38:17 PST
Comment on attachment 82415 [details]
Fixed per Adam's comment

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:256
> +    notImplemented();

These probably don't need notImplemented().  These are perfectly fine implementations.

> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h:78
> +    virtual bool canCopyCut(bool defaultValue) const;
> +    virtual bool canPaste(bool defaultValue) const;

Are these the same semantically as the functions below?  If not, consider renaming them to allowPaste, etc.
Comment 11 Ryosuke Niwa 2011-02-14 22:55:50 PST
Thanks for the review!

(In reply to comment #10)
> (From update of attachment 82415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82415&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:256
> > +    notImplemented();
> 
> These probably don't need notImplemented().  These are perfectly fine implementations.

Ok, will remove.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h:78
> > +    virtual bool canCopyCut(bool defaultValue) const;
> > +    virtual bool canPaste(bool defaultValue) const;
> 
> Are these the same semantically as the functions below?  If not, consider renaming them to allowPaste, etc.

Semantics depends on each implementation because some client might always return true/false to indicate that copy/cut/paste are always possible.  canUndo/canRedo are usually implemented to return true iff we have objects in the undo stack but clients are free to access control undo/redo per origin via these two functions as well.
Comment 12 Ryosuke Niwa 2011-02-15 00:22:03 PST
Committed r78532: <http://trac.webkit.org/changeset/78532>