[wip] adding rel=noopener for link_to when target=blank_ #25548

Jxck · 2016-06-28T03:03:05Z

I'm already discussed on ML here
https://groups.google.com/d/topic/rubyonrails-core/BPGwQXKWsws/discussion
copy are on Summary

Summary

link with target=blank_ will cause some kind of phishing attack known as tabnabbing.
detail of this attacks are described below.

this is caused by window.opener of JavaScript API, and it will prevent by rel=noopener new API.

so I propose adding this attribute to link_to when it given target: "_blank".

link_to "External link", "http://www.rubyonrails.org/", target: "_blank"

<!-- before -->
<a href="http://www.rubyonrails.org/" target="_blank">External link</a>
<!-- after -->
<a href="http://www.rubyonrails.org/" target="_blank" rel="noopener">External link</a>

here is noopener spec.

https://html.spec.whatwg.org/multipage/semantics.html#link-type-noopener

currently implemented by chrome/opera.

http://caniuse.com/#search=noopener

noreferrer is considered altenative of noopener for older browser.
but this cause not to send referrer to server, so it'll cause breakin change for some apps.
noopener is no side effect for apps, without using window.opener ofcourse.

API Change

basically rel=noopener will add when html option has target=_blank by default

link_to("Hello", url_hash, { target: "_blank" })
# <a target="_blank" rel="noopener" href="/">Hello</a>

you can opt out to adding rel=noopener with opener: true option.

option = url_hash.merge({ opener: true })
link_to("Hello", option, { target: "_blank" })
# <a target="_blank" href="/">Hello</a>

thanks
Jxck

rails-bot · 2016-06-28T03:03:08Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Jxck · 2016-06-28T03:16:40Z

this PR is WIP currently because I wanna ask guys about API for this case.

if you give string url, it seems no way to pass opener: true option for opt-out adding rel=noopener.

# how to opt-out from adding `rel=noopener` ?
link_to("Hello", "http://www.example.com", target: "_blank")
<a target="_blank" rel="noopener" href="http://www.example.com">Hello</a>

because current link_to has pattern for no url_options.

# http://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to
link_to(body, url, html_options = {}) ## pattern A (NO URL_OPTIONS !!!)

link_to(body, url_options = {}, html_options = {}) ## pattern B

link_to(options = {}, html_options = {}) do ## pattern C
end

link_to(url, html_options = {}) do ## pattern D (NO URL_OPTIONS !!!)
end

I think we have these options.

NO noopener for no url_options
- will no breaking, but less meaning for adding default.
Force noopener for no url_options (no way to opt-out)
- safe & meaningful, buthas small possibility to beaking existing app ?
adding one more option to link_to for noopener only for no url_options pattern
- no breaking but too match long argument ?
adding url_options for no url_options pattern
- breaking change

If I missed something for solve this problem, please mention me.
thanks

Jxck

johnpray · 2016-08-28T03:45:50Z

actionview/lib/action_view/helpers/url_helper.rb

+        end
+
+        def target_blank?(html_options)
+          html_options['target'] && html_options['target'].include?('_blank')


I think the problem noopener solves is present no matter the value of the target attribute. For example, as best I can tell, target="foo" causes the problem just like target="_blank". So perhaps the check here should just be for whether the target attribute has any value.

