diff --git a/modules/@angular/compiler/src/template_parser/template_parser.ts b/modules/@angular/compiler/src/template_parser/template_parser.ts index 01fb272b10..776ba51ae6 100644 --- a/modules/@angular/compiler/src/template_parser/template_parser.ts +++ b/modules/@angular/compiler/src/template_parser/template_parser.ts @@ -854,7 +854,7 @@ class TemplateParseVisitor implements html.Visitor { boundPropertyName = this._schemaRegistry.getMappedPropName(partValue); securityContext = this._schemaRegistry.securityContext(elementName, boundPropertyName); bindingType = PropertyBindingType.Property; - this._assertNoEventBinding(boundPropertyName, sourceSpan); + this._assertNoEventBinding(boundPropertyName, sourceSpan, false); if (!this._schemaRegistry.hasProperty(elementName, boundPropertyName, this._schemas)) { let errorMsg = `Can't bind to '${boundPropertyName}' since it isn't a known property of '${elementName}'.`; @@ -869,7 +869,7 @@ class TemplateParseVisitor implements html.Visitor { } else { if (parts[0] == ATTRIBUTE_PREFIX) { boundPropertyName = parts[1]; - this._assertNoEventBinding(boundPropertyName, sourceSpan); + this._assertNoEventBinding(boundPropertyName, sourceSpan, true); // NB: For security purposes, use the mapped property name, not the attribute name. const mapPropName = this._schemaRegistry.getMappedPropName(boundPropertyName); securityContext = this._schemaRegistry.securityContext(elementName, mapPropName); @@ -902,12 +902,23 @@ class TemplateParseVisitor implements html.Visitor { boundPropertyName, bindingType, securityContext, ast, unit, sourceSpan); } - private _assertNoEventBinding(propName: string, sourceSpan: ParseSourceSpan): void { + /** + * @param propName the name of the property / attribute + * @param sourceSpan + * @param isAttr true when binding to an attribute + * @private + */ + private _assertNoEventBinding(propName: string, sourceSpan: ParseSourceSpan, isAttr: boolean): + void { if (propName.toLowerCase().startsWith('on')) { - this._reportError( - `Binding to event attribute '${propName}' is disallowed ` + - `for security reasons, please use (${propName.slice(2)})=...`, - sourceSpan, ParseErrorLevel.FATAL); + let msg = `Binding to event attribute '${propName}' is disallowed for security reasons, ` + + `please use (${propName.slice(2)})=...`; + if (!isAttr) { + msg += + `\nIf '${propName}' is a directive input, make sure the directive is imported by the` + + ` current module.`; + } + this._reportError(msg, sourceSpan, ParseErrorLevel.FATAL); } } diff --git a/modules/@angular/core/test/linker/security_integration_spec.ts b/modules/@angular/core/test/linker/security_integration_spec.ts index b0f302d3fa..25a2982378 100644 --- a/modules/@angular/core/test/linker/security_integration_spec.ts +++ b/modules/@angular/core/test/linker/security_integration_spec.ts @@ -6,9 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {CUSTOM_ELEMENTS_SCHEMA, NO_ERRORS_SCHEMA} from '@angular/core'; -import {Component} from '@angular/core/src/metadata'; -import {TestBed, getTestBed} from '@angular/core/testing'; +import {Component, Directive, Input, NO_ERRORS_SCHEMA} from '@angular/core'; +import {ComponentFixture, TestBed, getTestBed} from '@angular/core/testing'; import {afterEach, beforeEach, beforeEachProviders, ddescribe, describe, expect, iit, inject, it} from '@angular/core/testing/testing_internal'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {DomSanitizer} from '@angular/platform-browser/src/security/dom_sanitization_service'; @@ -24,12 +23,22 @@ class SecuredComponent { ctxProp: any = 'some value'; } +@Directive({selector: '[onPrefixedProp]'}) +class OnPrefixDir { + @Input() onPrefixedProp: any; + @Input() onclick: any; +} + function declareTests({useJit}: {useJit: boolean}) { describe('security integration tests', function() { beforeEach(() => { - TestBed.configureCompiler({useJit: useJit}); - TestBed.configureTestingModule({declarations: [SecuredComponent]}); + TestBed.configureCompiler({useJit: useJit}).configureTestingModule({ + declarations: [ + SecuredComponent, + OnPrefixDir, + ] + }); }); let originalLog: (msg: any) => any; @@ -43,15 +52,10 @@ function declareTests({useJit}: {useJit: boolean}) { it('should disallow binding to attr.on*', () => { const template = `
`; TestBed.overrideComponent(SecuredComponent, {set: {template}}); - try { - TestBed.createComponent(SecuredComponent); - throw 'Should throw'; - } catch (e) { - expect(e.message).toContain( - `Template parse errors:\n` + - `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', () => { @@ -59,17 +63,31 @@ function declareTests({useJit}: {useJit: boolean}) { TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({ schemas: [NO_ERRORS_SCHEMA] }); - ; - try { - TestBed.createComponent(SecuredComponent); - throw 'Should throw'; - } catch (e) { - expect(e.message).toContain( - `Template parse errors:\n` + - `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* 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; + 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() { @@ -157,12 +175,8 @@ function declareTests({useJit}: {useJit: boolean}) { const template = `Text`; TestBed.overrideComponent(SecuredComponent, {set: {template}}); - try { - TestBed.createComponent(SecuredComponent); - throw 'Should throw'; - } catch (e) { - expect(e.message).toContain(`Can't bind to 'xlink:href'`); - } + expect(() => TestBed.createComponent(SecuredComponent)) + .toThrowError(/Can't bind to 'xlink:href'/); }); it('should escape unsafe HTML values', () => {