[Connection-Allowlist] Non-optional network_restrictions_id [chromium/src : main]
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
PTAL at the overall change, thanks!
Related details
- Andrew Verge
- Xiaochen Zhou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 7 comments![]( "Open in Gerrit")
/*network_restrictions_id=*/base::UnguessableToken(),Should this be GetTestNetworkRestrictionsId?
network_restrictions_id_(base::UnguessableToken()) {
TRACE_EVENT("navigation", "NavigationRequest::NavigationRequest",Please fix this WARNING reported by ClangTidy: check: readability-redundant-member-init
initializer for member 'network_restri...
check: readability-redundant-member-init
initializer for member 'network_restrictions_id_' is redundant (https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: readability-redundant-member-init` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel` or `mac-clang-tidy-rel`)
if (navigation_request->IsSameDocument()) {In this case, should we set the ID on the request to be the ID of the current document? If we're trying to make the ID non-optional, I do think it's beneficial to have it populated with the "right" value for consistency, in places where we can predict what that value is.
/*network_restrictions_id=*/network::GetTODONetworkRestrictionsId()),Note that here and below, the current Service Worker CL I have up for review will implement these.
// required by the interface but has not been implemented yet.Maybe add a comment here that we'll clean up this function eventually, unlike the other two.
mojo_base.mojom.UnguessableToken network_restrictions_id);Not blocking for this CL, but now that this is not optional, we should consider moving this higher up in the arguments list given its importance (I got this feedback on one of my Websocket/Webtransport CLs this week.)
TEST_F(WebSocketFactoryTest, CreateWebSocketBypassWithNoOpRestrictionsID) {Do we need these tests? Seems a little out of place to test the NoOp ID just in this one spot.
Related details
- Shivani Sharma
- Xiaochen Zhou
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. |
Xiaochen Zhou (Gerrit)
Xiaochen Zhou added 2 comments![]( "Open in Gerrit")
if (!IsNetworkForNetworkRestrictionsIdAndUrlAllowed(network_restrictions_id,Non-blocking comment: In IsNetworkForNetworkRestrictionsIdAndUrlAllowed, perhaps consider early return if the the id == GetNoOpNetworkRestrictionsId(), so we can avoid a map lookup.
const base::UnguessableToken& GetNoOpNetworkRestrictionsId();We would like to require developers to choose one of the followings for network restriction id param:
- The context's ID.
- Or use one of the 3 functions here.
But passing a `base::UnguessableToken::Null()` or `base::UnguessableToken{}` still compiles. Developers not aware of CA perhaps think this is reasonable.
Perhaps we can have a wrapper class around base::UnguessableToken that disables the default constructor (marking it private). And it offers factory methods like these 3 and force developer to choose one.
A lot more work is required though for adding the wrapper class.
Related details
- Shivani Sharma
Shivani Sharma (Gerrit)
Shivani Sharma added 9 comments![]( "Open in Gerrit")
/*network_restrictions_id=*/base::UnguessableToken(),Shivani SharmaShould this be GetTestNetworkRestrictionsId?
Done
network_restrictions_id_(base::UnguessableToken()) {
TRACE_EVENT("navigation", "NavigationRequest::NavigationRequest",Please fix this WARNING reported by ClangTidy: check: readability-redundant-member-init
initializer for member 'network_restri...
check: readability-redundant-member-init
initializer for member 'network_restrictions_id_' is redundant (https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: readability-redundant-member-init` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel` or `mac-clang-tidy-rel`)
Done
In this case, should we set the ID on the request to be the ID of the current document? If we're trying to make the ID non-optional, I do think it's beneficial to have it populated with the "right" value for consistency, in places where we can predict what that value is.
Done
/*network_restrictions_id=*/network::GetTODONetworkRestrictionsId()),Note that here and below, the current Service Worker CL I have up for review will implement these.
ack
if (!IsNetworkForNetworkRestrictionsIdAndUrlAllowed(network_restrictions_id,Non-blocking comment: In IsNetworkForNetworkRestrictionsIdAndUrlAllowed, perhaps consider early return if the the id == GetNoOpNetworkRestrictionsId(), so we can avoid a map lookup.
// required by the interface but has not been implemented yet.Maybe add a comment here that we'll clean up this function eventually, unlike the other two.
Done
const base::UnguessableToken& GetNoOpNetworkRestrictionsId();We would like to require developers to choose one of the followings for network restriction id param:
- The context's ID.
- Or use one of the 3 functions here.
But passing a `base::UnguessableToken::Null()` or `base::UnguessableToken{}` still compiles. Developers not aware of CA perhaps think this is reasonable.
Perhaps we can have a wrapper class around base::UnguessableToken that disables the default constructor (marking it private). And it offers factory methods like these 3 and force developer to choose one.
A lot more work is required though for adding the wrapper class.
Created 520464337 and added a TODO here. I think it will be a nice addition but ok to be unblocking for launch.
mojo_base.mojom.UnguessableToken network_restrictions_id);Not blocking for this CL, but now that this is not optional, we should consider moving this higher up in the arguments list given its importance (I got this feedback on one of my Websocket/Webtransport CLs this week.)
ack
TEST_F(WebSocketFactoryTest, CreateWebSocketBypassWithNoOpRestrictionsID) {Do we need these tests? Seems a little out of place to test the NoOp ID just in this one spot.
- Andrew Verge
- Xiaochen Zhou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaochen Zhou (Gerrit)
Xiaochen Zhou voted Code-Review+1![]( "Open in Gerrit")
- Andrew Verge
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Shivani Sharma
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
Thanks!
bashi@: PTAL at services/network*, thanks!
alexmos@: PTAL at content/browser*, thanks!
Related details
- Alex Moshchuk
- Kenichi Ishibashi
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 1 comment![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
1 comment
Related details
- 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. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 5 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
5 comments
Thanks, content/ LGTM with a few notes.
#include "services/network/public/cpp/constants.h"It seems that this is included and used in both the browser process code and network service process code. Does that mean that the browser process and NS process will have differing values for network::GetNoOpNetworkRestrictionsId(), etc, since we will create a unique token in each process? Is that ok?
base::optional_ref<base::UnguessableToken> network_restrictions_id)Would this also need to be updated to remove the optional at some point?
return base::UnguessableToken::Null();General note: it's invalid to send an empty UnguessableToken over Mojo - [see here](https://source.chromium.org/chromium/chromium/src/+/main:base/unguessable_token.h;l=47-50;drc=cdc524b176956e1aa3d4bb4e3b4d7b156b6cbaa9). I wonder if that can cause any issues if this is read for an initial empty document of a brand new tab, or something where we don't otherwise create or inherit the network restrictions ID. I guess that document is unlikely to be scripted by another frame to do a network request that would need this, but it could potentially happen in a corner case via extension content scripts or DevTools.
So I wonder if it should be invalid to return a null token here.
/*network_restrictions_id=*/base::UnguessableToken());GetTestNetworkRestrictionsId()?
Related details
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Shivani Sharma (Gerrit)
Shivani Sharma added 4 comments![]( "Open in Gerrit")
#include "services/network/public/cpp/constants.h"It seems that this is included and used in both the browser process code and network service process code. Does that mean that the browser process and NS process will have differing values for network::GetNoOpNetworkRestrictionsId(), etc, since we will create a unique token in each process? Is that ok?
That's a great point.
The Test and TODO constants are ok to be in both processes with different values.
The NoOp constant should only be necessary for populating the id in the browser process and network service should be agnostic to the value. If we tried to do early return based on NoOp before checking the network_restriction_ids_ map in the n/w service then it wouldn't really be working on the right NoOp value and other than that there is no use for it in the n/w service. Moving it to the content/browser layer makes sense but some of the layers like device/ cannot depend on content/ so instead I am adding a dcheck for it to only be invoked in the browser process.
base::optional_ref<base::UnguessableToken> network_restrictions_id)Would this also need to be updated to remove the optional at some point?
Done
return base::UnguessableToken::Null();General note: it's invalid to send an empty UnguessableToken over Mojo - [see here](https://source.chromium.org/chromium/chromium/src/+/main:base/unguessable_token.h;l=47-50;drc=cdc524b176956e1aa3d4bb4e3b4d7b156b6cbaa9). I wonder if that can cause any issues if this is read for an initial empty document of a brand new tab, or something where we don't otherwise create or inherit the network restrictions ID. I guess that document is unlikely to be scripted by another frame to do a network request that would need this, but it could potentially happen in a corner case via extension content scripts or DevTools.
So I wonder if it should be invalid to return a null token here.
Thanks! I was adding the changes to always have this be initialized to no-op id in the follow up CL, but actually moving these changes here so this CL is safe. Also added DCHECKs for id to be non-empty before the network context mojo calls are called.
/*network_restrictions_id=*/base::UnguessableToken());Shivani SharmaGetTestNetworkRestrictionsId()?
Done
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. |
Xiaochen Zhou (Gerrit)
Xiaochen Zhou voted and added 2 comments![]( "Open in Gerrit")
Votes added by Xiaochen Zhou
| Code-Review | +1 |
2 comments
* **Selecting the ID at Calling Sites**:It seems the note about `GetTODONetworkRestrictionsId()` has been removed, is it intentional?
Related details
- Alex Moshchuk
- Andrew Verge
- Kenichi Ishibashi
- 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")
* **Selecting the ID at Calling Sites**:It seems the note about `GetTODONetworkRestrictionsId()` has been removed, is it intentional?
Yes I am thinking that TODO will be removed soon after CA impl completion so doesn't really need to be in the doc.
Related details
- Alex Moshchuk
- Andrew Verge
- Kenichi Ishibashi
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 5 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
5 comments
#include "services/network/public/cpp/constants.h"Shivani SharmaIt seems that this is included and used in both the browser process code and network service process code. Does that mean that the browser process and NS process will have differing values for network::GetNoOpNetworkRestrictionsId(), etc, since we will create a unique token in each process? Is that ok?
That's a great point.
The Test and TODO constants are ok to be in both processes with different values.The NoOp constant should only be necessary for populating the id in the browser process and network service should be agnostic to the value. If we tried to do early return based on NoOp before checking the network_restriction_ids_ map in the n/w service then it wouldn't really be working on the right NoOp value and other than that there is no use for it in the n/w service. Moving it to the content/browser layer makes sense but some of the layers like device/ cannot depend on content/ so instead I am adding a dcheck for it to only be invoked in the browser process.
Ack. I'm not sure if there's a better way to check for the browser process from the network service code (I suppose we can't access switches::kProcessType :/); I'll leave that part to network service reviewers.
I noticed that this will probably come up for Xiaochen's suggestion in https://chromium-review.git.corp.google.com/c/chromium/src/+/7899247/comment/a32c8401_91fbd03f/ to return early for NoOp case. I agree that a cleaner way forward would be with some sort of wrapper or union type (as crbug.com/520464337 suggests), where the bypass_allowlist case is more explicit, or sending the NoOp token to the network service if it needs to be in sync there.
DCHECK(!network_restrictions_id.is_empty());nit: prefer CHECK in new code
DCHECK(!network_restrictions_id.is_empty());(CHECKs here as well)
return base::UnguessableToken::Null();Shivani SharmaGeneral note: it's invalid to send an empty UnguessableToken over Mojo - [see here](https://source.chromium.org/chromium/chromium/src/+/main:base/unguessable_token.h;l=47-50;drc=cdc524b176956e1aa3d4bb4e3b4d7b156b6cbaa9). I wonder if that can cause any issues if this is read for an initial empty document of a brand new tab, or something where we don't otherwise create or inherit the network restrictions ID. I guess that document is unlikely to be scripted by another frame to do a network request that would need this, but it could potentially happen in a corner case via extension content scripts or DevTools.
So I wonder if it should be invalid to return a null token here.
Thanks! I was adding the changes to always have this be initialized to no-op id in the follow up CL, but actually moving these changes here so this CL is safe. Also added DCHECKs for id to be non-empty before the network context mojo calls are called.
Acknowledged
DCHECKBrowserProcess();s/DCHECK/CHECK/g here as well, and it should also probably have a comment about why this is only safe to call from the browser process (i.e., covering the shared token discussion and maybe a TODO pointing to the bug you filed).
Related details
- Andrew Verge
- Kenichi Ishibashi
Shivani Sharma (Gerrit)
Shivani Sharma added 4 comments![]( "Open in Gerrit")
#include "services/network/public/cpp/constants.h"Shivani SharmaIt seems that this is included and used in both the browser process code and network service process code. Does that mean that the browser process and NS process will have differing values for network::GetNoOpNetworkRestrictionsId(), etc, since we will create a unique token in each process? Is that ok?
Alex MoshchukThat's a great point.
The Test and TODO constants are ok to be in both processes with different values.The NoOp constant should only be necessary for populating the id in the browser process and network service should be agnostic to the value. If we tried to do early return based on NoOp before checking the network_restriction_ids_ map in the n/w service then it wouldn't really be working on the right NoOp value and other than that there is no use for it in the n/w service. Moving it to the content/browser layer makes sense but some of the layers like device/ cannot depend on content/ so instead I am adding a dcheck for it to only be invoked in the browser process.
Ack. I'm not sure if there's a better way to check for the browser process from the network service code (I suppose we can't access switches::kProcessType :/); I'll leave that part to network service reviewers.
I noticed that this will probably come up for Xiaochen's suggestion in https://chromium-review.git.corp.google.com/c/chromium/src/+/7899247/comment/a32c8401_91fbd03f/ to return early for NoOp case. I agree that a cleaner way forward would be with some sort of wrapper or union type (as crbug.com/520464337 suggests), where the bypass_allowlist case is more explicit, or sending the NoOp token to the network service if it needs to be in sync there.
Acknowledged
DCHECK(!network_restrictions_id.is_empty());nit: prefer CHECK in new code
Done but with NotFatalUntil for M165 (over 6 months from now) to be on the safe side. Hope that's fine.
DCHECK(!network_restrictions_id.is_empty());Shivani Sharma(CHECKs here as well)
Done
s/DCHECK/CHECK/g here as well, and it should also probably have a comment about why this is only safe to call from the browser process (i.e., covering the shared token discussion and maybe a TODO pointing to the bug you filed).
- Alex Moshchuk
- Andrew Verge
- 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. |
Andrew Verge (Gerrit)
Andrew Verge voted Code-Review+1![]( "Open in Gerrit")
- Alex Moshchuk
- Kenichi Ishibashi
- Shivani Sharma
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alex Moshchuk
- Shivani Sharma