Add IPCs to check web copy/cut/paste for extensions with the appropriate permission bit.

Add IPCs to check web copy/cut/paste for extensions with the appropriate permission bit.

BUG=73116
TEST=brower_tests --gtest_filter=ExtensionApiTest.Clipboard

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83990

[rniwa-cr, 2011-02-16 03:41:50]

Thanks for the patch!

http://codereview.chromium.org/6480106/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/6480106/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_host.cc:674: return
extension_->HasApiPermission(Extension::kClipboardPermission);
Is extension_ origin controlled?  i.e. only script context that comes from the
extension's origin can access this function.

[dcheng, 2011-02-16 06:32:24]

On 2011/02/16 03:41:50, rniwa-cr wrote:
> Thanks for the patch!
> 
>
http://codereview.chromium.org/6480106/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_host.cc (right):
> 
>
http://codereview.chromium.org/6480106/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_host.cc:674: return
> extension_->HasApiPermission(Extension::kClipboardPermission);
> Is extension_ origin controlled?  i.e. only script context that comes from the
> extension's origin can access this function.

On 2011/02/16 03:41:50, rniwa-cr wrote:
> Thanks for the patch!
> 
>
http://codereview.chromium.org/6480106/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_host.cc (right):
> 
>
http://codereview.chromium.org/6480106/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_host.cc:674: return
> extension_->HasApiPermission(Extension::kClipboardPermission);
> Is extension_ origin controlled?  i.e. only script context that comes from the
> extension's origin can access this function.

I believe each extension host only hosts pages from one extension. I could be
wrong though.

Personally, I'd also like to make both copy/cut and paste depend on having the
clipboard permission; however, I know aa was against this idea in the past. I
still believe it's the right thing to do though, as the fact that pages could
copy/cut was by accident, not by design.

[Aaron Boodman, 2011-02-16 21:52:34]

Thanks for doing this.

I agree that ideally copy would require a permission, from a consistency point
of view. However, given that we aren't going to show any message to users,
whether there is a permission requirement or not, it doesn't make a huge amount
of difference. And since adding the requirement would break existing extensions,
I'm against it.

A few other things to keep in mind when you move this code:

1. It would also be nice to fix content script access to copy/paste at the same
time. Content scripts from an extension should also be allowed to trigger
copy/paste. We can do this as a separate change, though.

http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
TODO(dcheng): add a permission message.
No, there should not be a permission message. Clipboard access is not a scary
enough permission to ask users about. Having to install the app/extension is
enough.

http://codereview.chromium.org/6480106/diff/2001/chrome/common/render_message...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6480106/diff/2001/chrome/common/render_message...
chrome/common/render_messages_internal.h:2390:
IPC_SYNC_MESSAGE_ROUTED0_1(ViewHostMsg_IsWebCopyCutEnabled,
This looks like a synchronous message to the browser's UI thread. Such messages
are disallowed because they can deadlock the browser.

You can only do synchronous messages to the browser's IO thread.

Extensions are accessible on the IO thread (they are immutable), so you can just
move this code there. I think ChromeURLRequestContext would be the right place.
There is other code that checks extension permissions there, eg for Geolocation.

[Aaron Boodman, 2011-02-16 21:52:56]

On 2011/02/16 21:52:34, Aaron Boodman wrote:
> A few other things to keep in mind when you move this code:

Actually there turned out to just be one :)

[rniwa-cr, 2011-02-17 12:12:54]

On 2011/02/16 21:52:34, Aaron Boodman wrote:
> I agree that ideally copy would require a permission, from a consistency point
> of view. However, given that we aren't going to show any message to users,
> whether there is a permission requirement or not, it doesn't make a huge
amount
> of difference.

We should show a message.

>
http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
> TODO(dcheng): add a permission message.
> No, there should not be a permission message. Clipboard access is not a scary
> enough permission to ask users about. Having to install the app/extension is
> enough.

Clipboard access is quite scary. Extensions / Web apps can pull contents
continuously and obtain sensitive data such as username, password, credit card
number, full name, mail addresses, etc... I don't mind sharing my physical
locations but I do mind sharing my clipboard contents.

