fix(core): don't consider inherited NG_ELEMENT_ID during DI (#37574)

Special DI tokens like `ChangeDetectorRef` and `ElementRef` can provide a factory via `NG_ELEMENT_ID`. The problem is that we were reading it off the token as `token[NG_ELEMENT_ID]` which will go up the prototype chain if it couldn't be found on the current token, resulting in the private `ViewRef` API being exposed, because it extends `ChangeDetectorRef`.

These changes fix the issue by guarding the property access with `hasOwnProperty`.

Fixes #36235.

PR Close #37574

packages/core/src/render3/di.ts

@@ -99,8 +99,12 @@ let nextNgElementId = 0;
 export function bloomAdd(
     injectorIndex: number, tView: TView, type: Type<any>|InjectionToken<any>|string): void {
   ngDevMode && assertEqual(tView.firstCreatePass, true, 'expected firstCreatePass to be true');
-  let id: number|undefined =
-      typeof type !== 'string' ? (type as any)[NG_ELEMENT_ID] : type.charCodeAt(0) || 0;
+  let id: number|undefined;
+  if (typeof type === 'string') {
+    id = type.charCodeAt(0) || 0;
+  } else if (type.hasOwnProperty(NG_ELEMENT_ID)) {
+    id = (type as any)[NG_ELEMENT_ID];
+  }
 
   // Set a unique ID on the directive type, so if something tries to inject the directive,
   // we can easily retrieve the ID and hash it into the bloom bit that should be checked.
@@ -584,7 +588,9 @@ export function bloomHashBitOrFactory(token: Type<any>|InjectionToken<any>|strin
   if (typeof token === 'string') {
     return token.charCodeAt(0) || 0;
   }
-  const tokenId: number|undefined = (token as any)[NG_ELEMENT_ID];
+  const tokenId: number|undefined =
+      // First check with `hasOwnProperty` so we don't get an inherited ID.
+      token.hasOwnProperty(NG_ELEMENT_ID) ? (token as any)[NG_ELEMENT_ID] : undefined;
   // Negative token IDs are used for special objects such as `Injector`
   return (typeof tokenId === 'number' && tokenId > 0) ? tokenId & BLOOM_MASK : tokenId;
 }

packages/core/test/acceptance/di_spec.ts

@@ -7,9 +7,9 @@
  */
 
 import {CommonModule} from '@angular/common';
-import {Attribute, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, Directive, ElementRef, EventEmitter, forwardRef, Host, HostBinding, Inject, Injectable, InjectionToken, INJECTOR, Injector, Input, LOCALE_ID, ModuleWithProviders, NgModule, NgZone, Optional, Output, Pipe, PipeTransform, Self, SkipSelf, TemplateRef, ViewChild, ViewContainerRef, ɵDEFAULT_LOCALE_ID as DEFAULT_LOCALE_ID} from '@angular/core';
+import {Attribute, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, Directive, ElementRef, EventEmitter, forwardRef, Host, HostBinding, Inject, Injectable, InjectionToken, INJECTOR, Injector, Input, LOCALE_ID, ModuleWithProviders, NgModule, NgZone, Optional, Output, Pipe, PipeTransform, Self, SkipSelf, TemplateRef, ViewChild, ViewContainerRef, ViewRef, ɵDEFAULT_LOCALE_ID as DEFAULT_LOCALE_ID} from '@angular/core';
 import {ɵINJECTOR_SCOPE} from '@angular/core/src/core';
-import {ViewRef} from '@angular/core/src/render3/view_ref';
+import {ViewRef as ViewRefInternal} from '@angular/core/src/render3/view_ref';
 import {TestBed} from '@angular/core/testing';
 import {ivyEnabled, onlyInIvy} from '@angular/private/testing';
 import {BehaviorSubject} from 'rxjs';
@@ -1627,7 +1627,8 @@ describe('di', () => {
         TestBed.configureTestingModule({declarations: [MyApp, MyPipe], imports: [CommonModule]});
         const fixture = TestBed.createComponent(MyApp);
         fixture.detectChanges();
-        expect((pipeInstance!.cdr as ViewRef<MyApp>).context).toBe(fixture.componentInstance);
+        expect((pipeInstance!.cdr as ViewRefInternal<MyApp>).context)
+            .toBe(fixture.componentInstance);
       });
 
       it('should inject current component ChangeDetectorRef into directives on the same node as components',
@@ -1643,7 +1644,7 @@ describe('di', () => {
            fixture.detectChanges();
            const app = fixture.componentInstance;
            const comp = fixture.componentInstance.component;
-           expect((comp!.cdr as ViewRef<MyComp>).context).toBe(comp);
+           expect((comp!.cdr as ViewRefInternal<MyComp>).context).toBe(comp);
            // ChangeDetectorRef is the token, ViewRef has historically been the constructor
            expect(app.directive.value).toContain('ViewRef');
 
@@ -1664,7 +1665,7 @@ describe('di', () => {
            const fixture = TestBed.createComponent(MyComp);
            fixture.detectChanges();
            const comp = fixture.componentInstance;
-           expect((comp!.cdr as ViewRef<MyComp>).context).toBe(comp);
+           expect((comp!.cdr as ViewRefInternal<MyComp>).context).toBe(comp);
            // ChangeDetectorRef is the token, ViewRef has historically been the constructor
            expect(comp.directive.value).toContain('ViewRef');
 
@@ -1692,7 +1693,7 @@ describe('di', () => {
            const fixture = TestBed.createComponent(MyApp);
            fixture.detectChanges();
            const app = fixture.componentInstance;
-           expect((app!.cdr as ViewRef<MyApp>).context).toBe(app);
+           expect((app!.cdr as ViewRefInternal<MyApp>).context).toBe(app);
            const comp = fixture.componentInstance.component;
            // ChangeDetectorRef is the token, ViewRef has historically been the constructor
            expect(app.directive.value).toContain('ViewRef');
@@ -1720,7 +1721,7 @@ describe('di', () => {
         const fixture = TestBed.createComponent(MyComp);
         fixture.detectChanges();
         const comp = fixture.componentInstance;
-        expect((comp!.cdr as ViewRef<MyComp>).context).toBe(comp);
+        expect((comp!.cdr as ViewRefInternal<MyComp>).context).toBe(comp);
         // ChangeDetectorRef is the token, ViewRef has historically been the constructor
         expect(comp.directive.value).toContain('ViewRef');
 
@@ -1743,7 +1744,7 @@ describe('di', () => {
         const fixture = TestBed.createComponent(MyComp);
         fixture.detectChanges();
         const comp = fixture.componentInstance;
-        expect((comp!.cdr as ViewRef<MyComp>).context).toBe(comp);
+        expect((comp!.cdr as ViewRefInternal<MyComp>).context).toBe(comp);
         // ChangeDetectorRef is the token, ViewRef has historically been the constructor
         expect(comp.directive.value).toContain('ViewRef');
 
@@ -1773,7 +1774,8 @@ describe('di', () => {
         TestBed.configureTestingModule({declarations: [MyApp, MyDirective]});
         const fixture = TestBed.createComponent(MyApp);
         fixture.detectChanges();
-        expect((dirInstance!.cdr as ViewRef<MyApp>).context).toBe(fixture.componentInstance);
+        expect((dirInstance!.cdr as ViewRefInternal<MyApp>).context)
+            .toBe(fixture.componentInstance);
       });
     });
   });
@@ -2300,4 +2302,14 @@ describe('di', () => {
       expect(fixture.componentInstance.dir.token).toBe('parent');
     });
   });
+
+  it('should not be able to inject ViewRef', () => {
+    @Component({template: ''})
+    class App {
+      constructor(_viewRef: ViewRef) {}
+    }
+
+    TestBed.configureTestingModule({declarations: [App]});
+    expect(() => TestBed.createComponent(App)).toThrowError(/NullInjectorError/);
+  });
 });