[Connection-Allowlist] Enforce network restrictions on navigations [chromium/src : main]
Shivani Sharma (Gerrit)
Shivani Sharma added 6 comments![]( "Open in Gerrit")
Thanks Alex! Addressed/responded to comments.
// connection allowlist: check whether navigation to the url is allowed.Shivani SharmaNit: capitalize
Done
if (!initiator_frame_token_ ||Shivani SharmaIt might be better to extract this out into a separate check after the feature check, with its own comment that explains it. I'm assuming this is implicitly enforcing the restrictions only for renderer-initiated navigations?
Done
if (matcher.value()->Match(common_params_->url)) {Shivani SharmaIs this going to be sufficient for cases that end up inheriting the initiator origin? If the initiator navigates some other frame (or itself) to about:blank, the resulting document would inherit the initiator's origin, but I'm guessing this is going to block the navigation, since it'll look for a match to "about:blank" - is that desirable? Not sure how connection allowlists spec treats those cases. Same question for srcdoc, and also for sandboxed frames (where the final origin might be opaque - should the allowlist still be enforced based on the target URL in that case?).
Thanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
}Shivani SharmaMight be worthwhile to also do a back navigation and then a forward navigation to verify what the behavior is. I'm guessing that this would depend on how the history navigation is done - history.back()/forward() should still respect connection allowlists (as that's renderer-initiated), while doing them from the browser (via NavigationController::GoBack/GoForward) wouldn't?
Thanks, added 2 tests: one for browser initiated and another for renderer initiated. For the latter, they are currently allowed with a TODO since on disallowing them with the normal error path in BeginNavigationImpl -> OnRequestFailedInternal leads to a crash in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;drc=5f5478c22e63ec0a3effbdc82c9fac2615690334;l=8910 for the history navigation case since render_frame_host_ is already set to null for error navigations and IsPageActivation returns true because of IsServedFromBackForwardCache()
->IssueKeepAliveHandle(keep_alive.InitWithNewPipeAndPassReceiver());Shivani SharmaThanks for adding a test to cover the no-initiator case. Ideally we'd let the blink side issue the keepalive handle and wait for the openee to go away before processing the navigation in the openee... but the timing for this is tricky to get right, and I see a couple of other tests using this pattern, so maybe this is ok. I guess one thing I'm worried about is that there is no check/guarantee that the openee is gone by the time the opener navigation needs to check connection allowlists, but I'm not sure if it's possible to add?
Yeah I tried a few observer patterns but couldn't get the timing right but with this test when I added logs in NavigationRequest::IsAllowedByConnectionAllowlist(), it indeed logged the condition `if (navigation_state)` being true verifying that the code path is being covered.
Related details
- Alex Moshchuk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 6 comments![]( "Open in Gerrit")
if (!initiator_frame_token_ || IsHistory()) {Can you expand/update the comment above to explain that this is more about not currently supporting the error navigation path from here when a history navigation results in restoring something from bfcache and hitting a CHECK, something along the lines of your explanation in another comment below? Currently, the comment makes it seem like the check is just done too late for those cases, which I don't think is necessarily true - I'd expect it to still work here, since the initiator_frame_token should still be [valid](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_controller_impl.cc;l=3488;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3) for history cases, and maybe we just need to fix something later on this path to avoid the CHECK.
if (matcher.value()->Match(common_params_->url)) {Shivani SharmaIs this going to be sufficient for cases that end up inheriting the initiator origin? If the initiator navigates some other frame (or itself) to about:blank, the resulting document would inherit the initiator's origin, but I'm guessing this is going to block the navigation, since it'll look for a match to "about:blank" - is that desirable? Not sure how connection allowlists spec treats those cases. Same question for srcdoc, and also for sandboxed frames (where the final origin might be opaque - should the allowlist still be enforced based on the target URL in that case?).
Thanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
Thanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
Hmm, I'm a bit confused: about:blank navigations should go through [this path](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=2928;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3) for `!NeedsUrlLoader()`. They will bypass a bunch of navigation stages by going straight to ReadyToCommit, but the new IsAllowedByConnectionAllowlist() check is before the `!NeedsUrlLoader()` check in BeginNavigationImpl() (on line 2879 above, with the about:blank path starting on line 2950), so I'd expect it to still be invoked. Does that flow differ from what you're seeing? Have you also tried navigating a non-initial frame at some HTTP URL to about:blank?
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
Ack.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
SG to handle this in a followup. Ack that the CDCs wouldn't run for about: navigations - they are invoked during WillProcessResponse, which about: navigations skip and go straight to ready-to-commit. They do run NavigationThrottle::WillCommitWithoutUrlLoader(), so that's one place to catch them. However, I don't remember how they get their subresource URLLoaderFactories - I'd have expected that something like about:srcdoc would inherit it from its parent, along with any of its restrictions, but sounds like that's not working?
// commitDeferringConditions don't get invoked. Fix this.nit: capitalize C
Shivani SharmaMight be worthwhile to also do a back navigation and then a forward navigation to verify what the behavior is. I'm guessing that this would depend on how the history navigation is done - history.back()/forward() should still respect connection allowlists (as that's renderer-initiated), while doing them from the browser (via NavigationController::GoBack/GoForward) wouldn't?
Thanks, added 2 tests: one for browser initiated and another for renderer initiated. For the latter, they are currently allowed with a TODO since on disallowing them with the normal error path in BeginNavigationImpl -> OnRequestFailedInternal leads to a crash in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;drc=5f5478c22e63ec0a3effbdc82c9fac2615690334;l=8910 for the history navigation case since render_frame_host_ is already set to null for error navigations and IsPageActivation returns true because of IsServedFromBackForwardCache()
Acknowledged
Shivani SharmaMight be worthwhile to also do a back navigation and then a forward navigation to verify what the behavior is. I'm guessing that this would depend on how the history navigation is done - history.back()/forward() should still respect connection allowlists (as that's renderer-initiated), while doing them from the browser (via NavigationController::GoBack/GoForward) wouldn't?
Thanks, added 2 tests: one for browser initiated and another for renderer initiated. For the latter, they are currently allowed with a TODO since on disallowing them with the normal error path in BeginNavigationImpl -> OnRequestFailedInternal leads to a crash in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;drc=5f5478c22e63ec0a3effbdc82c9fac2615690334;l=8910 for the history navigation case since render_frame_host_ is already set to null for error navigations and IsPageActivation returns true because of IsServedFromBackForwardCache()
That's unfortunate (and unexpected), but I guess it makes sense that maybe we haven't really had to fail history navigations that restore something from bfcache before. And I'm assuming it still makes sense that if something is going to be restored from bfcache but is disallowed by connection allowlists, it should be blocked, even though no network request is going to be made?
// The navigation should not be successful.Let's mention something similar to your other TODO, how this could eventually be allowed since it doesn't make a network request, once we find a way to restrict its subresources.
Related details
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 5 comments![]( "Open in Gerrit")
Can you expand/update the comment above to explain that this is more about not currently supporting the error navigation path from here when a history navigation results in restoring something from bfcache and hitting a CHECK, something along the lines of your explanation in another comment below? Currently, the comment makes it seem like the check is just done too late for those cases, which I don't think is necessarily true - I'd expect it to still work here, since the initiator_frame_token should still be [valid](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_controller_impl.cc;l=3488;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3) for history cases, and maybe we just need to fix something later on this path to avoid the CHECK.
Confirmed that initiator_frame_token is valid for these cases.
Created an issue on the explainer as to what the behavior should be and also an issue for the CHECK and added to this TODO comment.
if (matcher.value()->Match(common_params_->url)) {Shivani SharmaIs this going to be sufficient for cases that end up inheriting the initiator origin? If the initiator navigates some other frame (or itself) to about:blank, the resulting document would inherit the initiator's origin, but I'm guessing this is going to block the navigation, since it'll look for a match to "about:blank" - is that desirable? Not sure how connection allowlists spec treats those cases. Same question for srcdoc, and also for sandboxed frames (where the final origin might be opaque - should the allowlist still be enforced based on the target URL in that case?).
Alex MoshchukThanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
Thanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
Hmm, I'm a bit confused: about:blank navigations should go through [this path](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=2928;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3) for `!NeedsUrlLoader()`. They will bypass a bunch of navigation stages by going straight to ReadyToCommit, but the new IsAllowedByConnectionAllowlist() check is before the `!NeedsUrlLoader()` check in BeginNavigationImpl() (on line 2879 above, with the about:blank path starting on line 2950), so I'd expect it to still be invoked. Does that flow differ from what you're seeing? Have you also tried navigating a non-initial frame at some HTTP URL to about:blank?
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
Ack.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
SG to handle this in a followup. Ack that the CDCs wouldn't run for about: navigations - they are invoked during WillProcessResponse, which about: navigations skip and go straight to ready-to-commit. They do run NavigationThrottle::WillCommitWithoutUrlLoader(), so that's one place to catch them. However, I don't remember how they get their subresource URLLoaderFactories - I'd have expected that something like about:srcdoc would inherit it from its parent, along with any of its restrictions, but sounds like that's not working?
1) For the test EmptyIframeInjectedScriptFetch, even BeginNavigationImpl doesn't get invoked for the about:blank navigation, but if a non-empty iframe is navigated to about:blank, it does invoke IsAllowedByConnectionAllowlist() so in that case it will follow the same path as srcdoc.
2) Good to know about the NavigationThrottle::WillCommitWithoutUrlLoader() path, will create a follow up CL for this. I am not entirely sure which code path creates the separate factory for these but I checked in https://source.chromium.org/chromium/chromium/src/+/main:services/network/cors/cors_url_loader_factory.cc;l=438;drc=aeb46299fefab41457b0c50e5386616d2e8d64a2;bpv=1;bpt=1?q=cors_url_loader_factory.&ss=chromium%2Fchromium%2Fsrc
that it was different from its initiator.
// commitDeferringConditions don't get invoked. Fix this.Shivani Sharmanit: capitalize C
Done
Shivani SharmaMight be worthwhile to also do a back navigation and then a forward navigation to verify what the behavior is. I'm guessing that this would depend on how the history navigation is done - history.back()/forward() should still respect connection allowlists (as that's renderer-initiated), while doing them from the browser (via NavigationController::GoBack/GoForward) wouldn't?
Alex MoshchukThanks, added 2 tests: one for browser initiated and another for renderer initiated. For the latter, they are currently allowed with a TODO since on disallowing them with the normal error path in BeginNavigationImpl -> OnRequestFailedInternal leads to a crash in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;drc=5f5478c22e63ec0a3effbdc82c9fac2615690334;l=8910 for the history navigation case since render_frame_host_ is already set to null for error navigations and IsPageActivation returns true because of IsServedFromBackForwardCache()
Shivani SharmaThat's unfortunate (and unexpected), but I guess it makes sense that maybe we haven't really had to fail history navigations that restore something from bfcache before. And I'm assuming it still makes sense that if something is going to be restored from bfcache but is disallowed by connection allowlists, it should be blocked, even though no network request is going to be made?
We haven't settled on whether it definitely needs to be blocked as in the WICG issue (https://github.com/WICG/connection-allowlists/issues/4) and created a crbug for the CHECK. Updated the comment in the code to include these.
Let's mention something similar to your other TODO, how this could eventually be allowed since it doesn't make a network request, once we find a way to restrict its subresources.
- Alex Moshchuk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
2 comments
Thanks, LGTM for this CL, leaving the remaining issues (about:, history navigations) for followups.
if (matcher.value()->Match(common_params_->url)) {Shivani SharmaIs this going to be sufficient for cases that end up inheriting the initiator origin? If the initiator navigates some other frame (or itself) to about:blank, the resulting document would inherit the initiator's origin, but I'm guessing this is going to block the navigation, since it'll look for a match to "about:blank" - is that desirable? Not sure how connection allowlists spec treats those cases. Same question for srcdoc, and also for sandboxed frames (where the final origin might be opaque - should the allowlist still be enforced based on the target URL in that case?).
Alex MoshchukThanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
Shivani SharmaThanks for pointing out this case.
A few observations:
- about:blank doesn't go through this function and the navigation succeeds as tested in the existing test: EmptyIframeInjectedScriptFetch.
Hmm, I'm a bit confused: about:blank navigations should go through [this path](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=2928;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3) for `!NeedsUrlLoader()`. They will bypass a bunch of navigation stages by going straight to ReadyToCommit, but the new IsAllowedByConnectionAllowlist() check is before the `!NeedsUrlLoader()` check in BeginNavigationImpl() (on line 2879 above, with the about:blank path starting on line 2950), so I'd expect it to still be invoked. Does that flow differ from what you're seeing? Have you also tried navigating a non-initial frame at some HTTP URL to about:blank?
- For sandbox frames, since the network is accessed, makes sense to check against the restrictions.
Ack.
- For the remaining local schemes like about:srcdoc, added a TODO, as it has its own URLLoaderFactory for subresources and the CommitDeferringConditions doesn't get executed for it. So even though the policy container is inherited, the CDC logic doesn't get triggered for them. Also added a test. As a follow up, it would be helpful to evaluate if there is a path forward via CDC or another approach in these cases. If not, we might have to continue to disallow them for an initiating frame with connection allowlist header.
SG to handle this in a followup. Ack that the CDCs wouldn't run for about: navigations - they are invoked during WillProcessResponse, which about: navigations skip and go straight to ready-to-commit. They do run NavigationThrottle::WillCommitWithoutUrlLoader(), so that's one place to catch them. However, I don't remember how they get their subresource URLLoaderFactories - I'd have expected that something like about:srcdoc would inherit it from its parent, along with any of its restrictions, but sounds like that's not working?
1) For the test EmptyIframeInjectedScriptFetch, even BeginNavigationImpl doesn't get invoked for the about:blank navigation, but if a non-empty iframe is navigated to about:blank, it does invoke IsAllowedByConnectionAllowlist() so in that case it will follow the same path as srcdoc.
2) Good to know about the NavigationThrottle::WillCommitWithoutUrlLoader() path, will create a follow up CL for this. I am not entirely sure which code path creates the separate factory for these but I checked in https://source.chromium.org/chromium/chromium/src/+/main:services/network/cors/cors_url_loader_factory.cc;l=438;drc=aeb46299fefab41457b0c50e5386616d2e8d64a2;bpv=1;bpt=1?q=cors_url_loader_factory.&ss=chromium%2Fchromium%2Fsrc
that it was different from its initiator.
Some initial about:blank "navigations" are only surfaced to the browser via DidCommitNavigation, without going through any of the normal navigation paths. Maybe that's what you were seeing. In any case, I'd expect that you wouldn't want to block same-origin navigations to about:blank, whether they're initial or a result or navigating away from some other document, regardless of connection allowlist, due to compat implications, but that can be ironed out in followups.
Related details
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Connection-Allowlist] Enforce network restrictions on navigations
Explainer: https://github.com/WICG/connection-allowlists
This CL enforces connection-allowlist header to be checked for
navigations. The earlier CL https://chromium-review.googlesource.com/c/chromium/src/+/7085134 enforced the check
for subresources in the network service while navigation will be checked
in the browser process at the time the navigation is started.
TODO: Note that this CL does not check for redirects and the committed
URL since that behavior has not been decided yet
(https://github.com/WICG/connection-allowlists?tab=readme-ov-file#youre-going-to-have-to-rethink-a-ban-on-redirects)
It tests that only renderer-initiated navigations should be blocked and
not the ones initiated from the browser, like via the omnibox. Also
tests NavigationStateKeepAlive code path.
Bug: 447954811
- M content/DEPS
- M content/browser/BUILD.gn
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/renderer_host/navigation_request.h
- M content/browser/renderer_host/render_frame_host_impl_browsertest.cc
Code-Review: +1 by Alex Moshchuk
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |