fix(ivy): validate props and attrs with "on" prefix at runtime (#28054)

Prior to this change we performed prop and attr name validation at compile time, which failed in case a given prop/attr is an input to a Directive (thus should not be a subject to this check). Since Directive matching in Ivy happens at runtime, the corresponding checks are now moved to runtime as well.

PR Close #28054

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));
       }
     });

packages/compiler/src/template_parser/binding_parser.ts

@@ -238,8 +238,9 @@ export class BindingParser {
     }
   }
 
-  createBoundElementProperty(elementSelector: string, boundProp: ParsedProperty):
+  createBoundElementProperty(
-      BoundElementProperty {
+      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(

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<T>(
       }
     }
   } 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)) {

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];

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*', () => {
+      modifiedInIvy('on-prefixed attributes validation happens at runtime in Ivy')
-        const template = `<div [attr.onclick]="ctxProp"></div>`;
+          .it('should disallow binding to attr.on*', () => {
-        TestBed.overrideComponent(SecuredComponent, {set: {template}});
+            const template = `<div [attr.onclick]="ctxProp"></div>`;
+            TestBed.overrideComponent(SecuredComponent, {set: {template}});
 
-        expect(() => TestBed.createComponent(SecuredComponent))
+            expect(() => TestBed.createComponent(SecuredComponent))
-            .toThrowError(
+                .toThrowError(
-                /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../);
+                    /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../);
-      });
+          });
 
-      it('should disallow binding to on* with NO_ERRORS_SCHEMA', () => {
+      // this test is similar to the previous one, but since on-prefixed attributes validation now
-        const template = `<div [onclick]="ctxProp"></div>`;
+      // happens at runtime, we need to invoke change detection to trigger elementProperty call
-        TestBed.overrideComponent(SecuredComponent, {set: {template}}).configureTestingModule({
+      onlyInIvy('on-prefixed attributes validation happens at runtime in Ivy')
-          schemas: [NO_ERRORS_SCHEMA]
+          .it('should disallow binding to attr.on*', () => {
-        });
+            const template = `<div [attr.onclick]="ctxProp"></div>`;
+            TestBed.overrideComponent(SecuredComponent, {set: {template}});
 
-        expect(() => TestBed.createComponent(SecuredComponent))
+            expect(() => {
-            .toThrowError(
+              const cmp = TestBed.createComponent(SecuredComponent);
-                /Binding to event property 'onclick' is disallowed for security reasons, please use \(click\)=.../);
+              cmp.detectChanges();
-      });
+            })
+                .toThrowError(
+                    /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../);
+          });
 
-      fixmeIvy(
+      modifiedInIvy('on-prefixed attributes validation happens at runtime in Ivy')
-          'FW-786: Element properties and directive inputs are not distinguished for sanitisation purposes')
+          .it('should disallow binding to on* with NO_ERRORS_SCHEMA', () => {
-          .it('should disallow binding to on* unless it is consumed by a directive', () => {
+            const template = `<div [onclick]="ctxProp"></div>`;
-            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"
+            expect(() => TestBed.createComponent(SecuredComponent))
-            let cmp: ComponentFixture<SecuredComponent> = undefined !;
+                .toThrowError(
-            expect(() => cmp = TestBed.createComponent(SecuredComponent)).not.toThrow();
+                    /Binding to event property 'onclick' is disallowed for security reasons, please use \(click\)=.../);
-
-            // 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);
           });
 
+      // 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 = `<div [onclick]="ctxProp"></div>`;
+            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 = `<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> = 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() {