Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec: Wait for network revocation in nested fenced frames before disabling network. #176

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Aug 14, 2024

This PR introduces a new algorithm: Recalculate the untrusted network status of all frames. This is called whenever a fenced frame marks its network as disabled, and checks to see if any ancestor fenced frames are now allowed to have their network access be fully revoked and gain access to unpartitioned data.

This PR modifies disableUntrustedNetwork() to not resolve the promise, and instead puts the promise into the fenced frame config instance to be resolved once the frame tree is considered to have its network fully revoked.

This builds off of the work in #146, and this should only be merged after #146 is merged.

See: issue #168


Preview | Diff

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't made it through everything quite yet but getting there. This is a start for now.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@blu25 blu25 self-assigned this Oct 30, 2024
@blu25 blu25 marked this pull request as ready for review November 1, 2024 18:47
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
1. If |config|'s [=fenced frame config instance/untrusted network status=] is [=untrusted
network status/disabled for this and descendant trees=], then [=iteration/continue=].

1. Let |networkCutoffReady| be true if |navigablesWithNetworkChildren| does not [=set/contain=]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would |navigablesWithNetworkChildren| already contain |currentNavigable|? That would require iterating over the same |currentNavigable| twice in this loop. Is that even possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|navigablesWithNetworkChildren| is set every time a frame is determined to not be ready for network cutoff. However, it is the frame's |ancestorFencedRoot| which ends up being put into |navigablesWithNetworkChildren|. So in the while loop, this will be the first time this frame is visited. But if it has a child fenced frame whose network is not disabled yet, the frame will have already been added to this list.

I'm realizing I used the wrong removal method. I currently am dequeuing from the list. I should be popping instead, since my intention is to have this loop go in reverse DFS order to ensure that the deepest nested frames are hit first. I'll fix that.

1. For each |promise| in |config|'s [=fenced frame config instance/on network disabled
promises=]:

1. [=Queue a global task=] on the [=DOM manipulation task source=] given |global|, to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about this. |global| comes from the current nav's active window, but is it guaranteed that |promise| was constructed within that global? For example, what if inside a fenced frame, a cross-origin descendant iframe calls disableUntrustedNetwork(). First, is that allowed? Second, if it's allowed, then I think we'd end up scheduling the resolution of that Promise (that was created in the cross-origin descendant global) on the global of its nearest fenced frame root (since at this point |currentNavigable| is guaranteed to be a fenced navigable's global).

Does this make sense / am I right about this being possible?

Copy link
Collaborator Author

@blu25 blu25 Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, what if inside a fenced frame, a cross-origin descendant iframe calls disableUntrustedNetwork(). First, is that allowed?

No. disableUntrustedNetwork() throws a TypeError if the origin that calls the method is cross-origin to the mapped URL of the ff config instance. The promises can only come from either the fenced frame root or a same-origin descendant. So I don't think what you're describing is possible, and a promise from this list will always be resolved with the correct global.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, same-origin descendant iframes have different globals, but we resolve them here on the fenced frame's global, which is bad, right?


1. <p class=XXX>TODO: destroy the nested traversable.</p>

1. [=Recalculate the untrusted network status of all fenced frame descendants=] given the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also have to do this during iframe teardown? I don't think so but I want to be sure.

[=browsing context/fenced frame config instance=].
1. Let |context| be [=this=]'s [=relevant global object=]'s [=Window/browsing context=].

1. If |context| is null, then throw a {{SecurityError}} {{DOMException}}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check, do we have a web platform test exercising this specific condition?

1. For each |promise| in |config|'s [=fenced frame config instance/on network disabled
promises=]:

1. [=Queue a global task=] on the [=DOM manipulation task source=] given |global|, to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, same-origin descendant iframes have different globals, but we resolve them here on the fenced frame's global, which is bad, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants