Add sanbdoxed_pages to allow extension/app pages to be served in a sandboxed, unique origin

Add sandboxed_pages to allow extension/app pages to be served in a
sandboxed, unique origin. This allows manifest_version 2 extensions to have
pages that are exempt from their Content Security Policy (but these pages
can't call extension APIs either).

Depends on http://webkit.org/b/88014

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

[Mihai Parparita -not on Chrome, 2012-06-01 00:23:14]

Adam: FYI, this is why I've been bugging you about CSP/sandbox bugs.

[Aaron Boodman, 2012-06-04 07:05:41]

http://codereview.chromium.org/10458063/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/10458063/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:282: content_security_policy =
extension->IsResourceSandboxed(resource_path) ?
It's kinda a shame that this doesn't pick up any of the other declarations from
content_security_policy().

Perhaps we are going about this wrong? What if we just allow developers to
specify the sandbox directive for some resources, and with that allow
allow-scripts, allow-forms, etc.

I'm not sure why Adam decided that the <meta> tag syntax was not appropriate for
policy delivery in the extension system, but if we can't use that, per-resource
policies could be defined in the manifest.

http://codereview.chromium.org/10458063/diff/12001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10458063/diff/12001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1583: ListValue* list_value;
= NULL;

[Mihai Parparita -not on Chrome, 2012-06-04 18:27:02]

http://codereview.chromium.org/10458063/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/10458063/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:282: content_security_policy =
extension->IsResourceSandboxed(resource_path) ?
On 2012/06/04 07:05:42, Aaron Boodman wrote:
> It's kinda a shame that this doesn't pick up any of the other declarations
from
> content_security_policy().
> 
> Perhaps we are going about this wrong? What if we just allow developers to
> specify the sandbox directive for some resources, and with that allow
> allow-scripts, allow-forms, etc.
> 
> I'm not sure why Adam decided that the <meta> tag syntax was not appropriate
for
> policy delivery in the extension system, but if we can't use that,
per-resource
> policies could be defined in the manifest.

BTW, the content_security_policy manifest value isn't used for platform apps
(they can't override the default CSP, since they can't load remote scripts
regardless).

Generally, the <meta> tag approach isn't enough, since if there are multiple CSP
values for a page (the manifest one in the header, and the <meta> one) then they
all have to (i.e. additional CSP values can only make things stricter). <meta>
is also annoying because you have to repeat it for every page.

I can change this approach to something like:

sandboxed_resources: {
  // Additional CSP that would be combined with the the default sandboxed CSP
(as defined in this CL)
  "content_security_policy": "media-src: *"
  "resources" [
    "
  ]
}

However, that seems like additional complexity that no developers have asked
for. I can still turn sandboxed_resources into a dictionary so that we have the
option of extending this in the future.

[Mihai Parparita -not on Chrome, 2012-06-05 04:32:39]

As discussed in #crx, I've switched to:

"sadboxed_pages": {
  "pages": [
     "page1.html",
      "page2.html"
  ],
  "content_security_policy": "sandbox allow-scripts; script-src:
https://www.google.com" // optional
}

(I decided to go with "sandboxed_pages" instead of "sandboxed_resources" or the
like, since only pages (as opposed to .js, .css, etc.) can actually have a CSP
value or be sandboxed in general.

http://codereview.chromium.org/10458063/diff/12001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10458063/diff/12001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1583: ListValue* list_value;
On 2012/06/04 07:05:42, Aaron Boodman wrote:
> = NULL;

Done (though I don't think it's necessary; we don't look at the value that it's
set to if the return value of GetList indicates an error).

[Mihai Parparita -not on Chrome, 2012-06-05 22:21:56]

PTAL

http://codereview.chromium.org/10458063/diff/26001/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10458063/diff/26001/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:899: // We have an explicit
check for sandboxed pages first since:
I ended up having to add this, since API bindings were still getting inserted (I
thought it was tryserver test flakiness, but it was just luck that the test
passed locally).

Let me know if you can think of a cleaner way to handle this.

[Aaron Boodman, 2012-06-05 23:20:11]

On 2012/06/05 22:21:56, Mihai Parparita wrote:
> PTAL
> 
>
http://codereview.chromium.org/10458063/diff/26001/chrome/renderer/extensions...
> File chrome/renderer/extensions/extension_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/10458063/diff/26001/chrome/renderer/extensions...
> chrome/renderer/extensions/extension_dispatcher.cc:899: // We have an explicit
> check for sandboxed pages first since:
> I ended up having to add this, since API bindings were still getting inserted
(I
> thought it was tryserver test flakiness, but it was just luck that the test
> passed locally).
> 
> Let me know if you can think of a cleaner way to handle this.

Nope. LGTM.

[Mihai Parparita -not on Chrome, 2012-06-05 23:39:06]

http://codereview.chromium.org/10458063/diff/33002/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10458063/diff/33002/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:957: // We should never end
up with sandboxed contexts trying to invoke extension
Adam also suggested checking at API invocation time that we're not in a
sandboxed page. I added a CHECK, since we should never be able to get here in
normal circumstances.

[commit-bot: I haz the power, 2012-06-05 23:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10458063/33002

[commit-bot: I haz the power, 2012-06-06 01:41:25]

Change committed as 140689