diff --git a/.gitignore b/.gitignore index b6a0fa19..d9869bb8 100644 --- a/.gitignore +++ b/.gitignore @@ -16,4 +16,7 @@ package-lock.json .eslintcache # Test output reports, coverage, etc -reports/ \ No newline at end of file +reports/ + +# Generic venv directory for experimenting / testing +venv/ diff --git a/lib/apply-remediation-implementation.ts b/lib/apply-remediation-implementation.ts deleted file mode 100644 index bf0ec511..00000000 --- a/lib/apply-remediation-implementation.ts +++ /dev/null @@ -1,90 +0,0 @@ -import * as path from 'path'; - -import { ManifestFiles, DependencyUpdates } from './types'; -import { legacyCommon } from '@snyk/cli-interface'; - -export function applyUpgrades( - manifests: ManifestFiles, - upgrades: DependencyUpdates, - topLevelDeps: legacyCommon.DepTree -) { - const requirementsFileName = Object.keys(manifests).find( - (fn) => path.basename(fn) === 'requirements.txt' - ) as string; - - // Updates to requirements.txt - const patch: { [zeroBasedIndex: number]: string | false } = {}; // false means remove the line - const append: string[] = []; - - const originalRequirementsLines = manifests[requirementsFileName].split('\n'); - - const extraMarkers = /--| \[|;/; - - for (const upgradeFrom of Object.keys(upgrades)) { - const pkgName = upgradeFrom.split('@')[0].toLowerCase(); - const newVersion = upgrades[upgradeFrom].upgradeTo.split('@')[1]; - const topLevelDep = (topLevelDeps.dependencies || {})[ - pkgName - ] as legacyCommon.DepTreeDep; - if (topLevelDep && topLevelDep.labels && topLevelDep.labels.provenance) { - // Top level dependency, to be updated in a manifest - - const lineNumbers = topLevelDep.labels.provenance - .split(':')[1] - .split('-') - .map((x) => parseInt(x)); - // TODO(kyegupov): what if the original version spec was range, e.g. >=1.0,<2.0 ? - // TODO(kyegupov): prevent downgrades - const firstLineNo = lineNumbers[0] - 1; - const lastLineNo = - lineNumbers.length > 1 ? lineNumbers[1] - 1 : lineNumbers[0] - 1; - const originalRequirementString = originalRequirementsLines - .slice(firstLineNo, lastLineNo + 1) - .join('\n') - .replace(/\\\n/g, ''); - const firstExtraMarkerPos = originalRequirementString.search( - extraMarkers - ); - if (firstExtraMarkerPos > -1) { - // maybe we should reinstate linebreaks here? - patch[lineNumbers[0] - 1] = - pkgName + - '==' + - newVersion + - ' ' + - originalRequirementString.slice(firstExtraMarkerPos).trim(); - } else { - patch[lineNumbers[0] - 1] = pkgName + '==' + newVersion; - } - if (lastLineNo > firstLineNo) { - for (let i = firstLineNo + 1; i <= lastLineNo; i++) { - patch[i - 1] = false; - } - } - } else { - // The dependency is not a top level: we are pinning a transitive. - append.push( - pkgName + - '>=' + - newVersion + - ' # not directly required, pinned by Snyk to avoid a vulnerability' - ); - } - } - const lines: string[] = []; - originalRequirementsLines.forEach((line, i) => { - if (patch[i] === false) { - return; - } - if (patch[i]) { - lines.push(patch[i] as string); - } else { - lines.push(line); - } - }); - // Drop extra trailing newlines - while (lines.length > 0 && !lines[lines.length - 1]) { - lines.pop(); - } - manifests[requirementsFileName] = lines.concat(append).join('\n') + '\n'; -} diff --git a/lib/index.ts b/lib/index.ts index 8d146313..62a3c80b 100644 --- a/lib/index.ts +++ b/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 | undefined): t is T { + return typeof t !== 'undefined'; } diff --git a/lib/requirements-file-parser.ts b/lib/requirements-file-parser.ts new file mode 100644 index 00000000..2ef442c1 --- /dev/null +++ b/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; + }); +} diff --git a/lib/types.ts b/lib/types.ts index 6f757c36..f178adcd 100644 --- a/lib/types.ts +++ b/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 { diff --git a/test/fixtures/updated-manifest/requirements.txt b/test/fixtures/updated-manifest/requirements.txt index 7c1cd1da..a52701a5 100644 --- a/test/fixtures/updated-manifest/requirements.txt +++ b/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 \ No newline at end of file diff --git a/test/remediation.spec.ts b/test/remediation.spec.ts index 9f2d840c..c1abd166 100644 --- a/test/remediation.spec.ts +++ b/test/remediation.spec.ts @@ -1,117 +1,196 @@ -import { readdirSync, readFileSync } from 'fs'; +import { readFileSync } from 'fs'; import * as path from 'path'; import { applyRemediationToManifests } from '../lib'; -import { chdirWorkspaces, deactivateVirtualenv } from './test-utils'; -import { activateVirtualenv, pipInstall } from './test-utils'; - -function readDirAsFiles(dir: string) { - return readdirSync(dir).reduce( - (files: { [fileName: string]: string }, fileName) => { - files[fileName] = readFileSync(path.join(dir, fileName), 'utf8'); - return files; - }, - {} - ); -} describe('remediation', () => { - beforeEach(() => { - activateVirtualenv('remediation'); - }); + it('does not add extra new lines', () => { + const upgrades = { + 'django@1.6.1': { upgradeTo: 'django@2.0.1' }, + 'transitive@1.0.0': { upgradeTo: 'transitive@1.1.1' }, + }; - afterEach(() => { - deactivateVirtualenv(); - }); + const manifests = { + 'requirements.txt': 'Django==1.6.1', + }; + + const expectedManifests = { + 'requirements.txt': + 'Django==2.0.1\ntransitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability', + }; - it('fixes a pip app', async () => { - chdirWorkspaces('pip-app'); - pipInstall(); + const result = applyRemediationToManifests(manifests, upgrades); + // Note no extra newline was added to the expected manifest + expect(result).toEqual(expectedManifests); + }); + + it('ignores casing in upgrades (treats all as lowercase)', () => { const upgrades = { 'Django@1.6.1': { upgradeTo: 'Django@2.0.1' }, - 'transitive@1.0.0': { upgradeTo: 'transitive@1.1.1' }, }; const manifests = { - 'requirements.txt': readFileSync('requirements.txt', 'utf8'), + 'requirements.txt': 'django==1.6.1\n', }; - const expectedUpdatedManifests = readDirAsFiles( - '../../fixtures/updated-manifest' - ); + const expectedManifests = { + 'requirements.txt': 'django==2.0.1\n', + }; - const result = await applyRemediationToManifests( - '.', - manifests, - upgrades, - {} - ); + const result = applyRemediationToManifests(manifests, upgrades); - expect(result).toEqual(expectedUpdatedManifests); + expect(result).toEqual(expectedManifests); }); - it('retains python markers', async () => { - chdirWorkspaces('pip-app-with-python-markers'); - pipInstall(); + it('maintains package name casing when upgrading', () => { + const upgrades = { + 'django@1.6.1': { upgradeTo: 'django@2.0.1' }, + }; + const manifests = { + 'requirements.txt': 'Django==1.6.1\n', + }; + + const expectedManifests = { + 'requirements.txt': 'Django==2.0.1\n', + }; + + const result = applyRemediationToManifests(manifests, upgrades); + + expect(result).toEqual(expectedManifests); + }); + + it('maintains comments when upgrading', () => { const upgrades = { + 'django@1.6.1': { upgradeTo: 'django@2.0.1' }, + }; + + const manifests = { + 'requirements.txt': 'django==1.6.1 # this is a comment\n', + }; + + const expectedManifests = { + 'requirements.txt': 'django==2.0.1 # this is a comment\n', + }; + + const result = applyRemediationToManifests(manifests, upgrades); + + expect(result).toEqual(expectedManifests); + }); + + it('maintains version comparator when upgrading', () => { + const upgrades = { + 'django@1.6.1': { upgradeTo: 'django@2.0.1' }, 'click@7.0': { upgradeTo: 'click@7.1' }, }; const manifests = { - 'requirements.txt': readFileSync('requirements.txt', 'utf8'), + 'requirements.txt': 'django>=1.6.1\nclick>7.0', }; - const expectedUpdatedManifests = readDirAsFiles( - '../../fixtures/updated-manifests-with-python-markers' - ); + const expectedManifests = { + 'requirements.txt': 'django>=2.0.1\nclick>7.1', + }; - const result = await applyRemediationToManifests( - '.', - manifests, - upgrades, - {} - ); + const result = applyRemediationToManifests(manifests, upgrades); + + expect(result).toEqual(expectedManifests); + }); + + it('fixes a pip app', () => { + const upgrades = { + 'django@1.6.1': { upgradeTo: 'django@2.0.1' }, + 'transitive@1.0.0': { upgradeTo: 'transitive@1.1.1' }, + }; + + const manifests = { + 'requirements.txt': readFileSync( + path.resolve('test', 'workspaces', 'pip-app', 'requirements.txt'), + 'utf8' + ), + }; + + const expectedManifests = { + 'requirements.txt': readFileSync( + path.resolve( + 'test', + 'fixtures', + 'updated-manifest', + 'requirements.txt' + ), + 'utf8' + ), + }; + + const result = applyRemediationToManifests(manifests, upgrades); - expect(result).toEqual(expectedUpdatedManifests); + expect(result).toEqual(expectedManifests); }); - it('handles no-op upgrades', async () => { - chdirWorkspaces('pip-app'); - pipInstall(); + it('retains python markers', () => { + const upgrades = { + 'click@7.0': { upgradeTo: 'click@7.1' }, + }; + + const manifests = { + 'requirements.txt': readFileSync( + path.resolve( + 'test', + 'workspaces', + 'pip-app-with-python-markers', + 'requirements.txt' + ), + 'utf8' + ), + }; + + const expectedManifests = { + 'requirements.txt': readFileSync( + path.resolve( + 'test', + 'fixtures', + 'updated-manifests-with-python-markers', + 'requirements.txt' + ), + 'utf8' + ), + }; + + const result = applyRemediationToManifests(manifests, upgrades); + + expect(result).toEqual(expectedManifests); + }); + it('handles no-op upgrades', () => { const upgrades = {}; const manifests = { - 'requirements.txt': readFileSync('requirements.txt', 'utf8'), + 'requirements.txt': readFileSync( + path.resolve('test', 'workspaces', 'pip-app', 'requirements.txt'), + 'utf8' + ), }; - const result = await applyRemediationToManifests( - '.', - manifests, - upgrades, - {} - ); + const result = applyRemediationToManifests(manifests, upgrades); expect(result).toEqual(manifests); }); - it('cannot fix a Pipfile app', async () => { - chdirWorkspaces('pipfile-pipapp'); - + it('cannot fix a Pipfile app', () => { const upgrades = { 'Django@1.6.1': { upgradeTo: 'Django@2.0.1' }, 'transitive@1.0.0': { upgradeTo: 'transitive@1.1.1' }, }; const manifests = { - Pipfile: readFileSync('Pipfile', 'utf8'), + Pipfile: readFileSync( + path.resolve('test', 'workspaces', 'pipfile-pipapp', 'Pipfile'), + 'utf8' + ), }; - await expect( - applyRemediationToManifests('.', manifests, upgrades, {}) - ).rejects.toMatchObject({ - message: 'Remediation only supported for requirements.txt file', - }); + expect(() => applyRemediationToManifests(manifests, upgrades)).toThrow( + 'Remediation only supported for requirements.txt file' + ); }); });