Allow extensions on chrome:// URLs, when flag is set and permission is explicitly requested

Allow extensions on chrome:// URLs, when flag is set and permission is explicitly requested in the manifest.

BUG=174183
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189586

[Matt Perry, 2013-03-16 01:16:29]

I don't understand the difference between allowed_schemes and valid_schemes.
Conceptually they seem like the same idea. The reason for valid_schemes is to
whitelist the types of schemes that are matchable by that URLPattern. When is a
scheme in allowed_schemes but not in valid_schemes (or vice versa)?

[aboxhall, 2013-03-18 14:46:18]

On 2013/03/16 01:16:29, Matt Perry wrote:
> I don't understand the difference between allowed_schemes and valid_schemes.
> Conceptually they seem like the same idea. The reason for valid_schemes is to
> whitelist the types of schemes that are matchable by that URLPattern. When is
a
> scheme in allowed_schemes but not in valid_schemes (or vice versa)?

That's a reasonable question. In theory, it seems like they probably should be
the same thing, but have a look at
https://codereview.chromium.org/12792005/diff/13001/chrome/common/extensions/...
lines 192-198 (new code) and
https://codereview.chromium.org/12792005/diff/13001/chrome/common/extensions/...
lines 599-610 (old code). In each case, we're modifying valid_schemes after
parsing so that we can avoid matching some scheme which we nonetheless want to
allow in the pattern. It really confused me when I was reading through the code
that sometimes valid_schemes was used in parsing, and sometimes it was modified
after parsing so that things which were valid but shouldn't be matched wouldn't
cause a parse error, that it began to seem like there were really two concepts
here - one relating to parsing, and one relating to matching.

[aboxhall, 2013-03-18 14:48:14]

On 2013/03/18 14:46:18, aboxhall wrote:
> On 2013/03/16 01:16:29, Matt Perry wrote:
> > I don't understand the difference between allowed_schemes and valid_schemes.
> > Conceptually they seem like the same idea. The reason for valid_schemes is
to
> > whitelist the types of schemes that are matchable by that URLPattern. When
is
> a
> > scheme in allowed_schemes but not in valid_schemes (or vice versa)?
> 
> That's a reasonable question. In theory, it seems like they probably should be
> the same thing, but have a look at
>
https://codereview.chromium.org/12792005/diff/13001/chrome/common/extensions/...
> lines 192-198 (new code) and
>
https://codereview.chromium.org/12792005/diff/13001/chrome/common/extensions/...
> lines 599-610 (old code). In each case, we're modifying valid_schemes after
> parsing so that we can avoid matching some scheme which we nonetheless want to
> allow in the pattern. It really confused me when I was reading through the
code
> that sometimes valid_schemes was used in parsing, and sometimes it was
modified
> after parsing so that things which were valid but shouldn't be matched
wouldn't
> cause a parse error, that it began to seem like there were really two concepts
> here - one relating to parsing, and one relating to matching.

Also note that I would have had to modify UnpickleURLPatternSet() in
user_script.cc to add another temporary change to valid_schemes as per the
existing code for file://.

[Matt Perry, 2013-03-19 00:43:15]

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2178: URLPattern
pattern(UserScript::kValidUserScriptSchemes);
How about this instead: replace kValidUserScriptSchemes with
GetValidUserScriptSchemes, which includes SCHEME_CHROMEUI iff the flag is there.

