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.