feat: improve-remediation

.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