[dcheng, 2011-02-18 06:38:32]

> 1. It would also be nice to fix content script access to copy/paste at the
same
> time. Content scripts from an extension should also be allowed to trigger
> copy/paste. We can do this as a separate change, though.

Once content script is injected into a page, isn't running in the same security
context as the rest of the page? Is there any way we can safely distinguish? (I
may not be understanding this correctly)

> Extensions are accessible on the IO thread (they are immutable), so you can
just
> move this code there. I think ChromeURLRequestContext would be the right
place.
> There is other code that checks extension permissions there, eg for
Geolocation.

Looking at how other this is implemented for other permissions, it looks like we
trust the URL passed in from the renderer process. Is this a potential security
issue?

http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
TODO(dcheng): add a permission message.
On 2011/02/16 21:52:34, Aaron Boodman wrote:
> No, there should not be a permission message. Clipboard access is not a scary
> enough permission to ask users about. Having to install the app/extension is
> enough.

What constitutes scary? Normal web pages can't do this after all.

http://codereview.chromium.org/6480106/diff/2001/chrome/common/render_message...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6480106/diff/2001/chrome/common/render_message...
chrome/common/render_messages_internal.h:2390:
IPC_SYNC_MESSAGE_ROUTED0_1(ViewHostMsg_IsWebCopyCutEnabled,
On 2011/02/16 21:52:34, Aaron Boodman wrote:
> This looks like a synchronous message to the browser's UI thread. Such
messages
> are disallowed because they can deadlock the browser.
> 
> You can only do synchronous messages to the browser's IO thread.
> 
> Extensions are accessible on the IO thread (they are immutable), so you can
just
> move this code there. I think ChromeURLRequestContext would be the right
place.
> There is other code that checks extension permissions there, eg for
Geolocation.

I've moved this to RenderMessageFilter. I'm not 100% what I've done is correct,
but I haven't had time to wire up the WebKit side. It compiles at least. I will
test this change before landing the patch of course =)

[Aaron Boodman, 2011-02-22 21:35:42]

On 2011/02/18 06:38:32, dcheng wrote:
> > 1. It would also be nice to fix content script access to copy/paste at the
> same
> > time. Content scripts from an extension should also be allowed to trigger
> > copy/paste. We can do this as a separate change, though.
> 
> Once content script is injected into a page, isn't running in the same
security
> context as the rest of the page? Is there any way we can safely distinguish?
(I
> may not be understanding this correctly)

You can distinguish. Content scripts run in an 'isolated world', which is a
separate v8 context. We do not have any abstractions to represent the concept of
worlds in the WebKit API however. That is why I positioned this as something
that should be done in a separate change.

> > Extensions are accessible on the IO thread (they are immutable), so you can
> just
> > move this code there. I think ChromeURLRequestContext would be the right
> place.
> > There is other code that checks extension permissions there, eg for
> Geolocation.
> 
> Looking at how other this is implemented for other permissions, it looks like
we
> trust the URL passed in from the renderer process. Is this a potential
security
> issue?

It would be better to whitelist processes from the browser side. So far we have
only used this type of permission checking for relatively low privilege things
like geolocation, notifications, etc, which owned renderers could still probably
get access to.

>
http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension.cc (right):
> 
>
http://codereview.chromium.org/6480106/diff/2001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
> TODO(dcheng): add a permission message.
> On 2011/02/16 21:52:34, Aaron Boodman wrote:
> > No, there should not be a permission message. Clipboard access is not a
scary
> > enough permission to ask users about. Having to install the app/extension is
> > enough.
> 
> What constitutes scary? Normal web pages can't do this after all.

It is a judgement call.

Extensions and applications are different from normal web pages. They have an
explicit installation step which prevents random web pages you surf across from
prompting for permission, lowering the value of the privilege. They also
typically come from the store/gallery, where there are reviews, and from where
Google can remove malicious ones.

On the other side, there is the question of how much value a warning would have.
To be useful, a warning must be meaningful to users. How would a user interpret
"access to clipboard"? Do users even know what a clipboard is?

For these reasons, in my mind, clipboard has always been an example of a
privilege that we would not prompt for.

>
http://codereview.chromium.org/6480106/diff/2001/chrome/common/render_message...
> File chrome/common/render_messages_internal.h (right):
> 
>
http://codereview.chromium.org/6480106/diff/2001/chrome/common/render_message...
> chrome/common/render_messages_internal.h:2390:
> IPC_SYNC_MESSAGE_ROUTED0_1(ViewHostMsg_IsWebCopyCutEnabled,
> On 2011/02/16 21:52:34, Aaron Boodman wrote:
> > This looks like a synchronous message to the browser's UI thread. Such
> messages
> > are disallowed because they can deadlock the browser.
> > 
> > You can only do synchronous messages to the browser's IO thread.
> > 
> > Extensions are accessible on the IO thread (they are immutable), so you can
> just
> > move this code there. I think ChromeURLRequestContext would be the right
> place.
> > There is other code that checks extension permissions there, eg for
> Geolocation.
> 
> I've moved this to RenderMessageFilter. I'm not 100% what I've done is
correct,
> but I haven't had time to wire up the WebKit side. It compiles at least. I
will
> test this change before landing the patch of course =)

I haven't reviewed this bit yet; I wanted to reply to your big picture questions
first.

[Aaron Boodman, 2011-02-22 21:49:01]

+mpcomplete

http://codereview.chromium.org/6480106/diff/7001/chrome/browser/notifications...
File chrome/browser/notifications/balloon_host.h (left):

http://codereview.chromium.org/6480106/diff/7001/chrome/browser/notifications...
chrome/browser/notifications/balloon_host.h:92: virtual void StartDragging(const
WebDropData& drop_data,
rebase needed?

http://codereview.chromium.org/6480106/diff/7001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6480106/diff/7001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:852: // TODO(dcheng): is
this right? Will it be NULL if it's not an extension?
No, this is not correct, unfortunately. Extensions can affect other URLs via
their 'extent' feature.

Looks like you need to add a method to ExtensionInfoMap like
GetExtensionInfoForURL() or URLHasExtensionInfo(). Take a look at the
implementation of CheckURLAccessToPermission() to see what I mean.

http://codereview.chromium.org/6480106/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480106/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
TODO(dcheng): add a permission message.
I still maintain the TODO here is not correct.

[rniwa-cr, 2011-02-23 02:06:36]

On 2011/02/22 21:35:42, Aaron Boodman wrote:
> You can distinguish. Content scripts run in an 'isolated world', which is a
> separate v8 context. We do not have any abstractions to represent the concept
of
> worlds in the WebKit API however. That is why I positioned this as something
> that should be done in a separate change.

This is impossible to distinguish from WebKit side.  There is currently no way
for EditorClient to figure out from which world copy/paste is invoked.

> It is a judgement call.
> 
> Extensions and applications are different from normal web pages. They have an
> explicit installation step which prevents random web pages you surf across
from
> prompting for permission, lowering the value of the privilege. They also
> typically come from the store/gallery, where there are reviews, and from where
> Google can remove malicious ones.

But that doesn't no way justify random extension being able to access my
clipboard contents.  Why should I expect a random game app to be able to read
contents of my clipboard?  Also see crbug.com/73478.

> On the other side, there is the question of how much value a warning would
have.
> To be useful, a warning must be meaningful to users. How would a user
interpret
> "access to clipboard"? Do users even know what a clipboard is?

A better example would be "can see and modify what you copy and paste", and
that's a very scary thing.

- Ryosuke

[Aaron Boodman, 2011-02-23 02:21:45]

On 2011/02/23 02:06:36, rniwa-cr wrote:
> On 2011/02/22 21:35:42, Aaron Boodman wrote:
> > You can distinguish. Content scripts run in an 'isolated world', which is a
> > separate v8 context. We do not have any abstractions to represent the
concept
> of
> > worlds in the WebKit API however. That is why I positioned this as something
> > that should be done in a separate change.
> 
> This is impossible to distinguish from WebKit side.  There is currently no way
> for EditorClient to figure out from which world copy/paste is invoked.

I'm not sure what you're saying... At a high level, it is certainly possible. As
in, we have all the necessary pieces of information. I'm sure a lot of plumbing
would have to be done though, as this would be the first time we tried to do it.

Again: this is why I suggested this for future work.

> > It is a judgement call.
> > 
> > Extensions and applications are different from normal web pages. They have
an
> > explicit installation step which prevents random web pages you surf across
> from
> > prompting for permission, lowering the value of the privilege. They also
> > typically come from the store/gallery, where there are reviews, and from
where
> > Google can remove malicious ones.
> 
> But that doesn't no way justify random extension being able to access my
> clipboard contents.  Why should I expect a random game app to be able to read
> contents of my clipboard?  Also see crbug.com/73478.
> 
> > On the other side, there is the question of how much value a warning would
> have.
> > To be useful, a warning must be meaningful to users. How would a user
> interpret
> > "access to clipboard"? Do users even know what a clipboard is?
> 
> A better example would be "can see and modify what you copy and paste", and
> that's a very scary thing.

Hm. I am still not sure such a message is adding value, but I will admit that
read-access to the clipboard is more scary than previous privileges we have
considered enabling without warnings.

Perhaps the privilege should be split into read and write, with only the read
permission having an associated warning.

[dcheng, 2011-02-23 02:33:37]

I haven't uploaded the changes yet, so the comments regarding code changes are
inaccurate.

http://codereview.chromium.org/6480106/diff/7001/chrome/browser/notifications...
File chrome/browser/notifications/balloon_host.h (left):

http://codereview.chromium.org/6480106/diff/7001/chrome/browser/notifications...
chrome/browser/notifications/balloon_host.h:92: virtual void StartDragging(const
WebDropData& drop_data,
On 2011/02/22 21:49:01, Aaron Boodman wrote:
> rebase needed?

The original patch modified lines in this file and I removed the duplicate (and
unnecessary) overload for StartDragging. Since I don't touch this file anymore
except for the cleanup, I've reverted this file.

http://codereview.chromium.org/6480106/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6480106/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
TODO(dcheng): add a permission message.
On 2011/02/22 21:49:01, Aaron Boodman wrote:
> I still maintain the TODO here is not correct.

I don't care strongly enough for this to block the change. That being said, one
could easily make the case that the current permission messages aren't all that
meaningful either--a lot of extensions have explanations in their description
for what they really do when Chrome gives a big scary warning about "this
extension has access to your browsing history" or "this extension has your data
on all websites".

In fact, why even have warnings for things that don't even show up in the
install UI? Whether or not the manifest specifies them is irrelevant, since the
user won't know either way without inspecting the code.

[Aaron Boodman, 2011-02-25 19:29:40]

On 2011/02/23 02:33:37, dcheng wrote:
> I haven't uploaded the changes yet, so the comments regarding code changes are
> inaccurate.

Ok. I may have seen some intermediate changes to the review, before you hit
publish? Sorry, I periodically scan Rietveld to see if there's anything that's
been updated that needs attention. Sometimes I hit things that aren't ready for
review.

>
http://codereview.chromium.org/6480106/diff/7001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension.cc (right):
> 
>
http://codereview.chromium.org/6480106/diff/7001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:225: { kClipboardPermission, 0 },  //
> TODO(dcheng): add a permission message.
> On 2011/02/22 21:49:01, Aaron Boodman wrote:
> > I still maintain the TODO here is not correct.
> 
> I don't care strongly enough for this to block the change. That being said,
one
> could easily make the case that the current permission messages aren't all
that
> meaningful either--a lot of extensions have explanations in their description
> for what they really do when Chrome gives a big scary warning about "this
> extension has access to your browsing history" or "this extension has your
data
> on all websites".

I think the reason developers frequently have those explanations is because
their users *do* understand the meaning of the warnings, and they ask the
developers questions about why the permission is needed. So in that sense the
system is working.

(A separate problem is that the permissions are too coarse, and so developers
have to ask for more privilege than they need, which users call them on. This is
something we work on improving all the time, which is why in a previous mail I
brought up the possibility of separating read and write permission to the
clipboard.)

It's a judgement call as to what is understandable and what isn't. But to me
there is a big difference between "access to your private data on google.com"
and "access to your clipboard".

> In fact, why even have warnings for things that don't even show up in the
> install UI? Whether or not the manifest specifies them is irrelevant, since
the
> user won't know either way without inspecting the code.

I think you mean: "why even have permissions...?". The reason is that the
extension permission system has several goals:

1. Inform users during installation about what an extension will be capable of.
2. Inform us (eg Chrome, Google) about what capabilities are being used.
3. Enforce the principle of least privilege: only allow an extension to have
access to what it needs to get its job done.

Even if we don't display a warning to users, goals 2 and 3 are still very
important. Goal 3 in particular protects users extension written by
well-meaning, but novice developers, whose extensions might be exploited by
malicious web pages.

[Aaron Boodman, 2011-03-03 22:39:29]

I thought about this some and chatted with Erik Kay.

Our conclusion is that:

1) We'd prefer separate permissions (eg clipboard-read and clipboard-write).

2) Neither permission should have a warning for now, but we're kind of on the
fence about clipboard-read. We're going to do some more thinking about that one.

