diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts index 33a24fb424..f3a94c866f 100644 --- a/packages/compiler/src/render3/r3_template_transform.ts +++ b/packages/compiler/src/render3/r3_template_transform.ts @@ -240,7 +240,11 @@ class HtmlAstToIvyAst implements html.Visitor { literal.push(new t.TextAttribute( prop.name, prop.expression.source || '', prop.sourceSpan, undefined, i18n)); } else { - const bep = this.bindingParser.createBoundElementProperty(elementName, prop); + // we skip validation here, since we do this check at runtime due to the fact that we need + // to make sure a given prop is not an input of some Directive (thus should not be a subject + // of this check) and Directive matching happens at runtime + const bep = this.bindingParser.createBoundElementProperty( + elementName, prop, /* skipValidation */ true); bound.push(t.BoundAttribute.fromBoundElementProperty(bep, i18n)); } }); diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index bef79f124b..50af4423ea 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -238,8 +238,9 @@ export class BindingParser { } } - createBoundElementProperty(elementSelector: string, boundProp: ParsedProperty): - BoundElementProperty { + createBoundElementProperty( + elementSelector: string, boundProp: ParsedProperty, + skipValidation: boolean = false): BoundElementProperty { if (boundProp.isAnimation) { return new BoundElementProperty( boundProp.name, BindingType.Animation, SecurityContext.NONE, boundProp.expression, null, @@ -252,11 +253,13 @@ export class BindingParser { const parts = boundProp.name.split(PROPERTY_PARTS_SEPARATOR); let securityContexts: SecurityContext[] = undefined !; - // Check check for special cases (prefix style, attr, class) + // Check for special cases (prefix style, attr, class) if (parts.length > 1) { if (parts[0] == ATTRIBUTE_PREFIX) { boundPropertyName = parts[1]; - this._validatePropertyOrAttributeName(boundPropertyName, boundProp.sourceSpan, true); + if (!skipValidation) { + this._validatePropertyOrAttributeName(boundPropertyName, boundProp.sourceSpan, true); + } securityContexts = calcPossibleSecurityContexts( this._schemaRegistry, elementSelector, boundPropertyName, true); @@ -286,7 +289,9 @@ export class BindingParser { securityContexts = calcPossibleSecurityContexts( this._schemaRegistry, elementSelector, boundPropertyName, false); bindingType = BindingType.Property; - this._validatePropertyOrAttributeName(boundPropertyName, boundProp.sourceSpan, false); + if (!skipValidation) { + this._validatePropertyOrAttributeName(boundPropertyName, boundProp.sourceSpan, false); + } } return new BoundElementProperty( diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index d37441a510..ca4929b23e 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -10,6 +10,7 @@ import {InjectFlags, InjectionToken, Injector} from '../di'; import {resolveForwardRef} from '../di/forward_ref'; import {Type} from '../interface/type'; import {QueryList} from '../linker'; +import {validateAttribute, validateProperty} from '../sanitization/sanitization'; import {Sanitizer} from '../sanitization/security'; import {StyleSanitizeFn} from '../sanitization/style_sanitizer'; import {assertDataInRange, assertDefined, assertEqual, assertLessThan, assertNotEqual} from '../util/assert'; @@ -973,6 +974,7 @@ export function elementEnd(): void { export function elementAttribute( index: number, name: string, value: any, sanitizer?: SanitizerFn | null): void { if (value !== NO_CHANGE) { + ngDevMode && validateAttribute(name); const lView = getLView(); const renderer = lView[RENDERER]; const element = getNativeByIndex(index, lView); @@ -1064,11 +1066,14 @@ function elementPropertyInternal( } } } else if (tNode.type === TNodeType.Element) { + if (ngDevMode) { + validateProperty(propName); + ngDevMode.rendererSetProperty++; + } 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, tNode.tagName || '', propName) as any) : value; - ngDevMode && ngDevMode.rendererSetProperty++; if (isProceduralRenderer(renderer)) { renderer.setProperty(element as RElement, propName, value); } else if (!isAnimationProp(propName)) { diff --git a/packages/core/src/sanitization/sanitization.ts b/packages/core/src/sanitization/sanitization.ts index 9b92fb35e3..c8736e7257 100644 --- a/packages/core/src/sanitization/sanitization.ts +++ b/packages/core/src/sanitization/sanitization.ts @@ -179,6 +179,24 @@ export const defaultStyleSanitizer = (function(prop: string, value?: string): st return sanitizeStyle(value); } as StyleSanitizeFn); +export function validateProperty(name: string) { + if (name.toLowerCase().startsWith('on')) { + const msg = `Binding to event property '${name}' is disallowed for security reasons, ` + + `please use (${name.slice(2)})=...` + + `\nIf '${name}' is a directive input, make sure the directive is imported by the` + + ` current module.`; + throw new Error(msg); + } +} + +export function validateAttribute(name: string) { + if (name.toLowerCase().startsWith('on')) { + const msg = `Binding to event attribute '${name}' is disallowed for security reasons, ` + + `please use (${name.slice(2)})=...`; + throw new Error(msg); + } +} + function getSanitizer(): Sanitizer|null { const lView = getLView(); return lView && lView[SANITIZER]; diff --git a/packages/core/test/linker/security_integration_spec.ts b/packages/core/test/linker/security_integration_spec.ts index 8baa99918c..fb518c98b6 100644 --- a/packages/core/test/linker/security_integration_spec.ts +++ b/packages/core/test/linker/security_integration_spec.ts @@ -10,7 +10,7 @@ import {Component, Directive, HostBinding, Input, NO_ERRORS_SCHEMA, ɵivyEnabled import {ComponentFixture, TestBed, getTestBed} from '@angular/core/testing'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {DomSanitizer} from '@angular/platform-browser/src/security/dom_sanitization_service'; -import {fixmeIvy} from '@angular/private/testing'; +import {fixmeIvy, modifiedInIvy, onlyInIvy} from '@angular/private/testing'; { if (ivyEnabled) { @@ -52,47 +52,79 @@ function declareTests(config?: {useJit: boolean}) { afterEach(() => { getDOM().log = originalLog; }); describe('events', () => { - it('should disallow binding to attr.on*', () => { - const template = `
`; - TestBed.overrideComponent(SecuredComponent, {set: {template}}); + modifiedInIvy('on-prefixed attributes validation happens at runtime in Ivy') + .it('should disallow binding to attr.on*', () => { + const template = `
`; + TestBed.overrideComponent(SecuredComponent, {set: {template}}); - expect(() => TestBed.createComponent(SecuredComponent)) - .toThrowError( - /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../); - }); + expect(() => TestBed.createComponent(SecuredComponent)) + .toThrowError( + /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../); + }); - it('should disallow binding to on* with NO_ERRORS_SCHEMA', () => { - const template = `
`; - TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({ - schemas: [NO_ERRORS_SCHEMA] - }); + // this test is similar to the previous one, but since on-prefixed attributes validation now + // happens at runtime, we need to invoke change detection to trigger elementProperty call + onlyInIvy('on-prefixed attributes validation happens at runtime in Ivy') + .it('should disallow binding to attr.on*', () => { + const template = `
`; + TestBed.overrideComponent(SecuredComponent, {set: {template}}); - expect(() => TestBed.createComponent(SecuredComponent)) - .toThrowError( - /Binding to event property 'onclick' is disallowed for security reasons, please use \(click\)=.../); - }); + expect(() => { + const cmp = TestBed.createComponent(SecuredComponent); + cmp.detectChanges(); + }) + .toThrowError( + /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../); + }); - fixmeIvy( - 'FW-786: Element properties and directive inputs are not distinguished for sanitisation purposes') - .it('should disallow binding to on* unless it is consumed by a directive', () => { - const template = `
`; + modifiedInIvy('on-prefixed attributes validation happens at runtime in Ivy') + .it('should disallow binding to on* with NO_ERRORS_SCHEMA', () => { + const template = `
`; TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({ schemas: [NO_ERRORS_SCHEMA] }); - // should not throw for inputs starting with "on" - let cmp: ComponentFixture = undefined !; - expect(() => cmp = TestBed.createComponent(SecuredComponent)).not.toThrow(); - - // must bind to the directive not to the property of the div - const value = cmp.componentInstance.ctxProp = {}; - cmp.detectChanges(); - const div = cmp.debugElement.children[0]; - expect(div.injector.get(OnPrefixDir).onclick).toBe(value); - expect(getDOM().getProperty(div.nativeElement, 'onclick')).not.toBe(value); - expect(getDOM().hasAttribute(div.nativeElement, 'onclick')).toEqual(false); + expect(() => TestBed.createComponent(SecuredComponent)) + .toThrowError( + /Binding to event property 'onclick' is disallowed for security reasons, please use \(click\)=.../); }); + // this test is similar to the previous one, but since on-prefixed attributes validation now + // happens at runtime, we need to invoke change detection to trigger elementProperty call + onlyInIvy('on-prefixed attributes validation happens at runtime in Ivy') + .it('should disallow binding to on* with NO_ERRORS_SCHEMA', () => { + const template = `
`; + TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({ + schemas: [NO_ERRORS_SCHEMA] + }); + + expect(() => { + const cmp = TestBed.createComponent(SecuredComponent); + cmp.detectChanges(); + }) + .toThrowError( + /Binding to event property 'onclick' is disallowed for security reasons, please use \(click\)=.../); + }); + + it('should disallow binding to on* unless it is consumed by a directive', () => { + const template = `
`; + TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({ + schemas: [NO_ERRORS_SCHEMA] + }); + + // should not throw for inputs starting with "on" + let cmp: ComponentFixture = undefined !; + expect(() => cmp = TestBed.createComponent(SecuredComponent)).not.toThrow(); + + // must bind to the directive not to the property of the div + const value = cmp.componentInstance.ctxProp = {}; + cmp.detectChanges(); + const div = cmp.debugElement.children[0]; + expect(div.injector.get(OnPrefixDir).onclick).toBe(value); + expect(getDOM().getProperty(div.nativeElement, 'onclick')).not.toBe(value); + expect(getDOM().hasAttribute(div.nativeElement, 'onclick')).toEqual(false); + }); + }); describe('safe HTML values', function() {