Implement clipboardRead/Write content capabilities

Implement clipboardRead/Write content capabilities

This adds support for clipboardRead and clipboardWrite
capability grants from white-listed extensions to web
contents.

BUG=409269
R=kalman@chromium.org

Committed: https://crrev.com/e261b16ec7149db6debc70a47f93b20abc8de334
Cr-Commit-Position: refs/heads/master@{#308022}

[Ken Rockot(use gerrit already), 2014-12-10 02:00:31]

Here's a first pass at implementation. Saving unlimitedStorage for a future CL.

[not at google - send to devlin, 2014-12-10 17:33:55]

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/content_capabilities_browsertest.cc (right):

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:73: list = "\"" +
list + "\"";
This is borderline too nitty for me, but I'll say it anyway - a JSON list would
be ["a", "b", "c"] not "a", "b", "c". So consider doing that, then not wrapping
it in the [] as part of LoadExtensionWithCapabilities.

Also, subtle bug which exists right now is that MakeJSONList() will return an
empty string, which would end up above having a singleton [""] list above,
rather than the empty list. The best way to fix this, I think, would be to make
the change I suggest here.

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:107:
MakeJSONList("clipboardRead"));
Is it worth testing an empty set of permissions?

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:155:
EXPECT_TRUE(result);
You should be able to cut out a bunch of this boilerplate by writing a function
that returns a testing::AssertionResult, like,

testing::AssertionResult TestClipboardAccess(
    const Extension* extension, const char* origin,
    bool can_read, bool can_write) {
  ui_test_utils::NavigateToURL(...);
  bool result = false;
  CHECK(content::ExecuteScriptAndExtractBool(...));
  if (result != can_read)
    return testing::AssertionFailure() << ...;
  ...
  return testing::AssertionSuccess();
}

then

InitializeTestServer();
scoped_refptr<const Extension> extension = ...;
EXPECT_TRUE(TestClipboardAccess(...));

https://codereview.chromium.org/789063002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/content_capabilities/bar.example.com.html
(right):

https://codereview.chromium.org/789063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/content_capabilities/bar.example.com.html:6: <script
src="capability_tests.js"></script>
You may want to upload with more aggressive copy detection, or just disable it
altogether.

https://codereview.chromium.org/789063002/diff/20001/extensions/common/manife...
File extensions/common/manifest_handlers/content_capabilities_handler.cc
(right):

https://codereview.chromium.org/789063002/diff/20001/extensions/common/manife...
extensions/common/manifest_handlers/content_capabilities_handler.cc:69:
supported_schemes |= URLPattern::SCHEME_HTTP;
Mention why (... and shame there is no https mode for the embedded test
server..?)

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
extensions/renderer/script_context.cc:279: content_capabilities_ = permissions;
Can you just make ScriptContext (lazily, I suppose) create content_capabilities_
internally, rather than requiring proxying through Dispatcher?

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
extensions/renderer/script_context.cc:285: return true;
Consider being a little conservative here, and checking that it's actually a web
context type as well.

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
extensions/renderer/script_context.h:161: bool
HasEffectiveAPIPermission(APIPermission::ID permission) const;
What is "Effective" about this?

Ideally we'd call this HasAPIPermission and do all permission checks here (both
for the Extension, if it exists, and the content capabilities if this exists).

In fact, what would it take to include the content capability check in
IsAnyFeatureAvailableToContext? (probably too much; terribly specific function).

In lieu of all that, since the implementation only checks content capabilities,
so I'd call this HasContentCapability.

[Ken Rockot(use gerrit already), 2014-12-10 18:31:41]

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/content_capabilities_browsertest.cc (right):

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:73: list = "\"" +
list + "\"";
On 2014/12/10 17:33:54, kalman wrote:
> This is borderline too nitty for me, but I'll say it anyway - a JSON list
would
> be ["a", "b", "c"] not "a", "b", "c". So consider doing that, then not
wrapping
> it in the [] as part of LoadExtensionWithCapabilities.
> 
> Also, subtle bug which exists right now is that MakeJSONList() will return an
> empty string, which would end up above having a singleton [""] list above,
> rather than the empty list. The best way to fix this, I think, would be to
make
> the change I suggest here.

