[Connection-Allowlist] Block subresource and worker requests redirects [chromium/src : main]
Mike West (Gerrit)
Mike West added 2 comments![]( "Open in Gerrit")
Tests look good, thanks! I have a question about the implementation approach.
}I was expecting this to be somewhat similar to the approach you took in navigation, performing a check against `context_->IsNetworkForNonceAndUrlAllowed(...)` and passing in the redirect state. Is that an approach you rejected? It seems preferable to avoid holding another flag here if possible, since we're going to have the whole connection allowlists object in the network context to do the evaluation.
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 1 comment![]( "Open in Gerrit")
I was expecting this to be somewhat similar to the approach you took in navigation, performing a check against `context_->IsNetworkForNonceAndUrlAllowed(...)` and passing in the redirect state. Is that an approach you rejected? It seems preferable to avoid holding another flag here if possible, since we're going to have the whole connection allowlists object in the network context to do the evaluation.
The scenario here is a bit different than navigation, where both the navigation start and redirect checks were on the NavigationRequest and also the checking function IsAllowedByConnectionAllowlist(). So we could store the state that this is a navigation allowed via connection allowlist and thus block the redirect.
Here the start check is in CorsURLLoaderFactory, redirect check is in CorsURLLoader which does not have the network_restrictions_id nor does it know that it was allowed via connection allowlist.
But I agree that there is benefit to centralize the logic so that reporting can happen from one place. Updated to propagate the network_restrictions_id to CorsURLLoader and on redirect invoke context_->IsNetworkForNonceAndUrlAllowed(...). This has the downside of checking the map again but that should be ok given its O(log n) time, only invoked at most for the 1st redirect and we need to access it anyways for reporting. We do make sure though to only check for requests that were initially allowed via connection allowlists.
Related details
- Mike West
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 1 comment![]( "Open in Gerrit")
- Kenichi Ishibashi
- Mike West
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
2 comments
I'll defer to mkwst@ for actual review, so stamp-ish lgtm.
bool IsNetworkForNonceAndUrlAllowed(const base::UnguessableToken& nonce,The style guide suggests using return value over output parameters. It would be nice to follow the suggestion. Not a blocker though so feel free to mark this resolved if you disagree.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
Related details
- Mike West
- 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. |
Mike West (Gerrit)
Mike West added 4 comments![]( "Open in Gerrit")
Shivani SharmaI was expecting this to be somewhat similar to the approach you took in navigation, performing a check against `context_->IsNetworkForNonceAndUrlAllowed(...)` and passing in the redirect state. Is that an approach you rejected? It seems preferable to avoid holding another flag here if possible, since we're going to have the whole connection allowlists object in the network context to do the evaluation.
The scenario here is a bit different than navigation, where both the navigation start and redirect checks were on the NavigationRequest and also the checking function IsAllowedByConnectionAllowlist(). So we could store the state that this is a navigation allowed via connection allowlist and thus block the redirect.
Here the start check is in CorsURLLoaderFactory, redirect check is in CorsURLLoader which does not have the network_restrictions_id nor does it know that it was allowed via connection allowlist.
But I agree that there is benefit to centralize the logic so that reporting can happen from one place. Updated to propagate the network_restrictions_id to CorsURLLoader and on redirect invoke context_->IsNetworkForNonceAndUrlAllowed(...). This has the downside of checking the map again but that should be ok given its O(log n) time, only invoked at most for the 1st redirect and we need to access it anyways for reporting. We do make sure though to only check for requests that were initially allowed via connection allowlists.
Thanks for the explanation. I dug through the code a bit this morning, and this is more complicated than I thought. :) It spreads the checks over more layers than I expected and requires ductwork in both directions to pass state around that we really should already have access to. Hrm.
I also think it's going to be substantially more complicated when we have to deal with report-only policies, so I'm glad I've been sick and haven't finished any of the reporting CLs yet... :)
I have small thoughts below for potential improvements. I would very much like to avoid the out parameter in particular.
options |= mojom::kURLLoadOptionAllowedByConnectionAllowlist;We only use the out parameter here to determine whether we'll ask the `CorsURLLoader` to perform connection allowlist checks by setting this option, and it's not actually clear to me that that's a meaningful cost. Doing a map lookup on one response (due to the `redirect_count_ == 0` check) is trivial, and the complexity we'd add via this additional flag and the out parameter seems higher than it's worth.
How would you feel about dropping the out parameter and this flag, and shifting the `CorsURLLoader` to perform the allowlist check `if (redirect_count_ == 0 && network_restrictions_id_)`?
*out_is_allowlisted = true;Would we need this out parameter if we didn't have the exception list below?
Skimming through the code, it looks like the exception mechanism is only called from `RenderFrameHostImpl::ExemptUrlFromNetworkRevocationForTesting()`, and none of the relevant tests in `NetworkContextTest` seem to deal with redirects. Can we simply apply the same behavior there, and avoid the out-parameter entirely (e.g. all redirected requests are either allowed or blocked as per connection allowlist)? Is it called elsewhere that I missed?
(More broadly: do we need the exception list at all at this point? What pieces of Fenced Frame infrastructure are we keeping?)
const uint32 kURLLoadOptionAllowedByConnectionAllowlist = 1024;Nit: Perhaps rename this to something like `kURLLoadOptionConnectionAllowlistEnforced` (or `...Present` or `...Declared` or something)? Using `AllowedByConnectionAllowlist` to determine whether we're _blocking_ a redirect response due to connection allowlist is a little unclear.
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 6 comments![]( "Open in Gerrit")
Shivani SharmaI was expecting this to be somewhat similar to the approach you took in navigation, performing a check against `context_->IsNetworkForNonceAndUrlAllowed(...)` and passing in the redirect state. Is that an approach you rejected? It seems preferable to avoid holding another flag here if possible, since we're going to have the whole connection allowlists object in the network context to do the evaluation.
Mike WestThe scenario here is a bit different than navigation, where both the navigation start and redirect checks were on the NavigationRequest and also the checking function IsAllowedByConnectionAllowlist(). So we could store the state that this is a navigation allowed via connection allowlist and thus block the redirect.
Here the start check is in CorsURLLoaderFactory, redirect check is in CorsURLLoader which does not have the network_restrictions_id nor does it know that it was allowed via connection allowlist.
But I agree that there is benefit to centralize the logic so that reporting can happen from one place. Updated to propagate the network_restrictions_id to CorsURLLoader and on redirect invoke context_->IsNetworkForNonceAndUrlAllowed(...). This has the downside of checking the map again but that should be ok given its O(log n) time, only invoked at most for the 1st redirect and we need to access it anyways for reporting. We do make sure though to only check for requests that were initially allowed via connection allowlists.
Thanks for the explanation. I dug through the code a bit this morning, and this is more complicated than I thought. :) It spreads the checks over more layers than I expected and requires ductwork in both directions to pass state around that we really should already have access to. Hrm.
I also think it's going to be substantially more complicated when we have to deal with report-only policies, so I'm glad I've been sick and haven't finished any of the reporting CLs yet... :)
I have small thoughts below for potential improvements. I would very much like to avoid the out parameter in particular.
Acknowledged
options |= mojom::kURLLoadOptionAllowedByConnectionAllowlist;We only use the out parameter here to determine whether we'll ask the `CorsURLLoader` to perform connection allowlist checks by setting this option, and it's not actually clear to me that that's a meaningful cost. Doing a map lookup on one response (due to the `redirect_count_ == 0` check) is trivial, and the complexity we'd add via this additional flag and the out parameter seems higher than it's worth.
How would you feel about dropping the out parameter and this flag, and shifting the `CorsURLLoader` to perform the allowlist check `if (redirect_count_ == 0 && network_restrictions_id_)`?
sg to make it simpler given the trivial cost
bool IsNetworkForNonceAndUrlAllowed(const base::UnguessableToken& nonce,The style guide suggests using return value over output parameters. It would be nice to follow the suggestion. Not a blocker though so feel free to mark this resolved if you disagree.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
N/A since the out param is removed.
Would we need this out parameter if we didn't have the exception list below?
Skimming through the code, it looks like the exception mechanism is only called from `RenderFrameHostImpl::ExemptUrlFromNetworkRevocationForTesting()`, and none of the relevant tests in `NetworkContextTest` seem to deal with redirects. Can we simply apply the same behavior there, and avoid the out-parameter entirely (e.g. all redirected requests are either allowed or blocked as per connection allowlist)? Is it called elsewhere that I missed?
(More broadly: do we need the exception list at all at this point? What pieces of Fenced Frame infrastructure are we keeping?)
Thanks, the network revocation part of of fenced frames is slated for removal and at that point, I agree that returning bool because of
`if (!network_revocation_nonces_.contains(nonce))`
is the same as saying that that connection allowlist is not present to be consulted.
That makes it an easier decision to remove the bool.
const uint32 kURLLoadOptionAllowedByConnectionAllowlist = 1024;Nit: Perhaps rename this to something like `kURLLoadOptionConnectionAllowlistEnforced` (or `...Present` or `...Declared` or something)? Using `AllowedByConnectionAllowlist` to determine whether we're _blocking_ a redirect response due to connection allowlist is a little unclear.
- Kenichi Ishibashi
- 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")
Votes added by Mike West
| Code-Review | +1 |
1 comment
Thanks for taking another pass. This is much simpler. :)
Related details
- Kenichi Ishibashi
- 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/58160.
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
- Kenichi Ishibashi
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
2 comments
lgtm
HandleComplete(URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));optional: Do we have test coverage for this case? If not, it would be nice to have.
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 2 comments![]( "Open in Gerrit")
Thanks both!
HandleComplete(URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));optional: Do we have test coverage for this case? If not, it would be nice to have.
The test third_party/blink/web_tests/external/wpt/connection-allowlist/tentative/fetch-redirect.sub.window.js covers the redirect failure case. Was there something specific you were looking for in the test?
Related details
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
HandleComplete(URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));Shivani Sharmaoptional: Do we have test coverage for this case? If not, it would be nice to have.
The test third_party/blink/web_tests/external/wpt/connection-allowlist/tentative/fetch-redirect.sub.window.js covers the redirect failure case. Was there something specific you were looking for in the test?
Acknowledged, Gerrit implies that this isn't covered by tests. Probably it doesn't recognize WPTs.
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. |