Visitar URL original
refactor(compiler): remove img/video src attribute from the URL secur… · angular/angular@6fa2b08 · GitHub
Skip to content

Commit 6fa2b08

Browse files
JeanMecheNothingEverHappens
JeanMeche
authored and
NothingEverHappens
committed
refactor(compiler): remove img/video src attribute from the URL security context.
Browsers are already blocking script execution on those attributes. (cherry picked from commit 439ff1b)
1 parent 93d4bd9 commit 6fa2b08

11 files changed

Lines changed: 90 additions & 14 deletions

File tree

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_bindings/property_bindings/sanitization.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ template: function MyComponent_Template(rf, ctx) {
77
$r3$.ɵɵadvance();
88
$r3$.ɵɵattribute("style", ctx.evil, $r3$.ɵɵsanitizeStyle);
99
$r3$.ɵɵadvance();
10-
$r3$.ɵɵdomProperty("src", ctx.nonEvil, $r3$.ɵɵsanitizeUrl);
10+
$r3$.ɵɵdomProperty("src", ctx.nonEvil);
1111
$r3$.ɵɵadvance();
1212
$r3$.ɵɵdomProperty("sandbox", ctx.evil, $r3$.ɵɵvalidateAttribute);
1313
$r3$.ɵɵadvance();
@@ -23,7 +23,7 @@ template: function MyComponent_Template(rf, ctx) {
2323
$r3$.ɵɵadvance();
2424
$r3$.ɵɵtwoWayProperty("srcdoc", ctx.evil, $r3$.ɵɵsanitizeHtml);
2525
$r3$.ɵɵadvance();
26-
$r3$.ɵɵtwoWayProperty("src", ctx.evil, $r3$.ɵɵsanitizeUrl);
26+
$r3$.ɵɵtwoWayProperty("src", ctx.evil);
2727
$r3$.ɵɵadvance();
2828
$r3$.ɵɵtwoWayProperty("src", ctx.evil, $r3$.ɵɵsanitizeResourceUrl);
2929
$r3$.ɵɵadvance();

‎packages/compiler/src/schema/dom_security_schema.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,6 @@ export function SECURITY_SCHEMA(): SecuritySchema {
8686
['area', ['href']],
8787
['a', ['href', 'xlink:href']],
8888
['form', ['action']],
89-
90-
// The below two items are safe and should be removed but they require a G3 clean-up as a small number of tests fail.
91-
['img', ['src']],
92-
['video', ['src']],
9389
]);
9490

9591
registerContext(SecurityContext.URL, MATH_ML_NAMESPACE, [

‎packages/core/src/render3/instructions/shared.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {hasSkipHydrationAttrOnRElement} from '../../hydration/skip_hydration';
1212
import {PRESERVE_HOST_CONTENT, PRESERVE_HOST_CONTENT_DEFAULT} from '../../hydration/tokens';
1313
import {processTextNodeMarkersBeforeHydration} from '../../hydration/utils';
1414
import {ViewEncapsulation} from '../../metadata/view';
15+
import {unwrapSafeValue} from '../../sanitization/bypass';
1516
import {validateAgainstEventProperties} from '../../sanitization/sanitization';
1617

1718
import {ProfilerEvent} from '../../../primitives/devtools';
@@ -305,6 +306,15 @@ export function setDomProperty<T>(
305306
// It is assumed that the sanitizer is only added when the compiler determines that the
306307
// property is risky, so sanitization can be done without further checks.
307308
value = sanitizer != null ? (sanitizer(value, tNode.value || '', propName) as any) : value;
309+
310+
// The `src` property previously used a sanitizer which mapped `null`/`undefined` to `''`.
311+
// Now that `img` and `video` `src` are no longer sanitized, we still need to map `null` and
312+
// `undefined` to `''` to avoid stringifying them to `'null'` or `'undefined'` and causing
313+
// broken network requests.
314+
315+
// TODO(v23): Remove this workaround once we can introduce a breaking change
316+
value = unwrapImgVideoSrcValue(propName, element as RElement, value);
317+
308318
renderer.setProperty(element as RElement, propName, value);
309319
} else if (tNode.type & TNodeType.AnyContainer) {
310320
// If the node is a container and the property didn't
@@ -315,6 +325,28 @@ export function setDomProperty<T>(
315325
}
316326
}
317327

328+
/**
329+
* This function allows us to workaround a breaking change introduced by #71095
330+
* src attributes/bindings used to be sanitized which was responsible for:
331+
* - converting undefined/null to ''
332+
* - supporting bypassed values (via bypassSecurityTrustResourceUrl)
333+
*
334+
* This workaround is intended to be dropped in v23 when the breaking change window opens.
335+
*/
336+
function unwrapImgVideoSrcValue(propName: string, element: RElement, value: unknown): any {
337+
if (
338+
propName === 'src' &&
339+
((element as RElement).tagName === 'IMG' || (element as RElement).tagName === 'VIDEO')
340+
) {
341+
if (value == null) {
342+
return '';
343+
}
344+
345+
return unwrapSafeValue(value);
346+
}
347+
return value;
348+
}
349+
318350
/** If node is an OnPush component, marks its LView dirty. */
319351
export function markDirtyIfOnPush(lView: LView, viewIndex: number): void {
320352
ngDevMode && assertLView(lView);
@@ -515,6 +547,8 @@ export function elementAttributeInternal(
515547
}
516548

517549
const element = getNativeByTNode(tNode, lView) as RElement;
550+
// TODO(v23): Remove this workaround once we can introduce a breaking change
551+
value = unwrapImgVideoSrcValue(name, element, value);
518552
setElementAttribute(lView[RENDERER], element, namespace, tNode.value, name, value, sanitizer);
519553
}
520554

‎packages/core/src/sanitization/bypass.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ abstract class SafeValueImpl implements SafeValue {
6666
toString() {
6767
return (
6868
`SafeValue must use [property]=binding: ${this.changingThisBreaksApplicationSecurity}` +
69-
` (see ${XSS_SECURITY_URL})`
69+
(ngDevMode ? ` (see ${XSS_SECURITY_URL})` : '')
7070
);
7171
}
7272
}

‎packages/core/src/sanitization/dom_security_schema.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,6 @@ export function SECURITY_SCHEMA(): SecuritySchema {
8686
['area', ['href']],
8787
['a', ['href', 'xlink:href']],
8888
['form', ['action']],
89-
90-
// The below two items are safe and should be removed but they require a G3 clean-up as a small number of tests fail.
91-
['img', ['src']],
92-
['video', ['src']],
9389
]);
9490

9591
registerContext(SecurityContext.URL, MATH_ML_NAMESPACE, [

‎packages/core/test/acceptance/attributes_spec.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,12 @@
77
*/
88

99
import {By, DomSanitizer, SafeUrl} from '@angular/platform-browser';
10-
import {ChangeDetectionStrategy, Component, provideZoneChangeDetection} from '../../src/core';
10+
import {
11+
ChangeDetectionStrategy,
12+
Component,
13+
inject,
14+
provideZoneChangeDetection,
15+
} from '../../src/core';
1116
import {TestBed} from '../../testing';
1217

1318
describe('attribute creation', () => {
@@ -209,6 +214,21 @@ describe('attribute binding', () => {
209214
// should not start with `unsafe:`.
210215
expect(a.href.indexOf('unsafe:')).toBe(-1);
211216
});
217+
218+
it('should bind attribute src and not expose a safe value', () => {
219+
@Component({
220+
template: `<img [attr.src]="badUrl" />`,
221+
})
222+
class Comp {
223+
badUrl = inject(DomSanitizer).bypassSecurityTrustUrl('javascript:true');
224+
}
225+
226+
const fixture = TestBed.createComponent(Comp);
227+
fixture.detectChanges();
228+
229+
const img = fixture.debugElement.query(By.css('img')).nativeElement;
230+
expect(img.getAttribute('src')).toBe('javascript:true');
231+
});
212232
});
213233

214234
describe('attribute interpolation', () => {

‎packages/core/test/acceptance/property_binding_spec.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,32 @@ describe('property bindings', () => {
5050
expect(spanEl.id).toBe('testId');
5151
});
5252

53+
it('should not set `src` to "undefined" string for img/video elements when bound to undefined', () => {
54+
@Component({
55+
template: `
56+
<img [src]="imgSrc" />
57+
<video [src]="videoSrc"></video>
58+
`,
59+
})
60+
class Comp {
61+
imgSrc: string | undefined;
62+
videoSrc: string | undefined;
63+
}
64+
65+
const fixture = TestBed.createComponent(Comp);
66+
fixture.detectChanges();
67+
68+
const img = fixture.nativeElement.querySelector('img') as HTMLImageElement;
69+
const video = fixture.nativeElement.querySelector('video') as HTMLVideoElement;
70+
71+
// Both should have their src attribute set to an empty string, not "undefined"
72+
// (Wait, actually they might not have the attribute if the property is set to '', but `img.src` should not contain 'undefined')
73+
expect(img.getAttribute('src')).not.toBe('undefined');
74+
expect(img.getAttribute('src')).toBe('');
75+
expect(video.getAttribute('src')).not.toBe('undefined');
76+
expect(video.getAttribute('src')).toBe('');
77+
});
78+
5379
it('should update bindings when value changes', () => {
5480
@Component({
5581
template: `<a [title]="title"></a>`,

‎packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,7 @@
232232
"SUBSTITUTION_EXPR_START",
233233
"SVG_NAMESPACE",
234234
"SafeSubscriber",
235+
"SafeValueImpl",
235236
"Sanitizer",
236237
"ShadowDomRenderer",
237238
"SharedStylesHost",
@@ -849,7 +850,9 @@
849850
"unregisterLView",
850851
"unregisteredTrigger",
851852
"unsupportedTriggerEvent",
853+
"unwrapImgVideoSrcValue",
852854
"unwrapRNode",
855+
"unwrapSafeValue",
853856
"updateAncestorTraversalFlagsOnAttach",
854857
"updateMicroTaskStatus",
855858
"validateStyleParams",

‎packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,6 @@
283283
"ViewContext",
284284
"ViewEncapsulation",
285285
"ViewRef",
286-
"XSS_SECURITY_URL",
287286
"ZONELESS_ENABLED",
288287
"ZoneAwareEffectScheduler",
289288
"\\u0275FORM_CONTROL_INTEGRATION",
@@ -1055,6 +1054,7 @@
10551054
"unregisterLView",
10561055
"untracked",
10571056
"untracked2",
1057+
"unwrapImgVideoSrcValue",
10581058
"unwrapRNode",
10591059
"unwrapResponse",
10601060
"unwrapSafeValue",

‎packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,6 @@
277277
"ViewContext",
278278
"ViewEncapsulation",
279279
"ViewRef",
280-
"XSS_SECURITY_URL",
281280
"ZONELESS_ENABLED",
282281
"ZoneAwareEffectScheduler",
283282
"\\u0275FORM_CONTROL_INTEGRATION",
@@ -1053,6 +1052,7 @@
10531052
"unregisterLView",
10541053
"untracked",
10551054
"untracked2",
1055+
"unwrapImgVideoSrcValue",
10561056
"unwrapRNode",
10571057
"unwrapResponse",
10581058
"unwrapSafeValue",

0 commit comments

Comments
 (0)