[Blink/Network] Support origin targeting for secure exemptions [chromium/src : main]
Daniel Cheng (Gerrit)
Daniel Cheng added 6 comments![]( "Open in Gerrit")
While I am an OWNER for everything, it would be best to have a network services owner that's familiar with cookies review this as well. I don't quite know the motivation here; I've mostly left questions about plumbing and such below.
std::set<std::string, std::less<>> secure_origin_cookies_allowed_origins_;I know it's inconsistent, but it seems like this should probably be an `absl::flat_hash_set`.
secure_origin_cookies_allowed_origins_.insert(origin.Serialize());Do we want to only allow tuple origins here?
(I feel like we could potentially get into trouble if we ever inserted an opaque origin into this set–they serialize as "null"–though maybe some other part of the system would make sure this works out OK)
// Origins that unconditionally allow cookies from secure origins.Similar question about opaque origins: do we want to allow them? Filter them? say they're not legal to pass here? something else?
static bool ShouldTreatURLAsFirstPartyWhenTopLevelEmbeddingSecure(I know this is missing a comment, but can we add one to this with the changes?
URLSchemesSet first_party_when_top_level_with_secure_embedded_origins;It's a bit weird to have this here; this thing contains "origins" but everything else is schemes, including the class itself (which is a "scheme registry").
Related details
- Chromium Cookie Reviews
- Duncan Mercer
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")
Reviewer source(s):
dylan...@google.com is from context(chrome/net/cookies/reviewers.gwsq)
Related details
- Duncan Mercer
- Dylan Cutler
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. |
Lily Chen (Gerrit)
Lily Chen added 4 comments![]( "Open in Gerrit")
While I am an OWNER for everything, it would be best to have a network services owner that's familiar with cookies review this as well. I don't quite know the motivation here; I've mostly left questions about plumbing and such below.
I can review for //net/cookies/OWNERS since Duncan and I have chatted about this change. It's a temporary workaround to make the Lens UI work.
const net::SiteForCookies& site_for_cookies) const {This should probably pass the top level origin in addition to SiteForCookies. Then line 210 should be:
```
if (secure_origin_cookies_allowed_schemes_.contains(
top_level_origin.scheme()) && !site_for_cookies.IsNull) {
return true;
}
```
and the secure_origin_cookies_allowed_origins_ can directly check the origin.
(In general, converting a SiteForCookies to an Origin is a pretty cursed thing to do, because they contain different subsets of info and represent different concepts.)
secure_origin_cookies_allowed_origins_.insert(origin.Serialize());Do we want to only allow tuple origins here?
(I feel like we could potentially get into trouble if we ever inserted an opaque origin into this set–they serialize as "null"–though maybe some other part of the system would make sure this works out OK)
+1. I think this should only allow tuple origins, which have a well defined serialization.
// Like RegisterURLSchemeAsFirstPartyWhenTopLevelEmbeddingSecure, but limits
// the exception to a specific URL on the top-level scheme.This shouldn't claim to operate on a per-URL level if it actually operates on origins.
Related details
- Daniel Cheng
- Duncan Mercer
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. |
Duncan Mercer (Gerrit)
Duncan Mercer added 7 comments![]( "Open in Gerrit")
std::set<std::string, std::less<>> secure_origin_cookies_allowed_origins_;I know it's inconsistent, but it seems like this should probably be an `absl::flat_hash_set`.
Done
This should probably pass the top level origin in addition to SiteForCookies. Then line 210 should be:
```
if (secure_origin_cookies_allowed_schemes_.contains(
top_level_origin.scheme()) && !site_for_cookies.IsNull) {
return true;
}
```and the secure_origin_cookies_allowed_origins_ can directly check the origin.
(In general, converting a SiteForCookies to an Origin is a pretty cursed thing to do, because they contain different subsets of info and represent different concepts.)
Done
secure_origin_cookies_allowed_origins_.insert(origin.Serialize());Lily ChenDo we want to only allow tuple origins here?
(I feel like we could potentially get into trouble if we ever inserted an opaque origin into this set–they serialize as "null"–though maybe some other part of the system would make sure this works out OK)
+1. I think this should only allow tuple origins, which have a well defined serialization.
Done
// Origins that unconditionally allow cookies from secure origins.Similar question about opaque origins: do we want to allow them? Filter them? say they're not legal to pass here? something else?
Added a comment. I added logic to ignore them.
// Like RegisterURLSchemeAsFirstPartyWhenTopLevelEmbeddingSecure, but limits
// the exception to a specific URL on the top-level scheme.This shouldn't claim to operate on a per-URL level if it actually operates on origins.
I clarified the comment. Please lmk if you wanted me to change the function or parameters as well.
static bool ShouldTreatURLAsFirstPartyWhenTopLevelEmbeddingSecure(I know this is missing a comment, but can we add one to this with the changes?
Done
URLSchemesSet first_party_when_top_level_with_secure_embedded_origins;It's a bit weird to have this here; this thing contains "origins" but everything else is schemes, including the class itself (which is a "scheme registry").
Ah, true. I think theres two options that we could do instead:
1) Keep it in SchemeRegistry. This would allow us to reuse the existing plumbing since this is a temporary exception that should be cleaned up by end of Q2 once Lens moves completely to <webview>.
2) Create a new OriginRegistry. Define a new singleton in for origin-based policy overrides very similar to this scheme one. It might be a bit overkill for this temporary exception though.
Lmk which you think is best
Related details
- Daniel Cheng
- Lily Chen
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. |
Lily Chen (Gerrit)
Lily Chen added 6 comments![]( "Open in Gerrit")
This change updates WebSecurityPolicy to support origin targeting for secure embedder exceptions by allowing specific scheme and host pairs instead of applying globally to a scheme. It also migrates secure cookie exemptions in CookieManager and CookieSettings from scheme/host-based registration to url::Origin-based registration, enabling precise unscoped exemptions for secure origins like chrome-untrusted://lens.Can you specify that this affects exemptions for third-party cookie blocking and SameSite cookies (as opposed to affecting the cookie origin itself, for example)? It's currently unclear from the context provided in this description.
This change updates WebSecurityPolicy to support origin targeting for secure embedder exceptions by allowing specific scheme and host pairs instead of applying globally to a scheme. It also migrates secure cookie exemptions in CookieManager and CookieSettings from scheme/host-based registration to url::Origin-based registration, enabling precise unscoped exemptions for secure origins like chrome-untrusted://lens.nit: wrap description to 72 chars.
return top_level_origin.scheme() == kChromeUIScheme &&This should also check !site_for_cookies.IsNull(), to avoid changing behavior.
// This currently returns true if the `site_for_cookies` is a Chrome UI scheme
// URL and the `url` is secure.nit: update comment
bool CookieAccessDelegateImpl::ShouldIgnoreSameSiteRestrictions(This really should take the top_level_origin as well. (Making an origin out of site_for_cookies.RepresentativeUrl() is not really a sensible thing to do.)
Looks like both of the existing call sites have access to an IsolationInfo, from which you can get the top_level_origin.
void set_secure_origin_cookies_allowed_origins(nit: add a comment that opaque origins are skipped.
Related details
- Daniel Cheng
- Duncan Mercer
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. |
Duncan Mercer (Gerrit)
Duncan Mercer added 7 comments![]( "Open in Gerrit")
This change updates WebSecurityPolicy to support origin targeting for secure embedder exceptions by allowing specific scheme and host pairs instead of applying globally to a scheme. It also migrates secure cookie exemptions in CookieManager and CookieSettings from scheme/host-based registration to url::Origin-based registration, enabling precise unscoped exemptions for secure origins like chrome-untrusted://lens.Can you specify that this affects exemptions for third-party cookie blocking and SameSite cookies (as opposed to affecting the cookie origin itself, for example)? It's currently unclear from the context provided in this description.
Done
This change updates WebSecurityPolicy to support origin targeting for secure embedder exceptions by allowing specific scheme and host pairs instead of applying globally to a scheme. It also migrates secure cookie exemptions in CookieManager and CookieSettings from scheme/host-based registration to url::Origin-based registration, enabling precise unscoped exemptions for secure origins like chrome-untrusted://lens.nit: wrap description to 72 chars.
Done
This should also check !site_for_cookies.IsNull(), to avoid changing behavior.
Done
// This currently returns true if the `site_for_cookies` is a Chrome UI scheme
// URL and the `url` is secure.Duncan Mercernit: update comment
Done
bool CookieAccessDelegateImpl::ShouldIgnoreSameSiteRestrictions(This really should take the top_level_origin as well. (Making an origin out of site_for_cookies.RepresentativeUrl() is not really a sensible thing to do.)
Looks like both of the existing call sites have access to an IsolationInfo, from which you can get the top_level_origin.
Done
nit: add a comment that opaque origins are skipped.
Done
URLSchemesSet first_party_when_top_level_with_secure_embedded_origins;Duncan MercerIt's a bit weird to have this here; this thing contains "origins" but everything else is schemes, including the class itself (which is a "scheme registry").
Ah, true. I think theres two options that we could do instead:
1) Keep it in SchemeRegistry. This would allow us to reuse the existing plumbing since this is a temporary exception that should be cleaned up by end of Q2 once Lens moves completely to <webview>.
2) Create a new OriginRegistry. Define a new singleton in for origin-based policy overrides very similar to this scheme one. It might be a bit overkill for this temporary exception though.Lmk which you think is best
Talked offline. Given the temporary nature, will stick with option 1. However, using base::PassKey to avoid new callers and left a todo to cleanup.
Related details
- Daniel Cheng
- Lily Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |