Skip to content

Commit

Permalink
fix(ivy): sanitization for Host Bindings (angular#27939)
Browse files Browse the repository at this point in the history
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 angular#27939
  • Loading branch information
AndrewKushnir authored and mhevery committed Jan 9, 2019
1 parent 1de4031 commit b2db7a2
Show file tree
Hide file tree
Showing 67 changed files with 523 additions and 100 deletions.
151 changes: 151 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<a [unsafeAttrs]="ctxProp">Link Title</a>'
})
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: '<a [unsafeProps]="ctxProp">Link Title</a>'
})
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: '<a href="example.com">Link Title</a>',
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<T extends ts.Node>(
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler/src/render3/r3_identifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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};
}
47 changes: 36 additions & 11 deletions packages/compiler/src/render3/view/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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());
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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(
Expand Down
7 changes: 4 additions & 3 deletions packages/compiler/src/render3/view/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, 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
Expand Down Expand Up @@ -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);
Expand All @@ -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:
Expand Down
6 changes: 6 additions & 0 deletions packages/compiler/src/template_parser/binding_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, '']);
Expand Down
2 changes: 2 additions & 0 deletions packages/core/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ ng_module(
),
module_name = "@angular/core",
deps = [
"//packages/core/src/interfaces",
"//packages/core/src/utils",
"//packages:types",
"@ngdeps//zone.js",
"@rxjs",
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/application_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {APP_BOOTSTRAP_LISTENER, PLATFORM_INITIALIZER} from './application_tokens
import {Console} from './console';
import {Injectable, InjectionToken, Injector, StaticProvider} from './di';
import {ErrorHandler} from './error_handler';
import {Type} from './interfaces/type';
import {isDevMode} from './is_dev_mode';
import {CompilerFactory, CompilerOptions} from './linker/compiler';
import {ComponentFactory, ComponentRef} from './linker/component_factory';
Expand All @@ -24,7 +25,6 @@ import {WtfScopeFn, wtfCreateScope, wtfLeave} from './profile/profile';
import {assertNgModuleType} from './render3/assert';
import {NgModuleFactory as R3NgModuleFactory} from './render3/ng_module_ref';
import {Testability, TestabilityRegistry} from './testability/testability';
import {Type} from './type';
import {scheduleMicroTask, stringify} from './util';
import {isPromise} from './util/lang';
import {NgZone, NoopNgZone} from './zone/ng_zone';
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export * from './platform_core_providers';
export {TRANSLATIONS, TRANSLATIONS_FORMAT, LOCALE_ID, MissingTranslationStrategy} from './i18n/tokens';
export {ApplicationModule} from './application_module';
export {wtfCreateScope, wtfLeave, wtfStartTimeRange, wtfEndTimeRange, WtfScopeFn} from './profile/profile';
export {Type} from './type';
export {Type} from './interfaces/type';
export {EventEmitter} from './event_emitter';
export {ErrorHandler} from './error_handler';
export * from './core_private_export';
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 @@ -151,6 +151,7 @@ export {
sanitizeStyle as ɵsanitizeStyle,
sanitizeUrl as ɵsanitizeUrl,
sanitizeResourceUrl as ɵsanitizeResourceUrl,
sanitizeUrlOrResourceUrl as ɵsanitizeUrlOrResourceUrl,
} from './sanitization/sanitization';

export {
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/di/defs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Type} from '../interfaces/type';
import {NG_INJECTABLE_DEF, NG_INJECTOR_DEF} from '../render3/fields';
import {Type} from '../type';

import {ClassProvider, ClassSansProvider, ConstructorProvider, ConstructorSansProvider, ExistingProvider, ExistingSansProvider, FactoryProvider, FactorySansProvider, StaticClassProvider, StaticClassSansProvider, ValueProvider, ValueSansProvider} from './provider';


/**
* Information about how a type or `InjectionToken` interfaces with the DI system.
*
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/di/forward_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Type} from '../type';
import {Type} from '../interfaces/type';
import {stringify} from '../util';
import {getClosureSafeProperty} from '../util/property';

Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/di/injectable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Type} from '../interfaces/type';
import {compileInjectable as render3CompileInjectable} from '../render3/jit/injectable';
import {Type} from '../type';
import {TypeDecorator, makeDecorator} from '../util/decorators';

import {InjectableDef, InjectableType, defineInjectable, getInjectableDef} from './defs';
import {ClassSansProvider, ConstructorSansProvider, ExistingSansProvider, FactorySansProvider, StaticClassSansProvider, ValueSansProvider} from './provider';
import {convertInjectableProviderToFactory} from './util';



/**
* Injectable providers used in `@Injectable` decorator.
*
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/di/injection_token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Type} from '../type';
import {Type} from '../interfaces/type';

import {defineInjectable} from './defs';

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/di/injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Type} from '../interfaces/type';
import {injectInjector} from '../render3/di';
import {Type} from '../type';
import {stringify} from '../util';
import {noop} from '../util/noop';
import {getClosureSafeProperty} from '../util/property';
Expand Down

0 comments on commit b2db7a2

Please sign in to comment.