Support chrome-extension:// scheme in URLPattern.

Support chrome-extension:// scheme in URLPattern.

This CL is mostly motivated to allow users of the web request API to filter network traffic to the extension. The URLParser was configured to broadly before in that is allowed parsing URLPattern::SCHEME_ALL.

BUG=105656
TEST=no


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

[battre, 2011-12-05 14:23:07]

aa: main reviewer because you expressed interest in this in url_pattern.cc
security: for request to review this in url_constants.cc
jam: for content/public/OWNERS
mpcomplete: FYI

I have added unit tests. I tested this feature manually for a web request API
extension but did not add a new browser test. I think the coverage by the unit
tests is sufficient and the cost of an additional browser test is not justified.

http://codereview.chromium.org/8800006/diff/1/chrome/common/extensions/docs/m...
File chrome/common/extensions/docs/match_patterns.html (right):

http://codereview.chromium.org/8800006/diff/1/chrome/common/extensions/docs/m...
chrome/common/extensions/docs/match_patterns.html:348:
<code>chrome-extension</code>),
There are other schemes that were permitted but not documented here before.
Assuming that the lack of documentation was intentional, I have not added them
here.

[jam, 2011-12-05 17:48:24]

http://codereview.chromium.org/8800006/diff/1/content/public/common/url_const...
File content/public/common/url_constants.h (right):

http://codereview.chromium.org/8800006/diff/1/content/public/common/url_const...
content/public/common/url_constants.h:27: CONTENT_EXPORT extern const char
kChromeExtensionScheme[];
this doesn't belong in content since extensions is chrome related. please see
the url_constants file in chrome/common which already has this under
kExtensionScheme

[battre, 2011-12-05 18:36:43]

Thanks, John. Moving you to CC.

http://codereview.chromium.org/8800006/diff/1/content/public/common/url_const...
File content/public/common/url_constants.h (right):

http://codereview.chromium.org/8800006/diff/1/content/public/common/url_const...
content/public/common/url_constants.h:27: CONTENT_EXPORT extern const char
kChromeExtensionScheme[];
On 2011/12/05 17:48:28, John Abd-El-Malek wrote:
> this doesn't belong in content since extensions is chrome related. please see
> the url_constants file in chrome/common which already has this under
> kExtensionScheme

Done.

[Matt Perry, 2011-12-05 20:26:50]

We should be careful to only allow this for the webRequest API. We don't want to
accidentally let chrome-extension schemes to be specified for host permissions,
for example.

[palmer, 2011-12-05 21:33:58]

If I am installing extension B, and it wants permission to intercept traffic for
extension A (which I already have), will I be notified of that?

What about if I do not yet have A installed, but I install it later?

Sorry if these questions are naïve; I am very new to Chrome extensions.

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/webRequest.html (right):

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
chrome/common/extensions/docs/webRequest.html:672:
<code>*://www.google.com/foo*bar</code>, <code>chrome-extension://*/*</code>.
Is "chrome-extension://*/*" actually legal, though? Do you have to register to
handle a specific extension ID, or can you really register to handle all
extensions?

If you always need to give an ID, and I think? you should, document it that way
(as you do in the CL comment).

[Matt Perry, 2011-12-05 22:49:31]

On 2011/12/05 21:33:58, Chris P. wrote:
> If I am installing extension B, and it wants permission to intercept traffic
for
> extension A (which I already have), will I be notified of that?
> 
> What about if I do not yet have A installed, but I install it later?

Extensions cannot intercept traffic to chrome-extension URLs other than their
own.

[Aaron Boodman, 2011-12-06 06:40:31]

Agree with Matt's comment, but I think this CL won't have any effect at all as
coded. Somebody has to pass it into the URLPattern constructor in order for it
to match things.

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:19: // TODO(aa): What about more obscure
schemas like data: and javascript: ?
Nit: s/schemas/schemes/

[Aaron Boodman, 2011-12-06 06:43:22]

For the future.

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/static/webRequest.html (right):

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
chrome/common/extensions/docs/static/webRequest.html:199:
<code>*://www.google.com/foo*bar</code>, <code>chrome-extension://*/*</code>.
I don't think that you should call out chrome-extension:// here since it is such
an edge case.

Instead, I think we should do a quick spruce of the match pattern docs
throughout.

In match_pattern.html, it should describe the general syntax, but not a lot of
specifics. It should describe that some parts can be allowed or not allowed
depending on the API.

In each API that uses match patterns, it should describe the specifics of what
is legal. That is where you could mention chrome-extension:// for web request.

[battre, 2011-12-06 12:14:15]

On 2011/12/05 20:26:50, Matt Perry wrote:
> We should be careful to only allow this for the webRequest API. We don't want
to
> accidentally let chrome-extension schemes to be specified for host
permissions,
> for example.

Yes, we should be careful. It should be the responsibility of the user of
URLPattern, though, to consider whether it is safe to use wildcard that allows
all schemas (even those that will be added in the future).

Here is my analysis:

You can specify which schemes may be processed in the following ways:
- URLPattern::URLPattern() --> default to SCHEME_NONE
- URLPattern::URLPattern(int valid_schemes)
- URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
- void URLPattern::SetValidSchemes(int valid_schemes)

We need to analyze the last three:

- URLPattern::URLPattern(int valid_schemes)
  - used in Extension::ParsePermissions(...):
    - for component extensions, if the command line flag
kExposePrivateExtensionApi is set, and for scripting whitelisted extensions
(used for testing) we allow all URL Patterns --> extensions can gain additional
host permissions, should we change this?
    - for everything else we use kValidHostPermissionSchemes (http, https,
chrome) --> nothing changes

- URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
  - used in various unit tests --> ignored
  - QueryTabsFunction::RunImpl() --> This function would gain access to
extension config tabs

- void URLPattern::SetValidSchemes(int valid_schemes)
  - ExtensionPrefs::ReadExtensionPrefURLPatternSet --> called with either
Extension::kValidHostPermissionSchemes or UserScript::kValidUserScriptSchemes,
neither contains the chrome-extension:// scheme --> safe
  - Extension::LoadUserScriptHelper --> by default
UserScript::kValidUserScriptSchemes, can be extended to SCHEME_ALL in case
CanExecuteScriptEverywhere()  returns true (same as for
Extension::ParsePermissions)
  - ParamTraits<URLPattern>::Read(const Message* m, void** iter, param_type* p)
and
    UserScript::Unpickle
    returns only the URLPatterns with the same valid schemes that were
serialized

Overall there are two cases, where permissions are elevated:
- if Extension::CanExecuteScriptEverywhere() returns true
- in QueryTabsFunction::RunImpl()

Is this ok with you?

[battre, 2011-12-06 12:45:16]

On 2011/12/06 06:40:31, Aaron Boodman wrote:
> Agree with Matt's comment, but I think this CL won't have any effect at all as
> coded. Somebody has to pass it into the URLPattern constructor in order for it
> to match things.

I tried it with this listener:

chrome.webRequest.onBeforeRequest.addListener(
  function(details) {
    console.log(details.url);
  },
  {urls: ["chrome-extension://*/*"]},
  [])

and this manifest file
{
  "name": "Webrequest chrome-extension:// test",
  "version": "0.1",
  "description": "content length",
  "background_page": "background.html",
  "options_page": "options.html",
  "permissions": ["webRequest", "webRequestBlocking", "tabs", "<all_urls>"]
}

I observed only requests to the options.html page and no general browsing
traffic.

[battre, 2011-12-06 12:51:35]

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/static/webRequest.html (right):

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
chrome/common/extensions/docs/static/webRequest.html:199:
<code>*://www.google.com/foo*bar</code>, <code>chrome-extension://*/*</code>.
On 2011/12/06 06:43:23, Aaron Boodman wrote:
> I don't think that you should call out chrome-extension:// here since it is
such
> an edge case.
> 
> Instead, I think we should do a quick spruce of the match pattern docs
> throughout.
> 
> In match_pattern.html, it should describe the general syntax, but not a lot of
> specifics. It should describe that some parts can be allowed or not allowed
> depending on the API.
> 
> In each API that uses match patterns, it should describe the specifics of what
> is legal. That is where you could mention chrome-extension:// for web request.

