Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2909513002:
Move PlzNavigate frame-src CSP check to NavigationRequest (Closed)
DescriptionMove PlzNavigate frame-src CSP check to NavigationRequest
This is in the process of moving upgrade-insecure-requests to the browser
process for frame-src checks for PlzNavigate. By doing the frame-src CSP check
in NavigationRequest instead of a NavigationThrottle, we will be able to modify
the request URL if required by upgrade-insecure-requests. This CL just moves the
existing CSP check from AncestorThrottle to NavigationRequest but does not yet
implement upgrade-insecure-requests or reporting for upgrade-insecure-requests
(that will be in a follow-up CL).
BUG=713388
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2909513002
Cr-Commit-Position: refs/heads/master@{#477388}
Committed: https://chromium.googlesource.com/chromium/src/+/1cfb38a9685f1e8561435982acbe797912cdb9e1
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : move csp before navigation throttles #Patch Set 4 : remove redundant check #Patch Set 5 : allow null SourceLocation #Patch Set 6 : create navigation handle before checking csp #Patch Set 7 : fix ordering again #Patch Set 8 : rebase #Patch Set 9 : comment tweaks #
Total comments: 19
Patch Set 10 : nasko comments #Patch Set 11 : use same comment in OnRequestRedirected #Patch Set 12 : rebase #
Dependent Patchsets: Messages
Total messages: 47 (35 generated)
Description was changed from ========== Move PlzNavigate frame-src CSP check to NavigationRequest This is in the process of moving upgrade-insecure-requests to the browser process for frame-src checks for PlzNavigate. By doing the frame-src CSP check in NavigationRequest instead of a NavigationThrottle, we will be able to modify the request URL if required by upgrade-insecure-requests. This CL just moves the existing CSP check from AncestorThrottle to NavigationRequest but does not yet implement upgrade-insecure-requests or reporting for upgrade-insecure-requests (that will be in a follow-up CL). BUG=713388 ========== to ========== Move PlzNavigate frame-src CSP check to NavigationRequest This is in the process of moving upgrade-insecure-requests to the browser process for frame-src checks for PlzNavigate. By doing the frame-src CSP check in NavigationRequest instead of a NavigationThrottle, we will be able to modify the request URL if required by upgrade-insecure-requests. This CL just moves the existing CSP check from AncestorThrottle to NavigationRequest but does not yet implement upgrade-insecure-requests or reporting for upgrade-insecure-requests (that will be in a follow-up CL). BUG=713388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estark@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 estark@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 estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estark@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 estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
estark@chromium.org changed reviewers: + mkwst@chromium.org
mkwst: could you please take a look at this and its follow-up https://codereview.chromium.org/2910573002/ before I pass them on to content OWNERS? A testing note: this CL is pretty much covered by existing tests, the follow-up has additional tests.
This CL LGTM % a tiny nit. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.h:229: bool is_redirect); Style nit: If this was blink, I'd ask for this to be an enum. As it isn't blink, I'll defer to the //content owners (but I'd prefer an enum :) ).
estark@chromium.org changed reviewers: + nasko@chromium.org
Nasko: I'm sending this CL and a follow-up your way but I know you're OOO -- there's no rush on these. The goal is to enable CSP reporting for frame navigation requests that are upgraded via upgrade-insecure-requests. We talked about this approach a few weeks ago, lmk if you've paged it out and want me to give some more context. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.h:229: bool is_redirect); On 2017/05/30 07:27:51, Mike West wrote: > Style nit: If this was blink, I'd ask for this to be an enum. As it isn't blink, > I'll defer to the //content owners (but I'd prefer an enum :) ). Nasko: any preference? Personally I'd be more inclined to enum for a public method but feel that it's slightly overkill for a private method.
nasko@chromium.org changed reviewers: + clamy@chromium.org
Adding clamy@, as in a previous discussion about this, she mentioned that we might not be able to upgrade on redirects.
On 2017/06/01 05:25:48, nasko (out until 6-1) wrote: > Adding clamy@, as in a previous discussion about this, she mentioned that we > might not be able to upgrade on redirects. The issue with redirects is at the net stack level. It should be easy to implement some mechanism in the UI thread to determine we need to upgrade to https on a redirect and pass that info to the net stack. However I don't know if we can actually change the url in the URLRequest itself. We should ask mmenke@.
On 2017/06/01 14:58:31, clamy (slow) wrote: > On 2017/06/01 05:25:48, nasko (out until 6-1) wrote: > > Adding clamy@, as in a previous discussion about this, she mentioned that we > > might not be able to upgrade on redirects. > > The issue with redirects is at the net stack level. It should be easy to > implement some mechanism in the UI thread to determine we need to upgrade to > https on a redirect and pass that info to the net stack. However I don't know if > we can actually change the url in the URLRequest itself. We should ask mmenke@. I posted this in the follow-up CL but meant to post it here: upgrade-insecure-requests doesn't work at all for redirects right now, either with or without PlzNavigate, and I'm not trying to fix that here. I'm just trying to bring UIR reporting in PlzNavigate to parity with non-PlzNavigate. We will have to fix the redirect issue separately in some other way since we can't modify the URLRequest while following the redirect. (https://crbug.com/615885)
Mostly nits and just one question. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:391: // navigation throttles allow, but would block it otherwise. For the same I'm finding this sentence a bit hard to parse, but unfortunately cannot offer a better alternative :(. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:392: // reason, the navigation handle is created afterwards -- so that it gets the nit: s/navigation handle/NavigationHande/ https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:400: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has nit: empty line before the comment. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:535: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has nit: an empty line before the comment. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:932: CSPDisposition::DO_NOT_CHECK) nit: two line if statement needs {}. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:948: is_redirect, source_location)) { Can't we just pass in common_params_.source_location? Why do we need to make a copy? https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.h:229: bool is_redirect); On 2017/06/01 04:12:51, estark wrote: > On 2017/05/30 07:27:51, Mike West wrote: > > Style nit: If this was blink, I'd ask for this to be an enum. As it isn't > blink, > > I'll defer to the //content owners (but I'd prefer an enum :) ). > > Nasko: any preference? Personally I'd be more inclined to enum for a public > method but feel that it's slightly overkill for a private method. bools are used quite a bit through content/, so I'm fine with that approach, especially for private method, as Emily noted.
Thanks, Nasko. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:391: // navigation throttles allow, but would block it otherwise. For the same On 2017/06/05 20:46:29, nasko (slow) wrote: > I'm finding this sentence a bit hard to parse, but unfortunately cannot offer a > better alternative :(. Reworded, hopefully better now https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:392: // reason, the navigation handle is created afterwards -- so that it gets the On 2017/06/05 20:46:29, nasko (slow) wrote: > nit: s/navigation handle/NavigationHande/ Done. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:400: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has On 2017/06/05 20:46:29, nasko (slow) wrote: > nit: empty line before the comment. Done. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:535: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has On 2017/06/05 20:46:30, nasko (slow) wrote: > nit: an empty line before the comment. Done. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:932: CSPDisposition::DO_NOT_CHECK) On 2017/06/05 20:46:29, nasko (slow) wrote: > nit: two line if statement needs {}. Done. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:948: is_redirect, source_location)) { On 2017/06/05 20:46:29, nasko (slow) wrote: > Can't we just pass in common_params_.source_location? Why do we need to make a > copy? Changed to value_or https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.h:229: bool is_redirect); On 2017/06/05 20:46:30, nasko (slow) wrote: > On 2017/06/01 04:12:51, estark wrote: > > On 2017/05/30 07:27:51, Mike West wrote: > > > Style nit: If this was blink, I'd ask for this to be an enum. As it isn't > > blink, > > > I'll defer to the //content owners (but I'd prefer an enum :) ). > > > > Nasko: any preference? Personally I'd be more inclined to enum for a public > > method but feel that it's slightly overkill for a private method. > > bools are used quite a bit through content/, so I'm fine with that approach, > especially for private method, as Emily noted. Acknowledged.
LGTM https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:391: // navigation throttles allow, but would block it otherwise. For the same On 2017/06/06 00:38:02, estark wrote: > On 2017/06/05 20:46:29, nasko (slow) wrote: > > I'm finding this sentence a bit hard to parse, but unfortunately cannot offer > a > > better alternative :(. > > Reworded, hopefully better now Much better! Thanks! Can you also use the same wording in NavigationRequest::OnRequestRedirected? A very similar comment lives there and it will be awesome if the two are in sync. https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:948: is_redirect, source_location)) { On 2017/06/06 00:38:03, estark wrote: > On 2017/06/05 20:46:29, nasko (slow) wrote: > > Can't we just pass in common_params_.source_location? Why do we need to make a > > copy? > > Changed to value_or Nice! TIL about value_or and the code does look nicer that way.
https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2909513002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:391: // navigation throttles allow, but would block it otherwise. For the same On 2017/06/06 01:03:44, nasko (slow) wrote: > On 2017/06/06 00:38:02, estark wrote: > > On 2017/06/05 20:46:29, nasko (slow) wrote: > > > I'm finding this sentence a bit hard to parse, but unfortunately cannot > offer > > a > > > better alternative :(. > > > > Reworded, hopefully better now > > Much better! Thanks! > Can you also use the same wording in NavigationRequest::OnRequestRedirected? A > very similar comment lives there and it will be awesome if the two are in sync. Done.
Patchset #12 (id:220001) has been deleted
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2909513002/#ps240001 (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": 240001, "attempt_start_ts": 1496776997159700,
"parent_rev": "8b1bdd82d7a48acc3def05005120f3d2f78423b6", "commit_rev":
"1cfb38a9685f1e8561435982acbe797912cdb9e1"}
Message was sent while issue was closed.
Description was changed from ========== Move PlzNavigate frame-src CSP check to NavigationRequest This is in the process of moving upgrade-insecure-requests to the browser process for frame-src checks for PlzNavigate. By doing the frame-src CSP check in NavigationRequest instead of a NavigationThrottle, we will be able to modify the request URL if required by upgrade-insecure-requests. This CL just moves the existing CSP check from AncestorThrottle to NavigationRequest but does not yet implement upgrade-insecure-requests or reporting for upgrade-insecure-requests (that will be in a follow-up CL). BUG=713388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move PlzNavigate frame-src CSP check to NavigationRequest This is in the process of moving upgrade-insecure-requests to the browser process for frame-src checks for PlzNavigate. By doing the frame-src CSP check in NavigationRequest instead of a NavigationThrottle, we will be able to modify the request URL if required by upgrade-insecure-requests. This CL just moves the existing CSP check from AncestorThrottle to NavigationRequest but does not yet implement upgrade-insecure-requests or reporting for upgrade-insecure-requests (that will be in a follow-up CL). BUG=713388 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2909513002 Cr-Commit-Position: refs/heads/master@{#477388} Committed: https://chromium.googlesource.com/chromium/src/+/1cfb38a9685f1e8561435982acbe... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/1cfb38a9685f1e8561435982acbe... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
