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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
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.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
std::optional<LocalFrameToken> initiator_frame_token;Helmut JanuschkaIs 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.
Why didn't it work cleanly? I'd assume it is the same as capturing the WorldId here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/frame_load_request.cc;l=94;drc=44c3d61b466d13e43fd307e40b0c64d95be1e793;bpv=1;bpt=1
It would just be initiator_local_frame_token_ = origin_window->GetFrame()->GetLocalFrameToken();
Yes it would have to be optional on the FrameLoadRequest, but then you can CHECK that it has a value in the FrameLoader...
Right now the condition on the mojom saying if it is a renderer initiated or not isn't validate because there is the case where the frame went away...
Even in a same stack, a frame can disappear if something makes a call to execute javascript.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
std::optional<LocalFrameToken> initiator_frame_token;Helmut JanuschkaIs 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?
Dave TapuskaTried 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.
Why didn't it work cleanly? I'd assume it is the same as capturing the WorldId here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/frame_load_request.cc;l=94;drc=44c3d61b466d13e43fd307e40b0c64d95be1e793;bpv=1;bpt=1
It would just be initiator_local_frame_token_ = origin_window->GetFrame()->GetLocalFrameToken();
Yes it would have to be optional on the FrameLoadRequest, but then you can CHECK that it has a value in the FrameLoader...
Right now the condition on the mojom saying if it is a renderer initiated or not isn't validate because there is the case where the frame went away...
Even in a same stack, a frame can disappear if something makes a call to execute javascript.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ping!, rebased and conflict solved, all discussions are resolved is there anything i can do now?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Keep these two fields in sync. BeginNavigation() DCHECKs thatThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Keep these two fields in sync. BeginNavigation() DCHECKs thatThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
nly create this when `origin_window->GetFrame()` exists. moved it into `FrameLoadRequest` so `initiator_frame_token` and the keep-alive stay in sync (required by the `BeginNavigation()` DCHECK) once the token is captured early for same-document correctness; for `BeginNavigation` paths this is not extra work, because that code already issued a keep-alive when missing.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ping
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Keep these two fields in sync. BeginNavigation() DCHECKs thatHelmut JanuschkaThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
nly create this when `origin_window->GetFrame()` exists. moved it into `FrameLoadRequest` so `initiator_frame_token` and the keep-alive stay in sync (required by the `BeginNavigation()` DCHECK) once the token is captured early for same-document correctness; for `BeginNavigation` paths this is not extra work, because that code already issued a keep-alive when missing.
This still seems odd to me. Adding toyoshim@ to review this.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
initiator frame token to the browser process. This meant the browser
could not determine the true initiator origin for these navigations,
and instead fell back to using the navigating frame's own origin.Sorry, I'm confused here. So what is the practical example case of the same-document, but cross-origin navigation? I think the same-document implies the same-origin navigation, and the initiator's origin should be locked in the frame so that the browser process can ensure the frame is correctly bound to a permitted origin, and it is set to the initiator. But I may miss something.
Bug: 367440964, 40062719Can you add me to the cc list in the bug? I want to check the context of this CL
// Keep these two fields in sync. BeginNavigation() DCHECKs thatHelmut JanuschkaThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
Dave Tapuskanly create this when `origin_window->GetFrame()` exists. moved it into `FrameLoadRequest` so `initiator_frame_token` and the keep-alive stay in sync (required by the `BeginNavigation()` DCHECK) once the token is captured early for same-document correctness; for `BeginNavigation` paths this is not extra work, because that code already issued a keep-alive when missing.
This still seems odd to me. Adding toyoshim@ to review this.
I'm trying to know more context about this change now, but first of all, can you clarify what you are changing from the viewpoint of the behavior, why such change is necessary, and is it aligned with the web standard? If so, which part is the relevant spec, and why existing wpt is not failing on the scenario, or... just missing it?
// Keep these two fields in sync. BeginNavigation() DCHECKs thatHelmut JanuschkaThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
Dave Tapuskanly create this when `origin_window->GetFrame()` exists. moved it into `FrameLoadRequest` so `initiator_frame_token` and the keep-alive stay in sync (required by the `BeginNavigation()` DCHECK) once the token is captured early for same-document correctness; for `BeginNavigation` paths this is not extra work, because that code already issued a keep-alive when missing.
Takashi ToyoshimaThis still seems odd to me. Adding toyoshim@ to review this.
I'm trying to know more context about this change now, but first of all, can you clarify what you are changing from the viewpoint of the behavior, why such change is necessary, and is it aligned with the web standard? If so, which part is the relevant spec, and why existing wpt is not failing on the scenario, or... just missing it?
I suspect there isn't WPT coverage for this, or if there is that test isn't run with a configuration where the difference in behavior is visible (for instance, when site isolation is disabled). I used Gemini to create a CL with a WPT that surfaces some web-visible differences in behavior that occurs when site isolation is disabled: https://chromium-review.googlesource.com/c/chromium/src/+/7638790 (ignore the trybot failures there, what's important is that both virtual test suite runs pass, confirming the differences).
Using that CL I confirmed that this change aligns the site-isolation-disabled behavior with Chrome's site-isolation-enabled behavior, and at least w.r.t. the sec-fetch-site header this aligns with behavior in Firefox. Firefox is different than Chrome in how it sets the referer header in the case that the WPT tests, but that's a separate issue.
initiator frame token to the browser process. This meant the browser
could not determine the true initiator origin for these navigations,
and instead fell back to using the navigating frame's own origin.Sorry, I'm confused here. So what is the practical example case of the same-document, but cross-origin navigation? I think the same-document implies the same-origin navigation, and the initiator's origin should be locked in the frame so that the browser process can ensure the frame is correctly bound to a permitted origin, and it is set to the initiator. But I may miss something.
same-document navigation does not change the target frame's origin.
Here, "cross-origin" refers to the initiator frame, not the destination URL change. A practical case is: frame A updates frame C to `C#foo` (for example via `iframe.src += "#foo"`). For frame C this is still a same-document navigation (fragment-only), but it is cross-origin-initiated by A.
The bug was that for synchronous same-document commits we were not propagating the initiator frame token, so it could not recover A's origin and fell back to C's own origin when filling `initiator_origin`.
Bug: 367440964, 40062719Can you add me to the cc list in the bug? I want to check the context of this CL
done, about http://crbug.com/40062719 i cant add you, its google internal.
Bug: 367440964, 40062719Helmut JanuschkaCan you add me to the cc list in the bug? I want to check the context of this CL
done, about http://crbug.com/40062719 i cant add you, its google internal.
Hum... still my access to 40062719 is denied.
Can you double check if my address is correctly registered to the cc?
Bug: 367440964, 40062719Helmut JanuschkaCan you add me to the cc list in the bug? I want to check the context of this CL
Takashi Toyoshimadone, about http://crbug.com/40062719 i cant add you, its google internal.
Hum... still my access to 40062719 is denied.
Can you double check if my address is correctly registered to the cc?
https://issues.chromium.org/issues/367440964
copy pasted it from here `toyo...@chromium.org` you are in CC, now also added you to collabs. i might not be allowed todo it? (as i am an external?)
// Keep these two fields in sync. BeginNavigation() DCHECKs thatHelmut JanuschkaThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
Dave Tapuskanly create this when `origin_window->GetFrame()` exists. moved it into `FrameLoadRequest` so `initiator_frame_token` and the keep-alive stay in sync (required by the `BeginNavigation()` DCHECK) once the token is captured early for same-document correctness; for `BeginNavigation` paths this is not extra work, because that code already issued a keep-alive when missing.
Takashi ToyoshimaThis still seems odd to me. Adding toyoshim@ to review this.
Andrew WilliamsI'm trying to know more context about this change now, but first of all, can you clarify what you are changing from the viewpoint of the behavior, why such change is necessary, and is it aligned with the web standard? If so, which part is the relevant spec, and why existing wpt is not failing on the scenario, or... just missing it?
I suspect there isn't WPT coverage for this, or if there is that test isn't run with a configuration where the difference in behavior is visible (for instance, when site isolation is disabled). I used Gemini to create a CL with a WPT that surfaces some web-visible differences in behavior that occurs when site isolation is disabled: https://chromium-review.googlesource.com/c/chromium/src/+/7638790 (ignore the trybot failures there, what's important is that both virtual test suite runs pass, confirming the differences).
Using that CL I confirmed that this change aligns the site-isolation-disabled behavior with Chrome's site-isolation-enabled behavior, and at least w.r.t. the sec-fetch-site header this aligns with behavior in Firefox. Firefox is different than Chrome in how it sets the referer header in the case that the WPT tests, but that's a separate issue.
Regarding the DCHECK that was introduced in [1], I think the original intention is to ensure if the initiator frame is kept alive while we send the token asynchronously. But in your case, navigation is handled in the renderer side as for a same-document navigation, and you send this token back to the browser process on commit notification (== StartNavigation) timing. This request disappears immediately after we return from the StartNavigation. So, when the token arrives in the browser process, this request instance is already destroyed. So, even if you keep the handle here, the token could be invalid when it arrives in the browser process. Why don't we send the origin directly? Maybe security matters as browser does not want to trust renderer sending origin. So another approach is to make this request routes to the browser as we do for Desktop, Site Isolation enabled path? It causes a performance loss, but will be an expected trade-off here.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2648489
// Keep these two fields in sync. BeginNavigation() DCHECKs thatHelmut JanuschkaThis seems odd. Why would we issue a keey alive for every frame load request. Can you explain why you'd need this?
Dave Tapuskanly create this when `origin_window->GetFrame()` exists. moved it into `FrameLoadRequest` so `initiator_frame_token` and the keep-alive stay in sync (required by the `BeginNavigation()` DCHECK) once the token is captured early for same-document correctness; for `BeginNavigation` paths this is not extra work, because that code already issued a keep-alive when missing.
Takashi ToyoshimaThis still seems odd to me. Adding toyoshim@ to review this.
Andrew WilliamsI'm trying to know more context about this change now, but first of all, can you clarify what you are changing from the viewpoint of the behavior, why such change is necessary, and is it aligned with the web standard? If so, which part is the relevant spec, and why existing wpt is not failing on the scenario, or... just missing it?
Takashi ToyoshimaI suspect there isn't WPT coverage for this, or if there is that test isn't run with a configuration where the difference in behavior is visible (for instance, when site isolation is disabled). I used Gemini to create a CL with a WPT that surfaces some web-visible differences in behavior that occurs when site isolation is disabled: https://chromium-review.googlesource.com/c/chromium/src/+/7638790 (ignore the trybot failures there, what's important is that both virtual test suite runs pass, confirming the differences).
Using that CL I confirmed that this change aligns the site-isolation-disabled behavior with Chrome's site-isolation-enabled behavior, and at least w.r.t. the sec-fetch-site header this aligns with behavior in Firefox. Firefox is different than Chrome in how it sets the referer header in the case that the WPT tests, but that's a separate issue.
Regarding the DCHECK that was introduced in [1], I think the original intention is to ensure if the initiator frame is kept alive while we send the token asynchronously. But in your case, navigation is handled in the renderer side as for a same-document navigation, and you send this token back to the browser process on commit notification (== StartNavigation) timing. This request disappears immediately after we return from the StartNavigation. So, when the token arrives in the browser process, this request instance is already destroyed. So, even if you keep the handle here, the token could be invalid when it arrives in the browser process. Why don't we send the origin directly? Maybe security matters as browser does not want to trust renderer sending origin. So another approach is to make this request routes to the browser as we do for Desktop, Site Isolation enabled path? It causes a performance loss, but will be an expected trade-off here.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2648489
removed the eager keep-alive creation from `FrameLoadRequest` and now only capture `initiator_frame_token` there.
I moved keep-alive issuance to the IPC boundary (in `LocalFrameClientImpl::BeginNavigation` and `RemoteFrame::Navigate`), where it actually protects async browser-routed navigation. This avoids creating a keep-alive for every `FrameLoadRequest`.
For same-document commits, behavior remains token-based (no extra keep-alive from `FrameLoadRequest`), which matches your concern: a handle tied to `FrameLoadRequest` lifetime does not meaningfully protect the later same-document commit IPC path.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |