Fenced frame: Network cutoff callback waits for sub-fenced frames. [chromium/src : main]
Andrew Verge (Gerrit)
Andrew Verge added 3 comments![]( "Open in Gerrit")
// TODO(averge): Credentialless iframes have their own per-`Page` nonce thatAndrew VergeWasn't this TODO handled in https://chromium-review.googlesource.com/c/chromium/src/+/5376066 ?
+1
`RevokeNetworkForNonces` will only call this function after both nonces have been tagged for revocation, so there's no extra work to do.
bool network_cutoff_ready = all_children_have_network_cutoff &&In the case where we have a fenced frame and an iframe nested underneath it, traversal will hit the iframe first. It will use the properties of the closest ancestor, which is the parent fenced frame, and trigger the revocation callback for it.
The iteration will then move up to the fenced frame itself, but all of the work has been done already when examining the iframe. Can we add a clause like this:
```
// If network has already been disabled for this node previously, ignore it.
if (properties->has_disabled_untrusted_network()) {
continue;
}
```
This prevents a fenced frame tree frame which has already had network fully disabled (meaning, bit is set and callback has been run) from being examined again. Might save some cycles?
continue;Do you need continues? Whether you enter this inner if clause or not, you'll still repeat the loop at the end of `if (network_cutoff_ready)`
Same below.
Related details
- Garrett Tanzer
- Liam Brady
- Nasko Oskov
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 7 comments![]( "Open in Gerrit")
Fenced frame: Network cutoff callback waits for subframes.Liam Bradynit: sub-fenced frames
Done
accessed until all subframes have also cut off their own network.Liam Bradynit: sub-fenced frames here and in other places.
Done
network cutoff has truly kicked in. In other words, once all subframes
in a fenced frame tree have had their network cut off, only then willLiam Bradynested fenced frame start their own FF tree, so this sounds confusing and seems to handle iframes. Suggest rephrasing to:
"once all sub-fenced frame trees have had..."
Done
The function that determines the network cutoff status involves a
recursive depth-first traversal of the entire frame tree (including
crossing fenced frame boundaries) to see if any subframes still haveLiam BradyI haven't yet looked at the code, but can we instead recompute the ancestor FFs' disable
We need to look at each frame, since a fenced frame's same-origin child iframe could've been the frame that called `disableUntrustedNetwork()`, which would require calling the resolve callback housed in that `RenderFrameHost`'s `FencedDocumentData`.
I think we can do what Andrew's suggesting [here](https://chromium-review.googlesource.com/c/chromium/src/+/5426603/comment/cef6b2f2_9efb7e0e/) to avoid repeating work.
// TODO(averge): Credentialless iframes have their own per-`Page` nonce thatAndrew VergeWasn't this TODO handled in https://chromium-review.googlesource.com/c/chromium/src/+/5376066 ?
+1
`RevokeNetworkForNonces` will only call this function after both nonces have been tagged for revocation, so there's no extra work to do.
Done
bool network_cutoff_ready = all_children_have_network_cutoff &&In the case where we have a fenced frame and an iframe nested underneath it, traversal will hit the iframe first. It will use the properties of the closest ancestor, which is the parent fenced frame, and trigger the revocation callback for it.
The iteration will then move up to the fenced frame itself, but all of the work has been done already when examining the iframe. Can we add a clause like this:
```
// If network has already been disabled for this node previously, ignore it.
if (properties->has_disabled_untrusted_network()) {
continue;
}
```This prevents a fenced frame tree frame which has already had network fully disabled (meaning, bit is set and callback has been run) from being examined again. Might save some cycles?
We still need to run any callbacks registered in the `RenderFrameHost`'s `FencedDocumentData`, but I can add this to save some redundant processing.
continue;Do you need continues? Whether you enter this inner if clause or not, you'll still repeat the loop at the end of `if (network_cutoff_ready)`
Same below.
- Andrew Verge
- Garrett Tanzer
- Nasko Oskov
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 2 comments![]( "Open in Gerrit")
bool is_partition_nonce_revoked_ = false;
// Whether this fenced frame has had its network access cut off (i.e., if this
// is true, untrusted network access should be disabled and access to
// unpartitioned storage should be enabled.)
bool has_disabled_untrusted_network_ = false;both of these are states of the progress of disableUntrustedNetwork so suggest putting them in an enum. Using kCurrentFrameTreeComplete for is_partition_nonce_revoked_
`enum class DisableUntrustedNetworkStatus {
kNotStarted,
kCurrentFrameTreeComplete,
kCurrentAndNestedFrameTreesComplete
}` // The nonce that will be included in the IsolationInfo for network requestsreplace the rest of the comment with "IsolationInfo, CookiePartitionKey and StorageKey for network, cookie and storage partitioning, respectively. As part of IsolationInfo it is also used to identify which network requests should be disallowed in the network service if the initiator fenced frame tree has had its network cutoff via disableUntrustedNetwork()."
Related details
- Andrew Verge
- Garrett Tanzer
- Liam Brady
- Nasko Oskov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Garrett Tanzer (Gerrit)
Garrett Tanzer added 2 comments![]( "Open in Gerrit")
bool is_partition_nonce_revoked_ = false;
// Whether this fenced frame has had its network access cut off (i.e., if this
// is true, untrusted network access should be disabled and access to
// unpartitioned storage should be enabled.)
bool has_disabled_untrusted_network_ = false;both of these are states of the progress of disableUntrustedNetwork so suggest putting them in an enum. Using kCurrentFrameTreeComplete for is_partition_nonce_revoked_
`enum class DisableUntrustedNetworkStatus {
kNotStarted,
kCurrentFrameTreeComplete,
kCurrentAndNestedFrameTreesComplete
}`
+1
<script src="resources/utils.js"></script>I think at least some of the WPT test cases could be integrated into the existing shared storage get() tests:
For example, there is a TODO explicitly for this:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html;l=123
Related details
- Andrew Verge
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
could_call_get = true;You can skip all the `could_call_get` stuff by adding an `assert_unreached()` at this line instead. If the get() completes successfully, the assert will be executed next, which will throw immediately.
Then you can double check the type of error in the catch, and make sure it's not `AssertionError`, I believe
https://web-platform-tests.org/writing-tests/testharness-api.html#assert_unreached
Related details
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 4 comments![]( "Open in Gerrit")
bool is_partition_nonce_revoked_ = false;
// Whether this fenced frame has had its network access cut off (i.e., if this
// is true, untrusted network access should be disabled and access to
// unpartitioned storage should be enabled.)
bool has_disabled_untrusted_network_ = false;Garrett Tanzerboth of these are states of the progress of disableUntrustedNetwork so suggest putting them in an enum. Using kCurrentFrameTreeComplete for is_partition_nonce_revoked_
`enum class DisableUntrustedNetworkStatus {
kNotStarted,
kCurrentFrameTreeComplete,
kCurrentAndNestedFrameTreesComplete
}`
+1
Done
// The nonce that will be included in the IsolationInfo for network requestsreplace the rest of the comment with "IsolationInfo, CookiePartitionKey and StorageKey for network, cookie and storage partitioning, respectively. As part of IsolationInfo it is also used to identify which network requests should be disallowed in the network service if the initiator fenced frame tree has had its network cutoff via disableUntrustedNetwork()."
Done
I think at least some of the WPT test cases could be integrated into the existing shared storage get() tests:
For example, there is a TODO explicitly for this:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html;l=123
Done
You can skip all the `could_call_get` stuff by adding an `assert_unreached()` at this line instead. If the get() completes successfully, the assert will be executed next, which will throw immediately.
Then you can double check the type of error in the catch, and make sure it's not `AssertionError`, I believe
https://web-platform-tests.org/writing-tests/testharness-api.html#assert_unreached
Resolving since I'm merging this test into the test in `content-shared-storage-get.https.html`, which already does that.
Related details
- Andrew Verge
- Garrett Tanzer
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Garrett Tanzer (Gerrit)
Garrett Tanzer added 3 comments![]( "Open in Gerrit")
bool HasDisabledUntrustedNetwork() const {I would rather keep the names of these accessors similar to the enums. It's not really clear that "HasDisabledUntrustedNetwork" applies to all descendants.
`HasDisabledNetworkForCurrentFrameTree`
`HasDisabledNetworkForCurrentAndDescendantFrameTrees`
kCurrentAndNestedFrameTreesCompletecan we say `Descendant` instead of `Nested`, since nested could mean 1 level of nesting only
// Set after all descendant fenced frames have had network cutoff.nit: cutoff -> cut off.
Related details
- Andrew Verge
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 3 comments![]( "Open in Gerrit")
I would rather keep the names of these accessors similar to the enums. It's not really clear that "HasDisabledUntrustedNetwork" applies to all descendants.
`HasDisabledNetworkForCurrentFrameTree`
`HasDisabledNetworkForCurrentAndDescendantFrameTrees`
Done
can we say `Descendant` instead of `Nested`, since nested could mean 1 level of nesting only
Done
// Set after all descendant fenced frames have had network cutoff.nit: cutoff -> cut off.
- Andrew Verge
- Garrett Tanzer
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 5 comments![]( "Open in Gerrit")
void MarkPartitionNonceRevoked() {For consistency, MarkDisabledNetworkForCurrentFrameTree() and MarkDisabledNetworkForCurrentAndDescendantFrameTrees() for the next function.
return disable_untrusted_network_status_ !=
DisableUntrustedNetworkStatus::kNotStarted;disable_untrusted_network_status_ == DisableUntrustedNetworkStatus::kCurrentFrameTreeComplete
// revocation can only happen when network access has been disabled for its
// FencedFrameProperties' partition_nonce_ as well as for all of its
// descendants. This is calculated for all FrameTreeNodes, including nodesSince disabling network is not just nonce-based, let's rephrase this to:
"// Network revocation can only happen when network access has been disabled for this fenced frame tree as well as for all of its descendant fenced frame trees."
and then the next sentence can be removed since it's redundant.
// If we are not in a frame tree with a FencedFrameProperties, then network
// cutoff isn't possible.what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?
fenced_document_data->RunDisabledUntrustedNetworkCallbacks();shouldn't RunDisabledUntrustedNetworkCallbacks only get called when properties->MarkUntrustedNetworkDisabled() is called?
Related details
- Andrew Verge
- Garrett Tanzer
- Liam Brady
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge added 1 comment![]( "Open in Gerrit")
// If we are not in a frame tree with a FencedFrameProperties, then network
// cutoff isn't possible.what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?
Could we just skip checking the URN iframe properties, and use `GetFencedFrameProperties(kFrameTreeRoot)` unconditionally?
- If this traversal hits a fenced frame or any of its descendant iframes of any type, the revocation check will be done for the fenced frame root's properties.
- If a child frame revokes network on behalf of the fenced frame root, all other frames in the tree will hit the clause on line 9291 and continue until we exit the fenced frame
- If network is not ready to be revoked, then each parent frame will propagate that bit up via `node_ids_with_network_allowed_descendants`, same as right now.
Related details
- Garrett Tanzer
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Garrett Tanzer (Gerrit)
Garrett Tanzer added 1 comment![]( "Open in Gerrit")
// If we are not in a frame tree with a FencedFrameProperties, then network
// cutoff isn't possible.Andrew Vergewhat about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?
Could we just skip checking the URN iframe properties, and use `GetFencedFrameProperties(kFrameTreeRoot)` unconditionally?
- If this traversal hits a fenced frame or any of its descendant iframes of any type, the revocation check will be done for the fenced frame root's properties.
- If a child frame revokes network on behalf of the fenced frame root, all other frames in the tree will hit the clause on line 9291 and continue until we exit the fenced frame
- If network is not ready to be revoked, then each parent frame will propagate that bit up via `node_ids_with_network_allowed_descendants`, same as right now.
- Andrew Verge
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 5 comments![]( "Open in Gerrit")
For consistency, MarkDisabledNetworkForCurrentFrameTree() and MarkDisabledNetworkForCurrentAndDescendantFrameTrees() for the next function.
Done
return disable_untrusted_network_status_ !=
DisableUntrustedNetworkStatus::kNotStarted;Liam Bradydisable_untrusted_network_status_ == DisableUntrustedNetworkStatus::kCurrentFrameTreeComplete
For documentation sake, the reason why this is fine is because `CalculateUntrustedNetworkStatus()` has a check for `HasDisabledNetworkForCurrentAndDescendantFrameTrees` with a `continue;` before we check `HasDisabledNetworkForCurrentFrameTree`. There's no point in this CL where we need to check that either of the 2 states are in place, so it's fine to have `HasDisabledNetworkForCurrentFrameTree` be exclusively checking if the current frame tree is disabled but descendant trees are not.
// revocation can only happen when network access has been disabled for its
// FencedFrameProperties' partition_nonce_ as well as for all of its
// descendants. This is calculated for all FrameTreeNodes, including nodesSince disabling network is not just nonce-based, let's rephrase this to:
"// Network revocation can only happen when network access has been disabled for this fenced frame tree as well as for all of its descendant fenced frame trees."and then the next sentence can be removed since it's redundant.
Done
// If we are not in a frame tree with a FencedFrameProperties, then network
// cutoff isn't possible.what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?
This is referring to the fact that we don't need to calculate anything since there are no ancestors that could potentially have their network cut off. I'll update the comment to clarify.
I'll also use `kFrameTreeRoot` for the traversal.
fenced_document_data->RunDisabledUntrustedNetworkCallbacks();shouldn't RunDisabledUntrustedNetworkCallbacks only get called when properties->MarkUntrustedNetworkDisabled() is called?
If you have a fenced frame (A1) that has a child iframe (A2), and both frames call `window.fence.disableUntrustedNetwork()`, the promise for both A1 and A2 need to resolve. Since the promise is saved to the `FencedDocumentData`, and since the A1 and A2 documents will have their own distinct `FencedDocumentData`, we need to call the relevant callbacks for each document associated with the `FencedFrameConfig`.
This is verified with the last test in `content-shared-storage-get-nested.https.html`.
Related details
- Andrew Verge
- Garrett Tanzer
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Verge
| Code-Review | +1 |
1 comment
Giving my LGTM for WPTs, CalculateUntrustedNetworkStatus, and shared_storage_browsertest, since those were requested of me in the initial review comment and/or offline discussion. Will defer to the other reviewers for the remaining files.
Related details
- Garrett Tanzer
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Garrett Tanzer (Gerrit)
Garrett Tanzer added 2 comments![]( "Open in Gerrit")
will lgtm once the WPT comment is addressed
let disable_network_promise = window.fence.disableUntrustedNetwork();As the TODO says, this test should still exist. It should just use a timeout to check that disableUntrustedNetwork doesn't resolve, then attempt to access shared storage get (which should fail, even though network for the current frame tree is disabled).
You might also want to do a fetch here and in the nested fenced frame to confirm that network is disabled in the parent but not the child.
Related details
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 1 comment![]( "Open in Gerrit")
let disable_network_promise = window.fence.disableUntrustedNetwork();As the TODO says, this test should still exist. It should just use a timeout to check that disableUntrustedNetwork doesn't resolve, then attempt to access shared storage get (which should fail, even though network for the current frame tree is disabled).
You might also want to do a fetch here and in the nested fenced frame to confirm that network is disabled in the parent but not the child.
- Garrett Tanzer
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Garrett Tanzer (Gerrit)
Garrett Tanzer voted Code-Review+1![]( "Open in Gerrit")
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 1 comment![]( "Open in Gerrit")
Removing nasko@ from the reviewers list since he's OOO.
alexmos@: PTAL at the following files:
- render_frame_host_impl.h
- render_frame_host_impl.cc
- shared_storage_browsertest.cc
Thanks!
Related details
- Alex Moshchuk
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 5 comments![]( "Open in Gerrit")
// vector to account for the web platform having multiple calls tonit: "supporting"?
// tree has had its network cutoff via disableUntrustedNetwork().nit: "cut off"
CHECK(can_disable_untrusted_network_);Can we check that disable_untrusted_network_status_ isn't kCurrentAndDescendantFrameTreesComplete at this point, i.e. that we aren't relaxing it by accident?
->HasDisabledNetworkForCurrentAndDescendantFrameTrees()) {Why wouldn't cases like this use HasDisabledNetworkForCurrentFrameTree(), since on first glance, no new navigations should happen in the current FF once it has disabled network, regardless of descendants, and also the error message only talks about the current fenced frame's network? Is this because for HasDisabledNetworkForCurrentFrameTree(), unpartitioned storage hasn't been granted to the current FF yet (due to descendants still having network), and may never be granted if a descendant FF never disables its network, so you don't want to block navigations out of the current FF forever? Seems worth explaining in a comment somewhere, basically when you would use HasDisabledNetworkForCurrentFrameTree() vs HasDisabledNetworkForCurrentAndDescendantFrameTrees() to gate access to features that require network access. (similarly below)
properties->MarkDisabledNetworkForCurrentAndDescendantFrameTrees();I'm having trouble following this logic for more complex hierarchies. Consider the following case: A(FF1(B(C)),FF2). So, A embeds a fenced frame, which embeds an iframe B and a nested FF2, and B further embeds C. The list of nodes would be {A,FF1,B,FF2,C} (breadth-first, per https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/frame_tree.cc;l=84;drc=a009c99eedf0ee26bfbf0dd0cfef48c7db4cc679), and when reversed it would be {C,FF2,B,FF1,A}. Suppose FF1 disables network and we get here and start with C. Wouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Could we add a browser or unit test for exercising how CalculateUntrustedNetworkStatus() handles some complicated hierarchies like this?
Related details
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 5 comments![]( "Open in Gerrit")
// vector to account for the web platform having multiple calls toLiam Bradynit: "supporting"?
Done
// tree has had its network cutoff via disableUntrustedNetwork().Liam Bradynit: "cut off"
Done
Can we check that disable_untrusted_network_status_ isn't kCurrentAndDescendantFrameTreesComplete at this point, i.e. that we aren't relaxing it by accident?
There's currently nothing that checks this, but I agree that this should be in place. I'll add an early return to `RenderFrameHostImpl::RevokeNetworkForNonceCallback` (doing it in the callback to prevent race conditions) and the `CHECK` here.
->HasDisabledNetworkForCurrentAndDescendantFrameTrees()) {Why wouldn't cases like this use HasDisabledNetworkForCurrentFrameTree(), since on first glance, no new navigations should happen in the current FF once it has disabled network, regardless of descendants, and also the error message only talks about the current fenced frame's network? Is this because for HasDisabledNetworkForCurrentFrameTree(), unpartitioned storage hasn't been granted to the current FF yet (due to descendants still having network), and may never be granted if a descendant FF never disables its network, so you don't want to block navigations out of the current FF forever? Seems worth explaining in a comment somewhere, basically when you would use HasDisabledNetworkForCurrentFrameTree() vs HasDisabledNetworkForCurrentAndDescendantFrameTrees() to gate access to features that require network access. (similarly below)
This should definitely be disabled as soon as `disableUntrustedNetwork()` is called for the fenced frame, regardless of the status of its children. This also matches what's in the design doc (as well as the current impl behavior):
(Preferred) Network is disabled immediately, and all requests in progress are canceled.
Network is disabled immediately, and requests in progress may complete, but no further packets will be sent out from the device.
Good catch.
I've also updated some WPTs to check this case.
properties->MarkDisabledNetworkForCurrentAndDescendantFrameTrees();I'm having trouble following this logic for more complex hierarchies. Consider the following case: A(FF1(B(C)),FF2). So, A embeds a fenced frame, which embeds an iframe B and a nested FF2, and B further embeds C. The list of nodes would be {A,FF1,B,FF2,C} (breadth-first, per https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/frame_tree.cc;l=84;drc=a009c99eedf0ee26bfbf0dd0cfef48c7db4cc679), and when reversed it would be {C,FF2,B,FF1,A}. Suppose FF1 disables network and we get here and start with C. Wouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Could we add a browser or unit test for exercising how CalculateUntrustedNetworkStatus() handles some complicated hierarchies like this?
Wouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Yes. I think what we need to do instead is only set the bit and run the callbacks on the fenced frame root, once we've had a chance to look at all its descendants.
So this means that the loop will continue for `C` and `B`, and will run for `FF2` and `FF1`. If either `C` or `B` were the ones that called `disableUntrustedNetwork()`, their callbacks will now get put into the `FencedDocumentData` of `FF1`.
Related details
- Alex Moshchuk
- Andrew Verge
- Garrett Tanzer
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 7 comments![]( "Open in Gerrit")
Thanks, a couple more comments about tests and adding/removing fenced frames in the middle of all this.
}Thanks for adding this test. Can you also check the state on the second FF, which should also have both HasDisabledNetworkForCurrentFrameTree and HasDisabledNetworkForCurrentAndDescendantFrameTrees as true at this point?
// the frame tree, as an ancestor's revocation status could've changed as aThere may be some unnecessary work here for sibling FFs, e.g. with A(FF1(FF2),FF3,FF4), disabling network in FF2 only needs to recalculate FF2 and FF1, not FF3 or FF4. But it's probably easiest to just recalculate the whole tree?
void RenderFrameHostImpl::CalculateUntrustedNetworkStatus() {FWIW, this reminds me of what we do for collecting beforeunload/unload ACKs before proceeding with navigation or frame detach (things like PendingDeletionCheckCompletedOnSubtree(), etc). No need to change your approach, just wanted to provide context about how we've dealt with this kind of problem in the past in slightly different ways if you haven't seen it.
// FrameTreeNode.Is it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
Probably would be good to cover these in tests too.
properties->MarkDisabledNetworkForCurrentAndDescendantFrameTrees();Liam BradyI'm having trouble following this logic for more complex hierarchies. Consider the following case: A(FF1(B(C)),FF2). So, A embeds a fenced frame, which embeds an iframe B and a nested FF2, and B further embeds C. The list of nodes would be {A,FF1,B,FF2,C} (breadth-first, per https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/frame_tree.cc;l=84;drc=a009c99eedf0ee26bfbf0dd0cfef48c7db4cc679), and when reversed it would be {C,FF2,B,FF1,A}. Suppose FF1 disables network and we get here and start with C. Wouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Could we add a browser or unit test for exercising how CalculateUntrustedNetworkStatus() handles some complicated hierarchies like this?
Wouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Yes. I think what we need to do instead is only set the bit and run the callbacks on the fenced frame root, once we've had a chance to look at all its descendants.
So this means that the loop will continue for `C` and `B`, and will run for `FF2` and `FF1`. If either `C` or `B` were the ones that called `disableUntrustedNetwork()`, their callbacks will now get put into the `FencedDocumentData` of `FF1`.
Thanks for fixing this and adding a test for it. The new version seems like it should work.
Can you please also add a test to validate the ordering and DisabledForCurrent/DisabledForCurrentAndDescendant for a similar scenario but with two independent chains of FFs that are disabling network, which are possible IIUC? For example:
- A(FF1(FF2),FF3(FF4))
- FF1 disables network, no callbacks run
- FF4 disables network, FF4 callbacks run
- FF3 disables network, FF3 callbacks run
- FF2 disables network, FF2 and FF1 callbacks run
// ancestor fenced frames will be ready for network cutoff either.The code doesn't actually check that GetParentOrOuterDocument()->GetMainFrame() is a fenced frame; it could be a regular frame (e.g., top frame). Is that ok, since we'll probably just not use it?
Related details
- Andrew Verge
- Garrett Tanzer
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 5 comments![]( "Open in Gerrit")
Thanks for adding this test. Can you also check the state on the second FF, which should also have both HasDisabledNetworkForCurrentFrameTree and HasDisabledNetworkForCurrentAndDescendantFrameTrees as true at this point?
Done
// the frame tree, as an ancestor's revocation status could've changed as aThere may be some unnecessary work here for sibling FFs, e.g. with A(FF1(FF2),FF3,FF4), disabling network in FF2 only needs to recalculate FF2 and FF1, not FF3 or FF4. But it's probably easiest to just recalculate the whole tree?
If `A` were instead a fenced frame, giving us `FF0(FF1(FF2),FF3,FF4)`, we would still need to look at `FF3` and `FF4` to check if their network has been cut off to determine if `FF0` can have network cutoff as well. I think the extra work to figure out what frames actually need to be looked at isn't worth the small reduction in unnecessary checks.
Is it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
Probably would be good to cover these in tests too.
Is it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
properties->MarkDisabledNetworkForCurrentAndDescendantFrameTrees();Liam BradyI'm having trouble following this logic for more complex hierarchies. Consider the following case: A(FF1(B(C)),FF2). So, A embeds a fenced frame, which embeds an iframe B and a nested FF2, and B further embeds C. The list of nodes would be {A,FF1,B,FF2,C} (breadth-first, per https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/frame_tree.cc;l=84;drc=a009c99eedf0ee26bfbf0dd0cfef48c7db4cc679), and when reversed it would be {C,FF2,B,FF1,A}. Suppose FF1 disables network and we get here and start with C. Wouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Could we add a browser or unit test for exercising how CalculateUntrustedNetworkStatus() handles some complicated hierarchies like this?
Alex MoshchukWouldn't we immediately reach this line and mark FF1 as MarkDisabledNetworkForCurrentAndDescendantFrameTrees(), even though we haven't heard anything from FF2?
Yes. I think what we need to do instead is only set the bit and run the callbacks on the fenced frame root, once we've had a chance to look at all its descendants.
So this means that the loop will continue for `C` and `B`, and will run for `FF2` and `FF1`. If either `C` or `B` were the ones that called `disableUntrustedNetwork()`, their callbacks will now get put into the `FencedDocumentData` of `FF1`.
Thanks for fixing this and adding a test for it. The new version seems like it should work.
Can you please also add a test to validate the ordering and DisabledForCurrent/DisabledForCurrentAndDescendant for a similar scenario but with two independent chains of FFs that are disabling network, which are possible IIUC? For example:
- A(FF1(FF2),FF3(FF4))
- FF1 disables network, no callbacks run
- FF4 disables network, FF4 callbacks run
- FF3 disables network, FF3 callbacks run
- FF2 disables network, FF2 and FF1 callbacks run
Done
// ancestor fenced frames will be ready for network cutoff either.The code doesn't actually check that GetParentOrOuterDocument()->GetMainFrame() is a fenced frame; it could be a regular frame (e.g., top frame). Is that ok, since we'll probably just not use it?
I can add a `IsNestedWithinFencedFrame()` check just to make it more clear that we don't need to check this for non-fenced frames.
Related details
- Alex Moshchuk
- Andrew Verge
- Garrett Tanzer
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
3 comments
LGTM, thanks for your patience here!
// the frame tree, as an ancestor's revocation status could've changed as aLiam BradyThere may be some unnecessary work here for sibling FFs, e.g. with A(FF1(FF2),FF3,FF4), disabling network in FF2 only needs to recalculate FF2 and FF1, not FF3 or FF4. But it's probably easiest to just recalculate the whole tree?
If `A` were instead a fenced frame, giving us `FF0(FF1(FF2),FF3,FF4)`, we would still need to look at `FF3` and `FF4` to check if their network has been cut off to determine if `FF0` can have network cutoff as well. I think the extra work to figure out what frames actually need to be looked at isn't worth the small reduction in unnecessary checks.
Agreed
// FrameTreeNode.Liam BradyIs it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
Probably would be good to cover these in tests too.
Is it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
Is it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
But is the new empty <fencedframe> going to set its disable_untrusted_network_status_ to DisableUntrustedNetworkStatus::kCurrentAndDescendantFrameTreesComplete, if it's added from a FF that's already had its network access revoked? I understand that it's not going to have network access, but wanted to check if its presence could break CalculateUntrustedNetworkStatus(). E.g., FF1(FF2), FF1 disables network, FF1 adds an empty FF3, FF2 disables network - that'll leave FF1 hanging without resolving its promise. Maybe that's ok since FF1 is kind of shooting itself in the foot by adding FF3. Or maybe RFHI::CreateFencedFrame() could account for it - leaving that up to you all to decide. :)
> Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
- Andrew Verge
- Garrett Tanzer
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Garrett Tanzer (Gerrit)
Garrett Tanzer voted Code-Review+1![]( "Open in Gerrit")
- Andrew Verge
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Verge
| Code-Review | +1 |
1 comment
New traversal algorithm looks great!
Related details
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady added 1 comment![]( "Open in Gerrit")
// FrameTreeNode.Liam BradyIs it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
Probably would be good to cover these in tests too.
Alex MoshchukIs it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
Is it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
But is the new empty <fencedframe> going to set its disable_untrusted_network_status_ to DisableUntrustedNetworkStatus::kCurrentAndDescendantFrameTreesComplete, if it's added from a FF that's already had its network access revoked? I understand that it's not going to have network access, but wanted to check if its presence could break CalculateUntrustedNetworkStatus(). E.g., FF1(FF2), FF1 disables network, FF1 adds an empty FF3, FF2 disables network - that'll leave FF1 hanging without resolving its promise. Maybe that's ok since FF1 is kind of shooting itself in the foot by adding FF3. Or maybe RFHI::CreateFencedFrame() could account for it - leaving that up to you all to decide. :)
> Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
Thanks!
It won't because once a fenced frame is marked as being `DisableUntrustedNetworkStatus::kCurrentAndDescendantFrameTreesComplete`, the invariant is that it can never be walked back. Because of that, we don't touch the network revocation status if we see that it's already set to `kCurrentAndDescendantFrameTreesComplete`. So even if a fenced frame is created whose network status is `kNotStarted`, it won't ever be able to navigate so it's safe to ignore that when doing the calculation.
I've added a browsertest to verify this behavior.
Related details
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
// FrameTreeNode.Liam BradyIs it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
Probably would be good to cover these in tests too.
Alex MoshchukIs it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?
This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
Liam BradyIs it allowed to add a nested FF (without necessarily navigating it) after the network has already been disabled, such that the value of HasDisabledNetworkForCurrentAndDescendantFrameTrees() may change due to the extra new descendant appearing?
Embedder-initiated navigations of fenced frames after network cutoff is explicitly disallowed. So while you can add as many `<fencedframe>`s as you want, none of those fenced frames will be able to be navigated to anything, so they will for all intents and purposes not have network access.
But is the new empty <fencedframe> going to set its disable_untrusted_network_status_ to DisableUntrustedNetworkStatus::kCurrentAndDescendantFrameTreesComplete, if it's added from a FF that's already had its network access revoked? I understand that it's not going to have network access, but wanted to check if its presence could break CalculateUntrustedNetworkStatus(). E.g., FF1(FF2), FF1 disables network, FF1 adds an empty FF3, FF2 disables network - that'll leave FF1 hanging without resolving its promise. Maybe that's ok since FF1 is kind of shooting itself in the foot by adding FF3. Or maybe RFHI::CreateFencedFrame() could account for it - leaving that up to you all to decide. :)
> Similarly, what about A(FF1(FF2)), FF1 disables network access, and then removes FF2. That seems like it should also trigger running this function, since FF1 should now MarkDisabledNetworkForCurrentAndDescendantFrameTrees()?This should trigger that function. I've added that to `RenderFrameHostImpl::DestroyFencedFrame()` and added a WPT to verify.
Thanks!
It won't because once a fenced frame is marked as being `DisableUntrustedNetworkStatus::kCurrentAndDescendantFrameTreesComplete`, the invariant is that it can never be walked back. Because of that, we don't touch the network revocation status if we see that it's already set to `kCurrentAndDescendantFrameTreesComplete`. So even if a fenced frame is created whose network status is `kNotStarted`, it won't ever be able to navigate so it's safe to ignore that when doing the calculation.
I've added a browsertest to verify this behavior.
Thanks for adding that test, and also adding handling for the case where the parent FF is at kCurrent rather than kCurrentAndDescendant when it adds a blank FF per our offline discussion. New changes LGTM!
Related details
- Liam Brady
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Brady (Gerrit)
Liam Brady voted Commit-Queue+2![]( "Open in Gerrit")
- Shivani Sharma
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fenced frame: Network cutoff callback waits for sub-fenced frames.
The `window.fence.disableUntrustedNetwork()` promise currently resolves
as soon as the frame's config's partition nonce is added to the network
cutoff list. However, the features gated behind network cutoff
(currently just `sharedStorage.get()`) still aren't allowed to be
accessed until all sub-fenced frames have also cut off their own
network. Sub-fenced frames still need to turn off their own network by
calling `disableUntrustedNetwork()`. To enforce that, we check every
sub-fenced frame for network cutoff whenever we call
`sharedStorage.get()`.
This CL modifies the behavior to not have the promise resolve until
network cutoff has truly kicked in. In other words, once all sub-fenced
frame trees have had their network cut off, only then will
`window.fence.disableUntrustedNetwork()` resolve.
Because it's possible for a sub-fenced frame's network cutoff call to resolve a
promise, the callback for the promise is stored in the fenced document
data if it's not resolved during the course of its
`disableUntrustedNetwork()` call. During each
`disableUntrustedNetwork()` call, each fenced frame properties' network
cutoff value is recalculated, taking child properties' network cutoff
values into account. If it is determined that this frame is ready for
network cutoff, the bit is flipped in its properties and the frame's
document's original `window.fence.disableUntrustedNetwork()` function
call is resolved.
The function that determines the network cutoff status involves a full
traversal of the entire frame tree (including crossing fenced frame
boundaries) to determine what frames have been affected by a
`disableUntrustedNetwork()` call, starting with the most deeply nested
frames and working up the frame tree.
See explainer: https://github.com/WICG/fenced-frame/blob/master/explainer/fenced_frames_with_local_unpartitioned_data_access.md#revoking-network-access
See overall design document: https://docs.google.com/document/d/1JnwuFl84P5jR_9PkpM68WdCD63UerKxWm-v4XXj2sRc/edit?usp=sharing
- M content/browser/fenced_frame/fenced_document_data.cc
- M content/browser/fenced_frame/fenced_document_data.h
- M content/browser/fenced_frame/fenced_frame.cc
- M content/browser/fenced_frame/fenced_frame_browsertest.cc
- M content/browser/fenced_frame/fenced_frame_config.h
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/renderer_host/render_frame_host_impl.h
- M content/browser/shared_storage/shared_storage_browsertest.cc
- A third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get-nested.https.html
- M third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html
- A third_party/blink/web_tests/wpt_internal/fenced_frame/disable-untrusted-network-remove-frame.https.html
- M third_party/blink/web_tests/wpt_internal/fenced_frame/revoke-manual-report-event-beacons.https.html
- M third_party/blink/web_tests/wpt_internal/fenced_frame/revoke-popup.https.html
- M third_party/blink/web_tests/wpt_internal/fenced_frame/revoke-unfenced-top-navigation.https.html
Code-Review: +1 by Alex Moshchuk, +1 by Garrett Tanzer, +1 by Andrew Verge
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |