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: fix 'src' permissions policy allowlist. #172

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Aug 1, 2024

The 'src' allowlist is a feature unique to iframes and fenced frames that, if set, only allows a given feature for the origin loaded in the src/config attribute. When loading a fenced frame with a fenced frame config, that origin is opaque to the embedder but transparent to the inner content. When loading an iframe with a URN that maps to a fenced frame config, it will see the opaque urn in the src attribute and assume that is the src that the permissions policy allowlists need to check for, which is wrong. When loading a fenced frame, there is no src attribute, and the existing permissions policy spec has no way to handle the config attribute. This results in the src allowlist being incorrect when calculating the permissions policy for a frame loaded with a fenced frame config.

This is fixed by updating the declared origin algorithm to account for cases when a fenced frame config instance is installed in the navigated inner document, setting the expected src origin in the allowlists to the config's mapped url.

The declared origin algorithm is invoked as part of creating a permissions policy for a navigable from response, which in turn happens when initializing the document. This also happens right after the (monkeypatched in) step where the fenced frame config is installed into the inner document's browsing context, so the required information about the mapped URL will be in place by the time the permissions policy is constructed.


Preview | Diff

@domfarolino
Copy link
Collaborator

The mismatch is fixed by recalculating the document's permissions policy allowlists once the mapped URL is known. More specifically, this is done during the create and initialize a Document object algorithm right after the |permissionsPolicy| is first loaded into the document.

Any allowlists that match the opaque 'src' (a value set by the embedder that doesn't and shouldn't know the final navigated URL) are replaced with the document's fenced frame config's mapped URL's origin.

I'm curious, was this also an implementation bug as well that had to be fixed in a similar way? One thing that jumped out to me is: should we even have any allowlists that match an opaque URL? If they are just going to be replaced in the future by allowlists for "transparent" URLs, maybe we can just avoid adding them in the first place when the FFC's url is opaque (to the embedder)?

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

I'm curious, was this also an implementation bug as well that had to be fixed in a similar way? One thing that jumped out to me is: should we even have any allowlists that match an opaque URL? If they are just going to be replaced in the future by allowlists for "transparent" URLs, maybe we can just avoid adding them in the first place when the FFC's url is opaque (to the embedder)?

With the latest patch, the allowlist will now never match an opaque URL for fenced frames. While the implementation has a much more strict boundary between embedder and inner contents (because of the renderer/browser model) and forces us to do the src replacement, the spec makes it easier to look into a fenced frame's inner document when building the permissions policy (similar to what we do when deriving a permissions policy directly from a fenced frame config instance).

@domfarolino domfarolino merged commit c10d8f6 into master Sep 11, 2024
2 checks passed
@domfarolino domfarolino deleted the liam-src-allowlist branch September 11, 2024 17:23
github-actions bot added a commit that referenced this pull request Sep 11, 2024
SHA: c10d8f6
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