Share schemes needed for CSP between the browser and the renderer.

Share schemes needed for CSP between the browser and the renderer.

This CL is similar to this one: crrev.com/2623353002/
There is a list of scheme that bypasses the Content-Security-Policy.
With this CL, this list is provided by content embedders instead of
blink embedders. It will be used for checking CSP in the browser process.

BUG=685074
Review-Url: https://codereview.chromium.org/2679383003
Cr-Commit-Position: refs/heads/master@{#449261}
Committed: https://chromium.googlesource.com/chromium/src/+/eb73e4330ecb59905157b11ce20ca29345d9635e

[arthursonzogni, 2017-02-08 10:04:58]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 10:05:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 11:46:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 11:46:50]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-02-08 13:31:42]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 13:32:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 13:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 13:33:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[arthursonzogni, 2017-02-08 13:37:25]

Patchset #2 (id:20001) has been deleted

[arthursonzogni, 2017-02-08 13:59:22]

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

[arthursonzogni, 2017-02-08 13:59:22]

This change will allow us to know which scheme should be bypassed for CSP on the
browser process.

Do you agree with the global change?
I will ask brettw@ to review the remaining files outside of third_party/WebKit/
if you are okay with this change.

One additional question: do you think that the change made in
crrev.com/730203007 are still used somewhere?
AFAIU, the only scheme that is actually added is "chrome-extension".
There was in the past: "chrome-extension-resource" and "chrome-resource", but
it's no more the case.

[Mike West, 2017-02-08 14:59:52]

mkwst@chromium.org changed reviewers:
+ brettw@chromium.org, esprehn@chromium.org, nasko@chromium.org

[Mike West, 2017-02-08 14:59:53]

I think the general idea of moving this registry out of Blink is a good one,
given the other patch that moves some of CSP out, but it's not clear to me what
the brightline is for the scheme registries in //url. My intuition is that this
is narrow enough to be over the line, but I'd like Brett's opinion.

I also wonder whether it makes sense for //content to be blind to extensions as
a concept. We're doing a lot of indirection here and elsewhere to set up very
specific registries for things that could be simplified if we taught
blink/content about some sort of extension concept. *shrug* CCing nasko@ and
esprehn@ who have thought about the layering here much more than I have.

https://codereview.chromium.org/2679383003/diff/1/extensions/renderer/dispatc...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/2679383003/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:275: extension_scheme);
We register `extension_scheme` as bypassing CSP, supporting Fetch, and being
"first-party" when top-level. I wonder if it would be better to wrap these
concepts up into one ball of schemes that are non-webby, but somehow privileged.
I don't have a good name for that concept.

https://codereview.chromium.org/2679383003/diff/1/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2679383003/diff/1/url/url_util.h#newcode104
url/url_util.h:104: URL_EXPORT const std::vector<std::string>&
GetBypassingCSPSchemes();
I'm pretty hard-pressed to call this a concept that //url should know about.

[arthursonzogni, 2017-02-08 15:13:55]

Description was changed from

==========
Share schemes needed for CSP between the browser and the renderer.

This CL is similar to this one: crrev.com/2623353002/
There is a list of scheme that bypasses the Content-Security-Policy.
With this CL this list is provided by contents embedders instead of
blink embedders. It will be used to know this list when checking the CSP
in the browser process.

BUG=685074
==========

to

==========
Share schemes needed for CSP between the browser and the renderer.

This CL is similar to this one: crrev.com/2623353002/
There is a list of scheme that bypasses the Content-Security-Policy.
With this CL, this list is provided by content embedders instead of
blink embedders. It will be used for checking CSP in the browser process.

BUG=685074
==========

[nasko, 2017-02-09 00:13:15]

> I also wonder whether it makes sense for //content to be blind to extensions
as
> a concept. 

Yes! //content is very much oblivious about extensions and should stay that way.

> We're doing a lot of indirection here and elsewhere to set up very
> specific registries for things that could be simplified if we taught
> blink/content about some sort of extension concept. 

Yes, it will be easier, but extensions are not part of the web, therefore have
no place in //content.

https://codereview.chromium.org/2679383003/diff/1/content/public/common/conte...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/2679383003/diff/1/content/public/common/conte...
content/public/common/content_client.h:114: std::vector<std::string>
bypassing_csp_schemes;
nit: I'd reorder the name a bit - csp_bypassing_schemes, matches a bit better
the rest of the names.

https://codereview.chromium.org/2679383003/diff/1/extensions/renderer/dispatc...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/2679383003/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:275: extension_scheme);
On 2017/02/08 14:59:53, Mike West (sloooooow) wrote:
> We register `extension_scheme` as bypassing CSP, supporting Fetch, and being
> "first-party" when top-level. I wonder if it would be better to wrap these
> concepts up into one ball of schemes that are non-webby, but somehow
privileged.
> I don't have a good name for that concept.

In what sense are they non-webby? To me non-webby means mailto: or external
protocols. Extensions are very much webby, but not web : ), also privileged.

[jam1, 2017-02-09 00:39:22]

On 2017/02/08 14:59:53, Mike West (sloooooow) wrote:
> I think the general idea of moving this registry out of Blink is a good one,
> given the other patch that moves some of CSP out, but it's not clear to me
what
> the brightline is for the scheme registries in //url. My intuition is that
this
> is narrow enough to be over the line, but I'd like Brett's opinion.

IMO this is similar to cors/local-scheme/no-acccess. It's not used by url/, but
url/ is the most convenient place to share this data.

> 
> I also wonder whether it makes sense for //content to be blind to extensions
as
> a concept. We're doing a lot of indirection here and elsewhere to set up very
> specific registries for things that could be simplified if we taught
> blink/content about some sort of extension concept. *shrug* CCing nasko@ and
> esprehn@ who have thought about the layering here much more than I have.

We spent a lot of effort separating extensions, among other non web-platform
features, out of content. For both practical pov of having the core
multi-process browser code not be bloated (i.e. since it's used on limited
hardware such as chromecast), to a general layered system where there's
separation of concerns.

[brettw, 2017-02-09 01:02:57]

lgtm

[Mike West, 2017-02-09 06:31:25]

Ok, then. LGTM if y'all are happy. :)

[jam, 2017-02-09 06:41:36]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2017-02-09 06:41:36]

lgtm for content/chrome/extensions

[arthursonzogni, 2017-02-09 09:15:53]

All right, thanks for the review!

https://codereview.chromium.org/2679383003/diff/1/content/public/common/conte...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/2679383003/diff/1/content/public/common/conte...
content/public/common/content_client.h:114: std::vector<std::string>
bypassing_csp_schemes;
On 2017/02/09 00:13:15, nasko (very slow) wrote:
> nit: I'd reorder the name a bit - csp_bypassing_schemes, matches a bit better
> the rest of the names.

Done.

[arthursonzogni, 2017-02-09 09:17:43]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-02-09 09:17:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jam@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2679383003/#ps40001
(title: "Nit #1 @nasko.")

[commit-bot: I haz the power, 2017-02-09 09:17:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 09:20:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 09:20:27]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[arthursonzogni, 2017-02-09 09:47:55]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-02-09 09:47:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jam@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2679383003/#ps60001
(title: "Rebase and fix conflict.")

[commit-bot: I haz the power, 2017-02-09 09:48:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 11:54:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486633675057120,
"parent_rev": "d9960f46f78bbe38e99cc083175f830582e8fceb", "commit_rev":
"eb73e4330ecb59905157b11ce20ca29345d9635e"}

[commit-bot: I haz the power, 2017-02-09 11:55:26]

Description was changed from

==========
Share schemes needed for CSP between the browser and the renderer.

This CL is similar to this one: crrev.com/2623353002/
There is a list of scheme that bypasses the Content-Security-Policy.
With this CL, this list is provided by content embedders instead of
blink embedders. It will be used for checking CSP in the browser process.

BUG=685074
==========

to

==========
Share schemes needed for CSP between the browser and the renderer.

This CL is similar to this one: crrev.com/2623353002/
There is a list of scheme that bypasses the Content-Security-Policy.
With this CL, this list is provided by content embedders instead of
blink embedders. It will be used for checking CSP in the browser process.

BUG=685074

Review-Url: https://codereview.chromium.org/2679383003
Cr-Commit-Position: refs/heads/master@{#449261}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb73e4330ecb59905157b11ce20c...
==========

[commit-bot: I haz the power, 2017-02-09 11:55:27]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/eb73e4330ecb59905157b11ce20c...