Add schema chrome-extension-resource:// for extension resources

Add schema chrome-extension-resource:// for extension resources. It may be used for bundle JS libraries in future
like Google Analytics JS library.

Mark a test case flaky for OSX, and reland it.

BUG=122987
TEST=unit_tests --gtest_filter=ExtensionFileUtil.*
TEST=browser_tests --gtest_filter=ExtensionResourceRequestPolicyTest*

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

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

[abarth-chromium, 2012-04-11 20:15:33]

Can this scheme be referenced by random web pages?  I didn't see anything to
prevent it, but maybe I missed it.  You can look at how web_accessible_resources
works to see how to stop that.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
File chrome/common/extensions/csp_validator.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource://", true)) {
I can haz unit test?

[abarth-chromium, 2012-04-11 20:16:29]

Also, it should be easy to add tests for this patch.  You want to test that the
resource is loadable from an extension but not from a random web page.

[abarth-chromium, 2012-04-11 20:18:16]

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
File chrome/common/extensions/csp_validator.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource://", true)) {
Note: If an extension uses chrome-extension-resource in its CSP policy, older
versions of Chrome will reject the extension because it won't validate.

[Peng, 2012-04-11 20:23:51]

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
File chrome/common/extensions/csp_validator.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource://", true)) {
On 2012/04/11 20:15:33, abarth wrote:
> I can haz unit test?

I will test it.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/extens...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource://", true)) {
On 2012/04/11 20:18:16, abarth wrote:
> Note: If an extension uses chrome-extension-resource in its CSP policy, older
> versions of Chrome will reject the extension because it won't validate.

That could be a problem. Any idea?

Maybe make the new scheme in the default CSP policy? Because  those libraries
are for extensions, it is not very necessary to limit it.

[Tom Sepez, 2012-04-11 20:31:16]

