Make <all_urls> and file:///* in permissions trigger "Allow file access"

Make <all_urls> and file:///* in permissions trigger "Allow file access"

Move ExtensionWantsFileAccess static functions to Extension::wants_file_access
which is set as part of the Extension initialization process.

When file:/// access is granted/revoked, instead of storing that in a bit on the
UserScript (which didn't help with extension host permissions), we use that
to filter down the list of valid schemes for permissions/content script match
matterns. That way we don't have to manually exclude file:///* patterns at
various call sites (e.g. in UserScriptSlave).

BUG=76705
TEST=unit_tests
R=aa@chromium.org

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

[Aaron Boodman, 2011-03-30 01:24:54]

lgtm

Some design ideas below. Feel free to ignore.

http://codereview.chromium.org/6772022/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6772022/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:623: InitWantsFileAccess(match_str,
parse_strictness);
The handling of wants_file_access_ is kind of weird, but I'm having trouble
putting my finger on what bothers me about it. So here are some alternate ideas
to consider:

1. Instead of parsing each pattern twice, you could instead always parse with
SCHEME_FILE, then check MatchesScheme(FILE), then remove SCHEME_FILE from
allowed_schemes if necessary.

2. You could always parse URLPatterns with SCHEME_FILE and then centralize
handling of wants_file_access_ to the end of ::InitFromValue(). Iterate through
all the content script matches and set wants_file_access_ and remove SCHEME_FILE
from the URLPatterns as necessary.

3. You could nix wants_files_access_ completely and make this functionality part
of ExtensionPrefs. ExtensionPrefs has access to the real manifest, and could
have code that looks at it and decides whether the extension wants file access,
without having to have anything in Extension.

4. Maybe you already considered this and have some future plan?

[Mihai Parparita -not on Chrome, 2011-03-30 23:00:49]

On Tue, Mar 29, 2011 at 6:24 PM,  <aa@chromium.org> wrote:
> 1. Instead of parsing each pattern twice, you could instead always parse
> with SCHEME_FILE, then check MatchesScheme(FILE), then remove
> SCHEME_FILE from allowed_schemes if necessary.
>
> 2. You could always parse URLPatterns with SCHEME_FILE and then
> centralize handling of wants_file_access_ to the end of
> ::InitFromValue(). Iterate through all the content script matches and
> set wants_file_access_ and remove SCHEME_FILE from the URLPatterns as
> necessary.

I ran into something like this when fixing a unit test failure.
Removing file access from a URLPattern is a bit tricky, since you have
to handle the wildcard scheme case too:
http://codereview.chromium.org/6772022/diff/5001/chrome/browser/extensions/ex...

> 3. You could nix wants_files_access_ completely and make this
> functionality part of ExtensionPrefs. ExtensionPrefs has access to the
> real manifest, and could have code that looks at it and decides whether
> the extension wants file access, without having to have anything in
> Extension.

See above. I'm actually confused as to what ExtensionPrefs does with
the manifest (it stores a copy?).

Mihai

[Aaron Boodman, 2011-03-31 16:04:57]

http://codereview.chromium.org/6772022/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6772022/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:587: if (!allow_file_access &&
pattern.MatchesScheme(chrome::kFileScheme)) {
I'm not sure why this branch was needed. Perhaps it is related to ALL_SCHEMES |
SCHEME_FILE ?

http://codereview.chromium.org/6772022/diff/8001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6772022/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:628: valid_user_script_schemes |=
URLPattern::SCHEME_FILE;
It seems weird to OR SCHEME_ALL with one of the other values.

[Mihai Parparita -not on Chrome, 2011-03-31 21:56:30]

http://codereview.chromium.org/6772022/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/6772022/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:587: if (!allow_file_access &&
pattern.MatchesScheme(chrome::kFileScheme)) {
On 2011/03/31 16:04:57, Aaron Boodman wrote:
> I'm not sure why this branch was needed. Perhaps it is related to ALL_SCHEMES
|
> SCHEME_FILE ?

Without the changes in this file,
ExtensionServiceTest.LoadAllExtensionsFromDirectorySources fails (ends up in the
NOTREACHED branch above). That's because we load
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/g...,
which has a file:/// permission. However, I removed SCHEME_FILE from
UserScript::kValidUserScriptSchemes, so file:/// permissions are not valid. I
added it back to mask at line 574, but it seemed more correct to actually filter
out those patterns if they weren't really a granted permission, hence the
filtering here.

http://codereview.chromium.org/6772022/diff/8001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6772022/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:628: valid_user_script_schemes |=
URLPattern::SCHEME_FILE;
On 2011/03/31 16:04:57, Aaron Boodman wrote:
> It seems weird to OR SCHEME_ALL with one of the other values.

Should be a noop, but OK, re-wrote this to only |= SCHEME_FILE if we're not
allowed to execute scripts everywhere.

[Mihai Parparita -not on Chrome, 2011-04-01 00:08:10]

OK, re-did this a bit to leave the file scheme in the default
permitted schemes, and to remove it from the valid schemes if it's not
actually allowed. This also let me remove InitWantsFileAccess and
generally clean things up.

Mihai

On Thu, Mar 31, 2011 at 2:56 PM,  <mihaip@chromium.org> wrote:
>
>
http://codereview.chromium.org/6772022/diff/8001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_prefs.cc (right):
>
>
http://codereview.chromium.org/6772022/diff/8001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_prefs.cc:587: if (!allow_file_access
> && pattern.MatchesScheme(chrome::kFileScheme)) {
> On 2011/03/31 16:04:57, Aaron Boodman wrote:
>>
>> I'm not sure why this branch was needed. Perhaps it is related to
>
> ALL_SCHEMES |
>>
>> SCHEME_FILE ?
>
> Without the changes in this file,
> ExtensionServiceTest.LoadAllExtensionsFromDirectorySources fails (ends
> up in the NOTREACHED branch above). That's because we load
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/g...,
> which has a file:/// permission. However, I removed SCHEME_FILE from
> UserScript::kValidUserScriptSchemes, so file:/// permissions are not
> valid. I added it back to mask at line 574, but it seemed more correct
> to actually filter out those patterns if they weren't really a granted
> permission, hence the filtering here.
>
>
http://codereview.chromium.org/6772022/diff/8001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension.cc (right):
>
>
http://codereview.chromium.org/6772022/diff/8001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:628: valid_user_script_schemes |=
> URLPattern::SCHEME_FILE;
> On 2011/03/31 16:04:57, Aaron Boodman wrote:
>>
>> It seems weird to OR SCHEME_ALL with one of the other values.
>
> Should be a noop, but OK, re-wrote this to only |= SCHEME_FILE if we're
> not allowed to execute scripts everywhere.
>
> http://codereview.chromium.org/6772022/
>

[Aaron Boodman, 2011-04-01 00:17:03]

lgtm