fix(compiler): move detection of unsafe properties for binding to ElementSchemaRegistry (#11378)

modules/@angular/compiler/src/schema/dom_element_schema_registry.ts

@@ -344,4 +344,26 @@ export class DomElementSchemaRegistry extends ElementSchemaRegistry {
   getMappedPropName(propName: string): string { return _ATTR_TO_PROP[propName] || propName; }
 
   getDefaultComponentElementName(): string { return 'ng-component'; }
+
+  validateProperty(name: string): {error: boolean, msg?: 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.`;
+      return {error: true, msg: msg};
+    } else {
+      return {error: false};
+    }
+  }
+
+  validateAttribute(name: string): {error: boolean, msg?: string} {
+    if (name.toLowerCase().startsWith('on')) {
+      const msg = `Binding to event attribute '${name}' is disallowed for security reasons, ` +
+          `please use (${name.slice(2)})=...`;
+      return {error: true, msg: msg};
+    } else {
+      return {error: false};
+    }
+  }
 }

modules/@angular/compiler/src/schema/element_schema_registry.ts

@@ -14,4 +14,6 @@ export abstract class ElementSchemaRegistry {
   abstract securityContext(tagName: string, propName: string): any;
   abstract getMappedPropName(propName: string): string;
   abstract getDefaultComponentElementName(): string;
+  abstract validateProperty(name: string): {error: boolean, msg?: string};
+  abstract validateAttribute(name: string): {error: boolean, msg?: string};
 }

modules/@angular/compiler/src/template_parser/template_parser.ts

@@ -927,7 +927,7 @@ class TemplateParseVisitor implements html.Visitor {
         boundPropertyName = this._schemaRegistry.getMappedPropName(partValue);
         securityContext = this._schemaRegistry.securityContext(elementName, boundPropertyName);
         bindingType = PropertyBindingType.Property;
-        this._assertNoEventBinding(boundPropertyName, sourceSpan, false);
+        this._validatePropertyOrAttributeName(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}'.`;
@@ -942,7 +942,7 @@ class TemplateParseVisitor implements html.Visitor {
     } else {
       if (parts[0] == ATTRIBUTE_PREFIX) {
         boundPropertyName = parts[1];
-        this._assertNoEventBinding(boundPropertyName, sourceSpan, true);
+        this._validatePropertyOrAttributeName(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);
@@ -975,23 +975,19 @@ class TemplateParseVisitor implements html.Visitor {
         boundPropertyName, bindingType, securityContext, ast, unit, sourceSpan);
   }
 
+
   /**
    * @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):
+  private _validatePropertyOrAttributeName(
-      void {
+      propName: string, sourceSpan: ParseSourceSpan, isAttr: boolean): void {
-    if (propName.toLowerCase().startsWith('on')) {
+    const report = isAttr ? this._schemaRegistry.validateAttribute(propName) :
-      let msg = `Binding to event attribute '${propName}' is disallowed for security reasons, ` +
+                            this._schemaRegistry.validateProperty(propName);
-          `please use (${propName.slice(2)})=...`;
+    if (report.error) {
-      if (!isAttr) {
+      this._reportError(report.msg, sourceSpan, ParseErrorLevel.FATAL);
-        msg +=
-            `\nIf '${propName}' is a directive input, make sure the directive is imported by the` +
-            ` current module.`;
-      }
-      this._reportError(msg, sourceSpan, ParseErrorLevel.FATAL);
     }
   }
 

modules/@angular/compiler/test/schema/dom_element_schema_registry_spec.ts

@@ -105,6 +105,47 @@ export function main() {
       expect(registry.getMappedPropName('exotic-unknown')).toEqual('exotic-unknown');
     });
 
+    it('should return an error message when asserting event properties', () => {
+      let report = registry.validateProperty('onClick');
+      expect(report.error).toBeTruthy();
+      expect(report.msg)
+          .toEqual(
+              `Binding to event property 'onClick' is disallowed for security reasons, please use (Click)=...
+If 'onClick' is a directive input, make sure the directive is imported by the current module.`);
+
+      report = registry.validateProperty('onAnything');
+      expect(report.error).toBeTruthy();
+      expect(report.msg)
+          .toEqual(
+              `Binding to event property 'onAnything' is disallowed for security reasons, please use (Anything)=...
+If 'onAnything' is a directive input, make sure the directive is imported by the current module.`);
+    });
+
+    it('should return an error message when asserting event attributes', () => {
+      let report = registry.validateAttribute('onClick');
+      expect(report.error).toBeTruthy();
+      expect(report.msg)
+          .toEqual(
+              `Binding to event attribute 'onClick' is disallowed for security reasons, please use (Click)=...`);
+
+      report = registry.validateAttribute('onAnything');
+      expect(report.error).toBeTruthy();
+      expect(report.msg)
+          .toEqual(
+              `Binding to event attribute 'onAnything' is disallowed for security reasons, please use (Anything)=...`);
+    });
+
+    it('should not return an error message when asserting non-event properties or attributes',
+       () => {
+         let report = registry.validateProperty('title');
+         expect(report.error).toBeFalsy();
+         expect(report.msg).not.toBeDefined();
+
+         report = registry.validateProperty('exotic-unknown');
+         expect(report.error).toBeFalsy();
+         expect(report.msg).not.toBeDefined();
+       });
+
     it('should return security contexts for elements', () => {
       expect(registry.securityContext('iframe', 'srcdoc')).toBe(SecurityContext.HTML);
       expect(registry.securityContext('p', 'innerHTML')).toBe(SecurityContext.HTML);

modules/@angular/compiler/test/template_parser/template_parser_spec.ts

@@ -26,7 +26,8 @@ const someModuleUrl = 'package:someModule';
 const MOCK_SCHEMA_REGISTRY = [{
   provide: ElementSchemaRegistry,
   useValue: new MockSchemaRegistry(
-      {'invalidProp': false}, {'mappedAttr': 'mappedProp'}, {'unknown': false, 'un-known': false}),
+      {'invalidProp': false}, {'mappedAttr': 'mappedProp'}, {'unknown': false, 'un-known': false},
+      ['onEvent'], ['onEvent']),
 }];
 
 export function main() {
@@ -141,7 +142,8 @@ export function main() {
     });
   });
 
-  describe('TemplateParser', () => {
+  describe(
+      'TemplateParser', () => {
         beforeEach(() => {
           TestBed.configureCompiler({providers: [TEST_COMPILER_PROVIDERS, MOCK_SCHEMA_REGISTRY]});
         });
@@ -276,12 +278,26 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
 2. If 'unknown' is a Web Component then add "CUSTOM_ELEMENTS_SCHEMA" to the '@NgModule.schemas' of this component to suppress this message. ("[ERROR ->]<unknown></unknown>"): TestComp@0:0`);
               });
 
-          it('should throw error when binding to an unknown custom element w/o bindings', () => {
+              it('should throw error when binding to an unknown custom element w/o bindings',
-            expect(() => parse('<un-known></un-known>', [])).toThrowError(`Template parse errors:
+                 () => {
+                   expect(() => parse('<un-known></un-known>', []))
+                       .toThrowError(`Template parse errors:
 'un-known' is not a known element:
 1. If 'un-known' is an Angular component, then verify that it is part of this module.
 2. If 'un-known' is a Web Component then add "CUSTOM_ELEMENTS_SCHEMA" to the '@NgModule.schemas' of this component to suppress this message. ("[ERROR ->]<un-known></un-known>"): TestComp@0:0`);
                  });
+
+              it('should throw error when binding to an invalid property', () => {
+                expect(() => parse('<my-component [onEvent]="bar"></my-component>', []))
+                    .toThrowError(`Template parse errors:
+Binding to property 'onEvent' is disallowed for security reasons ("<my-component [ERROR ->][onEvent]="bar"></my-component>"): TestComp@0:14`);
+              });
+
+              it('should throw error when binding to an invalid attribute', () => {
+                expect(() => parse('<my-component [attr.onEvent]="bar"></my-component>', []))
+                    .toThrowError(`Template parse errors:
+Binding to attribute 'onEvent' is disallowed for security reasons ("<my-component [ERROR ->][attr.onEvent]="bar"></my-component>"): TestComp@0:14`);
+              });
             });
 
             it('should parse bound properties via [...] and not report them as attributes', () => {
@@ -298,16 +314,20 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
               ]);
             });
 
-        it('should parse bound properties via {{...}} and not report them as attributes', () => {
+            it('should parse bound properties via {{...}} and not report them as attributes',
+               () => {
                  expect(humanizeTplAst(parse('<div prop="{{v}}">', []))).toEqual([
                    [ElementAst, 'div'],
-            [BoundElementPropertyAst, PropertyBindingType.Property, 'prop', '{{ v }}', null]
+                   [
+                     BoundElementPropertyAst, PropertyBindingType.Property, 'prop', '{{ v }}', null
+                   ]
                  ]);
                });
 
             it('should parse bound properties via bind-animate- and not report them as attributes',
                () => {
-             expect(humanizeTplAst(parse('<div bind-animate-someAnimation="value2">', [], [], [])))
+                 expect(
+                     humanizeTplAst(parse('<div bind-animate-someAnimation="value2">', [], [], [])))
                      .toEqual([
                        [ElementAst, 'div'],
                        [
@@ -473,8 +493,8 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
                  });
                  expect(humanizeTplAst(parse('<div a c b a b>', [dirA, dirB, dirC]))).toEqual([
                    [ElementAst, 'div'], [AttrAst, 'a', ''], [AttrAst, 'c', ''], [AttrAst, 'b', ''],
-               [AttrAst, 'a', ''], [AttrAst, 'b', ''], [DirectiveAst, dirA], [DirectiveAst, dirB],
+                   [AttrAst, 'a', ''], [AttrAst, 'b', ''], [DirectiveAst, dirA],
-               [DirectiveAst, dirC]
+                   [DirectiveAst, dirB], [DirectiveAst, dirC]
                  ]);
                });
 
@@ -554,7 +574,8 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
                 inputs: ['b:a']
               });
               expect(humanizeTplAst(parse('<div [a]="expr"></div>', [dirA]))).toEqual([
-            [ElementAst, 'div'], [DirectiveAst, dirA], [BoundDirectivePropertyAst, 'b', 'expr']
+                [ElementAst, 'div'], [DirectiveAst, dirA],
+                [BoundDirectivePropertyAst, 'b', 'expr']
               ]);
             });
 
@@ -632,8 +653,12 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
                 isHost = true;
                 value = value.substring(5);
               }
-          return new CompileDiDependencyMetadata(
+              return new CompileDiDependencyMetadata({
-              {token: createToken(value), isOptional: isOptional, isSelf: isSelf, isHost: isHost});
+                token: createToken(value),
+                isOptional: isOptional,
+                isSelf: isSelf,
+                isHost: isHost
+              });
             }
 
             function createProvider(
@@ -649,7 +674,8 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
             }
 
             function createDir(
-            selector: string, {providers = null, viewProviders = null, deps = [], queries = []}: {
+                selector: string,
+                {providers = null, viewProviders = null, deps = [], queries = []}: {
                   providers?: CompileProviderMetadata[],
                   viewProviders?: CompileProviderMetadata[],
                   deps?: string[],
@@ -802,7 +828,8 @@ Can't bind to 'invalidProp' since it isn't a known property of 'my-component'.
             it('should mark directives and dependencies of directives as eager', () => {
               var provider0 = createProvider('service0');
               var provider1 = createProvider('service1');
-          var dirA = createDir('[dirA]', {providers: [provider0, provider1], deps: ['service0']});
+              var dirA =
+                  createDir('[dirA]', {providers: [provider0, provider1], deps: ['service0']});
               var elAst: ElementAst = <ElementAst>parse('<div dirA>', [dirA])[0];
               expect(elAst.providers.length).toBe(3);
               expect(elAst.providers[0].providers).toEqual([provider0]);
@@ -1072,8 +1099,9 @@ Reference "#a" is defined several times ("<div #a></div><div [ERROR ->]#a></div>
                       {moduleUrl: someModuleUrl, name: 'DirB', reference: {} as Type<any>})
                 });
                 expect(humanizeTplAst(parse('<div template="a b" b>', [dirA, dirB]))).toEqual([
-              [EmbeddedTemplateAst], [DirectiveAst, dirA], [BoundDirectivePropertyAst, 'a', 'b'],
+                  [EmbeddedTemplateAst], [DirectiveAst, dirA],
-              [ElementAst, 'div'], [AttrAst, 'b', ''], [DirectiveAst, dirB]
+                  [BoundDirectivePropertyAst, 'a', 'b'], [ElementAst, 'div'], [AttrAst, 'b', ''],
+                  [DirectiveAst, dirB]
                 ]);
               });
 
@@ -1148,8 +1176,9 @@ Reference "#a" is defined several times ("<div #a></div><div [ERROR ->]#a></div>
 
           describe('project text nodes', () => {
             it('should project text nodes with wildcard selector', () => {
-          expect(humanizeContentProjection(parse('<div>hello</div>', [createComp('div', ['*'])])))
+              expect(humanizeContentProjection(parse('<div>hello</div>', [
-              .toEqual([['div', null], ['#text(hello)', 0]]);
+                createComp('div', ['*'])
+              ]))).toEqual([['div', null], ['#text(hello)', 0]]);
             });
           });
 
@@ -1222,9 +1251,10 @@ Reference "#a" is defined several times ("<div #a></div><div [ERROR ->]#a></div>
           });
 
           it('should project children of components with ngNonBindable', () => {
-        expect(humanizeContentProjection(parse('<div ngNonBindable>{{hello}}<span></span></div>', [
+            expect(
-          createComp('div', ['*'])
+                humanizeContentProjection(parse(
-        ]))).toEqual([['div', null], ['#text({{hello}})', 0], ['span', 0]]);
+                    '<div ngNonBindable>{{hello}}<span></span></div>', [createComp('div', ['*'])])))
+                .toEqual([['div', null], ['#text({{hello}})', 0], ['span', 0]]);
           });
 
           it('should match the element when there is an inline template', () => {
@@ -1302,7 +1332,8 @@ Can't have multiple template bindings on one element. Use only one attribute nam
           });
 
           it('should report invalid property names', () => {
-        expect(() => parse('<div [invalidProp]></div>', [])).toThrowError(`Template parse errors:
+            expect(() => parse('<div [invalidProp]></div>', []))
+                .toThrowError(`Template parse errors:
 Can't bind to 'invalidProp' since it isn't a known property of 'div'. ("<div [ERROR ->][invalidProp]></div>"): TestComp@0:5`);
           });
 
@@ -1430,7 +1461,8 @@ Property binding a not used by any directive on an embedded template. Make sure
           });
 
           it('should keep nested children of elements with ngNonBindable', () => {
-        expect(humanizeTplAst(parse('<div ngNonBindable><span>{{b}}</span></div>', []))).toEqual([
+            expect(humanizeTplAst(parse('<div ngNonBindable><span>{{b}}</span></div>', [])))
+                .toEqual([
                   [ElementAst, 'div'], [AttrAst, 'ngNonBindable', ''], [ElementAst, 'span'],
                   [TextAst, '{{b}}']
                 ]);
@@ -1454,10 +1486,11 @@ Property binding a not used by any directive on an embedded template. Make sure
 
           it('should convert <ng-content> elements into regular elements inside of elements with ngNonBindable',
              () => {
-           expect(humanizeTplAst(parse('<div ngNonBindable><ng-content></ng-content>a</div>', [])))
+               expect(
+                   humanizeTplAst(parse('<div ngNonBindable><ng-content></ng-content>a</div>', [])))
                    .toEqual([
-                 [ElementAst, 'div'], [AttrAst, 'ngNonBindable', ''], [ElementAst, 'ng-content'],
+                     [ElementAst, 'div'], [AttrAst, 'ngNonBindable', ''],
-                 [TextAst, 'a']
+                     [ElementAst, 'ng-content'], [TextAst, 'a']
                    ]);
              });
 
@@ -1490,8 +1523,10 @@ Property binding a not used by any directive on an embedded template. Make sure
           });
 
           it('should support variables', () => {
-        expect(humanizeTplAstSourceSpans(parse('<template let-a="b"></template>', []))).toEqual([
+            expect(humanizeTplAstSourceSpans(parse('<template let-a="b"></template>', [])))
-          [EmbeddedTemplateAst, '<template let-a="b">'], [VariableAst, 'a', 'b', 'let-a="b"']
+                .toEqual([
+                  [EmbeddedTemplateAst, '<template let-a="b">'],
+                  [VariableAst, 'a', 'b', 'let-a="b"']
                 ]);
           });
 
@@ -1536,8 +1571,8 @@ Property binding a not used by any directive on an embedded template. Make sure
               template: new CompileTemplateMetadata({ngContentSelectors: []})
             });
             expect(humanizeTplAstSourceSpans(parse('<div a>', [dirA, comp]))).toEqual([
-          [ElementAst, 'div', '<div a>'], [AttrAst, 'a', '', 'a'], [DirectiveAst, dirA, '<div a>'],
+              [ElementAst, 'div', '<div a>'], [AttrAst, 'a', '', 'a'],
-          [DirectiveAst, comp, '<div a>']
+              [DirectiveAst, dirA, '<div a>'], [DirectiveAst, comp, '<div a>']
             ]);
           });
 
@@ -1573,7 +1608,8 @@ Property binding a not used by any directive on an embedded template. Make sure
               inputs: ['aProp']
             });
             expect(humanizeTplAstSourceSpans(parse('<div [aProp]="foo"></div>', [dirA]))).toEqual([
-          [ElementAst, 'div', '<div [aProp]="foo">'], [DirectiveAst, dirA, '<div [aProp]="foo">'],
+              [ElementAst, 'div', '<div [aProp]="foo">'],
+              [DirectiveAst, dirA, '<div [aProp]="foo">'],
               [BoundDirectivePropertyAst, 'aProp', 'foo', '[aProp]="foo"']
             ]);
           });
@@ -1605,8 +1641,8 @@ The pipe 'test' could not be found ("[ERROR ->]{{a | test}}"): TestComp@0:0`);
                 '<template ngPluralCase="many">big</template>' +
                 '</ng-container>';
 
-        expect(humanizeTplAst(parse(shortForm, []))).toEqual(humanizeTplAst(parse(expandedForm, [
+            expect(humanizeTplAst(parse(shortForm, [
-        ])));
+            ]))).toEqual(humanizeTplAst(parse(expandedForm, [])));
           });
 
           it('should expand other messages', () => {
@@ -1616,8 +1652,8 @@ The pipe 'test' could not be found ("[ERROR ->]{{a | test}}"): TestComp@0:0`);
                 '<template ngSwitchCase="other">bar</template>' +
                 '</ng-container>';
 
-        expect(humanizeTplAst(parse(shortForm, []))).toEqual(humanizeTplAst(parse(expandedForm, [
+            expect(humanizeTplAst(parse(shortForm, [
-        ])));
+            ]))).toEqual(humanizeTplAst(parse(expandedForm, [])));
           });
 
           it('should be possible to escape ICU messages', () => {

modules/@angular/compiler/testing/schema_registry_mock.ts

@@ -13,7 +13,8 @@ export class MockSchemaRegistry implements ElementSchemaRegistry {
   constructor(
       public existingProperties: {[key: string]: boolean},
       public attrPropMapping: {[key: string]: string},
-      public existingElements: {[key: string]: boolean}) {}
+      public existingElements: {[key: string]: boolean}, public invalidProperties: Array<string>,
+      public invalidAttributes: Array<string>) {}
 
   hasProperty(tagName: string, property: string, schemas: SchemaMetadata[]): boolean {
     const value = this.existingProperties[property];
@@ -32,4 +33,23 @@ export class MockSchemaRegistry implements ElementSchemaRegistry {
   getMappedPropName(attrName: string): string { return this.attrPropMapping[attrName] || attrName; }
 
   getDefaultComponentElementName(): string { return 'ng-component'; }
+
+  validateProperty(name: string): {error: boolean, msg?: string} {
+    if (this.invalidProperties.indexOf(name) > -1) {
+      return {error: true, msg: `Binding to property '${name}' is disallowed for security reasons`};
+    } else {
+      return {error: false};
+    }
+  }
+
+  validateAttribute(name: string): {error: boolean, msg?: string} {
+    if (this.invalidAttributes.indexOf(name) > -1) {
+      return {
+        error: true,
+        msg: `Binding to attribute '${name}' is disallowed for security reasons`
+      };
+    } else {
+      return {error: false};
+    }
+  }
 }

modules/@angular/compiler/testing/test_bindings.ts

@@ -19,7 +19,7 @@ export function createUrlResolverWithoutPackagePrefix(): UrlResolver {
 // internal test packages.
 // TODO: get rid of it or move to a separate @angular/internal_testing package
 export var TEST_COMPILER_PROVIDERS: Provider[] = [
-  {provide: ElementSchemaRegistry, useValue: new MockSchemaRegistry({}, {}, {})},
+  {provide: ElementSchemaRegistry, useValue: new MockSchemaRegistry({}, {}, {}, [], [])},
   {provide: ResourceLoader, useClass: MockResourceLoader},
   {provide: UrlResolver, useFactory: createUrlResolverWithoutPackagePrefix}
 ];

modules/@angular/core/test/linker/security_integration_spec.ts

@@ -66,7 +66,7 @@ function declareTests({useJit}: {useJit: boolean}) {
 
         expect(() => TestBed.createComponent(SecuredComponent))
             .toThrowError(
-                /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../);
+                /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', () => {