Generate safe javascript url instead of throwing with disableJavaScriptURLs is on

[sebmarkbage]

We currently throw an error when disableJavaScriptURLs is on and trigger an error boundary. I kind of thought that's what would happen with CSP or Trusted Types anyway. However, that's not what happens. Instead, in those environments what happens is that the error is triggered when you try to actually visit those links. So if you preventDefault() or something it'll never show up and since the error just logs to the console or to a violation logger, it's effectively a noop to users.

We can simulate the same without CSP by simply generating a different javascript: url that throws instead of executing the potential attack vector.

This still allows these to be used - at least as long as you preventDefault before using them in practice. This might be legit for forms. We still don't recommend using them for links-as-buttons since it'll be possible to "Open in a New Tab" and other weird artifacts. For links we still recommend the technique of assigning a button role etc.

It also is a little nicer when an attack actually happens because at least it doesn't allow an attacker to trigger error boundaries and effectively deny access to a page.

[react-sizebot]

Comparing: 85de6fd...6b5a9e9

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	162.20 kB	162.20 kB	=	51.29 kB	51.29 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	163.85 kB	163.85 kB	=	51.79 kB	51.79 kB
facebook-www/ReactDOM-prod.classic.js	+0.03%	556.07 kB	556.25 kB	+0.08%	98.25 kB	98.33 kB
facebook-www/ReactDOM-prod.modern.js	+0.03%	539.78 kB	539.97 kB	+0.06%	95.83 kB	95.88 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactDOMServer-prod.classic.js	=	132.03 kB	130.87 kB	+0.06%	24.89 kB	24.90 kB
facebook-www/ReactDOMServer-prod.modern.js	=	128.46 kB	127.30 kB	+0.05%	24.19 kB	24.20 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js	=	133.40 kB	132.19 kB	=	25.48 kB	25.45 kB

Generated by 🚫 dangerJS against 6b5a9e9

[sophiebits]

javascript:void 0 URLs or whatever turning into this throw would still trigger CSP reporting if you don't prevent it, yeah? Does that matter?

[sophiebits]

Where was the coercion checked?

[sebmarkbage]

It's not really, the original type was string which tripped me up and I originally removed all the checks and then restored a few. Not really sure what we should do about these. I'd rather just remove them all. It's a pain to work with.

I'm going to send a follow up to remove sanitizeURL from these cases since stylesheets can't trigger javascript urls. However, we still need to coerce.

[sophiebits]

I don't follow this part.

[sebmarkbage]

I think this was a bug earlier. If you have an attribute but the prop value was function or symbol, then it's an error because there shouldn't exist an attribute at all.

Not sure why it's not covered by attribute table.

Returning the prop value causes the error because it won't be the same as expected.

[sebmarkbage]

javascript:void 0 URLs or whatever turning into this throw would still trigger CSP reporting if you don't prevent it, yeah? Does that matter?

Yea but they probably would just with the void(0) too or are those special cased? Unless you use a trusted policy that allow exactly that.

Doesn't really matter because we already warn in DEV for using javascript: urls at all. This just relaxes that a bit again and if you really do need then, you should be preventing default to prevent navigation and triggering the Navigation API etc too.

[sophiebits]

Oh yeah I guess so. Were you going to add them automatically for formAction though in a followup? I guess you can do CSP unsafe-hashes which is now supported in modern browsers but you can sorta do that with the throw too.

[sebmarkbage]

Were you going to add them automatically for formAction though in a followup

I'm going to add a different error message but it'll still throw. Because it indicates that our preventDefault() was stopped before we could.