From 0384020f20fc71f1ba06a2b3d0a12f499e21a65f Mon Sep 17 00:00:00 2001 From: ghe Date: Fri, 26 Mar 2021 13:42:46 +0000 Subject: [PATCH] feat: basic pip fix -r support --- .../src/lib/errors/invalid-workspace.ts | 10 ++ .../python/handlers/pip-requirements/index.ts | 117 ++++++++++++--- .../update-dependencies/generate-pins.ts | 10 +- .../update-dependencies/generate-upgrades.ts | 17 ++- .../update-dependencies/index.ts | 27 ++-- .../update-dependencies.spec.ts.snap | 13 ++ .../update-dependencies.spec.ts | 139 ++++++++++++------ .../workspaces/pip-app/requirements.txt | 1 + packages/snyk-fix/test/unit/fix.spec.ts | 2 +- 9 files changed, 258 insertions(+), 78 deletions(-) create mode 100644 packages/snyk-fix/src/lib/errors/invalid-workspace.ts diff --git a/packages/snyk-fix/src/lib/errors/invalid-workspace.ts b/packages/snyk-fix/src/lib/errors/invalid-workspace.ts new file mode 100644 index 0000000000..6150766d05 --- /dev/null +++ b/packages/snyk-fix/src/lib/errors/invalid-workspace.ts @@ -0,0 +1,10 @@ +import { CustomError, ERROR_CODES } from './custom-error'; + +export class MissingFileNameError extends CustomError { + public constructor() { + super( + 'Filename is missing from test result. Please contact support@snyk.io.', + ERROR_CODES.MissingFileName, + ); + } +} diff --git a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/index.ts b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/index.ts index aa78a2cdd8..d57d45acdd 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/index.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/index.ts @@ -2,6 +2,7 @@ import * as debugLib from 'debug'; import * as pathLib from 'path'; import { + DependencyPins, EntityToFix, FixChangesSummary, FixOptions, @@ -18,6 +19,11 @@ import { extractProvenance, PythonProvenance, } from './extract-version-provenance'; +import { + ParsedRequirements, + parseRequirementsFile, + Requirement, +} from './update-dependencies/requirements-file-parser'; const debug = debugLib('snyk-fix:python:requirements.txt'); @@ -37,17 +43,18 @@ export async function pipRequirementsTxt( for (const entity of fixable) { try { - const { remediation, targetFile, workspace } = getRequiredData(entity); - const { dir, base } = pathLib.parse(targetFile); - const provenance = await extractProvenance(workspace, dir, base); - const changes = await fixIndividualRequirementsTxt( - workspace, - dir, - base, - remediation, - provenance, + const { changes } = await applyAllFixes( + entity, + // dir, + // base, + // remediation, + // provenance, options, ); + if (!changes.length) { + debug('Manifest has not changed!'); + throw new NoFixesCouldBeAppliedError(); + } handlerResult.succeeded.push({ original: entity, changes }); } catch (e) { handlerResult.failed.push({ original: entity, error: e }); @@ -82,27 +89,95 @@ export function getRequiredData( export async function fixIndividualRequirementsTxt( workspace: Workspace, dir: string, + entryFileName: string, fileName: string, remediation: RemediationChanges, - provenance: PythonProvenance, + parsedRequirements: ParsedRequirements, options: FixOptions, -): Promise { - // TODO: allow handlers per fix type (later also strategies or combine with strategies) - const { updatedManifest, changes } = updateDependencies( - provenance[fileName], + directUpgradesOnly: boolean, +): Promise<{ changes: FixChangesSummary[]; appliedRemediation: string[] }> { + const fullFilePath = pathLib.join(dir, fileName); + const { updatedManifest, changes, appliedRemediation } = updateDependencies( + parsedRequirements, remediation.pin, + directUpgradesOnly, + pathLib.join(dir, entryFileName) !== fullFilePath ? fileName : undefined, ); - - if (!changes.length) { - debug('Manifest has not changed!'); - throw new NoFixesCouldBeAppliedError(); - } - if (!options.dryRun) { + if (!options.dryRun && changes.length > 0) { debug('Writing changes to file'); await workspace.writeFile(pathLib.join(dir, fileName), updatedManifest); } else { debug('Skipping writing changes to file in --dry-run mode'); } - return changes; + return { changes, appliedRemediation }; +} + +export async function applyAllFixes( + entity: EntityToFix, + options: FixOptions, +): Promise<{ changes: FixChangesSummary[] }> { + const { remediation, targetFile: entryFileName, workspace } = getRequiredData( + entity, + ); + const { dir, base } = pathLib.parse(entryFileName); + const provenance = await extractProvenance(workspace, dir, base); + const upgradeChanges: FixChangesSummary[] = []; + const appliedUpgradeRemediation: string[] = []; + for (const fileName of Object.keys(provenance)) { + const skipApplyingPins = true; + const { changes, appliedRemediation } = await fixIndividualRequirementsTxt( + workspace, + dir, + base, + fileName, + remediation, + provenance[fileName], + options, + skipApplyingPins, + ); + appliedUpgradeRemediation.push(...appliedRemediation); + // what if we saw the file before and already fixed it? + upgradeChanges.push(...changes); + } + // now do left overs as pins + add tests + const requirementsTxt = await workspace.readFile(entryFileName); + + const toPin: RemediationChanges = filterOutAppliedUpgrades( + remediation, + appliedUpgradeRemediation, + ); + const directUpgradesOnly = false; + const { changes: pinnedChanges } = await fixIndividualRequirementsTxt( + workspace, + dir, + base, + base, + toPin, + parseRequirementsFile(requirementsTxt), + options, + directUpgradesOnly, + ); + + return { changes: [...upgradeChanges, ...pinnedChanges] }; +} + +function filterOutAppliedUpgrades( + remediation: RemediationChanges, + appliedRemediation: string[], +): RemediationChanges { + const pinRemediation: RemediationChanges = { + ...remediation, + pin: {}, // delete the pin remediation so we can add only not applied + }; + const pins = remediation.pin; + const lowerCasedAppliedRemediation = appliedRemediation.map((i) => + i.toLowerCase(), + ); + for (const pkgAtVersion of Object.keys(pins)) { + if (!lowerCasedAppliedRemediation.includes(pkgAtVersion.toLowerCase())) { + pinRemediation.pin[pkgAtVersion] = pins[pkgAtVersion]; + } + } + return pinRemediation; } diff --git a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-pins.ts b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-pins.ts index 779c50a2d8..dc43ee4b2a 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-pins.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-pins.ts @@ -6,7 +6,11 @@ import { Requirement } from './requirements-file-parser'; export function generatePins( requirements: Requirement[], updates: DependencyPins, -): { pinnedRequirements: string[]; changes: FixChangesSummary[] } { +): { + pinnedRequirements: string[]; + changes: FixChangesSummary[]; + appliedRemediation: string[]; +} { // 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. @@ -20,8 +24,10 @@ export function generatePins( return { pinnedRequirements: [], changes: [], + appliedRemediation: [], }; } + const appliedRemediation: string[] = []; const changes: FixChangesSummary[] = []; const pinnedRequirements = Object.keys(lowerCasedPins) .map((pkgNameAtVersion) => { @@ -32,6 +38,7 @@ export function generatePins( success: true, userMessage: `Pinned ${pkgName} from ${version} to ${newVersion}`, }); + appliedRemediation.push(pkgNameAtVersion); return `${newRequirement} # not directly required, pinned by Snyk to avoid a vulnerability`; }) .filter(isDefined); @@ -39,5 +46,6 @@ export function generatePins( return { pinnedRequirements, changes, + appliedRemediation, }; } diff --git a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-upgrades.ts b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-upgrades.ts index fe37307dc7..62a5169bbc 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-upgrades.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/generate-upgrades.ts @@ -6,7 +6,12 @@ import { UpgradedRequirements } from './types'; export function generateUpgrades( requirements: Requirement[], updates: DependencyPins, -): { updatedRequirements: UpgradedRequirements; changes: FixChangesSummary[] } { + referenceFileInChanges?: string, +): { + updatedRequirements: UpgradedRequirements; + changes: FixChangesSummary[]; + appliedRemediation: string[]; +} { // 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. @@ -19,9 +24,11 @@ export function generateUpgrades( return { updatedRequirements: {}, changes: [], + appliedRemediation: [], }; } + const appliedRemediation: string[] = []; const changes: FixChangesSummary[] = []; const updatedRequirements = {}; requirements.map( @@ -58,16 +65,22 @@ export function generateUpgrades( const updatedRequirement = `${originalName}${versionComparator}${newVersion}`; changes.push({ success: true, - userMessage: `Upgraded ${originalName} from ${version} to ${newVersion}`, + userMessage: `Upgraded ${originalName} from ${version} to ${newVersion}${ + referenceFileInChanges + ? ` (upgraded in ${referenceFileInChanges})` + : '' + }`, }); updatedRequirements[originalText] = `${updatedRequirement}${ extras ? extras : '' }`; + appliedRemediation.push(upgrade); }, ); return { updatedRequirements, changes, + appliedRemediation, }; } diff --git a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/index.ts b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/index.ts index 9edda80154..5b0fdda2c7 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/index.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/update-dependencies/index.ts @@ -20,7 +20,12 @@ export function updateDependencies( parsedRequirementsData: ParsedRequirements, updates: DependencyPins, directUpgradesOnly = false, -): { updatedManifest: string; changes: FixChangesSummary[] } { + referenceFileInChanges?: string, +): { + updatedManifest: string; + changes: FixChangesSummary[]; + appliedRemediation: string[]; +} { const { requirements, endsWithNewLine: shouldEndWithNewLine, @@ -33,19 +38,22 @@ export function updateDependencies( } debug('Finished parsing manifest'); - const { updatedRequirements, changes: upgradedChanges } = generateUpgrades( - requirements, - updates, - ); + const { + updatedRequirements, + changes: upgradedChanges, + appliedRemediation, + } = generateUpgrades(requirements, updates, referenceFileInChanges); debug('Finished generating upgrades to apply'); let pinnedRequirements: string[] = []; let pinChanges: FixChangesSummary[] = []; + let appliedPinsRemediation: string[] = []; if (!directUpgradesOnly) { - ({ pinnedRequirements, changes: pinChanges } = generatePins( - requirements, - updates, - )); + ({ + pinnedRequirements, + changes: pinChanges, + appliedRemediation: appliedPinsRemediation, + } = generatePins(requirements, updates)); debug('Finished generating pins to apply'); } @@ -64,5 +72,6 @@ export function updateDependencies( return { updatedManifest, changes: [...pinChanges, ...upgradedChanges], + appliedRemediation: [...appliedPinsRemediation, ...appliedRemediation], }; } diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/__snapshots__/update-dependencies.spec.ts.snap b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/__snapshots__/update-dependencies.spec.ts.snap index 37ceccdf94..980322383a 100644 --- a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/__snapshots__/update-dependencies.spec.ts.snap +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/__snapshots__/update-dependencies.spec.ts.snap @@ -1,5 +1,18 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`fix *req*.txt / *.txt Python projects fixes multiple files that are included via -r 1`] = ` +Array [ + Object { + "success": true, + "userMessage": "Upgraded Django from 1.6.1 to 2.0.1", + }, + Object { + "success": true, + "userMessage": "Upgraded Jinja2 from 2.7.2 to 2.7.3 (upgraded in base2.txt)", + }, +] +`; + exports[`fix *req*.txt / *.txt Python projects retains python markers 1`] = ` "amqp==2.4.2 apscheduler==3.6.0 diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/update-dependencies.spec.ts b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/update-dependencies.spec.ts index a637ca42f8..bb3d000608 100644 --- a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/update-dependencies.spec.ts +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/update-dependencies.spec.ts @@ -42,10 +42,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -80,11 +77,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { changes: [ { success: true, - userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', + userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', }, { success: true, - userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', + userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', }, ], }, @@ -126,10 +123,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -170,11 +164,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { changes: [ { success: true, - userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', + userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', }, { success: true, - userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', + userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', }, ], }, @@ -217,10 +211,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -261,11 +252,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { changes: [ { success: true, - userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', + userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', }, { success: true, - userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', + userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', }, ], }, @@ -308,10 +299,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -353,11 +341,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { changes: [ { success: true, - userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', + userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', }, { success: true, - userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', + userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', }, ], }, @@ -395,10 +383,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -478,10 +463,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -560,10 +542,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -647,10 +626,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -732,10 +708,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { const entityToFix = { workspace: { readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); + return readFileHelper(workspacesPath, path); }, writeFile: async (path: string, contents: string) => { const res = pathLib.parse(path); @@ -782,4 +755,82 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }); }); + it('fixes multiple files that are included via -r', async () => { + // Arrange + const targetFile = 'pip-app/requirements.txt'; + filesToDelete = [ + pathLib.resolve(workspacesPath, 'pip-app/fixed-requirements.txt'), + pathLib.resolve(workspacesPath, 'pip-app/fixed-base2.txt'), + ]; + const testResult = { + ...generateTestResult(), + remediation: { + unresolved: [], + upgrade: {}, + patch: {}, + ignore: {}, + pin: { + 'django@1.6.1': { + upgradeTo: 'django@2.0.1', + vulns: [], + isTransitive: false, + }, + 'Jinja2@2.7.2': { + upgradeTo: 'Jinja2@2.7.3', + vulns: [], + isTransitive: true, + }, + }, + }, + }; + const entityToFix = { + workspace: { + readFile: async (path: string) => { + return readFileHelper(workspacesPath, path); + }, + writeFile: async (path: string, contents: string) => { + const res = pathLib.parse(path); + const fixedPath = pathLib.resolve( + workspacesPath, + res.dir, + `fixed-${res.base}`, + ); + fs.writeFileSync(fixedPath, contents, 'utf-8'); + }, + }, + scanResult: generateScanResult('pip', targetFile), + testResult, + }; + const writeFileSpy = jest.spyOn(entityToFix.workspace, 'writeFile'); + + // Act + const result = await snykFix.fix([entityToFix], { + quiet: true, + stripAnsi: true, + }); + + // 2 files needed to have changes + expect(writeFileSpy).toHaveBeenCalledTimes(2); + expect(result.results.python.succeeded[0].original).toEqual(entityToFix); + expect(result.results.python.succeeded[0].changes).toMatchSnapshot(); + }); }); + +function readFileHelper(workspacesPath: string, path: string): string { + // because we write multiple time the file + // may be have already been updated in fixed-* name + // so try read that first + const res = pathLib.parse(path); + const fixedPath = pathLib.resolve( + workspacesPath, + res.dir, + `fixed-${res.base}`, + ); + let file; + try { + file = fs.readFileSync(fixedPath, 'utf-8'); + } catch (e) { + file = fs.readFileSync(pathLib.resolve(workspacesPath, path), 'utf-8'); + } + return file; +} diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/pip-app/requirements.txt b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/pip-app/requirements.txt index a8d089e1c8..44d5b49554 100644 --- a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/pip-app/requirements.txt +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/pip-app/requirements.txt @@ -1,2 +1,3 @@ -r base.txt +-r base2.txt Django==1.6.1 diff --git a/packages/snyk-fix/test/unit/fix.spec.ts b/packages/snyk-fix/test/unit/fix.spec.ts index 6eb4df456b..b0d27fed6e 100644 --- a/packages/snyk-fix/test/unit/fix.spec.ts +++ b/packages/snyk-fix/test/unit/fix.spec.ts @@ -17,7 +17,7 @@ describe('Snyk fix', () => { }); // Assert - expect(writeFileSpy).toHaveBeenCalled(); + expect(writeFileSpy).toHaveBeenCalledTimes(1); expect(res.exceptions).toMatchSnapshot(); expect(res.results).toMatchSnapshot(); });