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

Don't set focus to text fragments across origins #227

Open
zcorpan opened this issue Jun 22, 2023 · 5 comments
Open

Don't set focus to text fragments across origins #227

zcorpan opened this issue Jun 22, 2023 · 5 comments

Comments

@zcorpan
Copy link

zcorpan commented Jun 22, 2023

https://wicg.github.io/scroll-to-text-fragment/#issue-e253a983 says

Run the focusing steps for target, with the Document’s viewport as the fallback target.
(Issue 5) Implementation note: Blink doesn’t currently set focus for text fragments, it probably should? TODO: file crbug.

and https://web.dev/text-fragments/#privacy says

Since a naive Text Fragments implementation might decide that a successful match should cause a focus switch, on evil-ads.example.com I could listen for the blur event and thus know when a match occurred. In Chrome, we have implemented Text Fragments in such a way that the above scenario cannot happen.

This suggests the spec here is wrong and focusing target, at least across origins, would be a privacy leak.

@bokand
Copy link
Collaborator

bokand commented Jun 22, 2023

I think the real mitigation here is that scrolling won't occur if the initiator of the navigation isn't same-origin:

If the navigationParam’s request has a sec-fetch-site header and its value is "same-origin" set allow text fragment scroll to true and abort these sub-steps.

However, I don't feel too strongly about this since I'm not sure applying focus is super useful anyway (we do set sequential focus for keyboard navigation and accessibility, which are very useful, but I think those aren't programmatically detectable?).

@annevk
Copy link
Collaborator

annevk commented Jun 28, 2023

"Focusing steps" takes an element or navigable, so it seems to me there's a logic error here.

@bokand
Copy link
Collaborator

bokand commented Jul 4, 2023

The spec adds a monkey patch to make target the lowest-common-ancestor Element when scrolling to a text-fragment Range.

@bokand
Copy link
Collaborator

bokand commented Dec 13, 2023

Hmm, taking a closer look - it seems Chrome doesn't actually apply focus, nor does Safari. I can't think of how applying focus would be useful here and given the potential for leaks it adds I'd err to avoiding the focus steps (for text directives) in the spec. Any objections?

@zcorpan
Copy link
Author

zcorpan commented Apr 24, 2024

@jnjaeschke what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants