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

Permissions Policy unload #444

Open
fergald opened this issue Jan 28, 2022 · 17 comments
Open

Permissions Policy unload #444

fergald opened this issue Jan 28, 2022 · 17 comments

Comments

@fergald
Copy link
Contributor

fergald commented Jan 28, 2022

We (chromium) would like to propose an unload permissions policy to help sites migrate away from use of unload handlers. TL;DR this header allows disabling firing the unload event on frames and subframes.

This was previously discussed in WICG/document-policy#39

The main motivation for this is that they are a large blocker of BFCache on desktop, they are a footgun (often don't fire on mobile) and in general there are better alternatives for (hopefully) all use cases.

We know that many sites are not in a position to use this header immediately, 3rd party iframes often use unload but this lets us make a start. We have reached out to some well-known iframe providers, asking them to remove their unloads. Once they do, they can guarantee remaining unload-free by shipping this header.

@rakina @clelland @chikamune @annevk @johannhof @craigfrancis

@fergald
Copy link
Contributor Author

fergald commented Feb 8, 2022

Pinging on this. Are people silently in agreement with this? Are there other people I should include?

@hober @cdumez for some WebKit coverage.

@johannhof
Copy link
Member

johannhof commented Feb 8, 2022

I don't understand why this is being discussed separately for both document policy and permissions policy and maybe I just overlooked it but I can't see a discussion of moving to PP in WICG/document-policy#39.

Intuitively it feels well placed in document policy and not something for PP.

The explainer says

Different from Permissions-Policy, Document-Policy can not be used to force disabling its child frames' unload handlers recursively. In Chrome, even if the main frame doesn’t use unload handlers at all, the iframe’s unload handlers can block Chrome’s BFCache.

But that's only true for the header, right? I'm not sure to what extent it's supported but the required DP for the iframe should be controllable via the policy attribute.

@fergald
Copy link
Contributor Author

fergald commented Feb 8, 2022

@johannhof Thanks for the feedback,

DP applies only to the specific frame and not to the entire subtree of frames, so a 3rd-party iframe with policy=... set on it could still introduce unload handlers into the page via it's own subframes.

If that makes sense, I will update the explainer to be more explicit.

@fergald
Copy link
Contributor Author

fergald commented Feb 9, 2022

@smaug---- who I missed on the original post.

@johannhof
Copy link
Member

DP applies only to the specific frame and not to the entire subtree of frames, so a 3rd-party iframe with policy=... set on it could still introduce unload handlers into the page via it's own subframes.

I might be misunderstanding something about Document Policy here, but https://github.com/WICG/document-policy/blob/main/document-policy-explainer.md#deeper-nesting describes that mechanism and it seems to contradict what you're saying, and seems to work in the way you'd want for unload handlers.

@fergald
Copy link
Contributor Author

fergald commented Feb 11, 2022

Thanks, I had forgotten about the Sec-Required-Document-Policy header.

This is not actually standardized based on this and testing it out where I see no Sec-Required-Document-Policy header in Chrome or Firefox (but I could be doing it wrong). This looks to be partly implemented in Chrome and disabled by default.

That said, if it is implemented, it would prevent loading of subframe entirely if the required header is not present. So PP applies consistently through the whole sub-tree whereas DP will either

  • work on the outermost frame and do nothing on the rest of the subtree
  • work on the outermost frame and prevent parts of the rest of the subtree from loading

This definitely needs to go into the explainer.

@annevk
Copy link
Member

annevk commented Feb 14, 2022

Permissions Policy isn't really for sandboxing features. It's for delegating the ability to request to use a particular feature. If the nested document already has access to the feature it's a poor fit.

@fergald
Copy link
Contributor Author

fergald commented Feb 21, 2022

Permissions Policy isn't really for sandboxing features. It's for delegating the ability to request to use a particular feature. If the nested document already has access to the feature it's a poor fit.

OK but Document-Policy has rather extreme impact on the subtree (or would if implemented). Do you think we should have a version of Document-Policy can recurse nicely on the subtree without causing pages to to fail to load? Or should we have something else entirely?

@annevk
Copy link
Member

annevk commented Feb 22, 2022

That's kind of the point. We don't want to introduce features that can influence the control flow of cross-origin documents without those documents being okay with that.

@fergald
Copy link
Contributor Author

fergald commented Feb 24, 2022

The principled argument (which I agree with in general) is that site-A should have no control over site-B's code execution and Sec-Required-Document-Policy is the right way to ensure that site-B is not "coerced". This is unimplemented. I would assume that it's unimplemented because no site would ever actually use it - nobody would risk getting empty iframes just to ensure an optimisation, especially when the optimizations are almost always local to the frame and not optimizing probably hurts the site-B as much as site-A. Sadly unload handlers are not local and may not hurt site-B at all.

Separating the implementation (Document-Policy vs Permissions-Policy vs something else) from the principle (site-A should not alter site-B). The argument for making an exception for unload handlers is that they are already being semi-randomly skipped on all mobile browsers and on all WebKit. You could also argue that not running unload is essentially the same as a crash. A weaker argument is that they hurt the whole page, not just the iframe in question.

So, implementation aside, how do you feel about the goal of allowing a frame to stop all of its subframe unload handlers? If you're opposed to that, debating how to do it is not useful.

@annevk
Copy link
Member

annevk commented Mar 1, 2022

I'm rather weary of providing all these tiny cross-origin control flow points, although I'll concede that in the case of unload there's likely several policies in play already.

@fergald
Copy link
Contributor Author

fergald commented Mar 16, 2022

@cdumez @hober Does WebKit have any opinion on this proposal?

If anyone has any other ideas for

  • reducing unload usage
  • allow sites to protect their BFCache hitrate

I would love to hear them.

Fairly long-timescale but if we can reduce unload usage enough then maybe we can get rid of unload and this cross-origin control flow ceases to exist.

@fergald
Copy link
Contributor Author

fergald commented May 9, 2022

I've updated the explainer to add more discussion on the issue of cross-origin control. TL;DR the risk of exploit seems very low, pre-existing and document-domain and sync-xhr had similar risks.

@annevk
Copy link
Member

annevk commented May 9, 2022

Note that document-domain and sync-xhr don't have cross-browser support for this reason.

@jeffkaufman
Copy link

Looking at this from the perspective of an ad server, this would be a useful tool for two reasons:

  • When putting an ad on the page, we would like to minimize its impact on the rest of the page. For example, we typically set sandbox flags that prohibit disruptive actions, such as locking the screen orientation or prompting the user to start a presentation. Marking an ad iframe as disallowing unload allows us to ensure that it doesn't prevent the publisher page from participating in the back-forward cache.

  • If browsers are eventually going to fully stop respecting unload, this functionality would let us improve the transition. For example, we could gradually roll the feature out in advance of the browser-mediated deprecation, and have the ability to temporarily restore full functionality for partners who hadn't realized they were still depending on deprecated functionality.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 27, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 27, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 28, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 29, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 2, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 8, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 9, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750822
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032917}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750822
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032917}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750822
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032917}
@nicjansma
Copy link

From the perspective of a company that provides a security product that helps protect against malicious activity, a Permissions Policy blocking unload handlers would be very useful.

Our product needs to monitor unload event activity. If this was enabled, it would prevent malicious scripts from exploiting the unload event to evade our security product's tracking.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 5, 2022
…testonly

Automatic update from web-platform-tests
Adds `unload` to Permissions-Policy.

This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750822
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032917}

--

wpt-commits: c4ad1de47c85a65efde93aae9362359475fac31a
wpt-pr: 35242
@fergald
Copy link
Contributor Author

fergald commented Sep 29, 2022

This is now available as an Origin Trial in Chromium 107 (entering Beta shortly). If you would like to try it out, docs are here and you can get a token.

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This policy controls whether unload handlers can be set.

See w3c/webappsec-permissions-policy#444

Bug: 1324111
Change-Id: Ia7c418e7ca3131f5102fde407011e00048a94182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750822
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032917}
NOKEYCHECK=True
GitOrigin-RevId: 5c436e20b6d923241eadd2afe8b846ed32d46eea
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

No branches or pull requests

7 participants
@jeffkaufman @nicjansma @annevk @johannhof @fergald and others