Consider the origin when computing extension permissions

Consider the origin when computing extension permissions

This patch teaches the extension system to use the document's origin when
computing extension permissions. Ideally, we'd use only the document's origin,
but because app extents don't cover entire origins, we need to also consider
the document's URL.

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

[abarth-chromium, 2011-11-28 20:19:26]

Thoughts on the approach?  I need to write some integration tests too.

[abarth-chromium, 2011-11-29 01:44:46]

@aa: Thoughts on this CL?

[aa, 2011-11-29 05:44:44]

Looks complicated and subtle. I'm going to come back to it after I get
the easier reviews knocked out. Ping me if I forget.

On Mon, Nov 28, 2011 at 5:44 PM,  <abarth@chromium.org> wrote:
> @aa: Thoughts on this CL?
>
> http://codereview.chromium.org/8659009/
>

[Aaron Boodman, 2011-11-30 01:52:00]

So what are the cases where origin.ToString() !=
document.url().GetOrigin().ToString()? Are there any others than the unique
origin thing?

[abarth-chromium, 2011-11-30 01:57:33]

On 2011/11/30 01:52:00, Aaron Boodman wrote:
> So what are the cases where origin.ToString() !=
> document.url().GetOrigin().ToString()? Are there any others than the unique
> origin thing?

about:blank
blob URLs
filesystem URLs

about:blank inherits its origin based on some complicated rules.

blob and filesystem are more complicated URLs that have an "inner" URL that
contains the security context.  EricU is working on teaching GURL about them so
that url.GetOrigin() will return the right thing in those cases.

You're barking up the right tree though.  This patch isn't a complete solution
to this problem.  The complete solution is to look up the extensions by origin
and then match the extents against the URL.  This patch is a shorter-term,
lower-risk fix for the specific issue of unique origins (e.g., from sandboxing).

[Aaron Boodman, 2011-11-30 02:01:31]

OK, not as bad as I initially read it.

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.cc (right):

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.cc:37: return origin.isUnique() ? "" :
url.host();
What will the host() be in the case of a unique origin. Seems like it would
already be the empty string.

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.cc:53: // In the common case, the
document's origin will coorespond to its URL,
typo: coorespond

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:49: std::string
GetIdByURL(WebKit::WebSecurityOrigin origin,
What about defining an ExtensionURLInfo struct or something to encapsulate this
weirdness? That would also provide a convenient place to explain why we need it.

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:60: const Extension*
GetByURLWithoutSecurityCheck(const GURL& url) const;
ExtensionURLInfo could have a constructor that just takes a URL and has a
warning.

[Aaron Boodman, 2011-11-30 02:04:45]

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:49: std::string
GetIdByURL(WebKit::WebSecurityOrigin origin,
On 2011/11/30 02:01:31, Aaron Boodman wrote:
> What about defining an ExtensionURLInfo struct or something to encapsulate
this
> weirdness? That would also provide a convenient place to explain why we need
it.

And would at least ensure people think twice when looking up extensions by URL.

[abarth-chromium, 2011-11-30 02:04:49]

Thanks for the review.

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.cc (right):

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.cc:37: return origin.isUnique() ? "" :
url.host();
On 2011/11/30 02:01:31, Aaron Boodman wrote:
> What will the host() be in the case of a unique origin. Seems like it would
> already be the empty string.

In the case of iframe sandbox, it can actually be anything.  You can take
whatever URL you like and render it in a sandboxed iframe.  The iframe will then
have a unique origin, but it will remember its normal URL.

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:49: std::string
GetIdByURL(WebKit::WebSecurityOrigin origin,
On 2011/11/30 02:01:31, Aaron Boodman wrote:
> What about defining an ExtensionURLInfo struct or something to encapsulate
this
> weirdness? That would also provide a convenient place to explain why we need
it.

Will do.

[aa, 2011-11-30 02:10:17]

On Tue, Nov 29, 2011 at 6:04 PM,  <abarth@chromium.org> wrote:
> Thanks for the review.
>
>
>
http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
> File chrome/common/extensions/extension_set.cc (right):
>
>
http://codereview.chromium.org/8659009/diff/4004/chrome/common/extensions/ext...
> chrome/common/extensions/extension_set.cc:37: return origin.isUnique() ?
> "" : url.host();
> On 2011/11/30 02:01:31, Aaron Boodman wrote:
>>
>> What will the host() be in the case of a unique origin. Seems like it
>
> would
>>
>> already be the empty string.
>
> In the case of iframe sandbox, it can actually be anything.  You can
> take whatever URL you like and render it in a sandboxed iframe.  The
> iframe will then have a unique origin, but it will remember its normal
> URL.

I was afraid of that.

- a

[abarth-chromium, 2011-11-30 22:44:58]

Patch updated with fancy ExtensionURLInfo object.

[Aaron Boodman, 2011-11-30 23:32:18]

LGTM

http://codereview.chromium.org/8659009/diff/15001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/8659009/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension_set.h:71: std::string GetIdByInfo(const
ExtensionURLInfo& info) const;
sorry to nit, but can you name it either GetIdByURL or GetIdByURLInfo ?

[abarth-chromium, 2011-11-30 23:34:45]

> sorry to nit, but can you name it either GetIdByURL or GetIdByURLInfo ?

Sure.  I'll rename it back to GetIdByURL

[commit-bot: I haz the power, 2011-12-02 07:23:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/8659009/22001