Just a few nits here.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
chrome/browser/extensions/extension_protocols.cc:329: ExtensionInfoMap*
extension_info_map)
nit: Indentation.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
chrome/browser/extensions/extension_protocols.cc:344: // Creates URLRequestJobs
for extension:// URLs.
chrome-extension-resource:// urls.   Suspect the comment was cut-n-paste from a
place where it's also wrong (extension vs chrome-extension) ???

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
chrome/browser/extensions/extension_protocols.cc:348: LOG(ERROR)<< "host()=" <<
request->url().host()
Debugging?

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/profi...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/profi...
chrome/browser/profiles/profile_io_data.cc:282:
chrome::kExtensionResourceScheme,
A compulsive person would insist on alphabetizing, but the rest of the list
isn't in order.  Or by expected frequency of access.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/resou...
File chrome/browser/resources/ext/analytics/ga.js (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/resou...
chrome/browser/resources/ext/analytics/ga.js:1: (function(){var g=void
0,h=!0,i=null,j=!1,aa=encodeURIComponent,ba=Infinity,ca=setTimeout,da=decodeURIComponent,k=Math;function
ea(a,b){return a.onload=b}function fa(a,b){return a.name=b}
Should the file path not be /ext, but /extensions, (vs /external is a prefix
match), and to keep parallelism with the use of "extensions" in the tree?

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/chrome...
File chrome/common/chrome_paths.h (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/chrome...
chrome/common/chrome_paths.h:90: DIR_RESOURCES_EXT,            // Full path to
the resources/ext.
Comment probably should describe purpose of directory rather than literal path
which is an implementation detail and may rev out underneath.

https://chromiumcodereview.appspot.com/9909019/diff/1001/content/browser/chil...
File content/browser/child_process_security_policy_impl.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/content/browser/chil...
content/browser/child_process_security_policy_impl.cc:226: LOG(ERROR) << "scheme
== " << scheme;
Remove Debugging.  No need to rev this file.

[Peng, 2012-04-11 21:40:12]

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
chrome/browser/extensions/extension_protocols.cc:329: ExtensionInfoMap*
extension_info_map)
On 2012/04/11 20:31:16, Tom Sepez wrote:
> nit: Indentation.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
chrome/browser/extensions/extension_protocols.cc:344: // Creates URLRequestJobs
for extension:// URLs.
On 2012/04/11 20:31:16, Tom Sepez wrote:
> chrome-extension-resource:// urls.   Suspect the comment was cut-n-paste from
a
> place where it's also wrong (extension vs chrome-extension) ???

Done.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/exten...
chrome/browser/extensions/extension_protocols.cc:348: LOG(ERROR)<< "host()=" <<
request->url().host()
On 2012/04/11 20:31:16, Tom Sepez wrote:
> Debugging?

Done.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/profi...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/profi...
chrome/browser/profiles/profile_io_data.cc:282:
chrome::kExtensionResourceScheme,
On 2012/04/11 20:31:16, Tom Sepez wrote:
> A compulsive person would insist on alphabetizing, but the rest of the list
> isn't in order.  Or by expected frequency of access.

I guess is by frequency of access. Done.

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/resou...
File chrome/browser/resources/ext/analytics/ga.js (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/browser/resou...
chrome/browser/resources/ext/analytics/ga.js:1: (function(){var g=void
0,h=!0,i=null,j=!1,aa=encodeURIComponent,ba=Infinity,ca=setTimeout,da=decodeURIComponent,k=Math;function
ea(a,b){return a.onload=b}function fa(a,b){return a.name=b}
On 2012/04/11 20:31:16, Tom Sepez wrote:
> Should the file path not be /ext, but /extensions, (vs /external is a prefix
> match), and to keep parallelism with the use of "extensions" in the tree? 

There is an extensions folder in resources already. How about change ext to
extension_resource?

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/chrome...
File chrome/common/chrome_paths.h (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/chrome/common/chrome...
chrome/common/chrome_paths.h:90: DIR_RESOURCES_EXT,            // Full path to
the resources/ext.
On 2012/04/11 20:31:16, Tom Sepez wrote:
> Comment probably should describe purpose of directory rather than literal path
> which is an implementation detail and may rev out underneath.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/1001/content/browser/chil...
File content/browser/child_process_security_policy_impl.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/1001/content/browser/chil...
content/browser/child_process_security_policy_impl.cc:226: LOG(ERROR) << "scheme
== " << scheme;
On 2012/04/11 20:31:16, Tom Sepez wrote:
> Remove Debugging.  No need to rev this file.

Done.

[miket_OOO, 2012-04-12 18:38:39]

LGTM but see comments.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:346:
PathService::Get(chrome::DIR_RESOURCES_EXTENSION, &path);
Is this the EXT from the other CL? Did you want it to mean "extra" or
"extension"?

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:347: path =
path.Append(request->url().host()).Append(request->url().path());
I'm not sure whether request has already been sanitized or whether it's even a
potential user input, but please make sure it's safe to build a path out of
components taken from it.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:364: return new
ExtensionResourceProtocolHandler();
Is there a reason these arguments need to be in the signature, but unused in the
method body?

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/prof...
chrome/browser/profiles/profile_io_data.cc:480:
profile_params_->extension_info_map));
See earlier comment -- can we get rid of the parameters?

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/reso...
File chrome/browser/resources/extension_resource/analytics/ga.js (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/reso...
chrome/browser/resources/extension_resource/analytics/ga.js:25:
Pc(a,Ic(b,W("__utmv")));nd=!c;return
h},pd=function(a){nd||0<W("__utmb")[u]||(X("__utmd","1",a.c(M,"/"),a.c(Ta,""),1E4),0==W("__utmd")[u]&&a[qa]())};var
sd=function(a){a.get(P)==g?qd(a):a.get(Ib)&&!a.get(wc)?qd(a):a.get(Pb)&&rd(a)},td=function(a){a.get(Vb)&&!a.get(Ob)&&(rd(a),a.set(Tb,a.get(Nb)))},qd=function(a){var
b=a.get(Sa);a.set(Gb,h);a.set(P,za()^dd(a)&2147483647);a.set(Hb,"");a.set(Kb,b);a.set(Lb,b);a.set(Mb,b);a.set(Nb,1);a.set(Ob,h);a.set(Qb,0);a.set(Q,10);a.set(Rb,b);a.set(O,[]);a.set(Ib,j);a.set(Pb,j)},rd=function(a){a.set(Lb,a.get(Mb));a.set(Mb,a.get(Sa));a.p(Nb);a.set(Ob,h);a.set(Qb,0);a.set(Q,10);a.set(Rb,a.get(Sa));a.set(Pb,j)};var
ud="daum:q,eniro:search_word,naver:query,pchome:q,images.google:q,google:q,yahoo:p,yahoo:q,msn:q,bing:q,aol:query,aol:q,lycos:q,lycos:query,ask:q,netscape:query,cnn:query,about:terms,mamma:q,voila:rdata,virgilio:qs,live:q,baidu:wd,alice:qs,yandex:text,najdi:q,seznam:q,rakuten:qt,biglobe:q,goo.ne:MT,wp:szukaj,onet:qt,yam:k,kvasir:q,ozu:q,terra:query,rambler:query,conduit:q,babylon:q,search-results:q,avg:q,comcast:q,incredimail:q,startsiden:q".split(","),Bd=function(a){if(a.get(bb)&&!a.get(wc)){for(var
b=
I like what you did here.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/common/chrom...
File chrome/common/chrome_paths.h (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/common/chrom...
chrome/common/chrome_paths.h:90: DIR_RESOURCES_EXTENSION,      // Full path to
extensoion resources.
typo (extensoion) -- and now that I see your comment, I agree that you meant to
expand EXT to EXTENSION.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:715: // allowed. If not
allowed, we reset the URL to something invalid to prevent
I think this comment should be updated.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:44: << "hosted
app.";
I don't think the wrapping << is necessary.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:48: // Disallow
loading of extension resources which are not explicitely listed
"explicitly"

"which" should be "that"

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:55: // -
chrome-extension:// (for legacy reasons -- some extensions interop)
"interoperate" (avoid abbreviations)

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:65: std::string
message = base::StringPrintf(
This error message should go in the strings file for possible future i18n. See
http://codereview.chromium.org/9834022/diff/8003/chrome/common/extensions/ext...
for an example.

I'm not clear whether this error will be user-visible, and if so, please check
and make sure that resource_url can't possibly come from a user input (XSS
threat).

(I know these error messages aren't yours, but because they're no longer
obviously just error messages, i18n and security are new concerns.)

[Peng, 2012-04-12 20:13:22]

Fixed issues. Please reviews it again. Thanks.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:346:
PathService::Get(chrome::DIR_RESOURCES_EXTENSION, &path);
On 2012/04/12 18:38:39, miket wrote:
> Is this the EXT from the other CL? Did you want it to mean "extra" or
> "extension"?

I renamed it from EXT to extension. Because it is for
chrome-externsion-resource.

And this CL replaces the old CL, because of some bug of codereview web site. I
can not upload the old CL now. :(

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:347: path =
path.Append(request->url().host()).Append(request->url().path());
On 2012/04/12 18:38:39, miket wrote:
> I'm not sure whether request has already been sanitized or whether it's even a
> potential user input, but please make sure it's safe to build a path out of
> components taken from it.

Done

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:364: return new
ExtensionResourceProtocolHandler();
On 2012/04/12 18:38:39, miket wrote:
> Is there a reason these arguments need to be in the signature, but unused in
the
> method body?

I copied & pasted it. I think it may be useful in future. Anyway I removed it
for now.

done

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/prof...
chrome/browser/profiles/profile_io_data.cc:480:
profile_params_->extension_info_map));
On 2012/04/12 18:38:39, miket wrote:
> See earlier comment -- can we get rid of the parameters?

Done.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/reso...
File chrome/browser/resources/extension_resource/analytics/ga.js (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/browser/reso...
chrome/browser/resources/extension_resource/analytics/ga.js:25:
Pc(a,Ic(b,W("__utmv")));nd=!c;return
h},pd=function(a){nd||0<W("__utmb")[u]||(X("__utmd","1",a.c(M,"/"),a.c(Ta,""),1E4),0==W("__utmd")[u]&&a[qa]())};var
sd=function(a){a.get(P)==g?qd(a):a.get(Ib)&&!a.get(wc)?qd(a):a.get(Pb)&&rd(a)},td=function(a){a.get(Vb)&&!a.get(Ob)&&(rd(a),a.set(Tb,a.get(Nb)))},qd=function(a){var
b=a.get(Sa);a.set(Gb,h);a.set(P,za()^dd(a)&2147483647);a.set(Hb,"");a.set(Kb,b);a.set(Lb,b);a.set(Mb,b);a.set(Nb,1);a.set(Ob,h);a.set(Qb,0);a.set(Q,10);a.set(Rb,b);a.set(O,[]);a.set(Ib,j);a.set(Pb,j)},rd=function(a){a.set(Lb,a.get(Mb));a.set(Mb,a.get(Sa));a.p(Nb);a.set(Ob,h);a.set(Qb,0);a.set(Q,10);a.set(Rb,a.get(Sa));a.set(Pb,j)};var
ud="daum:q,eniro:search_word,naver:query,pchome:q,images.google:q,google:q,yahoo:p,yahoo:q,msn:q,bing:q,aol:query,aol:q,lycos:q,lycos:query,ask:q,netscape:query,cnn:query,about:terms,mamma:q,voila:rdata,virgilio:qs,live:q,baidu:wd,alice:qs,yandex:text,najdi:q,seznam:q,rakuten:qt,biglobe:q,goo.ne:MT,wp:szukaj,onet:qt,yam:k,kvasir:q,ozu:q,terra:query,rambler:query,conduit:q,babylon:q,search-results:q,avg:q,comcast:q,incredimail:q,startsiden:q".split(","),Bd=function(a){if(a.get(bb)&&!a.get(wc)){for(var
b=
On 2012/04/12 18:38:39, miket wrote:
> I like what you did here.

:) done

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/common/chrom...
File chrome/common/chrome_paths.h (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/common/chrom...
chrome/common/chrome_paths.h:90: DIR_RESOURCES_EXTENSION,      // Full path to
extensoion resources.
On 2012/04/12 18:38:39, miket wrote:
> typo (extensoion) -- and now that I see your comment, I agree that you meant
to
> expand EXT to EXTENSION.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:715: // allowed. If not
allowed, we reset the URL to something invalid to prevent
On 2012/04/12 18:38:39, miket wrote:
> I think this comment should be updated.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:44: << "hosted
app.";
On 2012/04/12 18:38:39, miket wrote:
> I don't think the wrapping << is necessary.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:48: // Disallow
loading of extension resources which are not explicitely listed
On 2012/04/12 18:38:39, miket wrote:
> "explicitly"
> 
> "which" should be "that"

Done.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:55: // -
chrome-extension:// (for legacy reasons -- some extensions interop)
On 2012/04/12 18:38:39, miket wrote:
> "interoperate" (avoid abbreviations)

Done.

https://chromiumcodereview.appspot.com/9909019/diff/19002/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:65: std::string
message = base::StringPrintf(
On 2012/04/12 18:38:39, miket wrote:
> This error message should go in the strings file for possible future i18n. See
>
http://codereview.chromium.org/9834022/diff/8003/chrome/common/extensions/ext...
> for an example.
> 
> I'm not clear whether this error will be user-visible, and if so, please check
> and make sure that resource_url can't possibly come from a user input (XSS
> threat).
> 
> (I know these error messages aren't yours, but because they're no longer
> obviously just error messages, i18n and security are new concerns.)

The message will go to javascript console for developers. Probably non-i18n is
better.

[Peng, 2012-04-12 20:13:22]

Fixed issues. Please reviews it again. Thanks.

[miket_OOO, 2012-04-12 20:52:02]

lgtm

[Peng, 2012-04-13 03:22:53]

On 2012/04/12 20:52:02, miket wrote:
> lgtm

Tom & abarth, could please you review this CL from security aspect again? Thanks

[abarth-chromium, 2012-04-13 17:11:24]

This all looks fine, but as far as I can tell this patch has no testing.  There
are a bunch of things that deserve unit tests, and we should have one
integration test that shows that an extension can actually load ga.js via this
mechanism.  These tests should be easy to write.  Please add them before
landing.

http://codereview.chromium.org/9909019/diff/14020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/9909019/diff/14020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_protocols.cc:355: full_path = FilePath();
This is an important security check, and I'm glad to see it here.  Can we add a
unit test to make sure that it continues to function as expected?

http://codereview.chromium.org/9909019/diff/14020/chrome/common/extensions/cs...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/9909019/diff/14020/chrome/common/extensions/cs...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource://", true)) {
This file has a nice unit testing along with it.  Please add a unit test for
this change.

http://codereview.chromium.org/9909019/diff/14020/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (left):

http://codereview.chromium.org/9909019/diff/14020/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:710:
!ExtensionResourceRequestPolicy::CanRequestResource(
This check is also important for security.  Can you add some testing to make
sure it works the way we expect?

http://codereview.chromium.org/9909019/diff/14020/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/9909019/diff/14020/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:21: bool
ExtensionResourceRequestPolicy::CanRequestResource(
This function is getting very complicated.  Are there tests for it?  Can we add
some cases for the new functionality?

http://codereview.chromium.org/9909019/diff/14020/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:81: GURL
page_url = frame->top()->document().url();
It looks like the page_url variable is unused.  Perhaps remove?

[Tom Sepez, 2012-04-13 17:20:50]

Absolutely must have new tests that show:
- Extensions can't perform arbitrary directory traversal '..'.
- chrome-extension-resources can't be loaded from http:// pages.

When someone changes something down the road that breaks one of these rules, we
need to know about it rather than just having chrome silently become vulnerable.

http://codereview.chromium.org/9909019/diff/14020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/9909019/diff/14020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_protocols.cc:348:
extension_file_util::ExtensionURLToRelativeFilePath(request->url());
Is this where we count on unsafe directory traversal being blocked?  Why do we
know that this will happen?

http://codereview.chromium.org/9909019/diff/14020/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/9909019/diff/14020/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:91: // used by interal pages).
This comment goes with the one the line before.

[miket_OOO, 2012-04-13 17:41:20]

> When someone changes something down the road that breaks one of these rules,
we
> need to know about it rather than just having chrome silently become
vulnerable.

Peng, this feedback is 1,000x better than my "please make sure [not the product
of user input]" comments earlier in the review. A manual review is just a
snapshot of the current state of the code, but a test catches future refactoring
and enhancements that introduce new sources of trouble, any of which might occur
far away from this code.

Tom, thanks for highlighting this point. I'll improve my review comments in the
future to ask for tests, not just inspection, on security issues.

[Tom Sepez, 2012-04-13 18:59:22]

On 2012/04/13 17:41:20, miket wrote:
> > When someone changes something down the road that breaks one of these rules,
> we
> > need to know about it rather than just having chrome silently become
> vulnerable.

Assuming, of course, that they don't just disable the test so they can check in.
 The super-paranoid put comments in their tests along the lines that flunking
the test means you have a security issue.

[miket_OOO, 2012-04-13 19:16:16]

> Assuming, of course, that they don't just disable the test so they can check
in.
>  The super-paranoid put comments in their tests along the lines that flunking
> the test means you have a security issue.

The most paranoid of all write parallel sets of tests that don't fail but
instead page the Security Team. I'll start working on that. You can thank me
later!

[Aaron Boodman, 2012-04-16 18:54:36]

Please do not add this new permission. We've had a lot of trouble with these
chrome:// permissions before, and we shouldn't add more of them.

I don't think a permission is required here at all.

http://codereview.chromium.org/9909019/diff/11003/chrome/common/extensions/ex...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/9909019/diff/11003/chrome/common/extensions/ex...
chrome/common/extensions/extension_file_util.cc:629: std::string host =
net::UnescapeURLComponent(url.host(),
This is duplicating a lot of code from above. Can you combine it?

http://codereview.chromium.org/9909019/diff/11003/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/9909019/diff/11003/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:79: if
(resource_url.SchemeIs(chrome::kExtensionResourceScheme)) {
Split these two cases into two separate helper functions.

http://codereview.chromium.org/9909019/diff/11003/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:83:
!frame_url.SchemeIs(chrome::kExtensionResourceScheme)) {
Currently I don't think there are any cases where a resource embeds another
resource, so maybe remove this bit.

http://codereview.chromium.org/9909019/diff/11003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/accessible_cer/manifest.json (right):

http://codereview.chromium.org/9909019/diff/11003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:17:
"chrome-extension-resource://analytics"
We don't need a permission for this. Also, perhaps it would be better to use
just chrome-resource for the scheme, to not couple it to extensions.

[Aaron Boodman, 2012-04-16 18:56:15]

http://codereview.chromium.org/9909019/diff/11003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/accessible_cer/manifest.json (right):

http://codereview.chromium.org/9909019/diff/11003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:4:
"manifest_version": 2,
We should make the default security policy for manifest_version=2 allow
chrome-resource://.

http://codereview.chromium.org/9909019/diff/11003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:17:
"chrome-extension-resource://analytics"
On 2012/04/16 18:54:36, Aaron Boodman wrote:
> We don't need a permission for this. Also, perhaps it would be better to use
> just chrome-resource for the scheme, to not couple it to extensions.

Actually, I don't see this getting checked anywhere in the patch. Is it just old
test data that didn't get removed?

[Aaron Boodman, 2012-04-16 18:59:57]

http://codereview.chromium.org/9909019/diff/11003/chrome/common/extensions/ex...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/9909019/diff/11003/chrome/common/extensions/ex...
chrome/common/extensions/extension_file_util.cc:629: std::string host =
net::UnescapeURLComponent(url.host(),
On 2012/04/16 18:54:36, Aaron Boodman wrote:
> This is duplicating a lot of code from above. Can you combine it?

Actually, on second thought, it is a bit weird to have all this support for
chrome-resource URLs in extension_file_util and extension_protocols.

The rest of those files specifically deal with the chrome-extension:// scheme.

Is it possible to refactor some of the code shared here between
ExtensionResourceURLToFilePath and ExtensionURLToFilePath into file_util.h, and
then just implement a separate module for implementing the chrome-resource
scheme, rather than reusing extension_protocols.*?

[Peng, 2012-04-17 13:52:05]

CL is updated. Please review it again. Thanks.

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/browser/exte...
File chrome/browser/extensions/extension_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:348:
extension_file_util::ExtensionURLToRelativeFilePath(request->url());
On 2012/04/13 17:20:51, Tom Sepez wrote:
> Is this where we count on unsafe directory traversal being blocked?  Why do we
> know that this will happen?

This function just decode the escape and get the relative file path.

The code for blocking direct traversal is between 352  and 356 of this
reversion.

In the next reversion, I will move this logic into
ExtensionResourceURLToFilePath().

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/browser/exte...
chrome/browser/extensions/extension_protocols.cc:355: full_path = FilePath();
On 2012/04/13 17:11:24, abarth wrote:
> This is an important security check, and I'm glad to see it here.  Can we add
a
> unit test to make sure that it continues to function as expected?

Done.

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/common/chrom...
File chrome/common/chrome_paths.h (right):

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/common/chrom...
chrome/common/chrome_paths.h:91: // used by interal pages).
On 2012/04/13 17:20:51, Tom Sepez wrote:
> This comment goes with the one the line before.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/common/exten...
File chrome/common/extensions/csp_validator.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/common/exten...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource://", true)) {
On 2012/04/13 17:11:24, abarth wrote:
> This file has a nice unit testing along with it.  Please add a unit test for
> this change.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (left):

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:710:
!ExtensionResourceRequestPolicy::CanRequestResource(
On 2012/04/13 17:11:24, abarth wrote:
> This check is also important for security.  Can you add some testing to make
> sure it works the way we expect?

I think it need add browser some tests for it. Working on it.

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/renderer/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:21: bool
ExtensionResourceRequestPolicy::CanRequestResource(
On 2012/04/13 17:11:24, abarth wrote:
> This function is getting very complicated.  Are there tests for it?  Can we
add
> some cases for the new functionality?

This function's input & output highly depend on environment. For example: this
function need get input from WebFrame, and send some output to WebFrame. I
didn't find a good way to create a unit test for this function. :(
Probably I can do is adding a browser_test for it. Do you think it is OK?

https://chromiumcodereview.appspot.com/9909019/diff/14020/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:81: GURL
page_url = frame->top()->document().url();
On 2012/04/13 17:11:24, abarth wrote:
> It looks like the page_url variable is unused.  Perhaps remove?

Done.

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/common/exten...
File chrome/common/extensions/extension_file_util.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/common/exten...
chrome/common/extensions/extension_file_util.cc:629: std::string host =
net::UnescapeURLComponent(url.host(),
On 2012/04/16 18:59:57, Aaron Boodman wrote:
> On 2012/04/16 18:54:36, Aaron Boodman wrote:
> > This is duplicating a lot of code from above. Can you combine it?
> 
> Actually, on second thought, it is a bit weird to have all this support for
> chrome-resource URLs in extension_file_util and extension_protocols.
> 
> The rest of those files specifically deal with the chrome-extension:// scheme.
> 
> Is it possible to refactor some of the code shared here between
> ExtensionResourceURLToFilePath and ExtensionURLToFilePath into file_util.h,
and
> then just implement a separate module for implementing the chrome-resource
> scheme, rather than reusing extension_protocols.*?

* Remove duplicated code.
* Handle C-E-R:// in a new extension_resource_protocols.{h,cc}

Done.

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/renderer/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:79: if
(resource_url.SchemeIs(chrome::kExtensionResourceScheme)) {
On 2012/04/16 18:54:36, Aaron Boodman wrote:
> Split these two cases into two separate helper functions.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:83:
!frame_url.SchemeIs(chrome::kExtensionResourceScheme)) {
On 2012/04/16 18:54:36, Aaron Boodman wrote:
> Currently I don't think there are any cases where a resource embeds another
> resource, so maybe remove this bit.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/accessible_cer/manifest.json (right):

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:4:
"manifest_version": 2,
On 2012/04/16 18:56:15, Aaron Boodman wrote:
> We should make the default security policy for manifest_version=2 allow
> chrome-resource://.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/11003/chrome/test/data/ex...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:17:
"chrome-extension-resource://analytics"
On 2012/04/16 18:56:15, Aaron Boodman wrote:
> On 2012/04/16 18:54:36, Aaron Boodman wrote:
> > We don't need a permission for this. Also, perhaps it would be better to use
> > just chrome-resource for the scheme, to not couple it to extensions.
> 
> Actually, I don't see this getting checked anywhere in the patch. Is it just
old
> test data that didn't get removed?

Done.

[Aaron Boodman, 2012-04-18 17:52:24]

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/cs...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/cs...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource:", true)) {
Why not "//" here too?

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/ex...
chrome/common/extensions/extension_file_util.cc:639:
base::ThreadRestrictions::ScopedAllowIO allow_io;
This shouldn't be needed. If you are hitting a DCHECK here, consider removing
the PathExists() and AbsolutePath() calls.

http://codereview.chromium.org/9909019/diff/48001/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/url_constants...
chrome/common/url_constants.h:348: extern const char kExtensionResourceScheme[];
Add a comment to be consistent with the ones above, even though it's kinda
silly.

[Tom Sepez, 2012-04-18 18:03:01]

Tests seem OK, just a few nits and one worry.  Thanks.

http://codereview.chromium.org/9909019/diff/48001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_resource_protocols.h (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_resource_protocols.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
Nit: 2012

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/cs...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/cs...
chrome/common/extensions/csp_validator.cc:48: continue;
There has needed to be a comment here for some time.  There's some trickery
going on here to prevent wildcarding entire schemes.  CSP itself treats scheme:
and scheme:// differently.  The former is a wildcard for the entire scheme, the
latter is the specific URL with no other components and does not match any other
scheme: URL (I didn't write the spec).

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
Worry about promoting a c-e-r:// scheme to a c-e:// scheme on error.

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/accessible_cer/main.html (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/accessible_cer/main.html:6: <!--
This can be completely removed, no?

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessabile_chrome_resource_scheme.html
(right):

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessabile_chrome_resource_scheme.html:5:
<script type="text/javascript">
Isn't this just replaced by the same src="" as in the other test?

[Aaron Boodman, 2012-04-18 18:06:20]

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
On 2012/04/18 18:03:01, Tom Sepez wrote:
> Worry about promoting a c-e-r:// scheme to a c-e:// scheme on error.

Theoretically, this URL should have no privileges, but yeah, it's a hack and I
can see there being problems.

The real problem is that the underlying WebKit interfaces don't allow us to make
the request fail here.

Is there a better URL we can redirect to here?

[Tom Sepez, 2012-04-18 18:24:38]

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
Maybe you invent c-e-r://null and check for that in the URLRequest logic,
returning '' in that case?

[Peng, 2012-04-18 18:29:16]

http://codereview.chromium.org/9909019/diff/48001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_resource_protocols.h (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_resource_protocols.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/04/18 18:03:01, Tom Sepez wrote:
> Nit: 2012

Done.

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/cs...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/cs...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource:", true)) {
On 2012/04/18 17:52:24, Aaron Boodman wrote:
> Why not "//" here too?
Because I tried "CER://" here and default CSP, it doesn't work.

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/ex...
chrome/common/extensions/extension_file_util.cc:639:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/04/18 17:52:24, Aaron Boodman wrote:
> This shouldn't be needed. If you are hitting a DCHECK here, consider removing
> the PathExists() and AbsolutePath() calls.

I are hitting a DCHECK here. PathExists() and AbsolutePath() are necessary. If
remove it here, I need call them in caller of this function. What's do you
think?

http://codereview.chromium.org/9909019/diff/48001/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/url_constants...
chrome/common/url_constants.h:348: extern const char kExtensionResourceScheme[];
On 2012/04/18 17:52:24, Aaron Boodman wrote:
> Add a comment to be consistent with the ones above, even though it's kinda
> silly.

Done.

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
On 2012/04/18 18:06:21, Aaron Boodman wrote:
> On 2012/04/18 18:03:01, Tom Sepez wrote:
> > Worry about promoting a c-e-r:// scheme to a c-e:// scheme on error.
> 
> Theoretically, this URL should have no privileges, but yeah, it's a hack and I
> can see there being problems.
> 
> The real problem is that the underlying WebKit interfaces don't allow us to
make
> the request fail here.
> 
> Is there a better URL we can redirect to here?

Tom, Aaron, Any suggestion? I don't know any other better options.

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/accessible_cer/main.html (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/accessible_cer/main.html:6: <!--
On 2012/04/18 18:03:01, Tom Sepez wrote:
> This can be completely removed, no?

Done.

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessabile_chrome_resource_scheme.html
(right):

http://codereview.chromium.org/9909019/diff/48001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessabile_chrome_resource_scheme.html:5:
<script type="text/javascript">
On 2012/04/18 18:03:01, Tom Sepez wrote:
> Isn't this just replaced by the same src="" as in the other test?
Yes. Another test is an extension. This on is a web page. Probably can not use
the same js file, except creating symbol link in this folder?

[Peng, 2012-04-18 18:35:12]

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
On 2012/04/18 18:24:38, Tom Sepez wrote:
> Maybe you invent c-e-r://null and check for that in the URLRequest logic,
> returning '' in that case?

Only requesting from web page will be redirected to c-e-r://null. probably
should return some error instead of '' empty page?

[Peng, 2012-04-18 18:47:32]

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
On 2012/04/18 18:24:38, Tom Sepez wrote:
> Maybe you invent c-e-r://null and check for that in the URLRequest logic,
> returning '' in that case?

Done.

[Aaron Boodman, 2012-04-18 18:51:01]

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/common/extensions/ex...
chrome/common/extensions/extension_file_util.cc:639:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/04/18 18:29:16, Peng wrote:
> On 2012/04/18 17:52:24, Aaron Boodman wrote:
> > This shouldn't be needed. If you are hitting a DCHECK here, consider
removing
> > the PathExists() and AbsolutePath() calls.
> 
> I are hitting a DCHECK here. PathExists() and AbsolutePath() are necessary. If
> remove it here, I need call them in caller of this function. What's do you
> think?
> 

I see. I remember the background on why this is needed now. Nevermind.

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
On 2012/04/18 18:24:38, Tom Sepez wrote:
> Maybe you invent c-e-r://null and check for that in the URLRequest logic,
> returning '' in that case?

How is that different from what we do now?

Can we use the Chrome 404 page instead? I don't know what its URL is, but
there's probably a constant somewhere. And presumably we're extra careful to
make sure it doesn't have any privileges.

[Aaron Boodman, 2012-04-18 19:13:49]

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/9909019/diff/48001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:721:
(url.SchemeIs(chrome::kExtensionResourceScheme) &&
On 2012/04/18 18:51:01, Aaron Boodman wrote:
> On 2012/04/18 18:24:38, Tom Sepez wrote:
> > Maybe you invent c-e-r://null and check for that in the URLRequest logic,
> > returning '' in that case?
> 
> How is that different from what we do now?
> 
> Can we use the Chrome 404 page instead? I don't know what its URL is, but
> there's probably a constant somewhere. And presumably we're extra careful to
> make sure it doesn't have any privileges.

I discussed with peng offline. We are going to use
chrome-extension-resource://invalid. This results in a page that has the origin:
data:text/html,chromewebdata, which should be very low privilege.

[Tom Sepez, 2012-04-18 19:33:38]

> We are going to use chrome-extension-resource://invalid. This results in a
page that
> has the origin: data:text/html,chromewebdata, which should be very low
privilege.
SGTM.
The rest of the fixes LGTM, but I'm not an OWNER.

[Peng, 2012-04-19 15:11:49]

Hi Sky, James, Could you please review this CL? Thanks.

[sky, 2012-04-19 15:54:00]

LGTM

[miket_OOO, 2012-04-19 18:21:07]

I have not re-reviewed because I subsequent reviews are sufficiently thorough.
Rubber-stamp LGTM.

[commit-bot: I haz the power, 2012-04-19 18:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/47005

[commit-bot: I haz the power, 2012-04-19 18:24:26]

Can't process patch for file
chrome/browser/resources/extension_resource/analytics/ga.js.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2012-04-19 18:30:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/54001

[commit-bot: I haz the power, 2012-04-19 18:30:03]

Can't process patch for file
chrome/browser/resources/extension_resource/analytics/ga.js.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2012-04-19 19:00:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/56002

[commit-bot: I haz the power, 2012-04-19 19:44:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/56002

[commit-bot: I haz the power, 2012-04-19 20:11:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/56002

[commit-bot: I haz the power, 2012-04-19 20:24:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/56002

[Peng, 2012-05-16 15:11:21]

Update this CL.
I removed GA library from this CL. This CL is only for adding a new schema
chrome-extension-resource:// now.

[miket_OOO, 2012-05-16 16:21:41]

On 2012/05/16 15:11:21, Peng wrote:
> Update this CL.
> I removed GA library from this CL. This CL is only for adding a new schema
> chrome-extension-resource:// now.

Reviewed only files changed since sky's most recent LGTM. LGTM.

[commit-bot: I haz the power, 2012-05-16 16:30:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/73002

[commit-bot: I haz the power, 2012-05-16 16:31:04]

Presubmit check for 9909019-73002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  chrome/test/data/extensions/api_test/accessible_cer/background.js

New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    chrome/common/extensions/extension_file_util.cc:649

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/resources
    chrome/browser/profiles

Presubmit checks took 5.3s to calculate.

[Peng, 2012-05-16 17:33:02]

Hi arv, Could you please review code under chrome/browser/resources?

Hi Miranda, Could you please review code under chrome/browser/profiles?

Thanks.

[Mihai Parparita -not on Chrome, 2012-05-16 23:17:43]

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/browser/exte...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/browser/exte...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:202:
"web_accessible/nonaccessabile_chrome_resource_scheme.html"));
Typo ("nonaccessabile")

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/browser/exte...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:222: #if
defined(OS_WIN)
Why is this disabled on Windows?

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/common/exten...
File chrome/common/extensions/csp_validator.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/common/exten...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource:", true)) {
The other comparisons have a // at the end, do we not want that here?

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/renderer/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:88: const
ExtensionSet* loaded_extensions) {
The loaded_extensions parameter is unused.

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/accessible_cer/manifest.json (right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/test/data/ex...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:6:
"platform_app": true,
Does this need to be a platform app? It can just be a regular extension.

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/test/data/ex...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:10: "app": {
BTW, the app/launch/container section of the manifest is no longer used for
platform apps.

[Peng, 2012-05-17 14:49:57]

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/browser/exte...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/browser/exte...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:202:
"web_accessible/nonaccessabile_chrome_resource_scheme.html"));
On 2012/05/16 23:17:44, Mihai Parparita wrote:
> Typo ("nonaccessabile")

Done.

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/browser/exte...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:222: #if
defined(OS_WIN)
On 2012/05/16 23:17:44, Mihai Parparita wrote:
> Why is this disabled on Windows?
Done

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/common/exten...
File chrome/common/extensions/csp_validator.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/common/exten...
chrome/common/extensions/csp_validator.cc:47: StartsWithASCII(source,
"chrome-extension-resource:", true)) {
On 2012/05/16 23:17:44, Mihai Parparita wrote:
> The other comparisons have a // at the end, do we not want that here?
Yes. Because we would like to support C-E-R: in CSP, otherwise, we have to add
C-E-R://{library_names} for each bundled libraries.
Done

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/renderer/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/renderer/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:88: const
ExtensionSet* loaded_extensions) {
On 2012/05/16 23:17:44, Mihai Parparita wrote:
> The loaded_extensions parameter is unused.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/accessible_cer/manifest.json (right):

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/test/data/ex...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:6:
"platform_app": true,
On 2012/05/16 23:17:44, Mihai Parparita wrote:
> Does this need to be a platform app? It can just be a regular extension.
Done

https://chromiumcodereview.appspot.com/9909019/diff/71008/chrome/test/data/ex...
chrome/test/data/extensions/api_test/accessible_cer/manifest.json:10: "app": {
On 2012/05/16 23:17:44, Mihai Parparita wrote:
> BTW, the app/launch/container section of the manifest is no longer used for
> platform apps.

Done.

[Mihai Parparita -not on Chrome, 2012-05-17 15:08:07]

LGTM

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/browser/exte...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/browser/exte...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:19:
command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis);
These two flags should no longer be necessary now that you're no longer loading
a platform app for this test.

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/test/data/ex...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessible_chrome_resource_scheme.html
(right):

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/test/data/ex...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessible_chrome_resource_scheme.html:6:
if (typeof(_gaq) == 'undefined') {
Shouldn't this be testing for the type of _demo (since that's what the demo
library defines)?

[Peng, 2012-05-17 15:32:35]

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/browser/exte...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/browser/exte...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:19:
command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis);
On 2012/05/17 15:08:07, Mihai Parparita wrote:
> These two flags should no longer be necessary now that you're no longer
loading
> a platform app for this test.

Done.

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/test/data/ex...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessible_chrome_resource_scheme.html
(right):

https://chromiumcodereview.appspot.com/9909019/diff/71009/chrome/test/data/ex...
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/nonaccessible_chrome_resource_scheme.html:6:
if (typeof(_gaq) == 'undefined') {
On 2012/05/17 15:08:07, Mihai Parparita wrote:
> Shouldn't this be testing for the type of _demo (since that's what the demo
> library defines)?

Done.

[James Hawkins, 2012-05-17 17:51:13]

resources/ LGTM

[Elliot Glaysher, 2012-05-17 18:20:05]

profiles lgtm

[commit-bot: I haz the power, 2012-05-17 18:21:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/76003

[commit-bot: I haz the power, 2012-05-17 18:22:24]

Presubmit check for 9909019-76003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    chrome/common/extensions/extension_file_util.cc:649

Presubmit checks took 4.1s to calculate.

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

[commit-bot: I haz the power, 2012-05-17 19:13:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/69033

[commit-bot: I haz the power, 2012-05-17 19:13:54]

Presubmit check for 9909019-69033 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    chrome/browser/extensions/extension_resource_protocols.cc:39

Presubmit checks took 4.3s to calculate.

[Elliot Glaysher, 2012-05-17 19:23:40]

https://chromiumcodereview.appspot.com/9909019/diff/69033/chrome/browser/exte...
File chrome/browser/extensions/extension_resource_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/69033/chrome/browser/exte...
chrome/browser/extensions/extension_resource_protocols.cc:39:
base::ThreadRestrictions::ScopedAllowIO allow_io;
The presubmit check is doing its job by complaining about this line.

[Peng, 2012-05-17 19:26:11]

https://chromiumcodereview.appspot.com/9909019/diff/69033/chrome/browser/exte...
File chrome/browser/extensions/extension_resource_protocols.cc (right):

https://chromiumcodereview.appspot.com/9909019/diff/69033/chrome/browser/exte...
chrome/browser/extensions/extension_resource_protocols.cc:39:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2012/05/17 19:23:41, Elliot Glaysher wrote:
> The presubmit check is doing its job by complaining about this line.

I can not find a easy way to fix it. :( Do you have any good idea?

[Mihai Parparita -not on Chrome, 2012-05-17 19:40:27]

On Thu, May 17, 2012 at 12:26 PM, <penghuang@chromium.org> wrote:

> I can not find a easy way to fix it. :( Do you have any good idea?
>

The most expedient solution may be to give up on allowing arbitrary paths
in ExtensionResourceURLToFilePath, and have a whitelist of known resources
to on-disk paths, and reject everything else?

Mihai

[Peng, 2012-05-17 21:40:01]

On 2012/05/17 19:40:27, Mihai Parparita wrote:
> On Thu, May 17, 2012 at 12:26 PM, <mailto:penghuang@chromium.org> wrote:
> 
> > I can not find a easy way to fix it. :( Do you have any good idea?
> >
> 
> The most expedient solution may be to give up on allowing arbitrary paths
> in ExtensionResourceURLToFilePath, and have a whitelist of known resources
> to on-disk paths, and reject everything else?
> 
> Mihai

Fixed it by inheriting net::URLRequestFileJob, and resolving file_path_ in this
Job class.

[commit-bot: I haz the power, 2012-05-17 21:43:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/78003

[commit-bot: I haz the power, 2012-05-17 21:49:39]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-05-17 23:12:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/78003

[commit-bot: I haz the power, 2012-05-18 01:50:23]

Change committed as 137794

[commit-bot: I haz the power, 2012-05-18 17:53:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/9909019/78005

[commit-bot: I haz the power, 2012-05-18 19:09:22]

Change committed as 137918

[Steve McKay, 2013-06-13 20:54:05]

On 2012/05/18 19:09:22, I haz the power (commit-bot) wrote:
> Change committed as 137918

This patch is referenced from a privacy review for an aborted effort
(https://code.google.com/p/chromium/issues/detail?id=123803). As that project
was aborted can this change be reverted? Or is this functionality now being used
for other purposes?