Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes bin overwrites #7755

Merged
merged 6 commits into from
Dec 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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