feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355

adrukh · 2019-02-01T08:55:17Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Applying patches on snyk protect requires the patch binary to be available. This is problematic especially on Windows envs where GNU binaries are not installed by default.

With the help of @saschanaz in #335 we're giving jsdiff another go, with the intent to remove the dependency on the patch binary, and apply patches with pure JS.

adrukh · 2019-02-01T15:19:54Z

I tested this manually by applying the patch and the jsdiff versions against https://github.com/snyk/goof, comparing the resulting node_modules folder. The only difference has been the content of the 'patch flag' files, which is expected (they contain the timestamp of patching).

We will huddle internally on how to best move forward in the least risky way. Really excited about this opportunity, thanks again @saschanaz !

miiila · 2019-02-12T14:31:55Z

Hi @saschanaz, I just wanna let you know we haven't forgotten your awesome contribution and we've proceed a bit more with internal testing and we hope to merge this PR soon.

Thank you again for your help!

michael-go · 2019-02-17T08:25:41Z

src/lib/protect/apply-patch.js

@@ -1,6 +1,7 @@
 module.exports = applyPatch;

 var debug = require('debug')('snyk');
+var diff = require('diff');


we added the @types pkg for it, so why not .ts it? 😉

michael-go · 2019-02-17T08:26:41Z

package.json

