Fix initiator origin for same-document renderer commits [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hello reviewers, thanks in advance for review, please let me know if you want me to address anything, https://issues.chromium.org/issues/40062719 is a non public bug, bit it most likely is fixed with this CL.
Related details
- Alex Moshchuk
- Chromium IPC Reviews
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
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: kin...@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): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Moshchuk
- Dave Tapuska
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 2 comments![]( "Open in Gerrit")
virtual WebSecurityOrigin LastNavigationInitiatorOrigin() const = 0;I don't love "Last" APIs. and would prefer the values be passed directly on the callback. I called out where I think that can happen is that possible?
GetLocalFrameClient().DidFinishSameDocumentNavigation(Can't we just plumb the values in here, instead of storing them?
Related details
- Alex Moshchuk
- Helmut Januschka
- Kinuko Yasuda
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
virtual WebSecurityOrigin LastNavigationInitiatorOrigin() const = 0;I don't love "Last" APIs. and would prefer the values be passed directly on the callback. I called out where I think that can happen is that possible?
Removed the "Last" APIs and plumbed the initiator_frame_token directly through the DidFinishSameDocumentNavigation callback instead.
GetLocalFrameClient().DidFinishSameDocumentNavigation(Can't we just plumb the values in here, instead of storing them?
- Alex Moshchuk
- Dave Tapuska
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
std::optional<LocalFrameToken> initiator_frame_token;Is it possible to capture the local frame token at the time the FrameLoadRequest s created?
That should cause us to always have a frame token if we have an origin window no? Then these don't need to be optional. (some of the tests might need to be updated that call this method directly), I just want to understand the motivation of having the frame token is optional, and I imagine it is around that the origin_window could become detached by the time this method executes, but if you fetch it in the FrameLoadRequest ctor then I think that goes away.
Thoughts?
Related details
- Alex Moshchuk
- Helmut Januschka
- Kinuko Yasuda
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. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 4 comments![]( "Open in Gerrit")
FrameNavigationEntry correct when site isolation is disabled.Can you expand the description to provide more context on the case you're fixing here? Presumably, this is about https://chromium-review.googlesource.com/c/chromium/src/+/5854091/comment/d91e62f1_b92b84d7/, with more context on https://crbug.com/367440964?
std::optional<url::Origin> initiator_origin_for_request =This could probably use a comment about why we need the initiator origin at all on the synchronous commit path, since it's not obvious how that interacts with same-document navigations (which don't change the underlying origin, but which could indeed by initiated by frames from other origins, and the initiator origin needs to eventually propagate to the FrameNavigationEntry).
mojo::ReportBadMessage(Is this always safe? What if we had a frame tree A(B,C), and B navigated C to C#foo, and then B got destroyed before we processed the same-document commit here?
initiator_origin = initiator_frame->GetLastCommittedOrigin();Similarly here, I worry that with A(B,C), B could navigate the sibling C to C#foo and then navigate itself to some other origin, prior to the C#foo commit getting processed.
Related details
- Helmut Januschka
- Kinuko Yasuda
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
FrameNavigationEntry correct when site isolation is disabled.Can you expand the description to provide more context on the case you're fixing here? Presumably, this is about https://chromium-review.googlesource.com/c/chromium/src/+/5854091/comment/d91e62f1_b92b84d7/, with more context on https://crbug.com/367440964?
Done
std::optional<url::Origin> initiator_origin_for_request =This could probably use a comment about why we need the initiator origin at all on the synchronous commit path, since it's not obvious how that interacts with same-document navigations (which don't change the underlying origin, but which could indeed by initiated by frames from other origins, and the initiator origin needs to eventually propagate to the FrameNavigationEntry).
Done. Added a comment explaining that while same-document navigations don't change the document's origin, they can be initiated by cross-origin frames, and the initiator origin needs to propagate to the FrameNavigationEntry for security decisions.
Is this always safe? What if we had a frame tree A(B,C), and B navigated C to C#foo, and then B got destroyed before we processed the same-document commit here?
Done. Handled gracefully by leaving initiator_origin as nullopt when the frame is gone. Added a unit test for this case.
initiator_origin = initiator_frame->GetLastCommittedOrigin();Similarly here, I worry that with A(B,C), B could navigate the sibling C to C#foo and then navigate itself to some other origin, prior to the C#foo commit getting processed.
Done
Is it possible to capture the local frame token at the time the FrameLoadRequest s created?
That should cause us to always have a frame token if we have an origin window no? Then these don't need to be optional. (some of the tests might need to be updated that call this method directly), I just want to understand the motivation of having the frame token is optional, and I imagine it is around that the origin_window could become detached by the time this method executes, but if you fetch it in the FrameLoadRequest ctor then I think that goes away.
Thoughts?
Tried moving the capture to the FrameLoadRequest ctor, but it's shared between same-document and cross-document navigations so that doesn't work cleanly. The token is optional to handle the case where origin_window's frame is gone, though in practice FrameLoadRequest creation and StartNavigation are synchronous on the same stack so there's no real race.
Related details
- Alex Moshchuk
- Dave Tapuska
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |