Fixes bin overwrites (yarnpkg#7755)

* Fixes potential file overwrite at install time

* Fixes the regexp

* Adds warnings

* Fixes overzealous removals

* Fixes test

* Adds tests

__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",
@@ -92,6 +110,7 @@ Array [
 
 exports[`empty bin string: empty bin string 1`] = `
 Array [
+  "foo: Invalid bin field for \\"foo\\".",
   "foo: No license field",
 ]
 `;

__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"
+    }
+}

__tests__/fixtures/normalize-manifest/dangerous bin name/expected.json

@@ -0,0 +1,5 @@
+{
+    "name": "foo",
+    "version": "",
+    "bin": {}
+}

__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"
+    }
+}

__tests__/fixtures/normalize-manifest/dangerous bin values/expected.json

@@ -0,0 +1,5 @@
+{
+    "name": "foo",
+    "version": "",
+    "bin": {}
+}

__tests__/fixtures/normalize-manifest/empty bin string/expected.json

@@ -1,5 +1,4 @@
 {
   "name": "foo",
-  "version": "",
-  "bin": ""
+  "version": ""
 }

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.',

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',
@@ -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;

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 = /^\.\.([\\\/]|$)/;
+
 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;