Move IsSandboxed to SecurityPrincipal interface [chromium/src : main]
Charlie Reis (Gerrit)
Charlie Reis voted and added 3 comments![]( "Open in Gerrit")
Votes added by Charlie Reis
| Code-Review | +1 |
3 comments
Thanks! LGTM with a few nits. I think this is a nice simple example of how SecurityPrincipal will be used outside content/, and you may want to consider making this first use instead of the slightly-more-complex StoragePartitionConfig case in https://chromium-review.googlesource.com/c/chromium/src/+/7231185, or the less-obviously-security-related IsGuest case in https://chromium-review.googlesource.com/c/chromium/src/+/7234613.
separation between site identity management (SiteInstance) and security
boundary representation (SecurityPrincipal).This doesn't quite match my interpretation-- the distinction is more about "instance" vs "site/principal." Maybe we can say something like: "instance management within a browsing context group (SiteInstance) and security properties of the principal being isolated (SecurityPrincipal)"?
// sandboxed iframe.Let's use a comment closer to the one that was in SiteInstance (since the "iframe" term isn't meaningful here, and since the process-isolated part is important):
Returns true if this SecurityPrincipal is for process-isolated sandboxed documents only.
Related details
- Alex Moshchuk
- Viktoriya Bryhider
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. |
Charlie Reis (Gerrit)
Charlie Reis voted and added 4 comments![]( "Open in Gerrit")
Votes added by Charlie Reis
| Code-Review | +1 |
4 comments
Thanks! Still LGTM.
(Sorry if I'm jumping the gun before you've replied to the open comments-- usually it helps to reply to those and mark them done to wrap up the discussion. I saw I was back in the attention set, though.)
separation between site identity management (SiteInstance) and security
boundary representation (SecurityPrincipal).This doesn't quite match my interpretation-- the distinction is more about "instance" vs "site/principal." Maybe we can say something like: "instance management within a browsing context group (SiteInstance) and security properties of the principal being isolated (SecurityPrincipal)"?
Thanks!
(SecurityPrincipal)minor nit: End with period.
Let's use a comment closer to the one that was in SiteInstance (since the "iframe" term isn't meaningful here, and since the process-isolated part is important):
Returns true if this SecurityPrincipal is for process-isolated sandboxed documents only.
Thanks!
Related details
- Alex Moshchuk
- Viktoriya Bryhider
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. |
Viktoriya Bryhider (Gerrit)
Viktoriya Bryhider added 1 comment![]( "Open in Gerrit")
if (!has_site_) {I was thinking yesterday that moving IsSandboxed to SecurityPrincipal without has_site_ may introduce behavior change but now cannot come up with use-case when it will happen.
- If site is not set - it will be default SiteInfo - with default is_sandboxed which is `false`
- If it is accesses from SiteInfo object - then it was not checking this flag either and can be `true` even if site is not set in SiteInstance.
Leaving the comment for sanity-check
Related details
- Alex Moshchuk
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao voted Code-Review+1![]( "Open in Gerrit")
- Alex Moshchuk
- Viktoriya Bryhider
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. |
Viktoriya Bryhider (Gerrit)
Viktoriya Bryhider added 1 comment![]( "Open in Gerrit")
minor nit: End with period.
Done
Related details
- Alex Moshchuk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
4 comments
Alex MoshchukThanks! LGTM with a few nits. I think this is a nice simple example of how SecurityPrincipal will be used outside content/, and you may want to consider making this first use instead of the slightly-more-complex StoragePartitionConfig case in https://chromium-review.googlesource.com/c/chromium/src/+/7231185, or the less-obviously-security-related IsGuest case in https://chromium-review.googlesource.com/c/chromium/src/+/7234613.
+1 that this is a nice first use case!
Thanks, LGTM as well with one minor comment.
if (!has_site_) {I was thinking yesterday that moving IsSandboxed to SecurityPrincipal without has_site_ may introduce behavior change but now cannot come up with use-case when it will happen.
- If site is not set - it will be default SiteInfo - with default is_sandboxed which is `false`
- If it is accesses from SiteInfo object - then it was not checking this flag either and can be `true` even if site is not set in SiteInstance.
Leaving the comment for sanity-check
Sounds reasonable to me.
bool HasSandboxedSiteInstance(RenderFrameHost* frame) {I think we can actually clean this up and remove this helper as well; I think this is left over from before we had SiteInstance::IsSandboxed(). The test use cases like `content::HasSandboxedSiteInstance(frame)` can now become `frame->GetSiteInstance()->GetSecurityPrincipal()->IsSandboxed()`. Fell free to do it here or in a separate followup CL.
Related details
- Viktoriya Bryhider
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. |