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

Feature Policy: focus-without-user-activation #4585

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ehsan-karamad
Copy link

@ehsan-karamad ehsan-karamad commented Apr 30, 2019

focus-without-user-activation is a new feature policy that can be used to block
programmatic programmatic focus changes that are not triggered through user activation
(explainer: w3c/webappsec-permissions-policy#304).

The motivation behind the change is to provide better security for websites that embed
content from third parties
(issue: w3c/webappsec-permissions-policy#273).

This change makes modifications to the following focus API

  • autofocus
  • element.focus(options)
  • window.focus()

💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:59 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@ehsan-karamad ehsan-karamad changed the title [WIP] Spec change for focus-without-use-activation [WIP] Feature Policy: focus-without-user-activation May 6, 2019
@ehsan-karamad ehsan-karamad marked this pull request as ready for review May 6, 2019 16:16
@ehsan-karamad
Copy link
Author

@marian-r and @clelland FYI.

@ehsan-karamad ehsan-karamad changed the title [WIP] Feature Policy: focus-without-user-activation Feature Policy: focus-without-user-activation May 7, 2019
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This change makes modifications to the focus processing model such that for a new
focus target that the policy is disabled (i.e., in its node document or its own document if
the target is a browsing context):

  • Focus steps runs as normal but the focusable area is not changed.

This does not appear to be what is done in this patch. Instead, attempts at focusing are entirely aborted; no focusing steps are run.

@@ -53257,6 +53260,11 @@ form.method === input; // => true</code></pre>
<li><p>If <var>target</var>'s <span>active sandboxing flag set</span> has the
<span>sandboxed automatic features browsing context flag</span>, then return.</p></li>

<li><p>If this algorithm is not <span>triggered by user activation</span> and the
policy-controlled feature "<code
data-x="focus-without-user-activation-feature">focus-without-user-activation</code>" is disabled
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the correct verbiage here is

<var>target</var> is not <span>allowed to use</span> the "<code
data-x="focus-without-user-activation-feature">focus-without-user-activation</code>" feature

Cf. https://html.spec.whatwg.org/#playing-the-media-resource:allowed-to-use

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Done.

<li><p>If this algorithm is not <span>triggered by user activation</span> and the
policy-controlled feature "<code
data-x="focus-without-user-activation-feature">focus-without-user-activation</code>" is disabled
in <var>current</var>'s <span>active document</span>, return.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Additional nit: we're slowly moving toward using "then" for new if statements, so "then return". (Like the previous bullet.)

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@ehsan-karamad
Copy link
Author

This change makes modifications to the focus processing model such that for a new
focus target that the policy is disabled (i.e., in its node document or its own document if
the target is a browsing context):

  • Focus steps runs as normal but the focusable area is not changed.

This does not appear to be what is done in this patch. Instead, attempts at focusing are entirely aborted; no focusing steps are run.

Agreed and sorry for misrepresentation. The initial attempt was to modify update focus steps but since the algorithm is invoked from different parts of the spec, and that we only care about the API (now mentioned in the description), I believe the current spec change is more relevant.

@domenic domenic added needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels May 9, 2019
domenic
domenic previously approved these changes May 9, 2019
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

OK, this looks good to me spec-text wise. What remains before we can merge this is multi-implementer interest, and web platform tests.

@ehsan-karamad
Copy link
Author

OK, this looks good to me spec-text wise. What remains before we can merge this is multi-implementer interest, and web platform tests.

Thanks! I will be adding tests for this soon. I have a Chromium CL for that.

@annevk Would you be able to review this change?

@annevk
Copy link
Member

annevk commented May 10, 2019

If the default is not blocking for third-party contexts, this seems like a sandboxing feature, in which case it'll be affected by the planned changes to Feature Policy, right?

@domenic
Copy link
Member

domenic commented May 10, 2019

@ehsan-karamad it appears you force-pushed over my nit fixes, e.g. adding the word "feature" after feature names :(.

@domenic domenic dismissed their stale review May 10, 2019 13:41

My fixes got lost so now there are still nits to fix

@domenic
Copy link
Member

domenic commented May 10, 2019

Also, @ehsan-karamad, you work for Google, which is in the field of web technologies, so it's not appropriate to sign the Participant Agreement as an individual. Instead you need to make your membership in the Google organization public.

@ehsan-karamad
Copy link
Author

ames :(.

Uh very sorry! I still need to find my way around github.

@ehsan-karamad
Copy link
Author

Also, @ehsan-karamad, you work for Google, which is in the field of web technologies, so it's not appropriate to sign the Participant Agreement as an individual. Instead you need to make your membership in the Google organization public.

Thanks. I didn't know it is private by default actually. I made it public. I wouldn't mind un-signing the agreement as an individual if that is a thing.

@ehsan-karamad
Copy link
Author

If the default is not blocking for third-party contexts,

Yes, for now, we are thinking about making this blocking by default in sandboxed subframes. But that said,
it should perhaps be disabled for third-party contexts as well (treat like permissions?). The argument for
it is that the spec for autofocus actually blocks focus when requested by cross-origin.

this seems like a sandboxing feature, in which case it'll be affected by the planned changes to Feature Policy, right?

Yes. If there is no default enforcement to third party, sandbox flag might be the best fit and it will be definitely affected by how Feature Policy is changed.

@domenic
Copy link
Member

domenic commented May 10, 2019

No worries! Could you push an additional commit which ensures that all instances say allowed to use the "x" feature, instead of sometimes allowed to use the "x" or allowed to use "x"?

I've deleted your individual agreement submission; all is good there.

@ehsan-karamad
Copy link
Author

No worries! Could you push an additional commit which ensures that all instances say allowed to use the "x" feature, instead of sometimes allowed to use the "x" or allowed to use "x"?

Done. I pushed a change (not force this time) :).

I've deleted your individual agreement submission; all is good there.

Thanks!

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label May 14, 2019
@annevk
Copy link
Member

annevk commented May 14, 2019

Marked "do not merge yet" as we need to figure out if this is permission-like (disabled in third party contexts unless this is set, enabled otherwise) or sandbox-like (can be disabled anywhere).

cc @clelland

@clelland
Copy link
Contributor

Given that <input autofocus> is currently not expected to work in any cross-origin cases, per spec, I'd love to see programmatic focus-without-user-activation follow the same model (permission-like) so that it can't be used as a way around that restriction. So not just sandboxes, but all third-party content, unless delegated by the parent document.

I don't know yet how web-compatible that change would be, but that's where I would like to see this end up.

@foolip
Copy link
Member

foolip commented Jun 5, 2019

Just to make it explicit, is the idea to keep this PR open until there's multi-vendor support, or also until the change is proven web compatible by shipping it? @annevk, on the multi-vendor support front, who at Mozilla can speak to this?

Aside: the autofocus attribute plus programmatic element.focus() following the same rules is great, and analogous to media autoplay.

@foolip
Copy link
Member

foolip commented Jun 5, 2019

Oops, I spoke to soon in my aside. The proposal says

"focus-without-user-activation", which has a default allowlist of *.

That doesn't match the behavior of the autofocus attribute. Shouldn't the default allowlist be 'self'? If not, there is no web compat concern, so I suspect this is an oversight?

@ehsan-karamad
Copy link
Author

That doesn't match the behavior of the autofocus attribute. Shouldn't the default allowlist be 'self'? If not, there is no web compat concern, so I suspect this is an oversight?

I think this matches @clelland 's suggestion above and would place this policy along with the other permission-like policies.

@omarjcamo
Copy link

What's the state of this policy? Is there any plan to merge this or has the discussion moved somewhere else?

@clelland
Copy link
Contributor

I'd love to be able to ship this; there's a bit more discussion on w3c/webappsec-permissions-policy#273. Maybe 2024 is its year!

@ico85

This comment was marked as spam.

@siliu1
Copy link

siliu1 commented Oct 3, 2024

I don't have editor access to this PR. So I've created #10672 to continue the work to ship this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests
Development

Successfully merging this pull request may close these issues.

8 participants