Ignore paths when matching patterns for extensions.

Ignore paths when matching patterns for extensions.

BUG=94399
TEST=lots


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

[Aaron Boodman, 2011-10-16 03:28:04]

This will break any feature that relies on the path part of URLPatterns. This at
least includes content scripts, but I think there might be others.

I'm surprised this doesn't break any tests.

[dcheng, 2011-10-16 03:58:16]

On 2011/10/16 03:28:04, Aaron Boodman wrote:
> This will break any feature that relies on the path part of URLPatterns. This
at
> least includes content scripts, but I think there might be others.
> 
> I'm surprised this doesn't break any tests.

This was an exploratory change to see what would break. If you have suggestions
on better ways to approach this, I'm always looking for ideas =)

[Aaron Boodman, 2011-10-16 05:02:50]

Oh, I'm sorry, I was perusing Rietveld and didn't notice this was a draft.

I think that the right solution, conceptually, is:

1. Check whether the security origin of the document overlaps with the extent of
the app. This means that the permission will get applied to URLs outside the
pattern, but we can just define that as the behavior.

2. Check whether the frame is running in the process associated with the
relevant app. Apps are typically isolated to a single, protected, process. But
we can end up with frames in the wrong process in some edge cases and we don't
want to grant increased privileges in this case.

Mihai, does this sound right to you?

[dcheng, 2011-10-16 06:11:05]

On 2011/10/16 05:02:50, Aaron Boodman wrote:
> Oh, I'm sorry, I was perusing Rietveld and didn't notice this was a draft.
> 
> I think that the right solution, conceptually, is:
> 
> 1. Check whether the security origin of the document overlaps with the extent
of
> the app. This means that the permission will get applied to URLs outside the
> pattern, but we can just define that as the behavior.
> 
> 2. Check whether the frame is running in the process associated with the
> relevant app. Apps are typically isolated to a single, protected, process. But
> we can end up with frames in the wrong process in some edge cases and we don't
> want to grant increased privileges in this case.
> 
> Mihai, does this sound right to you?

For [1], why not just internally rewrite all <path>s to * for URL patterns in
the permissions of an extension manifest? This will help enforce uniform
handling of different permissions.

[Aaron Boodman, 2011-10-16 06:19:55]

On Sat, Oct 15, 2011 at 11:11 PM,  <dcheng@chromium.org> wrote:
> On 2011/10/16 05:02:50, Aaron Boodman wrote:
>>
>> Oh, I'm sorry, I was perusing Rietveld and didn't notice this was a draft.
>
>> I think that the right solution, conceptually, is:
>
>> 1. Check whether the security origin of the document overlaps with the
>> extent
>
> of
>>
>> the app. This means that the permission will get applied to URLs outside
>> the
>> pattern, but we can just define that as the behavior.
>
>> 2. Check whether the frame is running in the process associated with the
>> relevant app. Apps are typically isolated to a single, protected, process.
>> But
>> we can end up with frames in the wrong process in some edge cases and we
>> don't
>> want to grant increased privileges in this case.
>
>> Mihai, does this sound right to you?
>
> For [1], why not just internally rewrite all <path>s to * for URL patterns
> in
> the permissions of an extension manifest? This will help enforce uniform
> handling of different permissions.

That's an option. The paths were used at one time for a feature, but
aren't anymore. I can't remember if we decided to finally get rid of
them. I'm hoping Mihai does.

- a

[dcheng, 2011-10-16 06:46:56]

Here's an updated patch that attempts that approach.

Known issues I'd like to address if we think this is the right approach:
- Should use an enum instead of bool. Might want to combine with ParseOptions
- Some unit test expectations need to be updated.

[Mihai Parparita -not on Chrome, 2011-10-16 22:35:10]

Rather than adding a boolean param (or even an enum) to URLPattern that is one
value all the time but once, it might be cleaner to just reset the path /* where
we want to ignore it. Also, I thought we already do that:

http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ext...

More generally, Jon has looked at permissions a bunch recently (due the optional
permissions refactorings), so he should look at this CL too.

[dcheng, 2011-10-17 00:23:32]

Well, before I can make any more progress on this patch, there's some
fundamental issues that I'm not sure how to solve. The problem I'm trying to
solve deals primarily with web extents, and using the security origin to
determine which web extent a page belongs to (if any).

The problem is web extents can't overlap; if I relax it from the current
scheme://host/path to just scheme://host (to match what security origin)
provides, it's possible we'll cause previously non-overlapping extents to
overlap and prevent installation of one app--app1 and app2 from the extension
API tests are one example of this.

If I keep the path, and only ignore the path when matching security origins,
then the problem becomes which web extent should be matched... any thoughts?

[jstritar, 2011-10-17 15:52:55]

I'm not sure I understand the problem completely. Is it that webkit security
origins apply to the entire domain, but app web extents don't necessarily span
the domain?

In terms of your changes to URLPattern, I ran into a similar issue just last
week. I think the client should be reseting the URLPattern's path if it only
wants to match origins, rather than changing URLPattern like this, but it's
difficult to keep track of when and if this has happened. What do you all think
of having an OriginPattern that explicitly ignores paths?

[dcheng, 2011-10-17 23:09:59]

On 2011/10/17 15:52:55, jstritar wrote:
> I'm not sure I understand the problem completely. Is it that webkit security
> origins apply to the entire domain, but app web extents don't necessarily span
> the domain?
> 
> In terms of your changes to URLPattern, I ran into a similar issue just last
> week. I think the client should be reseting the URLPattern's path if it only
> wants to match origins, rather than changing URLPattern like this, but it's
> difficult to keep track of when and if this has happened. What do you all
think
> of having an OriginPattern that explicitly ignores paths?

The problem is the following:
Someone writes a hosted web app. They give their app the clipboardWrite
permission. In a page that is inside the hosted app's web extent, they create an
about:blank iframe and attempt to use document.execCommand('copy') in the iframe
to copy some stuff they put in there. Because we use the frame URL
('about:blank') instead of the security origin, we fail to match any web extents
so document.execCommand('copy') fails.

[Aaron Boodman, 2011-10-18 04:24:35]

On 2011/10/17 00:23:32, dcheng wrote:
> Well, before I can make any more progress on this patch, there's some
> fundamental issues that I'm not sure how to solve. The problem I'm trying to
> solve deals primarily with web extents, and using the security origin to
> determine which web extent a page belongs to (if any).
> 
> The problem is web extents can't overlap; if I relax it from the current
> scheme://host/path to just scheme://host (to match what security origin)
> provides, it's possible we'll cause previously non-overlapping extents to
> overlap and prevent installation of one app--app1 and app2 from the extension
> API tests are one example of this.

You only need to know whether a particular document gets the permission, so you
can punt on answering the hard question. If:

1. The security origin of the document overlaps with a web extent that is
granted the permission in question, and
2. The document is running in a process that is running the app in question

Then grant the privilege. Sooner or later the extensions team is going to have
to do something about the fundamental problem here, but I don't think it blocks
your issue.

Find me on IM or IRC tomorrow and we can talk through the details.

[dcheng, 2011-10-19 23:03:14]

On 2011/10/18 04:24:35, Aaron Boodman wrote:
> On 2011/10/17 00:23:32, dcheng wrote:
> > Well, before I can make any more progress on this patch, there's some
> > fundamental issues that I'm not sure how to solve. The problem I'm trying to
> > solve deals primarily with web extents, and using the security origin to
> > determine which web extent a page belongs to (if any).
> > 
> > The problem is web extents can't overlap; if I relax it from the current
> > scheme://host/path to just scheme://host (to match what security origin)
> > provides, it's possible we'll cause previously non-overlapping extents to
> > overlap and prevent installation of one app--app1 and app2 from the
extension
> > API tests are one example of this.
> 
> You only need to know whether a particular document gets the permission, so
you
> can punt on answering the hard question. If:
> 
> 1. The security origin of the document overlaps with a web extent that is
> granted the permission in question, and
> 2. The document is running in a process that is running the app in question
> 
> Then grant the privilege. Sooner or later the extensions team is going to have
> to do something about the fundamental problem here, but I don't think it
blocks
> your issue.
> 
> Find me on IM or IRC tomorrow and we can talk through the details.

I've updated the patch using the ideas you suggested. Several issues:
1) I'm a little uncertain about the organization of changes. Any improvements
here would be welcome.
2) Adding the line to check whether or not extension process bindings are
enabled for that renderer seems to break all the ClipboardApiTests that check
that permissions are appropriately granted. Any idea why?

[Aaron Boodman, 2011-10-20 06:53:50]

Regarding your question about AreBindingsEnabled() or whatever, I might have
misunderstood/misremembered how that code works. It recently changed.

I will look into it and get back to you with a solution.

http://codereview.chromium.org/8312005/diff/13001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/8312005/diff/13001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:605: //
TODO(dcheng): Where is the right place to convert to a URL? Ideally, we'd
I think in general, in Chrome, if we have a URL (or an origin), we should
represent it as a GURL. If the caller (in the renderer, or whatever) has it as a
string for some reason, you should convert it to a GURL early and leave it that
way from then on.

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

http://codereview.chromium.org/8312005/diff/13001/chrome/common/extensions/ex...
chrome/common/extensions/extension_set.h:70: const std::string& origin,
ExtensionAPIPermission::ID permission) const;
GURL again here.

http://codereview.chromium.org/8312005/diff/13001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8312005/diff/13001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:331: // TODO(dcheng): Combine in a
helper with the above?
Yes please. Looks like MatchesURL is just MatchesSecurityOrigin() +
MatchesPath().

http://codereview.chromium.org/8312005/diff/13001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern_set.h (right):

http://codereview.chromium.org/8312005/diff/13001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_set.h:59: bool MatchesSecurityOrigin(const
std::string& origin) const;
Don't forget to add comments before checking this in.

http://codereview.chromium.org/8312005/diff/13001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern_set.h:59: bool MatchesSecurityOrigin(const
std::string& origin) const;
GURL& ?

http://codereview.chromium.org/8312005/diff/13001/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/8312005/diff/13001/chrome/common/render_messag...
chrome/common/render_messages.h:550: std::string /* origin */,
Why string instead of GURL?

[Aaron Boodman, 2011-10-20 06:54:54]

Oh, and regarding code organization: it looks really nice, thanks.

[dcheng, 2011-10-20 18:39:53]

I haven't updated the patch yet, but in a previous version of this patch, abarth
suggested I use string instead of GURL for origins. CCing him here to see if
there's a particular reason to prefer strings.

[Aaron Boodman, 2011-10-20 18:42:02]

On 2011/10/20 18:39:53, dcheng wrote:
> in a previous version of this patch, abarth suggested I use string 
> instead of GURL for origins.

Heh. Classic.

I found the problem with AreBindingsEnabled(). It doesn't mean what I thought it
meant. I thought it meant IsExtensionProcess(). It actually meant
Are[extension]BindingsEnabled() :-).

Anyway. I'm working on a quick fix. I will send it out today and cc you on the
review. Sorry for delaying you.

[Aaron Boodman, 2011-10-24 15:08:52]

On 2011/10/20 18:42:02, Aaron Boodman wrote:
> On 2011/10/20 18:39:53, dcheng wrote:
> > in a previous version of this patch, abarth suggested I use string 
> > instead of GURL for origins.
> 
> Heh. Classic.
> 
> I found the problem with AreBindingsEnabled(). It doesn't mean what I thought
it
> meant. I thought it meant IsExtensionProcess(). It actually meant
> Are[extension]BindingsEnabled() :-).
> 
> Anyway. I'm working on a quick fix. I will send it out today and cc you on the
> review. Sorry for delaying you.

Sorry, it took longer than I expected, but the fix is in:
http://codereview.chromium.org/8361021/

You can now call IsExtensionProcess() on ExtensionInfoMap and it should work.

[dcheng, 2011-10-27 21:09:34]

I've updated the patch to use IsExtensionInProcess. I also ported the
"background" permission to use the security origin as well.

However, it breaks down for the "geolocation" permission, which was the next bit
I tried converting. The geolocation permission checks want to run on the UI
thread, but ExtensionInfoMap is meant for the IO thread.

Some potential solutions:
1) I could add the appropriate goop to make it bounce between the IO thread and
UI thread as needed but at this point it becomes kind of a giant hack for a
problem that will get solved the right way eventually (I hope?).
2) Don't use IsExtensionInProcess as part of the security origin check.
3) Limit the scope of the change to "fixing" the clipboard permissions, which
means they'll behave a little differently from everything else.

None of the alternatives seem great, but [3] seems to be the simplest. Any
thoughts?

[dcheng, 2011-10-27 23:00:21]

All the checks that currently happen on the IO thread use the "new" way now. For
notifications, I'll have to make a followup patch in WebKit to make it use the
security origin.

