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 disableUntrustedNetwork API surface #146

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

Conversation

gtanzer
Copy link
Collaborator

@gtanzer gtanzer commented Apr 3, 2024

@gtanzer gtanzer requested a review from domfarolino April 9, 2024 16:49
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
@gtanzer
Copy link
Collaborator Author

gtanzer commented Apr 11, 2024

Addressed comments % a question about "default" vs "initial" values.

@domfarolino
Copy link
Collaborator

Can you just "resolve" all comment threads that have been resolved at this point?

@domfarolino
Copy link
Collaborator

And resolve the merge conflict that this PR now has with master?

@gtanzer
Copy link
Collaborator Author

gtanzer commented May 4, 2024

@domfarolino Fixed merge conflicts.

The open threads aren't resolved per se. They depend on your response here: (whether I should commit the suggestions or do nothing)
#146

@domfarolino
Copy link
Collaborator

I don't care which word we use "default" vs "initial". I just want it to be consistent, and this PR makes it inconsistent with https://github.com/WICG/fenced-frame/pull/146/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR1213. So I'd either make this PR consistent with https://github.com/WICG/fenced-frame/pull/146/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR1213 or make that line consistent with this PR. Whichever word you prefer.

spec.bs Outdated
1. Wait on all nested fenced frames to disable network too.

Issue: Spec this waiting more formally.
(<a href="https://github.com/WICG/fenced-frame/issues/151">WICG/fenced-frame#151</a>)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a HUGE TODO, so I'm not really comfortable landing this PR without those fundamental mechanisms spec'ed out a bit more rigorously, just FYI. "Wait on all nested fenced frames to disable network too" is pretty rough, so I'd like to have a good draft of how this is done in hand before landing this PR.

Copy link
Collaborator

@blu25 blu25 Aug 14, 2024

Choose a reason for hiding this comment

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

I have created a draft PR #176 to spec this more formally. That should hopefully unblock this PR. The "files changed" section includes changes from this PR, but the meat and potatoes is the "Recalculate the untrusted network status of all frames" algorithm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey y'all, I'm likely going to pick up the network revocation spec work from gtanzer, since he's transitioned to other work. This seems like the first major PR to have in place, and this is the last outstanding comment. Is the draft PR #176 sufficient scaffolding for disabling network in nested frames? If so, I'll do whatever else is needed to get this PR over the finish line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to get #176 just a tiny bit further along before merging this, if that's alright. It's in decent shape, I just want to be more confident in its direction before proceeding.

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
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.

4 participants