[Connection-Allowlist] Redirects blocked for navigations [chromium/src : main]
Mike West (Gerrit)
Mike West added 4 comments![]( "Open in Gerrit")
bool is_redirect_allowed_ = true;Optional nit: I'd prefer something more specific to Connection Allowlist (as redirects might be disallowed for other reasons, including the request's `mode`). Perhaps invert this to something like `connection_allowlists_blocks_redirect_ = false`?
network::URLLoaderCompletionStatus(net::ERR_NETWORK_ACCESS_REVOKED);Nit for a future CL: We should make this an error more specific to connection allowlists. Or `ERR_UNSAFE_REDIRECT`?
return;Should we set `error_navigation_trigger_ = ErrorNavigationTrigger::kRedirectNotAllowed;`?
false /* is_response_check */);I wonder whether it might make sense to follow CSP's implementation here by centralizing the redirect check in the Connection Allowlist matching logic and passing in a boolean noting the redirect status. Since you'll end up holding a boolean on the policy object once we're parsing it out of the header, that might make more sense than holding the additional boolean on the NavigationRequest.
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 4 comments![]( "Open in Gerrit")
Optional nit: I'd prefer something more specific to Connection Allowlist (as redirects might be disallowed for other reasons, including the request's `mode`). Perhaps invert this to something like `connection_allowlists_blocks_redirect_ = false`?
Done
network::URLLoaderCompletionStatus(net::ERR_NETWORK_ACCESS_REVOKED);Nit for a future CL: We should make this an error more specific to connection allowlists. Or `ERR_UNSAFE_REDIRECT`?
Updating in this CL to ERR_UNSAFE_REDIRECT so we can also use the same in the subresource redirect behavior
Should we set `error_navigation_trigger_ = ErrorNavigationTrigger::kRedirectNotAllowed;`?
Done
I wonder whether it might make sense to follow CSP's implementation here by centralizing the redirect check in the Connection Allowlist matching logic and passing in a boolean noting the redirect status. Since you'll end up holding a boolean on the policy object once we're parsing it out of the header, that might make more sense than holding the additional boolean on the NavigationRequest.
I've added the check in NavigationRequest::IsAllowedByConnectionAllowlist() as that will allow for a single point for reporting, but still kept it as a boolean on NavigationRequest so we don't have to look into the policy during redirect. Added a TODO to possibly change that when we add the support for the parameter.
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
- Shivani Sharma
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 added 3 comments![]( "Open in Gerrit")
Thanks!
if (has_followed_redirect && connection_allowlists_blocks_redirect_) {Should we move this below the `base::FeatureList::IsEnabled(network::features::kConnectionAllowlists)` check, just to be safe?
connection_allowlists_blocks_redirect_ = true;Is this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
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 3 comments![]( "Open in Gerrit")
if (has_followed_redirect && connection_allowlists_blocks_redirect_) {Should we move this below the `base::FeatureList::IsEnabled(network::features::kConnectionAllowlists)` check, just to be safe?
Done
connection_allowlists_blocks_redirect_ = true;Is this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
Is this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
done
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
The next CL with the attribute for redirect will be a fast follow.
The TODO was added based on the thread here: https://chromium-review.googlesource.com/c/chromium/src/+/7599175/4..6/content/browser/renderer_host/navigation_request.cc#b3782
Although maybe continuing to keep the field could be simpler to not have to worry about testing the other early returns again for the redirect path or figuring which of them should be tested for redirect vs not which could introduce bugs.
I am not too sure of the out param though as I don't see many similar out booleans but I added a side effect comment on the function definition similar to how CheckContentSecurityPolicy() does. Does that sound ok to you or would you rather have the out param?
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
2 comments
LGTM, thanks!
connection_allowlists_blocks_redirect_ = true;Shivani SharmaIs this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
Is this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
done
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
The next CL with the attribute for redirect will be a fast follow.
The TODO was added based on the thread here: https://chromium-review.googlesource.com/c/chromium/src/+/7599175/4..6/content/browser/renderer_host/navigation_request.cc#b3782
Although maybe continuing to keep the field could be simpler to not have to worry about testing the other early returns again for the redirect path or figuring which of them should be tested for redirect vs not which could introduce bugs.I am not too sure of the out param though as I don't see many similar out booleans but I added a side effect comment on the function definition similar to how CheckContentSecurityPolicy() does. Does that sound ok to you or would you rather have the out param?
I guess we can keep it for now and try to revisit when reporting and redirect handling are more fleshed out - thanks for adding the comments about side effects, which helps.
For the out bool, a couple of existing examples are RFHI::ShouldBypassSecurityChecksForErrorPage() and MixedContentChecker::ShouldBlockInternal().
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. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/58123.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
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 added 3 comments![]( "Open in Gerrit")
Thanks!
if (!was_redirected_ &&@ale...@chromium.org: I just added this since we should only be checking at the first redirect, since Connection Allowlists will either allow all redirects or disallow all.
connection_allowlists_blocks_redirect_ = true;Shivani SharmaIs this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
Alex MoshchukIs this more of a bool for "Connection allowlists are in use, and were checked when this navigation started", to distinguish it from all the other early returns? And then we assume that redirects in all navigations that were subjected to connection allowlists should fail for now? Could we clarify that in the comment for the definition of this field?
done
This is a bit unfortunate, as I wouldn't expect IsAllowedByConnectionAllowlist() to make state changes on NavigationRequest. Would it be worth it if BeginNavigationImpl would set this state on NavigationRequest after it checks IsAllowedByConnectionAllowlist() (which could return an additional bool for it)? Might depend on whether you plan to address the TODO in the short vs long term.
The next CL with the attribute for redirect will be a fast follow.
The TODO was added based on the thread here: https://chromium-review.googlesource.com/c/chromium/src/+/7599175/4..6/content/browser/renderer_host/navigation_request.cc#b3782
Although maybe continuing to keep the field could be simpler to not have to worry about testing the other early returns again for the redirect path or figuring which of them should be tested for redirect vs not which could introduce bugs.I am not too sure of the out param though as I don't see many similar out booleans but I added a side effect comment on the function definition similar to how CheckContentSecurityPolicy() does. Does that sound ok to you or would you rather have the out param?
I guess we can keep it for now and try to revisit when reporting and redirect handling are more fleshed out - thanks for adding the comments about side effects, which helps.
For the out bool, a couple of existing examples are RFHI::ShouldBypassSecurityChecksForErrorPage() and MixedContentChecker::ShouldBlockInternal().
ack, thanks!
cc @ave...@chromium.org who might be adding the redirect attribute handling.
Related details
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
if (!was_redirected_ &&@ale...@chromium.org: I just added this since we should only be checking at the first redirect, since Connection Allowlists will either allow all redirects or disallow all.
- Alex Moshchuk
- 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] Redirects blocked for navigations
This CL implements blocking server-side redirects for navigations
that were allowed via connection allowlist. Follow up CLs will ensure
the same behavior for subresources and worker scripts.
Note that a follow up CL will also implement the redirection-allowed
attribute to allow redirects.
This behavior is in discussion in the issue:
https://github.com/WICG/connection-allowlists/issues/7
Bug: 447954811
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/renderer_host/navigation_request.h
- A third_party/blink/web_tests/external/wpt/connection-allowlist/tentative/navigation-redirect.sub.window.js
- A third_party/blink/web_tests/external/wpt/connection-allowlist/tentative/navigation-redirect.sub.window.js.headers
Code-Review: +1 by Alex Moshchuk, +1 by Mike West
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/58123
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |