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 37 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
@blu25 blu25 requested a review from domfarolino 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
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@blu25 blu25 requested a review from domfarolino November 5, 2024 17:30
spec.bs Outdated

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only if the iframe teardown results in a child fenced frame also being torn down, but that will get caught by this anyway, so we don't need to add anything for iframes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait how will it get caught by this? Check out the iframe destruction/removal path — I don't think it results in fenced frames being "removed" from the DOM, so I don't think we'd go through this path for fenced frames if their iframe parent is removed.

Also, note that the recalculate algorithm requires being in parallel, but the removal steps are not called from in parallel. That's a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I had assumed it was a cascading operation, but it looks like it's not. I'll add a separate removed from a document algorithm for iframes. I didn't see any place where this was being called for iframes to begin with, so it should be okay to add a new algorithm.

I'm not sure if there's some common path that would be better to put this in. I considered the node removal algorithm, but that lives in the DOM spec, and I don't think that's supposed to get access to the concepts of traversables and node navigables. Let me know if there's a better spot you think this could live in.

I also updated the calls to be run in parallel.

Copy link
Collaborator

@domfarolino domfarolino Dec 29, 2024

Choose a reason for hiding this comment

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

It's already being called here: https://html.spec.whatwg.org/C#the-iframe-element:html-element-removing-steps. It's using the more appropriate removing steps hook, instead of the removed from document hook, which I have a goal of removing from the HTML spec anyways, so I don't think we should add more instances of it sadly.

So I guess we should just monkeypatch the existing iframe algorithm, and as a follow-up, some day we can probably modify our spec to not use the removed from document hook.

spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@blu25 blu25 requested a review from domfarolino December 6, 2024 23:03
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@blu25 blu25 requested a review from domfarolino December 11, 2024 19:26
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.

One more issue I spotted is that the https://wicg.github.io/fenced-frame/#revoke-network-for-a-partition-nonce algorithm uses "this" which it cannot do since it is called from an in parallel context. I think we should fix that in this PR.

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
Copy link
Collaborator Author

blu25 commented Dec 12, 2024

One more issue I spotted is that the https://wicg.github.io/fenced-frame/#revoke-network-for-a-partition-nonce algorithm uses "this" which it cannot do since it is called from an in parallel context. I think we should fix that in this PR.

Modified to take in a relevant settings object instead of using this.

@blu25 blu25 requested a review from domfarolino December 12, 2024 18:47
spec.bs Outdated Show resolved Hide resolved
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