add external protocol action result code [chromium/src : main]
Joshua Thomas (Gerrit)
Joshua Thomas added 2 comments![]( "Open in Gerrit")
I know the conversation is still being had offline, but wanted to send a small CL before I head home.
navigation_handle()->IsExternalProtocol()) {This easily gets us the external protocol error. If we want the error to be more generally "kSchemeBlocked", then we have to rework how we reject the decision callback
(still shouldn't be too hard)
Related details
- David Bokan
- Robert Ogden
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. |
Robert Ogden (Gerrit)
Robert Ogden voted and added 2 comments![]( "Open in Gerrit")
Votes added by Robert Ogden
| Code-Review | +1 |
2 comments
base::FEATURE_DISABLED_BY_DEFAULT);let's enable by default to have this as a kill switch instead since we're blocking folks' work without this enabled
navigation_handle()->IsExternalProtocol()) {This easily gets us the external protocol error. If we want the error to be more generally "kSchemeBlocked", then we have to rework how we reject the decision callback
(still shouldn't be too hard)
this seems good to me. I think "internal" non-http protocols like file:// and ftp:// can remain blocked under the url status code
Related details
- David Bokan
- Joshua Thomas
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. |
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
navigation_handle()->IsExternalProtocol()) {This is based on common_params->url - do you know if that's updated for HTTP redirects? My guess would be no (but I'm not sure) so this won't be correct in cases where we get an HTTP redirect.
Could we pass in the reason from the policy check? That'd also make sure we're returning the reason that the policy code actually used to reject.
Related details
- Joshua Thomas
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. |
David Bokan (Gerrit)
David Bokan voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
2 comments
looks good - though I'd suggest actually plumbing the reason (via an ActorResultCode) from the policy checker to make sure we're not assuming why it blocked.
navigation_handle()->IsExternalProtocol()) {Robert OgdenThis easily gets us the external protocol error. If we want the error to be more generally "kSchemeBlocked", then we have to rework how we reject the decision callback
(still shouldn't be too hard)
this seems good to me. I think "internal" non-http protocols like file:// and ftp:// can remain blocked under the url status code
+1 - though if you take my suggestion of plumbing the reason from the policy checker you'd need to make this check more granular in that code too (i.e. check IsExternal there). (plumbing the result code also ensures we're also looking at redirected URLs)
Related details
- Joshua Thomas
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. |
Kevin McNee (Gerrit)
Kevin McNee added 1 comment![]( "Open in Gerrit")
navigation_handle()->IsExternalProtocol()) {This is based on common_params->url - do you know if that's updated for HTTP redirects? My guess would be no (but I'm not sure) so this won't be correct in cases where we get an HTTP redirect.
Could we pass in the reason from the policy check? That'd also make sure we're returning the reason that the policy code actually used to reject.
It's the same as NavigationHandle::GetURL which is commented as changing for redirects.
Joshua Thomas (Gerrit)
Joshua Thomas added 3 comments![]( "Open in Gerrit")
let's enable by default to have this as a kill switch instead since we're blocking folks' work without this enabled
Done
navigation_handle()->IsExternalProtocol()) {Robert OgdenThis easily gets us the external protocol error. If we want the error to be more generally "kSchemeBlocked", then we have to rework how we reject the decision callback
(still shouldn't be too hard)
David Bokanthis seems good to me. I think "internal" non-http protocols like file:// and ftp:// can remain blocked under the url status code
+1 - though if you take my suggestion of plumbing the reason from the policy checker you'd need to make this check more granular in that code too (i.e. check IsExternal there). (plumbing the result code also ensures we're also looking at redirected URLs)
Sounds good.
Instead of plumbing the actual result code, I passed back a new `MayActOnUrlBlockReason` since not all callers may choose to apply a ActionResultCode (history tool's validate function) or they may choose to use different codes (EE satety checks use `kUrlBlocked` vs the `kTriggeredBlockedNavigation` for the throttle)
I added block reasons for all of the different ones I saw, but let me know if you'd like some removed or split
Kevin McNeeThis is based on common_params->url - do you know if that's updated for HTTP redirects? My guess would be no (but I'm not sure) so this won't be correct in cases where we get an HTTP redirect.
Could we pass in the reason from the policy check? That'd also make sure we're returning the reason that the policy code actually used to reject.
It's the same as NavigationHandle::GetURL which is commented as changing for redirects.
Ack
Since we're passing back a blocked reason from site_policy.cc which doesn't have the NavigationHandle, I just did the underlying `ProfileIOData::IsHandledURL(url)` call directly on the url.
Related details
- David Bokan
- Kevin McNee
- Robert Ogden
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ke...@chromium.org
Reviewer source(s):
ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Abe Boujane
- David Bokan
- Ken Buchanan
- Kevin McNee
- Robert Ogden
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. |
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
Related details
- Abe Boujane
- David Bokan
- Joshua Thomas
- Kevin McNee
- Robert Ogden
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. |
Abe Boujane (Gerrit)
Abe Boujane voted and added 1 comment![]( "Open in Gerrit")
Votes added by Abe Boujane
| Code-Review | +1 |
1 comment
David Bokan (Gerrit)
David Bokan voted and added 4 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
4 comments
lgtm - wouldn't mind a second set of eyes over the site policy stuff from mcnee@
navigation_handle()->IsExternalProtocol()) {Robert OgdenThis easily gets us the external protocol error. If we want the error to be more generally "kSchemeBlocked", then we have to rework how we reject the decision callback
(still shouldn't be too hard)
David Bokanthis seems good to me. I think "internal" non-http protocols like file:// and ftp:// can remain blocked under the url status code
Joshua Thomas+1 - though if you take my suggestion of plumbing the reason from the policy checker you'd need to make this check more granular in that code too (i.e. check IsExternal there). (plumbing the result code also ensures we're also looking at redirected URLs)
Sounds good.
Instead of plumbing the actual result code, I passed back a new `MayActOnUrlBlockReason` since not all callers may choose to apply a ActionResultCode (history tool's validate function) or they may choose to use different codes (EE satety checks use `kUrlBlocked` vs the `kTriggeredBlockedNavigation` for the throttle)
I added block reasons for all of the different ones I saw, but let me know if you'd like some removed or split
looks good, thanks!
// TODO(crbug.com/448384918): The callback should return the explicit errorSeems we can tag this bug in the commit message
void MayActOnUrl(const GURL& url,This is kept because of usage in history and navigate tools? Please leave a TODO to move those to take a MayActOnUrlBlockReason
Related details
- Joshua Thomas
- Kevin McNee
- Robert Ogden
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. |
Robert Ogden (Gerrit)
Robert Ogden voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Joshua Thomas
- Kevin McNee
Joshua Thomas (Gerrit)
Joshua Thomas added 3 comments![]( "Open in Gerrit")
lgtm - wouldn't mind a second set of eyes over the site policy stuff from mcnee@
Ack, I'll wait for his review before submitting
// TODO(crbug.com/448384918): The callback should return the explicit errorSeems we can tag this bug in the commit message
Done
This is kept because of usage in history and navigate tools? Please leave a TODO to move those to take a MayActOnUrlBlockReason
Right. I filed a bug to cleanup and added the TODO here. Thanks
Related details
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kevin McNee
| Code-Review | +1 |
3 comments
LGTM, thanks.
(No need to address nits if you'd rather submit as is.)
actor::MayActOnUrlBlockReason block_reason);nit: Redundant namespace.
std::move(callback)));Optional nit: I think we could use OnceCallback::Then to make this more concise. (Same on line 353)
Related details
- Joshua Thomas
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. |
Joshua Thomas (Gerrit)
Joshua Thomas added 2 comments![]( "Open in Gerrit")
actor::MayActOnUrlBlockReason block_reason);Joshua Thomasnit: Redundant namespace.
Thanks. Done
Optional nit: I think we could use OnceCallback::Then to make this more concise. (Same on line 353)
Ah nice, 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. |
Joshua Thomas (Gerrit)
Joshua Thomas voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |