[Connection Allowlist] First pass at reporting. [chromium/src : main]
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Commit-Queue | +1 |
1 comment
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 2 comments![]( "Open in Gerrit")
}Additionally we also need to support reporting for the navigation checks in NavigationRequest::IsAllowedByConnectionAllowlist()
// Per
// https://wicg.github.io/connection-allowlists/#abstract-opdef-match-a-host-to-a-connection-allowlist,
// we need to match `synthetic_url` against a host-only variant against each
// URLPattern corresponding to `nonce`.we can still retain this comment?
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. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
Additionally we also need to support reporting for the navigation checks in NavigationRequest::IsAllowedByConnectionAllowlist()
Thanks for the pointer, I'd assumed everything ran through this chokepoint! Since this CL is already pretty complicated, I think I'd split the `NavigationRequest` bit out into a separate CL; I'll try to pull that together today.
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. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
bashi@: Part 2 of the CL I just sent you. This is all network service, and I'd appreciate your feedback.
Related details
- 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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 5 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
5 comments
lgtm
Disclaimer: I haven't taken a closer look at tests
bool enforced_allowlist_present = false;Just wondering if using `std::optional<std::set<std::unique_ptr<url_pattern::SimpleUrlPatternMatcher>>>` for `enforced_allowlisted_patterns` makes sense. Probably not since we need to convert to mojo?
const net::NetworkAnonymizationKey* reporting_nak = nullptr);nit, optional: `base::optional_ref<net::NetworkAnonymizationKey>` instead of pointer? Ditto below.
// TODO(): We need a reporting source, which we'll need to thread through likePlease add a bug number.
!std::ranges::any_of(
restriction.report_only_allowlisted_patterns,
[&url](const std::unique_ptr<url_pattern::SimpleUrlPatternMatcher>&
pattern) { return pattern->Match(url); })) {Is it possible to have helper functions (one calls Match(), the other calls HostOnlyMatch()) for this check to reduce the duplication and for readability?
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 6 comments![]( "Open in Gerrit")
Mike WestAdditionally we also need to support reporting for the navigation checks in NavigationRequest::IsAllowedByConnectionAllowlist()
Thanks for the pointer, I'd assumed everything ran through this chokepoint! Since this CL is already pretty complicated, I think I'd split the `NavigationRequest` bit out into a separate CL; I'll try to pull that together today.
Still working on tests, but it'll be in https://chromium-review.googlesource.com/c/chromium/src/+/7567805/
Just wondering if using `std::optional<std::set<std::unique_ptr<url_pattern::SimpleUrlPatternMatcher>>>` for `enforced_allowlisted_patterns` makes sense. Probably not since we need to convert to mojo?
I think we can use `std::optional`. That's probably clearer than carrying the additional boolean.
const net::NetworkAnonymizationKey* reporting_nak = nullptr);nit, optional: `base::optional_ref<net::NetworkAnonymizationKey>` instead of pointer? Ditto below.
This should really be unconditional; the pointer implementation is leftover from a previous attempt that I ended up backing out. :/ Fixing it up! Thanks for calling it out.
// TODO(): We need a reporting source, which we'll need to thread through likePlease add a bug number.
Done
!std::ranges::any_of(
restriction.report_only_allowlisted_patterns,
[&url](const std::unique_ptr<url_pattern::SimpleUrlPatternMatcher>&
pattern) { return pattern->Match(url); })) {Is it possible to have helper functions (one calls Match(), the other calls HostOnlyMatch()) for this check to reduce the duplication and for readability?
I moved the logic into two lambdas. WDYT?
// Per
// https://wicg.github.io/connection-allowlists/#abstract-opdef-match-a-host-to-a-connection-allowlist,
// we need to match `synthetic_url` against a host-only variant against each
// URLPattern corresponding to `nonce`.we can still retain this comment?
- 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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
lgtm
!std::ranges::any_of(
restriction.report_only_allowlisted_patterns,
[&url](const std::unique_ptr<url_pattern::SimpleUrlPatternMatcher>&
pattern) { return pattern->Match(url); })) {Mike WestIs it possible to have helper functions (one calls Match(), the other calls HostOnlyMatch()) for this check to reduce the duplication and for readability?
I moved the logic into two lambdas. WDYT?
- Mike West
- 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 5 comments![]( "Open in Gerrit")
// If `reporting_nak` is non-null, this method will queue a
// "connection-allowlist" report if the check fails (for either enforced or
// report-only lists).nit: To add this to the above function as well.
nit: `reporting_nak` to be replaced with `network_anonymization_key`
// TODO(482728970): We need a reporting source, which we'll need to thread /*enforced=*/false);Could we add tests for these reports: either browser tests or WPTs?
// Then, do the against the enforced allowlist, and return `false` to cancelnit: typo
// Then, do the against the enforced allowlist, and return `false` to cancelnit: typo
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. |
Mike West (Gerrit)
Mike West added 5 comments![]( "Open in Gerrit")
// If `reporting_nak` is non-null, this method will queue a
// "connection-allowlist" report if the check fails (for either enforced or
// report-only lists).nit: To add this to the above function as well.
nit: `reporting_nak` to be replaced with `network_anonymization_key`
Done
// TODO(482728970): We need a reporting source, which we'll need to threadCould we add tests for these reports: either browser tests or WPTs?
Reports won't actually be sent (or delivered to `ReportingObserver` instances) until I pipe the reporting source through to these callsites as per the TODO above. I can verify that the reports are properly queued, but can't do much more until a future CL. (see https://chromium.googlesource.com/chromium/src/+/HEAD/net/reporting/README.md#:~:text=A%20reporting%20source%2C%20used,using%20this%20source%20token. for some marginal context).
// Then, do the against the enforced allowlist, and return `false` to cancelMike Westnit: typo
Done
// Then, do the against the enforced allowlist, and return `false` to cancelMike Westnit: typo
Done
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. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
/*enforced=*/false);Mike WestCould we add tests for these reports: either browser tests or WPTs?
Reports won't actually be sent (or delivered to `ReportingObserver` instances) until I pipe the reporting source through to these callsites as per the TODO above. I can verify that the reports are properly queued, but can't do much more until a future CL. (see https://chromium.googlesource.com/chromium/src/+/HEAD/net/reporting/README.md#:~:text=A%20reporting%20source%2C%20used,using%20this%20source%20token. for some marginal context).
(Started on this in https://chromium-review.googlesource.com/c/chromium/src/+/7572362/ fwiw. Need to rebase and certainly agree that we'll want WPT in place before landing.)
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Shivani Sharma
| Code-Review | +1 |
1 comment
/*enforced=*/false);Mike WestCould we add tests for these reports: either browser tests or WPTs?
Mike WestReports won't actually be sent (or delivered to `ReportingObserver` instances) until I pipe the reporting source through to these callsites as per the TODO above. I can verify that the reports are properly queued, but can't do much more until a future CL. (see https://chromium.googlesource.com/chromium/src/+/HEAD/net/reporting/README.md#:~:text=A%20reporting%20source%2C%20used,using%20this%20source%20token. for some marginal context).
(Started on this in https://chromium-review.googlesource.com/c/chromium/src/+/7572362/ fwiw. Need to rebase and certainly agree that we'll want WPT in place before landing.)
- Mike West
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Shivani Sharma
| Code-Review | +1 |
- 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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
- 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 added 1 comment![]( "Open in Gerrit")
bashi@: Would you mind re-stamping //services/network/cors/cors_url_loader.cc? Thanks!
Related details
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
- 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 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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: services/network/network_context.h
Insertions: 2, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: services/network/network_context_unittest.cc
Insertions: 36, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: services/network/network_context.cc
Insertions: 46, Deletions: 27.
The diff is too large to show. Please review the diff.
```
```
The name of the file: services/network/websocket_factory.cc
Insertions: 14, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
[Connection Allowlist] First pass at reporting.
This CL wires up the `report-only` headers that were introduced in the
first CL in this chain, along with the reporting endpoints for
enforced allowlists.
This is the second CL in a short series which aims to implement
reporting for Connection Allowlist:
1. https://crbug.com/c/7557233 Passing report info during revocation.
2. https://crbug.com/c/7566886 [You are here.]
3. https://crbug.com/c/7567805 Reporting in `NavigationRequest`.
- M services/network/cors/cors_url_loader.cc
- M services/network/cors/cors_url_loader_factory.cc
- M services/network/network_context.cc
- M services/network/network_context.h
- M services/network/network_context_unittest.cc
- M services/network/websocket_factory.cc
Code-Review: +1 by Shivani Sharma, +1 by Kenichi Ishibashi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |