[Extensions] Use document url (not top url) for tab-specific permissions

[Extensions] Use document url (not top url) for tab-specific permissions

Fix a bug where activeTab would grant script access to all frames in a tab,
instead of just the top one. Also remove the top_document_url parameter from
CanAccessPage and similar methods.

BUG=495883
Committed: https://crrev.com/f994d1efce89b17cf3443ee5a7750e805b44a0e9
Cr-Commit-Position: refs/heads/master@{#332710}

[Devlin, 2015-06-03 00:14:34]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-06-03 00:14:50]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-06-03 00:15:15]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-06-03 00:15:16]

Die, top url, die!

[not at google - send to devlin, 2015-06-03 00:52:41]

lgtm

[Devlin, 2015-06-03 00:55:36]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-06-03 00:55:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1150683007/#ps40001
(title: "Latest master")

[commit-bot: I haz the power, 2015-06-03 00:57:26]

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

[commit-bot: I haz the power, 2015-06-03 02:08:09]

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

[commit-bot: I haz the power, 2015-06-03 02:08:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2015-06-03 17:45:03]

So, turns out we used that top url for something...

Namely, we used it to prevent content scripts into iframes on other extension
pages (I think we thought this was still fixed somehow, but apparently not).  Of
course, top url in an OOPI world won't work anyway, so we still need a new
solution.  Please take a look at the diff between patch set 2 and 3.

[not at google - send to devlin, 2015-06-03 20:15:44]

kalman@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[not at google - send to devlin, 2015-06-03 20:15:45]

lgtm, the comments are easy enough to address - but you may want to wait for an
opinion from nasko/creis.

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
creis/nasko -

Firstly, this is basically a side channel for sending down the URL of the top
frame. From a site isolation perspective it seems a slight regression, but
knowing the top URL isn't drastically bad, right?

Secondly, if we need to do this, perhaps other people will, and may it make
sense to include it as part of the content API?

