Create a proprietary scheme for loading web-accessible resources.

Create a proprietary scheme for loading web-accessible resources.

This adds a chrome-resource:// scheme which is web-accessible and exempt from
CSP, like chrome-extension:// web-accessible resources. This is intended to be
used to expose image and style resources from Blink to web content, permitting
new features to use a similar loading process to author content.

No resources are currently exposed; they will be added in subsequent CLs.

TEST=content_unittests:ResourceProtocolHandlerTest.*
BUG=414849

[jbroman, 2014-11-14 18:46:34]

jbroman@chromium.org changed reviewers:
+ mkwst@chromium.org

[jbroman, 2014-11-14 18:46:35]

Hey, here's the chrome-resource:// CL I mentioned. It's gained a little bit of
complexity to support the unit test, but this should still be a reasonable size
for a single CL.

I've called out a couple things I'm not certain about (because I mostly write
Blink code).

If this LGTY, then I'll try and find the necessary owners to land it.

https://codereview.chromium.org/647853002/diff/390001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/647853002/diff/390001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:124: static const
char* protocol_handlers_to_copy[] = {
I'm not sure why android_webview whitelists protocols to be supported here, as
opposed to copying everything from protocol_handlers.

https://codereview.chromium.org/647853002/diff/390001/content/browser/resourc...
File content/browser/resource_protocol.cc (right):

https://codereview.chromium.org/647853002/diff/390001/content/browser/resourc...
content/browser/resource_protocol.cc:62:
->GetDataResource(resource_info_.resource_id,
It seems to be okay to call both content::GetContentClient and
content::ContentClient::GetDataResource from this thread, but I can't find
documentation to make me confident of this. Is this correct?

[Mike West, 2014-11-15 13:28:12]

mkwst@chromium.org changed reviewers:
+ tsepez@chromium.org

[Mike West, 2014-11-15 13:28:13]

I won't be able to look at this in any detail until Monday. Adding Tom, as he's
probably the first person I'd talk to about protocols, etc. from the security
side.

[Tom Sepez, 2014-11-17 18:09:11]

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
File content/browser/resource_protocol.cc (right):

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.cc:32: static const char kBlinkHostname[] =
"blink";
nit: static redundant if we're in empty namespace {}.

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.cc:66: *data = GetContentClient()
nit: -> on previous line, indentation dubious.

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.cc:122: WebAccessibleResource& resource_info =
resources_[std::make_pair(host, path)];
Do we care about detecting duplicates?  Probably fine to just overwrite.

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
File content/browser/resource_protocol.h (right):

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.h:30: public:
nit: duplicate public:

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
File content/browser/resource_protocol_unittest.cc (right):

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol_unittest.cc:34: else
nit: else after return.

https://codereview.chromium.org/647853002/diff/410001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/647853002/diff/410001/content/renderer/render...
content/renderer/render_thread_impl.cc:983:
WebSecurityPolicy::registerURLSchemeAsBypassingContentSecurityPolicy(
Do we need to bypass CSP?  For a CSP-enabled web page that uses this feature,
I'd expect the page to explicitly list 'chrome-resources:' as part of their
policy.  For CSP-enabled pages that don't use this feature, it would be a shame
to allow them to be vulnerable by default to flaws in some chrome-resource://
script.

[jbroman, 2014-11-17 18:35:17]

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
File content/browser/resource_protocol.cc (right):

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.cc:32: static const char kBlinkHostname[] =
"blink";
On 2014/11/17 18:09:11, Tom Sepez wrote:
> nit: static redundant if we're in empty namespace {}.

Done.

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.cc:66: *data = GetContentClient()
On 2014/11/17 18:09:11, Tom Sepez wrote:
> nit: -> on previous line, indentation dubious.

This is what clang-format (with Chromium style) does. Do you feel strongly?

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.cc:122: WebAccessibleResource& resource_info =
resources_[std::make_pair(host, path)];
On 2014/11/17 18:09:11, Tom Sepez wrote:
> Do we care about detecting duplicates?  Probably fine to just overwrite.

Not sure. One could conceive of embedders wanting to override resources, so
I think this (the trivial implementation) at least does something reasonable.
Maybe at some point we might want to be able to distinguish between accidentally
and intentionally overriding of a resource, but I'm willing to punt on that
until/unless it becomes an issue.

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
File content/browser/resource_protocol.h (right):

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol.h:30: public:
On 2014/11/17 18:09:11, Tom Sepez wrote:
> nit: duplicate public:

You mean this doesn't make it super-extra-public? :)

Done.

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
File content/browser/resource_protocol_unittest.cc (right):

https://codereview.chromium.org/647853002/diff/410001/content/browser/resourc...
content/browser/resource_protocol_unittest.cc:34: else
On 2014/11/17 18:09:11, Tom Sepez wrote:
> nit: else after return.

Done.

https://codereview.chromium.org/647853002/diff/410001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/647853002/diff/410001/content/renderer/render...
content/renderer/render_thread_impl.cc:983:
WebSecurityPolicy::registerURLSchemeAsBypassingContentSecurityPolicy(
On 2014/11/17 18:09:11, Tom Sepez wrote:
> Do we need to bypass CSP?  For a CSP-enabled web page that uses this feature,
> I'd expect the page to explicitly list 'chrome-resources:' as part of their
> policy.  For CSP-enabled pages that don't use this feature, it would be a
shame
> to allow them to be vulnerable by default to flaws in some chrome-resource://
> script.

The goal is to be able to use this scheme to host resources for features the
author may not even be aware of, like the close button graphics in a plugin
placeholder, or some other style resource that's internal to a feature that
the author need not be aware of. I wouldn't expect an author to ever write
"chrome-resource://" in their own document (and would loosely describe the
contract for that scheme as "this is for Chrome's use and valid URLs may
change at any time without warning").

I don't think authors should need to be aware of implementation details of
each user agent to avoid strange breakage of UA features that are supposed
to be, from their perspective, entirely encapsulated.

I don't intend to put scripts there, just stylesheets (for use in UA shadow
roots) and images. I can make the warning comment at the registration sites
more specific if that's helpful. I agree that some caution would have to be
used before a resource is added to a CSP-bypassing scheme.

I see this as analogous to chrome-extension:// web-accessible resources, which
the author doesn't know about.

Is that clear and reasonable, or have I missed your point?

[Tom Sepez, 2014-11-17 19:26:15]

> Is that clear and reasonable, or have I missed your point?
Yes, its clear. Just dangerous.  The larger design principle that it violates is
that something (the entire web in this instance) shouldn't be able to get burned
by something it doesn't need to use.

So if the author isn't aware, how does it get loaded?  Via some other JS/HTML
from some other place?  Or is this to support something like blink in JS?

[Tom Sepez, 2014-11-17 19:43:01]

Perhaps the underlying issue is that schemeShouldBypassContentSecurityPolicy()
is too big a hammer.  If it were more granular so that we could bypass it only
for image-src and others, and not script-src etc. then I wouldn't have any
heartburn about approving this.

Mike, do you think that's a reasonable improvement we could make to the way CSP
internals work?

[Mike West, 2014-11-17 19:55:37]

On 2014/11/17 19:43:01, Tom Sepez wrote:
> Perhaps the underlying issue is that schemeShouldBypassContentSecurityPolicy()
> is too big a hammer.  If it were more granular so that we could bypass it only
> for image-src and others, and not script-src etc. then I wouldn't have any
> heartburn about approving this.

We'd need to keep the current, sweeping bypass for `chrome-extensions:`. I think
it's perfectly reasonable to add a more limited version for whatever we need
here. Even "everything but script-src, object-src, and style-src" would be
completely workable.

[Mike West, 2014-11-17 19:56:10]

On 2014/11/17 19:26:15, Tom Sepez wrote:
> So if the author isn't aware, how does it get loaded?  Via some other JS/HTML
> from some other place?  Or is this to support something like blink in JS?

This, I believe, is to support blink-in-JS features that need to present some
sort of UI.

[Tom Sepez, 2014-11-17 19:58:39]

jbroman - if you file a follow-on bug for a more granular bypass, then I'll
approve this.

[jbroman, 2014-11-17 20:15:23]

On 2014/11/17 19:56:10, Mike West wrote:
> On 2014/11/17 19:26:15, Tom Sepez wrote:
> > So if the author isn't aware, how does it get loaded?  Via some other
JS/HTML
> > from some other place?  Or is this to support something like blink in JS?
> 
> This, I believe, is to support blink-in-JS features that need to present some
> sort of UI.

My use case is moving plugin placeholders (the things that show up when a plugin
is disabled, configured for click-to-play, etc.) to being implemented with UA
shadow DOM (so that we don't need an in-process plugin that hosts a WebView,
using legacy codepaths to do so).

The author simply writes <embed src="foo.swf"></embed> or whatever; the fact
that we might override this with a click-to-play UI that contains images isn't
something authors need be aware of. One might also imagine the new
implementation of <marquee> loading its stylesheet in this way instead of having
the private CSS source embedded in a .js file (though that style is small enough
that it's probably not a big deal).

My implementation uses a dash of Blink-in-JS, though style resources are
somewhat separate. Essentially, I think that UA shadow DOM should be able to do
things like this internally (without having to re-implement
changing-background-image-on-hover, which is a solved problem):

<style>
#foo { background-image: url(...); }
#foo:hover { background-image: url(...); }
</style>

If features (esp. ones using Blink-in-JS) are built on top of web technologies
(HTML, CSS, JS), then loading resources the same way web content do is simply
being consistent from a layering standpoint.


On 2014/11/17 19:55:37, Mike West wrote:
> On 2014/11/17 19:43:01, Tom Sepez wrote:
> > Perhaps the underlying issue is that
schemeShouldBypassContentSecurityPolicy()
> > is too big a hammer.  If it were more granular so that we could bypass it
only
> > for image-src and others, and not script-src etc. then I wouldn't have any
> > heartburn about approving this.
> 
> We'd need to keep the current, sweeping bypass for `chrome-extensions:`. I
think
> it's perfectly reasonable to add a more limited version for whatever we need
> here. Even "everything but script-src, object-src, and style-src" would be
> completely workable.

Hmm, that seems straightforward enough. It seems to me that reviewers paying
attention
to what kind of files are 

My only question here is how sensitive style-src is (especially given the
content
of such files is not attacker-crafted). It seems mildly nicer to have it loaded
the
same way (more straightforward to load, can load images with relative rather
than
absolute URLs, etc.), but if CSS is a large security concern, it is possible to
work
around that.

[Mike West, 2014-11-18 20:38:51]

Conceptually, this LGTM from the Blink side. I'm not sure about the name (it
feels weird to have a chrome-* scheme defined in //content, though we're
apparently already doing this). Please do find an appropriate //content OWNER to
sign off on the code changes once Tom is happy from a security perspective.

https://codereview.chromium.org/647853002/diff/450001/content/public/common/u...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/647853002/diff/450001/content/public/common/u...
content/public/common/url_constants.h:22: CONTENT_EXPORT extern const char
kResourceScheme[];
Can we name this something less generic? I'd like to narrowly scope the scheme
to blink-in-js; `private-script-resources`, `blink-in-js-resources`,
`resources-that-we-wish-you-wouldn-t-block-via-csp-resources`, etc. Something
snappy. ;)

[Tom Sepez, 2014-11-18 20:47:17]

https://codereview.chromium.org/647853002/diff/450001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/647853002/diff/450001/content/renderer/render...
content/renderer/render_thread_impl.cc:994: resource_scheme);
I thought I'd see one of your newfangled constants here to say images and links
only.

[jbroman, 2014-11-18 20:47:18]

https://codereview.chromium.org/647853002/diff/450001/content/public/common/u...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/647853002/diff/450001/content/public/common/u...
content/public/common/url_constants.h:22: CONTENT_EXPORT extern const char
kResourceScheme[];
On 2014/11/18 20:38:51, Mike West wrote:
> Can we name this something less generic? I'd like to narrowly scope the scheme
> to blink-in-js; `private-script-resources`, `blink-in-js-resources`,
> `resources-that-we-wish-you-wouldn-t-block-via-csp-resources`, etc. Something
> snappy. ;)

It's not tied to Blink-in-JS (Blink-in-JS is more likely to need to load things
this way, but it's entirely reasonable to write the controlling code in C++ but
still want to express your style in CSS, getting device-scale-factor image
loading support, :hover support, media queries, etc. for free).

