False-positive security precaution warning (javascript: URLs)

[sergei-startsev]

Do you want to request a feature or report a bug?

Report a bug.

What is the current behavior?
React 16.9.0 deprecates javascript: URLs (@sebmarkbage in #15047). It was motivated by preventing XSS vulnerability that can be used by injecting client-side scripts:

<a href={url}>Unsafe Link</a>

The following code cannot be exploited by attackers, it cannot be used to inject XSS:

<a href="javascript:void(0)">Safe Link</a>

React 16.9 reports the security precaution warning for the example:

Warning: A future version of React will block javascript: URLs as a security precaution. Use event handlers instead if you can. If you need to generate unsafe HTML try using dangerouslySetInnerHTML instead. React was passed "javascript:void(0)".

What is the expected behavior?

I would expected that security precaution warnings aren't reported for values that cannot be controlled by attackers.

There were also concerns regarding common patterns like javascript:void(0), see @gaearon comment:

Especially javascript:void(0) seems like it's still pretty common because it's copy pasted from old samples etc. Is it dangerous to whitelist that one? Is it a vector by itself?

If there're tons of reported security issues, you definitely ignore something important.

For reference: Angular’s cross-site scripting security model

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.9.0 is affected. 16.8.6 doesn't report the warning.

[vkurchatkin]

There is no way for React to know if href can be controlled by an attacker or not

[gaearon]

My thinking is that we wouldn't want to add more runtime checks that slow down the app to account for every possible "safe" case. There's many variations on this pattern, and checking all of them will be costly. Checking just a few will annoy people that their particular pattern isn't checked against. So we're choosing to completely ban javascript: URLs. There are easy alternatives for your case, like <a href='#' onClick={e => e.preventDefault()}>, and dangerouslySetInnerHTML can be used if you insist on doing it this way.

[sergei-startsev]

@gaearon If there's no confidence that there're real security issues caused by using javascript: why was it decided to enable it by default in dev mode? Why was it not enabled in Strict Mode to allow developers to fix potential issues step by step?

StrictMode is a tool for highlighting potential problems in an application

I wouldn't like to fix hundreds of warnings by replacing javascript:void(0) to href='#' onClick={e => e.preventDefault()}> all at once. Given that this can be tricky for some cases. There're a lot of legacy apps written with React, why not to allow fix these issues smoothly?

[gaearon]

React has always added deprecation warnings in mainline minor releases by default. This is not a new policy. It's as old as React itself.

It's strict mode that is new — but even there, we can't keep pushing things down to strict mode forever. Strict mode is more about preparing for Concurrent Mode, and is not about security features. If something is too noisy, strict mode won't help it because then nobody would use it. And since we actively want to disable this behavior in next major, it shouldn't be hidden behind a mode.

If there's no confidence that there're real security issues caused by using javascript:

There are very real security issues that are being caused by this in React apps. Yes, there are a few safe concrete examples, but relying on them is a problem by itself. For example, it will prevent you from adopting Trusted Types later (which adds a lot more XSS defense). So we need to weed out this whole pattern from the ecosystem together. This is expected to take some work and we don't want to sugarcoat it.

I wouldn't like to fix hundreds of warnings

There shouldn't be "hundreds of warnings". Have you run this code? The intention is to only warn once:

react/packages/react-dom/src/shared/sanitizeURL.js

Lines 41 to 42 in 868d02d

	} else if (__DEV__ && !didWarn && isJavaScriptProtocol.test(url)) {
	didWarn = true;

If you're seeing hundreds of warnings, something is very wrong and we need more details.

See also this conversation: https://github.com/facebook/react/pull/15047/files#r264320611.

There're a lot of legacy apps written with React, why not to allow fix these issues smoothly?

We're "allowing" that by logging a deprecation. You're free to keep using this feature for now, but you should be aware it would be eventually removed in a major version. Silencing the warning doesn't change that, and makes it harder to find where it needs to be fixed.

[sergei-startsev]

React has always added deprecation warnings in mainline minor releases by default. This is not a new policy. It's as old as React itself.

The deprecation policy doesn't look consistent, some features have been deprecating for years:

The new names like UNSAFE_componentWillMount will keep working in both React 16.9 and in React 17.x.

Others are much faster:

In a future major release, React will throw an error if it encounters a javascript: URL.

---

Strict mode is more about preparing for Concurrent Mode.

I think it has to be clarified better in official guides, there're no references to Concurrent Mode in the doc. If you don't like Strict Mode, you can introduce CSP Mode or Security Mode.

---

There shouldn't be "hundreds of warnings". Have you run this code? The intention is to only warn once.

There're hundreds of warnings in Jest unit tests, I can create a repro repository if you need.

It's even worse in a real application - since you only see the first warning, you never know how many issues you need to fix until you fix them all.

[gaearon]

Others are much faster

If there is a viable migration path, we usually remove the old API in the next major.

In case of UNSAFE lifecycles, note that the old names will be removed in 17. But there is no automated “find and replace” migration path for the long tail which is why UNSAFE lifecycles will keep working.

In case of JavaScript URLs we believe there is a viable migration path. It is easy to find them in your codebase (by searching for javascript: after string literal start). Once you collect the intentional patterns you use, you can mass-replace them with equivalents. It’s also not comparable to class lifecycles in terms of how many you’ll likely have.

There're hundreds of warnings in Jest unit tests, I can create a repro repository if you need.

Jest intentionally doesn’t preserve module state between different test runs.

I understand it’s annoying. But how many actual components is this firing in? You probably have one or two components used all over the place which you can fix once and forget about it. And as I mentioned earlier, this pattern is easy to grep the codebase for so you shouldn’t have troubles finding the callsites.

As the last resort you can always monkeypatch the console in your tests to filter it out until you’re ready to take it on. But I’d like to understand better whether you’re just frustrated by the warning, or if it’s legitimately challenging to fix in your project — and why.

[sergei-startsev]

You probably have one or two components used all over the place which you can fix once and forget about it

Unfortunately this assumption isn't correct.

And as I mentioned earlier, this pattern is easy to grep the codebase for so you shouldn’t have troubles finding the callsites.

It's not true: some hrefs are dynamic, some click callbacks are passed as properties, simple "find and replace" doesn't work here.

As the last resort you can always monkeypatch the console in your tests to filter it out until you’re ready to take it on.

I wouldn't force developers to monkeypatch console, it doesn't looks like a solid solution.

But I’d like to understand better whether you’re just frustrated by the warning, or if it’s legitimately challenging to fix in your project — and why.

There're real XSS issues caused by passing dynamic values to href attribute, e.g.:

<a href={url}>Unsafe Link</a>

And safe, static attributes that cannot be exploited by attackers:

<a href="javascript:...">Safe Link</a>

Ideally I would distinguish static (safe) and dynamic (unsafe) values. If it's not possible in React, I'd like to have a solution that allow me to turn on it and fix issues feature by feature.

There's a real project with ~500.000 loc of jsx, there're ~100 static javascript: void in ~70 components, dynamic values aren't even counted.

[feng-fu]

Same warning, i use <a href="javascript:;"></a> as button in many component.
I just think it is correct.

[sebmarkbage]

I believe that specifying a fake link without a role isn't correct hints for a11y. It should have role="button" or role="link". Once you've done that the next step is making it tabbable. tabIndex="0". Once you've done that, there's not need for the href anymore.

Therefore we recommend this pattern instead:

<div role="link" tabIndex="0">...</div>

or

<div role="button" tabIndex="0">...</div>

[sergei-startsev]

@sebmarkbage There're different styles applied for a link with/without href .

[YassienW]

Ran into this today, i would like to point out that using <a href="#"></a> is ugly, and adds a useless entry to react-router's history. Excluding href isn't really a solution since it styles differently as @sergei-startsev pointed out

[chrisbateman]

This is a fairly common pattern (for example) when devs want to attach behavior to anchor/href styles.

Those cases are probably 99% of javascript: usage, so it might be worth explicitly including the href="#" + ev.preventDefault() suggestion in the error message.

[CaitlinWeb]

My use case is similar to many of the comments here, href="javascript:void(0)" which makes the browser see it as a link without an annoying hash in the url. This is related to accessibility as well as styling, as others have mentioned. It's important to note that links have a lot of browser-based functionality as well that role="link" does not satisfy and is a pain to reproduce (middle-mouse button opening a new tab is my favorite).

The solution of onClick={ e => e.preventDefault() } is not ideal because then you have to wrap all your link components' onClick event with that. Which results in something like:

<a href="#" onClick={ev => {
    ev.preventDefault();
    this.props.onClick(ev);
    return false; // old browsers, may not be needed
}} />

Compare to the simple javascript: way:

<a href="javascript:void(0)" onClick={this.props.onClick} />

@gaearon I think this should be re-opened for discussion.

[pllee]

Is there a React recommended way to get the previous behavior of href="javascript:void(0);"? Not having an href gets rid of the default keyboard behavior tab and tab -> enter to behave like onClick. Using # gets that behavior but the onClick handler then has the onus of stopping the event propagation to not get the default browser behavior of scrolling to the top.

dangerouslySetInnerHTML seems like a lot of extra work too. I know this sounds terrible but would something like dangerouslyAllowJsHrefs work? I am not worried about user injected links if my href is always hard coded. <a href="javascript:void(0)" dangerouslyAllowJsHrefs />

Allowing urls that are exactly javascript:void(0) and other variants would work as well but I think that would be a slippery slope that react might not want to go down.

[IllusionMH]

Is there a way to only wrap href value somehow?
I know it's old, but we need to provide bookmarklet functionality, which have to start with javascript:.

I'd like to render this component in dangerouslySetInnerHTML, but it's not just link with static string content - it contains several React components as children (icon, i18n, conditional separator). This would require us to duplicate logic and markup.

Having something like dangerouslySetHref="javascript:..." or href={React.dangerousHref('javascript:...')} would help to avoid problems with duplication and maintenance.

Are there any workarounds if using dangerouslySetInnerHTML is not an option?
Provide browser extensions instead of bookmarklet is not an option too 😞

[larafale]

can someone please tell me how to disable this warning in dev mode ?? please :)