If we use it in the renderer process, we'll have to pass that flag down.

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2200:
!pattern.MatchesScheme(chrome::kChromeUIScheme)) {
<all_urls> will return true for this, I believe.

[aboxhall, 2013-03-19 00:47:47]

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2200:
!pattern.MatchesScheme(chrome::kChromeUIScheme)) {
On 2013/03/19 00:43:15, Matt Perry wrote:
> <all_urls> will return true for this, I believe.

Yes, it will; already fixed.

[aboxhall, 2013-03-19 16:11:12]

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12792005/diff/19001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2178: URLPattern
pattern(UserScript::kValidUserScriptSchemes);
On 2013/03/19 00:43:15, Matt Perry wrote:
> How about this instead: replace kValidUserScriptSchemes with
> GetValidUserScriptSchemes, which includes SCHEME_CHROMEUI iff the flag is
there.
> 
> If we use it in the renderer process, we'll have to pass that flag down.

Done. The flag is passed down in chrome_content_browser_client.cc.

[Matt Perry, 2013-03-19 17:49:32]

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2194: pattern.SetValidSchemes(
Add a comment on what's happening here.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/user_script.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.cc:145:
pickle->WriteString(pattern->GetAsString());
Why change this?

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/user_script.h (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.h:30: enum {
Move this into the .cc file so no one accidentally uses it.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.h:42: static int
ValidUserScriptSchemes(bool canExecuteScriptEverywhere);
Comment this.

[dmazzoni, 2013-03-19 20:48:44]

https://codereview.chromium.org/12792005/diff/28001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6320: +        Enables running extensions on
chrome:// URLs, where extensions have the appropriate permission.
"where extensions have the appropriate permission" -> "when extensions
specifically request this permission"

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:827: return false;
Doesn't this allow any extension to run on chrome:// urls with the flag, even if
they don't request it? Or is that caught earlier?

[aboxhall, 2013-03-20 22:04:59]

Addressed comments, and added some tests.

https://codereview.chromium.org/12792005/diff/28001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6320: +        Enables running extensions on
chrome:// URLs, where extensions have the appropriate permission.
On 2013/03/19 20:48:44, Dominic Mazzoni wrote:
> "where extensions have the appropriate permission" -> "when extensions
> specifically request this permission"

Done.

https://codereview.chromium.org/12792005/diff/28001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6320: +        Enables running extensions on
chrome:// URLs, where extensions have the appropriate permission.
On 2013/03/19 20:48:44, Dominic Mazzoni wrote:
> "where extensions have the appropriate permission" -> "when extensions
> specifically request this permission"

Done.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:827: return false;
On 2013/03/19 20:48:44, Dominic Mazzoni wrote:
> Doesn't this allow any extension to run on chrome:// urls with the flag, even
if
> they don't request it? Or is that caught earlier?

It's caught later, specifically on either line 850 or 854/55. This line just
early-outs in the case of a chrome:// url.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2194: pattern.SetValidSchemes(
On 2013/03/19 17:49:32, Matt Perry wrote:
> Add a comment on what's happening here.

Done.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2194: pattern.SetValidSchemes(
On 2013/03/19 17:49:32, Matt Perry wrote:
> Add a comment on what's happening here.

Done.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/user_script.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.cc:145:
pickle->WriteString(pattern->GetAsString());
On 2013/03/19 17:49:32, Matt Perry wrote:
> Why change this?

Because I don't want to set valid_schemes until after parsing the pattern. I
could unpickle it and then add it after parsing the pattern, I guess; this could
conceivably break stuff?

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/user_script.h (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.h:30: enum {
On 2013/03/19 17:49:32, Matt Perry wrote:
> Move this into the .cc file so no one accidentally uses it.

Done.

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.h:42: static int
ValidUserScriptSchemes(bool canExecuteScriptEverywhere);
On 2013/03/19 17:49:32, Matt Perry wrote:
> Comment this.

Done.

[Matt Perry, 2013-03-20 22:32:48]

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/user_script.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.cc:145:
pickle->WriteString(pattern->GetAsString());
On 2013/03/20 22:04:59, aboxhall wrote:
> On 2013/03/19 17:49:32, Matt Perry wrote:
> > Why change this?
> 
> Because I don't want to set valid_schemes until after parsing the pattern. I
> could unpickle it and then add it after parsing the pattern, I guess; this
could
> conceivably break stuff?

Yeah I'd prefer to keep the old pickle order, just to avoid churn. You can
unpickle both the pattern and valid_schemes before parsing/setting the schemes.
(As in, the order you read the vars has no effect on the order of parsing.)

[aboxhall, 2013-03-20 22:51:47]

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/user_script.cc (right):

https://codereview.chromium.org/12792005/diff/28001/chrome/common/extensions/...
chrome/common/extensions/user_script.cc:145:
pickle->WriteString(pattern->GetAsString());
On 2013/03/20 22:32:48, Matt Perry wrote:
> On 2013/03/20 22:04:59, aboxhall wrote:
> > On 2013/03/19 17:49:32, Matt Perry wrote:
> > > Why change this?
> > 
> > Because I don't want to set valid_schemes until after parsing the pattern. I
> > could unpickle it and then add it after parsing the pattern, I guess; this
> could
> > conceivably break stuff?
> 
> Yeah I'd prefer to keep the old pickle order, just to avoid churn. You can
> unpickle both the pattern and valid_schemes before parsing/setting the
schemes.
> (As in, the order you read the vars has no effect on the order of parsing.)

Good call, done.

[Matt Perry, 2013-03-20 23:00:50]

lgtm

[commit-bot: I haz the power, 2013-03-20 23:12:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/12792005/38001

[commit-bot: I haz the power, 2013-03-20 23:12:44]

Presubmit check for 12792005-38001 failed and returned exit status 1.

INFO:root:Found 19 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/startup/bad_flags_prompt.cc

Presubmit checks took 3.4s to calculate.

[aboxhall, 2013-03-20 23:17:47]

sky@, could you take a look at  chrome/browser/ui/startup/bad_flags_prompt.cc
for OWNERS?

[sky, 2013-03-20 23:57:07]

LGTM

[commit-bot: I haz the power, 2013-03-21 00:02:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/12792005/38001

[commit-bot: I haz the power, 2013-03-21 12:57:33]

Change committed as 189586