Actually an empty string will lead to the list [] (consider:
StringPrintf("[%s]", ""))

But I agree it's semantically stupid that MakeJSONList doesn't return a JSON
list, so I've changed it anyway.

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:107:
MakeJSONList("clipboardRead"));
On 2014/12/10 17:33:54, kalman wrote:
> Is it worth testing an empty set of permissions?

Sure.

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:155:
EXPECT_TRUE(result);
On 2014/12/10 17:33:54, kalman wrote:
> You should be able to cut out a bunch of this boilerplate by writing a
function
> that returns a testing::AssertionResult, like,
> 
> testing::AssertionResult TestClipboardAccess(
>     const Extension* extension, const char* origin,
>     bool can_read, bool can_write) {
>   ui_test_utils::NavigateToURL(...);
>   bool result = false;
>   CHECK(content::ExecuteScriptAndExtractBool(...));
>   if (result != can_read)
>     return testing::AssertionFailure() << ...;
>   ...
>   return testing::AssertionSuccess();
> }
> 
> then
> 
> InitializeTestServer();
> scoped_refptr<const Extension> extension = ...;
> EXPECT_TRUE(TestClipboardAccess(...));

Good call, though I don't really see the value in using TestAssertion over
bools. The result should still be meaningful test failure output (i.e. line
number, excerpt including the name e.g. "CanReadClipboard"), shouldn't it?

https://codereview.chromium.org/789063002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/content_capabilities/bar.example.com.html
(right):

https://codereview.chromium.org/789063002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/content_capabilities/bar.example.com.html:6: <script
src="capability_tests.js"></script>
On 2014/12/10 17:33:54, kalman wrote:
> You may want to upload with more aggressive copy detection, or just disable it
> altogether.

Done.

https://codereview.chromium.org/789063002/diff/20001/extensions/common/manife...
File extensions/common/manifest_handlers/content_capabilities_handler.cc
(right):

https://codereview.chromium.org/789063002/diff/20001/extensions/common/manife...
extensions/common/manifest_handlers/content_capabilities_handler.cc:69:
supported_schemes |= URLPattern::SCHEME_HTTP;
On 2014/12/10 17:33:54, kalman wrote:
> Mention why (... and shame there is no https mode for the embedded test
> server..?)

Heh. Done.

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
extensions/renderer/script_context.cc:279: content_capabilities_ = permissions;
On 2014/12/10 17:33:54, kalman wrote:
> Can you just make ScriptContext (lazily, I suppose) create
content_capabilities_
> internally, rather than requiring proxying through Dispatcher?

What about handling the case where an extension (which grants
content_capabilities) is unloaded? That was the deciding motivation for doing
this through Dispatcher::UpdateBindings.

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/789063002/diff/20001/extensions/renderer/scri...
extensions/renderer/script_context.h:161: bool
HasEffectiveAPIPermission(APIPermission::ID permission) const;
On 2014/12/10 17:33:54, kalman wrote:
> What is "Effective" about this?
> 
> Ideally we'd call this HasAPIPermission and do all permission checks here
(both
> for the Extension, if it exists, and the content capabilities if this exists).
> 
> In fact, what would it take to include the content capability check in
> IsAnyFeatureAvailableToContext? (probably too much; terribly specific
function).
> 
> In lieu of all that, since the implementation only checks content
capabilities,
> so I'd call this HasContentCapability.

Heh, that's what it was and then I changed it mostly to preserve the original
terminology of the code which consumes it (i.e. essentially
effective_extension->has_permission)

But really I like this better, so SGTM and Done.

[Ken Rockot(use gerrit already), 2014-12-10 18:31:55]

(I actually ended up failing at copy detection again, but w/e)

[not at google - send to devlin, 2014-12-10 19:30:07]

lgtm

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/content_capabilities_browsertest.cc (right):

