Skip to content

Commit

Permalink
Fixes bin overwrites (#7755)
Browse files Browse the repository at this point in the history
* Fixes potential file overwrite at install time

* Fixes the regexp

* Adds warnings

* Fixes overzealous removals

* Fixes test

* Adds tests
  • Loading branch information
arcanis authored Dec 11, 2019
1 parent ac04da8 commit 039bafd
Show file tree
Hide file tree
Showing 9 changed files with 78 additions and 4 deletions.
19 changes: 19 additions & 0 deletions __tests__/__snapshots__/normalize-manifest.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -92,6 +110,7 @@ Array [

exports[`empty bin string: empty bin string 1`] = `
Array [
"foo: Invalid bin field for \\"foo\\".",
"foo: No license field",
]
`;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "foo",
"version": "",
"bin": {
"/tmp/foo": "main.js",
"../tmp/foo": "main.js",
"tmp/../../foo": "main.js"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "foo",
"version": "",
"bin": {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "foo",
"version": "",
"bin": {
"foo": "../../../../../foo",
"bar": "/hello/world",
"baz": "./foo/bar/../../../../../../foo"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "foo",
"version": "",
"bin": {}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{
"name": "foo",
"version": "",
"bin": ""
"version": ""
}
3 changes: 2 additions & 1 deletion src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
Expand Down
22 changes: 21 additions & 1 deletion src/util/normalize-manifest/fix.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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',
Expand Down Expand Up @@ -159,6 +161,24 @@ export default (async function(
info.bin = {[name]: 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];
warn(reporter.lang('invalidBinEntry', info.name, key));
} else {
bin[key] = path.normalize(target);
}
}
} else if (typeof info.bin !== 'undefined') {
delete info.bin;
warn(reporter.lang('invalidBinField', info.name));
}

// bundleDependencies is an alias for bundledDependencies
if (info.bundledDependencies) {
info.bundleDependencies = info.bundledDependencies;
Expand Down
7 changes: 7 additions & 0 deletions src/util/normalize-manifest/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@

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

const path = require('path');
const validateLicense = require('validate-npm-package-license');

const PARENT_PATH = /^\.\.([\\\/]|$)/;

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;
Expand Down

0 comments on commit 039bafd

Please sign in to comment.