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

jsx-no-target-blank: Allow to specify allowed hosts #2941

Closed
adarnon opened this issue Mar 6, 2021 · 11 comments
Closed

jsx-no-target-blank: Allow to specify allowed hosts #2941

adarnon opened this issue Mar 6, 2021 · 11 comments

Comments

@adarnon
Copy link

adarnon commented Mar 6, 2021

This rule is great to prevent the target="_blank" security vulnerability, but there are times where you wouldn't want to set noreferrer, such as if you're linking to another website under your control.

For theses cases, instead of manually overriding each line, I think it would be useful to be able to pass an "allowedHosts" option, so the rule would allow static href values that have one of these hosts.

@ljharb
Copy link
Member

ljharb commented Mar 6, 2021

Why would you want target blank when a user is navigating within your own site? Target blank is a bad UX practice overall.

@adarnon
Copy link
Author

adarnon commented Mar 7, 2021

@ljharb Not within the same site, but between different sites that I control. Let's say between app1 and app2 and my landing page. I want to be able to track interactions & conversion, and I know these destinations won't abuse the Referer header.

@ljharb
Copy link
Member

ljharb commented Mar 7, 2021

@adarnon sure, but why would you want those to open in a new window?

In general, forcing a new window/tab on the user is rude; every browser and platform has an "open in new tab/window" idiom that a user who wants a new tab/window already knows how to use (the ones who don't, don't want it).

@adarnon
Copy link
Author

adarnon commented Mar 7, 2021

@ljharb I don't think the purpose here is to discuss UX... There are links, such as a link to my company's Terms of Service or Support page, that in my opinion should open in a new window, because they are displayed during interactions that shouldn't be accidentally disturbed (e.g., while filling out a form)

@ljharb
Copy link
Member

ljharb commented Mar 7, 2021

If the ask is for a new linter rule, then one that enables poor UX means that UX is certainly worth discussing.

Do those cases happen often enough that an eslint override comment is insufficient?

@adarnon
Copy link
Author

adarnon commented Mar 7, 2021

Linters are meant to prevent programmer errors, not poor UX decisions. Target _blank is a necessary tool sometimes, and the purpose of this rule is to prevent security errors that this feature causes, not to prevent its usage overall.

I believe that eslint override comments should only be used when there's absolutely no other choice. They're hard to maintain and using them misses out on a big benefit of linter config - having a uniform way to do things across the entire app.

In this case, I think there's a generic use case for allowing certain hosts through without having to set noreferrer. When the string is static and not dynamic, and the user knows they're passing the call to a domain they control, the risks are minimal. Having an eslint override is dangerous - if sometime in the future someone changes the link href and forgets to remove the comment.

@ljharb
Copy link
Member

ljharb commented Mar 7, 2021

Poor UX decisions are programmer errors :-)

You're totally right about the purpose of this rule.

Having the config you're asking for is equally dangerous - if sometime in the future you no longer control that domain.

@ljharb ljharb closed this as completed Feb 21, 2022
@IanVS
Copy link

IanVS commented Apr 25, 2023

I also am looking for an option like this. I have a few links to docs articles from my app, which I want to open in a new tab to avoid accidentally disrupting the user's work (please don't argue the UX of this, that's not the point). I'm not worried about losing control of my docs website. As it is, I need to manually disable every link to these docs. I'm tempted to disable the rule in general because of the bad DX. I hope you'll reconsider this decision, and permit an option to allowlist particular hostnames.

For now, I modified the rule into a custom rule that adds an allowedHostnames option. Here's a gist in case anyone else might find it useful: https://gist.github.com/IanVS/669b0365551b771d1c0d613807aa6b74

@ljharb
Copy link
Member

ljharb commented Apr 27, 2023

I'm not sure how the "UX of this" isn't the entire point?

@IanVS
Copy link

IanVS commented Apr 27, 2023

The first line of the rule's doc is:

When creating a JSX element that has an a tag, it is often desired to have the link open in a new tab using the target='_blank' attribute.

So, the rule admits that it's a desired thing to do. Then it says the purpose of the rule, (which is not to prevent all use of target=_blank):

This rule aims to prevent user generated link hrefs and form actions from creating security vulnerabilities by requiring rel='noreferrer' for external link hrefs and form actions.

So, what's being proposed here is greater control over what is considered "external", that's all.

@ljharb
Copy link
Member

ljharb commented May 3, 2023

That's fair that the docs imply what you're saying - but the rule's name implies what i'm saying.

allowedHostnames seems interesting, but using URL, which isn't available until node 10 (we support node 4) isn't an option.

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

No branches or pull requests

3 participants