Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689653003:
PlzNavigate: Enforce 'form-action' CSP on the browser-side. (Closed)
|
Created:
3 years, 10 months ago by arthursonzogni Modified:
3 years, 9 months ago CC:
alexmos, blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, nasko Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Enforce 'form-action' CSP on the browser-side.
Use a NavigationThrottle to enforce the 'form-action' CSP on the
browser-side. Before this patch, this CSP wasn't checked on
redirections.
BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2689653003
Cr-Commit-Position: refs/heads/master@{#457966}
Committed: https://chromium.googlesource.com/chromium/src/+/7cc34c583545597cffae27ad41bd015db6a93394
Patch Set 1 : PlzNavigate: Enforce 'form-action' CSP on the browser-side. #
Total comments: 20
Patch Set 2 : Addressed comments @mkwst #Patch Set 3 : Add tests. #Patch Set 4 : Rebase. #
Total comments: 29
Patch Set 5 : Rebase. #Patch Set 6 : Addressed comments. #Patch Set 7 : Rebase. #Patch Set 8 : Disable renderer-side checks during a navigation. #Patch Set 9 : Duplicate tests with a redirect. #
Total comments: 2
Patch Set 10 : Add TODO. #
Total comments: 45
Patch Set 11 : Rebase #Patch Set 12 : Addressed comments @alexmos and @nasko. #Patch Set 13 : Rebase. #Messages
Total messages: 145 (114 generated)
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Description was changed from ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSPon the browser-side. Before this patch, this CSP wasn't checked on redirections. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
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
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 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-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...)
Description was changed from ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. 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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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
Patchset #1 (id:20001) has been deleted
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 #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: 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...)
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:120001) 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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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_...)
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 #1 (id:140001) 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
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_tsan_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
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_...)
Patchset #1 (id:160001) has been deleted
arthursonzogni@chromium.org changed reviewers: + clamy@chromium.org, mkwst@chromium.org
Hi Camille and Mike.
Please could you take a look at this patch? Thanks!
clamy: content/*
mkwst: - content/common/frame_messages.h (as an ipc/SECURITY_OWNER)
Could you also check the CSP are correctly enforced in
- content/browser/frame_host/form_submission_throttle.cc?
Especially the fact that the 'form-action' CSP is
bypassed when the request is initiated by an isolated
world.
CC: alexmos@, nasko@ and jam@.
N.B.
There is currently two regressions on the try-bot, but it shouldn't be caused by
this patch:
SiteDetailsBrowserTest.PlatformAppsNotIsolated
=> Caused by the dependent CL.
.../mixedContent/insecure-iframe-with-hsts.https.html
=> Missing the context line in a console log, probably a known recent
regression.
The general shape here looks reasonable. A few questions about the throttle, and I'd like to see some unit tests. :) https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:53: return NavigationThrottle::PROCEED; It seems valuable to add some unit tests for this method. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:58: return CheckContentSecurityPolicyFormAction(false /* is_redirect */); I might be misunderstanding things, but I thought that this fired at the beginning of a redirected request... That is, if A redirects to B, I think we'll see `WillStartRequest` twice (once for A and once for B) and WillRedirectRequest once (for the redirection). Is that accurate? If not, great! If so, then we can drop `WillRedirectRequest` from this throttle, but the `is_redirect` flag here is incorrect. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:19: class CONTENT_EXPORT FormSubmissionThrottle : public NavigationThrottle { Do you plan on creating a distinct throttle for `frame-src`/`child-src` enforcement? I have no idea how much a throttle costs in terms of memory or performance, so I'll defer to //content/OWNERS for the technical discussion, but it seems to me that it might make more sense to define a single throtle for all of CSP. *shrug* https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:928: throttles_.push_back(std::move(form_submission_throttle)); This needs to happen before the mixed content check. Can I assume that throttles execute in LIFO order? https://codereview.chromium.org/2689653003/diff/180001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/common/frame_m... content/common/frame_messages.h:360: IPC_STRUCT_TRAITS_MEMBER(is_form_submission) Addition of the bool to IPC looks fine. https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:6316: info.navigationType == blink::WebNavigationTypeFormSubmitted; This seems like you pasted the same thing twice. Did you mean Resubmitted? We apparently don't have any tests that cover the difference here: can you add one?
Thanks Mike, Some answers below: https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:53: return NavigationThrottle::PROCEED; On 2017/02/22 15:36:07, Mike West (sloooooow) wrote: > It seems valuable to add some unit tests for this method. Acknowledged. It looks hard since it depends on the frame_tree_node and its current_frame_host. These objects cannot be easily replaced by stub objects. I will look around the code source to find some tests that builds such objects. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:58: return CheckContentSecurityPolicyFormAction(false /* is_redirect */); On 2017/02/22 15:36:07, Mike West (sloooooow) wrote: > I might be misunderstanding things, but I thought that this fired at the > beginning of a redirected request... That is, if A redirects to B, I think we'll > see `WillStartRequest` twice (once for A and once for B) and WillRedirectRequest > once (for the redirection). Is that accurate? > > If not, great! If so, then we can drop `WillRedirectRequest` from this throttle, > but the `is_redirect` flag here is incorrect. I confirm that WillStartRequest is not called twice, which is great! https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:19: class CONTENT_EXPORT FormSubmissionThrottle : public NavigationThrottle { On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > Do you plan on creating a distinct throttle for `frame-src`/`child-src` > enforcement? I have no idea how much a throttle costs in terms of memory or > performance, so I'll defer to //content/OWNERS for the technical discussion, but > it seems to me that it might make more sense to define a single throtle for all > of CSP. *shrug* I am okay with this idea. The throttle is only created when there is a form submission though and I don't think the NavigationThrottles have an huge impact(But I don't really know) Do you want to merge the AncestorThrottle and the FormSubmissionThrottle into a ContentSecurityPolicyThrottle?(Even if XFO is not really a CSP)? https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:928: throttles_.push_back(std::move(form_submission_throttle)); On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > This needs to happen before the mixed content check. Can I assume that throttles > execute in LIFO order? I would say the FIFO order, they are executed from the first one (mixed_content_throttle, then devtools_throttles, ..., then form_submission_throttle) You are probably right, but please, can you tell why the mixed_content_checks should happens after the 'form-action' checks? What about the 'frame-src' and 'XFO' checks? https://codereview.chromium.org/2689653003/diff/180001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/common/frame_m... content/common/frame_messages.h:360: IPC_STRUCT_TRAITS_MEMBER(is_form_submission) On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > Addition of the bool to IPC looks fine. Acknowledged. https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:6316: info.navigationType == blink::WebNavigationTypeFormSubmitted; On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > This seems like you pasted the same thing twice. Did you mean Resubmitted? We > apparently don't have any tests that cover the difference here: can you add one? Oops, thanks! Ack for the test. I assume that a form resubmission happens when the current page is the result of a form submission and it gets reloaded. right? But if the first navigation was blocked by the 'form-action', it can't be reloaded since it wasn't loaded. It's probably for this reason that we don't have any test for this. Is it what you had in mind? Do you want to test something else?
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:53: return NavigationThrottle::PROCEED; On 2017/02/22 17:15:23, arthursonzogni wrote: > On 2017/02/22 15:36:07, Mike West (sloooooow) wrote: > > It seems valuable to add some unit tests for this method. > > Acknowledged. It looks hard since it depends on the frame_tree_node and its > current_frame_host. These objects cannot be easily replaced by stub objects. I > will look around the code source to find some tests that builds such objects. Tests added!
Patchset #3 (id:210020) 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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
nasko@chromium.org changed reviewers: + alexmos@chromium.org, nasko@chromium.org
Hey Alex, Can you help review this CL as mkwst@ seems to be out this week? Thanks in advance! Nasko
Overall looks good, some minor comments below. I tried to pitch in on some of questions in Mike's comments while Mike is out. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:19: class CONTENT_EXPORT FormSubmissionThrottle : public NavigationThrottle { On 2017/02/22 17:15:23, arthursonzogni wrote: > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > Do you plan on creating a distinct throttle for `frame-src`/`child-src` > > enforcement? I have no idea how much a throttle costs in terms of memory or > > performance, so I'll defer to //content/OWNERS for the technical discussion, > but > > it seems to me that it might make more sense to define a single throtle for > all > > of CSP. *shrug* > > I am okay with this idea. > The throttle is only created when there is a form submission though and I don't > think the NavigationThrottles have an huge impact(But I don't really know) > > Do you want to merge the AncestorThrottle and the FormSubmissionThrottle into a > ContentSecurityPolicyThrottle?(Even if XFO is not really a CSP)? I'm personally ok either way. Unifying them in one throttle will centralize all the policies, but I'm also not too worried about FormSubmissionThrottle's overhead, since it's gated on the is_form_submission() bit, so you'd need it very infrequently. The ancestor throttle is currently created only for subframes, whereas this matters for main frames too, so unifying them might also get confusing in terms of when the throttle gets created. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:928: throttles_.push_back(std::move(form_submission_throttle)); On 2017/02/22 17:15:23, arthursonzogni wrote: > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > This needs to happen before the mixed content check. Can I assume that > throttles > > execute in LIFO order? > > I would say the FIFO order, they are executed from the first one > (mixed_content_throttle, then devtools_throttles, ..., then > form_submission_throttle) > > You are probably right, but please, can you tell why the mixed_content_checks > should happens after the 'form-action' checks? > What about the 'frame-src' and 'XFO' checks? Mike would know these reasons much better, but one thing I recalled is this comment in FrameFetchContext::canRequestInternal: // Check for mixed content. We do this second-to-last so that when folks block // mixed content with a CSP policy, they don't get a warning. They'll still // get a warning in the console about CSP blocking the load. https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:6316: info.navigationType == blink::WebNavigationTypeFormSubmitted; On 2017/02/22 17:15:23, arthursonzogni wrote: > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > This seems like you pasted the same thing twice. Did you mean Resubmitted? We > > apparently don't have any tests that cover the difference here: can you add > one? > > Oops, thanks! > Ack for the test. I assume that a form resubmission happens when the current > page is the result of a form submission and it gets reloaded. right? But if the > first navigation was blocked by the 'form-action', it can't be reloaded since it > wasn't loaded. It's probably for this reason that we don't have any test for > this. > Is it what you had in mind? Do you want to test something else? Again, can't speak for Mike, but there's a Blink unittest, ParameterizedWebFrameTest.ReloadPost, that shows how to get the Resubmitted navigation type, which indeed just seems to be a reload (or back-forward) of a page resulting from a form submit. The original bug presumably would've prevented the throttle from being created for the resubmission. So perhaps you can have a test where the form submission succeeds (not blocked by form-action), and then reload the resulting page and ensure that the reload has a correct is_form_submission() bit? I'm also a bit confused by how a form resubmission is supposed to interact with CSP form-action, namely which frame's CSP is supposed to be checked for the resubmission. Presumably the old frame's CSP is gone after the first form submit loads, but it doesn't seem like the new doc's (the result of first submit) CSP should apply for the resubmit. The Blink check in FrameLoader::shouldContinueForNavigationPolicy checks for both regular form submits and resubmissions, but I'm not sure whether a CSP form-action can allow a form submission but then later block the resubmission (which is what you might also want in a test for this). https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:17: // A FormSubmittionThrottle is responsible for enforcing the 'form-action' CSP, nit: s/CSP/CSP directive/ https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:18: // blocking requests which violate them. Like frame-src, I'd document that this is currently only enforcing form-action for PlzNavigate, and Blink is still enforcing this without PlzNavigate. Presumably, the Blink check is the allowFormAction check in FrameLoader::shouldContinueForNavigationPolicy? Does that need an exclusion for PlzNavigate mode like the frame-src one? https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:29: // The FormSubmittionThrottle aren't used without PlzNavigate. nit: FormSubmittionThrottle -> FormSubmissionThrottle, and aren't -> isn't (also in second test) https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:59: // redirection. By using this behavior, this test can checks a case where Just curious, is this because of what the spec says? It strikes me as somewhat non-intuitive behavior. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:101: EXPECT_EQ(test.redirect_expectation, throttle->WillRedirectRequest()); Maybe not in this test, but it'd be nice to add an end-to-end test that performs an actual form navigation (1) has some coverage that is_form_submission() is initialized properly, and (2) verifies that you land on the error page when blocked by the throttle. Or perhaps this is covered elsewhere? https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:134: // Test the expectations with a FormSubmissionThrottle. Can you add a short comment to make it more obvious why these should PROCEED? I.e., because should_bypass_main_world_csp is true? https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:157: // is currently not available when PlzNavigate is disabled. I'll defer to clamy@ and nasko@ on whether this is ok to land with a DCHECK without PlzNavigate. Presumably, we could plumb this information without PlzNavigate as well - is that in your plans? If so, could you add a TODO?
I'm back! But I'm mostly out again tomorrow! Hooray for kindergarten closures! The general shape of this looks pretty reasonable. I'd second some of alexmos@'s comments, and I'd like to understand how you plan to handle violation reports, otherwise, this looks ~good enough to start with. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:49: return NavigationThrottle::CANCEL; How do you plan to deal with violation reports and `securitypolicyviolation` events? I don't think you need to block testing PlzNavigate on that, but we ought to be surfacing errors to developers before shipping. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:18: // blocking requests which violate them. On 2017/02/28 at 02:48:46, alexmos wrote: > Like frame-src, I'd document that this is currently only enforcing form-action for PlzNavigate, and Blink is still enforcing this without PlzNavigate. Presumably, the Blink check is the allowFormAction check in FrameLoader::shouldContinueForNavigationPolicy? Does that need an exclusion for PlzNavigate mode like the frame-src one? That kind of explicit carveout makes sense to me, if only because it means this code would be better tested via Blink's layout tests. Right now I don't think they're running through the WillStartRequest bits for blocked requests at all. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:59: // redirection. By using this behavior, this test can checks a case where On 2017/02/28 at 02:48:46, alexmos wrote: > Just curious, is this because of what the spec says? It strikes me as somewhat non-intuitive behavior. Totally non-intuitive. Totally what the spec says. See https://www.w3.org/TR/CSP2/#source-list-paths-and-redirects for detail. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:101: EXPECT_EQ(test.redirect_expectation, throttle->WillRedirectRequest()); On 2017/02/28 at 02:48:46, alexmos wrote: > Maybe not in this test, but it'd be nice to add an end-to-end test that performs an actual form navigation (1) has some coverage that is_form_submission() is initialized properly, and (2) verifies that you land on the error page when blocked by the throttle. Or perhaps this is covered elsewhere? Are we landing on an error page? That sounds great, but I thought it was blocked on changing the error page behavior in such a way as to commit a reasonable URL (we talked about this in the context of XFO, as I recall).
Thanks for the reviews. Some answers below. I still need to write a test about form re-submission. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:19: class CONTENT_EXPORT FormSubmissionThrottle : public NavigationThrottle { On 2017/02/28 02:48:46, alexmos (OOO on 3-7) wrote: > On 2017/02/22 17:15:23, arthursonzogni wrote: > > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > > Do you plan on creating a distinct throttle for `frame-src`/`child-src` > > > enforcement? I have no idea how much a throttle costs in terms of memory or > > > performance, so I'll defer to //content/OWNERS for the technical discussion, > > but > > > it seems to me that it might make more sense to define a single throtle for > > all > > > of CSP. *shrug* > > > > I am okay with this idea. > > The throttle is only created when there is a form submission though and I > don't > > think the NavigationThrottles have an huge impact(But I don't really know) > > > > Do you want to merge the AncestorThrottle and the FormSubmissionThrottle into > a > > ContentSecurityPolicyThrottle?(Even if XFO is not really a CSP)? > > I'm personally ok either way. Unifying them in one throttle will centralize all > the policies, but I'm also not too worried about FormSubmissionThrottle's > overhead, since it's gated on the is_form_submission() bit, so you'd need it > very infrequently. The ancestor throttle is currently created only for > subframes, whereas this matters for main frames too, so unifying them might also > get confusing in terms of when the throttle gets created. Yes, nice summary. What to decide for now? I would prefer keeping the two throttles if there is no preference. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:928: throttles_.push_back(std::move(form_submission_throttle)); On 2017/02/28 02:48:46, alexmos (OOO on 3-7) wrote: > On 2017/02/22 17:15:23, arthursonzogni wrote: > > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > > This needs to happen before the mixed content check. Can I assume that > > throttles > > > execute in LIFO order? > > > > I would say the FIFO order, they are executed from the first one > > (mixed_content_throttle, then devtools_throttles, ..., then > > form_submission_throttle) > > > > You are probably right, but please, can you tell why the mixed_content_checks > > should happens after the 'form-action' checks? > > What about the 'frame-src' and 'XFO' checks? > > Mike would know these reasons much better, but one thing I recalled is this > comment in FrameFetchContext::canRequestInternal: > > // Check for mixed content. We do this second-to-last so that when folks block > // mixed content with a CSP policy, they don't get a warning. They'll still > // get a warning in the console about CSP blocking the load. Done. Mike please confirm that it looks good to you to move the AncestorThrottle and the FormSubmissionThrottle before the MixedContentThrottle. https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2689653003/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:6316: info.navigationType == blink::WebNavigationTypeFormSubmitted; On 2017/02/28 02:48:46, alexmos (OOO on 3-7) wrote: > On 2017/02/22 17:15:23, arthursonzogni wrote: > > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > > This seems like you pasted the same thing twice. Did you mean Resubmitted? > We > > > apparently don't have any tests that cover the difference here: can you add > > one? > > > > Oops, thanks! > > Ack for the test. I assume that a form resubmission happens when the current > > page is the result of a form submission and it gets reloaded. right? But if > the > > first navigation was blocked by the 'form-action', it can't be reloaded since > it > > wasn't loaded. It's probably for this reason that we don't have any test for > > this. > > Is it what you had in mind? Do you want to test something else? > > Again, can't speak for Mike, but there's a Blink unittest, > ParameterizedWebFrameTest.ReloadPost, that shows how to get the Resubmitted > navigation type, which indeed just seems to be a reload (or back-forward) of a > page resulting from a form submit. The original bug presumably would've > prevented the throttle from being created for the resubmission. So perhaps you > can have a test where the form submission succeeds (not blocked by form-action), > and then reload the resulting page and ensure that the reload has a correct > is_form_submission() bit? > > I'm also a bit confused by how a form resubmission is supposed to interact with > CSP form-action, namely which frame's CSP is supposed to be checked for the > resubmission. Presumably the old frame's CSP is gone after the first form > submit loads, but it doesn't seem like the new doc's (the result of first > submit) CSP should apply for the resubmit. The Blink check in > FrameLoader::shouldContinueForNavigationPolicy checks for both regular form > submits and resubmissions, but I'm not sure whether a CSP form-action can allow > a form submission but then later block the resubmission (which is what you might > also want in a test for this). I will add a test with an allowed form submission to a page that disallow form submission. Then I reload the page to see what happens. I think I will not get the right behavior with my CL and maybe not with the current architecture too. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:49: return NavigationThrottle::CANCEL; On 2017/03/02 10:45:34, Mike West (Slow.) wrote: > How do you plan to deal with violation reports and `securitypolicyviolation` > events? I don't think you need to block testing PlzNavigate on that, but we > ought to be surfacing errors to developers before shipping. The violation report and the 'securitypolicyviolation' event are sent when the call to AllowContentSecurityPolicy returns false. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:17: // A FormSubmittionThrottle is responsible for enforcing the 'form-action' CSP, On 2017/02/28 02:48:46, alexmos (OOO on 3-7) wrote: > nit: s/CSP/CSP directive/ Done. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:18: // blocking requests which violate them. On 2017/02/28 02:48:46, alexmos wrote: > Like frame-src, I'd document that this is currently only enforcing form-action > for PlzNavigate, and Blink is still enforcing this without PlzNavigate. Done. > Presumably, the Blink check is the allowFormAction check in > FrameLoader::shouldContinueForNavigationPolicy? Yes, but not only, it is checked in a few different other places too. > Does that need an exclusion for > PlzNavigate mode like the frame-src one? It doesn't need, we cant check the CSP on both side. Not checking the CSP on the renderer-side could be a good thing in order to make the current Layout test testing the browser-side implementation instead of the renderer-side one. Maybe disabling the renderer-side checks with PlzNavigate for all the request is the right thing to do. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:29: // The FormSubmittionThrottle aren't used without PlzNavigate. On 2017/02/28 02:48:46, alexmos wrote: > nit: FormSubmittionThrottle -> FormSubmissionThrottle, and aren't -> isn't (also > in second test) Done. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:59: // redirection. By using this behavior, this test can checks a case where On 2017/03/02 10:45:34, Mike West (Slow.) wrote: > On 2017/02/28 at 02:48:46, alexmos wrote: > > Just curious, is this because of what the spec says? It strikes me as > somewhat non-intuitive behavior. > > Totally non-intuitive. Totally what the spec says. See > https://www.w3.org/TR/CSP2/#source-list-paths-and-redirects for detail. Acknowledged. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:101: EXPECT_EQ(test.redirect_expectation, throttle->WillRedirectRequest()); On 2017/03/02 10:45:34, Mike West (Slow.) wrote: > On 2017/02/28 at 02:48:46, alexmos wrote: > > Maybe not in this test, but it'd be nice to add an end-to-end test that > performs an actual form navigation (1) has some coverage that > is_form_submission() is initialized properly, and (2) verifies that you land on > the error page when blocked by the throttle. Or perhaps this is covered > elsewhere? There is some layout tests that does what you want to do. One problem could be that the form-action CSP is checked on both side, it is mainly the renderer-side one that is blocking the request. Maybe I would have to disable it on the renderer-side too in order to test it too. > Are we landing on an error page? That sounds great, but I thought it was blocked > on changing the error page behavior in such a way as to commit a reasonable URL > (we talked about this in the context of XFO, as I recall). I didn't use an error page when the form-action directive is infringed. I could by returning BLOCK_REQUEST instead of CANCEL in the NavigationThrottle. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:134: // Test the expectations with a FormSubmissionThrottle. On 2017/02/28 02:48:46, alexmos wrote: > Can you add a short comment to make it more obvious why these should PROCEED? > I.e., because should_bypass_main_world_csp is true? Done. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:157: // is currently not available when PlzNavigate is disabled. On 2017/02/28 02:48:47, alexmos wrote: > I'll defer to clamy@ and nasko@ on whether this is ok to land with a DCHECK > without PlzNavigate. Presumably, we could plumb this information without > PlzNavigate as well - is that in your plans? If so, could you add a TODO? Yes, my plan was to eventually plumb this information without PlzNavigate as well if we really need it. But I think that the old navigation architecture will be removed before we might need it. So, I think that no TODO is required here.
Patchset #6 (id:330001) 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
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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Just couple of drive-by comments. https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/180001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:19: class CONTENT_EXPORT FormSubmissionThrottle : public NavigationThrottle { On 2017/03/07 16:25:51, arthursonzogni wrote: > On 2017/02/28 02:48:46, alexmos (OOO on 3-7) wrote: > > On 2017/02/22 17:15:23, arthursonzogni wrote: > > > On 2017/02/22 15:36:08, Mike West (sloooooow) wrote: > > > > Do you plan on creating a distinct throttle for `frame-src`/`child-src` > > > > enforcement? I have no idea how much a throttle costs in terms of memory > or > > > > performance, so I'll defer to //content/OWNERS for the technical > discussion, > > > but > > > > it seems to me that it might make more sense to define a single throtle > for > > > all > > > > of CSP. *shrug* > > > > > > I am okay with this idea. > > > The throttle is only created when there is a form submission though and I > > don't > > > think the NavigationThrottles have an huge impact(But I don't really know) > > > > > > Do you want to merge the AncestorThrottle and the FormSubmissionThrottle > into > > a > > > ContentSecurityPolicyThrottle?(Even if XFO is not really a CSP)? > > > > I'm personally ok either way. Unifying them in one throttle will centralize > all > > the policies, but I'm also not too worried about FormSubmissionThrottle's > > overhead, since it's gated on the is_form_submission() bit, so you'd need it > > very infrequently. The ancestor throttle is currently created only for > > subframes, whereas this matters for main frames too, so unifying them might > also > > get confusing in terms of when the throttle gets created. > > Yes, nice summary. > What to decide for now? I would prefer keeping the two throttles if there is no > preference. I would prefer separate ones, unless we have clear data about overhead. Code simplicity and discoverability is key. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:157: // is currently not available when PlzNavigate is disabled. On 2017/03/07 16:25:51, arthursonzogni wrote: > On 2017/02/28 02:48:47, alexmos wrote: > > I'll defer to clamy@ and nasko@ on whether this is ok to land with a DCHECK > > without PlzNavigate. Presumably, we could plumb this information without > > PlzNavigate as well - is that in your plans? If so, could you add a TODO? > > Yes, my plan was to eventually plumb this information without PlzNavigate as > well if we really need it. But I think that the old navigation architecture will > be removed before we might need it. > So, I think that no TODO is required here. Adding a TODO and keeping it PlzNavigate specific is fine with me. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:158: bool is_form_submission() const; Since this is a simple accessor, it should be inlined in the header.
Sorry for the delayed response, I'm buried (as are we all this week, right?). I think this is looking pretty reasonable. The only thing I think you really need to change is the carveout in Blink to ensure that the browser-side is responsible for all the navigational checks when we're in plznavigate mode. Other than than, looking good. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:49: return NavigationThrottle::CANCEL; On 2017/03/07 at 16:25:51, arthursonzogni wrote: > On 2017/03/02 10:45:34, Mike West (Slow.) wrote: > > How do you plan to deal with violation reports and `securitypolicyviolation` > > events? I don't think you need to block testing PlzNavigate on that, but we > > ought to be surfacing errors to developers before shipping. > > The violation report and the 'securitypolicyviolation' event are sent when the call to AllowContentSecurityPolicy returns false. I see, thanks! https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:18: // blocking requests which violate them. > > Does that need an exclusion for > > PlzNavigate mode like the frame-src one? > It doesn't need, we cant check the CSP on both side. Not checking the CSP on the renderer-side could be a good thing in order to make the current Layout test testing the browser-side implementation instead of the renderer-side one. > > Maybe disabling the renderer-side checks with PlzNavigate for all the request is the right thing to do. I think it is, for the same reasons that we did so for Mixed Content. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:101: EXPECT_EQ(test.redirect_expectation, throttle->WillRedirectRequest()); On 2017/03/07 at 16:25:51, arthursonzogni wrote: > > Are we landing on an error page? That sounds great, but I thought it was blocked > > on changing the error page behavior in such a way as to commit a reasonable URL > > (we talked about this in the context of XFO, as I recall). > I didn't use an error page when the form-action directive is infringed. I could by returning BLOCK_REQUEST instead of CANCEL in the NavigationThrottle. If alexmos@ and creis@ are ok with landing on an error page (I recall some issues with the Chrome Web Store's in particular being committed), then I'd prefer it to a blank page.
Hi Mike, Here is the set of tests I was talking about yesterday. It shows what CSP applies when there is a form resubmission. The current behavior is that it is always the CSP of the the frame/window that is navigating again that applies this time, not the one of the frame/window that has initiated the navigation. Maybe it is not what we really want, I don't know. This CL follows the comments in https://codereview.chromium.org/2689653003/ asking some tests with form resubmission. Please, can you take a look? +CC @alexmos @nasko @clamy
Oops, sorry for the previous message, I though I was in https://codereview.chromium.org/2737343004 So yes, there is there tests about form *re*submission.
On 2017/03/09 13:13:02, arthursonzogni wrote: > Oops, sorry for the previous message, I though I was in > https://codereview.chromium.org/2737343004 > > So yes, there is there tests about form *re*submission. I did an experiment that only enforce form-action on the browser-side. I don't think it will be possible for the moment since there exist some form-submission that doesn't result in a navigation. So there are not checked on the browser-side. For instance, it happens when the url is a javascript url. Maybe it is the same with same-page navigation I didn't have tested it yet.
https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:18: // blocking requests which violate them. On 2017/03/09 08:20:04, Mike West (Slow.) wrote: > > > Does that need an exclusion for > > > PlzNavigate mode like the frame-src one? > > It doesn't need, we cant check the CSP on both side. Not checking the CSP on > the renderer-side could be a good thing in order to make the current Layout test > testing the browser-side implementation instead of the renderer-side one. > > > > Maybe disabling the renderer-side checks with PlzNavigate for all the request > is the right thing to do. > > I think it is, for the same reasons that we did so for Mixed Content. I tried, every layout tests are working with two exceptions. 1) What if the form url is a javascript url? It this case it is not a navigation and the request use the PlzNavigate path. 2) What if the form target another window? In addition, maybe there will be a problem (with and without PlzNavigate) in the following scenario: Two windows, 1) [a.com] with "form-action redir.com" 2) [b.com] redir.com is a redirection to b.com Then happens a form submission from 1) that target 2) with url=redir.com Is the request blocked after the redirection? I think the answer is no in both mode. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:101: EXPECT_EQ(test.redirect_expectation, throttle->WillRedirectRequest()); On 2017/03/09 08:20:04, Mike West (Slow.) wrote: > On 2017/03/07 at 16:25:51, arthursonzogni wrote: > > > Are we landing on an error page? That sounds great, but I thought it was > blocked > > > on changing the error page behavior in such a way as to commit a reasonable > URL > > > (we talked about this in the context of XFO, as I recall). > > I didn't use an error page when the form-action directive is infringed. I > could by returning BLOCK_REQUEST instead of CANCEL in the NavigationThrottle. > > If alexmos@ and creis@ are ok with landing on an error page (I recall some > issues with the Chrome Web Store's in particular being committed), then I'd > prefer it to a blank page. FYI: We currently have some issue with the error pages. This patch should fix them https://codereview.chromium.org/2736863003 A new patch will ensure that there will be no problems with CWS and error pages: https://codereview.chromium.org/2736863003/ Using an error page with form-action is not required for this patch to launch. When there will be no more problems with the error pages, I will enable them. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:157: // is currently not available when PlzNavigate is disabled. On 2017/03/09 05:35:17, nasko (slow) wrote: > On 2017/03/07 16:25:51, arthursonzogni wrote: > > On 2017/02/28 02:48:47, alexmos wrote: > > > I'll defer to clamy@ and nasko@ on whether this is ok to land with a DCHECK > > > without PlzNavigate. Presumably, we could plumb this information without > > > PlzNavigate as well - is that in your plans? If so, could you add a TODO? > > > > Yes, my plan was to eventually plumb this information without PlzNavigate as > > well if we really need it. But I think that the old navigation architecture > will > > be removed before we might need it. > > So, I think that no TODO is required here. > > Adding a TODO and keeping it PlzNavigate specific is fine with me. Okay, but please notice that I will have to remove DCHECK(IsBrowserSideNavigationEnabled()) << "This method is implemented only with PlzNavigate"; Done. https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:158: bool is_form_submission() const; On 2017/03/09 05:35:17, nasko (slow) wrote: > Since this is a simple accessor, it should be inlined in the header. Done.
On 2017/03/10 at 09:35:37, arthursonzogni wrote: > I tried, every layout tests are working with two exceptions. > 1) What if the form url is a javascript url? It this case it is not a navigation and the request use the PlzNavigate path. That's fair. We need to take care of this in Blink. If we're doing that anyway, great. I worry a bit about test coverage, because I'm not confident that we really exercise the redirect bits and pieces of `form-action`, and that's the only bit that will actually trigger all this new code. One option would be to disable the renderer-side checks if PlzNavigate is active _and_ the URL is JavaScript. > 2) What if the form target another window? What if it is? We should handle that case. Does this patch not handle that case? > In addition, maybe there will be a problem (with and without PlzNavigate) in the following scenario: > Two windows, > 1) [a.com] with "form-action redir.com" > 2) [b.com] > redir.com is a redirection to b.com > Then happens a form submission from 1) that target 2) with url=redir.com > Is the request blocked after the redirection? I think the answer is no in both mode. It should be blocked, but I wouldn't be surprised if it isn't. Please file a bug so we can add some tests. > FYI: We currently have some issue with the error pages. This patch should fix them > https://codereview.chromium.org/2736863003 > > A new patch will ensure that there will be no problems with CWS and error pages: > https://codereview.chromium.org/2736863003/ These are the same patch. :) > Using an error page with form-action is not required for this patch to launch. I agree that error pages aren't a blocker for this patch. > When there will be no more problems with the error pages, I will enable them. Can you file a bug for this, because I have questions that probably aren't reasonable to discuss here (like, what network error code will this return, what will the text on the page be, etc. :) ).
The patch looks pretty reasonable at this point. Can you rebase it so we can verify that the tests still pass? I'd also appreciate it if you would skim through the layout tests to verify that we have some redirection checks in there; I'm not sure we do, and if we don't force ourselves through these browser-side checks for the initial request, then I think we probably need more redirect test coverage.
On 2017/03/10 14:04:20, Mike West (Slow.) wrote: > On 2017/03/10 at 09:35:37, arthursonzogni wrote: > > I tried, every layout tests are working with two exceptions. > > 1) What if the form url is a javascript url? It this case it is not a > navigation and the request use the PlzNavigate path. > > That's fair. We need to take care of this in Blink. If we're doing that anyway, > great. I worry a bit about test coverage, because I'm not confident that we > really exercise the redirect bits and pieces of `form-action`, and that's the > only bit that will actually trigger all this new code. > > One option would be to disable the renderer-side checks if PlzNavigate is active > _and_ the URL is JavaScript. > > > 2) What if the form target another window? > > What if it is? We should handle that case. Does this patch not handle that case? No, this patch doesn't handle this case. That are the CSP of the navigating frame that applies. I should work on it. I think we should use the CSP of the frame that has initiated the navigation, not the CSP of the document that was here before the navigation, isn't it? With form resubmission, knowing what is the CSP that applies could be tricky. For instance: 1) the frame that is navigating 2) the frame that puts the data into the form and initiate the first navigation 3) the frame that initiates the navigation (the second one) could be 3 different frames :-) The current behavior(without PlzNavigate) with form **re**submission is 1) and I think that 3) should be the right one. > > In addition, maybe there will be a problem (with and without PlzNavigate) in > the following scenario: > > Two windows, > > 1) [a.com] with "form-action redir.com" > > 2) [b.com] > > http://redir.com is a redirection to b.com > > Then happens a form submission from 1) that target 2) with http://url=redir.com > > Is the request blocked after the redirection? I think the answer is no in both > mode. > > It should be blocked, but I wouldn't be surprised if it isn't. Please file a bug > so we can add some tests. I made a new test and it works without PlzNavigate. It is very surprising for me. It is as if the redirect checks are happening in the frame that has initiated the navigation, not in the one that is navigating. I will publish the test soon.
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
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
Hi, thanks for the reviews! I have tried to understand what is the current behavior in blink and made some tests: https://crrev.com/2747823002 https://crrev.com/2737343004 Blink currently enforce "form-action" for one navigation with two different set of CSP. * With the CSP of the frame that has initiated the navigation. * With the CSP of the frame that is navigating, i.e. the CSP of the document that was here before the new one. There is a check when the navigation starts and each time it is redirected. I think the right behavior is to always use the CSP of the frame that has initiated the navigation. I publish a bug, but it would be hard to fix it. Maybe with PlzNavigate it will be easier to do. https://crbug.com/700964 With PlzNavigate, I was able in the last patch to disable the CSP checks that happens during the navigation in the frame that is navigating. I keep the one that happens in the frame that has initiated the navigation. In the future, I could like to be able to know which frame has initiated the navigation and use the CSP of this document instead. I will not be able to remove entirely the checks that happens in blink. What should I do to land this CL? Should I duplicate all the available layout test such that they would use a redirect?
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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
On 2017/03/14 12:41:45, arthursonzogni wrote: > Hi, thanks for the reviews! > > I have tried to understand what is the current behavior in blink and made some > tests: > https://crrev.com/2747823002 > https://crrev.com/2737343004 > > Blink currently enforce "form-action" for one navigation with two different set > of CSP. > * With the CSP of the frame that has initiated the navigation. > * With the CSP of the frame that is navigating, i.e. the CSP of the document > that was here before the new one. There is a check when the navigation starts > and each time it is redirected. > > I think the right behavior is to always use the CSP of the frame that has > initiated the navigation. > I publish a bug, but it would be hard to fix it. Maybe with PlzNavigate it will > be easier to do. > https://crbug.com/700964 > > With PlzNavigate, I was able in the last patch to disable the CSP checks that > happens during the navigation in the frame that is navigating. > I keep the one that happens in the frame that has initiated the navigation. > > In the future, I could like to be able to know which frame has initiated the > navigation and use the CSP of this document instead. > > I will not be able to remove entirely the checks that happens in blink. What > should I do to land this CL? Should I duplicate all the available layout test > such that they would use a redirect? I duplicate the layout tests such that they are using redirect too. I found by chance a small bug in the renderer side implementation: https://codereview.chromium.org/2749863002/
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
On 2017/03/14 at 12:41:45, arthursonzogni wrote: > I will not be able to remove entirely the checks that happens in blink. What should I do to land this CL? Should I duplicate all the available layout test such that they would use a redirect? I think we can land this patch to get y'all unblocked generally, as long as we have some followup bugs to make sure that we have sane behavior for the edge cases we've uncovered here. Let's keep the blink behavior for now, and figure out how to remove it in future patches. LGTM. Thanks for going so many rounds on this.
https://codereview.chromium.org/2689653003/diff/410001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2689653003/diff/410001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1671: !browserSideNavigationEnabled && I'm fine with leaving this in, as long as we figure out a solution for the small regression. I'm also fine if you'd prefer to drop it and rely on the existing renderer-side behavior so that PlzNavigate can ship more quickly. Either way, please leave the TODO and link to the bug. :)
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_...)
Thank you very much! I will wait the depending patches to be committed. I will also need the L-G-T-M bit from someone that own content, Nasko? https://codereview.chromium.org/2689653003/diff/410001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2689653003/diff/410001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1671: !browserSideNavigationEnabled && On 2017/03/14 15:45:39, Mike West (Slow.) wrote: > I'm fine with leaving this in, as long as we figure out a solution for the small > regression. I'm also fine if you'd prefer to drop it and rely on the existing > renderer-side behavior so that PlzNavigate can ship more quickly. Either way, > please leave the TODO and link to the bug. :) Done. BUG and TODO added.
I'll defer to alexmos@ for the final approval of CSP behavior, I just did a pass for overall code mechanics review. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:35: FormSubmissionThrottle::CheckContentSecurityPolicyFormAction(bool is_redirect) { The method ordering in the .cc file should match the header file. And in general it is ok for the constructor to be up first, even if it is marked private and lower in the header file. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:48: return NavigationThrottle::CANCEL; I would invert the if statement and return PROCEED, where the default will return CANCEL. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:17: // A FormSubmissionThrottle is responsible for enforcing the 'form-action' CSP, nit: CSP directives. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:20: // the 'form-action' directive on the renderer side. nit: renderer process https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. No "(c)" and 2017 https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:86: std::unique_ptr<NavigationHandle> handle = NavigationHandleImpl::Create( This approach feels more like an unit test than a browser test. I'll defer to alexmos@, but I would've expected to see actual form submission coming from the renderer, using an observer to capture the results, and verify against the test expectations. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:923: // Check for mixed content. We do this after the AncestorThrottle and the nit: s/We do this/This is done/ https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:97: bool is_form_submission); There are too many booleans on this method. It might be useful to follow up later on and convert them to enums, so they are more meaningful when reading the callers of this method. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:175: // TODO(arthursonzogni) This value is correct only when PlzNavigate is nit: ":" after the TODO(). https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:176: // enabled. Make it work in both mode. nit: modes. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:224: false, // is_form_submission I know the browser pops up a dialog warning you if you refresh a form submission navigation. Does reload go through this path or a different one? I guess if the submission was allowed in the first place, the reload should be allowed too. However semantically it might be incorrect to say that it isn't a form submission.
LGTM with nits. Looks like all the followup work has bugs filed, and the rest seems good to go. Duplicating the four tests for redirects seems kind of messy, but since Mike is happy with it, I am too. :) Sorry in advance that some of my nits duplicate the ones from Nasko -- I typed them up before I saw Nasko's feedback and was too lazy to deduplicate. :) https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:49: return NavigationThrottle::CANCEL; On 2017/03/09 08:20:03, Mike West (Slow.) wrote: > On 2017/03/07 at 16:25:51, arthursonzogni wrote: > > On 2017/03/02 10:45:34, Mike West (Slow.) wrote: > > > How do you plan to deal with violation reports and `securitypolicyviolation` > > > events? I don't think you need to block testing PlzNavigate on that, but we > > > ought to be surfacing errors to developers before shipping. > > > > The violation report and the 'securitypolicyviolation' event are sent when the > call to AllowContentSecurityPolicy returns false. > > I see, thanks! I also found this non-obvious just by reading the code and seeing IsAllowedByCsp, as it reads like the function just returns a bool without any side effects. Perhaps we can at least put in a comment on IsAllowedByCsp? And maybe IsAllowedByCsp could be named better to reflect that it also sends violations, but it's ok to consider that in a followup. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: update year https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:29: // The FormSubmissionThrottle aren't used without PlzNavigate. nit: s/aren't/isn't/ (here and below) https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:39: // Form submissions are allowed by default when there is not CSP. nit: s/not/no/ https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:59: // redirection. By using this behavior, this test can checks a case where nit: s/checks/check/ https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:86: std::unique_ptr<NavigationHandle> handle = NavigationHandleImpl::Create( On 2017/03/16 21:49:47, nasko (slow) wrote: > This approach feels more like an unit test than a browser test. I'll defer to > alexmos@, but I would've expected to see actual form submission coming from the > renderer, using an observer to capture the results, and verify against the test > expectations. I had the same comment, and Arthur pointed out that we've got layout tests that cover this: https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... This should give us end-to-end coverage, given that we now skip the renderer-side enforcement with PlzNavigate. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:176: // enabled. Make it work in both mode. nit: s/mode/modes/ https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:236: false)); // is_form_submission: nit: no : at the end https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect.html (right): https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect.html:28: <p>Tests that default-src does. If this test passes, you will see a page indicating a form was POSTed.</p> nit: finish the first sentence (default-src does what?). (I know it was duplicated from an existing test, which is also broken, but maybe we can make at least this one complete.) https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-with-redirect.html (right): https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-with-redirect.html:28: <p>Tests that allowed form actions work correctly. If this test passes, you will see a page indicating a form was POSTed.</p> nit: update the description to mention redirects https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html (right): https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html:29: <p>Tests that blocking form actions works correctly. If this test passes, you will see a console error, and will not see a page indicating a form was POSTed.</p> nit: update description to mention redirects.
Patchset #12 (id:470001) has been deleted
Thanks for the reviews! Next step for me is to store the is_form_submission somewhere such that in: NavigationRequest::CreateBrowserInitiated is_form_submission could be set to something different of false. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 21:49:46, nasko (slow) wrote: > nit: 2017 Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 23:05:35, alexmos wrote: > nit: 2017 Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:35: FormSubmissionThrottle::CheckContentSecurityPolicyFormAction(bool is_redirect) { On 2017/03/16 21:49:46, nasko (slow) wrote: > The method ordering in the .cc file should match the header file. And in general > it is ok for the constructor to be up first, even if it is marked private and > lower in the header file. Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.cc:48: return NavigationThrottle::CANCEL; On 2017/03/16 21:49:46, nasko (slow) wrote: > I would invert the if statement and return PROCEED, where the default will > return CANCEL. Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle.h (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:17: // A FormSubmissionThrottle is responsible for enforcing the 'form-action' CSP, On 2017/03/16 21:49:47, nasko (slow) wrote: > nit: CSP directives. Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle.h:20: // the 'form-action' directive on the renderer side. On 2017/03/16 21:49:46, nasko (slow) wrote: > nit: renderer process Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/form_submission_throttle_browsertest.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/03/16 21:49:47, nasko (slow) wrote: > No "(c)" and 2017 Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/03/16 23:05:35, alexmos wrote: > nit: update year Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:29: // The FormSubmissionThrottle aren't used without PlzNavigate. On 2017/03/16 23:05:35, alexmos wrote: > nit: s/aren't/isn't/ (here and below) Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:39: // Form submissions are allowed by default when there is not CSP. On 2017/03/16 23:05:35, alexmos wrote: > nit: s/not/no/ Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:59: // redirection. By using this behavior, this test can checks a case where On 2017/03/16 23:05:36, alexmos wrote: > nit: s/checks/check/ Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/form_submission_throttle_browsertest.cc:86: std::unique_ptr<NavigationHandle> handle = NavigationHandleImpl::Create( On 2017/03/16 23:05:35, alexmos wrote: > On 2017/03/16 21:49:47, nasko (slow) wrote: > > This approach feels more like an unit test than a browser test. I'll defer to > > alexmos@, but I would've expected to see actual form submission coming from > the > > renderer, using an observer to capture the results, and verify against the > test > > expectations. > > I had the same comment, and Arthur pointed out that we've got layout tests that > cover this: > https://codereview.chromium.org/2689653003/diff/290001/content/browser/frame_... > > This should give us end-to-end coverage, given that we now skip the > renderer-side enforcement with PlzNavigate. Yes, I wanted to do an unit test initially, but since I needed the setup a frame tree and the render frame host, I had to make a browsertest. So yes, there is a set of layout test that provides an end-to-end coverage. I have added mine in: https://codereview.chromium.org/2749863002/ https://codereview.chromium.org/2737343004/ https://codereview.chromium.org/2749233002/ And also some layout test in this CL. Please note that blink is still partially enforcing the 'form-action' CSP with PlzNavigate. It is not the ideal solution to prove that it works. That's why I have duplicated the set of layout test such that the navigations are blocked on redirect, so that it tests the browser-side implementation. I found that when the navigation happens with several windows/frames, we don't use the CSP of the right frame, I filled the bug: https://crbug.com/700964 What is the actual behavior with PlzNavigate? Let say that there is two frame/window A and B, A initiates a form submission and B is navigating. renderer-side: check the url against A's CSP browser-side: check the url and its redirects against B's CSP And without PlzNavigate? renderer-side(1): check the url against A's CSP. renderer-side(2): check the url and its redirects against B's CSP. According to me the right behavior is to only use A's CSP. We will be in a better position on the browser-side to fix the problem(i.e. select A instead of B in the FormSubmissionThrottle) https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:923: // Check for mixed content. We do this after the AncestorThrottle and the On 2017/03/16 21:49:47, nasko (slow) wrote: > nit: s/We do this/This is done/ Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:97: bool is_form_submission); On 2017/03/16 21:49:47, nasko (slow) wrote: > There are too many booleans on this method. It might be useful to follow up > later on and convert them to enums, so they are more meaningful when reading the > callers of this method. Yes, I will be happy to fix this in a follow up. See https://crbug.com/702557 https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:175: // TODO(arthursonzogni) This value is correct only when PlzNavigate is On 2017/03/16 21:49:47, nasko (slow) wrote: > nit: ":" after the TODO(). Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:176: // enabled. Make it work in both mode. On 2017/03/16 21:49:47, nasko (slow) wrote: > nit: modes. Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:176: // enabled. Make it work in both mode. On 2017/03/16 23:05:36, alexmos wrote: > nit: s/mode/modes/ Done. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:224: false, // is_form_submission On 2017/03/16 21:49:47, nasko (slow) wrote: > I know the browser pops up a dialog warning you if you refresh a form submission > navigation. Does reload go through this path or a different one? I guess if the > submission was allowed in the first place, the reload should be allowed too. > However semantically it might be incorrect to say that it isn't a form > submission. You are right. The current behavior is to check the CSP on form resubmission. I think it is wrong since the CSPs of the previous document doesn't exist anymore. Instead it uses the CSP of the current document which doesn't make sense to me. It is probably better to allow every form resubmission since the navigation was allowed in the first place. I talk with @mkwst today. Not checking form-action on form resubmission is probably the right thing to do. Nevertheless, care should be taken when the current document is the error page. I will add a TODO for the moment. I am working on storing is_form_submission attribute close to the post_body. https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2689653003/diff/430001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:236: false)); // is_form_submission: On 2017/03/16 23:05:36, alexmos wrote: > nit: no : at the end Done. https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect.html (right): https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect.html:28: <p>Tests that default-src does. If this test passes, you will see a page indicating a form was POSTed.</p> On 2017/03/16 23:05:36, alexmos wrote: > nit: finish the first sentence (default-src does what?). (I know it was > duplicated from an existing test, which is also broken, but maybe we can make at > least this one complete.) Thanks! I will also fix the initial one. https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-with-redirect.html (right): https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-with-redirect.html:28: <p>Tests that allowed form actions work correctly. If this test passes, you will see a page indicating a form was POSTed.</p> On 2017/03/16 23:05:36, alexmos wrote: > nit: update the description to mention redirects Done. https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html (right): https://codereview.chromium.org/2689653003/diff/430001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked-with-redirect.html:29: <p>Tests that blocking form actions works correctly. If this test passes, you will see a console error, and will not see a page indicating a form was POSTed.</p> On 2017/03/16 23:05:36, alexmos wrote: > nit: update description to mention redirects. Done.
Description was changed from ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. 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 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by jam@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...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2689653003/#ps490001 (title: "Addressed comments @alexmos and @nasko.")
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2689653003/#ps490001 (title: "Addressed comments @alexmos and @nasko.")
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
Failed to apply patch for content/browser/frame_host/render_frame_host_impl.cc:
While running git apply --index -p1;
error: patch failed: content/browser/frame_host/render_frame_host_impl.cc:3467
error: content/browser/frame_host/render_frame_host_impl.cc: patch does not
apply
Patch: content/browser/frame_host/render_frame_host_impl.cc
Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc
b/content/browser/frame_host/render_frame_host_impl.cc
index
d876884c8b54eced000a6e7fba8bffab634483a6..d84e2f64cb2a0e05015d250760a548eac77eb6a1
100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -3467,8 +3467,9 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
params.url, params.redirects, frame_tree_node_, is_renderer_initiated,
params.was_within_same_page, base::TimeTicks::Now(),
pending_nav_entry_id,
- false, // started_from_context_menu
- CSPDisposition::CHECK); // should_check_main_world_csp
+ false, // started_from_context_menu
+ CSPDisposition::CHECK, // should_check_main_world_csp
+ false); // is_form_submission
}
// Determine if the current NavigationHandle can be used.
@@ -3521,8 +3522,9 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
params.url, params.redirects, frame_tree_node_, is_renderer_initiated,
params.was_within_same_page, base::TimeTicks::Now(),
entry_id_for_data_nav,
- false, // started_from_context_menu
- CSPDisposition::CHECK); // should_check_main_world_csp
+ false, // started_from_context_menu
+ CSPDisposition::CHECK, // should_check_main_world_csp
+ false); // is_form_submission
}
void RenderFrameHostImpl::BeforeUnloadTimeout() {
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...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/browser/frame_host/render_frame_host_impl.cc:
While running git apply --index -p1;
error: patch failed: content/browser/frame_host/render_frame_host_impl.cc:3467
error: content/browser/frame_host/render_frame_host_impl.cc: patch does not
apply
Patch: content/browser/frame_host/render_frame_host_impl.cc
Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc
b/content/browser/frame_host/render_frame_host_impl.cc
index
d876884c8b54eced000a6e7fba8bffab634483a6..d84e2f64cb2a0e05015d250760a548eac77eb6a1
100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -3467,8 +3467,9 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
params.url, params.redirects, frame_tree_node_, is_renderer_initiated,
params.was_within_same_page, base::TimeTicks::Now(),
pending_nav_entry_id,
- false, // started_from_context_menu
- CSPDisposition::CHECK); // should_check_main_world_csp
+ false, // started_from_context_menu
+ CSPDisposition::CHECK, // should_check_main_world_csp
+ false); // is_form_submission
}
// Determine if the current NavigationHandle can be used.
@@ -3521,8 +3522,9 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
params.url, params.redirects, frame_tree_node_, is_renderer_initiated,
params.was_within_same_page, base::TimeTicks::Now(),
entry_id_for_data_nav,
- false, // started_from_context_menu
- CSPDisposition::CHECK); // should_check_main_world_csp
+ false, // started_from_context_menu
+ CSPDisposition::CHECK, // should_check_main_world_csp
+ false); // is_form_submission
}
void RenderFrameHostImpl::BeforeUnloadTimeout() {
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2689653003/#ps510001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 510001, "attempt_start_ts": 1489818397209340,
"parent_rev": "5496c7e30984ea0712490134b0249ac97772fa5c", "commit_rev":
"7cc34c583545597cffae27ad41bd015db6a93394"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Enforce 'form-action' CSP on the browser-side. Use a NavigationThrottle to enforce the 'form-action' CSP on the browser-side. Before this patch, this CSP wasn't checked on redirections. BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2689653003 Cr-Commit-Position: refs/heads/master@{#457966} Committed: https://chromium.googlesource.com/chromium/src/+/7cc34c583545597cffae27ad41bd... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:510001) as https://chromium.googlesource.com/chromium/src/+/7cc34c583545597cffae27ad41bd... |
