From eed240be1c4c0d10bacd17a266bd21f8c2ae6a60 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 23 Jun 2016 15:20:04 -0700 Subject: [PATCH 1/5] Add specs for Module._nodeModulesPath --- spec/modules-spec.js | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/spec/modules-spec.js b/spec/modules-spec.js index fbc66a0825123..c3ad4bc705154 100644 --- a/spec/modules-spec.js +++ b/spec/modules-spec.js @@ -1,4 +1,5 @@ const assert = require('assert') +const Module = require('module') const path = require('path') const temp = require('temp') @@ -47,3 +48,50 @@ describe('third-party module', function () { }) }) }) + +describe('Module._nodeModulePaths', function () { + describe('when the path is inside the resources path', function () { + it('does not include paths outside of the resources path', function () { + let modulePath = process.resourcesPath + assert.deepEqual(Module._nodeModulePaths(modulePath), [ + path.join(process.resourcesPath, 'node_modules') + ]) + + modulePath = path.join(process.resourcesPath, 'foo') + assert.deepEqual(Module._nodeModulePaths(modulePath), [ + path.join(process.resourcesPath, 'foo', 'node_modules'), + path.join(process.resourcesPath, 'node_modules') + ]) + + modulePath = path.join(process.resourcesPath, 'node_modules', 'foo') + assert.deepEqual(Module._nodeModulePaths(modulePath), [ + path.join(process.resourcesPath, 'node_modules', 'foo', 'node_modules'), + path.join(process.resourcesPath, 'node_modules') + ]) + + modulePath = path.join(process.resourcesPath, 'node_modules', 'foo', 'bar') + assert.deepEqual(Module._nodeModulePaths(modulePath), [ + path.join(process.resourcesPath, 'node_modules', 'foo', 'bar', 'node_modules'), + path.join(process.resourcesPath, 'node_modules', 'foo', 'node_modules'), + path.join(process.resourcesPath, 'node_modules') + ]) + + modulePath = path.join(process.resourcesPath, 'node_modules', 'foo', 'node_modules', 'bar') + assert.deepEqual(Module._nodeModulePaths(modulePath), [ + path.join(process.resourcesPath, 'node_modules', 'foo', 'node_modules', 'bar', 'node_modules'), + path.join(process.resourcesPath, 'node_modules', 'foo', 'node_modules'), + path.join(process.resourcesPath, 'node_modules') + ]) + }) + }) + + describe('when the path is outside the resources path', function () { + it('includes paths outside of the resources path', function () { + let modulePath = path.resolve('/foo') + assert.deepEqual(Module._nodeModulePaths(modulePath), [ + path.join(modulePath, 'node_modules'), + path.join('node_modules') + ]) + }) + }) +}) From b273b70eee6a15808faad6f3fd5ad5d85d36e4d4 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 23 Jun 2016 15:27:45 -0700 Subject: [PATCH 2/5] Filter existing search paths instead reimplementing --- lib/common/reset-search-paths.js | 29 +++++++++-------------------- spec/modules-spec.js | 3 +-- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/lib/common/reset-search-paths.js b/lib/common/reset-search-paths.js index 4d09f6c89ca6b..8e217012f5829 100644 --- a/lib/common/reset-search-paths.js +++ b/lib/common/reset-search-paths.js @@ -9,30 +9,19 @@ module.paths = [] module.parent.paths = [] // Prevent Node from adding paths outside this app to search paths. +const originalNodeModulePaths = Module._nodeModulePaths Module._nodeModulePaths = function (from) { - from = path.resolve(from) + const paths = originalNodeModulePaths(from) + const rootPath = process.resourcesPath // If "from" is outside the app then we do nothing. - const skipOutsidePaths = from.startsWith(process.resourcesPath) - - // Following logic is copied from module.js. - const splitRe = process.platform === 'win32' ? /[\/\\]/ : /\// - const paths = [] - const parts = from.split(splitRe) - - let tip - let i - for (tip = i = parts.length - 1; i >= 0; tip = i += -1) { - const part = parts[tip] - if (part === 'node_modules') { - continue - } - const dir = parts.slice(0, tip + 1).join(path.sep) - if (skipOutsidePaths && !dir.startsWith(process.resourcesPath)) { - break - } - paths.push(path.join(dir, 'node_modules')) + const skipOutsidePaths = path.resolve(from).startsWith(rootPath) + if (skipOutsidePaths) { + return paths.filter(function (candidate) { + return candidate.startsWith(rootPath) + }) } + return paths } diff --git a/spec/modules-spec.js b/spec/modules-spec.js index c3ad4bc705154..87396ac764f85 100644 --- a/spec/modules-spec.js +++ b/spec/modules-spec.js @@ -89,8 +89,7 @@ describe('Module._nodeModulePaths', function () { it('includes paths outside of the resources path', function () { let modulePath = path.resolve('/foo') assert.deepEqual(Module._nodeModulePaths(modulePath), [ - path.join(modulePath, 'node_modules'), - path.join('node_modules') + path.join(modulePath, 'node_modules') ]) }) }) From c6906deef20ca3631862b84d1556cfcf8e4479de Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 23 Jun 2016 15:39:21 -0700 Subject: [PATCH 3/5] Add failing spec for trailing separator bug --- spec/modules-spec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/modules-spec.js b/spec/modules-spec.js index 87396ac764f85..f2c45b0a71b46 100644 --- a/spec/modules-spec.js +++ b/spec/modules-spec.js @@ -57,6 +57,11 @@ describe('Module._nodeModulePaths', function () { path.join(process.resourcesPath, 'node_modules') ]) + modulePath = process.resourcesPath + '-foo' + let nodeModulePaths = Module._nodeModulePaths(modulePath) + assert(nodeModulePaths.includes(path.join(modulePath, 'node_modules'))) + assert(nodeModulePaths.includes(path.join(modulePath, '..', 'node_modules'))) + modulePath = path.join(process.resourcesPath, 'foo') assert.deepEqual(Module._nodeModulePaths(modulePath), [ path.join(process.resourcesPath, 'foo', 'node_modules'), From 905e9e96451e84fab6203ed8d122cae3ed8e8d1e Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 23 Jun 2016 15:45:08 -0700 Subject: [PATCH 4/5] Include trailing separator in comparisons --- lib/common/reset-search-paths.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/common/reset-search-paths.js b/lib/common/reset-search-paths.js index 8e217012f5829..5cec77b606aa3 100644 --- a/lib/common/reset-search-paths.js +++ b/lib/common/reset-search-paths.js @@ -12,10 +12,11 @@ module.parent.paths = [] const originalNodeModulePaths = Module._nodeModulePaths Module._nodeModulePaths = function (from) { const paths = originalNodeModulePaths(from) - const rootPath = process.resourcesPath // If "from" is outside the app then we do nothing. - const skipOutsidePaths = path.resolve(from).startsWith(rootPath) + const rootPath = process.resourcesPath + path.sep + const fromPath = path.resolve(from) + path.sep + const skipOutsidePaths = fromPath.startsWith(rootPath) if (skipOutsidePaths) { return paths.filter(function (candidate) { return candidate.startsWith(rootPath) From bac4d51169b5b8167e90b60bd8107324de459186 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 23 Jun 2016 15:56:29 -0700 Subject: [PATCH 5/5] Reuse root path variable --- lib/common/reset-search-paths.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/common/reset-search-paths.js b/lib/common/reset-search-paths.js index 5cec77b606aa3..924cb388d4d9f 100644 --- a/lib/common/reset-search-paths.js +++ b/lib/common/reset-search-paths.js @@ -9,21 +9,19 @@ module.paths = [] module.parent.paths = [] // Prevent Node from adding paths outside this app to search paths. +const resourcesPathWithTrailingSlash = process.resourcesPath + path.sep const originalNodeModulePaths = Module._nodeModulePaths Module._nodeModulePaths = function (from) { const paths = originalNodeModulePaths(from) - - // If "from" is outside the app then we do nothing. - const rootPath = process.resourcesPath + path.sep const fromPath = path.resolve(from) + path.sep - const skipOutsidePaths = fromPath.startsWith(rootPath) - if (skipOutsidePaths) { + // If "from" is outside the app then we do nothing. + if (fromPath.startsWith(resourcesPathWithTrailingSlash)) { return paths.filter(function (candidate) { - return candidate.startsWith(rootPath) + return candidate.startsWith(resourcesPathWithTrailingSlash) }) + } else { + return paths } - - return paths } // Patch Module._resolveFilename to always require the Electron API when