feat(@angular-devkit/schematics): disable package script execution by default in NodePackageInstallTask

clydin · dgp1130 · commit 0e6425fd88ea · 2022-04-29T16:37:56.000-07:00
In an effort to improve supply chain security, the `NodePackageInstallTask` will now use the package
manager's `--ignore-scripts` option by default. Without the option, all direct and transitive
dependencies would have their scripts executed during the task's package manager installation operation.
The change only affects the package manager behavior controlled by the Schematics `NodePackageInstallTask`.

First-party Angular schematics do not currently require any direct or transitive dependency
`install`/`postinstall` scripts to execute. Only two dependencies within a v14.0 new project would
potentially be affected by this: `nice-napi` (transitive from `piscina`) and `esbuild`. The `nice-napi`
functionality of `piscina` is unused within the Angular CLI with no plans to use it in the future.
Even if it was used, the `install` script runs `node-gyp-build` which would only have an effect
(based on the current version 1.0.2) on platforms that are not Windows, darwin-x64, or linux-x64.
In the event this functionality is eventually used, the Angular CLI could be setup to automatically execute
this particular script for unsupported platforms. For `esbuild`, the `postinstall` functionality
performs an optional native binary bootstrap optimization but would only be performed if not
using Windows or Yarn. As such, it would not be performed for many users regardless of the change in
this commit. If noticeable performance regressions on platforms where the optimization was previously
performed are reported, the script could also be setup to be automatically run by the Angular CLI during
project creation and/or first build.

BREAKING CHANGE: Schematics `NodePackageInstallTask` will not execute package scripts by default
The `NodePackageInstallTask` will now use the package manager's `--ignore-scripts` option by default.
The `--ignore-scripts` option will prevent package scripts from executing automatically during an install.
If a schematic installs packages that need their `install`/`postinstall` scripts to be executed, the
`NodePackageInstallTask` now contains an `allowScripts` boolean option which can be enabled to provide the
previous behavior for that individual task. As with previous behavior, the `allowScripts` option will
prevent the individual task's usage of the `--ignore-scripts` option but will not override the package
manager's existing configuration.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -325,7 +325,7 @@ jobs:
           name: Execute E2E Tests
           command: |
             if (Test-Path env:CIRCLE_PULL_REQUEST) {
-              node tests\legacy-cli\run_e2e.js "--glob={tests/basic/**,tests/i18n/extract-ivy*.ts,tests/build/profile.ts,tests/test/test-sourcemap.ts}" --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX
+              node tests\legacy-cli\run_e2e.js "--glob={tests/basic/**,tests/i18n/extract-ivy*.ts,tests/build/profile.ts,tests/test/test-sourcemap.ts,tests/misc/check-postinstalls.ts}" --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX
             } else {
               node tests\legacy-cli\run_e2e.js --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX
             }
diff --git a/goldens/public-api/angular_devkit/schematics/tasks/index.md b/goldens/public-api/angular_devkit/schematics/tasks/index.md
@@ -11,6 +11,8 @@ export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePa
     constructor(workingDirectory?: string);
     constructor(options: NodePackageInstallTaskOptions);
     // (undocumented)
+    allowScripts: boolean;
+    // (undocumented)
     hideOutput: boolean;
     // (undocumented)
     packageManager?: string;
diff --git a/packages/angular_devkit/schematics/tasks/package-manager/executor.ts b/packages/angular_devkit/schematics/tasks/package-manager/executor.ts
@@ -100,6 +100,10 @@ export default function (
       args.push(taskPackageManagerProfile.quietArgument);
     }
 
+    if (!options.allowScripts) {
+      args.push('--ignore-scripts');
+    }
+
     if (factoryOptions.registry) {
       args.push(`--registry="${factoryOptions.registry}"`);
     }
diff --git a/packages/angular_devkit/schematics/tasks/package-manager/install-task.ts b/packages/angular_devkit/schematics/tasks/package-manager/install-task.ts
@@ -15,11 +15,13 @@ interface NodePackageInstallTaskOptions {
   workingDirectory?: string;
   quiet?: boolean;
   hideOutput?: boolean;
+  allowScripts?: boolean;
 }
 
 export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePackageTaskOptions> {
   quiet = true;
   hideOutput = true;
+  allowScripts = false;
   workingDirectory?: string;
   packageManager?: string;
   packageName?: string;
@@ -45,6 +47,9 @@ export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePa
       if (options.packageName != undefined) {
         this.packageName = options.packageName;
       }
+      if (options.allowScripts !== undefined) {
+        this.allowScripts = options.allowScripts;
+      }
     }
   }
 
@@ -58,6 +63,7 @@ export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePa
         workingDirectory: this.workingDirectory,
         packageManager: this.packageManager,
         packageName: this.packageName,
+        allowScripts: this.allowScripts,
       },
     };
   }