https://codereview.chromium.org/789063002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/content_capabilities_browsertest.cc:155:
EXPECT_TRUE(result);
On 2014/12/10 18:31:41, Ken Rockot wrote:
> On 2014/12/10 17:33:54, kalman wrote:
> > You should be able to cut out a bunch of this boilerplate by writing a
> function
> > that returns a testing::AssertionResult, like,
> > 
> > testing::AssertionResult TestClipboardAccess(
> >     const Extension* extension, const char* origin,
> >     bool can_read, bool can_write) {
> >   ui_test_utils::NavigateToURL(...);
> >   bool result = false;
> >   CHECK(content::ExecuteScriptAndExtractBool(...));
> >   if (result != can_read)
> >     return testing::AssertionFailure() << ...;
> >   ...
> >   return testing::AssertionSuccess();
> > }
> > 
> > then
> > 
> > InitializeTestServer();
> > scoped_refptr<const Extension> extension = ...;
> > EXPECT_TRUE(TestClipboardAccess(...));
> 
> Good call, though I don't really see the value in using TestAssertion over
> bools. The result should still be meaningful test failure output (i.e. line
> number, excerpt including the name e.g. "CanReadClipboard"), shouldn't it?

The value in testing::AssertionResult is that it maintains stack trace, and it
preserves any nested << you put inside that helper function.

It actually doesn't make a difference the way you have it now, but if you were
to go with the hybrid method that I wrote up above, take for example:

Say that clipboardRead works but clipboardWrite doesn't - it always return true,
or maybe it always returns false. In the always-returns-true case, the
ClipboardReadWrite test would fail - "expected true, got false". That's not very
good. What you want to see it "expected success", with the stack trace, and if
you streamed out "incorrect value for write" then you'd see that too.

This is super useful when you have more complex helper methods. Smaller methods
less so, but I just think that in the spirit of using the same patterns
everywhere, helper test methods should always return testing::AssertionResults.

Also, I preferred that hybrid way :) but NBD.

https://codereview.chromium.org/789063002/diff/40001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/789063002/diff/40001/extensions/renderer/scri...
extensions/renderer/script_context.cc:284: return false;
You might add a comment like,

Only web page contexts get granted content capabilities. Any other contexts
either have Extensions (and we use that extension's permissions instead) or
WebUI (which has its own brand of superpowers).

at least, that's the comment I'd write, because that's how I write. You probably
write differently.

[Ken Rockot(use gerrit already), 2014-12-10 20:40:25]

> The value in testing::AssertionResult is that it maintains stack trace, and it
> preserves any nested << you put inside that helper function.
> 
> It actually doesn't make a difference the way you have it now, but if you were
> to go with the hybrid method that I wrote up above, take for example:
> 
> Say that clipboardRead works but clipboardWrite doesn't - it always return
true,
> or maybe it always returns false. In the always-returns-true case, the
> ClipboardReadWrite test would fail - "expected true, got false". That's not
very
> good. What you want to see it "expected success", with the stack trace, and if
> you streamed out "incorrect value for write" then you'd see that too.
> 
> This is super useful when you have more complex helper methods. Smaller
methods
> less so, but I just think that in the spirit of using the same patterns
> everywhere, helper test methods should always return
testing::AssertionResults.
> 
> Also, I preferred that hybrid way :) but NBD.
> 

I see. Given the simplicity of these methods for now I'm going to leave it
as-is. I feel pretty awkward asserting that a value of type "AssertionResult" is
either "TRUE" or "FALSE".

https://codereview.chromium.org/789063002/diff/40001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/789063002/diff/40001/extensions/renderer/scri...
extensions/renderer/script_context.cc:284: return false;
On 2014/12/10 19:30:07, kalman wrote:
> You might add a comment like,
> 
> Only web page contexts get granted content capabilities. Any other contexts
> either have Extensions (and we use that extension's permissions instead) or
> WebUI (which has its own brand of superpowers).
> 
> at least, that's the comment I'd write, because that's how I write. You
probably
> write differently.

Done.

[Ken Rockot(use gerrit already), 2014-12-10 20:41:38]

rockot@chromium.org changed reviewers:
+ sky@chromium.org

[Ken Rockot(use gerrit already), 2014-12-10 20:41:39]

+sky: could you please take a look at
chrome/renderer/content_settings_observer.cc?

[not at google - send to devlin, 2014-12-10 20:52:53]

> 
> I see. Given the simplicity of these methods for now I'm going to leave it
> as-is. I feel pretty awkward asserting that a value of type "AssertionResult"
is
> either "TRUE" or "FALSE".

I agree, it's awkward, but that's the way it is :(

[sky, 2014-12-10 22:05:31]

https://codereview.chromium.org/789063002/diff/60001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789063002/diff/60001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:418: allowed =
calling_context->HasContentCapability(
I'm not familiar enough with extension code to know if this write, and I get
nervous with security related changes. Is there another OWNER you can find that
knows this code?

[not at google - send to devlin, 2014-12-10 22:11:34]

https://codereview.chromium.org/789063002/diff/60001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789063002/diff/60001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:418: allowed =
calling_context->HasContentCapability(
On 2014/12/10 22:05:31, sky wrote:
> I'm not familiar enough with extension code to know if this write, and I get
> nervous with security related changes. Is there another OWNER you can find
that
> knows this code?

I'm not sure the code is actually right, you've lost checking the Extension's
permissions? Am I misreading?

https://codereview.chromium.org/789063002/diff/60001/extensions/renderer/scri...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/789063002/diff/60001/extensions/renderer/scri...
extensions/renderer/script_context.cc:287: return false;
I would prefer if you surround this comment+body in curlies. It's many lines.

[Ken Rockot(use gerrit already), 2014-12-10 22:12:05]

rockot@chromium.org changed reviewers:
+ thestig@chromium.org

[Ken Rockot(use gerrit already), 2014-12-10 22:14:15]

On 2014/12/10 22:11:34, kalman wrote:
>
https://codereview.chromium.org/789063002/diff/60001/chrome/renderer/content_...
> File chrome/renderer/content_settings_observer.cc (right):
> 
>
https://codereview.chromium.org/789063002/diff/60001/chrome/renderer/content_...
> chrome/renderer/content_settings_observer.cc:418: allowed =
> calling_context->HasContentCapability(
> On 2014/12/10 22:05:31, sky wrote:
> > I'm not familiar enough with extension code to know if this write, and I get
> > nervous with security related changes. Is there another OWNER you can find
> that
> > knows this code?
> 
> I'm not sure the code is actually right, you've lost checking the Extension's
> permissions? Am I misreading?
> 

It is right. The extension's permission checking was rolled into the
implementation in ScriptContext.

That's where the original "HasEffectiveAPIPermission" name came from.

[Ken Rockot(use gerrit already), 2014-12-10 22:15:49]

+thestig for //chrome perhaps?

[Lei Zhang, 2014-12-10 23:01:02]

On 2014/12/10 22:15:49, Ken Rockot wrote:
> +thestig for //chrome perhaps?

Can you and kalman come to an agreement on the correctness of
HasContentCapability()? This is new for me too. Otherwise no issues with chrome/

[Ken Rockot(use gerrit already), 2014-12-10 23:20:42]

On 2014/12/10 23:01:02, Lei Zhang wrote:
> On 2014/12/10 22:15:49, Ken Rockot wrote:
> > +thestig for //chrome perhaps?
> 
> Can you and kalman come to an agreement on the correctness of
> HasContentCapability()? This is new for me too. Otherwise no issues with
chrome/

Ben, how about now?

[not at google - send to devlin, 2014-12-10 23:22:56]

lgtm

[Lei Zhang, 2014-12-10 23:42:40]

chrome/ lgtm

[sky, 2014-12-11 00:51:16]

sky@chromium.org changed reviewers:
- sky@chromium.org

[Ken Rockot(use gerrit already), 2014-12-11 01:39:39]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-12-11 01:40:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789063002/120001

[Ken Rockot(use gerrit already), 2014-12-11 01:44:02]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-12-11 02:54:19]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-12-11 02:54:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789063002/140001

[commit-bot: I haz the power, 2014-12-11 05:26:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 05:26:24]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2014-12-11 22:23:45]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:59:56]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-12 02:00:49]

Patchset 9 (id:??) landed as
https://crrev.com/e261b16ec7149db6debc70a47f93b20abc8de334
Cr-Commit-Position: refs/heads/master@{#308022}