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