Add API to create origins from nonces for sandboxed iframes [chromium/src : main]
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
url::Origin SandboxedOpaqueOriginCreator::CreateOriginForSandboxedFrame(Should we add some unit tests for the new functions?
Related details
- Dave Risney
- Monica Chintala
- Rakina Zata Amni
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. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 6 comments![]( "Open in Gerrit")
Thanks! Let me add @dch...@chromium.org to review as well, since he commented on this work a while back.
url::Origin SandboxedOpaqueOriginCreator::CreateOriginForSandboxedFrame(Should we add some unit tests for the new functions?
+1
// given nonce and URLI wonder if this should be removed, if we will only ever pass in `about:blank`. Are there other cases not covered here? (e.g. does `about:srcdoc` or `data:` URLs have the same problem?)
if (url.IsValid() && !ShouldTreatAsOpaqueOrigin(url)) {Are precursors never opaque? What happens if we pass in an opaque precursor anyways?
if (url.IsValid() && !ShouldTreatAsOpaqueOrigin(url)) {Can we make this a CHECK instead? Or are there actually cases where it's possible to pass an invalid URL here?
// Creates an origin with the given nonce and tuple.Let's add a comment on what the intended use case is (about:blank sandboxed iframes), and how we don't plan to add any more use cases here.
Related details
- Daniel Cheng
- Dave Risney
- Monica Chintala
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
How many places are going to need to call this? It's hard to tell because none of those uses were introduced in this CL, so I can't really tell if the new abstractions/passkeys make sense (as opposed to just using existing methods like `CreateOpaqueFromNormalizedPrecursorTuple()`).
I'm actually not sure how much the passkeys help: while they won't be able to call the creation methods on SecurityOrigin/Origin directly, they can just call the helpers directly instead.
Related details
- Dave Risney
- Monica Chintala
Monica Chintala (Gerrit)
Monica Chintala added 4 comments![]( "Open in Gerrit")
How many places are going to need to call this? It's hard to tell because none of those uses were introduced in this CL, so I can't really tell if the new abstractions/passkeys make sense (as opposed to just using existing methods like `CreateOpaqueFromNormalizedPrecursorTuple()`).
I'm actually not sure how much the passkeys help: while they won't be able to call the creation methods on SecurityOrigin/Origin directly, they can just call the helpers directly instead.
There are only two places where we call it (RenderFrameHostImpl::SetOriginDependentStateOfNewFrame and
DocumentLoader::CalculateOrigin) and the usage is in the next CL's.
This wrapper provides a single point for origin creation with nonces. To prevent misuse, I agree that we need to restrict via PassKey here as well. Added now! What do you think now?
CreateOpaqueFromNormalizedPrecursorTuple is private so cannot be used as an alternative.
// given nonce and URLI wonder if this should be removed, if we will only ever pass in `about:blank`. Are there other cases not covered here? (e.g. does `about:srcdoc` or `data:` URLs have the same problem?)
New opaque origin generation is used by all Sandboxed Iframes (about:blank, about:srcdoc, data:, blob, initial empty etc) so we should keep it.
if (url.IsValid() && !ShouldTreatAsOpaqueOrigin(url)) {Can we make this a CHECK instead? Or are there actually cases where it's possible to pass an invalid URL here?
I think we should keep it conditional. Invalid urls can still happen for opaque origins(when iframe created from sandboxed about:blank iframe). KURL would be null here when we be extract the origin url from the callers like below:
KURL(origin->GetOriginOrPrecursorOriginIfOpaque()
->ToUrlOrigin()
.GetURL())
if (url.IsValid() && !ShouldTreatAsOpaqueOrigin(url)) {Are precursors never opaque? What happens if we pass in an opaque precursor anyways?
Precursors are generally non-opaque. If opaque precursors happen somehow, it creates opaque without precursor as ShouldTreatAsOpaqueOrigin returns true and skips CreateInternal . This code is added for defensive to handle opaque cases if any.
Related details
- Daniel Cheng
- Dave Risney
- Rakina Zata Amni
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. |
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
url::Origin SandboxedOpaqueOriginCreator::CreateOriginForSandboxedFrame(Rakina Zata AmniShould we add some unit tests for the new functions?
+1
- Daniel Cheng
- Dave Risney
- Liang Zhao
- Rakina Zata Amni
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. |