Thirdly, Devlin, you could moot these issues by sending the IDs of the user
scripts to execute rather than the data needed to make that decision on the
renderer, but I think you already know that. Perhaps worth noting in a TODO
somewhere.

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.h (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.h:72: std::string
extension_id_;
Seem unnecessary given ProcessManager, plus you haven't used it anywhere.

https://codereview.chromium.org/1150683007/diff/60001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/common/exten...
extensions/common/extension_messages.h:429:
IPC_MESSAGE_ROUTED1(ExtensionMsg_SetTabExtensionOwner,
Btw I think maybe we should own up and call this "SetTopFrameURL" or similar -
"Owner" seems ambiguous to me.

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_injection_host.h (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_injection_host.h:36: blink::WebFrame* web_frame,
Perhaps begin the trend of preferring RenderFrame over WebFrame, since you're
changing call sites anyway? If it's not too much bother.

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:374: LOG(WARNING) << "Trying to
inject!";
oops

[Devlin, 2015-06-03 20:28:47]

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
On 2015/06/03 20:15:44, kalman wrote:
> creis/nasko -
> 
> Firstly, this is basically a side channel for sending down the URL of the top
> frame. From a site isolation perspective it seems a slight regression, but
> knowing the top URL isn't drastically bad, right?
> 
> Secondly, if we need to do this, perhaps other people will, and may it make
> sense to include it as part of the content API?
> 
> Thirdly, Devlin, you could moot these issues by sending the IDs of the user
> scripts to execute rather than the data needed to make that decision on the
> renderer, but I think you already know that. Perhaps worth noting in a TODO
> somewhere.

A bit more background - we prevent script injection into any frame of an
extension page, and previously did that based on web_frame->top->document->url,
but with OOPI, that obviously fails.  I don't know that I agree that this is a
regression - at least, no more so than something like tab id (they're both
tab-wide attributes that cause interesting cases in OOPI).

Re your third point, yes, and we obviously want to work towards that - but
that's a pretty massive change from this, with its own set of fun craziness
(this is one of the things I wanna talk about up in SEA next week).

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.h (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.h:72: std::string
extension_id_;
On 2015/06/03 20:15:44, kalman wrote:
> Seem unnecessary given ProcessManager, plus you haven't used it anywhere.

Totally accidental. Removed.

https://codereview.chromium.org/1150683007/diff/60001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/common/exten...
extensions/common/extension_messages.h:429:
IPC_MESSAGE_ROUTED1(ExtensionMsg_SetTabExtensionOwner,
On 2015/06/03 20:15:45, kalman wrote:
> Btw I think maybe we should own up and call this "SetTopFrameURL" or similar -
> "Owner" seems ambiguous to me.

Waiting to hear back from Nasko or Charlie before renaming.  For the record, I
chose the current name because I wanted it more clear that we're not sending
across the top frame url - we're sending across whether or not this tab is an
extension page, and, if so, which extension owns it.  There's a subtle
difference, but I think it's important from an architectural standpoint.

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_injection_host.h (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_injection_host.h:36: blink::WebFrame* web_frame,
On 2015/06/03 20:15:45, kalman wrote:
> Perhaps begin the trend of preferring RenderFrame over WebFrame, since you're
> changing call sites anyway? If it's not too much bother.

Sure, done.

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:374: LOG(WARNING) << "Trying to
inject!";
On 2015/06/03 20:15:45, kalman wrote:
> oops

Very oops.

[nasko, 2015-06-03 20:31:57]

Voiced an opinion. Also looked around in general and you get a bonus IPC LGTM
:).

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
On 2015/06/03 20:15:44, kalman wrote:
> creis/nasko -
> 
> Firstly, this is basically a side channel for sending down the URL of the top
> frame. 

It isn't obvious to me that you are sending the URL. Can you clarify how that
happens?

> From a site isolation perspective it seems a slight regression, but
> knowing the top URL isn't drastically bad, right?

Ideally we don't want to replicate the URL down to the renderer. Unfortunately,
we have to do it for the top-level frame due to another issue, so alexmos@ is
working on a CL as we speak to do this (writing the test and sending out for
review). 

Looking over the CL, this looks much cleaner (using tab ids instead of URLs). So
even if you had the URL for the top frame, I'd prefer this.

> Secondly, if we need to do this, perhaps other people will, and may it make
> sense to include it as part of the content API?

See above.

> Thirdly, Devlin, you could moot these issues by sending the IDs of the user
> scripts to execute rather than the data needed to make that decision on the
> renderer, but I think you already know that. Perhaps worth noting in a TODO
> somewhere.

[Devlin, 2015-06-03 21:07:43]

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
On 2015/06/03 20:31:56, nasko wrote:
> On 2015/06/03 20:15:44, kalman wrote:
> > creis/nasko -
> > 
> > Firstly, this is basically a side channel for sending down the URL of the
top
> > frame. 
> 
> It isn't obvious to me that you are sending the URL. Can you clarify how that
> happens?

If (and only if) this is an extension page (i.e, a page with a
chrome-extension:// scheme), we send down the id of the extension that "owns"
the page. Technically, this isn't sending the top url (and conceptually, it's
quite a bit cleaner), but it's still in the same neighborhood - which is what
Ben was talking about.
 
> Ideally we don't want to replicate the URL down to the renderer.
Unfortunately,
> we have to do it for the top-level frame due to another issue, so alexmos@ is
> working on a CL as we speak to do this (writing the test and sending out for
> review). 
> 
> Looking over the CL, this looks much cleaner (using tab ids instead of URLs).
So
> even if you had the URL for the top frame, I'd prefer this.

I'm a little confused on the tab id part, because we don't use them here. :)  We
send down the extension id if this is an extension page, and use that to
determine if we need to block script injection based on the top frame -
something we only do for extension pages.

That said, Ben said he's actually happy enough with this (my explanation
helped), so if your LG stands, I'll go ahead and push it through.

[Charlie Reis, 2015-06-03 21:12:04]

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
On 2015/06/03 21:07:42, Devlin wrote:
> On 2015/06/03 20:31:56, nasko wrote:
> > On 2015/06/03 20:15:44, kalman wrote:
> > > creis/nasko -
> > > 
> > > Firstly, this is basically a side channel for sending down the URL of the
> top
> > > frame. 
> > 
> > It isn't obvious to me that you are sending the URL. Can you clarify how
that
> > happens?
> 
> If (and only if) this is an extension page (i.e, a page with a
> chrome-extension:// scheme), we send down the id of the extension that "owns"
> the page. Technically, this isn't sending the top url (and conceptually, it's
> quite a bit cleaner), but it's still in the same neighborhood - which is what
> Ben was talking about.

I don't have the full context here, but we do already replicate the origin
(which contains the extension ID) to each renderer process in
FrameReplicationState.  Would that be sufficient?

>  
> > Ideally we don't want to replicate the URL down to the renderer.
> Unfortunately,
> > we have to do it for the top-level frame due to another issue, so alexmos@
is
> > working on a CL as we speak to do this (writing the test and sending out for
> > review). 
> > 
> > Looking over the CL, this looks much cleaner (using tab ids instead of
URLs).
> So
> > even if you had the URL for the top frame, I'd prefer this.
> 
> I'm a little confused on the tab id part, because we don't use them here. :) 
We
> send down the extension id if this is an extension page, and use that to
> determine if we need to block script injection based on the top frame -
> something we only do for extension pages.
> 
> That said, Ben said he's actually happy enough with this (my explanation
> helped), so if your LG stands, I'll go ahead and push it through.

[Devlin, 2015-06-03 21:23:51]

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
On 2015/06/03 21:12:03, Charlie Reis wrote:
> I don't have the full context here, but we do already replicate the origin
> (which contains the extension ID) to each renderer process in
> FrameReplicationState.  Would that be sufficient?

Now I'm out of context. :)  Looking at FrameReplicationState, I don't think it's
sufficient as-is.  The origin is sufficient, but we need a way of accessing the
main frame's origin from a separate process.  I can't quite follow all of the
happenings, but it looks like FrameReplicationState is for the render frame in
that process, and only accessible at creation?  And, of course, we need access
from the chrome layer, and FrameReplicationState is private to content.

[Charlie Reis, 2015-06-03 21:34:42]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org

[Charlie Reis, 2015-06-03 21:34:43]

[+alexmos]

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
ExtensionMsg_SetTabExtensionOwner(
On 2015/06/03 21:23:51, Devlin wrote:
> On 2015/06/03 21:12:03, Charlie Reis wrote:
> > I don't have the full context here, but we do already replicate the origin
> > (which contains the extension ID) to each renderer process in
> > FrameReplicationState.  Would that be sufficient?
> 
> Now I'm out of context. :)  Looking at FrameReplicationState, I don't think
it's
> sufficient as-is.  The origin is sufficient, but we need a way of accessing
the
> main frame's origin from a separate process.  I can't quite follow all of the
> happenings, but it looks like FrameReplicationState is for the render frame in
> that process, and only accessible at creation?  

No, it exists on RenderFrameProxy as well, so you can get it even for the main
frame.  It's meant for cases like this, and Alex would be a good person to chat
with about it.

> And, of course, we need access
> from the chrome layer, and FrameReplicationState is private to content.

Oh yeah...  We haven't exposed it yet, and RenderFrameProxies aren't exposed
either.  Maybe we should put that idea on hold until your visit next week, and
you can proceed with this in the meantime.

[Devlin, 2015-06-03 21:53:14]

On 2015/06/03 21:34:43, Charlie Reis wrote:
> [+alexmos]
> 
>
https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
> File extensions/browser/extension_web_contents_observer.cc (right):
> 
>
https://codereview.chromium.org/1150683007/diff/60001/extensions/browser/exte...
> extensions/browser/extension_web_contents_observer.cc:156: new_host->Send(new
> ExtensionMsg_SetTabExtensionOwner(
> On 2015/06/03 21:23:51, Devlin wrote:
> > On 2015/06/03 21:12:03, Charlie Reis wrote:
> > > I don't have the full context here, but we do already replicate the origin
> > > (which contains the extension ID) to each renderer process in
> > > FrameReplicationState.  Would that be sufficient?
> > 
> > Now I'm out of context. :)  Looking at FrameReplicationState, I don't think
> it's
> > sufficient as-is.  The origin is sufficient, but we need a way of accessing
> the
> > main frame's origin from a separate process.  I can't quite follow all of
the
> > happenings, but it looks like FrameReplicationState is for the render frame
in
> > that process, and only accessible at creation?  
> 
> No, it exists on RenderFrameProxy as well, so you can get it even for the main
> frame.  It's meant for cases like this, and Alex would be a good person to
chat
> with about it.
> 
> > And, of course, we need access
> > from the chrome layer, and FrameReplicationState is private to content.
> 
> Oh yeah...  We haven't exposed it yet, and RenderFrameProxies aren't exposed
> either.  Maybe we should put that idea on hold until your visit next week, and
> you can proceed with this in the meantime.

SGTM.  Going with this until then. :)

[Devlin, 2015-06-03 21:53:19]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-06-03 21:53:19]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1150683007/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-03 21:53:55]

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

[commit-bot: I haz the power, 2015-06-03 22:28:25]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-06-03 22:29:13]

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

[alexmos, 2015-06-04 20:27:54]

lgtm

Follow-up question.  There's another use of the top frame's URL in
extensions/renderer/script_injection_manager.cc that remains after this CL:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

[alexmos, 2015-06-04 20:28:47]

On 2015/06/04 20:27:54, alexmos wrote:
> lgtm
> 
> Follow-up question.  There's another use of the top frame's URL in
> extensions/renderer/script_injection_manager.cc that remains after this CL:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

Sorry, hit the wrong button before completing my comment.  The question is
whether this dependency can be removed or whether it should use the replicated
URL if the top frame is remote (which I'm working on in another CL).

[Devlin, 2015-06-04 20:29:56]

On 2015/06/04 20:27:54, alexmos wrote:
> lgtm
> 
> Follow-up question.  There's another use of the top frame's URL in
> extensions/renderer/script_injection_manager.cc that remains after this CL:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

Yep, it's on my list. :)  (That one's a pretty easy fix, luckily.)