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

Windows opened via <a target=_blank> should not have an opener by default #4078

Closed
cdumez opened this issue Oct 10, 2018 · 19 comments
Closed

Windows opened via <a target=_blank> should not have an opener by default #4078

cdumez opened this issue Oct 10, 2018 · 19 comments

Comments

@cdumez
Copy link

@cdumez cdumez commented Oct 10, 2018

Windows opened via <a target=_blank> currently have an opener unless specified otherwise via rel="noopener". While most developers expect a window opened via window.open() to have an opener, I believe most do not necessarily realize the same applies to windows opened via <a target=_blank>. It is unfortunately too rare to see Web developers use rel="noopener" in cases where it can and should definitely be used, even on top Web sites (see for example articles on Google News).

As a result, I would argue that we should switch the default behavior so that windows opened via <a target=_blank> do not get an opener, except if the developers explicitly asks for one via rel="opener".

This change would be beneficial for security: both in the general Web security aspect (e.g. not being able to navigate one's opener) but also for process isolation. For engines such as WebKit which do not currently support out of process iframes, this would allow for process swapping in more cases.

I understand this change could be risky for a compatibility point of view. It is hard - however - for me to tell how risky it would really be. Also, there would still be a way for Web content to get an opener if they really wanted to, either by using window.open(), or rel="opener".

We are considering experimenting with this new behavior in WebKit (via Safari Technology Preview) so I wanted to file this issue beforehand to gather some feedback and get a feel of how other browser vendors feel about this. For example, if you know of a good reason why we should definitely not do this, we'd really like know :)

@cdumez
Copy link
Author

@cdumez cdumez commented Oct 10, 2018

Loading

@geoffreygaren
Copy link

@geoffreygaren geoffreygaren commented Oct 10, 2018

There's a lot of discussion online by web developers who were surprised by the default opener policy. It's a common source of security bugs.

https://news.ycombinator.com/item?id=11553740
https://www.reddit.com/r/programming/comments/4zikpx/the_target_blank_vulnerability_by_example/
https://mathiasbynens.github.io/rel-noopener/

Loading

@domenic
Copy link
Member

@domenic domenic commented Oct 10, 2018

Previous discussions wherein we considered variants of this too risky to attempt: #2119 #2047 . I'm not sure anyone considered limiting it only to target=_blank.

If you search for "opener" on this tracker there are a bunch of still-open issues. Some are in this area but a lot seem to be about the spec being unclear/broken around browsing context names and opener-disowning.

/cc @whatwg/security and @csreis

Loading

@annevk
Copy link
Member

@annevk annevk commented Oct 10, 2018

To rephrase OP, the suggestion here would be that target=_blank results in a new top-level browsing context (i.e., not an auxiliary browsing context).

(I like it, if web-compatible.)

Loading

@csreis
Copy link

@csreis csreis commented Oct 10, 2018

I'd love to see this as well. Looking at #2047, I guess the main question is whether OAuth flows use target=_blank or not (and if so, if they could be updated to use named windows). /cc @hillbrad for thoughts on that, and @mikewest for thoughts on making noopener the default behavior in the target=_blank case.

Loading

@cdumez
Copy link
Author

@cdumez cdumez commented Oct 10, 2018

I did some very early testing with regards to OAuth, I tried both Facebook and Google Oauth on several top sites and both still seem to work (the newly opened window still gets an opener). They are likely relying on window.open() rather than <a target=_blank>. This seems encouraging.

URLs tested:

Loading

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Oct 10, 2018

This sounds like an interesting proposal. Would the newly opened thing also not be targetable by name from the page that opened it, to sever links in both directions?

Loading

@geoffreygaren
Copy link

@geoffreygaren geoffreygaren commented Oct 10, 2018

@bzbarsky Yes the proposal is to sever the link in both directions. I think that’s what the “top-level browsing context” phrase attempts to specify.

Note that the newly opened thing initially has no targetable name (because it is _blank), so observable cases of this detail are probably rare — but I guess the loaded page could in theory set window.name, making the behavior observable.

Loading

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Oct 10, 2018

but I guess the loaded page could in theory set window.name

Right, that would be the case to worry about.

Loading

@annevk
Copy link
Member

@annevk annevk commented Oct 11, 2018

New top-level browsing context was indeed meant to indicate a new name bucket as well.

Digression on name buckets

There are some oddities with rel=noopener and names: #1826 (I think that's a bug and we should always create a new top-level browsing context when it's used).

Defining the name bucket is #1440 and to some extent #313 is also about that. For both having a complete description of what browsers do today would help a lot (or pointers to the relevant algorithms).

Loading

@cdumez
Copy link
Author

@cdumez cdumez commented Oct 15, 2018

Will experiment with the new behavior in WebKit via
https://bugs.webkit.org/show_bug.cgi?id=190481.

Loading

kisg pushed a commit to paul99/webkit-mips that referenced this issue Oct 15, 2018
https://bugs.webkit.org/show_bug.cgi?id=190481

Reviewed by Alex Christensen.

Source/WebCore:

As an experiment, try and make it so that target=_blank on anchors implies `rel=noopener` for improved security.
WebContent can then request an opener relationship by using `rel=opener` instead.

This change was discussed at:
- whatwg/html#4078

We want to attempt this change is STP to see if it is Web-compatible. Preliminary testing seems to indicate
that OAuth workflows still work.

* html/HTMLAnchorElement.cpp:
(WebCore::HTMLAnchorElement::parseAttribute):
(WebCore::HTMLAnchorElement::handleClick):
(WebCore::HTMLAnchorElement::effectiveTarget const):
* html/HTMLAnchorElement.h:
* page/RuntimeEnabledFeatures.h:
(WebCore::RuntimeEnabledFeatures::setBlankAnchorTargetImpliesNoOpenerEnabled):
(WebCore::RuntimeEnabledFeatures::blankAnchorTargetImpliesNoOpenerEnabled const):

Source/WebKit:

* Shared/WebPreferences.yaml:

Tools:

Add API test coverage to make sure we can now swap process when target=_blank
is specified on an anchor but rel=noopener is not.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

LayoutTests:

Update existing tests to reflect behavior change.

* TestExpectations:
* http/tests/navigation/no-referrer-reset.html:
* http/tests/security/resources/referrer-policy-redirect-link.html:
* http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html:
* http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html:
* http/tests/security/xssAuditor/link-opens-new-window.html:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237144 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@cdumez
Copy link
Author

@cdumez cdumez commented Oct 25, 2018

Safari Technology Preview 68 contains this behavior change. https://webkit.org/blog/8475/release-notes-for-safari-technology-preview-68/

Loading

@sashafirsov
Copy link

@sashafirsov sashafirsov commented Oct 25, 2018

The target name concept for cross-documents control, communication, etc is broken from very beginning as given a global naming scope for all sites. I think this security gap could be solved by applying the app scope on top of name. So when window is opened or addressed the name is used only inside of this scope. Other little patches like adding extra parameter to anchors are the scotch patches on bumper. The problem should be addressed on fundamental principles level.
If some one curious, defined scoping model is implemented in epa-wg/embed-page. Of course it is a draft to start with.

Loading

@annevk
Copy link
Member

@annevk annevk commented Nov 15, 2018

@cdumez was it intentional to exclude <area> and <form> btw? It would be nice to be consistent among those I think.

Loading

@cdumez
Copy link
Author

@cdumez cdumez commented Nov 15, 2018

Yes, it was intentional, to limit the risk of Web breakage. Although I agree with you that all these should be consistent, I'd like to do it incrementally so we can identify which ones are OK (or not) in terms of Web compatibility.

For now, there was no reported regression that I know of from changing only <a>'s behavior in STP 68, which is encouraging.

I am not super worried about <area> but I do believe that <form> brings additional compatibility risk.

Loading

@annevk
Copy link
Member

@annevk annevk commented Nov 23, 2018

https://bugzilla.mozilla.org/show_bug.cgi?id=1503681 adds wpt tentative tests for <a> and <area>. We're also gonna try <form>, but separately. Fixing #2983 would be good for <form> too, if we indeed keep the rel=opener pattern.

Another thing that came up is that adding noreferrer to window.open() might be good.

Loading

@mystor
Copy link
Contributor

@mystor mystor commented Nov 23, 2018

One concern which came up in our bug (which @annevk has linked above) was the interaction of multiple opener noopener and noreferrer flags. We could:

  1. Apply the rules in order, so "opener noopener" is noopener, while "noopener opener" is opener. This has an interesting interaction for "noreferrer opener" which would be that the referrer wouldn't be sent, but the opener would be present.
  2. Specify a pecking order, and always use the most specific one, for example "opener" < "noopener" < "noreferrer". This would mean that if "noreferrer" is written, it wins, and "opener" only wins if nothing is written.

Loading

@annevk
Copy link
Member

@annevk annevk commented Nov 26, 2018

I tried to clarify the semantics of rel in d324aeb. I don't think we should do 1 as currently alternate stylesheet and stylesheet alternate are identical. It seems weird to let order matter sometimes.

Loading

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 27, 2018
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Nov 27, 2018
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 28, 2018
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 28, 2018
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Nov 29, 2018
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Nov 29, 2018
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 99ae47766ba9732664c0f828126beadb37220be4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 7a19fe6be68ce356cf2d94b09ad458d8c517bdcf
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 3451e101fd67496305b366e5d60744317a60a54f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 99ae47766ba9732664c0f828126beadb37220be4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 7a19fe6be68ce356cf2d94b09ad458d8c517bdcf
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 3451e101fd67496305b366e5d60744317a60a54f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 99ae47766ba9732664c0f828126beadb37220be4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 7a19fe6be68ce356cf2d94b09ad458d8c517bdcf
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…rea elements when no rel attribute is set, r=nika

In case anchor and area elements have target=_blank and no rel=opener/noopener,
this patch makes so that rel=noopener is implied. This feature is behind pref
'dom_targetBlankNoOpener_enabled'.

See: whatwg/html#4078

UltraBlame original commit: 3451e101fd67496305b366e5d60744317a60a54f
mikewest added a commit to web-platform-tests/wpt that referenced this issue Jan 24, 2020
whatwg/html#4078 changed `opener` semantics to require
explicit transfer to `_blank` targets via `rel=opener`. This patch brings this test into
line with that behavior.
mikewest added a commit to web-platform-tests/wpt that referenced this issue Jan 24, 2020
…21405)

