Allow extenions to override the default content_security_policy, but require

Allow extenions to override the default content_security_policy, but require
the explicit policy to meet a minimum security threshold.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112656

[abarth-chromium, 2011-12-02 00:41:56]

This patch isn't yet ready for review, but I thought I should send you a
work-in-progress CL in case you had high-level feedback on the approach.

Thanks!

[abarth-chromium, 2011-12-02 01:34:35]

Ok.  This is now ready for review.

[Aaron Boodman, 2011-12-02 05:51:42]

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
File chrome/common/extensions/csp_validator.cc (right):

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.cc:43: StartsWithASCII(source,
"chrome://", true) ||
Why chrome://?

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.cc:53: bool UpdateStatus(const
std::string& directive_name,
Please add a comment describing what the return value means.

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.cc:65: }
Chrome style is to include a comment like:

//  namespace

here

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.cc:76: bool
ContentSecurityPolicyIsSecure(const std::string& policy) {
I hate to tell you this after you wrote all this clever parsing code, but you
could have also represented the CSP policy in the manifest in a structured form,
like:

csp_policy: {
  default_src: ['self'],
  script_src: ['self', 'https://www.google.com']
}

This would also allow the individual parts of the policy to be overridden
without overriding the others.

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.cc:108: if
(default_src_status.seen_in_policy && !default_src_status.is_secure)
Style guide requires braces for an if statement that is more than two lines
total.

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
File chrome/common/extensions/csp_validator.h (right):

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.h:11: namespace extension_csp_validator {
We're working on introducing a new 'extensions' namespace. How do you feel about
extensions::csp_validator::Foo()? Too fancy?

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

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2263:
DCHECK(ContentSecurityPolicyIsSecure(content_security_policy_));
The extension system prefers CHECK to DCHECK.

[Aaron Boodman, 2011-12-02 05:52:20]

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
File chrome/common/extensions/csp_validator.h (right):

http://codereview.chromium.org/8773028/diff/7001/chrome/common/extensions/csp...
chrome/common/extensions/csp_validator.h:11: namespace extension_csp_validator {
On 2011/12/02 05:51:42, Aaron Boodman wrote:
> We're working on introducing a new 'extensions' namespace. How do you feel
about
> extensions::csp_validator::Foo()? Too fancy?

Sorry, I meant to change this to be more assertive before publishing. I think
you should do this.

[abarth-chromium, 2011-12-02 07:01:56]

> Why chrome://?

It's needed by the Bookmark Manager.  It's also harmless because the "chrome"
scheme can't be corrupted by network attackers.

> Please add a comment describing what the return value means.

Done.

> //  namespace

Done.

> I hate to tell you this after you wrote all this clever parsing code, but you
> could have also represented the CSP policy in the manifest in a structured
form,
> like:
> 
> csp_policy: {
>   default_src: ['self'],
>   script_src: ['self', 'https://www.google.com']
> }
> 
> This would also allow the individual parts of the policy to be overridden
> without overriding the others.

Yeah, but then folks would need to learn another representation for policies.  I
liked the idea that the policy strings are the same everywhere they appear (HTTP
headers, HTML meta elements, CRX manifests).  It's likely these policy strings
will be used in other locations, such as the XML manifests for W3C widgets.  If
this was just a feature for extensions, I would agree with you, but having a
common language that's used everywhere seems valuable.

> Style guide requires braces for an if statement that is more than two lines
> total.

Done.  Thanks for reminding me about these style points.  My Chromium style is a
bit rusty.

> We're working on introducing a new 'extensions' namespace. How do you feel
about
> extensions::csp_validator::Foo()? Too fancy?

Done.

> The extension system prefers CHECK to DCHECK.

Rock on.

[abarth-chromium, 2011-12-02 07:16:24]

Patch updated.  Thanks for your review.

[Aaron Boodman, 2011-12-02 07:25:09]

LGTM \m/

[abarth-chromium, 2011-12-02 07:37:43]

On 2011/12/02 07:25:09, Aaron Boodman wrote:
> LGTM \m/

Thanks!

[commit-bot: I haz the power, 2011-12-02 07:37:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/8773028/8010