[rniwa-cr, 2011-03-04 00:31:41]

On 2011/03/03 22:39:29, Aaron Boodman wrote:
> 1) We'd prefer separate permissions (eg clipboard-read and clipboard-write).

This makes sense.  WebKit end already provides this distinction although I'd
prefer to call it copy-cut and paste instead.

> 2) Neither permission should have a warning for now, but we're kind of on the
> fence about clipboard-read. We're going to do some more thinking about that
one.

Letting extensions read clipboard contents is a good enough reason for me, as a
user, not to use Chrome.  I'd seriously remove all Chrome installation from my
personal computers if we're going ahead with this change without a warning.  I
almost feel that we're betraying users and disrespecting their privacy if we
don't show a warning.

[Aaron Boodman, 2011-03-04 00:59:46]

On Thu, Mar 3, 2011 at 4:31 PM,  <rniwa@chromium.org> wrote:
> On 2011/03/03 22:39:29, Aaron Boodman wrote:
>>
>> 1) We'd prefer separate permissions (eg clipboard-read and
>> clipboard-write).
>
> This makes sense.  WebKit end already provides this distinction although I'd
> prefer to call it copy-cut and paste instead.

To me, these names are confusing. Does "copy-cut" mean the ability to
"copy" (eg write) things to the clipboard, or does it mean the ability
to read things that were copied or cut onto the clipboard?

Also, "clipboard-read" and "clipboard-write" are better because they
get at the underlying privilege the extension has rather than the
particular feature we're dealing with today.

For example, imagine in the future we added a richer API for
interacting with the clipboard, where you could add arbitrary data, or
read directly from it. In that world permission names like "cut",
"copy", and "paste" wouldn't make as much sense. We'd have to add new
ones, even though the underlying privilege -- ability to see clipboard
data -- is the same.

>> 2) Neither permission should have a warning for now, but we're kind of on
>> the
>> fence about clipboard-read. We're going to do some more thinking about
>> that
>
> one.
>
> Letting extensions read clipboard contents is a good enough reason for me,
> as a
> user, not to use Chrome.  I'd seriously remove all Chrome installation from
> my
> personal computers if we're going ahead with this change without a warning.
>  I
> almost feel that we're betraying users and disrespecting their privacy if we
> don't show a warning.

You could just not install extensions, but I guess you were going for
maximum drama ;-).

In a private email, Erik Kay proposed the following test for whether
permissions should have a warning:

1. An attack using the API would have a high chance of doing something bad
2. The risk of the permission (the warning we display) is
understandable to a large percentage of users

For #1, an extension would have to detect that something copied to the
clipboard was valuable. For passwords, this seems unlikely to succeed.
But there are some examples of sensitive things that are easier, such
as credit cards.

For #2, Erik thought that we could not create an understandable
warning. I agree we won't succeed for "moms", but extension users are
slightly more technical, and I think we can succeed for them.