whatwg/html#4078 changed `opener` semantics to require
explicit transfer to `_blank` targets via `rel=opener`. This patch brings this test into
line with that behavior.
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Feb 2, 2020
…boxed-popup.tentative.html`., a=testonly

Automatic update from web-platform-tests
Fix `sandbox-disallow-scripts-via-unsandboxed-popup.tentative.html`. (#21405)

whatwg/html#4078 changed `opener` semantics to require
explicit transfer to `_blank` targets via `rel=opener`. This patch brings this test into
line with that behavior.
--

wpt-commits: ccb23a9c120c02390a704ea40bb9425ef74ea03b
wpt-pr: 21405
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 5, 2020
…boxed-popup.tentative.html`., a=testonly

Automatic update from web-platform-tests
Fix `sandbox-disallow-scripts-via-unsandboxed-popup.tentative.html`. (#21405)

whatwg/html#4078 changed `opener` semantics to require
explicit transfer to `_blank` targets via `rel=opener`. This patch brings this test into
line with that behavior.
--

wpt-commits: ccb23a9c120c02390a704ea40bb9425ef74ea03b
wpt-pr: 21405
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=190481

Reviewed by Alex Christensen.

Source/WebCore:

As an experiment, try and make it so that target=_blank on anchors implies `rel=noopener` for improved security.
WebContent can then request an opener relationship by using `rel=opener` instead.

This change was discussed at:
- whatwg/html#4078

We want to attempt this change is STP to see if it is Web-compatible. Preliminary testing seems to indicate
that OAuth workflows still work.

* html/HTMLAnchorElement.cpp:
(WebCore::HTMLAnchorElement::parseAttribute):
(WebCore::HTMLAnchorElement::handleClick):
(WebCore::HTMLAnchorElement::effectiveTarget const):
* html/HTMLAnchorElement.h:
* page/RuntimeEnabledFeatures.h:
(WebCore::RuntimeEnabledFeatures::setBlankAnchorTargetImpliesNoOpenerEnabled):
(WebCore::RuntimeEnabledFeatures::blankAnchorTargetImpliesNoOpenerEnabled const):

Source/WebKit:

* Shared/WebPreferences.yaml:

Tools:

Add API test coverage to make sure we can now swap process when target=_blank
is specified on an anchor but rel=noopener is not.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

LayoutTests:

Update existing tests to reflect behavior change.

* TestExpectations:
* http/tests/navigation/no-referrer-reset.html:
* http/tests/security/resources/referrer-policy-redirect-link.html:
* http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html:
* http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html:
* http/tests/security/xssAuditor/link-opens-new-window.html:


Canonical link: https://commits.webkit.org/205518@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@237144 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants