From e8fed385badee4f3832036dad790a9dc4f736a63 Mon Sep 17 00:00:00 2001 From: Jahed Ahmed Date: Thu, 19 Aug 2021 18:16:13 +0000 Subject: [PATCH 1/4] refactor(protect): move stdout logs to top level --- packages/snyk-protect/src/lib/index.ts | 9 ++++---- packages/snyk-protect/src/lib/patch.ts | 23 ++++++++----------- .../test/acceptance/fix-pr.smoke.spec.ts | 2 +- .../test/acceptance/protect.spec.ts | 8 +++---- 4 files changed, 19 insertions(+), 23 deletions(-) diff --git a/packages/snyk-protect/src/lib/index.ts b/packages/snyk-protect/src/lib/index.ts index d25b81740b..e969de70b1 100644 --- a/packages/snyk-protect/src/lib/index.ts +++ b/packages/snyk-protect/src/lib/index.ts @@ -26,7 +26,7 @@ async function protect(projectFolderPath: string) { const snykFilePath = path.resolve(projectFolderPath, '.snyk'); if (!fs.existsSync(snykFilePath)) { - console.log('No .snyk file found'); + console.log('No .snyk file found.'); sendAnalytics({ type: ProtectResultType.NO_SNYK_FILE, }); @@ -67,7 +67,7 @@ async function protect(projectFolderPath: string) { > = await getAllPatches(vulnIdAndPackageNames, packageNameToVersionsMap); if (packageAtVersionsToPatches.size === 0) { - console.log('Nothing to patch'); + console.log('Nothing to patch.'); sendAnalytics({ type: ProtectResultType.NOTHING_TO_PATCH, }); @@ -83,7 +83,8 @@ async function protect(projectFolderPath: string) { vuldIdAndPatches?.forEach((vp) => { vp.patches.forEach((patchDiffs) => { patchDiffs.patchDiffs.forEach((diff) => { - applyPatchToFile(diff, fpp.path); + const patchedPath = applyPatchToFile(diff, fpp.path); + console.log(`Patched: ${patchedPath}`); }); }); patchedModules.push({ @@ -94,7 +95,7 @@ async function protect(projectFolderPath: string) { }); }); - console.log('Successfully applied Snyk patches'); + console.log('Applied Snyk patches.'); sendAnalytics({ type: ProtectResultType.APPLIED_PATCHES, diff --git a/packages/snyk-protect/src/lib/patch.ts b/packages/snyk-protect/src/lib/patch.ts index 1ce3266d03..f6a1534ff8 100644 --- a/packages/snyk-protect/src/lib/patch.ts +++ b/packages/snyk-protect/src/lib/patch.ts @@ -9,7 +9,7 @@ export function applyPatchToFile(patchContents: string, baseFolder: string) { const contentsToPatch = fs.readFileSync(targetFilePath, 'utf-8'); const patchedContents = patchString(patchContents, contentsToPatch); fs.writeFileSync(targetFilePath, patchedContents); - console.log(`patched ${targetFilePath}`); + return targetFilePath; } export function extractTargetFilePathFromPatch(patchContents: string): string { @@ -39,13 +39,11 @@ export function patchString( const contentsToPatchLines = contentsToPatch.split('\n'); if (!patchContentLines[2]) { - // return; - throw new Error('Invalid patch'); + throw new Error('Invalid patch.'); } const unparsedLineToPatch = /^@@ -(\d*),.*@@/.exec(patchContentLines[2]); if (!unparsedLineToPatch || !unparsedLineToPatch[1]) { - // return; - throw new Error('Invalid patch'); + throw new Error('Invalid patch.'); } let lineToPatch = parseInt(unparsedLineToPatch[1], 10) - 2; @@ -66,16 +64,13 @@ export function patchString( } case ' ': { if (currentLine !== nextLine) { - console.log( - 'Expected\n line from local file\n', - JSON.stringify(currentLine), - '\n to match patch line\n', - JSON.stringify(nextLine), - '\n', - ); throw new Error( - // `File ${filename} to be patched does not match, not patching`, - `File to be patched does not match, not patching`, + 'File does not match patch contents.' + + ' Expected\n' + + ' line from local file\n' + + ` ${JSON.stringify(currentLine)}\n` + + ' to match patch line\n' + + ` ${JSON.stringify(nextLine)}\n`, ); } break; diff --git a/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts b/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts index fd564e5890..9c01d8bbbf 100644 --- a/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts +++ b/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts @@ -16,7 +16,7 @@ describe('Fix PR', () => { ).toEqual( expect.objectContaining({ code: 0, - stdout: expect.stringContaining('Successfully applied Snyk patches'), + stdout: expect.stringContaining('Applied Snyk patches.'), stderr: expect.any(String), }), ); diff --git a/packages/snyk-protect/test/acceptance/protect.spec.ts b/packages/snyk-protect/test/acceptance/protect.spec.ts index 2fc579dc99..6a75f0f049 100644 --- a/packages/snyk-protect/test/acceptance/protect.spec.ts +++ b/packages/snyk-protect/test/acceptance/protect.spec.ts @@ -29,7 +29,7 @@ describe('@snyk/protect', () => { project.read('node_modules/nyc/node_modules/lodash/lodash.js'), ).resolves.toEqual(patchedLodash); - expect(log).toHaveBeenCalledWith('Successfully applied Snyk patches'); + expect(log).toHaveBeenCalledWith('Applied Snyk patches.'); expect(postJsonSpy).toHaveBeenCalledTimes(1); expect(postJsonSpy.mock.calls[0][1]).toEqual({ data: { @@ -68,7 +68,7 @@ describe('@snyk/protect', () => { project.read('node_modules/lodash/lodash.js'), ).resolves.toEqual(patchedLodash); - expect(log).toHaveBeenCalledWith('Successfully applied Snyk patches'); + expect(log).toHaveBeenCalledWith('Applied Snyk patches.'); expect(postJsonSpy).toHaveBeenCalledTimes(1); expect(postJsonSpy.mock.calls[0][1]).toEqual({ data: { @@ -107,7 +107,7 @@ describe('@snyk/protect', () => { await protect(project.path()); - expect(log).toHaveBeenCalledWith('Nothing to patch'); + expect(log).toHaveBeenCalledWith('Nothing to patch.'); expect(postJsonSpy).toHaveBeenCalledTimes(1); expect(postJsonSpy.mock.calls[0][1]).toEqual({ @@ -142,7 +142,7 @@ describe('@snyk/protect', () => { await protect(project.path()); - expect(log).toHaveBeenCalledWith('No .snyk file found'); + expect(log).toHaveBeenCalledWith('No .snyk file found.'); expect(postJsonSpy).toHaveBeenCalledTimes(1); expect(postJsonSpy.mock.calls[0][1]).toEqual({ From c9ddb4436d2ddbb901c5276a4cf3f38f6036edd3 Mon Sep 17 00:00:00 2001 From: Jahed Ahmed Date: Thu, 19 Aug 2021 18:16:37 +0000 Subject: [PATCH 2/4] chore(protect): move api url warnings to stderr --- packages/snyk-protect/src/lib/snyk-api.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snyk-protect/src/lib/snyk-api.ts b/packages/snyk-protect/src/lib/snyk-api.ts index 76a26417e5..4c422ef7d2 100644 --- a/packages/snyk-protect/src/lib/snyk-api.ts +++ b/packages/snyk-protect/src/lib/snyk-api.ts @@ -7,7 +7,7 @@ export function getApiBaseUrl(): string { // snyk CI environment - we use `.../api/v1` though the norm is just `.../api` apiBaseUrl = process.env.SNYK_API.replace('/v1', ''); } else { - console.log( + console.warn( 'Malformed SNYK_API value. Using default: https://snyk.io/api', ); } From ca2177abccd89f54668950e173e5acd7fa24cfe7 Mon Sep 17 00:00:00 2001 From: Jahed Ahmed Date: Thu, 19 Aug 2021 18:20:07 +0000 Subject: [PATCH 3/4] fix(protect): catch and log unexpected errors Otherwise we'll get "UnhandledPromiseRejectionWarning" warnings from NodeJS which warns us that their behaviour will change to non-zero exit codes in the future. We don't want that and we shouldn't block pipelines due to failures on our end. --- packages/snyk-protect/bin/snyk-protect | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/snyk-protect/bin/snyk-protect b/packages/snyk-protect/bin/snyk-protect index 0eef1829f1..58e501654c 100755 --- a/packages/snyk-protect/bin/snyk-protect +++ b/packages/snyk-protect/bin/snyk-protect @@ -1,2 +1,7 @@ #!/usr/bin/env node -require('../dist/index.js').protect(); +require('../dist/index.js') + .protect() + .catch((error) => { + // don't block pipelines on unexpected errors + console.error(error); + }); From 5e824c08abcffdb50cfc85e573e22431ee0f9ed5 Mon Sep 17 00:00:00 2001 From: Jahed Ahmed Date: Thu, 19 Aug 2021 12:14:51 +0000 Subject: [PATCH 4/4] fix(protect): skip previously patched files --- packages/snyk-protect/src/lib/patch.ts | 7 +++++++ .../test/acceptance/fix-pr.smoke.spec.ts | 18 +++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/snyk-protect/src/lib/patch.ts b/packages/snyk-protect/src/lib/patch.ts index f6a1534ff8..1983b7fe49 100644 --- a/packages/snyk-protect/src/lib/patch.ts +++ b/packages/snyk-protect/src/lib/patch.ts @@ -6,9 +6,16 @@ export function applyPatchToFile(patchContents: string, baseFolder: string) { baseFolder, extractTargetFilePathFromPatch(patchContents), ); + + const flagPath = `${targetFilePath}.snyk-protect.flag`; + if (fs.existsSync(flagPath)) { + return targetFilePath; + } + const contentsToPatch = fs.readFileSync(targetFilePath, 'utf-8'); const patchedContents = patchString(patchContents, contentsToPatch); fs.writeFileSync(targetFilePath, patchedContents); + fs.writeFileSync(flagPath, ''); return targetFilePath; } diff --git a/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts b/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts index 9c01d8bbbf..46c0e7685d 100644 --- a/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts +++ b/packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts @@ -9,6 +9,22 @@ describe('Fix PR', () => { const project = await createProject('fix-pr'); const patchedLodash = await getPatchedLodash(); + expect( + await runCommand('npm', ['install'], { + cwd: project.path(), + }), + ).toEqual( + expect.objectContaining({ + code: 0, + stdout: expect.stringContaining('Applied Snyk patches'), + stderr: expect.not.stringMatching(/snyk/gi), + }), + ); + + await expect( + project.read('node_modules/lodash/lodash.js'), + ).resolves.toEqual(patchedLodash); + expect( await runCommand('npm', ['install'], { cwd: project.path(), @@ -17,7 +33,7 @@ describe('Fix PR', () => { expect.objectContaining({ code: 0, stdout: expect.stringContaining('Applied Snyk patches.'), - stderr: expect.any(String), + stderr: expect.not.stringMatching(/snyk/gi), }), );