Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563843002:
Restrict app sandbox's CSP to disallow loading web content in them. (Closed)
DescriptionRestrict app sandbox's CSP to disallow loading web content in them.
The CL restricts loading subframes and scripts inside chrome app
sandbox pages (https://developer.chrome.com/apps/manifest/sandbox).
The restriction would only allow loading these resources from
'self', i.e. the app's resources.
1) scripts
Two CSP directives can affect subframe restriction:
script-src or
default-src fallback when script-src is not present.
2) subframes
Three possible CSP directives can affect script restriction:
frame-src or (deprecated)
child-src or
default-src fallback when none of frame-src/child-src is present.
This CL adds a GetEffectiveSandboxedPageCSP function, which
returns the effective CSP chrome will use when a certain
sandbox CSP (sandbox.content_security_policy key) is specified in
app's manifest.
The effective CSP is a restricted CSP that strips out any remote
hosts from #1 and #2. Any directive source value that is not
surrounded by single quotes are considered remote hosts. The
approach is to add 'self' as source value by default if 'none' was
not specified.
TODO: Update sandbox documentation page.
BUG=615585
Test=Loading a remote script from foo.com inside a chrome
apps sandbox page should not be possible, even with setting
the following "sandbox.content_security_policy" key in the
manifest:
script-src: foo.com
Same would go with loading <iframe> elements. It should not
be possible to load web content <iframe>s inside apps.
Committed: https://crrev.com/c0304f6696623d76672a4c8fbefa16378c3137b8
Cr-Commit-Position: refs/heads/master@{#441208}
Patch Set 1 #
Total comments: 10
Patch Set 2 : update parsing code, generalize for extensions and sandbox pages #Patch Set 3 : fix unit_tests #Patch Set 4 : sync #Patch Set 5 : sync #
Total comments: 10
Patch Set 6 : address comments + rework CL + StringPieces #
Total comments: 42
Patch Set 7 : address comments #Patch Set 8 : more commetns #
Total comments: 16
Patch Set 9 : address comments #Patch Set 10 : address comments #Patch Set 11 : comments #
Total comments: 2
Patch Set 12 : address comments + update doc #
Total comments: 6
Patch Set 13 : comments #Patch Set 14 : sync @tott #Messages
Total messages: 54 (30 generated)
lazyboy@chromium.org changed reviewers: + creis@chromium.org, rdevlin.cronin@chromium.org
Devlin & Charlie: Wanted to see how you feel about this approach... This CL uses CSP to add the restriction on loading web content (subframe and script) inside app sandbox. This is accomplished by manually parsing the CSP, similar to what we do in csp_validator.
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Overall, I like this approach. I think we should add an API test for it, though. (You were probably planning on it, but just to be sure. :)) https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator.cc:348: std::string GetEffectiveSandoxedPageCSP(const std::string& policy) { I kind of wonder if it makes sense to have a more general parsing for CSP in some kind of object that's easily modifiable. E.g., class CSP { public: CSP(std::string); std::string ToString(); // accessors and modifiers }; And then use that both here and in SanitizeCSP. WDYT? It seems like it would be fairly simple and would make a lot of this code more readable, but I haven't tried coding it up so maybe I'm missing something. (There may even already be this somewhere? But I couldn't find it. Maybe Charlie knows.) https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator.cc:383: source_lower[source_lower.size() - 1] == '\'') { When we see an insecure pattern, we could add an install warning to let developers know that it will be ignored. https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... File extensions/common/csp_validator_unittest.cc (right): https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator_unittest.cc:470: // TODO(lazyboy): Add comprehensive test cases. Can we address this TODO before landing? ;)
creis@chromium.org changed reviewers: + alexmos@chromium.org, mkwst@chromium.org
[+alexmos, mkwst for Devlin's question] Thanks for making progress on this! I'm ok with a CSP mechanism, given that we'll end up with a process swap in DoesSiteRequireDedicatedProcess (and then likely a renderer kill similar to https://crbug.com/612711) if it misses anything. Typo in CL description under "2) subframes": s/script/frame/ You'll also likely need to update site_details_browsertest.cc, which has a TODO for this bug above CreateAppWithSandboxPage. ExtensionApiTest.SandboxedPages is also failing in the try jobs. https://codereview.chromium.org/2563843002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2563843002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:309: // app. Isolating hosted apps is a good idea, but ought to be a separate nit: s/app/apps/ https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator.cc:348: std::string GetEffectiveSandoxedPageCSP(const std::string& policy) { On 2016/12/09 15:46:01, Devlin (in MON - PST plus 3) wrote: > I kind of wonder if it makes sense to have a more general parsing for CSP in > some kind of object that's easily modifiable. E.g., > > class CSP { > public: > CSP(std::string); > std::string ToString(); > // accessors and modifiers > }; > > And then use that both here and in SanitizeCSP. WDYT? It seems like it would > be fairly simple and would make a lot of this code more readable, but I haven't > tried coding it up so maybe I'm missing something. > > (There may even already be this somewhere? But I couldn't find it. Maybe > Charlie knows.) I haven't seen one, but I agree that avoiding a bunch of custom parsing logic for each use case is a good idea. alexmos@ and mkwst@ are more familiar with CSP and might know.
https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator.cc:348: std::string GetEffectiveSandoxedPageCSP(const std::string& policy) { On 2016/12/09 19:55:07, Charlie Reis wrote: > On 2016/12/09 15:46:01, Devlin (in MON - PST plus 3) wrote: > > I kind of wonder if it makes sense to have a more general parsing for CSP in > > some kind of object that's easily modifiable. E.g., > > > > class CSP { > > public: > > CSP(std::string); > > std::string ToString(); > > // accessors and modifiers > > }; > > > > And then use that both here and in SanitizeCSP. WDYT? It seems like it would > > be fairly simple and would make a lot of this code more readable, but I > haven't > > tried coding it up so maybe I'm missing something. > > > > (There may even already be this somewhere? But I couldn't find it. Maybe > > Charlie knows.) > > I haven't seen one, but I agree that avoiding a bunch of custom parsing logic > for each use case is a good idea. alexmos@ and mkwst@ are more familiar with > CSP and might know. I haven't seen one either, and I agree it's a good idea to have a common place for parsing CSP. We even have a bug filed for this: https://crbug.com/376522; the motivation there is to move some of CSP enforcement to the browser process. mkwst/arthursonzogni are adding another little bit of CSP frame_ancestors parsing in ancestor_throttle in https://codereview.chromium.org/2488743003/, which could also use this.
On 2016/12/12 19:04:11, alexmos wrote: > https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... > File extensions/common/csp_validator.cc (right): > > https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... > extensions/common/csp_validator.cc:348: std::string > GetEffectiveSandoxedPageCSP(const std::string& policy) { > On 2016/12/09 19:55:07, Charlie Reis wrote: > > On 2016/12/09 15:46:01, Devlin (in MON - PST plus 3) wrote: > > > I kind of wonder if it makes sense to have a more general parsing for CSP in > > > some kind of object that's easily modifiable. E.g., > > > > > > class CSP { > > > public: > > > CSP(std::string); > > > std::string ToString(); > > > // accessors and modifiers > > > }; > > > > > > And then use that both here and in SanitizeCSP. WDYT? It seems like it > would > > > be fairly simple and would make a lot of this code more readable, but I > > haven't > > > tried coding it up so maybe I'm missing something. > > > > > > (There may even already be this somewhere? But I couldn't find it. Maybe > > > Charlie knows.) > > > > I haven't seen one, but I agree that avoiding a bunch of custom parsing logic > > for each use case is a good idea. alexmos@ and mkwst@ are more familiar with > > CSP and might know. > > I haven't seen one either, and I agree it's a good idea to have a common place > for parsing CSP. We even have a bug filed for this: https://crbug.com/376522; > the motivation there is to move some of CSP enforcement to the browser process. > mkwst/arthursonzogni are adding another little bit of CSP frame_ancestors > parsing in ancestor_throttle in https://codereview.chromium.org/2488743003/, > which could also use this. Thanks, I'm currently working to generalize CSP enforcing code in this file (csp_validator.cc) so that we only have one parsing logic in extensions/. Will upload the patch once I'm done.
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Restrict app sandbox's CSP to disallow loading web content in them. The CL restricts loading subframes and scripts inside chrome app sandbox pages (https://developer.chrome.com/apps/manifest/sandbox). The restriction would only allow loading these resources from 'self', i.e. the app's resources. 1) scripts Two CSP directives can affect script restriction: script-src or default-src fallback when script-src is not present. 2) subframes Three possible CSP directives can affect script restriction: frame-src or (deprecated) child-src or default-src fallback when none of frame-src/child-src is present. This CL adds a GetEffectiveSandboxedPageCSP function, which returns the effective CSP chrome will use when a certain sandbox CSP (sandbox.content_security_policy key) is specified in app's manifest. The effective CSP is a restricted CSP that strips out any remote hosts from #1 and #2. Any directive source value that is not surrounded by single quotes are considered remote hosts. The approach is to add 'self' as source value by default if 'none' was not specified. TODO: Update sandbox documentation page. BUG=615585 Test=Loading a remote script from foo.com inside a chrome apps sandbox page should not be possible, even with setting the following "sandbox.content_security_policy" key in the manifest: script-src: foo.com Same would go with loading <iframe> elements. It should not be possible to load web content <iframe>s inside apps. ========== to ========== Restrict app sandbox's CSP to disallow loading web content in them. The CL restricts loading subframes and scripts inside chrome app sandbox pages (https://developer.chrome.com/apps/manifest/sandbox). The restriction would only allow loading these resources from 'self', i.e. the app's resources. 1) scripts Two CSP directives can affect subframe restriction: script-src or default-src fallback when script-src is not present. 2) subframes Three possible CSP directives can affect script restriction: frame-src or (deprecated) child-src or default-src fallback when none of frame-src/child-src is present. This CL adds a GetEffectiveSandboxedPageCSP function, which returns the effective CSP chrome will use when a certain sandbox CSP (sandbox.content_security_policy key) is specified in app's manifest. The effective CSP is a restricted CSP that strips out any remote hosts from #1 and #2. Any directive source value that is not surrounded by single quotes are considered remote hosts. The approach is to add 'self' as source value by default if 'none' was not specified. TODO: Update sandbox documentation page. BUG=615585 Test=Loading a remote script from foo.com inside a chrome apps sandbox page should not be possible, even with setting the following "sandbox.content_security_policy" key in the manifest: script-src: foo.com Same would go with loading <iframe> elements. It should not be possible to load web content <iframe>s inside apps. ==========
(patch set #5) https://codereview.chromium.org/2563843002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2563843002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:309: // app. Isolating hosted apps is a good idea, but ought to be a separate On 2016/12/09 19:55:07, Charlie Reis wrote: > nit: s/app/apps/ Done. https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator.cc:348: std::string GetEffectiveSandoxedPageCSP(const std::string& policy) { On 2016/12/12 19:04:11, alexmos wrote: > On 2016/12/09 19:55:07, Charlie Reis wrote: > > On 2016/12/09 15:46:01, Devlin (in MON - PST plus 3) wrote: > > > I kind of wonder if it makes sense to have a more general parsing for CSP in > > > some kind of object that's easily modifiable. E.g., > > > > > > class CSP { > > > public: > > > CSP(std::string); > > > std::string ToString(); > > > // accessors and modifiers > > > }; > > > > > > And then use that both here and in SanitizeCSP. WDYT? It seems like it > would > > > be fairly simple and would make a lot of this code more readable, but I > > haven't > > > tried coding it up so maybe I'm missing something. > > > > > > (There may even already be this somewhere? But I couldn't find it. Maybe > > > Charlie knows.) > > > > I haven't seen one, but I agree that avoiding a bunch of custom parsing logic > > for each use case is a good idea. alexmos@ and mkwst@ are more familiar with > > CSP and might know. > > I haven't seen one either, and I agree it's a good idea to have a common place > for parsing CSP. We even have a bug filed for this: https://crbug.com/376522; > the motivation there is to move some of CSP enforcement to the browser process. > mkwst/arthursonzogni are adding another little bit of CSP frame_ancestors > parsing in ancestor_throttle in https://codereview.chromium.org/2488743003/, > which could also use this. I've introduced a CSPEnforcer class that can sanitize csp for apps sandbox and (existing) extensions. https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator.cc:383: source_lower[source_lower.size() - 1] == '\'') { On 2016/12/09 15:46:01, Devlin wrote: > When we see an insecure pattern, we could add an install warning to let > developers know that it will be ignored. Done. https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... File extensions/common/csp_validator_unittest.cc (right): https://codereview.chromium.org/2563843002/diff/1/extensions/common/csp_valid... extensions/common/csp_validator_unittest.cc:470: // TODO(lazyboy): Add comprehensive test cases. On 2016/12/09 15:46:01, Devlin wrote: > Can we address this TODO before landing? ;) I've expanded the tests a bit and added an api_test
https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js (right): https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js:6: window.parent.postMessage(JSON.stringify(['loaded', 'local_frame.html']), nit: This wrapping looks a bit weird. Maybe window.parent.postMessage( JSON.stringify(...), '*'); ? https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/main.js (right): https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/main.js:18: chrome.test.succeed(); optional: we could just do chrome.test.assertEq(expectSuccess, succeeeded); chrome.test.succeed(); And the test will fail if the assertion is wrong. But this way works too. https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/main.js:44: 'sandboxed_pages_web_content/URL'.replace( I've seen this pattern before, but it's kind of silly. Let's just do var url = 'http://localhost:' + port + '/extensions/api_test/sandboxed/' + REMOTE_FILE_NAME; https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:23: // I couldn't find a better way than setTimeout to detect that :(. is iframe.contentWindow.onerror called? https://codereview.chromium.org/2563843002/diff/80001/extensions/common/csp_v... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/80001/extensions/common/csp_v... extensions/common/csp_validator.cc:297: std::vector<InstallWarning>* warnings) { As discussed briefly offline, I still find this pretty hard to grok. Maybe parsing, then enforcing (rather than an enforce-as-you-parse) approach would make this a little more readable.
https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js (right): https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js:6: window.parent.postMessage(JSON.stringify(['loaded', 'local_frame.html']), On 2016/12/15 00:04:10, Devlin wrote: > nit: This wrapping looks a bit weird. Maybe > window.parent.postMessage( > JSON.stringify(...), '*'); > ? Done. https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/main.js (right): https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/main.js:18: chrome.test.succeed(); On 2016/12/15 00:04:11, Devlin wrote: > optional: we could just do > chrome.test.assertEq(expectSuccess, succeeeded); > chrome.test.succeed(); > > And the test will fail if the assertion is wrong. > But this way works too. Done. https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/main.js:44: 'sandboxed_pages_web_content/URL'.replace( On 2016/12/15 00:04:11, Devlin wrote: > I've seen this pattern before, but it's kind of silly. Let's just do > var url = 'http://localhost:' + port + '/extensions/api_test/sandboxed/' + > REMOTE_FILE_NAME; Done. https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:23: // I couldn't find a better way than setTimeout to detect that :(. On 2016/12/15 00:04:11, Devlin wrote: > is iframe.contentWindow.onerror called? Because the frame can be cross-origin, I don't think it would be possible. iframe.onerror does not work as well. https://codereview.chromium.org/2563843002/diff/80001/extensions/common/csp_v... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/80001/extensions/common/csp_v... extensions/common/csp_validator.cc:297: std::vector<InstallWarning>* warnings) { On 2016/12/15 00:04:11, Devlin wrote: > As discussed briefly offline, I still find this pretty hard to grok. Maybe > parsing, then enforcing (rather than an enforce-as-you-parse) approach would > make this a little more readable. I think there are too many cases to consider here which makes things a bit more complicated. I also noticed that we just don't want to split by ";" then split each token by " \t\r\n" [1] right away, because the latter split might be unnecessary depending on whether we actually care about this. I think that's why enforce as we parse style was inspired. I've made CSPDirectiveToken to express each token and kept all parsing logic inside that. I've exposed ToString() which we call always: will return the value without doing [1] (well, I need one tokenizer.GetNext). I've generalized some more code with base::Bind too. Using SplitStringPiece to use more StringPieces. Let's see how you like this version.
BTW, I'll be OOO after today, so feel free to land this without me, once Devlin approves. (I don't think I'm needed for any of the owner reviews.) Thanks!
I'm still not in love with the approach, but I'm willing to accept that as a difference of opinion. :) So let's go with this for now. https://codereview.chromium.org/2563843002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:23: // I couldn't find a better way than setTimeout to detect that :(. Can we verify this in the C++ instead? Timeouts in JS basically don't work on slow bots, like Dr Memory. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:70: class DirectiveStatus { Class comments https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:72: DirectiveStatus(const std::string& name) { directive_names_.push_back(name); } Could we just do: DirectiveStatus(std::initializer_list<const char*> directives) { directive_names_.assign(directives.begin(), directives.end()); } (or maybe just ": directive_names_(directives.begin(), directives.end())" works too?) Then to construct DirectiveStatus({"script-src"}); DirectiveStatus({"script-src", "child-src"}); https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:79: bool Matches(const std::string& directive_name) const { \n function comments https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:85: } \n https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:96: bool seen_in_policy_ = false; if we don't need to copy this, DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:96: bool seen_in_policy_ = false; member comments https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:160: bool IsHashSource(const base::StringPiece& source) { usually we don't pass StringPiece by const&, because it's nearly as cheap to copy as to pass the ref. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:162: if (source.empty() || source[hash_end] != '\'') { since you're here, source.back()? https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:194: base::StringPiece source_literal = directive_value; May as well just have a mutable variable in the for loop for (base::StringPiece directive_value : directive_values) https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:236: std::string GetAppSandboxSecureDirectiveValues( function comments https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:243: base::StringPiece source_literal = directive_value; ditto https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:250: source_lower[source_lower.size() - 1] == '\'') { source_lower.back() https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:252: seen_self_or_none |= true; optional nit: could one-line this: seen_self_or_none |= source_lower == "'none'" || source_lower == "'self'"; https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:262: sane_csp_parts.push_back("'self'"); comment why we do this. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:310: class CSPDirectiveToken { comments for this class + for methods https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:344: return base::StringPrintf("%s%c", directive_token_.as_string().c_str(), doesn't base::StringPiece::as_string().c_str() == base::StringPiece::data()? (well, in the sense of the value of the cstring, not pointer equality) https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:370: class CSPEnforcer { comments https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:388: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:391: std::vector<InstallWarning>* warnings) { Maybe DCHECK(!directives_.empty()) https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... File extensions/common/csp_validator_unittest.cc (right): https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator_unittest.cc:146: TEST(ExtensionCSPValidator, IsSecure) { Are all the test changes the result of renames and git cl format?
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2563843002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:23: // I couldn't find a better way than setTimeout to detect that :(. On 2016/12/20 17:36:38, Devlin wrote: > Can we verify this in the C++ instead? > > Timeouts in JS basically don't work on slow bots, like Dr Memory. I've changed to test a bit to not rely on timeouts: load local_frame.html in frame element send 'ping' local_frame.html receive 'pong' from local_frame.html load remote_frame.html in the same iframe element^^^ send 'ping' still receive 'pong' from local_frame.html, i.e. remote frame didn't load. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:70: class DirectiveStatus { On 2016/12/20 17:36:38, Devlin wrote: > Class comments Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:72: DirectiveStatus(const std::string& name) { directive_names_.push_back(name); } On 2016/12/20 17:36:38, Devlin wrote: > Could we just do: > DirectiveStatus(std::initializer_list<const char*> directives) { > directive_names_.assign(directives.begin(), directives.end()); > } > (or maybe just ": directive_names_(directives.begin(), directives.end())" works > too?) > > Then to construct > DirectiveStatus({"script-src"}); > DirectiveStatus({"script-src", "child-src"}); Nice, done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:79: bool Matches(const std::string& directive_name) const { On 2016/12/20 17:36:38, Devlin wrote: > \n > function comments Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:85: } On 2016/12/20 17:36:39, Devlin wrote: > \n Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:96: bool seen_in_policy_ = false; On 2016/12/20 17:36:39, Devlin wrote: > member comments Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:96: bool seen_in_policy_ = false; On 2016/12/20 17:36:38, Devlin wrote: > if we don't need to copy this, DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:160: bool IsHashSource(const base::StringPiece& source) { On 2016/12/20 17:36:38, Devlin wrote: > usually we don't pass StringPiece by const&, because it's nearly as cheap to > copy as to pass the ref. Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:162: if (source.empty() || source[hash_end] != '\'') { On 2016/12/20 17:36:38, Devlin wrote: > since you're here, source.back()? Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:194: base::StringPiece source_literal = directive_value; On 2016/12/20 17:36:38, Devlin wrote: > May as well just have a mutable variable in the for loop > for (base::StringPiece directive_value : directive_values) Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:236: std::string GetAppSandboxSecureDirectiveValues( On 2016/12/20 17:36:38, Devlin wrote: > function comments Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:243: base::StringPiece source_literal = directive_value; On 2016/12/20 17:36:38, Devlin wrote: > ditto Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:250: source_lower[source_lower.size() - 1] == '\'') { On 2016/12/20 17:36:39, Devlin wrote: > source_lower.back() Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:252: seen_self_or_none |= true; On 2016/12/20 17:36:39, Devlin wrote: > optional nit: could one-line this: > seen_self_or_none |= source_lower == "'none'" || source_lower == "'self'"; Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:262: sane_csp_parts.push_back("'self'"); On 2016/12/20 17:36:38, Devlin wrote: > comment why we do this. Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:310: class CSPDirectiveToken { On 2016/12/20 17:36:39, Devlin wrote: > comments for this class + for methods Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:344: return base::StringPrintf("%s%c", directive_token_.as_string().c_str(), On 2016/12/20 17:36:39, Devlin wrote: > doesn't base::StringPiece::as_string().c_str() == base::StringPiece::data()? > (well, in the sense of the value of the cstring, not pointer equality) .data() isn't \0 terminated. So can't use. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:370: class CSPEnforcer { On 2016/12/20 17:36:38, Devlin wrote: > comments Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:388: }; On 2016/12/20 17:36:38, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator.cc:391: std::vector<InstallWarning>* warnings) { On 2016/12/20 17:36:38, Devlin wrote: > Maybe DCHECK(!directives_.empty()) Done. https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... File extensions/common/csp_validator_unittest.cc (right): https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... extensions/common/csp_validator_unittest.cc:146: TEST(ExtensionCSPValidator, IsSecure) { On 2016/12/20 17:36:39, Devlin wrote: > Are all the test changes the result of renames and git cl format? Yes. I've reverted some git cl format changes to make it obvious.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js (right): https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js:5: /* nit: dead code? https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/remote_frame.js (right): https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/remote_frame.js:6: window.parent.postMessage(JSON.stringify(['loaded', 'remote_frame.html']), nit: indentation window.parent.postMessage( JSON.stringify(...), '*') https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:11: if (data[0] == 'pong' && data[1] == identifier) { all these pings and pongs are getting hard to keep track of. Could we instead do something like 'sandboxed frame', etc? https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:36: // remote resource will fail to load, expect the local frame to respond I don't quite follow. So in trying to commit an invalid url, we reload the current url? https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:94: std::string name() const { const &? https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:104: }; Still missing disallow copy and assign from https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:416: std::vector<DirectiveStatus> directives_; could we make these members private and pass in the directives in the ctor? https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:418: std::vector<std::string> enforced_csp_parts_; Is this used?
https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js (right): https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/local_frame.js:5: /* On 2016/12/22 17:04:32, Devlin wrote: > nit: dead code? Removed. https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/remote_frame.js (right): https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/remote_frame.js:6: window.parent.postMessage(JSON.stringify(['loaded', 'remote_frame.html']), On 2016/12/22 17:04:32, Devlin wrote: > nit: indentation > window.parent.postMessage( > JSON.stringify(...), '*') Done. https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:11: if (data[0] == 'pong' && data[1] == identifier) { On 2016/12/22 17:04:32, Devlin wrote: > all these pings and pongs are getting hard to keep track of. Could we instead > do something like 'sandboxed frame', etc? Changed to sandboxed frame msg <-> local/remote frame msg https://codereview.chromium.org/2563843002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:36: // remote resource will fail to load, expect the local frame to respond On 2016/12/22 17:04:32, Devlin wrote: > I don't quite follow. So in trying to commit an invalid url, we reload the > current url? We keep the current url and resource as is, but iframe.onload fires. https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... File extensions/common/csp_validator.cc (right): https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:94: std::string name() const { On 2016/12/22 17:04:32, Devlin wrote: > const &? Done. https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:104: }; On 2016/12/22 17:04:32, Devlin wrote: > Still missing disallow copy and assign from > https://codereview.chromium.org/2563843002/diff/100001/extensions/common/csp_... Ah, my response to this didn't go thru: we do copy these as we're using vector<DirectiveStatus> in CSPEnforcer::directives_.push_back. I've tried to make them vector<unique_ptr<DirectiveStatus>> to avoid the copy but with std::initalizer_list, the initialization looks messy: we have to do something like: directives_.push_back(base::MakeUnique<DirectiveStatus, std::initializer_list<const char*>({kScriptSrc})); which I found adds too much noise. I forgot we allow emplace_back, so I've resorted to emplace_back(new T) and made the container vector<unique_ptr<DirectiveStatus>> https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:416: std::vector<DirectiveStatus> directives_; On 2016/12/22 17:04:32, Devlin wrote: > could we make these members private and pass in the directives in the ctor? Moved others except |directives_| as directives_ is constructed conditionally in ExtensionCSPEnforcer::ExtensionCSPEnforcer() https://codereview.chromium.org/2563843002/diff/140001/extensions/common/csp_... extensions/common/csp_validator.cc:418: std::vector<std::string> enforced_csp_parts_; On 2016/12/22 17:04:32, Devlin wrote: > Is this used? Good catch, removed.
nice; code lg. Were you planning on updating the documentation in this patch set? https://codereview.chromium.org/2563843002/diff/200001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/200001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:7: var identifier = +new Date; nit: maybe use performance.now()? performance.now() == performance.now(); // false (+new Date()) == (+new Date()); // true
Updated the docs. https://codereview.chromium.org/2563843002/diff/200001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2563843002/diff/200001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:7: var identifier = +new Date; On 2016/12/27 17:48:38, Devlin wrote: > nit: maybe use performance.now()? > > performance.now() == performance.now(); // false > (+new Date()) == (+new Date()); // true Done.
Largely lg; a few last nits. Since it's for public docs, I'd like to take one last look before commit once changes are made. :) https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... File chrome/common/extensions/docs/templates/articles/manifest/sandbox.html (right): https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:4: <b><em>Warning:</em></b> Starting version 57, Chrome will no longer load nitty nit: starting *in* version 57 https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:5: external web content or web scripts inside sandboxed pages in favor of nitty nit: maybe: "Starting in version 57, Chrome will no longer allow external web content (including embedded frames and scripts) inside sandboxed pages. Please use a <a ...>webview</a> instead." WDYT? https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:52: directive and may not have the <code>allow-same-origin</code> token (see maybe "but it must have the sandbox directive, and may not have the allow-same-origin token or allow external web content" or something like that?
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... File chrome/common/extensions/docs/templates/articles/manifest/sandbox.html (right): https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:4: <b><em>Warning:</em></b> Starting version 57, Chrome will no longer load On 2016/12/28 16:42:37, Devlin wrote: > nitty nit: starting *in* version 57 Done. https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:5: external web content or web scripts inside sandboxed pages in favor of On 2016/12/28 16:42:37, Devlin wrote: > nitty nit: > maybe: > "Starting in version 57, Chrome will no longer allow external web content > (including embedded frames and scripts) inside sandboxed pages. Please use a <a > ...>webview</a> instead." > > WDYT? Done. https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:52: directive and may not have the <code>allow-same-origin</code> token (see On 2016/12/28 16:42:37, Devlin wrote: > maybe "but it must have the sandbox directive, and may not have the > allow-same-origin token or allow external web content" or something like that? This sentence is talking about directives values of sandbox directive, so I've put the caution about loading external web content in the next sentence.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by lazyboy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2563843002/#ps260001 (title: "sync @tott")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1483474134315580,
"parent_rev": "ba22471125b2074d8cb1c1d3618a4d2000483088", "commit_rev":
"695615d5dcfbe28051f1bcf8252559071a0c5d15"}
Message was sent while issue was closed.
Description was changed from ========== Restrict app sandbox's CSP to disallow loading web content in them. The CL restricts loading subframes and scripts inside chrome app sandbox pages (https://developer.chrome.com/apps/manifest/sandbox). The restriction would only allow loading these resources from 'self', i.e. the app's resources. 1) scripts Two CSP directives can affect subframe restriction: script-src or default-src fallback when script-src is not present. 2) subframes Three possible CSP directives can affect script restriction: frame-src or (deprecated) child-src or default-src fallback when none of frame-src/child-src is present. This CL adds a GetEffectiveSandboxedPageCSP function, which returns the effective CSP chrome will use when a certain sandbox CSP (sandbox.content_security_policy key) is specified in app's manifest. The effective CSP is a restricted CSP that strips out any remote hosts from #1 and #2. Any directive source value that is not surrounded by single quotes are considered remote hosts. The approach is to add 'self' as source value by default if 'none' was not specified. TODO: Update sandbox documentation page. BUG=615585 Test=Loading a remote script from foo.com inside a chrome apps sandbox page should not be possible, even with setting the following "sandbox.content_security_policy" key in the manifest: script-src: foo.com Same would go with loading <iframe> elements. It should not be possible to load web content <iframe>s inside apps. ========== to ========== Restrict app sandbox's CSP to disallow loading web content in them. The CL restricts loading subframes and scripts inside chrome app sandbox pages (https://developer.chrome.com/apps/manifest/sandbox). The restriction would only allow loading these resources from 'self', i.e. the app's resources. 1) scripts Two CSP directives can affect subframe restriction: script-src or default-src fallback when script-src is not present. 2) subframes Three possible CSP directives can affect script restriction: frame-src or (deprecated) child-src or default-src fallback when none of frame-src/child-src is present. This CL adds a GetEffectiveSandboxedPageCSP function, which returns the effective CSP chrome will use when a certain sandbox CSP (sandbox.content_security_policy key) is specified in app's manifest. The effective CSP is a restricted CSP that strips out any remote hosts from #1 and #2. Any directive source value that is not surrounded by single quotes are considered remote hosts. The approach is to add 'self' as source value by default if 'none' was not specified. TODO: Update sandbox documentation page. BUG=615585 Test=Loading a remote script from foo.com inside a chrome apps sandbox page should not be possible, even with setting the following "sandbox.content_security_policy" key in the manifest: script-src: foo.com Same would go with loading <iframe> elements. It should not be possible to load web content <iframe>s inside apps. Review-Url: https://codereview.chromium.org/2563843002 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Restrict app sandbox's CSP to disallow loading web content in them. The CL restricts loading subframes and scripts inside chrome app sandbox pages (https://developer.chrome.com/apps/manifest/sandbox). The restriction would only allow loading these resources from 'self', i.e. the app's resources. 1) scripts Two CSP directives can affect subframe restriction: script-src or default-src fallback when script-src is not present. 2) subframes Three possible CSP directives can affect script restriction: frame-src or (deprecated) child-src or default-src fallback when none of frame-src/child-src is present. This CL adds a GetEffectiveSandboxedPageCSP function, which returns the effective CSP chrome will use when a certain sandbox CSP (sandbox.content_security_policy key) is specified in app's manifest. The effective CSP is a restricted CSP that strips out any remote hosts from #1 and #2. Any directive source value that is not surrounded by single quotes are considered remote hosts. The approach is to add 'self' as source value by default if 'none' was not specified. TODO: Update sandbox documentation page. BUG=615585 Test=Loading a remote script from foo.com inside a chrome apps sandbox page should not be possible, even with setting the following "sandbox.content_security_policy" key in the manifest: script-src: foo.com Same would go with loading <iframe> elements. It should not be possible to load web content <iframe>s inside apps. Review-Url: https://codereview.chromium.org/2563843002 ========== to ========== Restrict app sandbox's CSP to disallow loading web content in them. The CL restricts loading subframes and scripts inside chrome app sandbox pages (https://developer.chrome.com/apps/manifest/sandbox). The restriction would only allow loading these resources from 'self', i.e. the app's resources. 1) scripts Two CSP directives can affect subframe restriction: script-src or default-src fallback when script-src is not present. 2) subframes Three possible CSP directives can affect script restriction: frame-src or (deprecated) child-src or default-src fallback when none of frame-src/child-src is present. This CL adds a GetEffectiveSandboxedPageCSP function, which returns the effective CSP chrome will use when a certain sandbox CSP (sandbox.content_security_policy key) is specified in app's manifest. The effective CSP is a restricted CSP that strips out any remote hosts from #1 and #2. Any directive source value that is not surrounded by single quotes are considered remote hosts. The approach is to add 'self' as source value by default if 'none' was not specified. TODO: Update sandbox documentation page. BUG=615585 Test=Loading a remote script from foo.com inside a chrome apps sandbox page should not be possible, even with setting the following "sandbox.content_security_policy" key in the manifest: script-src: foo.com Same would go with loading <iframe> elements. It should not be possible to load web content <iframe>s inside apps. Committed: https://crrev.com/c0304f6696623d76672a4c8fbefa16378c3137b8 Cr-Commit-Position: refs/heads/master@{#441208} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c0304f6696623d76672a4c8fbefa16378c3137b8 Cr-Commit-Position: refs/heads/master@{#441208}
Message was sent while issue was closed.
On 2016/12/28 19:14:09, lazyboy wrote: > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > File chrome/common/extensions/docs/templates/articles/manifest/sandbox.html > (right): > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:4: > <b><em>Warning:</em></b> Starting version 57, Chrome will no longer load > On 2016/12/28 16:42:37, Devlin wrote: > > nitty nit: starting *in* version 57 > > Done. > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:5: > external web content or web scripts inside sandboxed pages in favor of > On 2016/12/28 16:42:37, Devlin wrote: > > nitty nit: > > maybe: > > "Starting in version 57, Chrome will no longer allow external web content > > (including embedded frames and scripts) inside sandboxed pages. Please use a > <a > > ...>webview</a> instead." > > > > WDYT? > > Done. > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:52: > directive and may not have the <code>allow-same-origin</code> token (see > On 2016/12/28 16:42:37, Devlin wrote: > > maybe "but it must have the sandbox directive, and may not have the > > allow-same-origin token or allow external web content" or something like that? > > This sentence is talking about directives values of sandbox directive, so I've > put the caution about loading external web content in the next sentence.
Message was sent while issue was closed.
On 2016/12/28 16:42:37, Devlin wrote: > Largely lg; a few last nits. Since it's for public docs, I'd like to take one > last look before commit once changes are made. :) > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > File chrome/common/extensions/docs/templates/articles/manifest/sandbox.html > (right): > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:4: > <b><em>Warning:</em></b> Starting version 57, Chrome will no longer load > nitty nit: starting *in* version 57 > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:5: > external web content or web scripts inside sandboxed pages in favor of > nitty nit: > maybe: > "Starting in version 57, Chrome will no longer allow external web content > (including embedded frames and scripts) inside sandboxed pages. Please use a <a > ...>webview</a> instead." > > WDYT? > > https://codereview.chromium.org/2563843002/diff/220001/chrome/common/extensio... > chrome/common/extensions/docs/templates/articles/manifest/sandbox.html:52: > directive and may not have the <code>allow-same-origin</code> token (see > maybe "but it must have the sandbox directive, and may not have the > allow-same-origin token or allow external web content" or something like that? Chrome Extensions do not allow the use of "webview" - only packaged apps do. There is now no way to load an external web page in a Chrome Extension as of Chrome 57.
Message was sent while issue was closed.
On 2017/03/21 00:24:23, ikemevans wrote: > Chrome Extensions do not allow the use of "webview" - only packaged apps do. > There is now no way to load an external web page in a Chrome Extension as of > Chrome 57. You can still load iframes with web pages inside Chrome Extensions. This CL only applied to Chrome Apps. Note: It's better to file bugs at https://new.crbug.com (or start a discussion at chromium-extension@chromium.org) for things like this rather than posting to closed CLs, where your question might go unseen. |
