Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8595,7 +8595,7 @@ runInEachFileSystem((os: string) => {
hostVars: 6,
hostBindings: function UnsafeAttrsDirective_HostBindings(rf, ctx) {
if (rf & 2) {
i0.ɵɵattribute("href", ctx.attrHref, i0.ɵɵsanitizeUrlOrResourceUrl)("src", ctx.attrSrc, i0.ɵɵsanitizeUrlOrResourceUrl)("action", ctx.attrAction, i0.ɵɵsanitizeUrl)("profile", ctx.attrProfile)("innerHTML", ctx.attrInnerHTML, i0.ɵɵsanitizeHtml)("title", ctx.attrSafeTitle);
i0.ɵɵattribute("href", ctx.attrHref, i0.ɵɵsanitizeUrlOrResourceUrl)("src", ctx.attrSrc, i0.ɵɵsanitizeUrlOrResourceUrl)("action", ctx.attrAction, i0.ɵɵsanitizeUrl)("profile", ctx.attrProfile)("innerHTML", ctx.attrInnerHTML, i0.ɵɵsanitizeMaybeScript)("title", ctx.attrSafeTitle);
}
}
`;
Expand Down
4 changes: 4 additions & 0 deletions packages/compiler/src/render3/r3_identifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,10 @@ export class Identifiers {
name: 'ɵɵsanitizeUrlOrResourceUrl',
moduleName: CORE,
};
static sanitizeMaybeScript: o.ExternalReference = {
name: 'ɵɵsanitizeMaybeScript',
moduleName: CORE,
};
static trustConstantHtml: o.ExternalReference = {name: 'ɵɵtrustConstantHtml', moduleName: CORE};
static trustConstantResourceUrl: o.ExternalReference = {
name: 'ɵɵtrustConstantResourceUrl',
Expand Down
13 changes: 12 additions & 1 deletion packages/compiler/src/schema/dom_security_schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,18 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {

registerContext(SecurityContext.HTML, ['iframe|srcdoc', '*|innerHTML', '*|outerHTML']);
registerContext(SecurityContext.STYLE, ['*|style']);
// NB: no SCRIPT contexts here, they are never allowed due to the parser stripping them.
// Writes to text-like properties of a `<script>` element become live script source. SVG
// `<script>` is preserved by the template parser (`mergeNsAndName('svg', 'script')` produces
// `:svg:script`, which `preparseElement` does not strip), so this matters at runtime even
// though top-level HTML `<script>` is stripped at compile time. The binding parser performs
// schema lookups using the local tag name (`script`).
Comment on lines +31 to +35
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we considered revisiting this? If an element with script tag within an SVG-namespaced tree behaves like a script tag—I am actually surprised it works this way, or is Angular just not creating the element with the SVG namespace?—then I'd expect we treat it equally to a script element outside of an SVG structure (or MathML namespaced tree).

For defence in depth it may be desirable to keep the new security contexts in place, but I don't see a reason to keep the ingestion pipeline as it is today if that doesn't accurately reflect how this behaves at runtime.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this behavior for SVGScriptElement and how it works according to the HTML spec. I think the Angular compiler follows the same browser behavior, although I may still have a gap in my understanding.

Regarding MathML, when placing content inside a <script> tag (tested locally), the script is not executed, it is treated as plain text instead. Also, to achieve this, we need to disable schema validation errors, since as I understand it, putting <script> inside MathML is not allowed by default.

This change is essentially a mirror of the existing URL sink behavior. Also, if we remove this host binding / binding handling, SVG script bindings will fail.

Test failed removing this

<!-- SVG <script> via host binding !-->
'<svg><script scriptHost></script></svg>'
<!-- Bindings !-->
`<svg><script [${propName}]="code"></script></svg>

At runtime, I believe there are other issues that I'm not sure are desirable to report here https://issuetracker.google.com/u/1/issues/510537066
Or maybe it always worked this way (I can't find anything in the documentation or code that indicates this is how it should be, but maybe it is correct).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even know SVGScriptElement is a thing!

registerContext(SecurityContext.SCRIPT, [
'script|innerHTML',
'script|outerHTML',
'script|text',
'script|textContent',
'script|innerText',
]);
registerContext(SecurityContext.URL, [
'*|formAction',
'area|href',
Expand Down
31 changes: 21 additions & 10 deletions packages/compiler/src/template/pipeline/src/ingest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,19 +118,30 @@ export function ingestHostBinding(
if (property.isAnimation) {
bindingKind = ir.BindingKind.Animation;
}
const securityContexts = bindingParser
.calcPossibleSecurityContexts(
input.componentSelector,
property.name,
bindingKind === ir.BindingKind.Attribute,
)
.filter((context) => context !== SecurityContext.NONE);
const rawSecurityContexts = bindingParser.calcPossibleSecurityContexts(
input.componentSelector,
property.name,
bindingKind === ir.BindingKind.Attribute,
);
// Preserve `NONE` when it co-occurs with `SCRIPT` so the host element's tag can be
// disambiguated at runtime by the `ɵɵsanitizeMaybeScript` dispatcher; otherwise filter
// `NONE` out as a non-sensitive context.
const keepNone = rawSecurityContexts.includes(SecurityContext.SCRIPT);
const securityContexts = rawSecurityContexts.filter(
(context) => keepNone || context !== SecurityContext.NONE,
);
ingestDomProperty(job, property, bindingKind, securityContexts);
}
for (const [name, expr] of Object.entries(input.attributes) ?? []) {
const securityContexts = bindingParser
.calcPossibleSecurityContexts(input.componentSelector, name, true)
.filter((context) => context !== SecurityContext.NONE);
const rawSecurityContexts = bindingParser.calcPossibleSecurityContexts(
input.componentSelector,
name,
true,
);
const keepNone = rawSecurityContexts.includes(SecurityContext.SCRIPT);
const securityContexts = rawSecurityContexts.filter(
(context) => keepNone || context !== SecurityContext.NONE,
);
ingestHostAttribute(job, name, expr, securityContexts);
}
for (const event of input.events ?? []) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ export function resolveSanitizers(job: CompilationJob): void {
// sanitization function and select the actual sanitizer at runtime based on a tag name
// that is provided while invoking sanitization function.
sanitizerFn = Identifiers.sanitizeUrlOrResourceUrl;
} else if (
Array.isArray(op.securityContext) &&
op.securityContext.includes(SecurityContext.SCRIPT)
) {
// Same reason as above: text-like properties of `<script>` (HTML or SVG) are in
// SecurityContext.SCRIPT, but the same property name on any other tag is HTML or NONE.
// Defer the choice to runtime via a dispatcher that inspects the tag name.
sanitizerFn = Identifiers.sanitizeMaybeScript;
} else {
sanitizerFn = sanitizerFns.get(getOnlySecurityContext(op.securityContext)) ?? null;
}
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/core_render3_private_export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ export {
} from './sanitization/bypass';
export {
ɵɵsanitizeHtml,
ɵɵsanitizeMaybeScript,
ɵɵsanitizeResourceUrl,
ɵɵsanitizeScript,
ɵɵsanitizeStyle,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/render3/jit/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ export const angularCoreEnv: {[name: string]: unknown} = (() => ({
'ɵɵvalidateAttribute': sanitization.ɵɵvalidateAttribute,
'ɵɵsanitizeUrl': sanitization.ɵɵsanitizeUrl,
'ɵɵsanitizeUrlOrResourceUrl': sanitization.ɵɵsanitizeUrlOrResourceUrl,
'ɵɵsanitizeMaybeScript': sanitization.ɵɵsanitizeMaybeScript,
'ɵɵtrustConstantHtml': sanitization.ɵɵtrustConstantHtml,
'ɵɵtrustConstantResourceUrl': sanitization.ɵɵtrustConstantResourceUrl,

Expand Down
18 changes: 18 additions & 0 deletions packages/core/src/sanitization/sanitization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,24 @@ export function ɵɵsanitizeUrlOrResourceUrl(unsafeUrl: any, tag: string, prop:
return getUrlSanitizer(tag, prop)(unsafeUrl);
}

/**
* Sanitizes a value bound to a property whose security context depends on the host tag — used
* for host bindings where the host element is unknown at compile time and the property may be a
* script-source sink on `<script>` or otherwise an HTML/text sink on any other tag.
*
* @codeGenApi
*/
export function ɵɵsanitizeMaybeScript(unsafeValue: any, tag: string, prop: string): any {
if (tag.toLowerCase() === 'script') {
return ɵɵsanitizeScript(unsafeValue);
}
const lowerProp = prop.toLowerCase();
if (lowerProp === 'innerhtml' || lowerProp === 'outerhtml') {
return ɵɵsanitizeHtml(unsafeValue);
}
return unsafeValue;
}
Comment on lines +267 to +276
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need that additional instruction ? Couldn't the compiler determine the right instruction at compile time ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added for host binding cases where we only know at runtime which tag can be used, the directive/component, as exemplified in the tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also another similar report regarding host binding, but I believe it would be outside the scope of this PR, or I'm unsure if it's supposed to work this way.

See https://issuetracker.google.com/u/1/issues/510537066


export function validateAgainstEventProperties(name: string) {
if (name.toLowerCase().startsWith('on')) {
const errorMessage =
Expand Down
55 changes: 55 additions & 0 deletions packages/core/test/acceptance/security_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -852,3 +852,58 @@ describe('innerHTML processing', () => {
expect(fixture.nativeElement.innerHTML).not.toContain('action');
});
});

describe('SVG <script> bindings', () => {
const SCRIPT_TEXT_PROPS = ['text', 'textContent', 'innerText', 'innerHTML', 'outerHTML'];

for (const propName of SCRIPT_TEXT_PROPS) {
it(`should error when '${propName}' is bound on an SVG <script>`, () => {
@Component({
template: `<svg><script [${propName}]="code"></script></svg>`,
changeDetection: ChangeDetectionStrategy.Eager,
})
class TestCmp {
code = '/* xss */';
}

expect(() => {
const fixture = TestBed.createComponent(TestCmp);
fixture.detectChanges();
}).toThrowError(/NG0905/);
});

it(`should error when '${propName}' is bound on an SVG <script> via host binding`, () => {
@Directive({selector: '[scriptHost]', host: {[`[${propName}]`]: 'code'}})
class ScriptHostDir {
code = '/* xss */';
}

@Component({
template: '<svg><script scriptHost></script></svg>',
imports: [ScriptHostDir],
changeDetection: ChangeDetectionStrategy.Eager,
})
class TestCmp {}

expect(() => {
const fixture = TestBed.createComponent(TestCmp);
fixture.detectChanges();
}).toThrowError(/NG0905/);
});
}

it('should allow values trusted via DomSanitizer.bypassSecurityTrustScript', () => {
@Component({
template: '<svg><script [textContent]="code"></script></svg>',
changeDetection: ChangeDetectionStrategy.Eager,
})
class TestCmp {
private readonly sanitizer = inject(DomSanitizer);
code = this.sanitizer.bypassSecurityTrustScript('/* trusted */');
}

const fixture = TestBed.createComponent(TestCmp);
fixture.detectChanges();
expect(fixture.nativeElement.querySelector('script').textContent).toContain('/* trusted */');
});
});
2 changes: 1 addition & 1 deletion packages/core/test/sanitization/sanitization_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe('sanitization', () => {
[SecurityContext.RESOURCE_URL, ɵɵsanitizeResourceUrl],
]);
Object.entries(schema).forEach(([key, context]) => {
if (context === SecurityContext.URL || SecurityContext.RESOURCE_URL) {
if (context === SecurityContext.URL || context === SecurityContext.RESOURCE_URL) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this while writing the unit tests (apparently it was always evaluated as true if I'm not mistaken)

const [tag, prop] = key.split('|');
const contexts = contextsByProp.get(prop) || new Set<number>();
contexts.add(context);
Expand Down
Loading