Some random alternatives (though the "chrome-" prefix is nice insofar as its
clear it's vendor-specific).

kPrivateResourceScheme = private-resource:// or chrome-private-resource://
kInternalResourceScheme = internal-resource:// or chrome-internal-resource://

(with corresponding file renames, probably)

I can bikeshed more over IM if you wish. :)

[Tom Sepez, 2014-11-18 20:49:12]

> Some random alternatives (though the "chrome-" prefix is nice insofar as its
> clear it's vendor-specific).
> 
I wouldn't lose sleep over this.  I like that chrome- is vendor-specific since
this scheme may occur in web pages in the wild.

[jbroman, 2014-11-18 20:49:54]

On 2014/11/18 20:47:17, Tom Sepez wrote:
>
https://codereview.chromium.org/647853002/diff/450001/content/renderer/render...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/647853002/diff/450001/content/renderer/render...
> content/renderer/render_thread_impl.cc:994: resource_scheme);
> I thought I'd see one of your newfangled constants here to say images and
links
> only.

I haven't updated this CL yet.

[jbroman, 2014-11-18 20:59:54]

On 2014/11/18 20:49:54, jbroman wrote:
> On 2014/11/18 20:47:17, Tom Sepez wrote:
> >
>
https://codereview.chromium.org/647853002/diff/450001/content/renderer/render...
> > File content/renderer/render_thread_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/647853002/diff/450001/content/renderer/render...
> > content/renderer/render_thread_impl.cc:994: resource_scheme);
> > I thought I'd see one of your newfangled constants here to say images and
> links
> > only.
> 
> I haven't updated this CL yet.

