FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(compiler): strip namespaced SVG script elements during template c… · angular/angular@fe1207e · GitHub

/ angular Public

Commit fe1207e

Browse files
alan-agius4
committed
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.
1 parent 3632fa4 commit fe1207e

6 files changed

Lines changed: 102 additions & 51 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
@@ -8278,34 +8278,6 @@ runInEachFileSystem((os: string) => {
82788278
expect(trim(jsContents)).toContain(trim(hostBindingsFn));
82798279
});
82808280

8281-
it('should generate sanitizers for URL properties in SVG script fn in Component', () => {
8282-
env.write(
8283-
'test.ts',
8284-
`
8285-
import {Component} from '@angular/core';
8286-
8287-
@Component({
8288-
selector: 'test-cmp',
8289-
template: \`
8290-
<svg>
8291-
<script [attr.xlink:href]="attr" [attr.href]="attr"></script>
8292-
</svg>
8293-
\`,
8294-
})
8295-
export class TestCmp {
8296-
attr = './script.js';
8297-
}
8298-
`,
8299-
);
8300-
8301-
env.driveMain();
8302-
8303-
const jsContents = env.getContents('test.js');
8304-
expect(jsContents).toContain(
8305-
'i0.ɵɵattribute("href", ctx.attr, i0.ɵɵsanitizeResourceUrl, "xlink")("href", ctx.attr, i0.ɵɵsanitizeResourceUrl);',
8306-
);
8307-
});
8308-
83098281
it('should not generate sanitizers for URL properties in hostBindings fn in Component', () => {
83108282
env.write(
83118283
`test.ts`,

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

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

118-
// The below are for Script SVG
119-
// See: https://developer.mozilla.org/en-US/docs/Web/API/SVGScriptElement/href
120-
registerContext(SecurityContext.RESOURCE_URL, SVG_NAMESPACE, [
121-
['script', ['src', 'href', 'xlink:href']],
122-
]);
123-
124118
// Keep this in sync with SECURITY_SENSITIVE_ELEMENTS in packages/core/src/sanitization/sanitization.ts
125119
// Unknown is the internal tag name for unknown elements example used for host-bindings.
126120
// These are unsafe as `attributeName` can be `href` or `xlink:href`

‎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: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,11 @@
88

99
import {NgIf} from '@angular/common';
1010
import {
11+
ChangeDetectionStrategy,
1112
Component,
13+
createComponent,
1214
Directive,
15+
EnvironmentInjector,
1316
inject,
1417
TemplateRef,
1518
Type,
@@ -838,3 +841,64 @@ describe('innerHTML processing', () => {
838841
expect(fixture.nativeElement.innerHTML).not.toContain('action');
839842
});
840843
});
844+
describe('Component host element validation', () => {
845+
it('should throw an error when dynamically mounting a component onto a script tag', () => {
846+
@Component({
847+
selector: 'my-sink',
848+
template: '',
849+
})
850+
class MySink {}
851+
852+
const scriptHost = document.createElement('script');
853+
document.head.appendChild(scriptHost);
854+
855+
try {
856+
const environmentInjector = TestBed.inject(EnvironmentInjector);
857+
expect(() => {
858+
createComponent(MySink, {
859+
environmentInjector,
860+
hostElement: scriptHost,
861+
});
862+
}).toThrowError(/"<script>" tag is not allowed as a component host element/);
863+
} finally {
864+
scriptHost.remove();
865+
}
866+
});
867+
868+
it('should throw an error when dynamically mounting a component onto an SVG script tag', () => {
869+
@Component({
870+
selector: 'my-svg-sink',
871+
template: '',
872+
})
873+
class MySvgSink {}
874+
875+
const svgScriptHost = document.createElementNS('http://www.w3.org/2000/svg', 'script');
876+
document.head.appendChild(svgScriptHost);
877+
878+
try {
879+
const environmentInjector = TestBed.inject(EnvironmentInjector);
880+
expect(() => {
881+
createComponent(MySvgSink, {
882+
environmentInjector,
883+
hostElement: svgScriptHost,
884+
});
885+
}).toThrowError(/"<script>" tag is not allowed as a component host element/);
886+
} finally {
887+
svgScriptHost.remove();
888+
}
889+
});
890+
});
891+
892+
describe('SVG <script> bindings', () => {
893+
it(`should remove svg <script> element`, () => {
894+
@Component({
895+
template: `<svg><script src="https://bad.com/script.js"></script></svg>`,
896+
changeDetection: ChangeDetectionStrategy.Default,
897+
})
898+
class TestCmp {}
899+
900+
const fixture = TestBed.createComponent(TestCmp);
901+
fixture.detectChanges();
902+
expect(fixture.nativeElement.querySelector('script')).toBeFalsy();
903+
});
904+
});

‎packages/core/test/sanitization/sanitization_spec.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ describe('sanitization', () => {
117117
[SecurityContext.RESOURCE_URL, ɵɵsanitizeResourceUrl],
118118
]);
119119
Object.entries(schema).forEach(([key, context]) => {
120-
if (context === SecurityContext.URL || SecurityContext.RESOURCE_URL) {
120+
if (context === SecurityContext.URL || context === SecurityContext.RESOURCE_URL) {
121121
const [tag, prop] = key.split('|');
122122
const contexts = contextsByProp.get(prop) || new Set<number>();
123123
contexts.add(context);
@@ -132,6 +132,31 @@ describe('sanitization', () => {
132132
});
133133
});
134134

135+
it('should select URL sanitizer case-insensitively', () => {
136+
expect(getUrlSanitizer('IFRAME', 'SRC')).toEqual(ɵɵsanitizeResourceUrl);
137+
expect(getUrlSanitizer('IFRAME', 'src')).toEqual(ɵɵsanitizeResourceUrl);
138+
expect(getUrlSanitizer('iframe', 'SRC')).toEqual(ɵɵsanitizeResourceUrl);
139+
expect(getUrlSanitizer('ScRiPt', 'xLiNk:HrEf')).toEqual(ɵɵsanitizeUrl);
140+
expect(getUrlSanitizer('A', 'HREF')).toEqual(ɵɵsanitizeUrl);
141+
});
142+
143+
it('should sanitize URL or ResourceURL case-insensitively', () => {
144+
const ERROR = /NG0904: unsafe value used in a resource URL context.*/;
145+
146+
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'IFRAME', 'SRC')).toThrowError(ERROR);
147+
148+
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'IFRAME', 'src')).toThrowError(ERROR);
149+
150+
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'iframe', 'SRC')).toThrowError(ERROR);
151+
152+
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:true', 'ScRiPt', 'xLiNk:HrEf')).toEqual(
153+
'unsafe:javascript:true',
154+
);
155+
156+
expect(ɵɵsanitizeUrlOrResourceUrl('javascript:true', 'A', 'HREF')).toEqual(
157+
'unsafe:javascript:true',
158+
);
159+
});
135160
it('should sanitize resourceUrls via sanitizeUrlOrResourceUrl', () => {
136161
const ERROR = /NG0904: unsafe value used in a resource URL context.*/;
137162
expect(() => ɵɵsanitizeUrlOrResourceUrl('http://server', 'iframe', 'src')).toThrowError(ERROR);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL