From 4a3322d449bce34a3bcddd6eb1d2803c6392ffcd Mon Sep 17 00:00:00 2001
From: nlf <quitlahok@gmail.com>
Date: Tue, 16 Feb 2021 13:36:14 -0800
Subject: [PATCH] fix: skip optional deps with mismatched platform or engine

PR-URL: https://github.com/npm/arborist/pull/231
Credit: @nlf
Close: #231
Reviewed-by: @isaacs
---
 lib/arborist/reify.js                         | 13 +++++++
 .../test-arborist-reify.js-TAP.test.js        | 36 +++++++++++++++++++
 test/arborist/reify.js                        | 11 ++++++
 3 files changed, 60 insertions(+)

diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js
index b33823e46..7fd0728b0 100644
--- a/lib/arborist/reify.js
+++ b/lib/arborist/reify.js
@@ -16,6 +16,7 @@ const mkdirp = require('mkdirp-infer-owner')
 const moveFile = require('@npmcli/move-file')
 const rimraf = promisify(require('rimraf'))
 const packageContents = require('@npmcli/installed-package-contents')
+const { checkEngine, checkPlatform } = require('npm-install-checks')
 
 const treeCheck = require('../tree-check.js')
 const relpath = require('../relpath.js')
@@ -446,7 +447,19 @@ module.exports = cls => class Reifier extends cls {
     process.emit('time', timer)
     this.addTracker('reify', node.name, node.location)
 
+    const { npmVersion, nodeVersion } = this.options
     const p = Promise.resolve()
+      .then(() => {
+        // when we reify an optional node, check the engine and platform
+        // first. be sure to ignore the --force and --engine-strict flags,
+        // since we always want to skip any optional packages we can't install.
+        // these checks throwing will result in a rollback and removal
+        // of the mismatches
+        if (node.optional) {
+          checkEngine(node.package, npmVersion, nodeVersion, false)
+          checkPlatform(node.package, false)
+        }
+      })
       .then(() => this[_checkBins](node))
       .then(() => this[_extractOrLink](node))
       .then(() => this[_warnDeprecated](node))
diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js
index 8dbeb3864..8e94a08c6 100644
--- a/tap-snapshots/test-arborist-reify.js-TAP.test.js
+++ b/tap-snapshots/test-arborist-reify.js-TAP.test.js
@@ -1018,6 +1018,24 @@ ArboristNode {
 }
 `
 
+exports[`test/arborist/reify.js TAP do not install optional deps with mismatched platform specifications > expect resolving Promise 1`] = `
+ArboristNode {
+  "edgesOut": Map {
+    "platform-specifying-test-package" => EdgeOut {
+      "name": "platform-specifying-test-package",
+      "spec": "1.0.0",
+      "to": null,
+      "type": "optional",
+    },
+  },
+  "location": "",
+  "name": "reify-do-not-install-optional-deps-with-mismatched-platform-specifications",
+  "packageName": "platform-test",
+  "path": "{CWD}/test/arborist/reify-do-not-install-optional-deps-with-mismatched-platform-specifications",
+  "version": "1.0.0",
+}
+`
+
 exports[`test/arborist/reify.js TAP do not update shrinkwrapped deps > expect resolving Promise 1`] = `
 ArboristNode {
   "children": Map {
@@ -29410,6 +29428,24 @@ exports[`test/arborist/reify.js TAP scoped registries > should preserve original
 @ruyadorno/theoretically-private-pkg@https://npm.pkg.github.com/@ruyadorno/theoretically-private-pkg/-/theoretically-private-pkg-1.2.3.tgz
 `
 
+exports[`test/arborist/reify.js TAP still do not install optional deps with mismatched platform specifications even when forced > expect resolving Promise 1`] = `
+ArboristNode {
+  "edgesOut": Map {
+    "platform-specifying-test-package" => EdgeOut {
+      "name": "platform-specifying-test-package",
+      "spec": "1.0.0",
+      "to": null,
+      "type": "optional",
+    },
+  },
+  "location": "",
+  "name": "reify-still-do-not-install-optional-deps-with-mismatched-platform-specifications-even-when-forced",
+  "packageName": "platform-test",
+  "path": "{CWD}/test/arborist/reify-still-do-not-install-optional-deps-with-mismatched-platform-specifications-even-when-forced",
+  "version": "1.0.0",
+}
+`
+
 exports[`test/arborist/reify.js TAP store files with a custom indenting > must match snapshot 1`] = `
 {
 	"name": "tab-indented-package-json",
diff --git a/test/arborist/reify.js b/test/arborist/reify.js
index 64460fb49..f5ef05641 100644
--- a/test/arborist/reify.js
+++ b/test/arborist/reify.js
@@ -363,6 +363,17 @@ t.test('do not update shrinkwrapped deps', t =>
     fixture(t, 'shrinkwrapped-dep-with-lock'),
     { update: { names: ['abbrev']}})))
 
+t.test('do not install optional deps with mismatched platform specifications', t =>
+  t.resolveMatchSnapshot(printReified(
+    fixture(t, 'optional-platform-specification'))))
+
+t.test('still do not install optional deps with mismatched platform specifications even when forced', t =>
+  t.resolveMatchSnapshot(printReified(
+    fixture(t, 'optional-platform-specification'), { force: true })))
+
+t.test('fail to install deps with mismatched platform specifications', t =>
+  t.rejects(printReified(fixture(t, 'platform-specification')), { code: 'EBADPLATFORM' }))
+
 t.test('dry run, do not get anything wet', async t => {
   const cases = [
     'shrinkwrapped-dep-with-lock-empty',