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

Local scheme navigation policy container patch #73

Merged
merged 26 commits into from
Jun 22, 2023
Merged

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Apr 13, 2023

When navigating to a URL with a local scheme (URLs like about:blank), iframes clone their policy container from the policy container of the navigation's initiating document. We don't want fenced frames to be able to do that, since that will tell the fenced frame information about a policy container that lives past a fenced frame boundary. This PR adds a patch to stop that inheritance from happening.

(Implementation detail) Note that with the Chromium's MPArch implementation of fenced frames, we get that behavior for free because the initiator frame token is not passed in to the policy container builder during the navigation request. However, we want to make sure that any other implementations also have this behavior, which is why this patch is needed.

The main algorithm that is patched is determine navigation params policy container. It now takes a new "fenced" parameter that prevents cloning the initiator policy container if set to true. There are 3 places where this function is called:


Preview | Diff

@blu25 blu25 requested a review from domfarolino April 13, 2023 19:12
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.

First pass. Also it looks like there are merge conflicts now as a few things have shifted out from under.

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
@domfarolino
Copy link
Collaborator

Is there anything with PNA that needs to be addressed in the navigation case? Perhaps only for about:blank navigations but maybe more, I can't recall exactly (though I thought there was some work to be done in that area).

@blu25
Copy link
Collaborator Author

blu25 commented May 9, 2023

Is there anything with PNA that needs to be addressed in the navigation case? Perhaps only for about:blank navigations but maybe more, I can't recall exactly (though I thought there was some work to be done in that area).

From what I understand, we only need to make sure that fenced frames aren't inheriting the policy container's IP address space from its embedder. I don't think that happens during the fetching process.

Though, looking at this again, I'm also not entirely convinced that this where the gating should happen.

spec.bs Outdated Show resolved Hide resolved
@domfarolino
Copy link
Collaborator

From what I understand, we only need to make sure that fenced frames aren't inheriting the policy container's IP address space from its embedder. I don't think that happens during the fetching process.

For navigations to local schemes I think it certainly can, right? https://html.spec.whatwg.org/C#populating-a-session-history-entry:determining-navigation-params-policy-container-2. So I think this is on the right track.

We also need to prevent the inheritance for initial Document creation, but that looks like it's being done mostly in https://wicg.github.io/fenced-frame/#creating-browsing-contexts-patch, which we should probably allude to in your PR, since that takes care of the creation case.

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

This pretty much LGTM after my adjustments, % one question here: #73 (review) that needs to be answered.

spec.bs Outdated Show resolved Hide resolved
@blu25
Copy link
Collaborator Author

blu25 commented May 30, 2023

After talking with Titouan offline, I think we need to be patching more than just PNA. I don't think we want to inherit any part of the policy container from a parent for local navigations, so I've rewritten the spec to reflect that. Instead of just setting the IP address space, now it will only inherit the policy container from the initiator if the navigation is a local navigation not in a fenced frame. I think this is simpler and what we want the behavior to be in the end. WDYT?

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 changed the title Add PNA monkey patches Local scheme navigation policy container patch Jun 21, 2023
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.

Looks good, I've made some very small editorial adjustments but nothing major. Thanks for the big improvements.

@domfarolino domfarolino merged commit b5bb926 into master Jun 22, 2023
@domfarolino domfarolino deleted the liam-pna branch June 22, 2023 14:11
github-actions bot added a commit that referenced this pull request Jun 22, 2023
SHA: b5bb926
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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