From c3aa24c3f9699242331ca014b1616e968c1a7f8c Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Thu, 3 Jan 2019 10:04:06 -0800 Subject: [PATCH] fix(ivy): sanitization for Host Bindings (#27939) This commit adds sanitization for `elementProperty` and `elementAttribute` instructions used in `hostBindings` function, similar to what we already have in the `template` function. Main difference is the fact that for some attributes (like "href" and "src") we can't define which SecurityContext they belong to (URL vs RESOURCE_URL) in Compiler, since information in Directive selector may not be enough to calculate it. In order to resolve the problem, Compiler injects slightly different sanitization function which detects proper Security Context at runtime. PR Close #27939 --- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 151 ++++++++++++++++++ .../compiler/src/render3/r3_identifiers.ts | 2 + .../compiler/src/render3/view/compiler.ts | 47 ++++-- .../compiler/src/render3/view/template.ts | 7 +- .../src/template_parser/binding_parser.ts | 6 + .../core/src/core_render3_private_export.ts | 1 + packages/core/src/render3/instructions.ts | 6 +- .../src/render3/interfaces/sanitization.ts | 2 +- packages/core/src/render3/jit/environment.ts | 3 +- .../core/src/sanitization/sanitization.ts | 33 ++++ .../test/linker/security_integration_spec.ts | 54 +++---- .../core/test/render3/host_binding_spec.ts | 60 ++++++- .../core/test/render3/integration_spec.ts | 52 ++++++ .../test/sanitization/sanatization_spec.ts | 54 ++++++- 14 files changed, 430 insertions(+), 48 deletions(-) diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 30ff6066b0..19b34ac4a7 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1475,6 +1475,157 @@ describe('ngtsc behavioral tests', () => { expect(afterCount).toBe(1); }); + describe('sanitization', () => { + it('should generate sanitizers for unsafe attributes in hostBindings fn in Directives', () => { + env.tsconfig(); + env.write(`test.ts`, ` + import {Component, Directive, HostBinding} from '@angular/core'; + + @Directive({ + selector: '[unsafeAttrs]' + }) + class UnsafeAttrsDirective { + @HostBinding('attr.href') + attrHref: string; + + @HostBinding('attr.src') + attrSrc: string; + + @HostBinding('attr.action') + attrAction: string; + + @HostBinding('attr.profile') + attrProfile: string; + + @HostBinding('attr.innerHTML') + attrInnerHTML: string; + + @HostBinding('attr.title') + attrSafeTitle: string; + } + + @Component({ + selector: 'foo', + template: 'Link Title' + }) + class FooCmp {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const hostBindingsFn = ` + hostBindings: function UnsafeAttrsDirective_HostBindings(rf, ctx, elIndex) { + if (rf & 1) { + i0.ɵallocHostVars(6); + } + if (rf & 2) { + i0.ɵelementAttribute(elIndex, "href", i0.ɵbind(ctx.attrHref), i0.ɵsanitizeUrlOrResourceUrl); + i0.ɵelementAttribute(elIndex, "src", i0.ɵbind(ctx.attrSrc), i0.ɵsanitizeUrlOrResourceUrl); + i0.ɵelementAttribute(elIndex, "action", i0.ɵbind(ctx.attrAction), i0.ɵsanitizeUrl); + i0.ɵelementAttribute(elIndex, "profile", i0.ɵbind(ctx.attrProfile), i0.ɵsanitizeResourceUrl); + i0.ɵelementAttribute(elIndex, "innerHTML", i0.ɵbind(ctx.attrInnerHTML), i0.ɵsanitizeHtml); + i0.ɵelementAttribute(elIndex, "title", i0.ɵbind(ctx.attrSafeTitle)); + } + } + `; + expect(trim(jsContents)).toContain(trim(hostBindingsFn)); + }); + + it('should generate sanitizers for unsafe properties in hostBindings fn in Directives', () => { + env.tsconfig(); + env.write(`test.ts`, ` + import {Component, Directive, HostBinding} from '@angular/core'; + + @Directive({ + selector: '[unsafeProps]' + }) + class UnsafePropsDirective { + @HostBinding('href') + propHref: string; + + @HostBinding('src') + propSrc: string; + + @HostBinding('action') + propAction: string; + + @HostBinding('profile') + propProfile: string; + + @HostBinding('innerHTML') + propInnerHTML: string; + + @HostBinding('title') + propSafeTitle: string; + } + + @Component({ + selector: 'foo', + template: 'Link Title' + }) + class FooCmp {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const hostBindingsFn = ` + hostBindings: function UnsafePropsDirective_HostBindings(rf, ctx, elIndex) { + if (rf & 1) { + i0.ɵallocHostVars(6); + } + if (rf & 2) { + i0.ɵelementProperty(elIndex, "href", i0.ɵbind(ctx.propHref), i0.ɵsanitizeUrlOrResourceUrl, true); + i0.ɵelementProperty(elIndex, "src", i0.ɵbind(ctx.propSrc), i0.ɵsanitizeUrlOrResourceUrl, true); + i0.ɵelementProperty(elIndex, "action", i0.ɵbind(ctx.propAction), i0.ɵsanitizeUrl, true); + i0.ɵelementProperty(elIndex, "profile", i0.ɵbind(ctx.propProfile), i0.ɵsanitizeResourceUrl, true); + i0.ɵelementProperty(elIndex, "innerHTML", i0.ɵbind(ctx.propInnerHTML), i0.ɵsanitizeHtml, true); + i0.ɵelementProperty(elIndex, "title", i0.ɵbind(ctx.propSafeTitle), null, true); + } + } + `; + expect(trim(jsContents)).toContain(trim(hostBindingsFn)); + }); + + it('should not generate sanitizers for URL properties in hostBindings fn in Component', () => { + env.tsconfig(); + env.write(`test.ts`, ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'foo', + template: 'Link Title', + host: { + '[src]': 'srcProp', + '[href]': 'hrefProp', + '[title]': 'titleProp', + '[attr.src]': 'srcAttr', + '[attr.href]': 'hrefAttr', + '[attr.title]': 'titleAttr', + } + }) + class FooCmp {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const hostBindingsFn = ` + hostBindings: function FooCmp_HostBindings(rf, ctx, elIndex) { + if (rf & 1) { + i0.ɵallocHostVars(6); + } + if (rf & 2) { + i0.ɵelementProperty(elIndex, "src", i0.ɵbind(ctx.srcProp), null, true); + i0.ɵelementProperty(elIndex, "href", i0.ɵbind(ctx.hrefProp), null, true); + i0.ɵelementProperty(elIndex, "title", i0.ɵbind(ctx.titleProp), null, true); + i0.ɵelementAttribute(elIndex, "src", i0.ɵbind(ctx.srcAttr)); + i0.ɵelementAttribute(elIndex, "href", i0.ɵbind(ctx.hrefAttr)); + i0.ɵelementAttribute(elIndex, "title", i0.ɵbind(ctx.titleAttr)); + } + } + `; + expect(trim(jsContents)).toContain(trim(hostBindingsFn)); + }); + }); }); function expectTokenAtPosition( diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts index 92c3820425..83139fa7da 100644 --- a/packages/compiler/src/render3/r3_identifiers.ts +++ b/packages/compiler/src/render3/r3_identifiers.ts @@ -213,4 +213,6 @@ export class Identifiers { o.ExternalReference = {name: 'ɵsanitizeResourceUrl', moduleName: CORE}; static sanitizeScript: o.ExternalReference = {name: 'ɵsanitizeScript', moduleName: CORE}; static sanitizeUrl: o.ExternalReference = {name: 'ɵsanitizeUrl', moduleName: CORE}; + static sanitizeUrlOrResourceUrl: + o.ExternalReference = {name: 'ɵsanitizeUrlOrResourceUrl', moduleName: CORE}; } diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index e881270c1b..fcec424ed4 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -30,7 +30,7 @@ import {prepareSyntheticListenerFunctionName, prepareSyntheticPropertyName, type import {R3ComponentDef, R3ComponentMetadata, R3DirectiveDef, R3DirectiveMetadata, R3QueryMetadata} from './api'; import {StylingBuilder, StylingInstruction} from './styling_builder'; -import {BindingScope, TemplateDefinitionBuilder, ValueConverter, prepareEventListenerParameters, renderFlagCheckIfStmt} from './template'; +import {BindingScope, TemplateDefinitionBuilder, ValueConverter, prepareEventListenerParameters, renderFlagCheckIfStmt, resolveSanitizationFn} from './template'; import {CONTEXT_NAME, DefinitionMap, RENDER_FLAGS, TEMPORARY_NAME, asLiteral, conditionallyCreateMapObjectLiteral, getQueryPredicate, temporaryAllocator} from './util'; const EMPTY_ARRAY: any[] = []; @@ -698,15 +698,45 @@ function createHostBindingsFunction( const value = binding.expression.visit(valueConverter); const bindingExpr = bindingFn(bindingContext, value); - const {bindingName, instruction, extraParams} = getBindingNameAndInstruction(binding); + const {bindingName, instruction, isAttribute} = getBindingNameAndInstruction(binding); + + const securityContexts = + bindingParser + .calcPossibleSecurityContexts(meta.selector || '', bindingName, isAttribute) + .filter(context => context !== core.SecurityContext.NONE); + + let sanitizerFn: o.ExternalExpr|null = null; + if (securityContexts.length) { + if (securityContexts.length === 2 && + securityContexts.indexOf(core.SecurityContext.URL) > -1 && + securityContexts.indexOf(core.SecurityContext.RESOURCE_URL) > -1) { + // Special case for some URL attributes (such as "src" and "href") that may be a part of + // different security contexts. In this case we use special santitization function and + // select the actual sanitizer at runtime based on a tag name that is provided while + // invoking sanitization function. + sanitizerFn = o.importExpr(R3.sanitizeUrlOrResourceUrl); + } else { + sanitizerFn = resolveSanitizationFn(securityContexts[0], isAttribute); + } + } const instructionParams: o.Expression[] = [ elVarExp, o.literal(bindingName), o.importExpr(R3.bind).callFn([bindingExpr.currValExpr]) ]; + if (sanitizerFn) { + instructionParams.push(sanitizerFn); + } + if (!isAttribute) { + if (!sanitizerFn) { + // append `null` in front of `nativeOnly` flag if no sanitizer fn defined + instructionParams.push(o.literal(null)); + } + // host bindings must have nativeOnly prop set to true + instructionParams.push(o.literal(true)); + } updateStatements.push(...bindingExpr.stmts); - updateStatements.push( - o.importExpr(instruction).callFn(instructionParams.concat(extraParams)).toStmt()); + updateStatements.push(o.importExpr(instruction).callFn(instructionParams).toStmt()); } } @@ -777,10 +807,9 @@ function createStylingStmt( } function getBindingNameAndInstruction(binding: ParsedProperty): - {bindingName: string, instruction: o.ExternalReference, extraParams: o.Expression[]} { + {bindingName: string, instruction: o.ExternalReference, isAttribute: boolean} { let bindingName = binding.name; let instruction !: o.ExternalReference; - const extraParams: o.Expression[] = []; // Check to see if this is an attr binding or a property binding const attrMatches = bindingName.match(ATTR_REGEX); @@ -797,13 +826,9 @@ function getBindingNameAndInstruction(binding: ParsedProperty): } else { instruction = R3.elementProperty; } - extraParams.push( - o.literal(null), // TODO: This should be a sanitizer fn (FW-785) - o.literal(true) // host bindings must have nativeOnly prop set to true - ); } - return {bindingName, instruction, extraParams}; + return {bindingName, instruction, isAttribute: !!attrMatches}; } function createHostListeners( diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index e9d085bee3..893b85b3fa 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -701,7 +701,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } } else if (instruction) { const params: any[] = []; - const sanitizationRef = resolveSanitizationFn(input, input.securityContext); + const isAttributeBinding = input.type === BindingType.Attribute; + const sanitizationRef = resolveSanitizationFn(input.securityContext, isAttributeBinding); if (sanitizationRef) params.push(sanitizationRef); // TODO(chuckj): runtime: security context @@ -1571,7 +1572,7 @@ export function makeBindingParser( new Parser(new Lexer()), interpolationConfig, new DomElementSchemaRegistry(), null, []); } -function resolveSanitizationFn(input: t.BoundAttribute, context: core.SecurityContext) { +export function resolveSanitizationFn(context: core.SecurityContext, isAttribute?: boolean) { switch (context) { case core.SecurityContext.HTML: return o.importExpr(R3.sanitizeHtml); @@ -1581,7 +1582,7 @@ function resolveSanitizationFn(input: t.BoundAttribute, context: core.SecurityCo // the compiler does not fill in an instruction for [style.prop?] binding // values because the style algorithm knows internally what props are subject // to sanitization (only [attr.style] values are explicitly sanitized) - return input.type === BindingType.Attribute ? o.importExpr(R3.sanitizeStyle) : null; + return isAttribute ? o.importExpr(R3.sanitizeStyle) : null; case core.SecurityContext.URL: return o.importExpr(R3.sanitizeUrl); case core.SecurityContext.RESOURCE_URL: diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index 178fa7c58c..bef79f124b 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -305,6 +305,12 @@ export class BindingParser { } } + calcPossibleSecurityContexts(selector: string, propName: string, isAttribute: boolean): + SecurityContext[] { + const prop = this._schemaRegistry.getMappedPropName(propName); + return calcPossibleSecurityContexts(this._schemaRegistry, selector, prop, isAttribute); + } + private _parseAnimationEvent( name: string, expression: string, sourceSpan: ParseSourceSpan, targetEvents: ParsedEvent[]) { const matches = splitAtPeriod(name, [name, '']); diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index 27741a3070..38c63da7d0 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/packages/core/src/core_render3_private_export.ts @@ -151,6 +151,7 @@ export { sanitizeStyle as ɵsanitizeStyle, sanitizeUrl as ɵsanitizeUrl, sanitizeResourceUrl as ɵsanitizeResourceUrl, + sanitizeUrlOrResourceUrl as ɵsanitizeUrlOrResourceUrl, } from './sanitization/sanitization'; export { diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 65d99a86f9..f3ee6ac4c5 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -976,7 +976,9 @@ export function elementAttribute( element.removeAttribute(name); } else { ngDevMode && ngDevMode.rendererSetAttribute++; - const strValue = sanitizer == null ? stringify(value) : sanitizer(value); + const tNode = getTNode(index, lView); + const strValue = + sanitizer == null ? stringify(value) : sanitizer(value, tNode.tagName || '', name); isProceduralRenderer(renderer) ? renderer.setAttribute(element, name, strValue) : element.setAttribute(name, strValue); } @@ -1059,7 +1061,7 @@ function elementPropertyInternal( const renderer = loadRendererFn ? loadRendererFn(tNode, lView) : lView[RENDERER]; // It is assumed that the sanitizer is only added when the compiler determines that the property // is risky, so sanitization can be done without further checks. - value = sanitizer != null ? (sanitizer(value) as any) : value; + value = sanitizer != null ? (sanitizer(value, tNode.tagName || '', propName) as any) : value; ngDevMode && ngDevMode.rendererSetProperty++; if (isProceduralRenderer(renderer)) { renderer.setProperty(element as RElement, propName, value); diff --git a/packages/core/src/render3/interfaces/sanitization.ts b/packages/core/src/render3/interfaces/sanitization.ts index 24970f93c9..e329c03024 100644 --- a/packages/core/src/render3/interfaces/sanitization.ts +++ b/packages/core/src/render3/interfaces/sanitization.ts @@ -9,4 +9,4 @@ /** * Function used to sanitize the value before writing it into the renderer. */ -export type SanitizerFn = (value: any) => string; +export type SanitizerFn = (value: any, tagName?: string, propName?: string) => string; diff --git a/packages/core/src/render3/jit/environment.ts b/packages/core/src/render3/jit/environment.ts index ffc043661e..6caff12a66 100644 --- a/packages/core/src/render3/jit/environment.ts +++ b/packages/core/src/render3/jit/environment.ts @@ -116,5 +116,6 @@ export const angularCoreEnv: {[name: string]: Function} = { 'ɵdefaultStyleSanitizer': sanitization.defaultStyleSanitizer, 'ɵsanitizeResourceUrl': sanitization.sanitizeResourceUrl, 'ɵsanitizeScript': sanitization.sanitizeScript, - 'ɵsanitizeUrl': sanitization.sanitizeUrl + 'ɵsanitizeUrl': sanitization.sanitizeUrl, + 'ɵsanitizeUrlOrResourceUrl': sanitization.sanitizeUrlOrResourceUrl }; diff --git a/packages/core/src/sanitization/sanitization.ts b/packages/core/src/sanitization/sanitization.ts index 5e03458356..969db77592 100644 --- a/packages/core/src/sanitization/sanitization.ts +++ b/packages/core/src/sanitization/sanitization.ts @@ -132,6 +132,39 @@ export function sanitizeScript(unsafeScript: any): string { throw new Error('unsafe value used in a script context'); } +/** + * Detects which sanitizer to use for URL property, based on tag name and prop name. + * + * The rules are based on the RESOURCE_URL context config from + * `packages/compiler/src/schema/dom_security_schema.ts`. + * If tag and prop names don't match Resource URL schema, use URL sanitizer. + */ +export function getUrlSanitizer(tag: string, prop: string) { + if ((prop === 'src' && (tag === 'embed' || tag === 'frame' || tag === 'iframe' || + tag === 'media' || tag === 'script')) || + (prop === 'href' && (tag === 'base' || tag === 'link'))) { + return sanitizeResourceUrl; + } + return sanitizeUrl; +} + +/** + * Sanitizes URL, selecting sanitizer function based on tag and property names. + * + * This function is used in case we can't define security context at compile time, when only prop + * name is available. This happens when we generate host bindings for Directives/Components. The + * host element is unknown at compile time, so we defer calculation of specific sanitizer to + * runtime. + * + * @param unsafeUrl untrusted `url`, typically from the user. + * @param tag target element tag name. + * @param prop name of the property that contains the value. + * @returns `url` string which is safe to bind. + */ +export function sanitizeUrlOrResourceUrl(unsafeUrl: any, tag: string, prop: string): any { + return getUrlSanitizer(tag, prop)(unsafeUrl); +} + /** * The default style sanitizer will handle sanitization for style properties by * sanitizing any CSS property that can include a `url` value (usually image-based properties) diff --git a/packages/core/test/linker/security_integration_spec.ts b/packages/core/test/linker/security_integration_spec.ts index 9fcab8d774..8baa99918c 100644 --- a/packages/core/test/linker/security_integration_spec.ts +++ b/packages/core/test/linker/security_integration_spec.ts @@ -171,39 +171,37 @@ function declareTests(config?: {useJit: boolean}) { checkEscapeOfHrefProperty(fixture, true); }); - fixmeIvy('FW-785: Host bindings are not sanitised') - .it('should escape unsafe properties if they are used in host bindings', () => { - @Directive({selector: '[dirHref]'}) - class HrefDirective { - // TODO(issue/24571): remove '!'. - @HostBinding('href') @Input() - dirHref !: string; - } + it('should escape unsafe properties if they are used in host bindings', () => { + @Directive({selector: '[dirHref]'}) + class HrefDirective { + // TODO(issue/24571): remove '!'. + @HostBinding('href') @Input() + dirHref !: string; + } - const template = `Link Title`; - TestBed.configureTestingModule({declarations: [HrefDirective]}); - TestBed.overrideComponent(SecuredComponent, {set: {template}}); - const fixture = TestBed.createComponent(SecuredComponent); + const template = `Link Title`; + TestBed.configureTestingModule({declarations: [HrefDirective]}); + TestBed.overrideComponent(SecuredComponent, {set: {template}}); + const fixture = TestBed.createComponent(SecuredComponent); - checkEscapeOfHrefProperty(fixture, false); - }); + checkEscapeOfHrefProperty(fixture, false); + }); - fixmeIvy('FW-785: Host bindings are not sanitised') - .it('should escape unsafe attributes if they are used in host bindings', () => { - @Directive({selector: '[dirHref]'}) - class HrefDirective { - // TODO(issue/24571): remove '!'. - @HostBinding('attr.href') @Input() - dirHref !: string; - } + it('should escape unsafe attributes if they are used in host bindings', () => { + @Directive({selector: '[dirHref]'}) + class HrefDirective { + // TODO(issue/24571): remove '!'. + @HostBinding('attr.href') @Input() + dirHref !: string; + } - const template = `Link Title`; - TestBed.configureTestingModule({declarations: [HrefDirective]}); - TestBed.overrideComponent(SecuredComponent, {set: {template}}); - const fixture = TestBed.createComponent(SecuredComponent); + const template = `Link Title`; + TestBed.configureTestingModule({declarations: [HrefDirective]}); + TestBed.overrideComponent(SecuredComponent, {set: {template}}); + const fixture = TestBed.createComponent(SecuredComponent); - checkEscapeOfHrefProperty(fixture, true); - }); + checkEscapeOfHrefProperty(fixture, true); + }); it('should escape unsafe style values', () => { const template = `
Text
`; diff --git a/packages/core/test/render3/host_binding_spec.ts b/packages/core/test/render3/host_binding_spec.ts index f3a7abf829..424456d816 100644 --- a/packages/core/test/render3/host_binding_spec.ts +++ b/packages/core/test/render3/host_binding_spec.ts @@ -9,10 +9,12 @@ import {ElementRef, QueryList} from '@angular/core'; import {AttributeMarker, defineComponent, template, defineDirective, InheritDefinitionFeature, ProvidersFeature, NgOnChangesFeature} from '../../src/render3/index'; -import {allocHostVars, bind, directiveInject, element, elementEnd, elementProperty, elementStyleProp, elementStyling, elementStylingApply, elementStart, listener, load, text, textBinding, loadQueryList, registerContentQuery, elementHostAttrs} from '../../src/render3/instructions'; +import {allocHostVars, bind, directiveInject, element, elementAttribute, elementEnd, elementProperty, elementStyleProp, elementStyling, elementStylingApply, elementStart, listener, load, text, textBinding, loadQueryList, registerContentQuery, elementHostAttrs} from '../../src/render3/instructions'; import {query, queryRefresh} from '../../src/render3/query'; import {RenderFlags} from '../../src/render3/interfaces/definition'; import {pureFunction1, pureFunction2} from '../../src/render3/pure_function'; +import {sanitizeUrl, sanitizeUrlOrResourceUrl, sanitizeHtml} from '../../src/sanitization/sanitization'; +import {bypassSanitizationTrustHtml, bypassSanitizationTrustResourceUrl, bypassSanitizationTrustUrl} from '../../src/sanitization/bypass'; import {ComponentFixture, TemplateFixture, createComponent, createDirective} from './render_util'; import {NgForOf} from './common_with_def'; @@ -1164,4 +1166,60 @@ describe('host bindings', () => { expect(hostBindingEl.className).toEqual('mat-toolbar'); }); }); + + describe('sanitization', () => { + function verify( + tag: string, prop: string, value: any, expectedSanitizedValue: any, sanitizeFn: any, + bypassFn: any, isAttribute: boolean = true) { + it('should sanitize potentially unsafe properties and attributes', () => { + let hostBindingDir: UnsafeUrlHostBindingDir; + class UnsafeUrlHostBindingDir { + // val: any = value; + static ngDirectiveDef = defineDirective({ + type: UnsafeUrlHostBindingDir, + selectors: [['', 'unsafeUrlHostBindingDir', '']], + factory: () => hostBindingDir = new UnsafeUrlHostBindingDir(), + hostBindings: (rf: RenderFlags, ctx: any, elementIndex: number) => { + if (rf & RenderFlags.Create) { + allocHostVars(1); + } + if (rf & RenderFlags.Update) { + const fn = isAttribute ? elementAttribute : elementProperty; + (fn as any)(elementIndex, prop, bind(ctx[prop]), sanitizeFn, true); + } + } + }); + } + + const fixture = new TemplateFixture(() => { + element(0, tag, ['unsafeUrlHostBindingDir', '']); + }, () => {}, 1, 0, [UnsafeUrlHostBindingDir]); + + const el = fixture.hostElement.querySelector(tag) !; + const current = () => isAttribute ? el.getAttribute(prop) : (el as any)[prop]; + + (hostBindingDir !as any)[prop] = value; + fixture.update(); + expect(current()).toEqual(expectedSanitizedValue); + + (hostBindingDir !as any)[prop] = bypassFn(value); + fixture.update(); + expect(current()).toEqual(value); + }); + } + + verify( + 'a', 'href', 'javascript:alert(1)', 'unsafe:javascript:alert(1)', sanitizeUrlOrResourceUrl, + bypassSanitizationTrustUrl); + verify( + 'script', 'src', bypassSanitizationTrustResourceUrl('javascript:alert(2)'), + 'javascript:alert(2)', sanitizeUrlOrResourceUrl, bypassSanitizationTrustResourceUrl); + verify( + 'blockquote', 'cite', 'javascript:alert(3)', 'unsafe:javascript:alert(3)', sanitizeUrl, + bypassSanitizationTrustUrl); + verify( + 'b', 'innerHTML', '', + '', sanitizeHtml, bypassSanitizationTrustHtml, + /* isAttribute */ false); + }); }); diff --git a/packages/core/test/render3/integration_spec.ts b/packages/core/test/render3/integration_spec.ts index 0a4c784e41..4acfc3ce65 100644 --- a/packages/core/test/render3/integration_spec.ts +++ b/packages/core/test/render3/integration_spec.ts @@ -2878,6 +2878,58 @@ describe('render3 integration test', () => { expect(anchor.getAttribute('href')).toEqual('http://foo'); }); + + it('should sanitize HostBindings data using provided sanitization interface', () => { + let hostBindingDir: UnsafeUrlHostBindingDir; + class UnsafeUrlHostBindingDir { + // @HostBinding() + cite: any = 'http://cite-dir-value'; + + static ngDirectiveDef = defineDirective({ + type: UnsafeUrlHostBindingDir, + selectors: [['', 'unsafeUrlHostBindingDir', '']], + factory: () => hostBindingDir = new UnsafeUrlHostBindingDir(), + hostBindings: (rf: RenderFlags, ctx: any, elementIndex: number) => { + if (rf & RenderFlags.Create) { + allocHostVars(1); + } + if (rf & RenderFlags.Update) { + elementProperty(elementIndex, 'cite', bind(ctx.cite), sanitizeUrl, true); + } + } + }); + } + + class SimpleComp { + static ngComponentDef = defineComponent({ + type: SimpleComp, + selectors: [['sanitize-this']], + factory: () => new SimpleComp(), + consts: 1, + vars: 0, + template: (rf: RenderFlags, ctx: SimpleComp) => { + if (rf & RenderFlags.Create) { + element(0, 'blockquote', ['unsafeUrlHostBindingDir', '']); + } + }, + directives: [UnsafeUrlHostBindingDir] + }); + } + + const sanitizer = new LocalSanitizer((value) => 'http://bar'); + + const fixture = new ComponentFixture(SimpleComp, {sanitizer}); + hostBindingDir !.cite = 'http://foo'; + fixture.update(); + + const anchor = fixture.hostElement.querySelector('blockquote') !; + expect(anchor.getAttribute('cite')).toEqual('http://bar'); + + hostBindingDir !.cite = sanitizer.bypassSecurityTrustUrl('http://foo'); + fixture.update(); + + expect(anchor.getAttribute('cite')).toEqual('http://foo'); + }); }); }); diff --git a/packages/core/test/sanitization/sanatization_spec.ts b/packages/core/test/sanitization/sanatization_spec.ts index db61da433d..5d538b6305 100644 --- a/packages/core/test/sanitization/sanatization_spec.ts +++ b/packages/core/test/sanitization/sanatization_spec.ts @@ -7,10 +7,12 @@ * found in the LICENSE file at https://angular.io/license */ +import {SECURITY_SCHEMA} from '@angular/compiler/src/schema/dom_security_schema'; import {setTNodeAndViewData} from '@angular/core/src/render3/state'; import {bypassSanitizationTrustHtml, bypassSanitizationTrustResourceUrl, bypassSanitizationTrustScript, bypassSanitizationTrustStyle, bypassSanitizationTrustUrl} from '../../src/sanitization/bypass'; -import {sanitizeHtml, sanitizeResourceUrl, sanitizeScript, sanitizeStyle, sanitizeUrl} from '../../src/sanitization/sanitization'; +import {getUrlSanitizer, sanitizeHtml, sanitizeResourceUrl, sanitizeScript, sanitizeStyle, sanitizeUrl, sanitizeUrlOrResourceUrl} from '../../src/sanitization/sanitization'; +import {SecurityContext} from '../../src/sanitization/security'; describe('sanitization', () => { beforeEach(() => setTNodeAndViewData(null !, [] as any)); @@ -69,4 +71,54 @@ describe('sanitization', () => { expect(() => sanitizeScript(bypassSanitizationTrustHtml('true'))).toThrowError(ERROR); expect(sanitizeScript(bypassSanitizationTrustScript('true'))).toEqual('true'); }); + + it('should select correct sanitizer for URL props', () => { + // making sure security schema we have on compiler side is in sync with the `getUrlSanitizer` + // runtime function definition + const schema = SECURITY_SCHEMA(); + const contextsByProp: Map> = new Map(); + const sanitizerNameByContext: Map = new Map([ + [SecurityContext.URL, 'sanitizeUrl'], [SecurityContext.RESOURCE_URL, 'sanitizeResourceUrl'] + ]); + Object.keys(schema).forEach(key => { + const context = schema[key]; + if (context === SecurityContext.URL || SecurityContext.RESOURCE_URL) { + const [tag, prop] = key.split('|'); + const contexts = contextsByProp.get(prop) || new Set(); + contexts.add(context); + contextsByProp.set(prop, contexts); + // check only in case a prop can be a part of both URL contexts + if (contexts.size === 2) { + expect(getUrlSanitizer(tag, prop).name).toEqual(sanitizerNameByContext.get(context) !); + } + } + }); + }); + + it('should sanitize resourceUrls via sanitizeUrlOrResourceUrl', () => { + const ERROR = 'unsafe value used in a resource URL context (see http://g.co/ng/security#xss)'; + expect(() => sanitizeUrlOrResourceUrl('http://server', 'iframe', 'src')).toThrowError(ERROR); + expect(() => sanitizeUrlOrResourceUrl('javascript:true', 'iframe', 'src')).toThrowError(ERROR); + expect( + () => sanitizeUrlOrResourceUrl( + bypassSanitizationTrustHtml('javascript:true'), 'iframe', 'src')) + .toThrowError(ERROR); + expect(sanitizeUrlOrResourceUrl( + bypassSanitizationTrustResourceUrl('javascript:true'), 'iframe', 'src')) + .toEqual('javascript:true'); + }); + + it('should sanitize urls via sanitizeUrlOrResourceUrl', () => { + expect(sanitizeUrlOrResourceUrl('http://server', 'a', 'href')).toEqual('http://server'); + expect(sanitizeUrlOrResourceUrl(new Wrap('http://server'), 'a', 'href')) + .toEqual('http://server'); + expect(sanitizeUrlOrResourceUrl('javascript:true', 'a', 'href')) + .toEqual('unsafe:javascript:true'); + expect(sanitizeUrlOrResourceUrl(new Wrap('javascript:true'), 'a', 'href')) + .toEqual('unsafe:javascript:true'); + expect(sanitizeUrlOrResourceUrl(bypassSanitizationTrustHtml('javascript:true'), 'a', 'href')) + .toEqual('unsafe:javascript:true'); + expect(sanitizeUrlOrResourceUrl(bypassSanitizationTrustUrl('javascript:true'), 'a', 'href')) + .toEqual('javascript:true'); + }); });