Skip to content

Commit 039bafd

Browse files
authored
Fixes bin overwrites (#7755)
* Fixes potential file overwrite at install time * Fixes the regexp * Adds warnings * Fixes overzealous removals * Fixes test * Adds tests
1 parent ac04da8 commit 039bafd

File tree

9 files changed

+78
-4
lines changed

9 files changed

+78
-4
lines changed

__tests__/__snapshots__/normalize-manifest.js.snap

+19
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,24 @@ Array [
6060
]
6161
`;
6262

63+
exports[`dangerous bin name: dangerous bin name 1`] = `
64+
Array [
65+
"foo: Invalid bin entry for \\"/tmp/foo\\" (in \\"foo\\").",
66+
"foo: Invalid bin entry for \\"../tmp/foo\\" (in \\"foo\\").",
67+
"foo: Invalid bin entry for \\"tmp/../../foo\\" (in \\"foo\\").",
68+
"foo: No license field",
69+
]
70+
`;
71+
72+
exports[`dangerous bin values: dangerous bin values 1`] = `
73+
Array [
74+
"foo: Invalid bin entry for \\"foo\\" (in \\"foo\\").",
75+
"foo: Invalid bin entry for \\"bar\\" (in \\"foo\\").",
76+
"foo: Invalid bin entry for \\"baz\\" (in \\"foo\\").",
77+
"foo: No license field",
78+
]
79+
`;
80+
6381
exports[`dedupe all trivial dependencies: dedupe all trivial dependencies 1`] = `
6482
Array [
6583
"No license field",
@@ -92,6 +110,7 @@ Array [
92110

93111
exports[`empty bin string: empty bin string 1`] = `
94112
Array [
113+
"foo: Invalid bin field for \\"foo\\".",
95114
"foo: No license field",
96115
]
97116
`;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"name": "foo",
3+
"version": "",
4+
"bin": {
5+
"/tmp/foo": "main.js",
6+
"../tmp/foo": "main.js",
7+
"tmp/../../foo": "main.js"
8+
}
9+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"name": "foo",
3+
"version": "",
4+
"bin": {}
5+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"name": "foo",
3+
"version": "",
4+
"bin": {
5+
"foo": "../../../../../foo",
6+
"bar": "/hello/world",
7+
"baz": "./foo/bar/../../../../../../foo"
8+
}
9+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"name": "foo",
3+
"version": "",
4+
"bin": {}
5+
}
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
{
22
"name": "foo",
3-
"version": "",
4-
"bin": ""
3+
"version": ""
54
}

src/reporters/lang/en.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ const messages = {
2626
couldntClearPackageFromCache: "Couldn't clear package $0 from cache",
2727
clearedPackageFromCache: 'Cleared package $0 from cache',
2828
packWroteTarball: 'Wrote tarball to $0.',
29-
29+
invalidBinField: 'Invalid bin field for $0.',
30+
invalidBinEntry: 'Invalid bin entry for $1 (in $0).',
3031
helpExamples: ' Examples:\n$0\n',
3132
helpCommands: ' Commands:\n$0\n',
3233
helpCommandsMore: ' Run `$0` for more information on specific commands.',

src/util/normalize-manifest/fix.js

+21-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import {MANIFEST_FIELDS} from '../../constants';
44
import type {Reporter} from '../../reporters/index.js';
5-
import {isValidLicense} from './util.js';
5+
import {isValidBin, isValidLicense} from './util.js';
66
import {normalizePerson, extractDescription} from './util.js';
77
import {hostedGitFragmentToGitUrl} from '../../resolvers/index.js';
88
import inferLicense from './infer-license.js';
@@ -12,6 +12,8 @@ const semver = require('semver');
1212
const path = require('path');
1313
const url = require('url');
1414

15+
const VALID_BIN_KEYS = /^[a-z0-9_-]+$/i;
16+
1517
const LICENSE_RENAMES: {[key: string]: ?string} = {
1618
'MIT/X11': 'MIT',
1719
X11: 'MIT',
@@ -159,6 +161,24 @@ export default (async function(
159161
info.bin = {[name]: info.bin};
160162
}
161163

164+
// Validate that the bin entries reference only files within their package, and that
165+
// their name is a valid file name
166+
if (typeof info.bin === 'object' && info.bin !== null) {
167+
const bin: Object = info.bin;
168+
for (const key of Object.keys(bin)) {
169+
const target = bin[key];
170+
if (!VALID_BIN_KEYS.test(key) || !isValidBin(target)) {
171+
delete bin[key];
172+
warn(reporter.lang('invalidBinEntry', info.name, key));
173+
} else {
174+
bin[key] = path.normalize(target);
175+
}
176+
}
177+
} else if (typeof info.bin !== 'undefined') {
178+
delete info.bin;
179+
warn(reporter.lang('invalidBinField', info.name));
180+
}
181+
162182
// bundleDependencies is an alias for bundledDependencies
163183
if (info.bundledDependencies) {
164184
info.bundleDependencies = info.bundledDependencies;

src/util/normalize-manifest/util.js

+7
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,19 @@
22

33
import type {PersonObject} from '../../types.js';
44

5+
const path = require('path');
56
const validateLicense = require('validate-npm-package-license');
67

8+
const PARENT_PATH = /^\.\.([\\\/]|$)/;
9+
710
export function isValidLicense(license: string): boolean {
811
return !!license && validateLicense(license).validForNewPackages;
912
}
1013

14+
export function isValidBin(bin: string): boolean {
15+
return !path.isAbsolute(bin) && !PARENT_PATH.test(path.normalize(bin));
16+
}
17+
1118
export function stringifyPerson(person: mixed): any {
1219
if (!person || typeof person !== 'object') {
1320
return person;

0 commit comments

Comments
 (0)