refactor(TemplateParser): clearer error message for on* binding (#11802)

fixes #11756

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(
+      let msg = `Binding to event attribute '${propName}' is disallowed for security reasons, ` +
-          `Binding to event attribute '${propName}' is disallowed ` +
+          `please use (${propName.slice(2)})=...`;
-              `for security reasons, please use (${propName.slice(2)})=...`,
+      if (!isAttr) {
-          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/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, Directive, Input, NO_ERRORS_SCHEMA} from '@angular/core';
-import {Component} from '@angular/core/src/metadata';
+import {ComponentFixture, TestBed, getTestBed} from '@angular/core/testing';
-import {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.configureCompiler({useJit: useJit}).configureTestingModule({
-      TestBed.configureTestingModule({declarations: [SecuredComponent]});
+        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 = `<div [attr.onclick]="ctxProp"></div>`;
         TestBed.overrideComponent(SecuredComponent, {set: {template}});
-        try {
+
-          TestBed.createComponent(SecuredComponent);
+        expect(() => TestBed.createComponent(SecuredComponent))
-          throw 'Should throw';
+            .toThrowError(
-        } catch (e) {
+                /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../);
-          expect(e.message).toContain(
-              `Template parse errors:\n` +
-              `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 {
+        expect(() => TestBed.createComponent(SecuredComponent))
-          TestBed.createComponent(SecuredComponent);
+            .toThrowError(
-          throw 'Should throw';
+                /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../);
-        } catch (e) {
-          expect(e.message).toContain(
-              `Template parse errors:\n` +
-              `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 = `<div [onPrefixedProp]="ctxProp" [onclick]="ctxProp"></div>`;
+        TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({
+          schemas: [NO_ERRORS_SCHEMA]
+        });
+
+        // should not throw for inputs starting with "on"
+        let cmp: ComponentFixture<SecuredComponent>;
+        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 = `<svg:circle [xlink:href]="ctxProp">Text</svg:circle>`;
         TestBed.overrideComponent(SecuredComponent, {set: {template}});
 
-        try {
+        expect(() => TestBed.createComponent(SecuredComponent))
-          TestBed.createComponent(SecuredComponent);
+            .toThrowError(/Can't bind to 'xlink:href'/);
-          throw 'Should throw';
-        } catch (e) {
-          expect(e.message).toContain(`Can't bind to 'xlink:href'`);
-        }
       });
 
       it('should escape unsafe HTML values', () => {