Generate safe javascript url instead of throwing with disableJavaScriptURLs is on (#26507)
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.
Sebastian Markbåge committed
Mar 29, 2023 at 23:39 UTC
4c2fc01900f50b5b1081a2fb8609ea2668bc05b6
4 files changed
+233
-142
packages/react-dom-bindings/src/client/DOMPropertyOperations.js
+28
-17
@@ -17,7 +17,6 @@ import {
17
} from '../shared/DOMProperty';
18
import sanitizeURL from '../shared/sanitizeURL';
19
import {
20
- disableJavaScriptURLs,
20
enableTrustedTypesIntegration,
21
enableCustomElementPropertySupport,
22
enableFilterEmptyStringAttributesDOM,
@@ -43,15 +42,6 @@ export function getValueForProperty(
42
const {propertyName} = propertyInfo;
43
return (node: any)[propertyName];
44
}
46
- if (!disableJavaScriptURLs && propertyInfo.sanitizeURL) {
47
- // If we haven't fully disabled javascript: URLs, and if
48
- // the hydration is successful of a javascript: URL, we
49
- // still want to warn on the client.
50
- if (__DEV__) {
51
- checkAttributeStringCoercion(expected, name);
52
- }
53
- sanitizeURL('' + (expected: any));
54
- }
45
46
const attributeName = propertyInfo.attributeName;
47
@@ -134,6 +124,11 @@ export function getValueForProperty(
124
}
125
126
// shouldRemoveAttribute
127
+ switch (typeof expected) {
128
+ case 'function':
129
+ case 'symbol': // eslint-disable-line
130
+ return value;
131
+ }
132
switch (propertyInfo.type) {
133
case BOOLEAN: {
134
if (expected) {
@@ -175,6 +170,16 @@ export function getValueForProperty(
170
if (__DEV__) {
171
checkAttributeStringCoercion(expected, name);
172
}
173
+ if (propertyInfo.sanitizeURL) {
174
+ // We have already verified this above.
175
+ // eslint-disable-next-line react-internal/safe-string-coercion
176
+ if (value === '' + (sanitizeURL(expected): any)) {
177
+ return expected;
178
+ }
179
+ return value;
180
+ }
181
+ // We have already verified this above.
182
+ // eslint-disable-next-line react-internal/safe-string-coercion
183
if (value === '' + (expected: any)) {
184
return expected;
185
}
@@ -395,19 +400,25 @@ export function setValueForProperty(node: Element, name: string, value: mixed) {
400
}
401
break;
402
default: {
403
+ if (__DEV__) {
404
+ checkAttributeStringCoercion(value, attributeName);
405
+ }
406
let attributeValue;
407
// `setAttribute` with objects becomes only `[object]` in IE8/9,
408
// ('' + value) makes it output the correct toString()-value.
409
if (enableTrustedTypesIntegration) {
402
- attributeValue = (value: any);
403
- } else {
404
- if (__DEV__) {
405
- checkAttributeStringCoercion(value, attributeName);
410
+ if (propertyInfo.sanitizeURL) {
411
+ attributeValue = (sanitizeURL(value): any);
412
+ } else {
413
+ attributeValue = (value: any);
414
}
415
+ } else {
416
+ // We have already verified this above.
417
+ // eslint-disable-next-line react-internal/safe-string-coercion
418
attributeValue = '' + (value: any);
408
- }
409
- if (propertyInfo.sanitizeURL) {
410
- sanitizeURL(attributeValue.toString());
419
+ if (propertyInfo.sanitizeURL) {
420
+ attributeValue = sanitizeURL(attributeValue);
421
+ }
422
}
423
const attributeNamespace = propertyInfo.attributeNamespace;
424
if (attributeNamespace) {
packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
+16
-23
@@ -736,12 +736,13 @@ function pushAttribute(
736
}
737
break;
738
default:
739
+ if (__DEV__) {
740
+ checkAttributeStringCoercion(value, attributeName);
741
+ }
742
if (propertyInfo.sanitizeURL) {
740
- if (__DEV__) {
741
- checkAttributeStringCoercion(value, attributeName);
742
- }
743
- value = '' + (value: any);
744
- sanitizeURL(value);
743
+ // We've already checked above.
744
+ // eslint-disable-next-line react-internal/safe-string-coercion
745
+ value = sanitizeURL('' + (value: any));
746
}
747
target.push(
748
attributeSeparator,
@@ -3844,15 +3845,12 @@ function writeStyleResourceDependencyHrefOnlyInJS(
3845
3846
function writeStyleResourceDependencyInJS(
3847
destination: Destination,
3847
- href: string,
3848
- precedence: string,
3848
+ href: mixed,
3849
+ precedence: mixed,
3850
props: Object,
3851
) {
3851
- if (__DEV__) {
3852
- checkAttributeStringCoercion(href, 'href');
3853
- }
3854
- const coercedHref = '' + (href: any);
3855
- sanitizeURL(coercedHref);
3852
+ // eslint-disable-next-line react-internal/safe-string-coercion
3853
+ const coercedHref = sanitizeURL('' + (href: any));
3854
writeChunk(
3855
destination,
3856
stringToChunk(escapeJSObjectForInstructionScripts(coercedHref)),
@@ -3939,8 +3937,7 @@ function writeStyleResourceAttributeInJS(
3937
if (__DEV__) {
3938
checkAttributeStringCoercion(value, attributeName);
3939
}
3942
- attributeValue = '' + (value: any);
3943
- sanitizeURL(attributeValue);
3940
+ value = sanitizeURL(value);
3941
break;
3942
}
3943
default: {
@@ -4041,15 +4038,12 @@ function writeStyleResourceDependencyHrefOnlyInAttr(
4038
4039
function writeStyleResourceDependencyInAttr(
4040
destination: Destination,
4044
- href: string,
4045
- precedence: string,
4041
+ href: mixed,
4042
+ precedence: mixed,
4043
props: Object,
4044
) {
4048
- if (__DEV__) {
4049
- checkAttributeStringCoercion(href, 'href');
4050
- }
4051
- const coercedHref = '' + (href: any);
4052
- sanitizeURL(coercedHref);
4045
+ // eslint-disable-next-line react-internal/safe-string-coercion
4046
+ const coercedHref = sanitizeURL('' + (href: any));
4047
writeChunk(
4048
destination,
4049
stringToChunk(escapeTextForBrowser(JSON.stringify(coercedHref))),
@@ -4136,8 +4130,7 @@ function writeStyleResourceAttributeInAttr(
4130
if (__DEV__) {
4131
checkAttributeStringCoercion(value, attributeName);
4132
}
4139
- attributeValue = '' + (value: any);
4140
- sanitizeURL(attributeValue);
4133
+ value = sanitizeURL(value);
4134
break;
4135
}
4136
default: {
packages/react-dom-bindings/src/shared/sanitizeURL.js
+12
-7
@@ -24,24 +24,29 @@ const isJavaScriptProtocol =
24
25
let didWarn = false;
26
27
-function sanitizeURL(url: string) {
27
+function sanitizeURL<T>(url: T): T | string {
28
+ // We should never have symbols here because they get filtered out elsewhere.
29
+ // eslint-disable-next-line react-internal/safe-string-coercion
30
+ const stringifiedURL = '' + (url: any);
31
if (disableJavaScriptURLs) {
29
- if (isJavaScriptProtocol.test(url)) {
30
- throw new Error(
31
- 'React has blocked a javascript: URL as a security precaution.',
32
- );
32
+ if (isJavaScriptProtocol.test(stringifiedURL)) {
33
+ // Return a different javascript: url that doesn't cause any side-effects and just
34
+ // throws if ever visited.
35
+ // eslint-disable-next-line no-script-url
36
+ return "javascript:throw new Error('React has blocked a javascript: URL as a security precaution.')";
37
}
38
} else if (__DEV__) {
35
- if (!didWarn && isJavaScriptProtocol.test(url)) {
39
+ if (!didWarn && isJavaScriptProtocol.test(stringifiedURL)) {
40
didWarn = true;
41
console.error(
42
'A future version of React will block javascript: URLs as a security precaution. ' +
43
'Use event handlers instead if you can. If you need to generate unsafe HTML try ' +
44
'using dangerouslySetInnerHTML instead. React was passed %s.',
41
- JSON.stringify(url),
45
+ JSON.stringify(stringifiedURL),
46
);
47
}
48
}
49
+ return url;
50
}
51
52
export default sanitizeURL;
packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.js
+177
-95
@@ -19,7 +19,38 @@ let ReactDOM;
19
let ReactDOMServer;
20
let ReactTestUtils;
21
22
-function runTests(itRenders, itRejectsRendering, expectToReject) {
22
+const EXPECTED_SAFE_URL =
23
+ "javascript:throw new Error('React has blocked a javascript: URL as a security precaution.')";
24
+
25
+describe('ReactDOMServerIntegration - Untrusted URLs', () => {
26
+ // The `itRenders` helpers don't work with the gate pragma, so we have to do
27
+ // this instead.
28
+ if (gate(flags => flags.disableJavaScriptURLs)) {
29
+ it("empty test so Jest doesn't complain", () => {});
30
+ return;
31
+ }
32
+
33
+ function initModules() {
34
+ jest.resetModules();
35
+ React = require('react');
36
+ ReactDOM = require('react-dom');
37
+ ReactDOMServer = require('react-dom/server');
38
+ ReactTestUtils = require('react-dom/test-utils');
39
+
40
+ // Make them available to the helpers.
41
+ return {
42
+ ReactDOM,
43
+ ReactDOMServer,
44
+ ReactTestUtils,
45
+ };
46
+ }
47
+
48
+ const {resetModules, itRenders} = ReactDOMServerIntegrationUtils(initModules);
49
+
50
+ beforeEach(() => {
51
+ resetModules();
52
+ });
53
+
54
itRenders('a http link with the word javascript in it', async render => {
55
const e = await render(
56
<a href="http://javascript:0/thisisfine">Click me</a>,
@@ -28,7 +59,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
59
expect(e.href).toBe('http://javascript:0/thisisfine');
60
});
61
31
- itRejectsRendering('a javascript protocol href', async render => {
62
+ itRenders('a javascript protocol href', async render => {
63
// Only the first one warns. The second warning is deduped.
64
const e = await render(
65
<div>
@@ -41,20 +72,17 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
72
expect(e.lastChild.href).toBe('javascript:notfineagain');
73
});
74
44
- itRejectsRendering(
45
- 'a javascript protocol with leading spaces',
46
- async render => {
47
- const e = await render(
48
- <a href={' \t \u0000\u001F\u0003javascript\n: notfine'}>p0wned</a>,
49
- 1,
50
- );
51
- // We use an approximate comparison here because JSDOM might not parse
52
- // \u0000 in HTML properly.
53
- expect(e.href).toContain('notfine');
54
- },
55
- );
75
+ itRenders('a javascript protocol with leading spaces', async render => {
76
+ const e = await render(
77
+ <a href={' \t \u0000\u001F\u0003javascript\n: notfine'}>p0wned</a>,
78
+ 1,
79
+ );
80
+ // We use an approximate comparison here because JSDOM might not parse
81
+ // \u0000 in HTML properly.
82
+ expect(e.href).toContain('notfine');
83
+ });
84
57
- itRejectsRendering(
85
+ itRenders(
86
'a javascript protocol with intermediate new lines and mixed casing',
87
async render => {
88
const e = await render(
@@ -65,7 +93,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
93
},
94
);
95
68
- itRejectsRendering('a javascript protocol area href', async render => {
96
+ itRenders('a javascript protocol area href', async render => {
97
const e = await render(
98
<map>
99
<area href="javascript:notfine" />
@@ -75,20 +103,17 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
103
expect(e.firstChild.href).toBe('javascript:notfine');
104
});
105
78
- itRejectsRendering('a javascript protocol form action', async render => {
106
+ itRenders('a javascript protocol form action', async render => {
107
const e = await render(<form action="javascript:notfine">p0wned</form>, 1);
108
expect(e.action).toBe('javascript:notfine');
109
});
110
83
- itRejectsRendering(
84
- 'a javascript protocol button formAction',
85
- async render => {
86
- const e = await render(<input formAction="javascript:notfine" />, 1);
87
- expect(e.getAttribute('formAction')).toBe('javascript:notfine');
88
- },
89
- );
111
+ itRenders('a javascript protocol button formAction', async render => {
112
+ const e = await render(<input formAction="javascript:notfine" />, 1);
113
+ expect(e.getAttribute('formAction')).toBe('javascript:notfine');
114
+ });
115
91
- itRejectsRendering('a javascript protocol input formAction', async render => {
116
+ itRenders('a javascript protocol input formAction', async render => {
117
const e = await render(
118
<button formAction="javascript:notfine">p0wned</button>,
119
1,
@@ -96,12 +121,12 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
121
expect(e.getAttribute('formAction')).toBe('javascript:notfine');
122
});
123
99
- itRejectsRendering('a javascript protocol iframe src', async render => {
124
+ itRenders('a javascript protocol iframe src', async render => {
125
const e = await render(<iframe src="javascript:notfine" />, 1);
126
expect(e.src).toBe('javascript:notfine');
127
});
128
104
- itRejectsRendering('a javascript protocol frame src', async render => {
129
+ itRenders('a javascript protocol frame src', async render => {
130
const e = await render(
131
<html>
132
<head />
@@ -114,7 +139,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
139
expect(e.lastChild.firstChild.src).toBe('javascript:notfine');
140
});
141
117
- itRejectsRendering('a javascript protocol in an SVG link', async render => {
142
+ itRenders('a javascript protocol in an SVG link', async render => {
143
const e = await render(
144
<svg>
145
<a href="javascript:notfine" />
@@ -124,7 +149,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
149
expect(e.firstChild.getAttribute('href')).toBe('javascript:notfine');
150
});
151
127
- itRejectsRendering(
152
+ itRenders(
153
'a javascript protocol in an SVG link with a namespace',
154
async render => {
155
const e = await render(
@@ -142,49 +167,15 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
167
it('rejects a javascript protocol href if it is added during an update', () => {
168
const container = document.createElement('div');
169
ReactDOM.render(<a href="thisisfine">click me</a>, container);
145
- expectToReject(() => {
170
+ expect(() => {
171
ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
147
- });
148
- });
149
-}
150
-
151
-describe('ReactDOMServerIntegration - Untrusted URLs', () => {
152
- // The `itRenders` helpers don't work with the gate pragma, so we have to do
153
- // this instead.
154
- if (gate(flags => flags.disableJavaScriptURLs)) {
155
- it("empty test so Jest doesn't complain", () => {});
156
- return;
157
- }
158
-
159
- function initModules() {
160
- jest.resetModules();
161
- React = require('react');
162
- ReactDOM = require('react-dom');
163
- ReactDOMServer = require('react-dom/server');
164
- ReactTestUtils = require('react-dom/test-utils');
165
-
166
- // Make them available to the helpers.
167
- return {
168
- ReactDOM,
169
- ReactDOMServer,
170
- ReactTestUtils,
171
- };
172
- }
173
-
174
- const {resetModules, itRenders} = ReactDOMServerIntegrationUtils(initModules);
175
-
176
- beforeEach(() => {
177
- resetModules();
178
- });
179
-
180
- runTests(itRenders, itRenders, fn =>
181
- expect(fn).toErrorDev(
172
+ }).toErrorDev(
173
'Warning: A future version of React will block javascript: URLs as a security precaution. ' +
174
'Use event handlers instead if you can. If you need to generate unsafe HTML try using ' +
175
'dangerouslySetInnerHTML instead. React was passed "javascript:notfine".\n' +
176
' in a (at **)',
186
- ),
187
- );
177
+ );
178
+ });
179
});
180
181
describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', () => {
@@ -216,34 +207,127 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', (
207
const {
208
resetModules,
209
itRenders,
219
- itThrowsWhenRendering,
210
clientRenderOnBadMarkup,
211
clientRenderOnServerString,
212
} = ReactDOMServerIntegrationUtils(initModules);
213
224
- const expectToReject = fn => {
225
- let msg;
226
- try {
227
- fn();
228
- } catch (x) {
229
- msg = x.message;
230
- }
231
- expect(msg).toContain(
232
- 'React has blocked a javascript: URL as a security precaution.',
233
- );
234
- };
235
-
214
beforeEach(() => {
215
resetModules();
216
});
217
240
- runTests(
241
- itRenders,
242
- (message, test) =>
243
- itThrowsWhenRendering(message, test, 'blocked a javascript: URL'),
244
- expectToReject,
218
+ itRenders('a http link with the word javascript in it', async render => {
219
+ const e = await render(
220
+ <a href="http://javascript:0/thisisfine">Click me</a>,
221
+ );
222
+ expect(e.tagName).toBe('A');
223
+ expect(e.href).toBe('http://javascript:0/thisisfine');
224
+ });
225
+
226
+ itRenders('a javascript protocol href', async render => {
227
+ // Only the first one warns. The second warning is deduped.
228
+ const e = await render(
229
+ <div>
230
+ <a href="javascript:notfine">p0wned</a>
231
+ <a href="javascript:notfineagain">p0wned again</a>
232
+ </div>,
233
+ );
234
+ expect(e.firstChild.href).toBe(EXPECTED_SAFE_URL);
235
+ expect(e.lastChild.href).toBe(EXPECTED_SAFE_URL);
236
+ });
237
+
238
+ itRenders('a javascript protocol with leading spaces', async render => {
239
+ const e = await render(
240
+ <a href={' \t \u0000\u001F\u0003javascript\n: notfine'}>p0wned</a>,
241
+ );
242
+ // We use an approximate comparison here because JSDOM might not parse
243
+ // \u0000 in HTML properly.
244
+ expect(e.href).toBe(EXPECTED_SAFE_URL);
245
+ });
246
+
247
+ itRenders(
248
+ 'a javascript protocol with intermediate new lines and mixed casing',
249
+ async render => {
250
+ const e = await render(
251
+ <a href={'\t\r\n Jav\rasCr\r\niP\t\n\rt\n:notfine'}>p0wned</a>,
252
+ );
253
+ expect(e.href).toBe(EXPECTED_SAFE_URL);
254
+ },
255
);
256
257
+ itRenders('a javascript protocol area href', async render => {
258
+ const e = await render(
259
+ <map>
260
+ <area href="javascript:notfine" />
261
+ </map>,
262
+ );
263
+ expect(e.firstChild.href).toBe(EXPECTED_SAFE_URL);
264
+ });
265
+
266
+ itRenders('a javascript protocol form action', async render => {
267
+ const e = await render(<form action="javascript:notfine">p0wned</form>);
268
+ expect(e.action).toBe(EXPECTED_SAFE_URL);
269
+ });
270
+
271
+ itRenders('a javascript protocol button formAction', async render => {
272
+ const e = await render(<input formAction="javascript:notfine" />);
273
+ expect(e.getAttribute('formAction')).toBe(EXPECTED_SAFE_URL);
274
+ });
275
+
276
+ itRenders('a javascript protocol input formAction', async render => {
277
+ const e = await render(
278
+ <button formAction="javascript:notfine">p0wned</button>,
279
+ );
280
+ expect(e.getAttribute('formAction')).toBe(EXPECTED_SAFE_URL);
281
+ });
282
+
283
+ itRenders('a javascript protocol iframe src', async render => {
284
+ const e = await render(<iframe src="javascript:notfine" />);
285
+ expect(e.src).toBe(EXPECTED_SAFE_URL);
286
+ });
287
+
288
+ itRenders('a javascript protocol frame src', async render => {
289
+ const e = await render(
290
+ <html>
291
+ <head />
292
+ <frameset>
293
+ <frame src="javascript:notfine" />
294
+ </frameset>
295
+ </html>,
296
+ );
297
+ expect(e.lastChild.firstChild.src).toBe(EXPECTED_SAFE_URL);
298
+ });
299
+
300
+ itRenders('a javascript protocol in an SVG link', async render => {
301
+ const e = await render(
302
+ <svg>
303
+ <a href="javascript:notfine" />
304
+ </svg>,
305
+ );
306
+ expect(e.firstChild.getAttribute('href')).toBe(EXPECTED_SAFE_URL);
307
+ });
308
+
309
+ itRenders(
310
+ 'a javascript protocol in an SVG link with a namespace',
311
+ async render => {
312
+ const e = await render(
313
+ <svg>
314
+ <a xlinkHref="javascript:notfine" />
315
+ </svg>,
316
+ );
317
+ expect(
318
+ e.firstChild.getAttributeNS('http://www.w3.org/1999/xlink', 'href'),
319
+ ).toBe(EXPECTED_SAFE_URL);
320
+ },
321
+ );
322
+
323
+ it('rejects a javascript protocol href if it is added during an update', () => {
324
+ const container = document.createElement('div');
325
+ ReactDOM.render(<a href="http://thisisfine/">click me</a>, container);
326
+ expect(container.firstChild.href).toBe('http://thisisfine/');
327
+ ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
328
+ expect(container.firstChild.href).toBe(EXPECTED_SAFE_URL);
329
+ });
330
+
331
itRenders('only the first invocation of toString', async render => {
332
let expectedToStringCalls = 1;
333
if (render === clientRenderOnBadMarkup) {
@@ -255,9 +339,8 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', (
339
// The hydration validation calls it one extra time.
340
// TODO: It would be good if we only called toString once for
341
// consistency but the code structure makes that hard right now.
258
- expectedToStringCalls = 2;
259
- }
260
- if (__DEV__) {
342
+ expectedToStringCalls = 5;
343
+ } else if (__DEV__) {
344
// Checking for string coercion problems results in double the
345
// toString calls in DEV
346
expectedToStringCalls *= 2;
@@ -283,14 +366,13 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', (
366
367
it('rejects a javascript protocol href if it is added during an update twice', () => {
368
const container = document.createElement('div');
286
- ReactDOM.render(<a href="thisisfine">click me</a>, container);
287
- expectToReject(() => {
288
- ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
289
- });
369
+ ReactDOM.render(<a href="http://thisisfine/">click me</a>, container);
370
+ expect(container.firstChild.href).toBe('http://thisisfine/');
371
+ ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
372
+ expect(container.firstChild.href).toBe(EXPECTED_SAFE_URL);
373
// The second update ensures that a global flag hasn't been added to the regex
374
// which would fail to match the second time it is called.
292
- expectToReject(() => {
293
- ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
294
- });
375
+ ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
376
+ expect(container.firstChild.href).toBe(EXPECTED_SAFE_URL);
377
});
378
});