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

Ensure restrictions on text directive scrolling match reality #240

Open
bokand opened this issue Nov 21, 2023 · 1 comment
Open

Ensure restrictions on text directive scrolling match reality #240

bokand opened this issue Nov 21, 2023 · 1 comment

Comments

@bokand
Copy link
Collaborator

bokand commented Nov 21, 2023

There's a few places where Chrome's implementation has drifted from the spec. In particular, Chrome allows text directives in iframes, as long as the initiator is same origin with the iframe's document.

Another place where I need to take a closer look is restrictions around same-document navigations.

More generally, we need to survey what other implementations are doing, ensure there's good browser test coverage and that the spec is up-to-date with real-world behavior.

@bokand
Copy link
Collaborator Author

bokand commented Nov 23, 2023

Some initial findings based on this test page:

Same Document

  • Indeed, the spec implicitly forbids same-document scrolling by virtue of the "text fragment user activation" bit being set in the document creation steps, but:
    • In Main Frame both Chrome and Safari allow same-document scrolling, even without user-activation from same-origin initiator
    • Safari allows even a cross-origin initiator to scroll the main frame to a text directive, without a user activation

Subframes

  • The spec blocks all text directive navigations in subframes
    • Safari does too
    • Chrome allows iframes to navigate if the initiator is same-origin
      • No user gesture requirement for same-document navigations

Opener Isolation

  • Safari seems to not implement this

bokand added a commit to bokand/ScrollToTextFragment that referenced this issue Nov 29, 2023
The main change in this commit is that enables same-document navigations
(with restrictions) by moving the security checks to also happen from
the navigate to fragment. As part of this, we do a fairly substantial
clean up and refresh of the 'restricting a text fragment' section. The
summarized high level changes:

  * Split out the restrictions into a single set of 'check if a text
    directive can be scrolled' steps, taking the necessary input as
    parameters.
  * Remove the 'allow text fragment scroll` flag on Document, instead
    computing this value and passing it through various steps into
    'scroll to the fragment'.
  * The restriction is placed only on the 'scroll to the fragment'
    steps, meaning the text directive is still the 'indicated part' and
    can remain highlighted.

Partially addresses WICG#240
bokand added a commit that referenced this issue Dec 13, 2023
* Use userInvolvement instead of sec-fetch-site

sec-fetch-site was being checked for 'none' to indicate that a navigation was
initiated from browser UI. However, we cannot inspect request headers from this
part of the algorithm.

Instead, the navigate algorithm now has a userInvolvement parameter which
provides this information explicitly. Plumb that into navigation params and use
it instead.

Additionally, this change removes the top-level browsing context check from the
document's text directive user activation flag since that's a confusing place to
check it. Instead, move it to where this flag is being read and remove a
(now-obviously) redundant check below.

* Use initiatorOrigin instead of sec-fetch-site

`sec-fetch-site: same-origin` was being checked to tell if a navigation was
initiated by a different origin. However, request headers can't be inspected at
this point of the algorithm.

Plumb through the initiatorOrigin parameter when loading a document and compare
that with navigation params's origin field, using the `is same site` steps.

* Enable same-document navigations

The main change in this commit is that enables same-document navigations
(with restrictions) by moving the security checks to also happen from
the navigate to fragment. As part of this, we do a fairly substantial
clean up and refresh of the 'restricting a text fragment' section. The
summarized high level changes:

  * Split out the restrictions into a single set of 'check if a text
    directive can be scrolled' steps, taking the necessary input as
    parameters.
  * Remove the 'allow text fragment scroll` flag on Document, instead
    computing this value and passing it through various steps into
    'scroll to the fragment'.
  * The restriction is placed only on the 'scroll to the fragment'
    steps, meaning the text directive is still the 'indicated part' and
    can remain highlighted.

Partially addresses #240
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

1 participant