So how about this new proposal: add a warning for clipboard-read like
"Things copied to your system clipboard".

- a

[rniwa-cr, 2011-03-04 01:14:32]

On 2011/03/04 00:59:46, Aaron Boodman wrote:
> Also, "clipboard-read" and "clipboard-write" are better because they
> get at the underlying privilege the extension has rather than the
> particular feature we're dealing with today.

Okay, fair enough.  I suggested those names because we're adding an ability to
invoke copy/cut/paste because I thought it's not intuitive to ask for
"clipboard-read" permission when I'm going to run document.execCommand("copy") /
document.execCommand("cut"), and "clipboard-write" when I'm going to use
document.execCommand("paste") in my extension.

> You could just not install extensions, but I guess you were going for maximum
drama ;-).
> 
> In a private email, Erik Kay proposed the following test for whether
permissions should have a warning:
> 
> 1. An attack using the API would have a high chance of doing something bad
> 2. The risk of the permission (the warning we display) is understandable to a
large percentage of users
> 
> For #1, an extension would have to detect that something copied to the
clipboard was valuable. For passwords, this seems unlikely to succeed. But there
are some examples of sensitive things that are easier, such as credit cards.

But the extension can send all data to someone, and that person can either
manually read through inputs or use some heuristics to deduce a potential
password.

> So how about this new proposal: add a warning for clipboard-read like "Things
copied to your system clipboard".

I'd very much like that.  I agree with you that we probably don't need an
explicit warning for clipboard-write (although still nice to document it as you
mention).  Safari is considering to let websites copy/cut (i.e. clipboard-write)
for all websites for example.  The argument against clipboard-write privilege
was that websites can inject arbitrary contents into where users paste stuff but
the security implication of that is much lower than clipboard-read.

- Ryosuke

[Aaron Boodman, 2011-03-04 01:19:54]

On 2011/03/04 01:14:32, rniwa-cr wrote:
> On 2011/03/04 00:59:46, Aaron Boodman wrote:
> > Also, "clipboard-read" and "clipboard-write" are better because they
> > get at the underlying privilege the extension has rather than the
> > particular feature we're dealing with today.
> 
> Okay, fair enough.  I suggested those names because we're adding an ability to
> invoke copy/cut/paste because I thought it's not intuitive to ask for
> "clipboard-read" permission when I'm going to run document.execCommand("copy")
/
> document.execCommand("cut"), and "clipboard-write" when I'm going to use
> document.execCommand("paste") in my extension.

The permissions don't correspond 1:1 with the APIs. For example, in order to use
the cookies API, you need host permissions to the origins you are going to
access.

You can find out which permissions you need for an API in the extension
documentation.

[dcheng, 2011-04-15 23:20:24]

Mind taking another look at this patch?

[abarth-chromium, 2011-04-15 23:22:08]

On 2011/04/15 23:20:24, dcheng wrote:
> Mind taking another look at this patch?