Done.

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/webRequest.html (right):

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/doc...
chrome/common/extensions/docs/webRequest.html:672:
<code>*://www.google.com/foo*bar</code>, <code>chrome-extension://*/*</code>.
On 2011/12/05 21:33:59, Chris P. wrote:
> Is "chrome-extension://*/*" actually legal, though? Do you have to register to
> handle a specific extension ID, or can you really register to handle all
> extensions?

We filter network events by URLPattern *and* make sure for chrome-extension://
URLs that only requests the the same extension are visible.

> If you always need to give an ID, and I think? you should, document it that
way
> (as you do in the CL comment).

You don't need to always give an ID. All requests to non-matching extensions are
just ignored.

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8800006/diff/4001/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:19: // TODO(aa): What about more obscure
schemas like data: and javascript: ?
On 2011/12/06 06:40:31, Aaron Boodman wrote:
> Nit: s/schemas/schemes/

Done.

[Matt Perry, 2011-12-06 20:28:01]

Actually, thinking about this some more, I don't think this change does
anything.

We currently use SCHEME_ALL for filters, which doesn't restrict the scheme type
to anything. People should be able to specify chrome-extension URLs in their
filters.

If people omit a filter, we allow all traffic through.

I think what the original bug might be complaining about is that he didn't ask
for chrome-extension host permissions in the manifest, but was still seeing
chrome-extension traffic. In that case, the bug is invalid, since that
permission is implied. If he doesn't want chrome-extension traffic, he should
specify a filter that doesn't include it. Looking at the code, I believe that
will work today (though I didn't test it).

[Aaron Boodman, 2011-12-06 20:31:17]

On Tue, Dec 6, 2011 at 4:14 AM,  <battre@chromium.org> wrote:
> On 2011/12/05 20:26:50, Matt Perry wrote:
>>
>> We should be careful to only allow this for the webRequest API. We don't
>> want
>
> to
>>
>> accidentally let chrome-extension schemes to be specified for host
>
> permissions,
>>
>> for example.
>
>
> Yes, we should be careful. It should be the responsibility of the user of
> URLPattern, though, to consider whether it is safe to use wildcard that
> allows
> all schemas (even those that will be added in the future).
>
> Here is my analysis:
>
> You can specify which schemes may be processed in the following ways:
> - URLPattern::URLPattern() --> default to SCHEME_NONE
> - URLPattern::URLPattern(int valid_schemes)
> - URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
> - void URLPattern::SetValidSchemes(int valid_schemes)
>
> We need to analyze the last three:
>
> - URLPattern::URLPattern(int valid_schemes)
>  - used in Extension::ParsePermissions(...):
>    - for component extensions, if the command line flag
> kExposePrivateExtensionApi is set, and for scripting whitelisted extensions
> (used for testing) we allow all URL Patterns --> extensions can gain
> additional
> host permissions, should we change this?
>    - for everything else we use kValidHostPermissionSchemes (http, https,
> chrome) --> nothing changes
>
> - URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
>  - used in various unit tests --> ignored
>  - QueryTabsFunction::RunImpl() --> This function would gain access to
> extension config tabs
>
> - void URLPattern::SetValidSchemes(int valid_schemes)
>  - ExtensionPrefs::ReadExtensionPrefURLPatternSet --> called with either
> Extension::kValidHostPermissionSchemes or
> UserScript::kValidUserScriptSchemes,
> neither contains the chrome-extension:// scheme --> safe
>  - Extension::LoadUserScriptHelper --> by default
> UserScript::kValidUserScriptSchemes, can be extended to SCHEME_ALL in case
> CanExecuteScriptEverywhere()  returns true (same as for
> Extension::ParsePermissions)
>  - ParamTraits<URLPattern>::Read(const Message* m, void** iter, param_type*
> p)
> and
>    UserScript::Unpickle
>    returns only the URLPatterns with the same valid schemes that were
> serialized
>
> Overall there are two cases, where permissions are elevated:
> - if Extension::CanExecuteScriptEverywhere() returns true
> - in QueryTabsFunction::RunImpl()

Thanks for this refresher.

SCHEME_ALL actually means that *all* schemes will be valid, even those
not listed in kValidSchemes. See URLPattern::IsValidScheme(). So
anyone that was using SCHEME_ALL before already had all the power. So
I still think this change has no effect (it would have already been
possible to specify chrome-extension://<id>/*).

There was actually a comment in URLPattern in capital letters saying
that third-party extensions should not be able to use patterns
initialized with SCHEME_ALL - that it's only for internal Chrome code.
Did you guys see this when you used it? :)

I think we should be conservative and only initialize this pattern to
those schemes that we intend it to be used with. I think those are
http, https, ftp, file, and chrome-extension.

As for QueryTabsFunction, it seems correct. That API doesn't use the
pattern to grant access to the content of tabs, only to see their
URLs. Can you add a comment there explaining why it is OK, and loosen
the warning in url_pattern.h to say something like "usually" instead
of "never", as well as make any changes that would have made it easier
for you to see in the first place?

Thanks,

- a

[battre, 2011-12-09 14:14:53]

On 2011/12/06 20:31:17, Aaron Boodman wrote:
> On Tue, Dec 6, 2011 at 4:14 AM,  <mailto:battre@chromium.org> wrote:
> > On 2011/12/05 20:26:50, Matt Perry wrote:
> >>
> >> We should be careful to only allow this for the webRequest API. We don't
> >> want
> >
> > to
> >>
> >> accidentally let chrome-extension schemes to be specified for host
> >
> > permissions,
> >>
> >> for example.
> >
> >
> > Yes, we should be careful. It should be the responsibility of the user of
> > URLPattern, though, to consider whether it is safe to use wildcard that
> > allows
> > all schemas (even those that will be added in the future).
> >
> > Here is my analysis:
> >
> > You can specify which schemes may be processed in the following ways:
> > - URLPattern::URLPattern() --> default to SCHEME_NONE
> > - URLPattern::URLPattern(int valid_schemes)
> > - URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
> > - void URLPattern::SetValidSchemes(int valid_schemes)
> >
> > We need to analyze the last three:
> >
> > - URLPattern::URLPattern(int valid_schemes)
> > &nbsp;- used in Extension::ParsePermissions(...):
> > &nbsp; &nbsp;- for component extensions, if the command line flag
> > kExposePrivateExtensionApi is set, and for scripting whitelisted extensions
> > (used for testing) we allow all URL Patterns --> extensions can gain
> > additional
> > host permissions, should we change this?
> > &nbsp; &nbsp;- for everything else we use kValidHostPermissionSchemes (http,
https,
> > chrome) --> nothing changes
> >
> > - URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
> > &nbsp;- used in various unit tests --> ignored
> > &nbsp;- QueryTabsFunction::RunImpl() --> This function would gain access to
> > extension config tabs
> >
> > - void URLPattern::SetValidSchemes(int valid_schemes)
> > &nbsp;- ExtensionPrefs::ReadExtensionPrefURLPatternSet --> called with
either
> > Extension::kValidHostPermissionSchemes or
> > UserScript::kValidUserScriptSchemes,
> > neither contains the chrome-extension:// scheme --> safe
> > &nbsp;- Extension::LoadUserScriptHelper --> by default
> > UserScript::kValidUserScriptSchemes, can be extended to SCHEME_ALL in case
> > CanExecuteScriptEverywhere() &nbsp;returns true (same as for
> > Extension::ParsePermissions)
> > &nbsp;- ParamTraits<URLPattern>::Read(const Message* m, void** iter,
param_type*
> > p)
> > and
> > &nbsp; &nbsp;UserScript::Unpickle
> > &nbsp; &nbsp;returns only the URLPatterns with the same valid schemes that
were
> > serialized
> >
> > Overall there are two cases, where permissions are elevated:
> > - if Extension::CanExecuteScriptEverywhere() returns true
> > - in QueryTabsFunction::RunImpl()
> 
> Thanks for this refresher.
> 
> SCHEME_ALL actually means that *all* schemes will be valid, even those
> not listed in kValidSchemes. See URLPattern::IsValidScheme(). So
> anyone that was using SCHEME_ALL before already had all the power. So
> I still think this change has no effect (it would have already been
> possible to specify chrome-extension://<id>/*).
> 
> There was actually a comment in URLPattern in capital letters saying
> that third-party extensions should not be able to use patterns
> initialized with SCHEME_ALL - that it's only for internal Chrome code.
> Did you guys see this when you used it? :)
> 
> I think we should be conservative and only initialize this pattern to
> those schemes that we intend it to be used with. I think those are
> http, https, ftp, file, and chrome-extension.

Done.

> As for QueryTabsFunction, it seems correct. That API doesn't use the
> pattern to grant access to the content of tabs, only to see their
> URLs. Can you add a comment there explaining why it is OK, and loosen
> the warning in url_pattern.h to say something like "usually" instead
> of "never", as well as make any changes that would have made it easier
> for you to see in the first place?

Done.

Thanks,
Dominic

[Aaron Boodman, 2011-12-09 16:28:25]

I think we need kathy's help with the docs. I see you have a separate CL going
with her though.

http://codereview.chromium.org/8800006/diff/22001/chrome/common/extensions/do...
File chrome/common/extensions/docs/static/manifest.html (right):

http://codereview.chromium.org/8800006/diff/22001/chrome/common/extensions/do...
chrome/common/extensions/docs/static/manifest.html:435: the chrome-extension://
scheme in match patterns for host permissions.
This line isn't necessary.

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

http://codereview.chromium.org/8800006/diff/22001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:102: // !!! SCHEME_ALL SHOULD ONLY BE
USED FOR INTERNAL CHROME CODE !!!
This last sentence seems to conflict with the above paragraph.

How about this:

- put a line break before the beginning of the "SCHEME_ALL will match..."
comment to visually break it up.
- remove this "!!! SCHEME_ALL..." comment
- Add "IMPORTANT! " to the beginning of the comment above.

[Aaron Boodman, 2011-12-09 16:28:46]

LGTM w/ above. No need for another review from me.

[battre, 2011-12-09 17:07:51]

http://codereview.chromium.org/8800006/diff/22001/chrome/common/extensions/do...
File chrome/common/extensions/docs/static/manifest.html (right):

http://codereview.chromium.org/8800006/diff/22001/chrome/common/extensions/do...
chrome/common/extensions/docs/static/manifest.html:435: the chrome-extension://
scheme in match patterns for host permissions.
On 2011/12/09 16:28:25, Aaron Boodman wrote:
> This line isn't necessary.

Done.

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

http://codereview.chromium.org/8800006/diff/22001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:102: // !!! SCHEME_ALL SHOULD ONLY BE
USED FOR INTERNAL CHROME CODE !!!
On 2011/12/09 16:28:25, Aaron Boodman wrote:
> This last sentence seems to conflict with the above paragraph.
> 
> How about this:
> 
> - put a line break before the beginning of the "SCHEME_ALL will match..."
> comment to visually break it up.
> - remove this "!!! SCHEME_ALL..." comment
> - Add "IMPORTANT! " to the beginning of the comment above.

Done.

[commit-bot: I haz the power, 2011-12-09 17:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8800006/22014

[commit-bot: I haz the power, 2011-12-09 17:50:58]

Try job failure for 8800006-22014 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-09 18:00:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8800006/25012

[commit-bot: I haz the power, 2011-12-09 19:51:46]

Try job failure for 8800006-25012 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-12-09 20:18:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8800006/25012

[commit-bot: I haz the power, 2011-12-09 23:18:46]

Try job failure for 8800006-25012 (retry) on win_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-09 23:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8800006/25012

[commit-bot: I haz the power, 2011-12-10 01:01:09]

Change committed as 113907