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 "disable embedder-initiated nested fenced frame navigations after disabling untrusted network" #148

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

Conversation

gtanzer
Copy link
Collaborator

@gtanzer gtanzer commented Apr 9, 2024

@gtanzer
Copy link
Collaborator Author

gtanzer commented Apr 10, 2024

Waiting for #146 to be merged for the refs in this to compile.

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 Aug 19, 2024

@domfarolino Addressed comments but still waiting for other PR to unblock

@domfarolino
Copy link
Collaborator

Just pinging @VergeA to make sure he's aware of this PR since I think he'll be taking it over once #146 lands.

@VergeA
Copy link
Collaborator

VergeA commented Nov 21, 2024

The changes from #146 have made it into the main branch (by way of other stacked PRs). I'll fix the merge conflicts here and ping again when it's ready for review.

@VergeA
Copy link
Collaborator

VergeA commented Nov 21, 2024

Alright, this should be up to date now, although this change will be affected by the transition to "untrusted network status" as written in #176.

spec.bs Outdated Show resolved Hide resolved
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.

I just want to double check something — you are intending for this PR to have no effect in a primary frame's Document, right? The only place this would have an effect in is inside a fenced frame which is trying to set the config attribute on a nested fenced frame. If that top-level fenced frame has a config instance with disabled network, then we would fail to navigate the nested fenced frame as expected. But if we're running the config setter from a primary main frame's script, the config instance will never be null and therefore this PR won't have an effect in that case. That's the intention, right?

Also, if there are any tests you can link to from the PR, please do so. That would help a lot.

@VergeA
Copy link
Collaborator

VergeA commented Dec 2, 2024

Yes, your understanding is correct. See the corresponding Chromium CL here which implements that behavior explicitly.

RE linking tests: Our network revocation tests are in Chromium's internal WPT directory right now. The plumbing to allow our RemoteContext-based fenced frame tests to work while disabling network everywhere else requires a window.internals API that is not available to other browsers. Is there a way you recommend linking internal tests that doesn't clash with the existing WPT linking format? Or should they be left out if they aren't upstreamed?

@domfarolino
Copy link
Collaborator

Is there a way you recommend linking internal tests that doesn't clash with the existing WPT linking format? Or should they be left out if they aren't upstreamed?

The right way would be to file whatever bugs are necessary against web driver and WPT's test harness infrastructure so that some sort of engine-agnostic API can be implemented to expose the right behavior. I recall being able to file RFCs about this somewhere, maybe you could try and track this down. In the meantime, I would file an issue against our repository describing the task of doing exactly that, and linking to it from our spec where this is implemented, since it's a pretty big issue — it means no other browser that implements fenced frames can assert the correct behavior here.

@VergeA
Copy link
Collaborator

VergeA commented Dec 10, 2024

@blu25 has already filed an issue against our repository to get a new Webdriver API implemented: #192.

It is already linked in the spec, on line 1993 in the incoming branch.

@domfarolino
Copy link
Collaborator

Sure, but starting that process with an actual RFC would be probably more useful to get the ball rolling on it.

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.

3 participants