fix(protect): skip previously patched files #2175
Conversation
ghost
changed the title
ghost
force-pushed
the
fix/snyk-protect-multiple
branch
2 times, most recently
from
176919a
to
f2d5d86
Compare
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.
ghost
force-pushed
the
fix/snyk-protect-multiple
branch
from
f2d5d86
to
b2ca8a9
Compare
ghost
changed the title
| .catch((error) => { | ||
| // don't block pipelines on unexpected errors | ||
| console.error(error); | ||
| }); |
There was a problem hiding this comment.
This is existing behaviour. Previously we let NodeJS handle "unhandled rejections" which it does by printing a warning and exiting with zero. However, the warnings state they'll change it to a non-zero in the future so it's best that we handle it ourselves. We still exit with zero as errors on our end should not block pipelines.
| @@ -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}`); | |||
There was a problem hiding this comment.
I moved this log line here so it's clear exactly what we're printing to stdout for the user to see in a single top-level file.
| console.log( | ||
| 'Expected\n line from local file\n', | ||
| JSON.stringify(currentLine), | ||
| '\n to match patch line\n', | ||
| JSON.stringify(nextLine), | ||
| '\n', | ||
| ); |
There was a problem hiding this comment.
I moved this into the error message, otherwise it prints out back-to-front which is confusing.
| @@ -94,7 +95,7 @@ async function protect(projectFolderPath: string) { | |||
| }); | |||
| }); | |||
|
|
|||
| console.log('Successfully applied Snyk patches'); | |||
| console.log('Applied Snyk patches.'); | |||
There was a problem hiding this comment.
"Applied" already implies success. Less words, better.
| project = await createProject('fix-pr'); | ||
| patchedLodash = await getPatchedLodash(); | ||
| }); | ||
|
|
||
| test('patches vulnerable dependencies on install', async () => { |
There was a problem hiding this comment.
I tried spliting this test up so that one runs npm install then another runs it again so it's clear what the second run is looking for. However Jest doesn't fail fast, so if the first test fails, it will still execute the second test which wouldn't make sense.
ghost
force-pushed
the
fix/snyk-protect-multiple
branch
from
b2ca8a9
to
5e824c0
Compare
ghost
marked this pull request as ready for review
In the old
snyk protectwe used to add placeholder files to patched modules so indicate we've already patched them. The new snyk/protect doesn't do this so it attempts to patch modules that have already been patched, causing a failure.This PR fixes that by creating the same placeholder file. The difference is that it's simpler and should work with any existing ignore rules a project might use to exclude them in their builds.
snyk protect:[filename].snyk-[vuln-id].flagcontaining a timestamp.Now:
[filename].snyk-protect.flagcontaining nothing.So a project using
*.snyk-*.flagwill still match the same files. Like:https://github.com/signalapp/Signal-Desktop/blob/f96246fecf4855db88d571e5825132ee4d5605c8/package.json#L435
See error logs