Skip to content

Commit c65d378

Browse files
SkyZeroZxmattrbeck
authored andcommitted
fix(core): validate SVG animation attributes outside the SVG namespace
Make the SVG animation security context depend on the tag name instead of the namespace the element was created in. An animation element declared outside an `<svg>` is created in the HTML namespace, but still animates once it ends up inside an SVG subtree, so to and `attributeName` bindings were reaching the DOM unvalidated. Fixes #70490 (cherry picked from commit 7168bed)
1 parent 4340a63 commit c65d378

5 files changed

Lines changed: 206 additions & 1 deletion

File tree

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,5 +210,14 @@ export function checkSecurityContext(
210210
}
211211
}
212212

213+
// An SVG animation element declared outside of an `<svg>` has no explicit namespace at compile
214+
// time, but can still animate once it is projected into an SVG subtree.
215+
if (context === undefined && (!namespace || namespace === NO_NAMESPACE)) {
216+
const svgSchema = attrSchema[SVG_NAMESPACE];
217+
if (svgSchema) {
218+
context = svgSchema[tagLower];
219+
}
220+
}
221+
213222
return context ?? SecurityContext.NONE;
214223
}

‎packages/compiler/test/schema/dom_element_schema_registry_spec.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,33 @@ If 'onAnything' is a directive input, make sure the directive is imported by the
172172
SecurityContext.ATTRIBUTE_NO_BINDING,
173173
);
174174

175+
// SVG animation elements are sensitive when their namespace is omitted at compile time.
176+
expect(registry.securityContext('set', 'to', true)).toBe(SecurityContext.ATTRIBUTE_NO_BINDING);
177+
expect(registry.securityContext('set', 'attributeName', true)).toBe(
178+
SecurityContext.ATTRIBUTE_NO_BINDING,
179+
);
180+
expect(registry.securityContext('animate', 'to', true)).toBe(
181+
SecurityContext.ATTRIBUTE_NO_BINDING,
182+
);
183+
expect(registry.securityContext('animate', 'from', true)).toBe(
184+
SecurityContext.ATTRIBUTE_NO_BINDING,
185+
);
186+
expect(registry.securityContext('animate', 'values', true)).toBe(
187+
SecurityContext.ATTRIBUTE_NO_BINDING,
188+
);
189+
expect(registry.securityContext('animate', 'attributeName', true)).toBe(
190+
SecurityContext.ATTRIBUTE_NO_BINDING,
191+
);
192+
expect(registry.securityContext('animateMotion', 'attributeName', true)).toBe(
193+
SecurityContext.ATTRIBUTE_NO_BINDING,
194+
);
195+
expect(registry.securityContext('animateTransform', 'attributeName', true)).toBe(
196+
SecurityContext.ATTRIBUTE_NO_BINDING,
197+
);
198+
199+
// The defensive SVG lookup must not apply to elements in an explicit foreign namespace.
200+
expect(registry.securityContext(':math:set', 'to', true)).toBe(SecurityContext.NONE);
201+
175202
// SVG link attributes
176203
expect(registry.securityContext(':svg:a', 'href', false)).toBe(SecurityContext.URL);
177204
expect(registry.securityContext(':svg:a', 'xlink:href', false)).toBe(SecurityContext.URL);

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,5 +210,14 @@ export function checkSecurityContext(
210210
}
211211
}
212212

213+
// An SVG animation element declared outside of an `<svg>` has no explicit namespace at compile
214+
// time, but can still animate once it is projected into an SVG subtree.
215+
if (context === undefined && (!namespace || namespace === NO_NAMESPACE)) {
216+
const svgSchema = attrSchema[SVG_NAMESPACE];
217+
if (svgSchema) {
218+
context = svgSchema[tagLower];
219+
}
220+
}
221+
213222
return context ?? SecurityContext.NONE;
214223
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ export function ɵɵvalidateAttribute<T = any>(value: T, tagName: string, attrib
358358
if (resolvedTagName === 'iframe') {
359359
const element = getNativeByTNode(tNode, lView) as RElement;
360360
enforceIframeSecurity(element as HTMLIFrameElement);
361-
} else if (namespace === SVG_NAMESPACE) {
361+
} else if (namespace === SVG_NAMESPACE || !namespace) {
362362
const config =
363363
SVG_ANIMATION_SENSITIVE_STATIC_VALUES[resolvedTagName]?.[attributeName.toLowerCase()];
364364
if (config) {

‎packages/core/test/render3/integration_spec.ts‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,13 @@ import {CommonModule} from '@angular/common';
1010
import {
1111
ChangeDetectionStrategy,
1212
Component,
13+
createComponent,
1314
Directive,
15+
EnvironmentInjector,
1416
HostBinding,
17+
inject,
1518
provideZoneChangeDetection,
19+
ViewContainerRef,
1620
} from '../../src/core';
1721
import {TestBed} from '../../testing';
1822

@@ -688,6 +692,162 @@ describe('sanitization', () => {
688692
});
689693
}
690694

695+
// One carrier per attribute: directive metadata has to be statically analyzable, so the bound
696+
// attribute name cannot be interpolated into `host`.
697+
const UNSAFE_VALUE = 'javascript:alert(1)';
698+
699+
@Directive({selector: '[bind-to]', host: {'[attr.to]': 'value'}})
700+
class BindTo {
701+
value = UNSAFE_VALUE;
702+
}
703+
704+
@Directive({selector: '[bind-from]', host: {'[attr.from]': 'value'}})
705+
class BindFrom {
706+
value = UNSAFE_VALUE;
707+
}
708+
709+
@Directive({selector: '[bind-values]', host: {'[attr.values]': 'value'}})
710+
class BindValues {
711+
value = UNSAFE_VALUE;
712+
}
713+
714+
@Directive({selector: '[bind-attribute-name]', host: {'[attr.attributeName]': 'value'}})
715+
class BindAttributeName {
716+
value = UNSAFE_VALUE;
717+
}
718+
719+
const attrCarriers = {
720+
'to': BindTo,
721+
'from': BindFrom,
722+
'values': BindValues,
723+
'attributeName': BindAttributeName,
724+
};
725+
726+
// Binds one attribute on a host element created in the HTML namespace. Going through a concrete
727+
// `hostElement` keeps the tag name out of the template, which lets the cases below be table
728+
// driven. Returns the change detection call so the caller can assert on it.
729+
function bindAttributeOutsideSvg(
730+
tagName: string,
731+
attrName: keyof typeof attrCarriers,
732+
staticAttributeName: string,
733+
): () => void {
734+
@Component({
735+
template: '',
736+
changeDetection: ChangeDetectionStrategy.Eager,
737+
})
738+
class DynamicHost {}
739+
740+
const hostElement = document.createElement(tagName);
741+
hostElement.setAttribute('attributeName', staticAttributeName);
742+
743+
const componentRef = createComponent(DynamicHost, {
744+
hostElement,
745+
environmentInjector: TestBed.inject(EnvironmentInjector),
746+
directives: [attrCarriers[attrName]],
747+
});
748+
749+
return () => {
750+
try {
751+
componentRef.changeDetectorRef.detectChanges();
752+
} finally {
753+
componentRef.destroy();
754+
}
755+
};
756+
}
757+
758+
// An animation element declared outside of an `<svg>` is created in the HTML namespace, but still
759+
// animates once it ends up inside an `<svg>` subtree - for example when it is projected into one
760+
// and the server-rendered markup is re-parsed by the browser.
761+
const svgAnimationAttrs: Record<string, (keyof typeof attrCarriers)[]> = {
762+
'set': ['to', 'attributeName'],
763+
'animate': ['to', 'from', 'values', 'attributeName'],
764+
'animateMotion': ['attributeName'],
765+
'animateTransform': ['attributeName'],
766+
};
767+
768+
for (const [tagName, attrNames] of Object.entries(svgAnimationAttrs)) {
769+
for (const attrName of attrNames) {
770+
it(`should throw when binding to \`${attrName}\` on a <${tagName}> outside of an svg element`, () => {
771+
const detectChanges = bindAttributeOutsideSvg(tagName, attrName, 'href');
772+
773+
// The tag name is reported as the security check lower-cased it.
774+
expect(detectChanges).toThrowError(
775+
new RegExp(
776+
`Angular has detected that the \`${attrName}\` was applied as a binding ` +
777+
`to the <${tagName.toLowerCase()}> element`,
778+
),
779+
);
780+
});
781+
}
782+
}
783+
784+
it('should throw when binding to a set element projected into an svg element', () => {
785+
@Component({
786+
selector: 'svg-wrapper',
787+
template: `
788+
<svg>
789+
<a>
790+
<g>
791+
<ng-content></ng-content>
792+
</g>
793+
</a>
794+
</svg>
795+
`,
796+
})
797+
class SvgWrapper {}
798+
799+
@Component({
800+
selector: 'test-comp',
801+
imports: [SvgWrapper],
802+
template: `
803+
<svg-wrapper>
804+
<set attributeName="href" [attr.to]="'javascript:alert(1)'"></set>
805+
</svg-wrapper>
806+
`,
807+
changeDetection: ChangeDetectionStrategy.Eager,
808+
})
809+
class TestComp {}
810+
811+
const fixture = TestBed.createComponent(TestComp);
812+
expect(() => fixture.detectChanges()).toThrowError(
813+
/Angular has detected that the `to` was applied/,
814+
);
815+
});
816+
817+
it('should throw when binding to a dynamically created set component host', () => {
818+
@Component({
819+
selector: 'set[drilldown]',
820+
template: '',
821+
host: {
822+
'attributeName': 'href',
823+
'[attr.to]': 'target',
824+
},
825+
changeDetection: ChangeDetectionStrategy.Eager,
826+
})
827+
class DynamicSetComp {
828+
target = 'javascript:alert(1)';
829+
}
830+
831+
@Component({
832+
selector: 'test-comp',
833+
template: '',
834+
})
835+
class TestComp {
836+
readonly viewContainerRef = inject(ViewContainerRef);
837+
}
838+
839+
const fixture = TestBed.createComponent(TestComp);
840+
fixture.componentInstance.viewContainerRef.createComponent(DynamicSetComp);
841+
842+
expect(() => fixture.detectChanges()).toThrowError(
843+
/Angular has detected that the `to` was applied/,
844+
);
845+
});
846+
847+
it('should not throw when binding to a set element outside of an svg element when attributeName is not href', () => {
848+
expect(bindAttributeOutsideSvg('set', 'to', 'display')).not.toThrow();
849+
});
850+
691851
it('should not throw when binding to animate element when attributeName is not href', () => {
692852
@Component({
693853
selector: 'test-comp',

0 commit comments

Comments
 (0)