@@ -51,11 +51,13 @@
  "dependencies": {
    "@snyk/dep-graph": "1.1.2",
    "@snyk/gemfile": "1.1.0",
+    "@types/diff": "^3.5.2",


why this, and the other types are actually here and not in devDependencies ? 🤔

yep - lodash & node types are in devDependencies so all these are here by mistake

I'll move this.

michael-go · 2019-02-17T08:28:31Z

src/lib/protect/apply-patch.js

-        if (error || out.indexOf('FAILED') !== -1) {
-          debug('patch command failed', relative, error, out);
-          return patchError(error, out, relative, vuln).catch(reject);
+function jsDiff(patchContent, relative, live) {


what is live ? I see we had such a var before, but maybe it's a chance to improve the naming?

live is a boolean that decides whether the patch result should be written to the file system or not.

Cool, thanks. Would suggest renaming it to isDryRun (and reversing the logic checks 😅).

This goes way up into the flow (https://github.com/snyk/snyk/blob/master/src/cli/commands/protect/index.js#L78), used in wizard flow too. Suggest to change this on a separate chore PR.

michael-go · 2019-02-17T09:10:03Z

src/lib/protect/apply-patch.js

+    diff.applyPatches(patchContent, {
+      loadFile: function (index, callback) {
+        try {
+          var fileName = stripFirstSlash(index.oldFileName);


would suggest add comment with a sample path(s), to illustrate why we do this.

michael-go · 2019-02-17T09:15:16Z

src/lib/protect/apply-patch.js

-          debug('patch command failed', relative, error, out);
-          return patchError(error, out, relative, vuln).catch(reject);
+function jsDiff(patchContent, relative, live) {
+  const files = {};


maybe rename to patchedFiles?

Doing this.

src/lib/protect/apply-patch.js

adrukh · 2019-02-17T10:53:14Z

Force-pushed after rebasing over master and attending to the majority of the comments.
@saschanaz please see last two comments re diff operations and how we can document them.

adrukh · 2019-02-17T11:15:46Z

@michael-go can you please ✅ so I can squash the commits and merge this? ✨

snyksec · 2019-02-17T11:51:09Z

🎉 This PR is included in version 1.131.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

adrukh · 2019-02-18T20:09:09Z

@saschanaz 👋
We've been running with this change since Sunday, monitoring our success vs failure rates. We are seeing some rare cases where the patching logic is not behaving properly. We had quite limited visibility into patching failures until this change, so it's hard to tell whether this is a new case, or simply deeper visibility into an older issue.

I was wondering if you could shed some light on the situation as we see it. Sometimes, we end up processing an empty hunk. This code runs - https://github.com/snyk/snyk/blob/49dc318db8a5e46bb324c10909239d954cdd3217/src/lib/protect/apply-patch.js#L46-L59 - with the index var being just { "hunks": [] }. We bail out as the index.oldFileName is undefined.

This is happening for our users here and there, but we are not able to reproduce the issue locally. Any thoughts about this would be greatly appreciated 🙏

saschanaz · 2019-02-18T23:47:17Z

@adrukh

https://github.com/kpdecker/jsdiff/blob/2c1a29c0309fef22219bcf2cbb475f20508a955b/src/patch/parse.js#L55-L69

Looks like it may happen when a patch file has no header. Is there an headerless patch on snyk?

adrukh · 2019-02-19T04:15:14Z

Hmmm

Not only do we not have headerless patches, but this is also not persistent at all in terms of reproduction.

Could this be a race condition when we have yet created the patch file locally, and are already trying to read it? Will check!

saschanaz · 2019-02-19T05:15:18Z

src/lib/protect/apply-patch.js

+            if (patchedFiles[fileName] === null) {
+              fs.unlinkSync(path.resolve(relative, fileName));
+            }
+            fs.writeFileSync(path.resolve(relative, fileName), patchedFiles[fileName]);


This line looks like a potential bug generator where null would trigger removal and then rewrite.

This area writes the patched dependency source back to the disk, seems ok :)

The value null was intended to delete the file (hence the unlink) but here it instead overwrites with the text null.

Aha! I see your point. Will make this safer.

See 908bc3a

adrukh · 2019-02-19T07:08:44Z

@saschanaz thanks for your assistance here! I think we found the culprit - indeed a race condition where we resolve fetching the patch once the data has been downloaded, but not once it has been written to disk. See https://github.com/snyk/snyk/blob/master/src/lib/protect/fetch-patch.js#L33-L35

We're going to fix this now, have been carrying this problem with us for a very long time! 😌

adrukh self-assigned this Feb 1, 2019

adrukh force-pushed the feat/jsdiff branch 2 times, most recently from 37e9ac6 to 655537e Compare February 1, 2019 09:00

michael-go reviewed Feb 17, 2019

View reviewed changes

src/lib/protect/apply-patch.js Show resolved Hide resolved

michael-go reviewed Feb 17, 2019

View reviewed changes

src/lib/protect/apply-patch.js Show resolved Hide resolved

adrukh force-pushed the feat/jsdiff branch from 655537e to 6f2ba8b Compare February 17, 2019 10:52

michael-go approved these changes Feb 17, 2019

View reviewed changes

feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries

3e5838a

adrukh force-pushed the feat/jsdiff branch from fa9f38a to 3e5838a Compare February 17, 2019 11:26

adrukh merged commit 90b0830 into master Feb 17, 2019

adrukh deleted the feat/jsdiff branch February 17, 2019 11:38

snyksec added the released label Feb 17, 2019

adrukh restored the feat/jsdiff branch February 17, 2019 15:16

adrukh deleted the feat/jsdiff branch February 17, 2019 18:54

saschanaz reviewed Feb 19, 2019

View reviewed changes

saschanaz mentioned this pull request Feb 24, 2019

Build command hates Windows speced/respec#1482

Closed

snyk-bot mentioned this pull request Mar 9, 2022

[Snyk] Upgrade nconf from 0.7.2 to 0.11.3 #2938

Closed

ariel-henryson mentioned this pull request Jul 27, 2022

[Snyk-dev] Security upgrade nconf from 0.7.2 to 0.11.4 #3496

Closed

snyk-bot mentioned this pull request Aug 30, 2022

[Snyk] Upgrade nconf from 0.7.2 to 0.12.0 #3697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355

feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355

adrukh commented Feb 1, 2019

adrukh commented Feb 1, 2019

miiila commented Feb 12, 2019

michael-go Feb 17, 2019

michael-go Feb 17, 2019

michael-go Feb 17, 2019

adrukh Feb 17, 2019

michael-go Feb 17, 2019

saschanaz Feb 17, 2019

michael-go Feb 17, 2019

adrukh Feb 17, 2019

michael-go Feb 17, 2019

michael-go Feb 17, 2019

adrukh Feb 17, 2019

adrukh commented Feb 17, 2019

adrukh commented Feb 17, 2019

snyksec commented Feb 17, 2019

adrukh commented Feb 18, 2019 •

edited

Loading

saschanaz commented Feb 18, 2019 •

edited

Loading

adrukh commented Feb 19, 2019

saschanaz Feb 19, 2019

adrukh Feb 19, 2019

saschanaz Feb 19, 2019 •

edited

Loading

adrukh Feb 19, 2019

adrukh Feb 19, 2019 •

edited

Loading

adrukh commented Feb 19, 2019

feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355

feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355

Conversation

adrukh commented Feb 1, 2019

What does this PR do?

adrukh commented Feb 1, 2019

miiila commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrukh commented Feb 17, 2019

adrukh commented Feb 17, 2019

snyksec commented Feb 17, 2019

adrukh commented Feb 18, 2019 • edited Loading

saschanaz commented Feb 18, 2019 • edited Loading

adrukh commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrukh Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

adrukh commented Feb 19, 2019

adrukh commented Feb 18, 2019 •

edited

Loading

saschanaz commented Feb 18, 2019 •

edited

Loading

saschanaz Feb 19, 2019 •

edited

Loading

adrukh Feb 19, 2019 •

edited

Loading