now for what's to come, I +1 @IllusionMH on dangerouslySetHref="javascript:", even better if the name could be short like setHref='javascript:'

[eikawata]

The problem with using the href=# for us is that it adds an entry to the history stack when user clicks on it. We do quite complex history management through window.history, so using javasacript:; was our solution, (We would really like to use a button element instead, but buttons cannot be dragged on some browsers such as FireFox...)

[rsshilli]

I also use the history intelligently so I can't use href="#".

How do you use dangerouslySetInnerHTML to add an href="javascript:void(0)" but not allow an XSS attack by accident with something else (ex. text in the link)? Does somebody have a code sample of the correct way to use dangerouslySetInnerHTML to solve this problem?

[YassienW]

I started using e.preventDefault() since it seems like the easier option

[Bappy1988]

how does one use dangerouslySetInnerHTML for this? all the documentation I've read says you use that feature with the __html key to set InnerHTML. I don't want to set innerHTML, I want to set the href attribute.

+1 to whoever suggested a dangerouslySetHref prop...

[gaearon]

@Bappy1988

You'd need to use dangerouslySetInnerHTML on the outer element. Whatever contains your link. E.g.:

<span dangerouslySetInnerHTML={{__html: '<a ... ></a>' }} />

You can also do something like

<a ref={node => {
  if (node) {
    // set its attributes via DOM API
  }
}}>...</a>