diff --git a/packages/angular_devkit/schematics/tasks/package-manager/options.ts b/packages/angular_devkit/schematics/tasks/package-manager/options.ts
@@ -23,4 +23,5 @@ export interface NodePackageTaskOptions {
   workingDirectory?: string;
   packageName?: string;
   packageManager?: string;
+  allowScripts?: boolean;
 }
diff --git a/tests/legacy-cli/e2e/assets/schematic-allow-scripts/collection.json b/tests/legacy-cli/e2e/assets/schematic-allow-scripts/collection.json
@@ -0,0 +1,9 @@
+{
+  "schematics": {
+    "test": {
+      "factory": "./index.js",
+      "schema": "./schema.json",
+      "description": "test schematic that creates a directory with a local test package that should not run its post-install script"
+    }
+  }
+}
diff --git a/tests/legacy-cli/e2e/assets/schematic-allow-scripts/index.js b/tests/legacy-cli/e2e/assets/schematic-allow-scripts/index.js
@@ -0,0 +1,16 @@
+const tasks = require("@angular-devkit/schematics/tasks");
+
+exports.default = ({ allowScripts, ignoreScripts = false }) => {
+    return (tree, context) => {
+        tree.create('/install-test/package.json', JSON.stringify({
+            name: 'install-test',
+            version: '0.0.0',
+            scripts: {
+                postinstall: `node run-post.js`,
+            }
+        }));
+        tree.create('/install-test/.npmrc', `ignore-scripts=${ignoreScripts}`);
+        tree.create('/install-test/run-post.js', 'require("fs").writeFileSync(__dirname + "/post-script-ran", "12345");')
+        context.addTask(new tasks.NodePackageInstallTask({ workingDirectory: 'install-test', allowScripts }));
+    };
+};
diff --git a/tests/legacy-cli/e2e/assets/schematic-allow-scripts/package.json b/tests/legacy-cli/e2e/assets/schematic-allow-scripts/package.json
@@ -0,0 +1,5 @@
+{
+  "name": "allow-scripts",
+  "version": "0.0.1",
+  "schematics": "./collection.json"
+}
diff --git a/tests/legacy-cli/e2e/assets/schematic-allow-scripts/schema.json b/tests/legacy-cli/e2e/assets/schematic-allow-scripts/schema.json
@@ -0,0 +1,14 @@
+{
+    "$schema": "http://json-schema.org/draft-07/schema",
+    "type": "object",
+    "additionalProperties": false,
+    "properties": {
+        "allowScripts": {
+            "type": "boolean"
+        },
+        "ignoreScripts": {
+            "type": "boolean"
+        }
+    }
+}
+  
diff --git a/tests/legacy-cli/e2e/tests/generate/install-allow-scripts.ts b/tests/legacy-cli/e2e/tests/generate/install-allow-scripts.ts
@@ -0,0 +1,31 @@
+import { copyAssets } from '../../utils/assets';
+import { expectFileNotToExist, expectFileToExist, rimraf } from '../../utils/fs';
+import { ng } from '../../utils/process';
+
+export default async function () {
+  // Copy test schematic into test project to ensure schematic dependencies are available
+  await copyAssets('schematic-allow-scripts', 'schematic-allow-scripts');
+
+  // By default should not run the postinstall from the added package.json in the schematic
+  await ng('generate', './schematic-allow-scripts:test');
+  await expectFileToExist('install-test/package.json');
+  await expectFileNotToExist('install-test/post-script-ran');
+
+  // Cleanup for next test case
+  await rimraf('install-test');
+
+  // Should run the postinstall if the allowScripts task option is enabled
+  // For testing purposes, this schematic exposes the task option via a schematic option
+  await ng('generate', './schematic-allow-scripts:test', '--allow-scripts');
+  await expectFileToExist('install-test/package.json');
+  await expectFileToExist('install-test/post-script-ran');
+
+  // Cleanup for next test case
+  await rimraf('install-test');
+
+  // Package manager configuration should take priority
+  // The `ignoreScripts` schematic option sets the value of the `ignore-scripts` option in a test project `.npmrc`
+  await ng('generate', './schematic-allow-scripts:test', '--allow-scripts', '--ignore-scripts');
+  await expectFileToExist('install-test/package.json');
+  await expectFileNotToExist('install-test/post-script-ran');
+}
diff --git a/tests/legacy-cli/e2e/tests/misc/check-postinstalls.ts b/tests/legacy-cli/e2e/tests/misc/check-postinstalls.ts
@@ -0,0 +1,58 @@
+import glob from 'glob';
+import { promisify } from 'util';
+import { readFile } from '../../utils/fs';
+
+const globAsync = promisify(glob);
+
+const CURRENT_SCRIPT_PACKAGES: ReadonlySet<string> = new Set([
+  'esbuild (postinstall)',
+  'nice-napi (install)',
+]);
+
+const POTENTIAL_SCRIPTS: ReadonlyArray<string> = ['preinstall', 'install', 'postinstall'];
+
+// Some packages include test and/or example code that causes false positives
+const FALSE_POSITIVE_PATHS: ReadonlySet<string> = new Set([
+  'node_modules/jasmine-spec-reporter/examples/protractor/package.json',
+  'node_modules/resolve/test/resolver/multirepo/package.json',
+]);
+
+export default async function () {
+  const manifestPaths = await globAsync('node_modules/**/package.json');
+  const newPackages: string[] = [];
+
+  for (const manifestPath of manifestPaths) {
+    if (FALSE_POSITIVE_PATHS.has(manifestPath)) {
+      continue;
+    }
+
+    let manifest;
+    try {
+      manifest = JSON.parse(await readFile(manifestPath));
+    } catch {
+      continue;
+    }
+
+    if (!manifest.scripts) {
+      continue;
+    }
+
+    for (const script of POTENTIAL_SCRIPTS) {
+      if (!manifest.scripts[script]) {
+        continue;
+      }
+
+      const packageScript = `${manifest.name} (${script})`;
+
+      if (!CURRENT_SCRIPT_PACKAGES.has(packageScript)) {
+        newPackages.push(packageScript + `[${manifestPath}]`);
+      }
+    }
+  }
+
+  if (newPackages.length) {
+    throw new Error(
+      'New install script package(s) detected:\n' + JSON.stringify(newPackages, null, 2),
+    );
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -325,7 +325,7 @@ jobs:`
`325`	`325`	`name: Execute E2E Tests`
`326`	`326`	`command: \|`
`327`	`327`	`if (Test-Path env:CIRCLE_PULL_REQUEST) {`
`328`		`- node tests\legacy-cli\run_e2e.js "--glob={tests/basic/*,tests/i18n/extract-ivy.ts,tests/build/profile.ts,tests/test/test-sourcemap.ts}" --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX`
	`328`	`+ node tests\legacy-cli\run_e2e.js "--glob={tests/basic/*,tests/i18n/extract-ivy.ts,tests/build/profile.ts,tests/test/test-sourcemap.ts,tests/misc/check-postinstalls.ts}" --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX`
`329`	`329`	`} else {`
`330`	`330`	`node tests\legacy-cli\run_e2e.js --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX`
`331`	`331`	`}`
Original file line number	Diff line number	Diff line change
`@@ -100,6 +100,10 @@ export default function (`
`100`	`100`	`args.push(taskPackageManagerProfile.quietArgument);`
`101`	`101`	`}`
`102`	`102`
	`103`	`+ if (!options.allowScripts) {`
	`104`	`+ args.push('--ignore-scripts');`
	`105`	`+ }`
	`106`	`+`
`103`	`107`	`if (factoryOptions.registry) {`
`104`	`108`	args.push(`--registry="${factoryOptions.registry}"`);
`105`	`109`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,11 +15,13 @@ interface NodePackageInstallTaskOptions {`
`15`	`15`	`workingDirectory?: string;`
`16`	`16`	`quiet?: boolean;`
`17`	`17`	`hideOutput?: boolean;`
	`18`	`+ allowScripts?: boolean;`
`18`	`19`	`}`
`19`	`20`
`20`	`21`	`export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePackageTaskOptions> {`
`21`	`22`	`quiet = true;`
`22`	`23`	`hideOutput = true;`
	`24`	`+ allowScripts = false;`
`23`	`25`	`workingDirectory?: string;`
`24`	`26`	`packageManager?: string;`
`25`	`27`	`packageName?: string;`
`@@ -45,6 +47,9 @@ export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePa`
`45`	`47`	`if (options.packageName != undefined) {`
`46`	`48`	`this.packageName = options.packageName;`
`47`	`49`	`}`
	`50`	`+ if (options.allowScripts !== undefined) {`
	`51`	`+ this.allowScripts = options.allowScripts;`
	`52`	`+ }`
`48`	`53`	`}`
`49`	`54`	`}`
`50`	`55`
`@@ -58,6 +63,7 @@ export class NodePackageInstallTask implements TaskConfigurationGenerator<NodePa`
`58`	`63`	`workingDirectory: this.workingDirectory,`
`59`	`64`	`packageManager: this.packageManager,`
`60`	`65`	`packageName: this.packageName,`
	`66`	`+ allowScripts: this.allowScripts,`
`61`	`67`	`},`
`62`	`68`	`};`
`63`	`69`	`}`
Original file line number	Diff line number	Diff line change
`@@ -23,4 +23,5 @@ export interface NodePackageTaskOptions {`
`23`	`23`	`workingDirectory?: string;`
`24`	`24`	`packageName?: string;`
`25`	`25`	`packageManager?: string;`
	`26`	`+ allowScripts?: boolean;`
`26`	`27`	`}`