Are we concerned about the perf impact of synchronous requests to the browser
for these cases?  (My guess would be no, but I figured I'd ask.)

[rniwa, 2011-04-15 23:27:46]

I don't think copy & paste should be triggered that often.  I mean we could
consider it as a DoS surface but there are many other ways for websites to
do that anyway.

- Ryosuke

On Fri, Apr 15, 2011 at 4:22 PM, <abarth@chromium.org> wrote:

> On 2011/04/15 23:20:24, dcheng wrote:
>
>> Mind taking another look at this patch?
>>
>
> Are we concerned about the perf impact of synchronous requests to the
> browser
> for these cases?  (My guess would be no, but I figured I'd ask.)
>
>
> http://codereview.chromium.org/6480106/
>

[dcheng, 2011-04-15 23:37:31]

On 2011/04/15 23:27:46, rniwa wrote:
> I don't think copy & paste should be triggered that often.  I mean we could
> consider it as a DoS surface but there are many other ways for websites to
> do that anyway.
> 
> - Ryosuke
> 
> On Fri, Apr 15, 2011 at 4:22 PM, <mailto:abarth@chromium.org> wrote:
> 
> > On 2011/04/15 23:20:24, dcheng wrote:
> >
> >> Mind taking another look at this patch?
> >>
> >
> > Are we concerned about the perf impact of synchronous requests to the
> > browser
> > for these cases?  (My guess would be no, but I figured I'd ask.)
> >
> >
> > http://codereview.chromium.org/6480106/
> >

Making it async might make things behave strangely. When a page calls
document.execCommand(copy/cut/paste), I believe the event handler in question
runs in its entirety before the next line executes. I know I've written some
layout tests that expect this behavior, and I wouldn't be surprised if there are
other pages that do the same.

[dcheng, 2011-04-16 01:35:16]

On 2011/04/15 23:37:31, dcheng wrote:
> On 2011/04/15 23:27:46, rniwa wrote:
> > I don't think copy & paste should be triggered that often.  I mean we could
> > consider it as a DoS surface but there are many other ways for websites to
> > do that anyway.
> > 
> > - Ryosuke
> > 
> > On Fri, Apr 15, 2011 at 4:22 PM, <mailto:abarth@chromium.org> wrote:
> > 
> > > On 2011/04/15 23:20:24, dcheng wrote:
> > >
> > >> Mind taking another look at this patch?
> > >>
> > >
> > > Are we concerned about the perf impact of synchronous requests to the
> > > browser
> > > for these cases?  (My guess would be no, but I figured I'd ask.)
> > >
> > >
> > > http://codereview.chromium.org/6480106/
> > >
> 
> Making it async might make things behave strangely. When a page calls
> document.execCommand(copy/cut/paste), I believe the event handler in question
> runs in its entirety before the next line executes. I know I've written some
> layout tests that expect this behavior, and I wouldn't be surprised if there
are
> other pages that do the same.

I also reverted the ExtensionHost change so we don't break the test for the
clipboard extension APIs.

[Aaron Boodman, 2011-04-20 22:07:28]

lgtm

[dcheng, 2011-04-20 23:26:52]

jam, mind taking a look at this patch for OWNERS LGTM? Thanks.

[dcheng, 2011-04-25 19:33:01]

Mind taking one more look? I realized the original change wasn't allowing
clipboard write to all extensions, so I hardcoded a check for extensions in
canTriggerClipboardWrite.

[Aaron Boodman, 2011-04-26 23:36:36]

Still lg. Thanks for fixing that.

[jam, 2011-04-27 00:54:12]

content code doesn't know about extensions, since that's a chrome feature.  so
if this is extension related code, it needs to reside in the chrome layer.

you can add callbacks to ContentRendererClient and have RenderView call to it,
and then the embedder (i.e. ChromeContentRendererClient) can send an ipc
(defined in render_messages.h) which gets dispatched in
ChromeRenderMessageFilter.

http://codereview.chromium.org/6480106/diff/26019/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6480106/diff/26019/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.h:186: 
nit: no need for blank line (I realize there was one there before, if you can
remove both, that'd be great)

http://codereview.chromium.org/6480106/diff/26019/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6480106/diff/26019/content/renderer/render_vie...
content/renderer/render_view.cc:1469: bool allowed;
nit should initialize this to false in case IPC fails

http://codereview.chromium.org/6480106/diff/26019/content/renderer/render_vie...
content/renderer/render_view.cc:1470: RenderThread::current()->Send(
RenderView can send IPCs directly, no need for "RenderThread::current()->"

[dcheng, 2011-05-03 04:33:01]

Many thanks to jam for the WebPermissionsClient changes. I've updated the change
to use ChromeRenderViewObserver.

rniwa, what do you think the correct behavior for using the default value should
be? Currently we ignore it and just do:
  return allowed;
Would it be better to do something like this in ChromeRenderViewObserver
instead?
  return allowed || default_value;

[jam, 2011-05-03 05:50:11]

lgtm

[rniwa-cr, 2011-05-03 16:38:48]

On 2011/05/03 04:33:01, dcheng wrote:
> Would it be better to do something like this in ChromeRenderViewObserver
> instead?
>   return allowed || default_value;

Yes, you're supposed to do that or otherwise you'll break the existing
chrome.experimental.clipboard.