What is your actual use case for this?

[gaearon]

How do you use dangerouslySetInnerHTML to add an href="javascript:void(0)" but not allow an XSS attack by accident with something else (ex. text in the link)?

You can't. And you shouldn't. The solution is to not use href="javascript:void(0)".

[gaearon]

The problem with using the href=# for us is that it adds an entry to the history stack when user clicks on it. We do quite complex history management through window.history, so using javasacript:; was our solution

href="#" is also bad. If you really want a non-accessible link that does something links shouldn't do, then you should e.preventDefault() in its click handler.

[gaearon]

can someone please tell me how to disable this warning in dev mode ?? please :)

It will be a hard error in the next major version. While you can always override console.error to do something else, you would only be creating problems for yourself in a future upgrade.

[gaearon]

@IllusionMH

I know it's old, but we need to provide bookmarklet functionality, which have to start with javascript:.

The ref solution (#16382 (comment)) should work for you.

[gaearon]

Which results in something like:

<a href="#" onClick={ev => {
    ev.preventDefault();
    this.props.onClick(ev);
    return false; // old browsers, may not be needed
}} />

First, the return false at the end is definitely not needed because React doesn't use it. Unlike browser event handlers, React ignores the return value completely.

Second, yes, it would be two lines instead of one. That's the downside. The upside is that your app would not be vulnerable to common security issues once we start enforcing this. So it seems worth it.

[IllusionMH]

@gaearon thanks!

In the end we've used ref approach but in way more verbose form.
I guess that we'll make it inline for simplicity.

[gaearon]

I think we have a conclusion here:

1. If you're just using <a> for the link styling, consider using a <button> and styling it as a link instead.
2. If you still must use <a> for some reason, a workaround is to give it href="#" onClick={e => e.preventDefault()}. This is not great, so prefer option 1 if you can.
3. If you absolutely insist on using a javascript: URL, set it yourself in a ref: <a ref={node => node && node.setAttribute('href', '...')}>. This works for the bookmarklet case.
4. If you have hundreds of links like this, as the original post says, one possible solution is to codemod every <a> to <MyLink> and implement either of the above solutions in MyLink. This blog post has an example of something similar.

We're not going to add exceptions for this rule, so I hope the above information helps. I'm going to lock this as resolved because we're going in circles and because people coming from search might miss this information. If you have a problem that you believe isn't addressed here, please file a new issue.