Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(protect): skip previously patched files #2175

Merged
4 commits merged into from
Aug 20, 2021
Merged

fix(protect): skip previously patched files #2175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8fed38
refactor(protect): move stdout logs to top level
Aug 19, 2021
c9ddb44
chore(protect): move api url warnings to stderr
Aug 19, 2021
ca2177a
fix(protect): catch and log unexpected errors
Aug 19, 2021
5e824c0
fix(protect): skip previously patched files
Aug 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/snyk-protect/bin/snyk-protect
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
});
Comment on lines +4 to +7
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

9 changes: 5 additions & 4 deletions packages/snyk-protect/src/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
});
Expand All @@ -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}`);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
});
patchedModules.push({
Expand All @@ -94,7 +95,7 @@ async function protect(projectFolderPath: string) {
});
});

console.log('Successfully applied Snyk patches');
console.log('Applied Snyk patches.');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Applied" already implies success. Less words, better.


sendAnalytics({
type: ProtectResultType.APPLIED_PATCHES,
Expand Down
30 changes: 16 additions & 14 deletions packages/snyk-protect/src/lib/patch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,17 @@ 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);
console.log(`patched ${targetFilePath}`);
fs.writeFileSync(flagPath, '');
return targetFilePath;
}

export function extractTargetFilePathFromPatch(patchContents: string): string {
Expand Down Expand Up @@ -39,13 +46,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;

Expand All @@ -66,16 +71,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',
);
Comment on lines -69 to -75
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this into the error message, otherwise it prints out back-to-front which is confusing.

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;
Expand Down
2 changes: 1 addition & 1 deletion packages/snyk-protect/src/lib/snyk-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
}
Expand Down
20 changes: 18 additions & 2 deletions packages/snyk-protect/test/acceptance/fix-pr.smoke.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,24 @@ describe('Fix PR', () => {
).toEqual(
expect.objectContaining<RunCLIResult>({
code: 0,
stdout: expect.stringContaining('Successfully applied Snyk patches'),
stderr: expect.any(String),
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(),
}),
).toEqual(
expect.objectContaining<RunCLIResult>({
code: 0,
stdout: expect.stringContaining('Applied Snyk patches.'),
stderr: expect.not.stringMatching(/snyk/gi),
}),
);

Expand Down
8 changes: 4 additions & 4 deletions packages/snyk-protect/test/acceptance/protect.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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({
Expand Down