Permissions that didn't get affected (because they are checked on the UI thread
are manipulated strangely):
geolocation and unlimitedStorage

Let me know what you think. I'm running this past the try bots now to see if
anything explodes.

[dcheng, 2011-10-28 20:20:45]

Oops. Fixed a really silly typo in chrome_content_browser_client. I think this
should pass try jobs now.

[Aaron Boodman, 2011-10-31 23:57:11]

http://codereview.chromium.org/8312005/diff/28002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_info_map.cc (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_info_map.cc:129: const GURL& origin, int
process_id,
How about adding GetByOrigin to ExtensionSet? That would simplify this a little,
and will be useful when we need to do the same thing on the UI thread.

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:311: if (!MatchesScheme(test.scheme()))
Can't testing the scheme and match_all_urls_ also go into
MatchesSecurityOriginHelper?

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:194: bool MatchesSecurityOrigin(const
GURL& test) const;
comment?

[dcheng, 2011-11-01 18:18:02]

http://codereview.chromium.org/8312005/diff/28002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_info_map.cc (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_info_map.cc:129: const GURL& origin, int
process_id,
On 2011/10/31 23:57:11, Aaron Boodman wrote:
> How about adding GetByOrigin to ExtensionSet? That would simplify this a
little,
> and will be useful when we need to do the same thing on the UI thread.

Before I update this, what would GetByOrigin return? A vector of Extension*?

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:311: if (!MatchesScheme(test.scheme()))
On 2011/10/31 23:57:11, Aaron Boodman wrote:
> Can't testing the scheme and match_all_urls_ also go into
> MatchesSecurityOriginHelper?

It won't have quite the same meaning if I move it in there. I would need to
return a trivalue from the helper with the following meanings:
1. True, and no further checks needed.
2. True, but make sure the path matches too.
3. False, doesn't match.

[Aaron Boodman, 2011-11-01 19:20:57]

http://codereview.chromium.org/8312005/diff/28002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_info_map.cc (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_info_map.cc:129: const GURL& origin, int
process_id,
On 2011/11/01 18:18:02, dcheng wrote:
> On 2011/10/31 23:57:11, Aaron Boodman wrote:
> > How about adding GetByOrigin to ExtensionSet? That would simplify this a
> little,
> > and will be useful when we need to do the same thing on the UI thread.
> 
> Before I update this, what would GetByOrigin return? A vector of Extension*?

Yeah, I realized this won't work on the train ride in. Nevermind.

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:311: if (!MatchesScheme(test.scheme()))
Sorry, I still don't see it. Coffee has not kicked in yet. Why doesn't the
following work:

bool MatchesURL(const GURL& test)
  return MatchesSecurityOrigin(test) &&
         MatchesPath(test.PathForRequest())

bool MatchesSecurityOrigin(const GURL& origin) {
  if (!MatchesScheme(test.scheme())
    return false

  if (match_all_urls_)
    return true

  if (scheme_ != chrome::kFileScheme && !MatchesHost(test))
    return false

  if (!MatchesPort(test.EffectiveIntPort())
    return false

  return true

[Aaron Boodman, 2011-11-01 21:50:39]

lgtm (issues resolved offline)

[dcheng, 2011-11-01 22:04:04]

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/8312005/diff/28002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:194: bool MatchesSecurityOrigin(const
GURL& test) const;
On 2011/10/31 23:57:11, Aaron Boodman wrote:
> comment?

Done.

[commit-bot: I haz the power, 2011-11-01 22:04:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8312005/37001

[commit-bot: I haz the power, 2011-11-01 22:04:31]

Presubmit check for 8312005-37001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/common/desktop_notification_messages.h,content/browser/mock_content_browser_client.h,content/browser/renderer_host/render_message_filter.cc,content/browser/renderer_host/render_message_filter.h,content/public/browser/content_browser_client.h,content/browser/mock_content_browser_client.cc,content/renderer/notification_provider.cc,content/renderer/notification_provider.h

Presubmit checks took 2.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[dcheng, 2011-11-01 22:10:25]

jam, do you mind taking a look at this patch for content/* approval? Thanks!

[jam, 2011-11-02 19:04:13]

lgtm

sorry for the delay, please feel free to IM me if I take more than an hour or
two

[commit-bot: I haz the power, 2011-11-02 19:05:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8312005/37001

[commit-bot: I haz the power, 2011-11-02 23:49:05]

Change committed as 108358