[Connection Allowlist] Use the correct reporting source. [chromium/src : main]
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
One more in the chain, bashi@. If you have time to take a look, brilliant! If not, let me know and I'll find another reviewer. I'd like to get these in by Monday if possible... :)
shivanisha@: Would you mind taking a look as well?
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. |
Shivani Sharma (Gerrit)
Shivani Sharma voted and added 4 comments![]( "Open in Gerrit")
Votes added by Shivani Sharma
| Code-Review | +1 |
4 comments
lgtm % nits, thanks!
navigation_request.GetRenderFrameHost()->GetReportingSource();nit: Can we add a comment to clarify here that the reporting source of the document being committed should be passed (vs the initiator document)?
const std::optional<base::UnguessableToken>& reporting_source,can this become non-optional once workers also support it?
std::optional<base::UnguessableToken> reporting_source;is the only reason this is optional is because it's not yet supported for workers? Can we add a TODO to make it non-optional once that's done
Related details
- 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
lgtm
auto url_matches_patterns = [&url](const auto& patterns) {Can we rename this to `restriction_allowed()` or something, and takes `const NetworkRestriction&`, capuring `is_direct` to reduce dups below?
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")
navigation_request.GetRenderFrameHost()->GetReportingSource();nit: Can we add a comment to clarify here that the reporting source of the document being committed should be passed (vs the initiator document)?
An excellent suggestion!
const std::optional<base::UnguessableToken>& reporting_source,can this become non-optional once workers also support it?
I think it can, yes, but Workers are going to take some (ha!) work and probably won't make branch.
auto url_matches_patterns = [&url](const auto& patterns) {Can we rename this to `restriction_allowed()` or something, and takes `const NetworkRestriction&`, capuring `is_direct` to reduce dups below?
Done
std::optional<base::UnguessableToken> reporting_source;is the only reason this is optional is because it's not yet supported for workers? Can we add a TODO to make it non-optional once that's done
No. It's optional because we don't have a reporting source when we're parsing the headers. We don't have it until we commit a document, so this is something of a placeholder due to the order in which we're able to do things. I'll add a comment in the mojo definition about the lifecycle challenge.
Related details
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")
Hey Camille! Can you stamp the portion of this change in //content/browser?
Related details
- Camille Lamy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 2 comments![]( "Open in Gerrit")
Thanks! One question below.
allowlists.reporting_source =Just to check, this is only called for the final network response, not server redirects?
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shivani Sharma
| Code-Review | +1 |
1 comment
allowlists.reporting_source =Just to check, this is only called for the final network response, not server redirects?
Yes, this is only called from Navigation throttle::WillProcessResponse() and WillCommitWithoutUrlLoader(), not from WillRedirectRequest()
Camille Lamy (Gerrit)
Camille Lamy added 2 comments![]( "Open in Gerrit")
Thanks! A few questions below.
mojo_base.mojom.UnguessableToken? reporting_source;Is this meant to be a reporting endpoint? In that case, would it make sense to do what COOP, COEP and DIP do and pass the reporting endpoint as a string here?
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
mojo_base.mojom.UnguessableToken? reporting_source;Is this meant to be a reporting endpoint? In that case, would it make sense to do what COOP, COEP and DIP do and pass the reporting endpoint as a string here?
the reporting endpoint string is also sent in the ConnectionAllowlist enforced and report_only fields above. The field `reporting_endpoint` is defined in line 53
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
mojo_base.mojom.UnguessableToken? reporting_source;Shivani SharmaIs this meant to be a reporting endpoint? In that case, would it make sense to do what COOP, COEP and DIP do and pass the reporting endpoint as a string here?
the reporting endpoint string is also sent in the ConnectionAllowlist enforced and report_only fields above. The field `reporting_endpoint` is defined in line 53
The reporting source token allows //net/reporting to bind the report we're trying to send to the document/worker from which it's being sent, so that the reporting endpoint (which //net/reporting calls "group") can be resolved. See https://source.chromium.org/chromium/chromium/src/+/main:net/reporting/reporting_service.h;l=64 for its eventual usage.
Related details
- Camille Lamy
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. |