Skip to content

Commit 68282df

Browse files
alan-agius4leonsenft
authored andcommitted
fix(compiler): strip namespaced SVG script elements during template compilation
Ensures that namespaced <script> elements (such as :svg:script) are correctly classified as PreparsedElementType.SCRIPT by the template preparser and stripped during compilation to prevent potential XSS vulnerabilities. Consequently, obsolete security schema mappings and runtime sanitization checks for <script> attributes have been removed since these elements are never present in compiled template outputs. (cherry picked from commit 90494cd)
1 parent 099bf57 commit 68282df

6 files changed

Lines changed: 30 additions & 54 deletions

File tree

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8622,34 +8622,6 @@ runInEachFileSystem((os: string) => {
86228622
expect(trim(jsContents)).toContain(trim(hostBindingsFn));
86238623
});
86248624

8625-
it('should generate sanitizers for URL properties in SVG script fn in Component', () => {
8626-
env.write(
8627-
'test.ts',
8628-
`
8629-
import {Component} from '@angular/core';
8630-
8631-
@Component({
8632-
selector: 'test-cmp',
8633-
template: \`
8634-
<svg>
8635-
<script [attr.xlink:href]="attr" [attr.href]="attr"></script>
8636-
</svg>
8637-
\`,
8638-
})
8639-
export class TestCmp {
8640-
attr = './script.js';
8641-
}
8642-
`,
8643-
);
8644-
8645-
env.driveMain();
8646-
8647-
const jsContents = env.getContents('test.js');
8648-
expect(jsContents).toContain(
8649-
'i0.ɵɵattribute("href", ctx.attr, i0.ɵɵsanitizeResourceUrl, "xlink")("href", ctx.attr, i0.ɵɵsanitizeResourceUrl);',
8650-
);
8651-
});
8652-
86538625
it('should not generate sanitizers for URL properties in hostBindings fn in Component', () => {
86548626
env.write(
86558627
`test.ts`,

packages/compiler/src/schema/dom_security_schema.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,6 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
9292
['object', ['codebase', 'data']],
9393
]);
9494

95-
// The below are for Script SVG
96-
// See: https://developer.mozilla.org/en-US/docs/Web/API/SVGScriptElement/href
97-
registerContext(SecurityContext.RESOURCE_URL, SVG_NAMESPACE, [
98-
['script', ['src', 'href', 'xlink:href']],
99-
]);
100-
10195
// Keep this in sync with SECURITY_SENSITIVE_ELEMENTS in packages/core/src/sanitization/sanitization.ts
10296
// The `unknown` elements refer to cases when we need to validate the input/binding in a directive (host bindings)
10397
// and the directive can be applied to multiple different elements (with different tag names). In this case we generate

packages/compiler/src/template_parser/template_preparser.ts

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ const LINK_ELEMENT = 'link';
1414
const LINK_STYLE_REL_ATTR = 'rel';
1515
const LINK_STYLE_HREF_ATTR = 'href';
1616
const LINK_STYLE_REL_VALUE = 'stylesheet';
17-
const STYLE_ELEMENT = 'style';
18-
const SCRIPT_ELEMENT = 'script';
17+
const STYLE_ELEMENTS: ReadonlySet<string> = new Set([':svg:style', 'style']);
18+
const SCRIPT_ELEMENTS: ReadonlySet<string> = new Set([':svg:script', 'script']);
1919
const NG_NON_BINDABLE_ATTR = 'ngNonBindable';
2020
const NG_PROJECT_AS = 'ngProjectAs';
2121

@@ -25,7 +25,8 @@ export function preparseElement(ast: html.Element): PreparsedElement {
2525
let relAttr: string | null = null;
2626
let nonBindable = false;
2727
let projectAs = '';
28-
ast.attrs.forEach((attr) => {
28+
29+
for (const attr of ast.attrs) {
2930
const lcAttrName = attr.name.toLowerCase();
3031
if (lcAttrName == NG_CONTENT_SELECT_ATTR) {
3132
selectAttr = attr.value;
@@ -40,15 +41,18 @@ export function preparseElement(ast: html.Element): PreparsedElement {
4041
projectAs = attr.value;
4142
}
4243
}
43-
});
44-
selectAttr = normalizeNgContentSelect(selectAttr);
44+
}
45+
46+
// Normalize selector to '*' if empty
47+
selectAttr ||= '*';
48+
4549
const nodeName = ast.name.toLowerCase();
4650
let type = PreparsedElementType.OTHER;
4751
if (isNgContent(nodeName)) {
4852
type = PreparsedElementType.NG_CONTENT;
49-
} else if (nodeName == STYLE_ELEMENT) {
53+
} else if (STYLE_ELEMENTS.has(nodeName)) {
5054
type = PreparsedElementType.STYLE;
51-
} else if (nodeName == SCRIPT_ELEMENT) {
55+
} else if (SCRIPT_ELEMENTS.has(nodeName)) {
5256
type = PreparsedElementType.SCRIPT;
5357
} else if (nodeName == LINK_ELEMENT && relAttr == LINK_STYLE_REL_VALUE) {
5458
type = PreparsedElementType.STYLESHEET;
@@ -73,10 +77,3 @@ export class PreparsedElement {
7377
public projectAs: string,
7478
) {}
7579
}
76-
77-
function normalizeNgContentSelect(selectAttr: string | null): string {
78-
if (selectAttr === null || selectAttr.length === 0) {
79-
return '*';
80-
}
81-
return selectAttr;
82-
}

packages/core/src/sanitization/sanitization.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,8 +219,7 @@ const RESOURCE_MAP: Record<string, Record<string, true | undefined> | undefined>
219219
'frame': {'src': true},
220220
'iframe': {'src': true},
221221
'media': {'src': true},
222-
'script': {'src': true, 'href': true, 'xlink:href': true},
223-
':svg:script': {'src': true, 'href': true, 'xlink:href': true},
222+
224223
'base': {'href': true},
225224
'link': {'href': true},
226225
'object': {'data': true, 'codebase': true},

packages/core/test/acceptance/security_spec.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -840,3 +840,17 @@ describe('Component host element validation', () => {
840840
}
841841
});
842842
});
843+
844+
describe('SVG <script> bindings', () => {
845+
it(`should remove svg <script> element`, () => {
846+
@Component({
847+
template: `<svg><script src="https://bad.com/script.js"></script></svg>`,
848+
changeDetection: ChangeDetectionStrategy.Eager,
849+
})
850+
class TestCmp {}
851+
852+
const fixture = TestBed.createComponent(TestCmp);
853+
fixture.detectChanges();
854+
expect(fixture.nativeElement.querySelector('script')).toBeFalsy();
855+
});
856+
});

packages/core/test/sanitization/sanitization_spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ describe('sanitization', () => {
118118
[SecurityContext.RESOURCE_URL, ɵɵsanitizeResourceUrl],
119119
]);
120120
Object.entries(schema).forEach(([key, context]) => {
121-
if (context === SecurityContext.URL || SecurityContext.RESOURCE_URL) {
121+
if (context === SecurityContext.URL || context === SecurityContext.RESOURCE_URL) {
122122
const [tag, prop] = key.split('|');
123123
const contexts = contextsByProp.get(prop) || new Set<number>();
124124
contexts.add(context);
@@ -137,7 +137,7 @@ describe('sanitization', () => {
137137
expect(getUrlSanitizer('IFRAME', 'SRC')).toEqual(ɵɵsanitizeResourceUrl);
138138
expect(getUrlSanitizer('IFRAME', 'src')).toEqual(ɵɵsanitizeResourceUrl);
139139
expect(getUrlSanitizer('iframe', 'SRC')).toEqual(ɵɵsanitizeResourceUrl);
140-
expect(getUrlSanitizer('ScRiPt', 'xLiNk:HrEf')).toEqual(ɵɵsanitizeResourceUrl);
140+
expect(getUrlSanitizer('ScRiPt', 'xLiNk:HrEf')).toEqual(ɵɵsanitizeUrl);
141141
expect(getUrlSanitizer('A', 'HREF')).toEqual(ɵɵsanitizeUrl);
142142
});
143143

@@ -150,8 +150,8 @@ describe('sanitization', () => {
150150

151151
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'iframe', 'SRC')).toThrowError(ERROR);
152152

153-
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'ScRiPt', 'xLiNk:HrEf')).toThrowError(
154-
ERROR,
153+
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:true', 'ScRiPt', 'xLiNk:HrEf')).toEqual(
154+
'unsafe:javascript:true',
155155
);
156156

157157
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:true', 'A', 'HREF')).toEqual(

0 commit comments

Comments
 (0)