(Side note: Using values other than _blank for target used to be used for opening the link in a frame with the matching name. I'm not sure if it has any practical use now, but it still works to open a new tab, at least in Chrome.)

Jxck · 2016-08-28T11:18:02Z

@LouieGeetoo
that's make sense.
but we need to solve option problem as I described. than I'll fix that.

eileencodes · 2016-08-29T13:11:09Z

actionview/lib/action_view/helpers/url_helper.rb

+      # but you can disable <tt>rel="noopener"</tt> with using <tt>opener: true</tt>.
+      #
+      #   link = link_to("Hello", "http://www.example.com", target: "_blank")
+      #   # => <a target="_blank" rel="noopener" href="http://www.example.com">Hello</a>


Should this example have opener true like: link = link_to("Hello", "http://www.example.com", target: "_blank", opener: true)? Right now this example is the same as the first example.

my mistake, I'll fix it.

Jxck · 2016-08-29T16:24:27Z

@eileencodes
thanks for review, but how do you think about this problem ?
#25548 (comment)

johnpray · 2016-08-29T17:18:55Z

@Jxck: Regarding the API question: I'm no authority on this (I only happened upon this issue because I was looking into the whole noopener problem overall), but would it make sense to make it an option within html_options instead of url_options? It's really affecting the HTML tag rather than the URL, after all.

If the developer is explicitly setting rel in html_options, maybe you can just pass that value through unchanged, assuming that such a developer will know to add noopener manually. That way the dev can override the default simply by doing something like link_to "http://example.com", target: "_blank", rel: ""

Or maybe there shouldn't be an easy way to override it, since it's a security concern and we can perhaps assume that 99.9% of users will want it.

Of course, I'm not sure whether either of those assumptions matches the Rails way of doing things.

Jxck · 2016-08-30T00:37:38Z

@LouieGeetoo

in that case, how this will work ?

link = link_to("Hello", url_hash, { target: "_blank", rel: "noreferrer" })
# 1, <a target="_blank" rel="noreferrer" href="/">Hello</a>
# 2, <a target="_blank" rel="noreferrer noopener" href="/">Hello</a>

from point of motivation to adding this feature, I expect 2.
but in that case, we can't describe 1 case because there is no way to opt-out.

if this should be result to 1, developer always care about adding noopener by themself every link_to which has another rel option. but it's seems frequent, so merit of this feature will decrease.

totally my point of view, opting-out from noopener should be url_options which is option for link_to behavior, not to html_option which is directory affect to html attributes.

connorshea · 2016-10-10T06:09:24Z

This seems like a good security improvement, would it be possible to backport to 5.0.x or is this considered too much of a new feature/breaking change for that?

jvenezia · 2016-10-10T10:18:06Z

Firefox does not support the noopener tag, should we use rel="noopener noreferrer" by default for full coverage?

I understand that this could be an issue because referrer will not be sent so could we propose this as a Rails option?

jvenezia

Not working if "_blank" is a symbol: target: :_blank

jvenezia · 2016-10-10T12:52:41Z

actionview/lib/action_view/helpers/url_helper.rb

@@ -556,10 +570,27 @@ def current_page?(options)
      end

      private
+        def default_noopener(rel)


Breaks if rel already have a symbol (rel: :nofollow).

could be replaced by something like that:

rel.to_s.split(' ').push('noopener').uniq

rafaelfranca

Could you update the PR with the proposed changes and add a CHANGELOG entry?

Other than that I like the idea. Thank you so much for taking your time to improve the security of the framework.

rafaelfranca · 2016-10-10T17:27:27Z

actionview/lib/action_view/helpers/url_helper.rb

@@ -98,6 +98,10 @@ def _filtered_referrer # :nodoc:
      #   the link. The drivers each provide mechanisms for listening for the
      #   completion of the Ajax request and performing JavaScript operations once
      #   they're complete
+      # * <tt>opener: false</tt> - If link has <tt>target="blank_"</tt> attribute,


Should be _blank

rafaelfranca · 2016-10-10T17:27:36Z

actionview/lib/action_view/helpers/url_helper.rb

+      # * <tt>opener: false</tt> - If link has <tt>target="blank_"</tt> attribute,
+      #   generated html has <tt>rel="noopener"</tt> by default for avoid touching <tt>window.opener</tt>
+      #   from opened tab. If this option is <tt>true</tt>, it will remove <tt>rel="noopener"</tt>
+      #   from html even if link has <tt>target="blank_"</tt> attribute.


Should be _blank

rafaelfranca · 2016-10-10T17:28:00Z

actionview/lib/action_view/helpers/url_helper.rb

@@ -556,10 +570,27 @@ def current_page?(options)
      end

      private
+        def default_noopener(rel)


Jxck · 2016-10-11T00:45:54Z

@rafaelfranca @jvenezia @connorshea
I think we don't solve problem on #25548 (comment)
after solve this API problem, I'll update this PR.

rafaelfranca · 2016-10-11T00:51:02Z

Would not link_to("Hello", "http://www.example.com", target: "_blank", opener: true) work? I don't see why not.

Jxck · 2016-10-11T01:11:53Z

@rafaelfranca which you mean 3 or 4 ? on (#25548 (comment))

I'll paste the pattern and choice again.

patterns

we are talking about A to C

# http://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to
link_to(body, url, html_options = {}) ## pattern A (NO URL_OPTIONS !!!)

link_to(body, url_options = {}, html_options = {}) ## pattern B

link_to(options = {}, html_options = {}) do ## pattern C
end

link_to(url, html_options = {}) do ## pattern D (NO URL_OPTIONS !!!)
end

choices

NO noopener for no url_options
Force noopener for no url_options (no way to opt-out)
adding one more option to link_to for noopener only for no url_options pattern
adding url_options for no url_options pattern

if you mean 3

one more extra option for noopener ({ opener: true}) to pattern A, D.
in that case, how to add it to B, C ?
I think we can merge {opener : true} into url_options in B, C,
but adding extra option for noopener to B, C too ?

if you mean 4

adding url_options to end of A, D arguments
and merge {opener: true} into url_options.
in that case, order of url_options and html_options is
opposite between A, D and B, C.

rafaelfranca · 2016-10-11T01:15:28Z

I mean taking opener: true as part of the html_options, so it is an new options that you didn't enumerate.

Jxck · 2016-10-11T01:24:27Z

humm, I think html_options should encoded directory into HTML text.
so in current semantics opener: true should be <a opener="true">.
and you saying make opener as specialized parameter in html_options for avoiding encode.
for example, how do we compare if opener="true" option will adding HTML standards ?
if that happens, it has possibility of breaking the web not only breaking the Rails API isn't it ?

I can't agree that.

rafaelfranca · 2016-10-11T01:29:46Z

We can always filter the options before encoding to HTML attributes. I still think that is the best way to proceed.

Jxck · 2016-10-11T01:30:49Z

again

how do we compare if opener="true" option will adding HTML standards ?

matthewd · 2016-10-11T01:31:48Z

I'm somewhat concerned about us mutilating a rel value that's been explicitly set

connorshea · 2016-10-11T01:34:13Z

@matthewd should we give a security warning instead of overwriting it if its already been defined?

rafaelfranca · 2016-10-11T01:34:33Z

I'm somewhat concerned about us mutilating a rel value that's been explicitly set

Yeah, that may happen but we can detect if rel was explicitly set and not apply noopener in that case.

how do we compare if opener="true" option will adding HTML standards ?

You just remove opener form the html_options before encoding.

connorshea · 2016-10-11T01:36:00Z

@rafaelfranca I think the concern is that currently all options in the html_options hash map directly to HTML attributes of the same name, so this would be out-of-the-norm.

EDIT: Actually that's incorrect, we already have some options that do not map directly.

rafaelfranca · 2016-10-11T01:37:33Z

That is not a concern in my option data and aria also have special meaning inside html_options. We do transform it before making attributes so it is fine to do the same with opener

matthewd · 2016-10-11T01:39:42Z

method and remote already get full transformative treatment

Jxck · 2016-10-11T01:41:54Z

You just remove opener form the html_options before encoding.

no, I'm asking is

if we add opener: true for avoiding rel="noopener" as you sed,
and after that if HTML standards adding opener attributes who has a "true" as attribute value.
and at that time, if we see code with opener: true in html_options.

which we should out ?

<a target="_blank"> (understand opener: true as opt-out noopener)
<a opener="true" target="_blank" rel="noopener"> (under stand opener: true as html attributes, default noopener)

or user point of view,
how to add opener=true attributes but avoid rel=noopenerattribute ?

rafaelfranca · 2016-10-11T01:44:43Z

Let's not care about future of HTML, if they add a opener attribute we deal with it later. We already have this same problem with method and remote and data and aria.

Jxck · 2016-10-11T01:49:13Z

Let's not care about future of HTML

everytime, that cause the breaking the web isn't it ?

connorshea · 2016-10-11T01:50:39Z

@Jxck I agree with the concern, but I think that's a discussion for a different thread.

Jxck · 2016-10-11T01:51:48Z

@connorshea which thread should we do that ?? (I'm talking about this issue)

rafaelfranca · 2016-10-11T01:52:31Z

everytime, that cause the breaking the web isn't it ?

It is not breaking anything because there is nothing to break right now. There is no opener attribute in the HTML spec and it may not ever exist, if it exist we can deal with it when we need to.

rafaelfranca · 2016-10-11T01:55:10Z

Ok, back to constructive discussion, could you make the changes that were requested? Otherwise I'll do myself.

Jxck · 2016-10-11T01:58:49Z

It is not breaking anything because there is nothing to break right now. There is no opener attribute in the HTML spec and it may not ever exist, if it exist we can deal with it when we need to.

standard case of you thought cause like this.
https://esdiscuss.org/topic/having-a-non-enumerable-array-prototype-contains-may-not-be-web-compatible

rafaelfranca · 2016-10-11T02:03:12Z

Ok. I'm out of this discussion. @matthewd feel free to merge it if you agree. Otherwise I'll close this in the next rails-bot iteration.

Jxck · 2016-10-11T03:00:48Z

@rafaelfranca @matthewd I found another choice.

I heard that data has already same problem, so how about adding data-opener property for rails specific custom attributes ?
(note that data-* is for custom attributes and we can define our free attributes safely)

in that case, data-opener: true as html_option can safely encode into <a data-opener=true>, and Rails should make that a way to opt-out rel=noopener.
it solves problem I already mention (it doesn't break anything if HTML have opener option as standard).

only other consider is data-opener is conflict with other framework who use data-* too.
so I recommend to save data-rails-* for Rails specific attributes, and use this in future use.

so wrap them all data-rails-opener: true for html_option is way to opt-out adding rel=noopener.

matthewd · 2016-10-11T03:56:53Z

No need to wait for the bot.

Jxck · 2016-10-11T03:57:21Z

or choice with small breaking change is to add rel=noopene to all link_to with target=_blank as default. and could only opt-out via url_options.

this mean, someone who has contents with depend on opener should update their contents when update rails with fixing link_to which has no url_options pattern to with url_options pattern.

contents which depend on opener seems so few.
and update rails always require few update.

jvenezia · 2016-10-12T12:20:06Z

@rafaelfranca @matthewd So do you plan to do this yourself? I think this feature is a real help for Rails security.

I made a gem adding this feature, i'll be using it in the meantime.

https://github.com/jvenezia/safe_target_blank

link_to 'Safe', 'safe.io', taget: :_blank
#=> '<a target="_blank" rel="noopener noreferrer" href="safe.io">Safe</a>'

link_to 'Safe', 'safe.io', taget: :_blank, referrer: true
#=> '<a target="_blank" rel="noopener" href="safe.io">Safe</a>'

[wip] adding rel=noopener for link_to when target=blank_

d46905a

rails-bot assigned matthewd Jun 28, 2016

maclover7 added the actionview label Jun 28, 2016

johnpray reviewed Aug 28, 2016
View reviewed changes

eileencodes reviewed Aug 29, 2016
View reviewed changes

jvenezia suggested changes Oct 10, 2016

View reviewed changes

jvenezia reviewed Oct 10, 2016

View reviewed changes

rafaelfranca requested changes Oct 10, 2016

View reviewed changes

matthewd closed this Oct 11, 2016

michald mentioned this pull request Feb 16, 2017

Add rel="noopener" to a link by default #28035

Closed

[wip] adding rel=noopener for link_to when target=blank_ #25548

[wip] adding rel=noopener for link_to when target=blank_ #25548

Conversation

Jxck commented Jun 28, 2016

Summary

API Change

rails-bot commented Jun 28, 2016

Jxck commented Jun 28, 2016 • edited Loading

johnpray Aug 28, 2016 • edited Loading

Choose a reason for hiding this comment

rafaelfranca Oct 10, 2016

Choose a reason for hiding this comment

Jxck commented Aug 28, 2016

eileencodes Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

Jxck Aug 29, 2016

Choose a reason for hiding this comment

Jxck commented Aug 29, 2016

johnpray commented Aug 29, 2016

Jxck commented Aug 30, 2016

connorshea commented Oct 10, 2016

jvenezia commented Oct 10, 2016 • edited Loading

jvenezia left a comment

Choose a reason for hiding this comment

jvenezia Oct 10, 2016

Choose a reason for hiding this comment

rafaelfranca Oct 10, 2016

Choose a reason for hiding this comment

rafaelfranca left a comment

Choose a reason for hiding this comment

rafaelfranca Oct 10, 2016

Choose a reason for hiding this comment

rafaelfranca Oct 10, 2016

Choose a reason for hiding this comment

rafaelfranca Oct 10, 2016

Choose a reason for hiding this comment

Jxck commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016

Jxck commented Oct 11, 2016 • edited Loading

patterns

choices

if you mean 3

if you mean 4

rafaelfranca commented Oct 11, 2016

Jxck commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016 • edited Loading

Jxck commented Oct 11, 2016

matthewd commented Oct 11, 2016

connorshea commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016

connorshea commented Oct 11, 2016 • edited Loading

rafaelfranca commented Oct 11, 2016

matthewd commented Oct 11, 2016

Jxck commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016

Jxck commented Oct 11, 2016 • edited Loading

connorshea commented Oct 11, 2016

Jxck commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016

Jxck commented Oct 11, 2016

rafaelfranca commented Oct 11, 2016

Jxck commented Oct 11, 2016

matthewd commented Oct 11, 2016

Jxck commented Oct 11, 2016

jvenezia commented Oct 12, 2016

Jxck commented Jun 28, 2016 •

edited

Loading

johnpray Aug 28, 2016 •

edited

Loading

eileencodes Aug 29, 2016 •

edited

Loading

jvenezia commented Oct 10, 2016 •

edited

Loading

Jxck commented Oct 11, 2016 •

edited

Loading

rafaelfranca commented Oct 11, 2016 •

edited

Loading

connorshea commented Oct 11, 2016 •

edited

Loading

Jxck commented Oct 11, 2016 •

edited

Loading