fix(core): disallow event attribute bindings in host bindings unconditionally

alan-agius4 · mattrbeck · commit 1c6553e97d96 · 2026-05-07T15:19:26.000-07:00
Moves the event attribute validation check outside of `ngDevMode` in the `elementAttributeInternal` instruction to ensure that bindings to event attributes like `on*` are always blocked at runtime. (cherry picked from commit 5b421c6)
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts
@@ -156,6 +156,21 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker {
     schemas: SchemaMetadata[],
     hostIsStandalone: boolean,
   ): void {
+    const report = REGISTRY.validateProperty(name);
+    if (report.error) {
+      const mapping = this.resolver.getTemplateSourceMapping(id);
+      const diag = makeTemplateDiagnostic(
+        id,
+        mapping,
+        span,
+        ts.DiagnosticCategory.Error,
+        ngErrorCode(ErrorCode.SCHEMA_INVALID_ATTRIBUTE),
+        report.msg!,
+      );
+      this._diagnostics.push(diag);
+      return;
+    }
+
     if (!REGISTRY.hasProperty(tagName, name, schemas)) {
       const mapping = this.resolver.getTemplateSourceMapping(id);
 
@@ -198,6 +213,21 @@ export class RegistryDomSchemaChecker implements DomSchemaChecker {
     span: ParseSourceSpan,
     schemas: SchemaMetadata[],
   ): void {
+    const report = REGISTRY.validateProperty(name);
+    if (report.error) {
+      const mapping = this.resolver.getHostBindingsMapping(id);
+      const diag = makeTemplateDiagnostic(
+        id,
+        mapping,
+        span,
+        ts.DiagnosticCategory.Error,
+        ngErrorCode(ErrorCode.SCHEMA_INVALID_ATTRIBUTE),
+        report.msg!,
+      );
+      this._diagnostics.push(diag);
+      return;
+    }
+
     for (const tagName of element.tagNames) {
       if (REGISTRY.hasProperty(tagName, name, schemas)) {
         continue;
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
@@ -187,6 +187,21 @@ runInEachFileSystem(() => {
       ]);
     });
 
+    it('should disallow binding to event properties starting with on', () => {
+      const messages = diagnose(
+        `<div [onclick]="handler"></div>`,
+        `
+      class TestComponent {
+        handler: any;
+      }`,
+      );
+
+      expect(messages).toEqual([
+        `TestComponent.html(1, 6): 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.`,
+      ]);
+    });
+
     it('checks text attributes that are consumed by bindings with literal string types', () => {
       const messages = diagnose(
         `<div dir mode="drak"></div><div dir mode="light"></div>`,
diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts
@@ -864,12 +864,6 @@ function extractHostBindings(
   // First parse the declarations from the metadata.
   const bindings = parseHostBindings(host || {});
 
-  // After that check host bindings for errors
-  const errors = verifyHostBindings(bindings, sourceSpan);
-  if (errors.length) {
-    throw new Error(errors.map((error: ParseError) => error.msg).join('\n'));
-  }
-
   // Next, loop over the properties of the object, looking for @HostBinding and @HostListener.
   for (const field in propMetadata) {
     if (propMetadata.hasOwnProperty(field)) {
@@ -889,6 +883,12 @@ function extractHostBindings(
     }
   }
 
+  // After that check host bindings for errors
+  const errors = verifyHostBindings(bindings, sourceSpan);
+  if (errors.length) {
+    throw new Error(errors.map((error: ParseError) => error.msg).join('\n'));
+  }
+
   return bindings;
 }
 
diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts
@@ -591,10 +591,11 @@ class HtmlAstToIvyAst implements html.Visitor {
         // Note that validation is skipped and property mapping is disabled
         // due to the fact that we need to make sure a given prop is not an
         // input of a directive and directive matching happens at runtime.
+        const isAttrOn = prop.name.toLowerCase().startsWith('attr.on');
         const bep = this.bindingParser.createBoundElementProperty(
           elementName,
           prop,
-          /* skipValidation */ true,
+          /* skipValidation */ !isAttrOn,
           /* mapPropertyName */ false,
         );
         bound.push(t.BoundAttribute.fromBoundElementProperty(bep, i18n));
diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts
@@ -618,9 +618,41 @@ export function verifyHostBindings(
   const bindingParser = makeBindingParser();
   bindingParser.createDirectiveHostEventAsts(bindings.listeners, sourceSpan);
   bindingParser.createBoundHostProperties(bindings.properties, sourceSpan);
+
+  validateNoEventBindings(bindings, bindingParser, sourceSpan);
+
   return bindingParser.errors;
 }
 
+/**
+ * Validates that there are no event attribute bindings in the host bindings.
+ * @param bindings - Map of host bindings for the component.
+ * @param bindingParser - Binding parser used to create the binding expression.
+ * @param sourceSpan - Source span where the host bindings were defined.
+ */
+function validateNoEventBindings(
+  bindings: ParsedHostBindings,
+  bindingParser: BindingParser,
+  sourceSpan: ParseSourceSpan,
+): void {
+  for (const prop in bindings.properties) {
+    const isAttr = prop.startsWith('attr.');
+    const boundName = isAttr ? prop.slice(5) : prop;
+
+    if (boundName.toLowerCase().startsWith('on')) {
+      const errorType = isAttr ? 'attribute' : 'property';
+      const suggestion = `(${boundName.slice(2)})=...`;
+
+      let msg = `Binding to event ${errorType} '${boundName}' is disallowed for security reasons, please use ${suggestion}`;
+      if (!isAttr) {
+        msg += `\nIf '${prop}' is a directive input, make sure the directive is imported by the current module.`;
+      }
+
+      bindingParser.errors.push(new ParseError(sourceSpan, msg));
+    }
+  }
+}
+
 function compileStyles(styles: string[], selector: string, hostSelector: string): string[] {
   const shadowCss = new ShadowCss();
   return styles.map((style) => {
diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts
@@ -12,10 +12,8 @@ import {hasSkipHydrationAttrOnRElement} from '../../hydration/skip_hydration';
 import {PRESERVE_HOST_CONTENT, PRESERVE_HOST_CONTENT_DEFAULT} from '../../hydration/tokens';
 import {processTextNodeMarkersBeforeHydration} from '../../hydration/utils';
 import {ViewEncapsulation} from '../../metadata/view';
-import {
-  validateAgainstEventAttributes,
-  validateAgainstEventProperties,
-} from '../../sanitization/sanitization';
+import {validateAgainstEventProperties} from '../../sanitization/sanitization';
+
 import {assertIndexInRange, assertNotSame} from '../../util/assert';
 import {escapeCommentText} from '../../util/dom';
 import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../ng_reflect';
@@ -296,7 +294,9 @@ export function setDomProperty<T>(
     const element = getNativeByTNode(tNode, lView) as RElement | RComment;
 
     if (ngDevMode) {
-      validateAgainstEventProperties(propName);
+      if (lView[TVIEW].firstUpdatePass) {
+        validateAgainstEventProperties(propName);
+      }
       if (!isPropertyValid(element, propName, tNode.value, lView[TVIEW].schemas)) {
         handleUnknownPropertyError(propName, tNode.value, tNode.type, lView);
       }
@@ -506,14 +506,14 @@ export function elementAttributeInternal(
 ) {
   if (ngDevMode) {
     assertNotSame(value, NO_CHANGE as any, 'Incoming value should never be NO_CHANGE.');
-    validateAgainstEventAttributes(name);
     assertTNodeType(
       tNode,
       TNodeType.Element,
       `Attempted to set attribute \`${name}\` on a container node. ` +
         `Host bindings are not valid on ng-container or ng-template.`,
     );
   }
+
   const element = getNativeByTNode(tNode, lView) as RElement;
   setElementAttribute(lView[RENDERER], element, namespace, tNode.value, name, value, sanitizer);
 }
diff --git a/packages/core/src/sanitization/sanitization.ts b/packages/core/src/sanitization/sanitization.ts
@@ -268,15 +268,6 @@ export function validateAgainstEventProperties(name: string) {
   }
 }
 
-export function validateAgainstEventAttributes(name: string) {
-  if (name.toLowerCase().startsWith('on')) {
-    const errorMessage =
-      `Binding to event attribute '${name}' is disallowed for security reasons, ` +
-      `please use (${name.slice(2)})=...`;
-    throw new RuntimeError(RuntimeErrorCode.INVALID_EVENT_BINDING, errorMessage);
-  }
-}
-
 function getSanitizer(): Sanitizer | null {
   const lView = getLView();
   return lView && lView[ENVIRONMENT].sanitizer;
diff --git a/packages/core/test/linker/security_integration_spec.ts b/packages/core/test/linker/security_integration_spec.ts
@@ -30,16 +30,14 @@ class OnPrefixDir {
 
 describe('security integration tests', function () {
   beforeEach(() => {
+    // Disable logging for these tests.
+    spyOn(console, 'log').and.callFake(() => {});
+
     TestBed.configureTestingModule({
       declarations: [SecuredComponent, OnPrefixDir],
     });
   });
 
-  beforeEach(() => {
-    // Disable logging for these tests.
-    spyOn(console, 'log').and.callFake(() => {});
-  });
-
   describe('events', () => {
     // 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
@@ -48,8 +46,7 @@ describe('security integration tests', function () {
       TestBed.overrideComponent(SecuredComponent, {set: {template}});
 
       expect(() => {
-        const cmp = TestBed.createComponent(SecuredComponent);
-        cmp.detectChanges();
+        TestBed.createComponent(SecuredComponent);
       }).toThrowError(
         /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../,
       );
@@ -89,6 +86,44 @@ describe('security integration tests', function () {
       expect(div.nativeElement.onclick).not.toBe(value);
       expect(div.nativeElement.hasAttribute('onclick')).toEqual(false);
     });
+
+    for (const ngDevModeValue of [true, false]) {
+      it(`should disallow binding to attr.on* in host bindings with ngDevMode=${ngDevModeValue}`, () => {
+        const originalNgDevMode = (globalThis as any).ngDevMode;
+        (globalThis as any).ngDevMode = ngDevModeValue;
+
+        @Directive({
+          selector: '[dirOnclick]',
+          standalone: false,
+        })
+        class LocalHostOnclickDirective {
+          @HostBinding('attr.onclick') @Input() dirOnclick: string | undefined;
+        }
+
+        @Component({
+          selector: 'local-comp',
+          template: `<button [dirOnclick]="ctxProp"></button>`,
+          standalone: false,
+        })
+        class LocalSecuredComponent {
+          ctxProp: any = 'some value';
+        }
+
+        try {
+          TestBed.configureTestingModule({
+            declarations: [LocalSecuredComponent, LocalHostOnclickDirective],
+          });
+
+          expect(() => {
+            TestBed.createComponent(LocalSecuredComponent);
+          }).toThrowError(
+            /Binding to event attribute 'onclick' is disallowed for security reasons, please use \(click\)=.../,
+          );
+        } finally {
+          (globalThis as any).ngDevMode = originalNgDevMode;
+        }
+      });
+    }
   });
 
   describe('safe HTML values', function () {
