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/extract-version-provenance.ts b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/extract-version-provenance.ts index b75f98d8ee..2b6a61241f 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/extract-version-provenance.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/extract-version-provenance.ts @@ -8,7 +8,7 @@ import { import { Workspace } from '../../../../types'; import { containsRequireDirective } from './contains-require-directive'; -interface PythonProvenance { +export interface PythonProvenance { [fileName: string]: ParsedRequirements; } @@ -20,7 +20,8 @@ export async function extractProvenance( fileName: string, provenance: PythonProvenance = {}, ): Promise { - const requirementsTxt = await workspace.readFile(path.join(dir, fileName)); + const requirementsFileName = path.join(dir, fileName); + const requirementsTxt = await workspace.readFile(requirementsFileName); provenance = { ...provenance, [fileName]: parseRequirementsFile(requirementsTxt), 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 2b457a64e7..4467986837 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 @@ -1,9 +1,14 @@ import * as debugLib from 'debug'; +import * as pathLib from 'path'; +const sortBy = require('lodash.sortby'); +const groupBy = require('lodash.groupby'); import { EntityToFix, + FixChangesSummary, FixOptions, - WithFixChangesApplied, + RemediationChanges, + Workspace, } from '../../../../types'; import { PluginFixResponse } from '../../../types'; import { updateDependencies } from './update-dependencies'; @@ -11,7 +16,11 @@ import { MissingRemediationDataError } from '../../../../lib/errors/missing-reme import { MissingFileNameError } from '../../../../lib/errors/missing-file-name'; import { partitionByFixable } from './is-supported'; import { NoFixesCouldBeAppliedError } from '../../../../lib/errors/no-fixes-applied'; -import { parseRequirementsFile } from './update-dependencies/requirements-file-parser'; +import { extractProvenance } from './extract-version-provenance'; +import { + ParsedRequirements, + parseRequirementsFile, +} from './update-dependencies/requirements-file-parser'; const debug = debugLib('snyk-fix:python:requirements.txt'); @@ -26,56 +35,207 @@ export async function pipRequirementsTxt( skipped: [], }; - const { fixable, skipped } = await partitionByFixable(entities); - handlerResult.skipped.push(...skipped); + const { fixable, skipped: notFixable } = await partitionByFixable(entities); + handlerResult.skipped.push(...notFixable); - for (const entity of fixable) { - try { - const fixedEntity = await fixIndividualRequirementsTxt(entity, options); - handlerResult.succeeded.push(fixedEntity); - } catch (e) { - handlerResult.failed.push({ original: entity, error: e }); - } + const ordered = sortByDirectory(fixable); + const fixedFilesCache: string[] = []; + for (const dir of Object.keys(ordered)) { + debug(`Fixing entities in directory ${dir}`); + const entitiesPerDirectory = ordered[dir].map((e) => e.entity); + const { failed, succeeded, skipped, fixedFiles } = await fixAll( + entitiesPerDirectory, + options, + fixedFilesCache, + ); + fixedFilesCache.push(...fixedFiles); + handlerResult.succeeded.push(...succeeded); + handlerResult.failed.push(...failed); + handlerResult.skipped.push(...skipped); } return handlerResult; } -// TODO: optionally verify the deps install -export async function fixIndividualRequirementsTxt( +export function getRequiredData( entity: EntityToFix, - options: FixOptions, -): Promise> { - const fileName = entity.scanResult.identity.targetFile; - const remediationData = entity.testResult.remediation; - if (!remediationData) { +): { + remediation: RemediationChanges; + targetFile: string; + workspace: Workspace; +} { + const { remediation } = entity.testResult; + if (!remediation) { throw new MissingRemediationDataError(); } - if (!fileName) { + const { targetFile } = entity.scanResult.identity; + if (!targetFile) { throw new MissingFileNameError(); } - const requirementsTxt = await entity.workspace.readFile(fileName); - const requirementsData = parseRequirementsFile(requirementsTxt); + const { workspace } = entity; + if (!workspace) { + throw new NoFixesCouldBeAppliedError(); + } + return { targetFile, remediation, workspace }; +} - // TODO: allow handlers per fix type (later also strategies or combine with strategies) - const { updatedManifest, changes } = updateDependencies( - requirementsData, - remediationData.pin, +async function fixAll( + entities: EntityToFix[], + options: FixOptions, + fixedCache: string[], +): Promise { + const handlerResult: PluginFixResponse = { + succeeded: [], + failed: [], + skipped: [], + }; + for (const entity of entities) { + const targetFile = entity.scanResult.identity.targetFile!; + try { + const { dir, base } = pathLib.parse(targetFile); + // parse & join again to support correct separator + if (fixedCache.includes(pathLib.join(dir, base))) { + handlerResult.succeeded.push({ + original: entity, + changes: [{ success: true, userMessage: 'Previously fixed' }], + }); + continue; + } + const { changes, fixedFiles } = await applyAllFixes(entity, options); + if (!changes.length) { + debug('Manifest has not changed!'); + throw new NoFixesCouldBeAppliedError(); + } + fixedCache.push(...fixedFiles); + handlerResult.succeeded.push({ original: entity, changes }); + } catch (e) { + debug(`Failed to fix ${targetFile}.\nERROR: ${e}`); + handlerResult.failed.push({ original: entity, error: e }); + } + } + return { ...handlerResult, fixedFiles: [] }; +} +// TODO: optionally verify the deps install +export async function fixIndividualRequirementsTxt( + workspace: Workspace, + dir: string, + entryFileName: string, + fileName: string, + remediation: RemediationChanges, + parsedRequirements: ParsedRequirements, + options: FixOptions, + 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, ); - // TODO: do this with the changes now that we only return new - if (updatedManifest === requirementsTxt) { - debug('Manifest has not changed!'); - throw new NoFixesCouldBeAppliedError(); + if (!changes.length) { + return { changes, appliedRemediation }; } + if (!options.dryRun) { debug('Writing changes to file'); - await entity.workspace.writeFile(fileName, updatedManifest); + await workspace.writeFile(pathLib.join(dir, fileName), updatedManifest); } else { debug('Skipping writing changes to file in --dry-run mode'); } - return { - original: entity, - changes, + return { changes, appliedRemediation }; +} + +export async function applyAllFixes( + entity: EntityToFix, + options: FixOptions, +): Promise<{ changes: FixChangesSummary[]; fixedFiles: string[] }> { + const { remediation, targetFile: entryFileName, workspace } = getRequiredData( + entity, + ); + const fixedFiles: string[] = []; + const { dir, base } = pathLib.parse(entryFileName); + const provenance = await extractProvenance(workspace, dir, base); + const upgradeChanges: FixChangesSummary[] = []; + const appliedUpgradeRemediation: string[] = []; + /* Apply all upgrades first across all files that are included */ + 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); + upgradeChanges.push(...changes); + fixedFiles.push(pathLib.join(dir, fileName)); + } + + /* Apply all left over remediation as pins in the entry targetFile */ + 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], fixedFiles }; +} + +function filterOutAppliedUpgrades( + remediation: RemediationChanges, + appliedRemediation: string[], +): RemediationChanges { + const pinRemediation: RemediationChanges = { + ...remediation, + pin: {}, // delete the pin remediation so we can collect un-applied remediation }; + 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; +} + +function sortByDirectory( + entities: EntityToFix[], +): { + [dir: string]: Array<{ + entity: EntityToFix; + dir: string; + base: string; + ext: string; + root: string; + name: string; + }>; +} { + const mapped = entities.map((e) => ({ + entity: e, + ...pathLib.parse(e.scanResult.identity.targetFile!), + })); + + const sorted = sortBy(mapped, 'dir'); + return groupBy(sorted, 'dir'); } diff --git a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/is-supported.ts b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/is-supported.ts index 25ce4b9cff..35f14f2913 100644 --- a/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/is-supported.ts +++ b/packages/snyk-fix/src/plugins/python/handlers/pip-requirements/is-supported.ts @@ -45,11 +45,13 @@ export async function isSupported( }; } - const { containsRequire } = await containsRequireDirective(requirementsTxt); - if (containsRequire) { + const { containsRequire, matches } = await containsRequireDirective( + requirementsTxt, + ); + if (containsRequire && matches.some((m) => m.includes('c'))) { return { supported: false, - reason: `Requirements with ${chalk.bold('-r')} or ${chalk.bold( + reason: `Requirements with ${chalk.bold( '-c', )} directive are not yet supported`, }; 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 383d90d6af..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 @@ -19,7 +19,13 @@ const debug = debugLib('snyk-fix:python:update-dependencies'); export function updateDependencies( parsedRequirementsData: ParsedRequirements, updates: DependencyPins, -): { updatedManifest: string; changes: FixChangesSummary[] } { + directUpgradesOnly = false, + referenceFileInChanges?: string, +): { + updatedManifest: string; + changes: FixChangesSummary[]; + appliedRemediation: string[]; +} { const { requirements, endsWithNewLine: shouldEndWithNewLine, @@ -32,17 +38,24 @@ 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'); - const { pinnedRequirements, changes: pinChanges } = generatePins( - requirements, - updates, - ); - debug('Finished generating pins to apply'); + let pinnedRequirements: string[] = []; + let pinChanges: FixChangesSummary[] = []; + let appliedPinsRemediation: string[] = []; + if (!directUpgradesOnly) { + ({ + pinnedRequirements, + changes: pinChanges, + appliedRemediation: appliedPinsRemediation, + } = generatePins(requirements, updates)); + debug('Finished generating pins to apply'); + } let updatedManifest = [ ...applyUpgrades(requirements, updatedRequirements), @@ -59,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..d554e57d51 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,55 @@ // 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 fixes multiple files via -r with the same name (some were already fixed) 1`] = ` +"Successful fixes: + + app-with-already-fixed/requirements.txt + ✔ Upgraded Django from 1.6.1 to 2.0.1 + ✔ Upgraded Django from 1.6.1 to 2.0.1 (upgraded in core/requirements.txt) + ✔ Upgraded Jinja2 from 2.7.2 to 2.7.3 (upgraded in lib/requirements.txt) + + app-with-already-fixed/core/requirements.txt + ✔ Previously fixed + + app-with-already-fixed/lib/requirements.txt + ✔ Previously fixed + +Summary: + + 0 items were not fixed + 3 items were successfully fixed" +`; + +exports[`fix *req*.txt / *.txt Python projects fixes multiple files via -r with the same name (some were already fixed) 2`] = ` +Array [ + Object { + "success": true, + "userMessage": "Upgraded Django from 1.6.1 to 2.0.1", + }, + Object { + "success": true, + "userMessage": "Upgraded Django from 1.6.1 to 2.0.1 (upgraded in core/requirements.txt)", + }, + Object { + "success": true, + "userMessage": "Upgraded Jinja2 from 2.7.2 to 2.7.3 (upgraded in lib/requirements.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 04fe080ad0..11fec6844e 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 @@ -1,6 +1,7 @@ import * as fs from 'fs'; import * as pathLib from 'path'; import * as snykFix from '../../../../../src'; +import { TestResult } from '../../../../../src/types'; import { generateScanResult, generateTestResult, @@ -12,9 +13,13 @@ describe('fix *req*.txt / *.txt Python projects', () => { filesToDelete.map((f) => fs.unlinkSync(f)); }); const workspacesPath = pathLib.resolve(__dirname, 'workspaces'); - it('skips projects with a -r option', async () => { + + it('fixes project with a -r option', async () => { // Arrange const targetFile = 'with-require/dev.txt'; + filesToDelete = [ + pathLib.join(workspacesPath, 'with-require/fixed-dev.txt'), + ]; const testResult = { ...generateTestResult(), @@ -38,28 +43,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -72,15 +60,22 @@ describe('fix *req*.txt / *.txt Python projects', () => { results: { python: { failed: [], - skipped: [ + skipped: [], + succeeded: [ { original: entityToFix, - userMessage: expect.stringContaining( - 'directive are not yet supported', - ), + changes: [ + { + success: true, + userMessage: 'Upgraded Django from 1.6.1 to 2.0.1', + }, + { + success: true, + userMessage: 'Pinned transitive from 1.0.0 to 1.1.1', + }, + ], }, ], - succeeded: [], }, }, }); @@ -92,6 +87,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'basic/fixed-prod.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -115,28 +111,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -162,11 +141,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', }, ], }, @@ -183,6 +162,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'basic-with-newline/fixed-prod.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -206,28 +186,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -253,11 +216,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', }, ], }, @@ -274,6 +237,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'with-custom-formatting/fixed-requirements.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -297,28 +261,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -345,11 +292,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', }, ], }, @@ -366,6 +313,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'lower-case-dep/fixed-req.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -384,28 +332,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -448,6 +379,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'basic/fixed-prod.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -467,28 +399,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -531,6 +446,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'long-versions/fixed-prod.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -549,28 +465,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -613,6 +512,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { workspacesPath, 'with-comparator/fixed-prod.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), @@ -636,28 +536,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -704,6 +587,7 @@ describe('fix *req*.txt / *.txt Python projects', () => { 'python-markers/fixed-prod.txt', ); + filesToDelete = [fixedFilePath]; const testResult = { ...generateTestResult(), remediation: { @@ -721,28 +605,11 @@ describe('fix *req*.txt / *.txt Python projects', () => { }, }; - const entityToFix = { - workspace: { - readFile: async (path: string) => { - return fs.readFileSync( - pathLib.resolve(workspacesPath, path), - 'utf-8', - ); - }, - writeFile: async (path: string, contents: string) => { - const res = pathLib.parse(path); - const fixedPath = pathLib.resolve( - workspacesPath, - res.dir, - `fixed-${res.base}`, - ); - filesToDelete = [fixedPath]; - fs.writeFileSync(fixedPath, contents, 'utf-8'); - }, - }, - scanResult: generateScanResult('pip', targetFile), + const entityToFix = generateEntityToFix( + workspacesPath, + targetFile, testResult, - }; + ); // Act const result = await snykFix.fix([entityToFix], { @@ -774,4 +641,166 @@ 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 = generateEntityToFix( + workspacesPath, + 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(); + }); + it('fixes multiple files via -r with the same name (some were already fixed)', async () => { + // Arrange + const targetFile1 = 'app-with-already-fixed/requirements.txt'; + const targetFile2 = 'app-with-already-fixed/lib/requirements.txt'; + const targetFile3 = 'app-with-already-fixed/core/requirements.txt'; + + filesToDelete = [ + pathLib.resolve( + workspacesPath, + 'app-with-already-fixed/fixed-requirements.txt', + ), + pathLib.resolve( + workspacesPath, + 'app-with-already-fixed/lib/fixed-requirements.txt', + ), + pathLib.resolve( + workspacesPath, + 'app-with-already-fixed/core/fixed-requirements.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 entityToFix1 = generateEntityToFix( + workspacesPath, + targetFile1, + testResult, + ); + const entityToFix2 = generateEntityToFix( + workspacesPath, + targetFile2, + testResult, + ); + const entityToFix3 = generateEntityToFix( + workspacesPath, + targetFile3, + testResult, + ); + const writeFileSpy = jest.spyOn(entityToFix1.workspace, 'writeFile'); + // Act + const result = await snykFix.fix( + [entityToFix2, entityToFix3, entityToFix1], + { + quiet: true, + stripAnsi: true, + }, + ); + // 3 files needed to have changes + expect(result.fixSummary).toMatchSnapshot(); + expect(writeFileSpy).toHaveBeenCalledTimes(3); + expect(result.results.python.succeeded[0].original).toEqual(entityToFix1); + 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; +} + +function generateEntityToFix( + workspacesPath: string, + targetFile: string, + testResult: TestResult, +): snykFix.EntityToFix { + 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, + }; + return entityToFix; +} diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/base.txt b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/base.txt new file mode 100644 index 0000000000..aba69a5a27 --- /dev/null +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/base.txt @@ -0,0 +1 @@ +click>7.0 diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/core/requirements.txt b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/core/requirements.txt new file mode 100644 index 0000000000..e5568357b9 --- /dev/null +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/core/requirements.txt @@ -0,0 +1 @@ +Django==1.6.1 ; python_version > '1.0' diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/lib/requirements.txt b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/lib/requirements.txt new file mode 100644 index 0000000000..c3eeef4fb7 --- /dev/null +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/lib/requirements.txt @@ -0,0 +1 @@ +Jinja2==2.7.2 diff --git a/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/requirements.txt b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/requirements.txt new file mode 100644 index 0000000000..3693dd3bd5 --- /dev/null +++ b/packages/snyk-fix/test/acceptance/plugins/python/update-dependencies/workspaces/app-with-already-fixed/requirements.txt @@ -0,0 +1,4 @@ +-r core/requirements.txt +-r lib/requirements.txt +-r base.txt +Django==1.6.1 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(); }); diff --git a/packages/snyk-fix/test/unit/plugins/python/handlers/update-dependencies/update-dependencies.spec.ts b/packages/snyk-fix/test/unit/plugins/python/handlers/update-dependencies/update-dependencies.spec.ts index 2b65831213..133a9455d2 100644 --- a/packages/snyk-fix/test/unit/plugins/python/handlers/update-dependencies/update-dependencies.spec.ts +++ b/packages/snyk-fix/test/unit/plugins/python/handlers/update-dependencies/update-dependencies.spec.ts @@ -306,4 +306,40 @@ describe('remediation', () => { ); } }); + it('skips pins if asked', () => { + const upgrades = { + 'django@1.6.1': { + upgradeTo: 'django@2.0.1', + vulns: [], + upgrades: [], + isTransitive: false, + }, + 'transitive@1.0.0': { + upgradeTo: 'transitive@1.1.1', + vulns: [], + upgrades: [], + isTransitive: true, + }, + }; + + const manifestContents = 'Django==1.6.1'; + + const expectedManifest = + 'Django==2.0.1\ntransitive>=1.1.1 # not directly required, pinned by Snyk to avoid a vulnerability'; + const directUpgradesOnly = false; + const requirements = parseRequirementsFile(manifestContents); + const result = updateDependencies( + requirements, + upgrades, + directUpgradesOnly, + ); + expect(result.changes.map((c) => c.userMessage).sort()).toEqual( + [ + 'Pinned transitive from 1.0.0 to 1.1.1', + 'Upgraded Django from 1.6.1 to 2.0.1', + ].sort(), + ); + // Note no extra newline was added to the expected manifest + expect(result.updatedManifest).toEqual(expectedManifest); + }); }); diff --git a/packages/snyk-fix/test/unit/plugins/python/is-supported.ts b/packages/snyk-fix/test/unit/plugins/python/is-supported.spec.ts similarity index 92% rename from packages/snyk-fix/test/unit/plugins/python/is-supported.ts rename to packages/snyk-fix/test/unit/plugins/python/is-supported.spec.ts index fbb2210090..24cc508b40 100644 --- a/packages/snyk-fix/test/unit/plugins/python/is-supported.ts +++ b/packages/snyk-fix/test/unit/plugins/python/is-supported.spec.ts @@ -13,14 +13,14 @@ describe('isSupported', () => { const res = await isSupported(entity); expect(res.supported).toBeFalsy(); }); - it('with -r directive in the manifest not supported', async () => { + it('with -r directive in the manifest is supported', async () => { const entity = generateEntityToFix( 'pip', 'requirements.txt', '-r prod.txt\nDjango==1.6.1', ); const res = await isSupported(entity); - expect(res.supported).toBeFalsy(); + expect(res.supported).toBeTruthy(); }); it('with -c directive in the manifest not supported', async () => { const entity = generateEntityToFix(