Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10213002:
Make downloads.download() respect host permissions (Closed)
|
Created:
8 years, 8 months ago by benjhayden Modified:
8 years, 7 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, rdsmith+dwatch_chromium.org, Eric U. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake downloads.download() respect host permissions
This behavior is already documented: http://code.google.com/chrome/extensions/trunk/experimental.downloads.html
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137245
Patch Set 1 #Patch Set 2 : " #Patch Set 3 : " #
Total comments: 4
Patch Set 4 : " #
Total comments: 2
Patch Set 5 : merge #Patch Set 6 : style #
Total comments: 4
Patch Set 7 : debugging #Patch Set 8 : aha #Patch Set 9 : -data #Patch Set 10 : style #Patch Set 11 : " #
Total comments: 3
Patch Set 12 : " #Patch Set 13 : " #Patch Set 14 : merge #
Total comments: 4
Patch Set 15 : merge #
Messages
Total messages: 33 (0 generated)
LGTM, but you should probably run this by an extensions person.
Mihai, would you mind checking my use of HasHostPermission()?
http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:386: if ((iodata_->url.SchemeIs("http") || Why do you have these scheme checks? HasHostPermission (despite the name) also checks schemes.
http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:386: if ((iodata_->url.SchemeIs("http") || On 2012/05/01 20:08:31, Mihai Parparita wrote: > Why do you have these scheme checks? HasHostPermission (despite the name) also > checks schemes. HasHostPermission() *disallows* schemes other than these, but download() should *allow* them. Extensions should be able to "download" data: and filesystem: URLs.
http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:386: if ((iodata_->url.SchemeIs("http") || On 2012/05/01 20:19:12, benjhayden_chromium wrote: > On 2012/05/01 20:08:31, Mihai Parparita wrote: > > Why do you have these scheme checks? HasHostPermission (despite the name) also > > checks schemes. > > HasHostPermission() *disallows* schemes other than these, but download() should > *allow* them. Extensions should be able to "download" data: and filesystem: > URLs. Then I'd rather have those be explicitly listed (i.e. making the check into !url.SchemeIs("data") && !url.SchemeIs("filesystem") && !GetExtension()->HasHostPermission(url)). Otherwise you risk allowing unknown schemes (e.g. your current code allows downloads from the chrome:// scheme, which doesn't seem wise).
PTAL http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/12002/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:386: if ((iodata_->url.SchemeIs("http") || On 2012/05/01 20:53:12, Mihai Parparita wrote: > On 2012/05/01 20:19:12, benjhayden_chromium wrote: > > On 2012/05/01 20:08:31, Mihai Parparita wrote: > > > Why do you have these scheme checks? HasHostPermission (despite the name) > also > > > checks schemes. > > > > HasHostPermission() *disallows* schemes other than these, but download() > should > > *allow* them. Extensions should be able to "download" data: and filesystem: > > URLs. > > Then I'd rather have those be explicitly listed (i.e. making the check into > !url.SchemeIs("data") && !url.SchemeIs("filesystem") && > !GetExtension()->HasHostPermission(url)). Otherwise you risk allowing unknown > schemes (e.g. your current code allows downloads from the chrome:// scheme, > which doesn't seem wise). Done.
LGTM
http://codereview.chromium.org/10213002/diff/20002/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/20002/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:385: !iodata_->url.SchemeIs("filesystem") && Isn't this a way to circumvent host permissions? The extension will be able to download filesystem:http://www.google.com, even if it doesn't have host permissions to google.com. That doesn't seem right? I'm not sure about blobs. Are blobs tied to an origin? Or are they globally unique ids? If the latter, I suppose the extension would have had already had to have access to an origin in order to get the blob URL in the first place, so it is fine.
On 2012/05/02 22:52:07, Aaron Boodman wrote: > http://codereview.chromium.org/10213002/diff/20002/chrome/browser/download/do... > File chrome/browser/download/download_extension_api.cc (right): > > http://codereview.chromium.org/10213002/diff/20002/chrome/browser/download/do... > chrome/browser/download/download_extension_api.cc:385: > !iodata_->url.SchemeIs("filesystem") && > Isn't this a way to circumvent host permissions? The extension will be able to > download http://filesystem:http://www.google.com, even if it doesn't have host > permissions to http://google.com. That doesn't seem right? > > I'm not sure about blobs. Are blobs tied to an origin? Or are they globally > unique ids? If the latter, I suppose the extension would have had already had to > have access to an origin in order to get the blob URL in the first place, so it > is fine. BTW, ericu is working on implementing first-class support for filesystem URLs in the extension system, so there's no need for this special case here.
+ericu fyi
http://codereview.chromium.org/10213002/diff/20002/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/20002/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:385: !iodata_->url.SchemeIs("filesystem") && On 2012/05/02 22:52:07, Aaron Boodman wrote: > Isn't this a way to circumvent host permissions? The extension will be able to > download http://filesystem:http://www.google.com, even if it doesn't have host > permissions to http://google.com. That doesn't seem right? Yeah, this isn't right. Just take !iodata_->url.SchemeIs("filesystem") off the restriction list. Once https://chromiumcodereview.appspot.com/10224011/ lands, HasHostPermission will do the right thing with filesystem URLs. And before it's landed, this check as written would be a security hole, as Aaron points out. > I'm not sure about blobs. Are blobs tied to an origin? Or are they globally > unique ids? If the latter, I suppose the extension would have had already had to > have access to an origin in order to get the blob URL in the first place, so it > is fine. Blobs are tied to an origin, but I don't know if you can view them cross-origin or not. MichaelN says you can. I'd suggest checking with abarth if you want a security expert opinion.
Looks like 10224011 landed. I merged, wrote a test for the filesystem: case, removed the scheme exceptions from download_extension_api.cc, and added all the permissions url patterns i could think of to manifest.json. The new test fails. It looks like gurl->inner_url()->scheme() == gurl->scheme(). I.e. inner_url() is never actually different from its outer_url.
On 2012/05/03 16:47:36, benjhayden_chromium wrote: > Looks like 10224011 landed. I merged, wrote a test for the filesystem: case, > removed the scheme exceptions from download_extension_api.cc, and added all the > permissions url patterns i could think of to manifest.json. The new test fails. > > It looks like gurl->inner_url()->scheme() == gurl->scheme(). I.e. inner_url() is > never actually different from its outer_url. It should *never* be the case that gurl->inner_url()->scheme() == gurl->scheme(). That would require a filesystem:filesystem:// URL, which is illegal. The parser should never return this. Are you sure that gurl->is_valid()?
http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... File chrome/test/data/extensions/api_test/downloads/manifest.json (right): http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... chrome/test/data/extensions/api_test/downloads/manifest.json:15: "filesystem:*", I don't think you should need either mention of filesystem: URLs in the manifest. Support for http://*/* should automatically grant filesystem:http://*/*, etc. http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... chrome/test/data/extensions/api_test/downloads/test.js:22: window.MozBlobBuilder); Why MozBlobBuilder? This won't run on Firefox.
The URL is filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt I added log statements to both gurl.cc and url_pattern.cc. The inner URL is filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt I also logged URLPattern::scheme_; this prints 'http', 'file', and 'https' but never 'chrome-extension' even though it's in manifest.json. http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... File chrome/test/data/extensions/api_test/downloads/manifest.json (right): http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... chrome/test/data/extensions/api_test/downloads/manifest.json:15: "filesystem:*", On 2012/05/03 17:14:55, ericu wrote: > I don't think you should need either mention of filesystem: URLs in the > manifest. Support for http://*/* should automatically grant > filesystem:http://*/*, etc. Done. http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... File chrome/test/data/extensions/api_test/downloads/test.js (right): http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... chrome/test/data/extensions/api_test/downloads/test.js:22: window.MozBlobBuilder); On 2012/05/03 17:14:55, ericu wrote: > Why MozBlobBuilder? This won't run on Firefox. Done.
On 2012/05/03 19:38:34, benjhayden_chromium wrote: > The URL is > filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt > I added log statements to both gurl.cc and url_pattern.cc. The inner URL is > filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt You're probably logging url.spec(); that'll be misleading. They both share the same string for a spec, but use different offsets into it. Debuggers also print out the spec for the URL. Feel free to DCHECK(!url.is_valid() || !url.inner_url() || !url.inner_url()->SchemeIsFileSystem()) anywhere. > I also logged URLPattern::scheme_; this prints 'http', 'file', and 'https' but > never 'chrome-extension' even though it's in manifest.json. That's odd. Try walking through the code that constructs the URLPattern [in extension.cc IIRC] and watching for the call that's supposed to set the allowable schemes. > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > File chrome/test/data/extensions/api_test/downloads/manifest.json (right): > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > chrome/test/data/extensions/api_test/downloads/manifest.json:15: "filesystem:*", > On 2012/05/03 17:14:55, ericu wrote: > > I don't think you should need either mention of filesystem: URLs in the > > manifest. Support for http://*/* should automatically grant > > filesystem:http://*/*, etc. > > Done. > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > chrome/test/data/extensions/api_test/downloads/test.js:22: > window.MozBlobBuilder); > On 2012/05/03 17:14:55, ericu wrote: > > Why MozBlobBuilder? This won't run on Firefox. > > Done.
On 2012/05/04 15:12:17, ericu wrote:
> On 2012/05/03 19:38:34, benjhayden_chromium wrote:
> > The URL is
> >
>
filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt
> > I added log statements to both gurl.cc and url_pattern.cc. The inner URL is
> >
>
filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt
>
> You're probably logging url.spec(); that'll be misleading. They both share
the
> same string for a spec, but use different offsets into it. Debuggers also
print
> out the spec for the URL. Feel free to DCHECK(!url.is_valid() ||
> !url.inner_url() || !url.inner_url()->SchemeIsFileSystem()) anywhere.
>
> > I also logged URLPattern::scheme_; this prints 'http', 'file', and 'https'
but
> > never 'chrome-extension' even though it's in manifest.json.
>
> That's odd. Try walking through the code that constructs the URLPattern [in
> extension.cc IIRC] and watching for the call that's supposed to set the
> allowable schemes.
Hokay so. Extension::ParsePermissions() loops over the permissions.
'chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/*' is not an API
permission, so it tries to parse it as a host permission.
const int kAllowedSchemes = CanExecuteScriptEverywhere() ?
URLPattern::SCHEME_ALL : kValidHostPermissionSchemes;
URLPattern pattern = URLPattern(kAllowedSchemes);
URLPattern::ParseResult parse_result = pattern.Parse(permission_str);
parse_result = 2 = PARSE_ERROR_INVALID_SCHEME
CanExecuteScriptEverywhere() is false because the api test is a user extension,
so kAllowedSchemes = kValidHostPermissionSchemes = 31.
kValidHostPermissionSchemes = UserScript::kValidUserScriptSchemes |
URLPattern::SCHEME_CHROMEUI;
UserScript::kValidUserScriptSchemes = URLPattern::SCHEME_HTTP |
URLPattern::SCHEME_HTTPS |
URLPattern::SCHEME_FILE |
URLPattern::SCHEME_FTP;
URLPattern::SCHEME_EXTENSION = 1 << 5 = 32.
I skimmed through the revision log for extension.cc to see if i could see when
SCHEME_EXTENSION might have been removed from kValidHostPermissionSchemes;
nothing jumped out at me, but I probably don't know what to look for.
>
> >
>
http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension...
> > File chrome/test/data/extensions/api_test/downloads/manifest.json (right):
> >
> >
>
http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension...
> > chrome/test/data/extensions/api_test/downloads/manifest.json:15:
> "filesystem:*",
> > On 2012/05/03 17:14:55, ericu wrote:
> > > I don't think you should need either mention of filesystem: URLs in the
> > > manifest. Support for http://*/* should automatically grant
> > > filesystem:http://*/*, etc.
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension...
> > File chrome/test/data/extensions/api_test/downloads/test.js (right):
> >
> >
>
http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension...
> > chrome/test/data/extensions/api_test/downloads/test.js:22:
> > window.MozBlobBuilder);
> > On 2012/05/03 17:14:55, ericu wrote:
> > > Why MozBlobBuilder? This won't run on Firefox.
> >
> > Done.
On 2012/05/04 18:05:03, benjhayden_chromium wrote: > On 2012/05/04 15:12:17, ericu wrote: > > On 2012/05/03 19:38:34, benjhayden_chromium wrote: > > > The URL is > > > > > > filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt > > > I added log statements to both gurl.cc and url_pattern.cc. The inner URL is > > > > > > filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt > > > > You're probably logging url.spec(); that'll be misleading. They both share > the > > same string for a spec, but use different offsets into it. Debuggers also > print > > out the spec for the URL. Feel free to DCHECK(!url.is_valid() || > > !url.inner_url() || !url.inner_url()->SchemeIsFileSystem()) anywhere. > > > > > I also logged URLPattern::scheme_; this prints 'http', 'file', and 'https' > but > > > never 'chrome-extension' even though it's in manifest.json. > > > > That's odd. Try walking through the code that constructs the URLPattern [in > > extension.cc IIRC] and watching for the call that's supposed to set the > > allowable schemes. > > Hokay so. Extension::ParsePermissions() loops over the permissions. > 'chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/*' is not an API > permission, so it tries to parse it as a host permission. > > const int kAllowedSchemes = CanExecuteScriptEverywhere() ? > URLPattern::SCHEME_ALL : kValidHostPermissionSchemes; > URLPattern pattern = URLPattern(kAllowedSchemes); > URLPattern::ParseResult parse_result = pattern.Parse(permission_str); > > parse_result = 2 = PARSE_ERROR_INVALID_SCHEME > > CanExecuteScriptEverywhere() is false because the api test is a user extension, > so kAllowedSchemes = kValidHostPermissionSchemes = 31. > kValidHostPermissionSchemes = UserScript::kValidUserScriptSchemes | > URLPattern::SCHEME_CHROMEUI; > UserScript::kValidUserScriptSchemes = URLPattern::SCHEME_HTTP | > URLPattern::SCHEME_HTTPS | > URLPattern::SCHEME_FILE | > URLPattern::SCHEME_FTP; > > URLPattern::SCHEME_EXTENSION = 1 << 5 = 32. > > I skimmed through the revision log for extension.cc to see if i could see when > SCHEME_EXTENSION might have been removed from kValidHostPermissionSchemes; > nothing jumped out at me, but I probably don't know what to look for. I could add SCHEME_EXTENSION to kValidHostPermissionSchemes, but I suspect that either that isn't the right place to add it or it was removed for a reason. Also this looks like somewhat tricky code. (Compare to the downloads code, which ranks "extremely tricky". :-) I would feel much more comfortable if somebody who knew what they were doing suggested the right way to fix this. > > > > > > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > > > File chrome/test/data/extensions/api_test/downloads/manifest.json (right): > > > > > > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > > > chrome/test/data/extensions/api_test/downloads/manifest.json:15: > > "filesystem:*", > > > On 2012/05/03 17:14:55, ericu wrote: > > > > I don't think you should need either mention of filesystem: URLs in the > > > > manifest. Support for http://*/* should automatically grant > > > > filesystem:http://*/*, etc. > > > > > > Done. > > > > > > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > > > File chrome/test/data/extensions/api_test/downloads/test.js (right): > > > > > > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... > > > chrome/test/data/extensions/api_test/downloads/test.js:22: > > > window.MozBlobBuilder); > > > On 2012/05/03 17:14:55, ericu wrote: > > > > Why MozBlobBuilder? This won't run on Firefox. > > > > > > Done.
On Fri, May 4, 2012 at 11:08 AM, <benjhayden@chromium.org> wrote: > On 2012/05/04 18:05:03, benjhayden_chromium wrote: >> >> On 2012/05/04 15:12:17, ericu wrote: >> > On 2012/05/03 19:38:34, benjhayden_chromium wrote: >> > > The URL is >> > > >> > > > > filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt >> >> > > I added log statements to both gurl.cc and url_pattern.cc. The inner >> > > URL > > is >> >> > > >> > > > > filesystem:chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/temporary/0.07071475544944406/0.txt >> >> > >> > You're probably logging url.spec(); that'll be misleading. They both >> > share >> the >> > same string for a spec, but use different offsets into it. Debuggers >> > also >> print >> > out the spec for the URL. Feel free to DCHECK(!url.is_valid() || >> > !url.inner_url() || !url.inner_url()->SchemeIsFileSystem()) anywhere. >> > >> > > I also logged URLPattern::scheme_; this prints 'http', 'file', and >> > > 'https' >> but >> > > never 'chrome-extension' even though it's in manifest.json. >> > >> > That's odd. Try walking through the code that constructs the URLPattern >> > [in >> > extension.cc IIRC] and watching for the call that's supposed to set the >> > allowable schemes. > > >> Hokay so. Extension::ParsePermissions() loops over the permissions. >> 'chrome-extension://fkkgfmjnokdaipgddojcidaofhhadnki/*' is not an API >> permission, so it tries to parse it as a host permission. > > >> const int kAllowedSchemes = CanExecuteScriptEverywhere() ? >> URLPattern::SCHEME_ALL : kValidHostPermissionSchemes; >> URLPattern pattern = URLPattern(kAllowedSchemes); >> URLPattern::ParseResult parse_result = >> pattern.Parse(permission_str); > > >> parse_result = 2 = PARSE_ERROR_INVALID_SCHEME > > >> CanExecuteScriptEverywhere() is false because the api test is a user > > extension, >> >> so kAllowedSchemes = kValidHostPermissionSchemes = 31. >> kValidHostPermissionSchemes = UserScript::kValidUserScriptSchemes | >> URLPattern::SCHEME_CHROMEUI; >> UserScript::kValidUserScriptSchemes = URLPattern::SCHEME_HTTP | >> URLPattern::SCHEME_HTTPS | >> URLPattern::SCHEME_FILE | >> URLPattern::SCHEME_FTP; > > >> URLPattern::SCHEME_EXTENSION = 1 << 5 = 32. > > >> I skimmed through the revision log for extension.cc to see if i could see >> when >> SCHEME_EXTENSION might have been removed from kValidHostPermissionSchemes; >> nothing jumped out at me, but I probably don't know what to look for. > > > I could add SCHEME_EXTENSION to kValidHostPermissionSchemes, but I suspect > that > either that isn't the right place to add it or it was removed for a reason. > Also > this looks like somewhat tricky code. (Compare to the downloads code, which > ranks "extremely tricky". :-) I would feel much more comfortable if somebody > who > knew what they were doing suggested the right way to fix this. Yeah, that's not going to be me. I suspect the reason that chrome-extension isn't a valid permission is that we don't want to give one extension blanket permission to grab files out of other extensions. Any extension already has permission to refer to its own files, so there's no need to list it here. If you can refer to your own files, you should be able to download them too, of course, but I don't know how that permission should get communicated. > > > >> > >> > > >> > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... >> >> > > File chrome/test/data/extensions/api_test/downloads/manifest.json >> > > (right): >> > > >> > > >> > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... >> >> > > chrome/test/data/extensions/api_test/downloads/manifest.json:15: >> > "filesystem:*", >> > > On 2012/05/03 17:14:55, ericu wrote: >> > > > I don't think you should need either mention of filesystem: URLs in >> > > > the >> > > > manifest. Support for http://*/* should automatically grant >> > > > filesystem:http://*/*, etc. >> > > >> > > Done. >> > > >> > > >> > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... >> >> > > File chrome/test/data/extensions/api_test/downloads/test.js (right): >> > > >> > > >> > > > > http://codereview.chromium.org/10213002/diff/33002/chrome/test/data/extension... >> >> > > chrome/test/data/extensions/api_test/downloads/test.js:22: >> > > window.MozBlobBuilder); >> > > On 2012/05/03 17:14:55, ericu wrote: >> > > > Why MozBlobBuilder? This won't run on Firefox. >> > > >> > > Done. > > > > http://codereview.chromium.org/10213002/
Eric, Mihai, Aaron, PTAL. This is working on my machine, but we'll see what the trybots say. I added a couple short-circuits to Extension::HasHostPermission() and some corresponding expectations to ExtensionScriptAndCaptureVisibleTest.Permissions. LMK if there's a better way to do this, or if it might break some corner case. I also kept data: URLs specifically white-listed in download_extension_api.cc. Thanks, Eric, for explaining inner_url to me!
http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... chrome/common/extensions/extension.cc:3234: if (url.SchemeIs(chrome::kExtensionScheme) && I think you can replace this entire new hunk with: if (url.GetOrigin() == extension_url_.GetOrigin()) return true; That said, it might make more sense to add this to the call site inside the download extension API. I'm nervous about implications of adding this on all other users of host permissions.
I've got nothing to add to what Aaron said; I'll let him take it from here. On Tue, May 8, 2012 at 10:32 PM, <aa@chromium.org> wrote: > > http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... > File chrome/common/extensions/extension.cc (right): > > http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... > chrome/common/extensions/extension.cc:3234: if > (url.SchemeIs(chrome::kExtensionScheme) && > I think you can replace this entire new hunk with: > > if (url.GetOrigin() == extension_url_.GetOrigin()) > return true; > > That said, it might make more sense to add this to the call site inside > the download extension API. I'm nervous about implications of adding > this on all other users of host permissions. > > http://codereview.chromium.org/10213002/
http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... chrome/common/extensions/extension.cc:3234: if (url.SchemeIs(chrome::kExtensionScheme) && On 2012/05/09 05:32:38, Aaron Boodman wrote: > I think you can replace this entire new hunk with: > > if (url.GetOrigin() == extension_url_.GetOrigin()) > return true; > > That said, it might make more sense to add this to the call site inside the > download extension API. I'm nervous about implications of adding this on all > other users of host permissions. If this check were not here, are there multiple places where it would be needed in the download API, or only one?
http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... chrome/common/extensions/extension.cc:3234: if (url.SchemeIs(chrome::kExtensionScheme) && On 2012/05/09 18:28:44, Aaron Boodman wrote: > On 2012/05/09 05:32:38, Aaron Boodman wrote: > > I think you can replace this entire new hunk with: > > > > if (url.GetOrigin() == extension_url_.GetOrigin()) > > return true; > > > > That said, it might make more sense to add this to the call site inside the > > download extension API. I'm nervous about implications of adding this on all > > other users of host permissions. > > If this check were not here, are there multiple places where it would be needed > in the download API, or only one? download() is the only function that will accept and access a URL. Even if there were another function, I could factor it out into a downloads-specific base class or function. The question is whether the origin check belongs in Extension. I'm happy to go with your judgement and put it in DownloadsDownloadFunction instead.
http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... chrome/common/extensions/extension.cc:3234: if (url.SchemeIs(chrome::kExtensionScheme) && On 2012/05/09 18:28:44, Aaron Boodman wrote: > On 2012/05/09 05:32:38, Aaron Boodman wrote: > > I think you can replace this entire new hunk with: > > > > if (url.GetOrigin() == extension_url_.GetOrigin()) > > return true; > > > > That said, it might make more sense to add this to the call site inside the > > download extension API. I'm nervous about implications of adding this on all > > other users of host permissions. > > If this check were not here, are there multiple places where it would be needed > in the download API, or only one? download() is the only function that will accept and access a URL. Even if there were another function, I could factor it out into a downloads-specific base class or function. The question is whether the origin check belongs in Extension. I'm happy to go with your judgement and put it in DownloadsDownloadFunction instead.
On 2012/05/09 21:36:42, benjhayden_chromium wrote: > http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... > File chrome/common/extensions/extension.cc (right): > > http://codereview.chromium.org/10213002/diff/49002/chrome/common/extensions/e... > chrome/common/extensions/extension.cc:3234: if > (url.SchemeIs(chrome::kExtensionScheme) && > On 2012/05/09 18:28:44, Aaron Boodman wrote: > > On 2012/05/09 05:32:38, Aaron Boodman wrote: > > > I think you can replace this entire new hunk with: > > > > > > if (url.GetOrigin() == extension_url_.GetOrigin()) > > > return true; > > > > > > That said, it might make more sense to add this to the call site inside the > > > download extension API. I'm nervous about implications of adding this on all > > > other users of host permissions. > > > > If this check were not here, are there multiple places where it would be > needed > > in the download API, or only one? > > download() is the only function that will accept and access a URL. Even if there > were another function, I could factor it out into a downloads-specific base > class or function. > > The question is whether the origin check belongs in Extension. I'm happy to go > with your judgement and put it in DownloadsDownloadFunction instead. I don't think it belongs. I'd prefer it in DownloadsDownloadyDownloaderFunction
merge
PTAL
lgtm https://chromiumcodereview.appspot.com/10213002/diff/43010/chrome/browser/dow... File chrome/browser/download/download_extension_api.cc (right): https://chromiumcodereview.appspot.com/10213002/diff/43010/chrome/browser/dow... chrome/browser/download/download_extension_api.cc:372: base::DictionaryValue* options = NULL; For future CL: You should change this to use JSON Schema Compiler. It would eliminate a lot of this error-prone manual parsing. See the alarms API for an example of how. https://chromiumcodereview.appspot.com/10213002/diff/43010/chrome/browser/dow... chrome/browser/download/download_extension_api.cc:383: if (!iodata_->url.SchemeIs("data") && Is there a use case for downloading a data URL?
Thanks, Aaron! I'll merge and commit this afternoon if there are no objections. http://codereview.chromium.org/10213002/diff/43010/chrome/browser/download/do... File chrome/browser/download/download_extension_api.cc (right): http://codereview.chromium.org/10213002/diff/43010/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:372: base::DictionaryValue* options = NULL; On 2012/05/14 21:32:13, Aaron Boodman wrote: > For future CL: You should change this to use JSON Schema Compiler. It would > eliminate a lot of this error-prone manual parsing. See the alarms API for an > example of how. That does look handy. Thanks! http://codereview.chromium.org/10213002/diff/43010/chrome/browser/download/do... chrome/browser/download/download_extension_api.cc:383: if (!iodata_->url.SchemeIs("data") && On 2012/05/14 21:32:13, Aaron Boodman wrote: > Is there a use case for downloading a data URL? Many websites use data URLs for images, and an extension may want to download those images. Some people generate data urls in javascript for short audio files. Somebody may want to write an extension for generating and saving sound effects or other snippets of audio. Any kind of small resource can be transmitted or generated as a data url, and an extension may want to download any kind of resource. If download() blocked data urls, then extensions could get around it by writing the data to a blob and downloading the filesystem: URL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10213002/47007
Change committed as 137245 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