There, it now only allows images and style.

[Tom Sepez, 2014-11-18 21:03:34]

lgtm

[jbroman, 2014-11-18 21:15:59]

jbroman@chromium.org changed reviewers:
+ davidben@chromium.org

[jbroman, 2014-11-18 21:16:00]

+davidben for content/ review

This has been reviewed by mkwst/tsepez for CSP/security concerns, and is waiting
for https://codereview.chromium.org/730203007/ to land on the Blink side.

If this LGTY, I'll go ask an android_webview owner to stamp it as well.

[davidben, 2014-11-19 00:21:20]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org

[davidben, 2014-11-19 00:21:21]

+mmenke in case he has opinions on how things bridging resource_ids into
URLRequestJobs should work. It seems like we might already have such a mechanism
for chrome:// links and WebUI and whatnot, but I don't know enough about them to
know if they're suitable.

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
File content/browser/resource_protocol.cc (right):

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.cc:104: for (const auto& resource :
kBlinkWebAccessibleResources) {
Skip the dummy entry?

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.cc:106: resource.scale_factor,
resource.content_type);
It's odd that this class simultaneously comes with the test of resources and
also allows callers to register their own (only for tests?). Might be better if
either this class be agnostic to the default set (with those registered by the
code that creates a ResourceProtocolHandler) or perhaps have a separate
RegisterBlinkResources() method.

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
File content/browser/resource_protocol.h (right):

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.h:32: // registering it.
What are resource_id, scale_factor, and content_type in this context? (Document
in comment.)

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.h:54: std::map<std::pair<std::string,
std::string>, WebAccessibleResource>
I'd either add a comment or a dedicated struct so it's clear what the two
components of the pair are.

[jbroman, 2014-11-19 15:06:54]

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
File content/browser/resource_protocol.cc (right):

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.cc:104: for (const auto& resource :
kBlinkWebAccessibleResources) {
On 2014/11/19 00:21:21, David Benjamin wrote:
> Skip the dummy entry?

Done.

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.cc:106: resource.scale_factor,
resource.content_type);
On 2014/11/19 00:21:21, David Benjamin wrote:
> It's odd that this class simultaneously comes with the test of resources and
> also allows callers to register their own (only for tests?). Might be better
if
> either this class be agnostic to the default set (with those registered by the
> code that creates a ResourceProtocolHandler) or perhaps have a separate
> RegisterBlinkResources() method.

Moved out into a RegisterDefaultWebAccessibleResources function (still in the
same header and source file, lacking any better place to put it).

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
File content/browser/resource_protocol.h (right):

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.h:32: // registering it.
On 2014/11/19 00:21:21, David Benjamin wrote:
> What are resource_id, scale_factor, and content_type in this context?
(Document
> in comment.)

Done.

https://codereview.chromium.org/647853002/diff/470001/content/browser/resourc...
content/browser/resource_protocol.h:54: std::map<std::pair<std::string,
std::string>, WebAccessibleResource>
On 2014/11/19 00:21:21, David Benjamin wrote:
> I'd either add a comment or a dedicated struct so it's clear what the two
> components of the pair are.

Done.

[mmenke, 2014-11-19 16:58:51]

I'll take a look, but I'm not familiar with the logic used for chrome URLs.

One thing this CL needs to do, though:  Update ProfileIOData::IsHandledProtocol
to return true for the new protocol.  Unfortunately, it's currently not called
just on the IO thread, so we can't call into the URLRequestJobFactory to see
what protocol handlers it has hooked up, unfortunately.

Also, the resource_protocol files should be called resource_protocol_handler.

[jbroman, 2014-11-19 18:55:05]

On 2014/11/19 16:58:51, mmenke wrote:
> I'll take a look, but I'm not familiar with the logic used for chrome URLs.
> 
> One thing this CL needs to do, though:  Update
ProfileIOData::IsHandledProtocol
> to return true for the new protocol.  Unfortunately, it's currently not called
> just on the IO thread, so we can't call into the URLRequestJobFactory to see
> what protocol handlers it has hooked up, unfortunately.

Done.

> Also, the resource_protocol files should be called resource_protocol_handler.

Done.

[mmenke, 2015-02-27 20:38:56]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org