feat: improve-remediation

This patch is a rewrite of the remediation implementation. It makes
several changes:

- Remove the reliance on `inspect`, and instead get top level deps by
  parsing the manifest file. This should be significantly quicker than a
  pip install, making the fix process much faster for larger projects
- Change the parser to attempt to better preserve package casing,
  version comparators, and extras (comments, markers, etc)
- Update tests and add new tests. In a couple of cases, this actually
  involves fixing the test fixture, as the fixture itself was making
  changes such as package name casing, adding extra new lines to the
  manifest etc.

.gitignore

@@ -16,4 +16,7 @@ package-lock.json
 .eslintcache
 
 # Test output reports, coverage, etc
-reports/
+reports/
+
+# Generic venv directory for experimenting / testing
+venv/

lib/index.ts

@@ -4,7 +4,7 @@ import * as subProcess from './sub-process';
 import { legacyPlugin as api } from '@snyk/cli-interface';
 import { ManifestFiles, DependencyUpdates } from './types';
 import { getMetaData, getDependencies } from './inspect-implementation';
-import { applyUpgrades } from './apply-remediation-implementation';
+import { parseRequirementsFile } from './requirements-file-parser';
 
 export interface PythonInspectOptions {
   command?: string; // `python` command override
@@ -55,17 +55,16 @@ export async function inspect(
   return { plugin, package: pkg };
 }
 
-// Given contents of manifest file(s) and a set of upgrades, and assuming that all the packages
-// were installed (e.g. using `pip install`), produce the updated manifests by detecting the
-// provenance of top-level packages and replacing their specifications and adding "pinned" packages
-// to the manifest.
-// Currently only supported for `requirements.txt` - at least one file named `**/requirements.txt`
-// must be in the manifests.
-export async function applyRemediationToManifests(
-  root: string,
+/**
+ * Given contents of manifest file(s) and a set of upgrades, apply the given
+ * upgrades to a manifest and return the upgraded manifest.
+ *
+ * Currently only supported for `requirements.txt` - at least one file named
+ * `requirements.txt` must be in the manifests.
+ **/
+export function applyRemediationToManifests(
   manifests: ManifestFiles,
-  upgrades: DependencyUpdates,
-  options: Options
+  upgrades: DependencyUpdates
 ) {
   const manifestNames = Object.keys(manifests);
   const targetFile = manifestNames.find(
@@ -78,18 +77,94 @@ export async function applyRemediationToManifests(
     throw new Error('Remediation only supported for requirements.txt file');
   }
 
-  // Calculate provenance via Python code.
-  // This currently requires costly setup of a virtual environment, when
-  // called from pip-deps.
-  // Alternative approaches to consider:
-  // - modify python code to not require installed packages in this case
-  // - replicate the parser of requirements.txt in JS code (in pip-deps?)
-  const provOptions = { ...options };
-  provOptions.args = provOptions.args || [];
-  provOptions.args.push('--only-provenance');
-  const topLevelDeps = (await inspect(root, targetFile, provOptions)).package;
+  if (Object.keys(upgrades).length === 0) {
+    return manifests;
+  }
+
+  const requirementsFileName = Object.keys(manifests).find(
+    (fn) => path.basename(fn) === 'requirements.txt'
+  );
+
+  // If there is no usable manifest, return
+  if (typeof requirementsFileName === 'undefined') {
+    return manifests;
+  }
+
+  const requirementsFile = manifests[requirementsFileName];
+
+  const requirements = parseRequirementsFile(requirementsFile);
+
+  // This is a bit of a hack, but an easy one to follow. If a file ends with a
+  // new line, ensure we keep it this way. Don't hijack customers formatting.
+  let endsWithNewLine = false;
+  if (requirements[requirements.length - 1].originalText === '\n') {
+    endsWithNewLine = true;
+  }
+
+  const topLevelDeps = requirements
+    .map(({ name }) => name && name.toLowerCase())
+    .filter(isDefined);
+
+  // Lowercase the upgrades object. This might be overly defensive, given that
+  // we control this input internally, but its a low cost guard rail. Outputs a
+  // mapping of upgrade to -> from, instead of the nested upgradeTo object.
+  const lowerCasedUpgrades: { [upgradeFrom: string]: string } = {};
+  Object.keys(upgrades).forEach((upgrade) => {
+    const { upgradeTo } = upgrades[upgrade];
+    lowerCasedUpgrades[upgrade.toLowerCase()] = upgradeTo.toLowerCase();
+  });
+
+  const updatedRequirements: string[] = requirements.map(
+    ({ name, versionComparator, version, originalText, extras }) => {
+      // Defensive patching; if any of these are undefined, return
+      if (
+        typeof name === 'undefined' ||
+        typeof versionComparator === 'undefined' ||
+        typeof version === 'undefined'
+      ) {
+        return originalText;
+      }
 
-  applyUpgrades(manifests, upgrades, topLevelDeps);
+      // Check if we have an upgrade; if we do, replace the version string with
+      // the upgrade, but keep the rest of the content
+      const upgrade = lowerCasedUpgrades[`${name.toLowerCase()}@${version}`];
+
+      if (!upgrade) {
+        return originalText;
+      }
+
+      const newVersion = upgrade.split('@')[1];
+      return `${name}${versionComparator}${newVersion}${extras ? extras : ''}`;
+    }
+  );
+
+  const pinnedRequirements = Object.keys(lowerCasedUpgrades)
+    .map((pkgNameAtVersion) => {
+      const pkgName = pkgNameAtVersion.split('@')[0];
+
+      // Pinning is only for non top level deps
+      if (topLevelDeps.indexOf(pkgName) >= 0) {
+        return;
+      }
+
+      const version = lowerCasedUpgrades[pkgNameAtVersion].split('@')[1];
+      return `${pkgName}>=${version} # not directly required, pinned by Snyk to avoid a vulnerability`;
+    })
+    .filter(isDefined);
+
+  let updatedManifest = [...updatedRequirements, ...pinnedRequirements].join(
+    '\n'
+  );
+
+  if (endsWithNewLine) {
+    updatedManifest += '\n';
+  }
+
+  return { [requirementsFileName]: updatedManifest };
+}
 
-  return manifests;
+// TS is not capable of determining when Array.filter has removed undefined
+// values without a manual Type Guard, so thats what this does
+function isDefined<T>(t: T | undefined): t is T {
+  return typeof t !== 'undefined';
 }

lib/requirements-file-parser.ts

@@ -0,0 +1,50 @@
+type VersionComparator = '<' | '<=' | '!=' | '==' | '>=' | '>' | '~=';
+
+interface Requirement {
+  originalText: string;
+  line: number;
+  name?: string;
+  versionComparator?: VersionComparator;
+  version?: string;
+  extras?: string;
+}
+
+/**
+ * Converts a requirements file into an array of parsed requirements, with data
+ * such as name, version, etc.
+ * @param requirementsFile A requirements.txt file as a string
+ */
+export function parseRequirementsFile(requirementsFile: string): Requirement[] {
+  return requirementsFile.split('\n').map((requirementText, line) => {
+    const requirement: Requirement = { originalText: requirementText, line };
+    const trimmedText = requirementText.trim();
+
+    // Quick returns for cases we cannot remediate
+    // - Empty line i.e. ''
+    // - 'editable' packages i.e. '-e git://git.myproject.org/MyProject.git#egg=MyProject'
+    // - Comments i.e. # This is a comment
+    // - Local files i.e. file:../../lib/project#egg=MyProject
+    if (
+      requirementText === '' ||
+      trimmedText.startsWith('-e') ||
+      trimmedText.startsWith('#') ||
+      trimmedText.startsWith('file:')
+    ) {
+      return requirement;
+    }
+
+    // Regex to match against a Python package specifier. Any invalid lines (or
+    // lines we can't handle) should have been returned this point.
+    const regex = /([A-Z0-9]*)(===|==|>=|<=|>|<|~=)(\d\.?\d?\.?\d?)(.*)/i;
+    const result = regex.exec(requirementText);
+
+    if (result !== null) {
+      requirement.name = result[1];
+      requirement.versionComparator = result[2] as VersionComparator;
+      requirement.version = result[3];
+      requirement.extras = result[4];
+    }
+
+    return requirement;
+  });
+}

lib/types.ts

@@ -1,10 +1,7 @@
-export interface UpgradeRemediation {
-  upgradeTo: string;
-  // Other fields are of no interest
-}
-
 export interface DependencyUpdates {
-  [from: string]: UpgradeRemediation;
+  [from: string]: {
+    upgradeTo: string;
+  };
 }
 
 export interface ManifestFiles {

test/fixtures/updated-manifest/requirements.txt

@@ -1,9 +1,9 @@
 Jinja2==2.7.2
-django==2.0.1
+Django==2.0.1
 python-etcd==0.4.5
 Django-Select2==6.0.1 # this version installs with lowercase so it catches a previous bug in pip_resolve.py
 irc==16.2 # this has a cyclic dependecy (interanl jaraco.text <==> jaraco.collections)
 testtools==\
     2.3.0 # this has a cycle (fixtures ==> testtols);
 ./packages/prometheus_client-0.6.0
-transitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability
+transitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability