fix(ngcc): prevent including JavaScript sources outside of the package (

#37596)

When ngcc creates an entry-point program, the `allowJs` option is enabled
in order to operate on the JavaScript source files of the entry-point.
A side-effect of this approach is that external modules that don't ship
declaration files will also have their JavaScript source files loaded
into the program, as the `allowJs` flag allows for them to be imported.
This may pose an issue in certain edge cases, where ngcc would inadvertently
operate on these external modules. This can introduce all sorts of undesirable
behavior and incompatibilities, e.g. the reflection host that is selected for
the entry-point's format could be incompatible with that of the external
module's JavaScript bundles.

To avoid these kinds of issues, module resolution that would resolve to
a JavaScript file located outside of the package will instead be rejected,
as if the file would not exist. This would have been the behavior when
`allowJs` is set to false, which is the case in typical Angular compilations.

Fixes #37508

PR Close #37596

packages/compiler-cli/ngcc/src/packages/entry_point_bundle.ts

@@ -50,7 +50,7 @@ export function makeEntryPointBundle(
   const rootDir = entryPoint.packagePath;
   const options: ts
       .CompilerOptions = {allowJs: true, maxNodeModuleJsDepth: Infinity, rootDir, ...pathMappings};
-  const srcHost = new NgccSourcesCompilerHost(fs, options, entryPoint.path);
+  const srcHost = new NgccSourcesCompilerHost(fs, options, entryPoint.packagePath);
   const dtsHost = new NgtscCompilerHost(fs, options);
 
   // Create the bundle programs, as necessary.
@@ -63,7 +63,7 @@ export function makeEntryPointBundle(
       [];
   const dts = transformDts ? makeBundleProgram(
                                  fs, isCore, entryPoint.packagePath, typingsPath, 'r3_symbols.d.ts',
-                                 options, dtsHost, additionalDtsFiles) :
+                                 {...options, allowJs: false}, dtsHost, additionalDtsFiles) :
                              null;
   const isFlatCore = isCore && src.r3SymbolsFile === null;
 

packages/compiler-cli/ngcc/src/packages/ngcc_compiler_host.ts

@@ -7,7 +7,8 @@
  */
 import * as ts from 'typescript';
 
-import {FileSystem, NgtscCompilerHost} from '../../../src/ngtsc/file_system';
+import {AbsoluteFsPath, FileSystem, NgtscCompilerHost} from '../../../src/ngtsc/file_system';
+import {isWithinPackage} from '../analysis/util';
 import {isRelativePath} from '../utils';
 
 /**
@@ -20,7 +21,7 @@ export class NgccSourcesCompilerHost extends NgtscCompilerHost {
   private cache = ts.createModuleResolutionCache(
       this.getCurrentDirectory(), file => this.getCanonicalFileName(file));
 
-  constructor(fs: FileSystem, options: ts.CompilerOptions, protected entryPointPath: string) {
+  constructor(fs: FileSystem, options: ts.CompilerOptions, protected packagePath: AbsoluteFsPath) {
     super(fs, options);
   }
 
@@ -36,13 +37,24 @@ export class NgccSourcesCompilerHost extends NgtscCompilerHost {
       // file was in the same directory. This is undesirable, as we need to have the actual
       // JavaScript being present in the program. This logic recognizes this scenario and rewrites
       // the resolved .d.ts declaration file to its .js counterpart, if it exists.
-      if (resolvedModule !== undefined && resolvedModule.extension === ts.Extension.Dts &&
-          containingFile.endsWith('.js') && isRelativePath(moduleName)) {
+      if (resolvedModule?.extension === ts.Extension.Dts && containingFile.endsWith('.js') &&
+          isRelativePath(moduleName)) {
         const jsFile = resolvedModule.resolvedFileName.replace(/\.d\.ts$/, '.js');
         if (this.fileExists(jsFile)) {
           return {...resolvedModule, resolvedFileName: jsFile, extension: ts.Extension.Js};
         }
       }
+
+      // Prevent loading JavaScript source files outside of the package root, which would happen for
+      // packages that don't have .d.ts files. As ngcc should only operate on the .js files
+      // contained within the package, any files outside the package are simply discarded. This does
+      // result in a partial program with error diagnostics, however ngcc won't gather diagnostics
+      // for the program it creates so these diagnostics won't be reported.
+      if (resolvedModule?.extension === ts.Extension.Js &&
+          !isWithinPackage(this.packagePath, this.fs.resolve(resolvedModule.resolvedFileName))) {
+        return undefined;
+      }
+
       return resolvedModule;
     });
   }

packages/compiler-cli/ngcc/test/helpers/utils.ts

@@ -68,7 +68,7 @@ export function makeTestBundleProgram(
   const rootDir = fs.dirname(entryPointPath);
   const options: ts.CompilerOptions =
       {allowJs: true, maxNodeModuleJsDepth: Infinity, checkJs: false, rootDir, rootDirs: [rootDir]};
-  const host = new NgccSourcesCompilerHost(fs, options, entryPointPath);
+  const host = new NgccSourcesCompilerHost(fs, options, rootDir);
   return makeBundleProgram(
       fs, isCore, rootDir, path, 'r3_symbols.js', options, host, additionalFiles);
 }

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts

@@ -401,6 +401,121 @@ runInEachFileSystem(() => {
       expect(es5Contents).toContain('ɵngcc0.ɵɵtext(0, "a - b - 3 - 4")');
     });
 
+    it('should not crash when scanning for ModuleWithProviders needs to evaluate code from an external package',
+       () => {
+         // Regression test for https://github.com/angular/angular/issues/37508
+         // During `ModuleWithProviders` analysis, return statements in methods are evaluated using
+         // the partial evaluator to identify whether they correspond with a `ModuleWithProviders`
+         // function. If an arbitrary method has a return statement that calls into an external
+         // module which doesn't have declaration files, ngcc would attempt to reflect on said
+         // module using the reflection host of the entry-point. This would crash in the case where
+         // e.g. the entry-point is UMD and the external module would be CommonJS, as the UMD
+         // reflection host would throw because it is unable to deal with CommonJS.
+
+         // Setup a non-TS package with CommonJS module format
+         loadTestFiles([
+           {
+             name: _(`/node_modules/identity/package.json`),
+             contents: `{"name": "identity", "main": "./index.js"}`,
+           },
+           {
+             name: _(`/node_modules/identity/index.js`),
+             contents: `
+            function identity(x) { return x; };
+            exports.identity = identity;
+            module.exports = identity;
+          `,
+           },
+         ]);
+
+         // Setup an Angular entry-point with UMD module format that references an export of the
+         // CommonJS package.
+         loadTestFiles([
+           {
+             name: _('/node_modules/test-package/package.json'),
+             contents: '{"name": "test-package", "main": "./index.js", "typings": "./index.d.ts"}'
+           },
+           {
+             name: _('/node_modules/test-package/index.js'),
+             contents: `
+            (function (global, factory) {
+              typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('identity')) :
+              typeof define === 'function' && define.amd ? define('test', ['exports', 'identity'], factory) :
+              (factory(global.test, global.identity));
+            }(this, (function (exports, identity) { 'use strict';
+              function Foo(x) {
+                // The below statement is analyzed for 'ModuleWithProviders', so is evaluated
+                // by ngcc. The reference into the non-TS CommonJS package used to crash ngcc.
+                return identity.identity(x);
+              }
+              exports.Foo = Foo;
+            })));
+          `
+           },
+           {
+             name: _('/node_modules/test-package/index.d.ts'),
+             contents: 'export declare class Foo { static doSomething(x: any): any; }'
+           },
+           {name: _('/node_modules/test-package/index.metadata.json'), contents: 'DUMMY DATA'},
+         ]);
+
+         expect(() => mainNgcc({
+                  basePath: '/node_modules',
+                  targetEntryPointPath: 'test-package',
+                  propertiesToConsider: ['main'],
+                }))
+             .not.toThrow();
+       });
+
+    it('should not be able to evaluate code in external packages when no .d.ts files are present',
+       () => {
+         loadTestFiles([
+           {
+             name: _(`/node_modules/external/package.json`),
+             contents: `{"name": "external", "main": "./index.js"}`,
+           },
+           {
+             name: _(`/node_modules/external/index.js`),
+             contents: `
+            export const selector = 'my-selector';
+          `,
+           },
+         ]);
+
+         compileIntoApf('test-package', {
+           '/index.ts': `
+          import {NgModule, Component} from '@angular/core';
+          import {selector} from 'external';
+
+          @Component({
+            selector,
+            template: ''
+          })
+          export class FooComponent {
+          }
+
+          @NgModule({
+            declarations: [FooComponent],
+          })
+          export class FooModule {}
+        `,
+         });
+
+         try {
+           mainNgcc({
+             basePath: '/node_modules',
+             targetEntryPointPath: 'test-package',
+             propertiesToConsider: ['esm2015', 'esm5'],
+           });
+           fail('should have thrown');
+         } catch (e) {
+           expect(e.message).toContain(
+               'Failed to compile entry-point test-package (esm2015 as esm2015) due to compilation errors:');
+           expect(e.message).toContain('NG1010');
+           expect(e.message).toContain('selector must be a string');
+         }
+       });
+
     it('should add ɵfac but not duplicate ɵprov properties on injectables', () => {
       compileIntoFlatEs2015Package('test-package', {
         '/index.ts': `

packages/compiler-cli/ngcc/test/packages/entry_point_bundle_spec.ts

@@ -13,8 +13,12 @@ import {makeEntryPointBundle} from '../../src/packages/entry_point_bundle';
 
 runInEachFileSystem(() => {
   describe('entry point bundle', () => {
+    let _: typeof absoluteFrom;
+    beforeEach(() => {
+      _ = absoluteFrom;
+    });
+
     function setupMockFileSystem(): void {
-      const _ = absoluteFrom;
       loadTestFiles([
         {
           name: _('/node_modules/test/package.json'),
@@ -210,6 +214,103 @@ runInEachFileSystem(() => {
              ].map(p => absoluteFrom(p).toString())));
        });
 
+    it('does not include .js files outside of the package when no .d.ts file is available', () => {
+      // Declare main "test" package with "entry" entry-point that imports all sorts of
+      // internal and external modules.
+      loadTestFiles([
+        {
+          name: _('/node_modules/test/entry/package.json'),
+          contents: `{"name": "test", "main": "./index.js", "typings": "./index.d.ts"}`,
+        },
+        {
+          name: _('/node_modules/test/entry/index.d.ts'),
+          contents: `
+              import 'external-js';
+              import 'external-ts';
+              import 'nested-js';
+              import './local';
+              import '../package';
+          `,
+        },
+        {
+          name: _('/node_modules/test/entry/index.js'),
+          contents: `
+              import 'external-js';
+              import 'external-ts';
+              import 'nested-js';
+              import './local';
+              import '../package';
+            `,
+        },
+        {name: _('/node_modules/test/entry/local.d.ts'), contents: `export {};`},
+        {name: _('/node_modules/test/entry/local.js'), contents: `export {};`},
+        {name: _('/node_modules/test/package.d.ts'), contents: `export {};`},
+        {name: _('/node_modules/test/package.js'), contents: `export {};`},
+      ]);
+
+      // Declare "external-js" package outside of the "test" package without .d.ts files, should
+      // not be included in the program.
+      loadTestFiles([
+        {
+          name: _('/node_modules/external-js/package.json'),
+          contents: `{"name": "external-js", "main": "./index.js"}`,
+        },
+        {name: _('/node_modules/external-js/index.js'), contents: 'export {};'},
+      ]);
+
+      // Same as "external-js" but located in a nested node_modules directory, which should also
+      // not be included in the program.
+      loadTestFiles([
+        {
+          name: _('/node_modules/test/node_modules/nested-js/package.json'),
+          contents: `{"name": "nested-js", "main": "./index.js"}`,
+        },
+        {name: _('/node_modules/test/node_modules/nested-js/index.js'), contents: 'export {}'},
+      ]);
+
+      // Declare "external-ts" which does have .d.ts files, so the .d.ts should be
+      // loaded into the program.
+      loadTestFiles([
+        {
+          name: _('/node_modules/external-ts/package.json'),
+          contents: `{"name": "external-ts", "main": "./index.js", "typings": "./index.d.ts"}`,
+        },
+        {name: _('/node_modules/external-ts/index.d.ts'), contents: 'export {};'},
+        {name: _('/node_modules/external-ts/index.js'), contents: 'export {};'},
+      ]);
+
+      const fs = getFileSystem();
+      const entryPoint: EntryPoint = {
+        name: 'test/entry',
+        path: absoluteFrom('/node_modules/test/entry'),
+        packageName: 'test',
+        packagePath: absoluteFrom('/node_modules/test'),
+        packageJson: {name: 'test/entry'},
+        typings: absoluteFrom('/node_modules/test/entry/index.d.ts'),
+        compiledByAngular: true,
+        ignoreMissingDependencies: false,
+        generateDeepReexports: false,
+      };
+      const esm5bundle = makeEntryPointBundle(
+          fs, entryPoint, './index.js', false, 'esm5', /* transformDts */ true,
+          /* pathMappings */ undefined, /* mirrorDtsFromSrc */ true);
+
+      expect(esm5bundle.src.program.getSourceFiles().map(sf => _(sf.fileName)))
+          .toEqual(jasmine.arrayWithExactContents([
+            _('/node_modules/test/entry/index.js'),
+            _('/node_modules/test/entry/local.js'),
+            _('/node_modules/test/package.js'),
+            _('/node_modules/external-ts/index.d.ts'),
+          ]));
+      expect(esm5bundle.dts!.program.getSourceFiles().map(sf => _(sf.fileName)))
+          .toEqual(jasmine.arrayWithExactContents([
+            _('/node_modules/test/entry/index.d.ts'),
+            _('/node_modules/test/entry/local.d.ts'),
+            _('/node_modules/test/package.d.ts'),
+            _('/node_modules/external-ts/index.d.ts'),
+          ]));
+    });
+
     describe(
         'including equivalently named, internally imported, src files in the typings program',
         () => {