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] Restrict based on new browsing context group #70

Merged
merged 2 commits into from
Dec 2, 2019

Conversation

bokand
Copy link
Collaborator

@bokand bokand commented Nov 19, 2019

Fixes #64

@bokand
Copy link
Collaborator Author

bokand commented Nov 19, 2019

@annevk - took a crack at this, would appreciate if you could take a look and let me know if this is on the right track.

As you noted, I'll still need to change target="_same" to allow noopener but I'll do that in a followup. Spec-wise, I think that's just a small change to the rules for choosing a browsing context to follow the noopener path for _self right?

@annevk
Copy link
Collaborator

annevk commented Nov 19, 2019

It's a little trickier as we don't have infrastructure for replacing the browsing context yet. https://gist.github.com/annevk/6f2dd8c79c77123f39797f6bdac43f3e will add that.

<em>document</em> return true.
4. If <em>document</em>'s <em>browsingContext</em> is a top level browsing context and its
[[HTML#tlbc-group|group]]'s [[HTML#browsing-context-set|browsing context set]]
has length 1 return true.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it guaranteed that a 'noopener' navigation will result in a new browsing context set? Reading #tlbc-group it's not clear to me if e.g. two separate 'noopener' navigations are done to the same origin, could they be placed in the same browsing context set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think if you specify a _blank target and noopener choosing a browsing context says you must create a new group. Without noopener you create an auxiliary browsing context which will be in an existing group. (You do need to specify a _blank target though.

That said, I'm not an expert here, is there something I'm missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, the part I was unclear on was whether a new browsing context was necessarily in a new group (it is).

@bokand
Copy link
Collaborator Author

bokand commented Nov 20, 2019

Thanks, btw you can preview the changes here: http://bokand.github.io/spec.html (not sure if there's a well-beaten path for spec reviews is but my experience is that reviewing the markup diffs is unpleasant)

@nickburris
Copy link
Collaborator

LGTM overall but would like review from @annevk as well :)

@bokand bokand force-pushed the openerRestrictions branch from e84b65f to 2950614 Compare November 27, 2019 17:04
@bokand
Copy link
Collaborator Author

bokand commented Nov 27, 2019

@annevk: ping. Does the updated restriction satisfy your concerns around cross-origin activation? From the spec with changes here:

Should Allow Text Fragment:
  ...
- If incumbentNavigationOrigin is equal to the origin of document return true.
- If document’s browsingContext is a top level browsing context and its group's browsing context set has length 1 return true.
- Otherwise, return false.

Also, w.r.t. target="_self" rel="noopener", could we make a link with a text directive imply noopener semantics? I would think this is preferable to requiring links to add noopener since there are cases where that'll be difficult for the user and generally make this more difficult to use.

e.g. pasted URLs in a doc/chat-app should work but the user doesn't have any control over how those get linkified. Any reason not to have the UA implicitly add noopener in these cases?

@bokand bokand merged commit d919662 into WICG:master Dec 2, 2019
@annevk
Copy link
Collaborator

annevk commented Dec 3, 2019

That might be doable, yes, but it would require patching HTML.

@hober hober mentioned this pull request Dec 4, 2019
5 tasks
@bokand
Copy link
Collaborator Author

bokand commented Dec 5, 2019

Also, w.r.t. target="_self" rel="noopener", could we make a link with a text directive imply noopener semantics? I would think this is preferable to requiring links to add noopener since there are cases where that'll be difficult for the user and generally make this more difficult to use.

That might be doable, yes, but it would require patching HTML.

Actually, given that we're checking we're the only browsing context in the browsing context group, I think the mainstream case navigating a simple top-level browsing context (A --> B) with an anchor link is satisfied without it. A will go away as part of the navigation and B will remain the only browsing context). The only time you'd need this is if you have, e.g. a popup window that survives the navigation and keeps A alive. Is my understanding correct?

Given that, I think an implicit behavior would be more magical and would silently break window.opener. Given that either window.opener or the scroll-to-text has to be lost in this case, I think it's better to lose scroll-to-text because it won't affect the page's functionality. Silently losing window.opener when the page expects it could break the page's assumptions.

I think this case will be rare so making authors add rel=noopener is fine.

It might also make sense to have a dev tools warning if scroll-to-text is blocked explaining why.

@annevk
Copy link
Collaborator

annevk commented Jan 13, 2020

Is my understanding correct?

The specification doesn't currently replace the browsing context group for such navigations, so if B opened a popup and then navigated back in history, that popup would have access to A, say. We should probably define things such that replacing actually happens.

@bokand bokand deleted the openerRestrictions branch February 26, 2020 01:03
@bokand
Copy link
Collaborator Author

bokand commented May 21, 2020

Sorry for the delay - picking this back up...

so if B opened a popup and then navigated back in history, that popup would have access to A, say.

Right, but I don't see why that's an issue given our current restrictions - at this point the text fragment in B has already been invoked while A wasn't in the browsing context group and any new navigation in either the main window or popup will now block invoking the text fragment which is intended.

Is your point that if we have target="self" rel="noopener" we could allow invoking the text fragment in more cases? Or is there something else I'm missing?

@bokand
Copy link
Collaborator Author

bokand commented Nov 23, 2020

@annevk Could you elaborate on your concern here? As it stands, if B's popup tries to navigate A, any text fragment invocation will be blocked.

Simply stated, a text-fragment can be invoked from a cross-origin source only if the browsing context is alone in its group (this allows the A->B without replacing the group), meaning the source cannot both control the target's URL and observe the navigation.

Replacing the browsing context group on on self navigations seems like it'd be useful more generally, but it doesn't seem specific to this (and the rule above would be compatible with replacing browsing context groups).

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.

Prevent invocation from popup
3 participants