From 752ce39e0de09df42f17dc982b18f91c8130e613 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 10 Dec 2019 23:17:41 +0100 Subject: [PATCH 1/6] Fixes potential file overwrite at install time --- src/util/normalize-manifest/fix.js | 20 +++++++++++++++++++- src/util/normalize-manifest/util.js | 7 +++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/util/normalize-manifest/fix.js b/src/util/normalize-manifest/fix.js index 95d19262a1..712653c517 100644 --- a/src/util/normalize-manifest/fix.js +++ b/src/util/normalize-manifest/fix.js @@ -2,7 +2,7 @@ import {MANIFEST_FIELDS} from '../../constants'; import type {Reporter} from '../../reporters/index.js'; -import {isValidLicense} from './util.js'; +import {isValidBin, isValidLicense} from './util.js'; import {normalizePerson, extractDescription} from './util.js'; import {hostedGitFragmentToGitUrl} from '../../resolvers/index.js'; import inferLicense from './infer-license.js'; @@ -12,6 +12,8 @@ const semver = require('semver'); const path = require('path'); const url = require('url'); +const VALID_BIN_KEYS = /^[a-z0-9_-]+$/i; + const LICENSE_RENAMES: {[key: string]: ?string} = { 'MIT/X11': 'MIT', X11: 'MIT', @@ -157,6 +159,22 @@ export default (async function( // Remove scoped package name for consistency with NPM's bin field fixing behaviour const name = info.name.replace(/^@[^\/]+\//, ''); info.bin = {[name]: info.bin}; + } else { + delete info.bin; + } + + // Validate that the bin entries reference only files within their package, and that + // their name is a valid file name + if (typeof info.bin === 'object' && info.bin !== null) { + const bin: Object = info.bin; + for (const key of Object.keys(bin)) { + const target = bin[key]; + if (!VALID_BIN_KEYS.test(key) || !isValidBin(target)) { + delete bin[key]; + } else { + bin[key] = path.normalize(target); + } + } } // bundleDependencies is an alias for bundledDependencies diff --git a/src/util/normalize-manifest/util.js b/src/util/normalize-manifest/util.js index 73f904882c..ba04ed4bf8 100644 --- a/src/util/normalize-manifest/util.js +++ b/src/util/normalize-manifest/util.js @@ -2,12 +2,19 @@ import type {PersonObject} from '../../types.js'; +const path = require('path'); const validateLicense = require('validate-npm-package-license'); +const PARENT_PATH = /^\.\.([\///]|$)/g; + export function isValidLicense(license: string): boolean { return !!license && validateLicense(license).validForNewPackages; } +export function isValidBin(bin: string): boolean { + return !path.isAbsolute(bin) && !PARENT_PATH.test(path.normalize(bin)); +} + export function stringifyPerson(person: mixed): any { if (!person || typeof person !== 'object') { return person; From 35a884ec448b4cad193feb08aa9ff20e7397894d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Wed, 11 Dec 2019 08:09:36 +0100 Subject: [PATCH 2/6] Fixes the regexp --- src/util/normalize-manifest/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/normalize-manifest/util.js b/src/util/normalize-manifest/util.js index ba04ed4bf8..3dcc40dfd5 100644 --- a/src/util/normalize-manifest/util.js +++ b/src/util/normalize-manifest/util.js @@ -5,7 +5,7 @@ import type {PersonObject} from '../../types.js'; const path = require('path'); const validateLicense = require('validate-npm-package-license'); -const PARENT_PATH = /^\.\.([\///]|$)/g; +const PARENT_PATH = /^\.\.([\\\/]|$)/g; export function isValidLicense(license: string): boolean { return !!license && validateLicense(license).validForNewPackages; From ef69693037865be3389ac470de8a4891ec4faf18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Wed, 11 Dec 2019 08:19:18 +0100 Subject: [PATCH 3/6] Adds warnings --- src/reporters/lang/en.js | 3 ++- src/util/normalize-manifest/fix.js | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js index 100c1692e9..5175666d90 100644 --- a/src/reporters/lang/en.js +++ b/src/reporters/lang/en.js @@ -26,7 +26,8 @@ const messages = { couldntClearPackageFromCache: "Couldn't clear package $0 from cache", clearedPackageFromCache: 'Cleared package $0 from cache', packWroteTarball: 'Wrote tarball to $0.', - + invalidBinField: 'Invalid bin field for $0.', + invalidBinEntry: 'Invalid bin entry for $1 (in $0).', helpExamples: ' Examples:\n$0\n', helpCommands: ' Commands:\n$0\n', helpCommandsMore: ' Run `$0` for more information on specific commands.', diff --git a/src/util/normalize-manifest/fix.js b/src/util/normalize-manifest/fix.js index 712653c517..3799d45e72 100644 --- a/src/util/normalize-manifest/fix.js +++ b/src/util/normalize-manifest/fix.js @@ -171,10 +171,14 @@ export default (async function( const target = bin[key]; if (!VALID_BIN_KEYS.test(key) || !isValidBin(target)) { delete bin[key]; + warn(reporter.lang('invalidBinEntry', info.name)); } else { bin[key] = path.normalize(target); } } + } else { + delete info.bin; + warn(reporter.lang('invalidBinField', info.name)); } // bundleDependencies is an alias for bundledDependencies From 85d8d79892e967f6529716a05cb4a9bc9769f811 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Wed, 11 Dec 2019 08:23:53 +0100 Subject: [PATCH 4/6] Fixes overzealous removals --- src/util/normalize-manifest/fix.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/normalize-manifest/fix.js b/src/util/normalize-manifest/fix.js index 3799d45e72..7d9796dee3 100644 --- a/src/util/normalize-manifest/fix.js +++ b/src/util/normalize-manifest/fix.js @@ -159,8 +159,6 @@ export default (async function( // Remove scoped package name for consistency with NPM's bin field fixing behaviour const name = info.name.replace(/^@[^\/]+\//, ''); info.bin = {[name]: info.bin}; - } else { - delete info.bin; } // Validate that the bin entries reference only files within their package, and that @@ -171,12 +169,12 @@ export default (async function( const target = bin[key]; if (!VALID_BIN_KEYS.test(key) || !isValidBin(target)) { delete bin[key]; - warn(reporter.lang('invalidBinEntry', info.name)); + warn(reporter.lang('invalidBinEntry', info.name, key)); } else { bin[key] = path.normalize(target); } } - } else { + } else if (typeof info.bin !== 'undefined') { delete info.bin; warn(reporter.lang('invalidBinField', info.name)); } From e4a752e8f7bb187b577b80a1bcb09d48cbc30e23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Wed, 11 Dec 2019 08:32:45 +0100 Subject: [PATCH 5/6] Fixes test --- __tests__/__snapshots__/normalize-manifest.js.snap | 1 + .../fixtures/normalize-manifest/empty bin string/expected.json | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/__tests__/__snapshots__/normalize-manifest.js.snap b/__tests__/__snapshots__/normalize-manifest.js.snap index c5a78707da..eedc1c6e56 100644 --- a/__tests__/__snapshots__/normalize-manifest.js.snap +++ b/__tests__/__snapshots__/normalize-manifest.js.snap @@ -92,6 +92,7 @@ Array [ exports[`empty bin string: empty bin string 1`] = ` Array [ + "foo: Invalid bin field for \\"foo\\".", "foo: No license field", ] `; diff --git a/__tests__/fixtures/normalize-manifest/empty bin string/expected.json b/__tests__/fixtures/normalize-manifest/empty bin string/expected.json index 7c894d5fd8..15a2224ef8 100644 --- a/__tests__/fixtures/normalize-manifest/empty bin string/expected.json +++ b/__tests__/fixtures/normalize-manifest/empty bin string/expected.json @@ -1,5 +1,4 @@ { "name": "foo", - "version": "", - "bin": "" + "version": "" } From 8cd85c9c463fb75df0621fc256126dca169bdc3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Wed, 11 Dec 2019 09:08:32 +0100 Subject: [PATCH 6/6] Adds tests --- .../__snapshots__/normalize-manifest.js.snap | 18 ++++++++++++++++++ .../dangerous bin name/actual.json | 9 +++++++++ .../dangerous bin name/expected.json | 5 +++++ .../dangerous bin values/actual.json | 9 +++++++++ .../dangerous bin values/expected.json | 5 +++++ src/util/normalize-manifest/util.js | 2 +- 6 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 __tests__/fixtures/normalize-manifest/dangerous bin name/actual.json create mode 100644 __tests__/fixtures/normalize-manifest/dangerous bin name/expected.json create mode 100644 __tests__/fixtures/normalize-manifest/dangerous bin values/actual.json create mode 100644 __tests__/fixtures/normalize-manifest/dangerous bin values/expected.json diff --git a/__tests__/__snapshots__/normalize-manifest.js.snap b/__tests__/__snapshots__/normalize-manifest.js.snap index eedc1c6e56..eb903b98d4 100644 --- a/__tests__/__snapshots__/normalize-manifest.js.snap +++ b/__tests__/__snapshots__/normalize-manifest.js.snap @@ -60,6 +60,24 @@ Array [ ] `; +exports[`dangerous bin name: dangerous bin name 1`] = ` +Array [ + "foo: Invalid bin entry for \\"/tmp/foo\\" (in \\"foo\\").", + "foo: Invalid bin entry for \\"../tmp/foo\\" (in \\"foo\\").", + "foo: Invalid bin entry for \\"tmp/../../foo\\" (in \\"foo\\").", + "foo: No license field", +] +`; + +exports[`dangerous bin values: dangerous bin values 1`] = ` +Array [ + "foo: Invalid bin entry for \\"foo\\" (in \\"foo\\").", + "foo: Invalid bin entry for \\"bar\\" (in \\"foo\\").", + "foo: Invalid bin entry for \\"baz\\" (in \\"foo\\").", + "foo: No license field", +] +`; + exports[`dedupe all trivial dependencies: dedupe all trivial dependencies 1`] = ` Array [ "No license field", diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin name/actual.json b/__tests__/fixtures/normalize-manifest/dangerous bin name/actual.json new file mode 100644 index 0000000000..d70ea69e80 --- /dev/null +++ b/__tests__/fixtures/normalize-manifest/dangerous bin name/actual.json @@ -0,0 +1,9 @@ +{ + "name": "foo", + "version": "", + "bin": { + "/tmp/foo": "main.js", + "../tmp/foo": "main.js", + "tmp/../../foo": "main.js" + } +} diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json b/__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json new file mode 100644 index 0000000000..80ce110c76 --- /dev/null +++ b/__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json @@ -0,0 +1,5 @@ +{ + "name": "foo", + "version": "", + "bin": {} +} diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin values/actual.json b/__tests__/fixtures/normalize-manifest/dangerous bin values/actual.json new file mode 100644 index 0000000000..a8e335abb3 --- /dev/null +++ b/__tests__/fixtures/normalize-manifest/dangerous bin values/actual.json @@ -0,0 +1,9 @@ +{ + "name": "foo", + "version": "", + "bin": { + "foo": "../../../../../foo", + "bar": "/hello/world", + "baz": "./foo/bar/../../../../../../foo" + } +} diff --git a/__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json b/__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json new file mode 100644 index 0000000000..80ce110c76 --- /dev/null +++ b/__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json @@ -0,0 +1,5 @@ +{ + "name": "foo", + "version": "", + "bin": {} +} diff --git a/src/util/normalize-manifest/util.js b/src/util/normalize-manifest/util.js index 3dcc40dfd5..7d30f6106b 100644 --- a/src/util/normalize-manifest/util.js +++ b/src/util/normalize-manifest/util.js @@ -5,7 +5,7 @@ import type {PersonObject} from '../../types.js'; const path = require('path'); const validateLicense = require('validate-npm-package-license'); -const PARENT_PATH = /^\.\.([\\\/]|$)/g; +const PARENT_PATH = /^\.\.([\\\/]|$)/; export function isValidLicense(license: string): boolean { return !!license && validateLicense(license).validForNewPackages;