[Connection Allowlist] Add reporting support for navigation. [chromium/src : main]
Mike West (Gerrit)
Mike West added 5 comments![]( "Open in Gerrit")
Hey Shivani! Can you take another pass over this? I think it's in reasonable shape. If you're happy with it, I'll find a //content OWNER to loop in.
Thanks!
const GURL& redirect_url,Mike WestIs this needed at all since we don't need to send this in the report?
Done
bool ConnectionAllowlistAllowsUrlAndReportIfNeeded(Mike WestIt will need changes in FrameConnectionAllowlistAllowsRequestAndReportIfNeeded() as well for consumer APIs like PaymentHandler but ok to be done in a follow up
Acknowledged
const std::optional<base::UnguessableToken>& reporting_source) {Mike WestJust to confirm, the PolicyContainerHost changes in https://chromium-review.git.corp.google.com/c/chromium/src/+/7914765 is not a blocker for this CL, right?
No. That is some cleanup we can do later to avoid some of this ductwork; it's not necessary for this CL, the functionality should work without it.
GURL check_url = redirect_url.value_or(common_params_->url);Mike WestShould we be sending the original URL in the report so that redirect URL is not exfiltrated? I believe this is what happens in https://source.chromium.org/chromium/chromium/src/+/main:services/network/network_context.cc;drc=478121030eb2e39b81d6223f51461f6e3b290351;l=3829
Yes, we should. Thanks for catching this.
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 8 comments![]( "Open in Gerrit")
Looks good. There is one change about using GetInitiatorDocumentRenderFrameHost in navigation_request.cc and rest are all nits.
CONTENT_EXPORT bool IsRedirectAllowedByConnectionAllowlist(ok to be in a follow up:
We can rename this to be IsRedirectAllowedByConnectionAllowlistReportIfNeeded()
// Returns true if the connection allowlist allows redirect.nit: add "If it is blocked, handles reporting and returns false."
bool EnforcesConnectionAllowlist(this function can be removed since its usage is now all replaced with the new HasActiveConnectionAllowlists
if (render_frame_host) {check can be removed since it's already checked in line 245
RenderFrameHostImpl* initiator_rfh = nullptr;
if (initiator_frame_token_.has_value()) {
initiator_rfh = RenderFrameHostImpl::FromFrameToken(
initiator_process_id_, *initiator_frame_token_);
}
Use GetInitiatorDocumentRenderFrameHost() instead to make sure that if the RFH has been assigned to another document, it won't be used.
if (render_frame_host_) {nit: this if can be removed since render_frame_host_ is being dereferenced earlier in this function as well, e.g. line 218
}, 'Connection-Allowlist report is delivered to the reporting endpoint for a blocked navigation redirect.');nit: not a redirect
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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
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 8 comments![]( "Open in Gerrit")
Thanks, Shivani! Since you're happy with the core change: Arthur, could you take a look at //content with your OWNERS hat on?
CONTENT_EXPORT bool IsRedirectAllowedByConnectionAllowlist(ok to be in a follow up:
We can rename this to be IsRedirectAllowedByConnectionAllowlistReportIfNeeded()
I'll do this in a followup, as we might want to debate the name a bit. :)
// Returns true if the connection allowlist allows redirect.nit: add "If it is blocked, handles reporting and returns false."
Done
this function can be removed since its usage is now all replaced with the new HasActiveConnectionAllowlists
Done
check can be removed since it's already checked in line 245
Done
RenderFrameHostImpl* initiator_rfh = nullptr;
if (initiator_frame_token_.has_value()) {
initiator_rfh = RenderFrameHostImpl::FromFrameToken(
initiator_process_id_, *initiator_frame_token_);
}
Use GetInitiatorDocumentRenderFrameHost() instead to make sure that if the RFH has been assigned to another document, it won't be used.
Done
nit: this if can be removed since render_frame_host_ is being dereferenced earlier in this function as well, e.g. line 218
Done
}, 'Connection-Allowlist report is delivered to the reporting endpoint for a blocked navigation redirect.');Mike Westnit: not a redirect
Done
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Message
**[Early Review]** This is an automated early review generated by an LLM. It is intended to help you catch obvious issues early and **potentially save a round of code review**.
If you find any suggestion irrelevant, please feel free to *ignore* or *close* it.
_I am going to take a look immediately._
Please see suggestions below.
1 comment
reporting_source = policies->connection_allowlists.reporting_source;**[Early Review]**
When checking if a redirect is allowed, passing `GetOriginalRequestURL()` to `IsRedirectAllowedByConnectionAllowlist` will cause the violation report to attribute the violation to the original navigation URL instead of the redirect target URL that actually triggered the block. Use `common_params_->url` or `GetURL()` instead.
Related details
- Arthur Sonzogni
- 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")
reporting_source = policies->connection_allowlists.reporting_source;**[Early Review]**
When checking if a redirect is allowed, passing `GetOriginalRequestURL()` to `IsRedirectAllowedByConnectionAllowlist` will cause the violation report to attribute the violation to the original navigation URL instead of the redirect target URL that actually triggered the block. Use `common_params_->url` or `GetURL()` instead.
That's the expected behavior. Since redirect is a global boolean so either all redirects are allowed or not, the exact URL is not relevant. Also, the exact URL could lead to a data leak (similar to how that was an issue with CSP: https://github.com/WICG/connection-allowlists#:~:text=It%20is%20a%20cross%2Dorigin%20data%20leak)
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
reporting_source = policies->connection_allowlists.reporting_source;Shivani Sharma**[Early Review]**
When checking if a redirect is allowed, passing `GetOriginalRequestURL()` to `IsRedirectAllowedByConnectionAllowlist` will cause the violation report to attribute the violation to the original navigation URL instead of the redirect target URL that actually triggered the block. Use `common_params_->url` or `GetURL()` instead.
That's the expected behavior. Since redirect is a global boolean so either all redirects are allowed or not, the exact URL is not relevant. Also, the exact URL could lead to a data leak (similar to how that was an issue with CSP: https://github.com/WICG/connection-allowlists#:~:text=It%20is%20a%20cross%2Dorigin%20data%20leak)
I agree! :)
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |