Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655463006:
PlzNavigate: Enforce 'frame-src' CSP on the browser. (Closed)
|
Created:
3 years, 11 months ago by arthursonzogni Modified:
3 years, 9 months ago CC:
chromium-reviews, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, Mike West, clamy Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Enforce frame-src CSP on the browser.
Use a NavigationThrottle to check infringement of the 'frame-src' on the
browser-side. Before this patch, a redirect during the navigation could
led to a child frame to be displayed inside its parent, even if it was
disallowed by its parent.
BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2655463006
Cr-Original-Commit-Position: refs/heads/master@{#457757}
Committed: https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f595344402b833bad
Review-Url: https://codereview.chromium.org/2655463006
Cr-Commit-Position: refs/heads/master@{#457945}
Committed: https://chromium.googlesource.com/chromium/src/+/7fed384c1a1d7a6c89531dddbb8b539061a9e455
Patch Set 1 #Patch Set 2 : Fix test:chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html #Patch Set 3 : Rebase #Patch Set 4 : Fix tests. #
Total comments: 62
Patch Set 5 : Addressed comments(alexmos@ and nasko@) #
Total comments: 16
Patch Set 6 : Addressed comments (alexmos@ #2) + Rebase from parent. #Patch Set 7 : Add TODO in the FrameLoader. #
Total comments: 35
Patch Set 8 : Rebase from master and parent. #Patch Set 9 : Addressed comments (nasko@) #Patch Set 10 : Rebase. #Patch Set 11 : Rebase. #Patch Set 12 : Move things from the FTN to the RFH. #Patch Set 13 : Rebase. #
Total comments: 35
Patch Set 14 : Rebase #Patch Set 15 : Addressed comments @alexmos. #
Total comments: 3
Patch Set 16 : Rebase from clamy's patch: https://crrev.com/2727633005/ #Patch Set 17 : Addressed comments #
Total comments: 14
Patch Set 18 : Rebase #Patch Set 19 : Addressed comments #
Total comments: 2
Patch Set 20 : Rebase. #Patch Set 21 : bool -> enum. #Patch Set 22 : Rebase. #Patch Set 23 : Add TODO for https://crbug.com/702540 and fix test expectations #Patch Set 24 : Rebase. #Messages
Total messages: 227 (163 generated)
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by arthursonzogni@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_...)
The CQ bit was checked by arthursonzogni@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by arthursonzogni@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
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_...)
The CQ bit was checked by arthursonzogni@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by arthursonzogni@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...
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
Patchset #4 (id:180001) has been deleted
arthursonzogni@chromium.org changed reviewers: + alexmos@chromium.org, nasko@chromium.org
Hi nasko@, alexmos@ and the others(in CC) Please could you review this CL? It depends on: https://codereview.chromium.org/2612793002/ nasko: content/* alexmos: Do you think you could review everything in this CL that is related to Content-Security-Policy? If you are okay with this change, then I will ask jochen@ to LGTM third_party/Webkit/* Thank you! https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1644: !browserSideNavigationEnabled) { Note: It is possible to check the CSP with PlzNavigate on the browser-side and on the renderer-side at the same time, but I prevent this to happens because 'shouldContinueForNavigationPolicy' is called two times with PlzNavigate: * One for the initial request. * A Second time when the request come back from the browser. The problem is that 'shouldCheckMainWorldContentSecurityPolicy' is does not correspond to reality the second time.
Thanks, Arthur! I made a first, fairly high-level pass through this - a few questions and comments below. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:203: return NavigationThrottle::BLOCK_REQUEST; Will this result in loading a regular error page? I remember we discussed whether to load a blank page with a unique origin (data:,) temporarily until we fix the error pages, because otherwise we can get the problems with the CWS kill and more down the road as we tighten up CanCommitURL for OOPIFs (https://crbug.com/622385, https://crbug.com/689733). Can you check if you get the kill in 622385 with this approach, if a parent page declares a CSP that disallows the CWS URL? Would the frame-src checks happen before XFO blocks it using BLOCK_RESPONSE, for which you used data:, when you moved XFO over to the browser? https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:22: if (!current_frame_host) Is this null check needed? https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:34: bool CSPContextImpl::SchemeShouldBypass(const base::StringPiece& scheme) { Perhaps name this SchemeShouldBypassCSP, so it's clear what the scheme should bypass? (This is what the Blink version of this does.) https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:42: const auto& bypassing_scheme = url::GetCSPBypassingSchemes(); nit: s/bypassing_scheme/bypassing_schemes/ https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:21: void ReportViolation(const CSPViolationParams& violation_params) override; Just a heads-up that lukasza@ worked on some plumbing for CSP violations in https://codereview.chromium.org/2190183002/. This will probably supercede most of that CL, but might be good to look over that CL to compare approaches and to see if any of the discussion applies here. For example, do the races Lukasz discussed in RenderFrameProxyHost::OnForwardContentSecurityPolicyViolation no longer apply here? https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:24: bool SchemeShouldBypass(const base::StringPiece& scheme) override; Can this be static? Doesn't look like it needs frame_tree_node_ for anything. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:419: std::unique_ptr<CSPContext> csp_context_; I'm wondering whether it'd be better to associate csp_context_ and csp_policies_ with RFH instead of FTN? I'm concerned we could end up checking the wrong frame's CSP. E.g., if the current RFH has a CSP, then it becomes a pending delete RFH and adds an iframe which needs to check frame-src, this might end up checking the wrong RFH's CSP policy (current RFH's, rather than pending delete RFH's). We've had this kind of issue with URLs and origins and ended up moving both from FTN to RFH (see issues 590034, 590035, 663740). https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:560: // PlzNavigate Does this have to be specific to PlzNavigate? I'm wondering if we can reuse this to fix https://bugs.chromium.org/p/chromium/issues/detail?id=611232 for OOPIFs as well. https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:7160: if (IsBrowserSideNavigationEnabled()) { I'm not too thrilled that there will be a behavioral difference in how frame-src in blocked between PlzNav and non-PlzNav modes. How feasible is it to have both modes load the error page? Could CheckContentSecurityPolicyFrameSrc just check frame-src regardless of PlzNavigate? https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:7162: // blocked page url. This isn't very intuitive, so perhaps explain why that is? (Errors currently commit with the URL of the original page.) https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:7171: EXPECT_EQ("", frame_title); I'd also check RFHI::last_successful_url() to make sure that points back to old_subframe_url. (In fact, that should be true regardless of whether PlzNavigate is enabled.) https://codereview.chromium.org/2655463006/diff/200001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/common/content... content/common/content_security_policy/csp_context.h:50: // Used in CSPContext::ReportViolation() General note for these CLs: for the new CSP things in content/ that will end up still having Blink equivalents, it might be useful to add a comment explaining what the correspondence is, and how we expect it to evolve over time. https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:62: bool should_bypass_main_world_CSP); for style consistency, I'd favor lowercasing the CSP, i.e., should_bypass_main_world_csp. https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:126: // surround it. It is actually used to bypass the 'frame-src' and 'child-src' nit: drop "actually" https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:126: // surround it. It is actually used to bypass the 'frame-src' and 'child-src' "frames that surround it" is a big vague. Should this be just "parent frame", if this is for frame-src only? I'm not familiar with this bit, but is it used to bypass other directives in Blink? https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:129: bool should_bypass_main_world_CSP; Do we have tests that things work properly when this is true, with PlzNavigate enabled? https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1644: !browserSideNavigationEnabled) { On 2017/02/10 16:42:22, arthursonzogni wrote: > Note: It is possible to check the CSP with PlzNavigate on the browser-side and > on the renderer-side at the same time, but I prevent this to happens because > 'shouldContinueForNavigationPolicy' is called two times with PlzNavigate: > * One for the initial request. > * A Second time when the request come back from the browser. > The problem is that 'shouldCheckMainWorldContentSecurityPolicy' is does not > correspond to reality the second time. I might be fuzzy on how this part works with PlzNavigate, but could you explain more about what's wrong with shouldCheckMainWorldContentSecurityPolicy the second time? https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2065: ContentSecurityPolicy::ViolationType::URLViolation); /* ViolationType */ Do you need to forward the redirect status also, rather than relying on the default? What about contextLine? I saw you filed https://crbug.com/690946, but what would it take to fix it? Also, are we sure that violation type is always going to be ContentSecurityPolicy::URLViolation?
Some preliminary comments from the two files I could get to. I will continue reviewing it next week. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:20: frame_tree_node_->render_manager()->current_frame_host(); nit: No need to indirect through render_manager() call, the current host is exposed through the FrameTreeNode itself. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:25: current_frame_host->AddMessageToConsole(CONSOLE_MESSAGE_LEVEL_ERROR, message); Is the log level always ERROR? https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:6: #define CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_H_ Missing _IMPL_ part in the include guard. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:26: FrameTreeNode* frame_tree_node_; // Never nullptr. nit: Comments like that usually go above the member variable. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:28: } // namespace content nit: Empty line before end of namespace. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:30: #endif // CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_H_ */ Why is there "*/" at the end? https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:181: const std::vector<CSPPolicy>& ContentSecurityPolicies() const { This should be hacker_case(), as it is a simple accessor. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:187: CSPContext* ContentSecurityPolicyContext() { return csp_context_.get(); } Same here, hacker_case().
clamy@chromium.org changed reviewers: + clamy@chromium.org
https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:419: std::unique_ptr<CSPContext> csp_context_; On 2017/02/10 22:59:53, alexmos wrote: > I'm wondering whether it'd be better to associate csp_context_ and csp_policies_ > with RFH instead of FTN? I'm concerned we could end up checking the wrong > frame's CSP. E.g., if the current RFH has a CSP, then it becomes a pending > delete RFH and adds an iframe which needs to check frame-src, this might end up > checking the wrong RFH's CSP policy (current RFH's, rather than pending delete > RFH's). We've had this kind of issue with URLs and origins and ended up moving > both from FTN to RFH (see issues 590034, 590035, 663740). Can the frame navigate while the RFH is in pending-delete mode? I thought we filtered navigation IPCs coming from it while it was in that mode (and if we don't we should). Considering we only check CSP when navigating, this should be fine?
Patchset #5 (id:220001) has been deleted
Thanks for the reviews! I fixed most of the issues. It remains the questions about: * Could we make it works without PlzNavigate? * Is there any race condition problems? I am investigating on these questions. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:203: return NavigationThrottle::BLOCK_REQUEST; On 2017/02/10 22:59:53, alexmos wrote: > Will this result in loading a regular error page? I remember we discussed > whether to load a blank page with a unique origin (data:,) temporarily until we > fix the error pages, because otherwise we can get the problems with the CWS kill > and more down the road as we tighten up CanCommitURL for OOPIFs > (https://crbug.com/622385, https://crbug.com/689733). Can you check if you get > the kill in 622385 with this approach, if a parent page declares a CSP that > disallows the CWS URL? Would the frame-src checks happen before XFO blocks it > using BLOCK_RESPONSE, for which you used data:, when you moved XFO over to the > browser? Yes you are right. I forgot this. XFO checks happens after 'frame-src'. I will have to create a new ThrottleCheckResult for this. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:20: frame_tree_node_->render_manager()->current_frame_host(); On 2017/02/11 00:01:23, nasko wrote: > nit: No need to indirect through render_manager() call, the current host is > exposed through the FrameTreeNode itself. Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:22: if (!current_frame_host) On 2017/02/10 22:59:53, alexmos wrote: > Is this null check needed? It is probably not needed indeed for this case(i.e. frame-src) Maybe in the future if we choose to handle frame-ancestor on the browser-side and the response is 204-No-content? Do you prefer to add a DCHECK for the moment to detect some edges cases we are not aware of? https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:25: current_frame_host->AddMessageToConsole(CONSOLE_MESSAGE_LEVEL_ERROR, message); On 2017/02/11 00:01:23, nasko wrote: > Is the log level always ERROR? Yes, see all calls to ContentSecurityPolicy::logToConsole(..). InfoMessageLevel is used one time, but it is for the message: "The Content-Security-Policy directive {{directive}} is implemented behind a flag which is currently disabled". We don't parse CSP on the browser-side for the moment, so I think the log level will be always ERROR. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:34: bool CSPContextImpl::SchemeShouldBypass(const base::StringPiece& scheme) { On 2017/02/10 22:59:53, alexmos wrote: > Perhaps name this SchemeShouldBypassCSP, so it's clear what the scheme should > bypass? (This is what the Blink version of this does.) Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:42: const auto& bypassing_scheme = url::GetCSPBypassingSchemes(); On 2017/02/10 22:59:53, alexmos wrote: > nit: s/bypassing_scheme/bypassing_schemes/ Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:6: #define CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_H_ On 2017/02/11 00:01:23, nasko wrote: > Missing _IMPL_ part in the include guard. Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:21: void ReportViolation(const CSPViolationParams& violation_params) override; On 2017/02/10 22:59:53, alexmos wrote: > Just a heads-up that lukasza@ worked on some plumbing for CSP violations in > https://codereview.chromium.org/2190183002/. This will probably supercede most > of that CL, but might be good to look over that CL to compare approaches and to > see if any of the discussion applies here. For example, do the races Lukasz > discussed in RenderFrameProxyHost::OnForwardContentSecurityPolicyViolation no > longer apply here? Thanks! I don't know yet if a race could happens, I will try to understand. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:24: bool SchemeShouldBypass(const base::StringPiece& scheme) override; On 2017/02/10 22:59:53, alexmos wrote: > Can this be static? Doesn't look like it needs frame_tree_node_ for anything. Yes, it doesn't need frame_tree_node_, but it's a virtual method, so it can't be static. In my previous patch: https://codereview.chromium.org/2612793002/ The code in content/common/content_security_policy/* is in a way self-contained. The behavior/state it depends on are implemented here in CSPContextImpl. It is also implemented in a different way in tests. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:26: FrameTreeNode* frame_tree_node_; // Never nullptr. On 2017/02/11 00:01:23, nasko wrote: > nit: Comments like that usually go above the member variable. Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:28: } // namespace content On 2017/02/11 00:01:23, nasko wrote: > nit: Empty line before end of namespace. Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:30: #endif // CONTENT_BROWSER_FRAME_HOST_CSP_CONTEXT_H_ */ On 2017/02/11 00:01:23, nasko wrote: > Why is there "*/" at the end? Oops, vim-snipmate + wrong conversion to the google coding style. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:181: const std::vector<CSPPolicy>& ContentSecurityPolicies() const { On 2017/02/11 00:01:23, nasko wrote: > This should be hacker_case(), as it is a simple accessor. Okay, I didn't know about this coding style rule. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:187: CSPContext* ContentSecurityPolicyContext() { return csp_context_.get(); } On 2017/02/11 00:01:23, nasko wrote: > Same here, hacker_case(). Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:560: // PlzNavigate On 2017/02/10 22:59:53, alexmos wrote: > Does this have to be specific to PlzNavigate? I'm wondering if we can reuse > this to fix https://bugs.chromium.org/p/chromium/issues/detail?id=611232 for > OOPIFs as well. I will probably need to forward the should_bypass_main_world from the renderer to the NavigationThrottle. It sounds feasible. https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:7162: // blocked page url. On 2017/02/10 22:59:53, alexmos wrote: > This isn't very intuitive, so perhaps explain why that is? (Errors currently > commit with the URL of the original page.) Done. https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:7171: EXPECT_EQ("", frame_title); On 2017/02/10 22:59:53, alexmos wrote: > I'd also check RFHI::last_successful_url() to make sure that points back to > old_subframe_url. (In fact, that should be true regardless of whether > PlzNavigate is enabled.) RFHI::last_successful_url() returns the empty url with PlzNavigate. The navigation is cross-origin, so we use another RFHI, not the previous one. There is no last successful url. https://codereview.chromium.org/2655463006/diff/200001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/common/content... content/common/content_security_policy/csp_context.h:50: // Used in CSPContext::ReportViolation() On 2017/02/10 22:59:53, alexmos wrote: > General note for these CLs: for the new CSP things in content/ that will end up > still having Blink equivalents, it might be useful to add a comment explaining > what the correspondence is, and how we expect it to evolve over time. Acknowledged. https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:62: bool should_bypass_main_world_CSP); On 2017/02/10 22:59:53, alexmos wrote: > for style consistency, I'd favor lowercasing the CSP, i.e., > should_bypass_main_world_csp. Done. https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:126: // surround it. It is actually used to bypass the 'frame-src' and 'child-src' On 2017/02/10 22:59:53, alexmos wrote: > nit: drop "actually" Done. I misunderstood what "main_world" means, it is simply the opposite of "isolated_world". This comment is very bad, I will update it. AFAIK, it is used for 'frame-src' and not for 'form-action'. It is also used for others CSP, for instance 'style-src', 'script-src', ..., but I only care about CSP when it can block a navigation. https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:129: bool should_bypass_main_world_CSP; On 2017/02/10 22:59:53, alexmos wrote: > Do we have tests that things work properly when this is true, with PlzNavigate > enabled? Yes, I did this to make this test work: http/tests/security/isolatedWorld/bypass-main-world-csp-iframes.html I was able to remove the failure expectation from: third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1644: !browserSideNavigationEnabled) { On 2017/02/10 22:59:53, alexmos wrote: > On 2017/02/10 16:42:22, arthursonzogni wrote: > > Note: It is possible to check the CSP with PlzNavigate on the browser-side and > > on the renderer-side at the same time, but I prevent this to happens because > > 'shouldContinueForNavigationPolicy' is called two times with PlzNavigate: > > * One for the initial request. > > * A Second time when the request come back from the browser. > > The problem is that 'shouldCheckMainWorldContentSecurityPolicy' is does not > > correspond to reality the second time. > > I might be fuzzy on how this part works with PlzNavigate, but could you explain > more about what's wrong with shouldCheckMainWorldContentSecurityPolicy the > second time? With PlzNavigate, we currently use a dirty hack to commit navigation inside blink. It is temporarily. After the navigation has been done in the browser, it is committed inside the renderer. The browser sends a request to the renderer, but in a way, it is the response. It is essentially a request with a few attributes that tells the renderer that it doesn't have to submit a new request and where it can find the response. So 2 requests are made, one for the request and one for the "response". For the second request, the renderer doesn't know if the request was submitted from an isolated world and it will block the request if we don't prevent it to do so. https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2065: ContentSecurityPolicy::ViolationType::URLViolation); /* ViolationType */ On 2017/02/10 22:59:54, alexmos wrote: > Do you need to forward the redirect status also, rather than relying on the > default? Yes, I completely ignored the redirect status. Thanks! I need to fix it. > What about contextLine? I saw you filed https://crbug.com/690946, but what > would it take to fix it? I don't know, it could be as easy as adding one more attribute to the navigation params. > Also, are we sure that violation type is always going to be > ContentSecurityPolicy::URLViolation? Yes, the violation type is always going to be a ContentSecurityPolicy::URLViolation. The other choices are: InlineViolation and EvalViolation. As far as it is used only in PlzNavigate, it will be for blocking a navigation.
https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:419: std::unique_ptr<CSPContext> csp_context_; On 2017/02/13 13:23:28, clamy wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > I'm wondering whether it'd be better to associate csp_context_ and > csp_policies_ > > with RFH instead of FTN? I'm concerned we could end up checking the wrong > > frame's CSP. E.g., if the current RFH has a CSP, then it becomes a pending > > delete RFH and adds an iframe which needs to check frame-src, this might end > up > > checking the wrong RFH's CSP policy (current RFH's, rather than pending delete > > RFH's). We've had this kind of issue with URLs and origins and ended up > moving > > both from FTN to RFH (see issues 590034, 590035, 663740). > > Can the frame navigate while the RFH is in pending-delete mode? I thought we > filtered navigation IPCs coming from it while it was in that mode (and if we > don't we should). Considering we only check CSP when navigating, this should be > fine? Yes, I wasn't sure how much was disallowed in unload handlers currently, but after double-checking with dcheng@, unload handlers should not be able to create new child frames or navigate. So I agree this isn't a concern then. I wonder if there could be other races though. E.g., suppose we have A-embed-B with B in an OOPIF, and A navigates to C. In parallel with the commit for C, B navigates to D (before B's process knows that its parent is about to go away), with that IPC arriving just after C commits (and before A's children are cleaned up). In the browser, will we try to check frame-src for D (which presumably would need A's CSP and not C's CSP), or will we recognize that its parent RFH A is pending deletion and abort the navigation? Not sure whether this is actually possible today; if the navigation to D does make it to browser process, the right thing to do is probably to abort it if any of the ancestor RFHs are pending deletion to match what Blink does (i.e., we should ensure that we never need to check CSP frame-src using a pending delete RFH). And perhaps it might be worthwhile to add something like CHECK(navigating_rfh->GetParent()->is_active()) when enforcing frame-src?
https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:22: if (!current_frame_host) On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > Is this null check needed? > > It is probably not needed indeed for this case(i.e. frame-src) > Maybe in the future if we choose to handle frame-ancestor on the browser-side > and the response is 204-No-content? > > Do you prefer to add a DCHECK for the moment to detect some edges cases we are > not aware of? I haven't ever seen any code null-checking current_frame_host(), out of numerous call sites, so my assumption is that something else will break very quickly if it happens to be null. You could add the DCHECK, but if it's null, we'll just crash on the next line and we will get some crash reports anyway, so I don't think it's really needed. I'm not exactly sure how the 204 works, but that should keep the RFH created initially (in RFHM::Init), no? See also this comment in ~RFHM: // We should always have a current RenderFrameHost except in some tests. SetRenderFrameHost(std::unique_ptr<RenderFrameHostImpl>()); (not sure what tests it's talking about, but I'd rather only include the null-check if any tests actually need it.) https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:24: bool SchemeShouldBypass(const base::StringPiece& scheme) override; On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > Can this be static? Doesn't look like it needs frame_tree_node_ for anything. > > Yes, it doesn't need frame_tree_node_, but it's a virtual method, so it can't be > static. > In my previous patch: > https://codereview.chromium.org/2612793002/ > The code in content/common/content_security_policy/* is in a way self-contained. > The behavior/state it depends on are implemented here in CSPContextImpl. It is > also implemented in a different way in tests. Acknowledged. https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:7171: EXPECT_EQ("", frame_title); On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > I'd also check RFHI::last_successful_url() to make sure that points back to > > old_subframe_url. (In fact, that should be true regardless of whether > > PlzNavigate is enabled.) > > RFHI::last_successful_url() returns the empty url with PlzNavigate. The > navigation is cross-origin, so we use another RFHI, not the previous one. There > is no last successful url. Ah, got it, thanks. That's actually really great; thanks for adding the comment with the explanation. I wish this would happen without PlzNavigate too. :) https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:126: // surround it. It is actually used to bypass the 'frame-src' and 'child-src' On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > nit: drop "actually" > > Done. I misunderstood what "main_world" means, it is simply the opposite of > "isolated_world". This comment is very bad, I will update it. > AFAIK, it is used for 'frame-src' and not for 'form-action'. > > It is also used for others CSP, for instance 'style-src', 'script-src', ..., but > I only care about CSP when it can block a navigation. Interesting, thanks. It's sad that an evil renderer can just pass this bit to turn off the enforcement of frame-src in the browser process. Perhaps down the road we could add some validation in the browser process, such as whether the renderer process really contained extension content scripts that could do this. By the way, it doesn't seem like the isolated world's CSP is actually enforced today (looking at how DOMWrapperWorld::setIsolatedWorldContentSecurityPolicy ignores the passed-in policy). It's only used to flip the bit that allows bypassing the main world CSP. So it doesn't seem like we need to worry about how to enforce frame-src from the isolated world's CSP, for example. https://codereview.chromium.org/2655463006/diff/200001/content/common/navigat... content/common/navigation_params.h:129: bool should_bypass_main_world_CSP; On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > Do we have tests that things work properly when this is true, with PlzNavigate > > enabled? > > Yes, I did this to make this test work: > http/tests/security/isolatedWorld/bypass-main-world-csp-iframes.html > > I was able to remove the failure expectation from: > third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation Acknowledged. https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1644: !browserSideNavigationEnabled) { On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > On 2017/02/10 16:42:22, arthursonzogni wrote: > > > Note: It is possible to check the CSP with PlzNavigate on the browser-side > and > > > on the renderer-side at the same time, but I prevent this to happens because > > > 'shouldContinueForNavigationPolicy' is called two times with PlzNavigate: > > > * One for the initial request. > > > * A Second time when the request come back from the browser. > > > The problem is that 'shouldCheckMainWorldContentSecurityPolicy' is does not > > > correspond to reality the second time. > > > > I might be fuzzy on how this part works with PlzNavigate, but could you > explain > > more about what's wrong with shouldCheckMainWorldContentSecurityPolicy the > > second time? > > With PlzNavigate, we currently use a dirty hack to commit navigation inside > blink. It is temporarily. After the navigation has been done in the browser, it > is committed inside the renderer. The browser sends a request to the renderer, > but in a way, it is the response. It is essentially a request with a few > attributes that tells the renderer that it doesn't have to submit a new request > and where it can find the response. > So 2 requests are made, one for the request and one for the "response". > > For the second request, the renderer doesn't know if the request was submitted > from an isolated world and it will block the request if we don't prevent it to > do so. Acknowledged, thanks for the explanation. Can you please add a brief comment here about why this is disabled for PlzNavigate, and reference a bug (if you have it) for removing that hack? https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/interstitial_page_navigator_impl.cc:45: ); nit: this looks a bit weird on its own line. Can you just combine this with the ) on the previous line into "));"? If the comment outside the ) seems too weird, you could always use /* should_bypass_main_world_csp */ inside the parens, like you do below. https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:462: bool NavigationHandleImpl::ShouldBypassMainWorldCSP() const { nit: you can move this simple getter into to the header as should_bypass_main_world_csp(). https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:497: // instance from a chrome extension. When it returns true, the navigation nit: I'd also mention content scripts, and make it into a complete sentence (or combine with previous one). Also, "When it returns true," -> "When true," (nothing returned here). Perhaps something like: "Whether or not the navigation has been started from an isolated world, such as from an extension content script. When true, the navigation should not be blocked by the parent frame's CSP." https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:498: // should not be blocked by the parent frame's Content-Security-Policy(CSP). nit: probably mentioning one of Content Security Policy or CSP is enough. https://codereview.chromium.org/2655463006/diff/240001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/240001/content/common/content... content/common/content_security_policy/csp_context.h:87: // Whether or not the violation happens after a redirection. nit: s/redirection/redirect/ https://codereview.chromium.org/2655463006/diff/240001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2655463006/diff/240001/content/public/browser... content/public/browser/navigation_handle.h:283: // instance from a chrome-extension. When it returns true, the navigation See my comment about the earlier copy of this comment :) https://codereview.chromium.org/2655463006/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:93: // Whether or not the violation happens after a redirection. nit: s/redirection/redirect/
Patchset #6 (id:250001) has been deleted
Patchset #6 (id:270001) has been deleted
Patchset #6 (id:290001) has been deleted
Thanks alexmos@! https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/csp_context_impl.cc:22: if (!current_frame_host) On 2017/02/14 06:57:19, alexmos (OOO until 2-21) wrote: > On 2017/02/13 16:33:20, arthursonzogni wrote: > > On 2017/02/10 22:59:53, alexmos wrote: > > > Is this null check needed? > > > > It is probably not needed indeed for this case(i.e. frame-src) > > Maybe in the future if we choose to handle frame-ancestor on the browser-side > > and the response is 204-No-content? > > > > Do you prefer to add a DCHECK for the moment to detect some edges cases we are > > not aware of? > > I haven't ever seen any code null-checking current_frame_host(), out of numerous > call sites, so my assumption is that something else will break very quickly if > it happens to be null. You could add the DCHECK, but if it's null, we'll just > crash on the next line and we will get some crash reports anyway, so I don't > think it's really needed. > > I'm not exactly sure how the 204 works, but that should keep the RFH created > initially (in RFHM::Init), no? > > See also this comment in ~RFHM: > // We should always have a current RenderFrameHost except in some tests. > SetRenderFrameHost(std::unique_ptr<RenderFrameHostImpl>()); > > (not sure what tests it's talking about, but I'd rather only include the > null-check if any tests actually need it.) You are probably right. I will remove the "if (!current_frame_host)". I initially put this because of this patch: https://codereview.chromium.org/2606933002/ But it doesn't apply to this case I think. https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/interstitial_page_navigator_impl.cc:45: ); On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > nit: this looks a bit weird on its own line. Can you just combine this with the > ) on the previous line into "));"? > If the comment outside the ) seems too weird, you could always use /* > should_bypass_main_world_csp */ inside the parens, like you do below. Yes, it looks weird to me too. The code formatter forces me to keep it like this. I will apply your suggestion. edit: I can't align the comments now :-( https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:462: bool NavigationHandleImpl::ShouldBypassMainWorldCSP() const { On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > nit: you can move this simple getter into to the header as > should_bypass_main_world_csp(). Done, but I can't move the implementation in the header, it is disallowed by the code formatter. Probably because it is an override. https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:497: // instance from a chrome extension. When it returns true, the navigation On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > nit: I'd also mention content scripts, and make it into a complete sentence (or > combine with previous one). Also, "When it returns true," -> "When true," > (nothing returned here). Perhaps something like: "Whether or not the > navigation has been started from an isolated world, such as from an extension > content script. When true, the navigation should not be blocked by the parent > frame's CSP." Done. https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:498: // should not be blocked by the parent frame's Content-Security-Policy(CSP). On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > nit: probably mentioning one of Content Security Policy or CSP is enough. Done. https://codereview.chromium.org/2655463006/diff/240001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/240001/content/common/content... content/common/content_security_policy/csp_context.h:87: // Whether or not the violation happens after a redirection. On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > nit: s/redirection/redirect/ Done. https://codereview.chromium.org/2655463006/diff/240001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2655463006/diff/240001/content/public/browser... content/public/browser/navigation_handle.h:283: // instance from a chrome-extension. When it returns true, the navigation On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > See my comment about the earlier copy of this comment :) Done. https://codereview.chromium.org/2655463006/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:93: // Whether or not the violation happens after a redirection. On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > nit: s/redirection/redirect/ Done.
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirection during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
alexmos@, you asked if it would be possible to make this patch working without Plznavigate as well. I think there is two things I need to add to make it possible: 1) Passing shouldCheckMainWorldContentSecurityPolicy to the browser, like it was done with PlzNavigate with common_params.should_check_main_world_csp. 2) Solving the error-page crash with the ChromeWebStore. I am working on a solution in https://codereview.chromium.org/2697713005/, but it only works with PlzNavigate. It sounds feasible, but since we will try PlzNavigate on Canary ~the next week, I really would like to commit this patch without non-plznavigate support and works on it then. A few others answers below: https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:203: return NavigationThrottle::BLOCK_REQUEST; On 2017/02/13 16:33:20, arthursonzogni wrote: > On 2017/02/10 22:59:53, alexmos wrote: > > Will this result in loading a regular error page? I remember we discussed > > whether to load a blank page with a unique origin (data:,) temporarily until > we > > fix the error pages, because otherwise we can get the problems with the CWS > kill > > and more down the road as we tighten up CanCommitURL for OOPIFs > > (https://crbug.com/622385, https://crbug.com/689733). Can you check if you > get > > the kill in 622385 with this approach, if a parent page declares a CSP that > > disallows the CWS URL? Would the frame-src checks happen before XFO blocks it > > using BLOCK_RESPONSE, for which you used data:, when you moved XFO over to the > > browser? > > Yes you are right. I forgot this. > XFO checks happens after 'frame-src'. > I will have to create a new ThrottleCheckResult for this. I am working on a solution. See https://codereview.chromium.org/2697713005/ Now, the browser will not ask the renderer to commit an error page with an URL that would make the browser kills the renderer if it tries to commit it. Unfortunately, it only works with PlzNavigate. https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1644: !browserSideNavigationEnabled) { On 2017/02/14 06:57:20, alexmos (OOO until 2-21) wrote: > On 2017/02/13 16:33:20, arthursonzogni wrote: > > On 2017/02/10 22:59:53, alexmos wrote: > > > On 2017/02/10 16:42:22, arthursonzogni wrote: > > > > Note: It is possible to check the CSP with PlzNavigate on the browser-side > > and > > > > on the renderer-side at the same time, but I prevent this to happens > because > > > > 'shouldContinueForNavigationPolicy' is called two times with PlzNavigate: > > > > * One for the initial request. > > > > * A Second time when the request come back from the browser. > > > > The problem is that 'shouldCheckMainWorldContentSecurityPolicy' is does > not > > > > correspond to reality the second time. > > > > > > I might be fuzzy on how this part works with PlzNavigate, but could you > > explain > > > more about what's wrong with shouldCheckMainWorldContentSecurityPolicy the > > > second time? > > > > With PlzNavigate, we currently use a dirty hack to commit navigation inside > > blink. It is temporarily. After the navigation has been done in the browser, > it > > is committed inside the renderer. The browser sends a request to the renderer, > > but in a way, it is the response. It is essentially a request with a few > > attributes that tells the renderer that it doesn't have to submit a new > request > > and where it can find the response. > > So 2 requests are made, one for the request and one for the "response". > > > > For the second request, the renderer doesn't know if the request was submitted > > from an isolated world and it will block the request if we don't prevent it to > > do so. > > Acknowledged, thanks for the explanation. Can you please add a brief comment > here about why this is disabled for PlzNavigate, and reference a bug (if you > have it) for removing that hack? Done. See http://crbug.com/692595
I'm personally ok landing this CL without unifying PlzNavigate and old navigation to behave the same or the CL to work in both cases. It can be done as a follow up. https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:419: std::unique_ptr<CSPContext> csp_context_; On 2017/02/14 05:44:28, alexmos (OOO until 2-21) wrote: > On 2017/02/13 13:23:28, clamy wrote: > > On 2017/02/10 22:59:53, alexmos wrote: > > > I'm wondering whether it'd be better to associate csp_context_ and > > csp_policies_ > > > with RFH instead of FTN? I'm concerned we could end up checking the wrong > > > frame's CSP. E.g., if the current RFH has a CSP, then it becomes a pending > > > delete RFH and adds an iframe which needs to check frame-src, this might end > > up > > > checking the wrong RFH's CSP policy (current RFH's, rather than pending > delete > > > RFH's). We've had this kind of issue with URLs and origins and ended up > > moving > > > both from FTN to RFH (see issues 590034, 590035, 663740). > > > > Can the frame navigate while the RFH is in pending-delete mode? I thought we > > filtered navigation IPCs coming from it while it was in that mode (and if we > > don't we should). Considering we only check CSP when navigating, this should > be > > fine? > > Yes, I wasn't sure how much was disallowed in unload handlers currently, but > after double-checking with dcheng@, unload handlers should not be able to create > new child frames or navigate. So I agree this isn't a concern then. > > I wonder if there could be other races though. E.g., suppose we have A-embed-B > with B in an OOPIF, and A navigates to C. In parallel with the commit for C, B > navigates to D (before B's process knows that its parent is about to go away), > with that IPC arriving just after C commits (and before A's children are cleaned > up). In the browser, will we try to check frame-src for D (which presumably > would need A's CSP and not C's CSP), or will we recognize that its parent RFH A > is pending deletion and abort the navigation? Not sure whether this is actually > possible today; if the navigation to D does make it to browser process, the > right thing to do is probably to abort it if any of the ancestor RFHs are > pending deletion to match what Blink does (i.e., we should ensure that we never > need to check CSP frame-src using a pending delete RFH). And perhaps it might > be worthwhile to add something like > CHECK(navigating_rfh->GetParent()->is_active()) when enforcing frame-src? I also think that the CSP is better off associated with RFH instead of FTN. If we had a document concept, that would've been even better, but RFH is the closest we have. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { Why do we need to subclass? Also, missing a class level comment. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:181: const std::vector<CSPPolicy>& csp_policies() const { return csp_policies_; } nit: I would move this higher in the file with all the other simple accessors. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:183: // Return the Content-Security-Policy context associated to this frame. nit: "associated with" https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:417: std::unique_ptr<CSPContext> csp_context_; What is the difference between a context and a policy? https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/interstitial_page_navigator_impl.cc:44: false /* should_bypass_main_world_csp */)); nit: Align vertically the comment with the other comments. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:496: // Whether or not the navigation has been issued by a content script or an nit: content script is an extensions system detail that probably doesn't belong in content/. What matters is whether it was issued by an isolated world, which is the web platform concept we should be using. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:702: } I think this code along with the necessary changes to NavigationHandleImpl should land separately, as it is just a bug. It will also include unit tests to ensure it works properly and not pollute this CL. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:563: void ContentSecurityPolicyViolation( nit: Start the method name with a verb, maybe Report... https://codereview.chromium.org/2655463006/diff/330001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/content... content/common/content_security_policy/csp_context.h:72: // The console message that was displayed to the user. "to be displayed"? This will be sent to the renderer to display the error in the future, right? https://codereview.chromium.org/2655463006/diff/330001/content/common/content... content/common/content_security_policy/csp_context.h:82: // The raw content security policy header that was infringed. nit: s/infringed/violated/ https://codereview.chromium.org/2655463006/diff/330001/content/common/content... content/common/content_security_policy/csp_context.h:90: bool followed_redirect; nit: after_redirect? https://codereview.chromium.org/2655463006/diff/330001/content/common/content... File content/common/content_security_policy/csp_policy.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/content... content/common/content_security_policy/csp_policy.cc:152: // original header This struct now defines a member for the original header. Is this intentionally still left blank? https://codereview.chromium.org/2655463006/diff/330001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/frame_m... content/common/frame_messages.h:913: // policy was infringed. nit: s/infringed/violated/, so it is more consistent wording with "violation" used in the IPC message name and params. https://codereview.chromium.org/2655463006/diff/330001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/navigat... content/common/navigation_params.h:128: bool should_bypass_main_world_csp; nit: Shouldn't we name the variable based on what it actually means instead of on what the desired effect of it is? If it indicates that this was initiated in an isolated world, then "is_initiated_from_isolated_world" seems more appropriate. The code checking the variable later can make a decision whether to bypass the CSP of main world or not. https://codereview.chromium.org/2655463006/diff/330001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/public/browser... content/public/browser/navigation_handle.h:285: virtual bool should_bypass_main_world_csp() const = 0; Why is this method on the public interface? I don't see any code outside of content/ using it. https://codereview.chromium.org/2655463006/diff/330001/content/renderer/conte... File content/renderer/content_security_policy_util.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/renderer/conte... content/renderer/content_security_policy_util.cc:65: for (size_t i = 0; i < violation_params.report_endpoints.size(); ++i) This for loop needs {} as the body spans more than one line.
Thanks Nasko! A few response below: https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/02/15 21:28:44, nasko wrote: > Why do we need to subclass? Because I have to implement the virtual methods The CSPContext represents the system on which the CSP are checked on. A system can report violation to the user, it can display messages on the console and it tells you when the CSP should by bypassed. By doing this, the CSP classes are not tied to anything like the CSP classes inside blink were. If one day, we want to check the CSP in another place, the network stack or the renderer for instance, you can. You will just have to implement these 3 functions. It is also useful in my test. I can make a custom implementation of CSPContext to check the console message and test what happens when a scheme is bypassed by the CSP for instance. > Also, missing a class level comment. Done. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:181: const std::vector<CSPPolicy>& csp_policies() const { return csp_policies_; } On 2017/02/15 21:28:44, nasko wrote: > nit: I would move this higher in the file with all the other simple accessors. Done. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:417: std::unique_ptr<CSPContext> csp_context_; On 2017/02/15 21:28:44, nasko wrote: > What is the difference between a context and a policy? Please see my response above. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/interstitial_page_navigator_impl.cc:44: false /* should_bypass_main_world_csp */)); On 2017/02/15 21:28:44, nasko wrote: > nit: Align vertically the comment with the other comments. I can't because of the formatter. I don't know why. Maybe my next try will be better. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:496: // Whether or not the navigation has been issued by a content script or an On 2017/02/15 21:28:44, nasko wrote: > nit: content script is an extensions system detail that probably doesn't belong > in content/. What matters is whether it was issued by an isolated world, which > is the web platform concept we should be using. Okay, I don't know exactly know what is a content script. I added this comment on alexmos@ suggestion. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:702: } On 2017/02/15 21:28:44, nasko wrote: > I think this code along with the necessary changes to NavigationHandleImpl > should land separately, as it is just a bug. It will also include unit tests to > ensure it works properly and not pollute this CL. Acknowledged. Please see: https://codereview.chromium.org/2698623006/ https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:563: void ContentSecurityPolicyViolation( On 2017/02/15 21:28:44, nasko wrote: > nit: Start the method name with a verb, maybe Report... Done. https://codereview.chromium.org/2655463006/diff/330001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/content... content/common/content_security_policy/csp_context.h:90: bool followed_redirect; On 2017/02/15 21:28:44, nasko wrote: > nit: after_redirect? I followed the naming I've found in: https://codereview.chromium.org/2190183002/ But yes, why not. Thanks. https://codereview.chromium.org/2655463006/diff/330001/content/common/content... File content/common/content_security_policy/csp_policy.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/content... content/common/content_security_policy/csp_policy.cc:152: // original header On 2017/02/15 21:28:44, nasko wrote: > This struct now defines a member for the original header. Is this intentionally > still left blank? I don't understand your question. Maybe I will have to store the raw policy header inside the CSPPolicy/ContentSecurityPolicy object. I will work on it. https://codereview.chromium.org/2655463006/diff/330001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/frame_m... content/common/frame_messages.h:913: // policy was infringed. On 2017/02/15 21:28:44, nasko wrote: > nit: s/infringed/violated/, so it is more consistent wording with "violation" > used in the IPC message name and params. Done. https://codereview.chromium.org/2655463006/diff/330001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/common/navigat... content/common/navigation_params.h:128: bool should_bypass_main_world_csp; On 2017/02/15 21:28:44, nasko wrote: > nit: Shouldn't we name the variable based on what it actually means instead of > on what the desired effect of it is? If it indicates that this was initiated in > an isolated world, then "is_initiated_from_isolated_world" seems more > appropriate. The code checking the variable later can make a decision whether to > bypass the CSP of main world or not. I agree. Edit: I don't know, I am using the same name as where I found this value: FrameLoadRequest:shouldCheckMainWorldContentSecurityPolicy() I would have to check that it really mean that the request was issued from an isolated world and it doesn't depend on any other thing. And also updating all these things in blink. https://codereview.chromium.org/2655463006/diff/330001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/public/browser... content/public/browser/navigation_handle.h:285: virtual bool should_bypass_main_world_csp() const = 0; On 2017/02/15 21:28:44, nasko wrote: > Why is this method on the public interface? I don't see any code outside of > content/ using it. You are right. https://codereview.chromium.org/2655463006/diff/330001/content/renderer/conte... File content/renderer/content_security_policy_util.cc (right): https://codereview.chromium.org/2655463006/diff/330001/content/renderer/conte... content/renderer/content_security_policy_util.cc:65: for (size_t i = 0; i < violation_params.report_endpoints.size(); ++i) On 2017/02/15 21:28:45, nasko wrote: > This for loop needs {} as the body spans more than one line. Done.
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
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_...)
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
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_...)
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/02/15 21:28:45, nasko (out until Feb 27th) wrote: > I'm personally ok landing this CL without unifying PlzNavigate and old > navigation to behave the same or the CL to work in both cases. It can be done as > a follow up. I'm ok with this too. I was mostly curious on what it would take; doing it in a followup CL is totally fine. I'm back from a vacation and can take another pass through this. Is the CL in stable shape right now? I see that the latest PS moves CSP things from FTN to RFH but the bots are red.
Patchset #13 (id:450001) has been deleted
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
In the latest patch, I applied suggestions from you(alexmos@) and nasko@, i.e moving the CSP from the frame_tree_node to the render_frame_host. I hope I did it well. So yes, the CL is in a stable shape right now. I got a patch failure in my last git-try :( I would be happy to have one more pass through this CL, Thank you very much! Nevertheless, it looks like I have a problem with: SiteDetailsBrowserTest.PlatformAppsNotIsolated From what I understand, it is a PlatformApp, it loads a sandboxed iframe that tries to navigate toward 127.0.0.1 It can't because the CSP of a sandboxed iframe. kDefaultSandboxedPageContentSecurityPolicy(=> "frame-src 'self'") The problems in the test is that details->GetOutOfProcessIframeCount() returns 1 instead of 0 I think it is because we now loads an error page instead of loading nothing. I don't know exactly why it uses an out of process iframe for loading the error page. Due to http://crbug.com/612711, we are not isolating iframes from platform apps with --isolate-extensions for the moment. That's what the test is checking. I will investigate, but maybe you will have an immediate understanding of the problem since you know the details of site isolation.
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/02/22 13:46:22, arthursonzogni wrote: > In the latest patch, I applied suggestions from you(alexmos@) and nasko@, i.e > moving the CSP from the frame_tree_node to the render_frame_host. I hope I did > it well. > > So yes, the CL is in a stable shape right now. I got a patch failure in my last > git-try :( > I would be happy to have one more pass through this CL, Thank you very much! > > Nevertheless, it looks like I have a problem with: > SiteDetailsBrowserTest.PlatformAppsNotIsolated > > From what I understand, it is a PlatformApp, it loads a sandboxed iframe that > tries to navigate toward 127.0.0.1 > > It can't because the CSP of a sandboxed iframe. > kDefaultSandboxedPageContentSecurityPolicy(=> "frame-src 'self'") > > The problems in the test is that > details->GetOutOfProcessIframeCount() returns 1 instead of 0 > I think it is because we now loads an error page instead of loading nothing. > I don't know exactly why it uses an out of process iframe for loading the error > page. > > Due to http://crbug.com/612711, we are not isolating iframes from platform apps > with --isolate-extensions for the moment. That's what the test is checking. > > I will investigate, but maybe you will have an immediate understanding of the > problem since you know the details of site isolation. I haven't followed that bug closely (lazyboy/creis would have more context), but it seems like the test is just wrong now, since https://codereview.chromium.org/2563843002 added those CSP restrictions to disallow web content inside iframes on sandboxed app pages for good. This is supposed to be deprecated starting in M57 - see also https://crbug.com/615585. Perhaps ping lazyboy@ to double-check what the test should be doing now, if anything? That CL also removed the platform app exception from DoesSiteRequireDedicatedProcess, which probably explains why the error page is placed into an OOPIF. Would this reintroduce the wrong storage partition kill from 612711 for that error page, with PlzNavigate enabled?
Okay, I will ping him. +CC @lazyboy
Thanks, this is looking good! Some more comments below. https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:462: bool NavigationHandleImpl::ShouldBypassMainWorldCSP() const { On 2017/02/15 09:26:09, arthursonzogni wrote: > On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > > nit: you can move this simple getter into to the header as > > should_bypass_main_world_csp(). > > Done, but I can't move the implementation in the header, it is disallowed by the > code formatter. Probably because it is an override. Ack, though that's surprising to me, as should_bypass_main_world_csp() is not overriding anything from NavigationHandle? https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/02/16 17:32:40, arthursonzogni wrote: > On 2017/02/15 21:28:44, nasko wrote: > > Why do we need to subclass? > Because I have to implement the virtual methods > > The CSPContext represents the system on which the CSP are checked on. A system > can report violation to the user, it can display messages on the console and it > tells you when the CSP should by bypassed. > > By doing this, the CSP classes are not tied to anything like the CSP classes > inside blink were. > > If one day, we want to check the CSP in another place, the network stack or the > renderer for instance, you can. You will just have to implement these 3 > functions. > > It is also useful in my test. I can make a custom implementation of CSPContext > to check the console message and test what happens when a scheme is bypassed by > the CSP for instance. > > > Also, missing a class level comment. > Done. As an alternative to this, would it be easier if RenderFrameHostImpl just extended CSPContext and implemented the three functions directly? Just curious how this compares to having CSPContextImpl as an accessor on RFHI, since CSPContextImpl is tied to its RFHI anyway. https://codereview.chromium.org/2655463006/diff/470001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2655463006/diff/470001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:48: // PlzNavigate: The first local frame has been replaced by an error page. nit: s/has been/will be/ Alternatively, we could just remove "the local frame will still be there. Therefore, expect the local frame to respond again." and this sentence. Since the eventual goal is to always go to the error page in both modes, and we will probably forget to go back and update this comment once we do that. Just saying that we rely on the SecurityPolicyViolationEvent to detect that the frame has been blocked might be good enough. https://codereview.chromium.org/2655463006/diff/470001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/BUILD.... content/browser/BUILD.gn:613: "frame_host/csp_context_impl.cc", Does this need the .h also? https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:172: // If PlzNavigate is enabled, "frame-src" is enforced on the browser-side, nit: "browser side", "renderer side" (no -). Same in some other places below as well. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: // The CSPContext class represents the system on which the CSP are enforced. nit: s/are/is/ https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:21: CSPContextImpl(RenderFrameHostImpl* render_frame); nit: explicit https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1847: const std::vector<ContentSecurityPolicy>& policies) { This wasn't in this CL, but I'm a bit confused by this. Why is this a vector? Wouldn't one header correspond to one policy? (I.e., I thought this message is sent multiple times, each time a new header, csp in <meta>, etc. is added -- so how does this correspond with this vector of policies?) https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:632: const std::vector<ContentSecurityPolicy>& content_security_policies() const { I wonder if content's ContentSecurityPolicy should just hide the fact that there are multiple policies within itself. I.e., behave similarly to Blink's ContentSecurityPolicy which I think stores the vector of m_policies internally. Then we won't need to pass this vector around whenever we want to check the CSP, which would make the code in callers easier to follow. Perhaps CSPContext could be part of that too; i.e., it'd be nice to just say parent->content_security_policy()->Allow(CSPDirective::FrameSrc, url, is_redirect)), instead of dealing with CSPContexts and vectors of policies. Or are there reasons that it has to be organized like this? (This is fine to think about for a followup cleanup.) https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:636: void ResetContentSecurityPolicy() { content_security_policies_.clear(); } Should this be ResetContentSecurityPolicies, since it's resetting a set of policies? https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:640: // policies. RenderFrameHostImpl. Never null. Is "RenderFrameHostImpl." needed? https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1179: std::vector<ContentSecurityPolicy> content_security_policies_; It might be good to document here why we need this to be a set of policies, rather than a single policy. (I guess the explanation is in the comment for FrameHostMsg_DidAddContentSecurityPolicy?) https://codereview.chromium.org/2655463006/diff/470001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/common/content... content/common/content_security_policy/csp_context.h:64: bool followed_redirect); after_redirect? (that's what it's named below now) https://codereview.chromium.org/2655463006/diff/470001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/common/frame_m... content/common/frame_messages.h:920: content::CSPViolationParams /* violation_params*/) nit: space before */ https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1637: settings && settings->getBrowserSideNavigationEnabled(); nit: could move this after the if statement below, which doesn't need this value. https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1651: // can't be enforced on both side instead. nit: s/side/sides/ https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:125: ContentSecurityPolicyDisposition shouldBypassMainWorldCSP) = 0; This arg is named as if it's a bool, but it's not, and it sounds completely different from the enum's name. :) Perhaps just policyDisposition? It's clear enough what it is for when you check it against the enum value. (or you could actually pass this as a bool) https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2075: ContentSecurityPolicy::ViolationType::URLViolation); /* ViolationType */ Does this still need to extract out and explicitly pass in the redirect info from the WebContentSecurityPolicyViolation?
On 2017/02/23 02:37:15, alexmos wrote: > On 2017/02/22 13:46:22, arthursonzogni wrote: > > In the latest patch, I applied suggestions from you(alexmos@) and nasko@, i.e > > moving the CSP from the frame_tree_node to the render_frame_host. I hope I did > > it well. > > > > So yes, the CL is in a stable shape right now. I got a patch failure in my > last > > git-try :( > > I would be happy to have one more pass through this CL, Thank you very much! > > > > Nevertheless, it looks like I have a problem with: > > SiteDetailsBrowserTest.PlatformAppsNotIsolated > > > > From what I understand, it is a PlatformApp, it loads a sandboxed iframe that > > tries to navigate toward 127.0.0.1 > > > > It can't because the CSP of a sandboxed iframe. > > kDefaultSandboxedPageContentSecurityPolicy(=> "frame-src 'self'") > > > > The problems in the test is that > > details->GetOutOfProcessIframeCount() returns 1 instead of 0 > > I think it is because we now loads an error page instead of loading nothing. > > I don't know exactly why it uses an out of process iframe for loading the > error > > page. > > > > Due to http://crbug.com/612711, we are not isolating iframes from platform > apps > > with --isolate-extensions for the moment. That's what the test is checking. > > > > I will investigate, but maybe you will have an immediate understanding of the > > problem since you know the details of site isolation. > > I haven't followed that bug closely (lazyboy/creis would have more context), but > it seems like the test is just wrong now, since > https://codereview.chromium.org/2563843002 added those CSP restrictions to > disallow web content inside iframes on sandboxed app pages for good. This is > supposed to be deprecated starting in M57 - see also https://crbug.com/615585. > Perhaps ping lazyboy@ to double-check what the test should be doing now, if > anything? With crbug.com/615585 fixed, we won't allow web content inside sandbox pages' iframes, so this test is incorrect and not doing anything anymore. We should be able to delete this test. That CL also removed the platform app exception from > DoesSiteRequireDedicatedProcess, which probably explains why the error page is > placed into an OOPIF. Would this reintroduce the wrong storage partition kill > from 612711 for that error page, with PlzNavigate enabled?
Patchset #14 (id:490001) has been deleted
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
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-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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Patchset #14 (id:510001) has been deleted
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Thanks @lazyboy! I made a patch for removing the test: https://codereview.chromium.org/2715933002/ Thanks @alexmos for the review! I applied your suggestions, some answers below. @alexmos: > DoesSiteRequireDedicatedProcess, which probably explains why the error page is > placed into an OOPIF. Would this reintroduce the wrong storage partition kill > from 612711 for that error page, with PlzNavigate enabled? I tried to understand what you are speaking about. I found that bad_message::ACI_WRONG_STORAGE_PARTITION is sent in AppCacheInterceptor::CompleteCrossSiteTransfer(...) But there is no cross site transferts with PlzNavigate, the right process is chosen when we really know where the navigation ends. So there is no kill. Is this okay? https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2655463006/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:462: bool NavigationHandleImpl::ShouldBypassMainWorldCSP() const { On 2017/02/24 06:40:27, alexmos wrote: > On 2017/02/15 09:26:09, arthursonzogni wrote: > > On 2017/02/14 06:57:20, alexmos (OOO soon) wrote: > > > nit: you can move this simple getter into to the header as > > > should_bypass_main_world_csp(). > > > > Done, but I can't move the implementation in the header, it is disallowed by > the > > code formatter. Probably because it is an override. > > Ack, though that's surprising to me, as should_bypass_main_world_csp() is not > overriding anything from NavigationHandle? You are right, after this change, I removed the "override" and now it is possible. Done :) https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/02/24 06:40:27, alexmos wrote: > On 2017/02/16 17:32:40, arthursonzogni wrote: > > On 2017/02/15 21:28:44, nasko wrote: > > > Why do we need to subclass? > > Because I have to implement the virtual methods > > > > The CSPContext represents the system on which the CSP are checked on. A system > > can report violation to the user, it can display messages on the console and > it > > tells you when the CSP should by bypassed. > > > > By doing this, the CSP classes are not tied to anything like the CSP classes > > inside blink were. > > > > If one day, we want to check the CSP in another place, the network stack or > the > > renderer for instance, you can. You will just have to implement these 3 > > functions. > > > > It is also useful in my test. I can make a custom implementation of CSPContext > > to check the console message and test what happens when a scheme is bypassed > by > > the CSP for instance. > > > > > Also, missing a class level comment. > > Done. > > As an alternative to this, would it be easier if RenderFrameHostImpl just > extended CSPContext and implemented the three functions directly? Just curious > how this compares to having CSPContextImpl as an accessor on RFHI, since > CSPContextImpl is tied to its RFHI anyway. Yes, it might be a good idea. I am just a little bit afraid of the method names of CSPContext are too simple. I will rename some of them. For instance Allow() => AllowContentSecurityPolicy(), ReportViolation() => ReportContentSecurityPolicyViolation(); https://codereview.chromium.org/2655463006/diff/470001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html (right): https://codereview.chromium.org/2655463006/diff/470001/chrome/test/data/exten... chrome/test/data/extensions/api_test/sandboxed_pages_csp/sandboxed.html:48: // PlzNavigate: The first local frame has been replaced by an error page. On 2017/02/24 06:40:27, alexmos wrote: > nit: s/has been/will be/ > > Alternatively, we could just remove "the local frame will still be there. > Therefore, expect the local frame to respond again." and this sentence. Since > the eventual goal is to always go to the error page in both modes, and we will > probably forget to go back and update this comment once we do that. Just saying > that we rely on the SecurityPolicyViolationEvent to detect that the frame has > been blocked might be good enough. Done. https://codereview.chromium.org/2655463006/diff/470001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/BUILD.... content/browser/BUILD.gn:613: "frame_host/csp_context_impl.cc", On 2017/02/24 06:40:27, alexmos wrote: > Does this need the .h also? Ack. (File removed) https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/ancestor_throttle.cc:172: // If PlzNavigate is enabled, "frame-src" is enforced on the browser-side, On 2017/02/24 06:40:27, alexmos wrote: > nit: "browser side", "renderer side" (no -). Same in some other places below as > well. Done. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: // The CSPContext class represents the system on which the CSP are enforced. On 2017/02/24 06:40:27, alexmos wrote: > nit: s/are/is/ Acknowledged. (File removed) https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:21: CSPContextImpl(RenderFrameHostImpl* render_frame); On 2017/02/24 06:40:27, alexmos wrote: > nit: explicit Acknowledged. (File removed) https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1847: const std::vector<ContentSecurityPolicy>& policies) { On 2017/02/24 06:40:27, alexmos wrote: > This wasn't in this CL, but I'm a bit confused by this. Why is this a vector? > Wouldn't one header correspond to one policy? (I.e., I thought this message is > sent multiple times, each time a new header, csp in <meta>, etc. is added -- so > how does this correspond with this vector of policies?) RFC2616, section 4.2 specifies that headers appearing multiple times can be combined with a comma. So, you can have two CSP defined inside one header :( We could split the header by comma and send the policies one by one if we wanted to. I choose not to modify how the data in the frame_replication_state in my patch. I think we can though. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:632: const std::vector<ContentSecurityPolicy>& content_security_policies() const { On 2017/02/24 06:40:27, alexmos wrote: > I wonder if content's ContentSecurityPolicy should just hide the fact that there > are multiple policies within itself. I.e., behave similarly to Blink's > ContentSecurityPolicy which I think stores the vector of m_policies internally. > Then we won't need to pass this vector around whenever we want to check the CSP, > which would make the code in callers easier to follow. Perhaps CSPContext could > be part of that too; i.e., it'd be nice to just say > parent->content_security_policy()->Allow(CSPDirective::FrameSrc, url, > is_redirect)), instead of dealing with CSPContexts and vectors of policies. Or > are there reasons that it has to be organized like this? (This is fine to think > about for a followup cleanup.) The name of the classes inside blink confused me. ContentSecurityPolicy is not a policy but the concept, it store several policies and does what I do in CSPContext also. DirectiveSourceList is the the policy :) I am not sure what is the best thing. I will try to store the policies inside the CSPContext, please tell me if it looks good to you. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:636: void ResetContentSecurityPolicy() { content_security_policies_.clear(); } On 2017/02/24 06:40:27, alexmos wrote: > Should this be ResetContentSecurityPolicies, since it's resetting a set of > policies? I copied FrameTreeNode::ResetContentSecurityPolicy, will Removed in the latest CL. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:640: // policies. RenderFrameHostImpl. Never null. On 2017/02/24 06:40:27, alexmos wrote: > Is "RenderFrameHostImpl." needed? Removed in the latest CL. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1179: std::vector<ContentSecurityPolicy> content_security_policies_; On 2017/02/24 06:40:27, alexmos wrote: > It might be good to document here why we need this to be a set of policies, > rather than a single policy. (I guess the explanation is in the comment for > FrameHostMsg_DidAddContentSecurityPolicy?) Removed in the latest CL. https://codereview.chromium.org/2655463006/diff/470001/content/common/content... File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/common/content... content/common/content_security_policy/csp_context.h:64: bool followed_redirect); On 2017/02/24 06:40:27, alexmos wrote: > after_redirect? (that's what it's named below now) Done. I tried to follow the naming of @lukasza in the first place and then I switched to after_redirect. Thanks. https://codereview.chromium.org/2655463006/diff/470001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/470001/content/common/frame_m... content/common/frame_messages.h:920: content::CSPViolationParams /* violation_params*/) On 2017/02/24 06:40:27, alexmos wrote: > nit: space before */ Done. https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1637: settings && settings->getBrowserSideNavigationEnabled(); On 2017/02/24 06:40:27, alexmos wrote: > nit: could move this after the if statement below, which doesn't need this > value. Done. https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1651: // can't be enforced on both side instead. On 2017/02/24 06:40:28, alexmos wrote: > nit: s/side/sides/ Done. https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:125: ContentSecurityPolicyDisposition shouldBypassMainWorldCSP) = 0; On 2017/02/24 06:40:28, alexmos wrote: > This arg is named as if it's a bool, but it's not, and it sounds completely > different from the enum's name. :) Perhaps just policyDisposition? It's clear > enough what it is for when you check it against the enum value. (or you could > actually pass this as a bool) I agree. I will use shouldCheckMainWorldContentSecurityPolicy. https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2075: ContentSecurityPolicy::ViolationType::URLViolation); /* ViolationType */ On 2017/02/24 06:40:28, alexmos wrote: > Does this still need to extract out and explicitly pass in the redirect info > from the WebContentSecurityPolicyViolation? I am not sure to understand, but if you mean that I forgot to pass violation.after_redirect, yes this is really important.
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/02/24 16:13:30, arthursonzogni wrote: > Thanks @lazyboy! I made a patch for removing the test: > https://codereview.chromium.org/2715933002/ > > Thanks @alexmos for the review! > I applied your suggestions, some answers below. > > @alexmos: > > DoesSiteRequireDedicatedProcess, which probably explains why the error page is > > placed into an OOPIF. Would this reintroduce the wrong storage partition kill > > from 612711 for that error page, with PlzNavigate enabled? > > I tried to understand what you are speaking about. I found that > bad_message::ACI_WRONG_STORAGE_PARTITION is sent in > AppCacheInterceptor::CompleteCrossSiteTransfer(...) > But there is no cross site transferts with PlzNavigate, the right process is > chosen when we really know where the navigation ends. > So there is no kill. Is this okay? > Nice. I had the same question as Alex, since I was concerned about renderer kills if we put a web (error page) OOPIF inside a platform app. Does your CL only cause this error page behavior in PlzNavigate (where there are no transfers), or also in default mode where it might cause a kill? Even if there is no kill, I'm a bit concerned what might happen if we have an OOPIF in a different StoragePartition than its parent frame. That isn't supposed to happen, and I'm worried about what assumptions it might violate (and what bugs that could cause). We certainly haven't verified it's a safe thing to do from a design perspective. Not sure what alternatives we have in this case, though.
Thanks! A couple more nits, a question for Nasko below, and we still need to figure out the situation around the wrong storage partition for the error page; otherwise this looks good from my side. I'll see if I can dig some more into Charlie's storage partition questions tomorrow. https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/02/24 16:13:29, arthursonzogni wrote: > On 2017/02/24 06:40:27, alexmos wrote: > > On 2017/02/16 17:32:40, arthursonzogni wrote: > > > On 2017/02/15 21:28:44, nasko wrote: > > > > Why do we need to subclass? > > > Because I have to implement the virtual methods > > > > > > The CSPContext represents the system on which the CSP are checked on. A > system > > > can report violation to the user, it can display messages on the console and > > it > > > tells you when the CSP should by bypassed. > > > > > > By doing this, the CSP classes are not tied to anything like the CSP classes > > > inside blink were. > > > > > > If one day, we want to check the CSP in another place, the network stack or > > the > > > renderer for instance, you can. You will just have to implement these 3 > > > functions. > > > > > > It is also useful in my test. I can make a custom implementation of > CSPContext > > > to check the console message and test what happens when a scheme is bypassed > > by > > > the CSP for instance. > > > > > > > Also, missing a class level comment. > > > Done. > > > > As an alternative to this, would it be easier if RenderFrameHostImpl just > > extended CSPContext and implemented the three functions directly? Just > curious > > how this compares to having CSPContextImpl as an accessor on RFHI, since > > CSPContextImpl is tied to its RFHI anyway. > > Yes, it might be a good idea. > I am just a little bit afraid of the method names of CSPContext are too simple. > I will rename some of them. For instance > Allow() => AllowContentSecurityPolicy(), > ReportViolation() => ReportContentSecurityPolicyViolation(); Thanks for trying this out! On one hand, I like the simplification and that we don't need to pass the rfh->content_security_policies() as an arg anymore. On the other hand, it was nice to have all CSP methods grouped into one object, i.e. it'd be still nice to write the Allow check as something like rfh->content_security_policy()->Allow(...). I'm not too thrilled by the name AllowContentSecurityPolicy, as it sounds like it's "allowing a CSP", rather than checking the frame's CSP to see if a directive is allowed; perhaps we can come up with a better name. I'm curious what Nasko thinks here. Nasko? https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1847: const std::vector<ContentSecurityPolicy>& policies) { On 2017/02/24 16:13:29, arthursonzogni wrote: > On 2017/02/24 06:40:27, alexmos wrote: > > This wasn't in this CL, but I'm a bit confused by this. Why is this a vector? > > > Wouldn't one header correspond to one policy? (I.e., I thought this message > is > > sent multiple times, each time a new header, csp in <meta>, etc. is added -- > so > > how does this correspond with this vector of policies?) > > RFC2616, section 4.2 specifies that headers appearing multiple times can be > combined with a comma. So, you can have two CSP defined inside one header :( > > We could split the header by comma and send the policies one by one if we wanted > to. I choose not to modify how the data in the frame_replication_state in my > patch. I think we can though. Acknowledged. I didn't know this, and indeed, Blink's ContentSecurityPolicy::addPolicyFromHeaderValue handles exactly this. It'd be nice to add a comment to explain this, as it's not obvious. https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2655463006/diff/470001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2075: ContentSecurityPolicy::ViolationType::URLViolation); /* ViolationType */ On 2017/02/24 16:13:30, arthursonzogni wrote: > On 2017/02/24 06:40:28, alexmos wrote: > > Does this still need to extract out and explicitly pass in the redirect info > > from the WebContentSecurityPolicyViolation? > > I am not sure to understand, but if you mean that I forgot to pass > violation.after_redirect, yes this is really important. Yes, this is what I meant. Thanks for fixing it! https://codereview.chromium.org/2655463006/diff/550001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2655463006/diff/550001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:617: render_frame_host->frame_tree_node()->ResetContentSecurityPolicy(); It looks a bit weird to call something to reset CSP twice on both RFH and FTN. Perhaps we could combine them somehow and call one from the other, or rename the FTN ResetContentSecurityPolicy into something like ResetCSPHeaders, since it's really about resetting replicated CSP headers. Not that big of a deal. https://codereview.chromium.org/2655463006/diff/550001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2655463006/diff/550001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2078: // See http://crbug.com/690946 // nit: remove extra // at the end
https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/03/01 02:22:28, alexmos wrote: > On 2017/02/24 16:13:29, arthursonzogni wrote: > > On 2017/02/24 06:40:27, alexmos wrote: > > > On 2017/02/16 17:32:40, arthursonzogni wrote: > > > > On 2017/02/15 21:28:44, nasko wrote: > > > > > Why do we need to subclass? > > > > Because I have to implement the virtual methods > > > > > > > > The CSPContext represents the system on which the CSP are checked on. A > > system > > > > can report violation to the user, it can display messages on the console > and > > > it > > > > tells you when the CSP should by bypassed. > > > > > > > > By doing this, the CSP classes are not tied to anything like the CSP > classes > > > > inside blink were. > > > > > > > > If one day, we want to check the CSP in another place, the network stack > or > > > the > > > > renderer for instance, you can. You will just have to implement these 3 > > > > functions. > > > > > > > > It is also useful in my test. I can make a custom implementation of > > CSPContext > > > > to check the console message and test what happens when a scheme is > bypassed > > > by > > > > the CSP for instance. > > > > > > > > > Also, missing a class level comment. > > > > Done. > > > > > > As an alternative to this, would it be easier if RenderFrameHostImpl just > > > extended CSPContext and implemented the three functions directly? Just > > curious > > > how this compares to having CSPContextImpl as an accessor on RFHI, since > > > CSPContextImpl is tied to its RFHI anyway. > > > > Yes, it might be a good idea. > > I am just a little bit afraid of the method names of CSPContext are too > simple. > > I will rename some of them. For instance > > Allow() => AllowContentSecurityPolicy(), > > ReportViolation() => ReportContentSecurityPolicyViolation(); > > Thanks for trying this out! On one hand, I like the simplification and that we > don't need to pass the rfh->content_security_policies() as an arg anymore. On > the other hand, it was nice to have all CSP methods grouped into one object, > i.e. it'd be still nice to write the Allow check as something like > rfh->content_security_policy()->Allow(...). I'm not too thrilled by the name > AllowContentSecurityPolicy, as it sounds like it's "allowing a CSP", rather than > checking the frame's CSP to see if a directive is allowed; perhaps we can come > up with a better name. I'm curious what Nasko thinks here. Nasko? I like the methods being folded into RFH and avoiding the Impl class. What if we rename the allow method to be a bit more intuitive when read - IsAllowedByCsp? Or something similar?
https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/03/03 23:16:53, nasko (slow) wrote: > On 2017/03/01 02:22:28, alexmos wrote: > > On 2017/02/24 16:13:29, arthursonzogni wrote: > > > On 2017/02/24 06:40:27, alexmos wrote: > > > > On 2017/02/16 17:32:40, arthursonzogni wrote: > > > > > On 2017/02/15 21:28:44, nasko wrote: > > > > > > Why do we need to subclass? > > > > > Because I have to implement the virtual methods > > > > > > > > > > The CSPContext represents the system on which the CSP are checked on. A > > > system > > > > > can report violation to the user, it can display messages on the console > > and > > > > it > > > > > tells you when the CSP should by bypassed. > > > > > > > > > > By doing this, the CSP classes are not tied to anything like the CSP > > classes > > > > > inside blink were. > > > > > > > > > > If one day, we want to check the CSP in another place, the network stack > > or > > > > the > > > > > renderer for instance, you can. You will just have to implement these 3 > > > > > functions. > > > > > > > > > > It is also useful in my test. I can make a custom implementation of > > > CSPContext > > > > > to check the console message and test what happens when a scheme is > > bypassed > > > > by > > > > > the CSP for instance. > > > > > > > > > > > Also, missing a class level comment. > > > > > Done. > > > > > > > > As an alternative to this, would it be easier if RenderFrameHostImpl just > > > > extended CSPContext and implemented the three functions directly? Just > > > curious > > > > how this compares to having CSPContextImpl as an accessor on RFHI, since > > > > CSPContextImpl is tied to its RFHI anyway. > > > > > > Yes, it might be a good idea. > > > I am just a little bit afraid of the method names of CSPContext are too > > simple. > > > I will rename some of them. For instance > > > Allow() => AllowContentSecurityPolicy(), > > > ReportViolation() => ReportContentSecurityPolicyViolation(); > > > > Thanks for trying this out! On one hand, I like the simplification and that > we > > don't need to pass the rfh->content_security_policies() as an arg anymore. On > > the other hand, it was nice to have all CSP methods grouped into one object, > > i.e. it'd be still nice to write the Allow check as something like > > rfh->content_security_policy()->Allow(...). I'm not too thrilled by the name > > AllowContentSecurityPolicy, as it sounds like it's "allowing a CSP", rather > than > > checking the frame's CSP to see if a directive is allowed; perhaps we can come > > up with a better name. I'm curious what Nasko thinks here. Nasko? > > I like the methods being folded into RFH and avoiding the Impl class. What if we > rename the allow method to be a bit more intuitive when read - IsAllowedByCsp? > Or something similar? I like IsAllowedByCsp() - I think we can go with that.
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
I was OOO the last week, I am back. Thanks everyone! I think I fixed the remaining comments. In addition, this are two sub patches that should fix the problem with OOPIF error pages in Platform Apps. https://codereview.chromium.org/2732883003/ https://codereview.chromium.org/2715933002/ https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... File content/browser/frame_host/csp_context_impl.h (right): https://codereview.chromium.org/2655463006/diff/330001/content/browser/frame_... content/browser/frame_host/csp_context_impl.h:14: class CSPContextImpl : public CSPContext { On 2017/03/03 23:40:35, alexmos wrote: > On 2017/03/03 23:16:53, nasko (slow) wrote: > > On 2017/03/01 02:22:28, alexmos wrote: > > > On 2017/02/24 16:13:29, arthursonzogni wrote: > > > > On 2017/02/24 06:40:27, alexmos wrote: > > > > > On 2017/02/16 17:32:40, arthursonzogni wrote: > > > > > > On 2017/02/15 21:28:44, nasko wrote: > > > > > > > Why do we need to subclass? > > > > > > Because I have to implement the virtual methods > > > > > > > > > > > > The CSPContext represents the system on which the CSP are checked on. > A > > > > system > > > > > > can report violation to the user, it can display messages on the > console > > > and > > > > > it > > > > > > tells you when the CSP should by bypassed. > > > > > > > > > > > > By doing this, the CSP classes are not tied to anything like the CSP > > > classes > > > > > > inside blink were. > > > > > > > > > > > > If one day, we want to check the CSP in another place, the network > stack > > > or > > > > > the > > > > > > renderer for instance, you can. You will just have to implement these > 3 > > > > > > functions. > > > > > > > > > > > > It is also useful in my test. I can make a custom implementation of > > > > CSPContext > > > > > > to check the console message and test what happens when a scheme is > > > bypassed > > > > > by > > > > > > the CSP for instance. > > > > > > > > > > > > > Also, missing a class level comment. > > > > > > Done. > > > > > > > > > > As an alternative to this, would it be easier if RenderFrameHostImpl > just > > > > > extended CSPContext and implemented the three functions directly? Just > > > > curious > > > > > how this compares to having CSPContextImpl as an accessor on RFHI, since > > > > > CSPContextImpl is tied to its RFHI anyway. > > > > > > > > Yes, it might be a good idea. > > > > I am just a little bit afraid of the method names of CSPContext are too > > > simple. > > > > I will rename some of them. For instance > > > > Allow() => AllowContentSecurityPolicy(), > > > > ReportViolation() => ReportContentSecurityPolicyViolation(); > > > > > > Thanks for trying this out! On one hand, I like the simplification and that > > we > > > don't need to pass the rfh->content_security_policies() as an arg anymore. > On > > > the other hand, it was nice to have all CSP methods grouped into one object, > > > i.e. it'd be still nice to write the Allow check as something like > > > rfh->content_security_policy()->Allow(...). I'm not too thrilled by the > name > > > AllowContentSecurityPolicy, as it sounds like it's "allowing a CSP", rather > > than > > > checking the frame's CSP to see if a directive is allowed; perhaps we can > come > > > up with a better name. I'm curious what Nasko thinks here. Nasko? > > > > I like the methods being folded into RFH and avoiding the Impl class. What if > we > > rename the allow method to be a bit more intuitive when read - IsAllowedByCsp? > > Or something similar? > > I like IsAllowedByCsp() - I think we can go with that. Done. https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2655463006/diff/470001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1847: const std::vector<ContentSecurityPolicy>& policies) { On 2017/03/01 02:22:28, alexmos wrote: > On 2017/02/24 16:13:29, arthursonzogni wrote: > > On 2017/02/24 06:40:27, alexmos wrote: > > > This wasn't in this CL, but I'm a bit confused by this. Why is this a > vector? > > > > > Wouldn't one header correspond to one policy? (I.e., I thought this message > > is > > > sent multiple times, each time a new header, csp in <meta>, etc. is added -- > > so > > > how does this correspond with this vector of policies?) > > > > RFC2616, section 4.2 specifies that headers appearing multiple times can be > > combined with a comma. So, you can have two CSP defined inside one header :( > > > > We could split the header by comma and send the policies one by one if we > wanted > > to. I choose not to modify how the data in the frame_replication_state in my > > patch. I think we can though. > > Acknowledged. I didn't know this, and indeed, Blink's > ContentSecurityPolicy::addPolicyFromHeaderValue handles exactly this. It'd be > nice to add a comment to explain this, as it's not obvious. I will add a comment. FYI, the |policies| argument could be empty if the |header| contain an invalid policy. https://codereview.chromium.org/2655463006/diff/550001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2655463006/diff/550001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:617: render_frame_host->frame_tree_node()->ResetContentSecurityPolicy(); On 2017/03/01 02:22:28, alexmos wrote: > It looks a bit weird to call something to reset CSP twice on both RFH and FTN. > Perhaps we could combine them somehow and call one from the other, or rename the > FTN ResetContentSecurityPolicy into something like ResetCSPHeaders, since it's > really about resetting replicated CSP headers. Not that big of a deal. Done.
Thanks for all the work on this! LGTM once Nasko fixes the blocking error page issue in https://codereview.chromium.org/2738643002/. https://codereview.chromium.org/2655463006/diff/590001/content/renderer/conte... File content/renderer/content_security_policy_util.h (right): https://codereview.chromium.org/2655463006/diff/590001/content/renderer/conte... content/renderer/content_security_policy_util.h:14: // Convert a WebContentSecurityPolicy into a CSPPolicy. These two classes nit: CSPPolicy => ContentSecurityPolicy, since that's what's returned from this? https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:75: // The name of the directive that infringe the policy. |directive| might be a nit: s/infringe/violates/
nick@chromium.org changed reviewers: + nick@chromium.org
https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... content/common/frame_messages.h:345: IPC_STRUCT_TRAITS_MEMBER(should_bypass_main_world_csp) I'm wondering if we ought to consider making this be the url::Origin corresponding to the SecurityOrigin of the isolated world in which the navigation originated (or a null origin, if it's the main world)? The FIXME here https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebFrame.h... suggests that existing user scripts enjoy carte blanche when it comes to CSP enforcement, and if so, it may be very very tough to ever actually lock this down and actually apply the extension's CSP. Though maybe we could at least put up a console warning when this happens, or some other gradual upgrade path. If this were an origin instead of a bool, we could at least double check that it corresponded to an extension that was supposed to run a user script in the page, which maybe provides some protection. And it would provide a pathway to potentially fixing the FIXME later.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrameClient.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrameClient.h:127: shouldCheckMainWorldContentSecurityPolicy) = 0; Main world or main frame? https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:90: WebVector<WebString> reportEndpoints; Can we use WebURL if these are URLs?
https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrameClient.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrameClient.h:127: shouldCheckMainWorldContentSecurityPolicy) = 0; On 2017/03/13 19:02:54, dcheng wrote: > Main world or main frame? Main world is right. Isolated worlds (e.g. user scripts) bypass the CSP that applies to the frame's main world. The CSP of the main frame may be a contributing factor in determining the effective CSP for a frame's main world but they are not the same.
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
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_...)
https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... content/common/frame_messages.h:345: IPC_STRUCT_TRAITS_MEMBER(should_bypass_main_world_csp) On 2017/03/13 18:44:29, ncarter wrote: > I'm wondering if we ought to consider making this be the url::Origin > corresponding to the SecurityOrigin of the isolated world in which the > navigation originated (or a null origin, if it's the main world)? I lack knowledge about isolated world. Okay, there is CSPs for extensions too and they are not used for now. It seems to me that they are not even stored in blink. Blink only knows that they are defined, but not what they are. I was thinking about frame-src. In the future, if we were able to apply the isolated world CSPs. Should we apply any extension's CSPs when the request is initiated by an user-script for the frame-src case? I ask this because frame-src doesn't use the CSP of the document that has initiated the navigation, but the CSPs of the parent of the document that is navigating. Does someone else want to say something? https://codereview.chromium.org/2655463006/diff/590001/content/renderer/conte... File content/renderer/content_security_policy_util.h (right): https://codereview.chromium.org/2655463006/diff/590001/content/renderer/conte... content/renderer/content_security_policy_util.h:14: // Convert a WebContentSecurityPolicy into a CSPPolicy. These two classes On 2017/03/13 17:26:26, alexmos wrote: > nit: CSPPolicy => ContentSecurityPolicy, since that's what's returned from this? Thanks! https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:75: // The name of the directive that infringe the policy. |directive| might be a On 2017/03/13 17:26:26, alexmos wrote: > nit: s/infringe/violates/ Done. https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:90: WebVector<WebString> reportEndpoints; On 2017/03/13 19:02:54, dcheng wrote: > Can we use WebURL if these are URLs? I don't know. Blink uses a Vector<String> in CPDirectiveList.h I would have to convert it into an URL and then back to a string when it comes back to blink. I am not sure it is 100% related, but I think it is nice to keep it as string because of the Reporting API that will launch: https://www.w3.org/TR/2016/NOTE-reporting-1-20160607/ Summary: ``` It allows web developers to associate a set of named reporting endpoints with an origin. Various platform features (like Content Security Policy, Network Error Reporting, and others) will use these endpoints to deliver feature-specific reports in a consistent manner. ```
Blink LGTM with two comments. Changing the report endpoints list to hold a URL can wait for a followup CL; if possible, it would be nice to use an enum value rather than a bool for the new parameter though (since often times, we need an inline comment to document what false/true means anyway). https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:90: WebVector<WebString> reportEndpoints; On 2017/03/14 15:38:57, arthursonzogni wrote: > On 2017/03/13 19:02:54, dcheng wrote: > > Can we use WebURL if these are URLs? > > I don't know. Blink uses a Vector<String> in CPDirectiveList.h > I would have to convert it into an URL and then back to a string when it comes > back to blink. > > I am not sure it is 100% related, but I think it is nice to keep it as string > because of the Reporting API that will launch: > https://www.w3.org/TR/2016/NOTE-reporting-1-20160607/ > > Summary: > ``` > It allows web developers to associate a set of named reporting endpoints with an > origin. Various platform features (like Content Security Policy, Network Error > Reporting, and others) will use these endpoints to deliver feature-specific > reports in a consistent manner. > ``` I talked with mkwst, and he's OK with changing this to use WebURL/KURL/GURL throughout (since we convert to a KURL eventually anyway). The reporting API linked there will use a new directive. As this involves changing other random plumbing that's not necessarily related to this CL though... would you be OK with addressing this in a followup? https://codereview.chromium.org/2655463006/diff/630001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2655463006/diff/630001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:303: bool shouldBypassMainWorldCSP; I would encourage the use of an enum for this (and similarly in the content layer): having so many bool parameters can be a little unreadable.
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Patchset #21 (id:670001) has been deleted
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Thanks for the review! A few comments below: https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h (right): https://codereview.chromium.org/2655463006/diff/590001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebContentSecurityPolicyStruct.h:90: WebVector<WebString> reportEndpoints; On 2017/03/16 08:34:46, dcheng wrote: > On 2017/03/14 15:38:57, arthursonzogni wrote: > > On 2017/03/13 19:02:54, dcheng wrote: > > > Can we use WebURL if these are URLs? > > > > I don't know. Blink uses a Vector<String> in CPDirectiveList.h > > I would have to convert it into an URL and then back to a string when it comes > > back to blink. > > > > I am not sure it is 100% related, but I think it is nice to keep it as string > > because of the Reporting API that will launch: > > https://www.w3.org/TR/2016/NOTE-reporting-1-20160607/ > > > > Summary: > > ``` > > It allows web developers to associate a set of named reporting endpoints with > an > > origin. Various platform features (like Content Security Policy, Network Error > > Reporting, and others) will use these endpoints to deliver feature-specific > > reports in a consistent manner. > > ``` > > I talked with mkwst, and he's OK with changing this to use WebURL/KURL/GURL > throughout (since we convert to a KURL eventually anyway). The reporting API > linked there will use a new directive. > > As this involves changing other random plumbing that's not necessarily related > to this CL though... would you be OK with addressing this in a followup? Thank you for clarifying that. I will address it in a followup. https://codereview.chromium.org/2655463006/diff/630001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2655463006/diff/630001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:303: bool shouldBypassMainWorldCSP; On 2017/03/16 08:34:47, dcheng wrote: > I would encourage the use of an enum for this (and similarly in the content > layer): having so many bool parameters can be a little unreadable. By seeing all of these booleans, I thought that I had to do the same, that it was a sort of implicit coding style rule that applies to content/ and that is different from the blink's one. Thanks for the information.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #20 (id:650001) has been deleted
Patchset #20 (id:690001) has been deleted
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_browser_side_navigation_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
It looks like there are some tests that are failing with Site Isolation: SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromMeta SitePerProcessBrowserTest.CrossSiteIframeBlockedByCSPInheritedBySrcDocParent SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders Do they need to be updated for different expectations or is something else going wrong?
https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... content/common/frame_messages.h:345: IPC_STRUCT_TRAITS_MEMBER(should_bypass_main_world_csp) On 2017/03/14 15:38:57, arthursonzogni wrote: > On 2017/03/13 18:44:29, ncarter wrote: > > I'm wondering if we ought to consider making this be the url::Origin > > corresponding to the SecurityOrigin of the isolated world in which the > > navigation originated (or a null origin, if it's the main world)? > > I lack knowledge about isolated world. Okay, there is CSPs for extensions > too and they are not used for now. It seems to me that they are not even stored > in blink. Blink only knows that they are defined, but not what they are. > > I was thinking about frame-src. In the future, if we were able to apply > the isolated world CSPs. Should we apply any extension's CSPs when the > request is initiated by an user-script for the frame-src case? I ask this > because frame-src doesn't use the CSP of the document that has initiated the > navigation, but the CSPs of the parent of the document that is navigating. > > Does someone else want to say something? Our docs say that "content scripts are generally not subject to the CSP of the extension", and additionally, "the CSP of the page does not apply to content scripts". https://developer.chrome.com/extensions/contentSecurityPolicy#interactions Given that people have been building extensions under these expectations, it seems hard to start enforcing any kind of CSP for content scripts. Would the extension needs to declare a separate CSP for content scripts, since the default extension CSP applies to background pages and event pages, but here we'd need to apply it to regular web pages? I don't know if the FIXME is actually fixable. I agree with Nick that having an origin here would at least let the browser process verify that there's an extension with a content script that was supposed to run on the page that sent this, to minimize malicious renderers misusing this to bypass CSP. Not much, but better than nothing. However, given the amount of plumbing involved, I'm ok not blocking this CL on it and exploring this in a followup. Perhaps you can file a bug to explore ways to lock down the use of this flag so we don't forget?
On 2017/03/16 22:00:33, nasko (slow) wrote: > It looks like there are some tests that are failing with Site Isolation: > > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromMeta > SitePerProcessBrowserTest.CrossSiteIframeBlockedByCSPInheritedBySrcDocParent > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders > > Do they need to be updated for different expectations or is something else going > wrong? It is because of your patch that makes the error page stay in the same render-frame-host. Before your patch, the render-frame-host is a newborn, so its last_successful_url() is GURL() After your patch, the render-frame-host last_successful_url() is the one of the document that was here before trying to navigate to the blocked page. I will update the expectations.
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Patchset #22 (id:750001) has been deleted
The CQ bit was checked by arthursonzogni@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...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2655463006/diff/590001/content/common/frame_m... content/common/frame_messages.h:345: IPC_STRUCT_TRAITS_MEMBER(should_bypass_main_world_csp) On 2017/03/17 00:24:27, alexmos wrote: > On 2017/03/14 15:38:57, arthursonzogni wrote: > > On 2017/03/13 18:44:29, ncarter wrote: > > > I'm wondering if we ought to consider making this be the url::Origin > > > corresponding to the SecurityOrigin of the isolated world in which the > > > navigation originated (or a null origin, if it's the main world)? > > > > I lack knowledge about isolated world. Okay, there is CSPs for extensions > > too and they are not used for now. It seems to me that they are not even > stored > > in blink. Blink only knows that they are defined, but not what they are. > > > > I was thinking about frame-src. In the future, if we were able to apply > > the isolated world CSPs. Should we apply any extension's CSPs when the > > request is initiated by an user-script for the frame-src case? I ask this > > because frame-src doesn't use the CSP of the document that has initiated the > > navigation, but the CSPs of the parent of the document that is navigating. > > > > Does someone else want to say something? > > Our docs say that "content scripts are generally not subject to the CSP of the > extension", and additionally, "the CSP of the page does not apply to content > scripts". > https://developer.chrome.com/extensions/contentSecurityPolicy#interactions > > Given that people have been building extensions under these expectations, it > seems hard to start enforcing any kind of CSP for content scripts. Would the > extension needs to declare a separate CSP for content scripts, since the default > extension CSP applies to background pages and event pages, but here we'd need to > apply it to regular web pages? I don't know if the FIXME is actually fixable. > > I agree with Nick that having an origin here would at least let the browser > process verify that there's an extension with a content script that was supposed > to run on the page that sent this, to minimize malicious renderers misusing this > to bypass CSP. Not much, but better than nothing. However, given the amount of > plumbing involved, I'm ok not blocking this CL on it and exploring this in a > followup. Perhaps you can file a bug to explore ways to lock down the use of > this flag so we don't forget? TODO and BUG added: https://crbug.com/702540
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2655463006/#ps790001 (title: "Add TODO for https://crbug.com/702540 and fix test expectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
CQ is committing da patch.
Bot data: {"patchset_id": 790001, "attempt_start_ts": 1489759208296270,
"parent_rev": "47d64d328b84b893246a65264781df243d428ae6", "commit_rev":
"1e3b610bfff1acd060ed8b3f595344402b833bad"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2655463006 Cr-Commit-Position: refs/heads/master@{#457757} Committed: https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f5953... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:790001) as https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f5953...
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:790001) has been created in https://codereview.chromium.org/2754303002/ by nektar@chromium.org. The reason for reverting is: Possibly broke browser_side_navigation_content_browsertests. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin10_Tes....
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:790001) has been created in https://codereview.chromium.org/2756913002/ by nektar@chromium.org. The reason for reverting is: Possibly broke browser_side_navigation_content_browsertests. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin10_Tes....
Message was sent while issue was closed.
[5868:1532:0317/082357.173:34929421:INFO:CONSOLE(0)] "Refused to frame 'http://c.com:57177/title3.html' because it violates the following Content Security Policy directive: "frame-src 'self' http://b.com:*". ", source: http://a.com:57177/frame-src-self-and-b.html (0) c:\c\win\src\contentrowser\site_per_process_browsertest.cc(7423): error: Failed Blocked RenderFrameHost shouldn't be loading anything [ FAILED ] SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders, where TypeParam = and GetParam() = (1200 ms)
Message was sent while issue was closed.
On 2017/03/17 15:49:50, nektarios wrote: > [5868:1532:0317/082357.173:34929421:INFO:CONSOLE(0)] "Refused to frame > 'http://c.com:57177/title3.html' because it violates the following Content > Security > Policy directive: "frame-src 'self' http://b.com:*%22. > ", source: http://a.com:57177/frame-src-self-and-b.html (0) > c:\c\win\src\contentrowser\site_per_process_browsertest.cc(7423): error: > Failed > Blocked RenderFrameHost shouldn't be loading anything > [ FAILED ] > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders, where > TypeParam = and GetParam() = (1200 ms) Thanks for having reverted this CL. It looks like this CL doesn't work on Windows... The iframe is blocked, which is good, but current_frame_host()->is_loading() is true.
Message was sent while issue was closed.
On 2017/03/17 15:53:56, arthursonzogni wrote: > On 2017/03/17 15:49:50, nektarios wrote: > > [5868:1532:0317/082357.173:34929421:INFO:CONSOLE(0)] "Refused to frame > > 'http://c.com:57177/title3.html' because it violates the following Content > > Security > > Policy directive: "frame-src 'self' http://b.com:*%22. > > ", source: http://a.com:57177/frame-src-self-and-b.html (0) > > c:\c\win\src\contentrowser\site_per_process_browsertest.cc(7423): error: > > Failed > > Blocked RenderFrameHost shouldn't be loading anything > > [ FAILED ] > > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders, where > > TypeParam = and GetParam() = (1200 ms) > > Thanks for having reverted this CL. > > It looks like this CL doesn't work on Windows... > The iframe is blocked, which is good, but current_frame_host()->is_loading() is > true. Is this a real bug that we reverted for? Seems like a timing difference, and Windows being slower than mac/linux on the bots causes it to show up. While I'm not familiar with how this oopif code works, seems like for non-oopif the onload always fires before didstoploading. So it's just a race whether this works for non-oopif (i.e. shipping code paths) with plznavigate. If so, we should separate fixing the race for ooopif from this CL in the interest of not blocking PlzNavigate canary.
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2655463006 Cr-Commit-Position: refs/heads/master@{#457757} Committed: https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f5953... ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2655463006 Cr-Commit-Position: refs/heads/master@{#457757} Committed: https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f5953... ==========
The CQ bit was checked by jam@chromium.org
On 2017/03/17 19:24:06, jam wrote: > On 2017/03/17 15:53:56, arthursonzogni wrote: > > On 2017/03/17 15:49:50, nektarios wrote: > > > [5868:1532:0317/082357.173:34929421:INFO:CONSOLE(0)] "Refused to frame > > > 'http://c.com:57177/title3.html' because it violates the following Content > > > Security > > > Policy directive: "frame-src 'self' http://b.com:*%22. > > > ", source: http://a.com:57177/frame-src-self-and-b.html (0) > > > c:\c\win\src\contentrowser\site_per_process_browsertest.cc(7423): error: > > > Failed > > > Blocked RenderFrameHost shouldn't be loading anything > > > [ FAILED ] > > > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders, > where > > > TypeParam = and GetParam() = (1200 ms) > > > > Thanks for having reverted this CL. > > > > It looks like this CL doesn't work on Windows... > > The iframe is blocked, which is good, but current_frame_host()->is_loading() > is > > true. > > Is this a real bug that we reverted for? Seems like a timing difference, and > Windows being slower than mac/linux on the bots causes it to show up. > > While I'm not familiar with how this oopif code works, seems like for non-oopif > the onload always fires before didstoploading. So it's just a race whether this > works for non-oopif (i.e. shipping code paths) with plznavigate. If so, we > should separate fixing the race for ooopif from this CL in the interest of not > blocking PlzNavigate canary. (Nasko/Alex/I chatted and the there's agreement that the test expectations needed to be updated for PlzNavigate. I did that in https://codereview.chromium.org/2753203004/ so I'm hitting CQ again for this cl).
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #24 (id:810001) has been deleted
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2655463006/#ps830001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the linux_site_isolation trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
CQ is committing da patch.
Bot data: {"patchset_id": 830001, "attempt_start_ts": 1489803903600170,
"parent_rev": "d16b86f20467acbb4c2659ad7080df5d07dfad11", "commit_rev":
"7fed384c1a1d7a6c89531dddbb8b539061a9e455"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2655463006 Cr-Commit-Position: refs/heads/master@{#457757} Committed: https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f5953... ========== to ========== PlzNavigate: Enforce frame-src CSP on the browser. Use a NavigationThrottle to check infringement of the 'frame-src' on the browser-side. Before this patch, a redirect during the navigation could led to a child frame to be displayed inside its parent, even if it was disallowed by its parent. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2655463006 Cr-Original-Commit-Position: refs/heads/master@{#457757} Committed: https://chromium.googlesource.com/chromium/src/+/1e3b610bfff1acd060ed8b3f5953... Review-Url: https://codereview.chromium.org/2655463006 Cr-Commit-Position: refs/heads/master@{#457945} Committed: https://chromium.googlesource.com/chromium/src/+/7fed384c1a1d7a6c89531dddbb8b... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:830001) as https://chromium.googlesource.com/chromium/src/+/7fed384c1a1d7a6c89531dddbb8b... |
