Feat/lockfile parser lib #188

miiila · 2018-08-09T11:23:46Z

Moved from https://github.com/snyk/snyk-internal/pull/417

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Adds in a fork for opt-in lockfile testing
Includes the new lockfile parser lib
Splits up some functionality for re-use

Assuming the https://github.com/snyk/nodejs-lockfile-parser/pull/5/files returns a tree the way we expect, i hope this is enough code to be able to get back a tree in the same format and carry on the test in the same way as before.

We are missing tests for this, need to get 1 in at least!

Where should the reviewer start?

Look in snyk-test/npm/index.js

How should this be manually tested?

snyk-dev test -d => will test using the old flow by looking at your node_modules
snyk-dev test -d --file=package-lock.json will go down the new flow and use the new lib

Any background context you want to provide?

What are the relevant tickets?

SC-5873

CLAassistant · 2018-08-09T11:23:52Z

All committers have signed the CLA.

For now we only support --file=package-lock.json as an opt in for lockfile tests

michael-go

please make sure this branch works also with snyk monitor

michael-go · 2018-08-14T07:31:10Z

lib/snyk-test/npm/index.js

+  var shrinkwrapFullPath = path.resolve(root, 'npm-shrinkwrap.json');
+
+  if (!fileSystem.existsSync(targetFileFullPath)) {
+    throw new Error('LockFile package.json not found at location: ' +


a typo - package.json is not a lock

michael-go · 2018-08-14T07:36:15Z

lib/snyk-test/npm/index.js

+
+function generateDependenciesFromLockfile(root, options) {
+  debug('Lockfile detected, generating dependency tree from lockfile');
+  var targetFileFullPath = path.resolve(root, 'package.json');


let's think about the case of monorepo and make sure it behaves as we expect, in example what about a case where user runs:

snyk test --file=node-subproject/package-lock.json

in this case the pacakge.json should be picked from the same location of the lock, but maybe it's ok as the user can also run:

snyk test node-project --file=package-lock.json

defo need to make sure that the correct lockfile is always used (according to NPM rules)

michael-go · 2018-08-14T07:39:49Z

lib/snyk-test/npm/index.js

+  var targetFile = fileSystem.readFileSync(targetFileFullPath);
+  var lockFile = fileSystem.readFileSync(lockFileFullPath);
+
+  analytics.add('local', true);


if we'er at it, maybe also add analytics metadata for the fact a lockfile is used?

Thank you - I had it in mind, but then I forgot :-)

michael-go · 2018-08-14T07:40:19Z

lib/snyk-test/npm/index.js

+  analytics.add('local', true);
+
+  var resolveModuleSpinnerLabel = 'Analyzing npm dependencies for ' +
+  lockFileFullPath;


an indentation is needed here

michael-go · 2018-08-14T07:46:19Z

lib/snyk-test/npm/index.js

+  var resolveModuleSpinnerLabel = 'Analyzing npm dependencies for ' +
+  lockFileFullPath;
+  debug(resolveModuleSpinnerLabel);
+  var payload = {


is it necessary to clone the payload creation (and this is not only the payload, but parameters for the http request) and policy collection into generateDependenciesFromLockfile() ? Would expect this function to just generate a tree and continue with the flow that already exists

We agreed with @lili2311 that refactoring to better shape will happen later (will be planned properly) and with upcoming work around yarn.lock.

I can try and help refactor this, don't think we should merge a PR with a code that is not in good shape - unless there's a really good reason for it to be

@michael-go 👋 We had this chat already with Mila yesterday, please see https://snyk.slack.com/archives/CBXBQHN6A/p1534157719000072 We will be refactoring this but in the next few PRs

We are happy with out decision to hold off the refactoring for this PR, this way we gain more knowledge over the next few PRs & features like supporting different use cases and yarn.lock to help us understand how we should restructure this file to make the best of it.

michael-go · 2018-08-14T07:48:38Z

package.json

@@ -48,6 +48,7 @@
    "snyk-gradle-plugin": "1.3.0",
    "snyk-module": "1.8.2",
    "snyk-mvn-plugin": "1.2.0",
+    "snyk-nodejs-lockfile-parser": "^1.2.2",


note we always pin our own dependencies to an exact version - maybe we shouldn't, but easier to stick to what we already do 😅

Hmm, I am not sure I am happy with this approach, however I can see the reasons... Will do it for now.

@miiila It means that the version of the main CLI library tells us the exact state of the Snyk modules on a user's machine. Very useful for debugging and support.

@gjvis And for those situations, when you break the flow by fixing the bug 😇

michael-go · 2018-08-14T07:49:17Z

test/acceptance/cli.acceptance.test.js

+      var pkg = req.body;
+      t.equal(req.method, 'POST', 'makes POST request');
+      t.match(req.url, '/vuln/npm', 'posts to correct url');
+      t.ok(pkg.dependencies['to-array'], 'dependency');


would check a transitive dependency too

There's no transitive dependecny right now :-(

so lets use a better fixture ...

michael-go · 2018-08-14T07:50:57Z

test/acceptance/cli.acceptance.test.js

+      t.includes(e.message, '--file=npm-shrinkwrap.json', 'Contains enough info about error');
+    });
+});
+
 test('`test` on a yarn package does work and displays appropriate text',


maybe also add a test that if lockfile exists but not given via --file it's not used?

michael-go · 2018-08-14T07:57:54Z

lib/snyk-test/npm/index.js

+
+  if (options.file.endsWith('package-lock.json') && fileSystem.existsSync(shrinkwrapFullPath)) {
+    throw new Error('`npm-shrinkwrap.json` was found while using package-lock.json.\n'
+    + 'Please use `--file=npm-shrinkwrap.json` flag instead.');


and we support that?

We don't and we will not => changing the message.

…riknwrap

lili2311 · 2018-08-14T11:29:29Z

lib/snyk-test/npm/index.js

+  var shrinkwrapFullPath = path.resolve(root, 'npm-shrinkwrap.json');
+
+  if (!fileSystem.existsSync(targetFileFullPath)) {
+    throw new Error('LockFile package.json not found at location: ' +


LockFile => Lockfile

michael-go · 2018-08-14T11:30:21Z

lib/snyk-test/npm/index.js

+      return lockFileParser.buildDepTree(manifestFile, lockFile, options);
+    })
+    .then(function (pkg) {
+      modules = pkg;


my comment here became outdated with some recent changes - but still, this needs refactoring. Why duplicate the http requests params creation & policy file collection code?

and also the name getDependenciesFrom... is misleading as the functions also query for vulns.

michael-go · 2018-08-14T13:19:02Z

lib/snyk-test/npm/index.js

@@ -11,20 +11,124 @@ var isCI = require('../../is-ci');
 var _ = require('lodash');
 var analytics = require('../../analytics');
 var common = require('../common');
+var fileSystem = require('fs');


any reason you're not calling the var fs like we always do?

michael-go · 2018-08-15T15:03:11Z

lib/snyk-test/npm/index.js

+  options.root = root;
+
+  // if the file exists, let's read the package file and post
+  // that up to the server.


the comment is misleading, as we'll upload the fully resolved dependency tree, not the package file

michael-go · 2018-08-15T15:09:09Z

lib/snyk-test/npm/index.js

-            if (error) {
-              return reject(error);
-            }
+  return queryForVulns(p, modules, options);


maybe it will be nicer to do:

return p.then(function(payload) { return queryForVulns(payload, modules, options); })

or even add it to he promise chain above,
instead of passing a promise as a parameter to queryVulns

I agree (and will address), however, this exactly the thin ice, when you are refactoring and suddenly realise, you are changing behaviour :-D

michael-go · 2018-08-15T15:21:35Z

lib/snyk-test/npm/index.js

+    });
+}
+
+function queryForVulns(p, modules, options) {


note that in the lockfile case - modules is just the pkgTree, but in the node_modules case it's an object with methods and such .. and in this function .pluck() is called on it and .numDependencies is checked

michael-go · 2018-08-15T15:38:14Z

lib/snyk-test/npm/index.js

+    },
+  };
+  options.hasDevDependencies = false;
+  options.root = root;


this doesn't look right - if we do this to pass data down to queryForVulns(), lets instead just use extra parameters for it, and not "abuse" the options object

Well, not sure if I agree - what is the options object for then? I am not a big fan of methods like queryForVulns(data, modules, hasDevDependencies, root, options).

michael-go

Approving but not happily - would prefer us to wait for a fixed lockfile-parser lib that doesn't hang on cycles

snyksec · 2018-08-16T18:32:42Z

🎉 This PR is included in version 1.91.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

miiila force-pushed the feat/lockfile-parser-lib branch from c651996 to c16e64d Compare August 9, 2018 11:58

miiila added the 🐣 in progress label Aug 9, 2018

miiila force-pushed the feat/lockfile-parser-lib branch from c16e64d to 9d4035b Compare August 11, 2018 13:22

lili2311 and others added 5 commits August 13, 2018 11:49

feat: opt-in to package-lock test by pointing to lockfile

4ccfdfe

feat: split up some logic for reuse for test

d3a575a

chore: update lockfile parser

6c459ae

chore: remove package-lock.json from auto detection

749a7cf

For now we only support --file=package-lock.json as an opt in for lockfile tests

chore: remove TODO

4d15755

miiila force-pushed the feat/lockfile-parser-lib branch 7 times, most recently from 408390b to f2fc748 Compare August 13, 2018 14:01

miiila removed the 🐣 in progress label Aug 13, 2018

miiila self-assigned this Aug 13, 2018

test: package-lock.json behaves the same as node_modules

f275033

miiila force-pushed the feat/lockfile-parser-lib branch from f2fc748 to f275033 Compare August 13, 2018 14:16

feat: fail when shrinkwrap is present and using lockfile

c0a0a4b

michael-go reviewed Aug 14, 2018

View reviewed changes

Mila Votradovec added 2 commits August 14, 2018 12:17

feat: support nested folders for lockfiles

8cfd8f8

chore: adding analytics + better naming + better error message for sh…

2cc1696

…riknwrap

lili2311 reviewed Aug 14, 2018

View reviewed changes

michael-go reviewed Aug 14, 2018

View reviewed changes

test: add fixture for transitive dependency

407f471

miiila force-pushed the feat/lockfile-parser-lib branch from caa01df to 407f471 Compare August 14, 2018 11:59

test: missing dep in package-lock

587b2da

michael-go reviewed Aug 14, 2018

View reviewed changes

michael-go reviewed Aug 15, 2018

View reviewed changes

Mila Votradovec added 2 commits August 15, 2018 17:25

refactor: remove code duplication

e23735a

chore: removing not-needed files

f11b79d

miiila force-pushed the feat/lockfile-parser-lib branch from a63bd8d to f6f360a Compare August 15, 2018 16:26

refactor: removing non-needed promises + moving to dedicated parameters

5cf157c

miiila force-pushed the feat/lockfile-parser-lib branch from f6f360a to 5cf157c Compare August 15, 2018 16:37

michael-go approved these changes Aug 16, 2018

View reviewed changes

miiila merged commit 88905df into master Aug 16, 2018

miiila deleted the feat/lockfile-parser-lib branch August 16, 2018 16:02

miiila mentioned this pull request Sep 21, 2018

Add yarn lock support #221

Merged

3 tasks

snyk-bot mentioned this pull request Oct 4, 2021

[Snyk] Security upgrade jest-junit from 12.2.0 to 13.0.0 MaxMood96/snyk#47

Open

snyk-bot mentioned this pull request Dec 10, 2021

[Snyk] Upgrade snyk-gradle-plugin from 3.2.4 to 3.17.0 thexdesk/snyk#4

Open

leongold mentioned this pull request Nov 30, 2022

[Snyk] Fix for 5 vulnerabilities leongold/snyk#25

Open

This was referenced Nov 30, 2022

[Snyk] Fix for 7 vulnerabilities mikolajroszak/mikolaj.com-snyk#38

Open

[Snyk] Fix for 6 vulnerabilities allRounderWiser/snyk#153

Open

david-j-bailey mentioned this pull request Nov 30, 2022

[Snyk] Fix for 7 vulnerabilities david-j-bailey/snyk#35

Open

snyk-bot mentioned this pull request Dec 1, 2022

[Snyk] Fix for 6 vulnerabilities thexdesk/snyk#14

Open

froscheimer mentioned this pull request Dec 1, 2022

[Snyk] Fix for 6 vulnerabilities froscheimer/snyk#186

Open

snyk-bot mentioned this pull request Dec 1, 2022

[Snyk] Fix for 4 vulnerabilities jeremy001181/snyk#15

Open

sapans30 mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities sapans30/snyk#36

Open

snyk-bot mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities dawit-snyk/snyk#15

Open

hiep98 mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities hiep98/snyk#28

Open

This was referenced Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities toharzand/snyk#41

Open

[Snyk] Fix for 6 vulnerabilities mmmd03/snyk#19

Open

[Snyk] Fix for 7 vulnerabilities laozhudetui/snyk#41

Open

RotemMiara mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities RotemMiara/cli#13

Open

snyk-bot mentioned this pull request Mar 30, 2023

[Snyk] Upgrade boom from 2.10.1 to 7.3.0 info-zezotechnology-com/cli-snyk-zezo-f3c86dc2#54

Open

Feat/lockfile parser lib #188

Feat/lockfile parser lib #188

Conversation

miiila commented Aug 9, 2018 • edited by lili2311 Loading

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

CLAassistant commented Aug 9, 2018 • edited Loading

michael-go left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

michael-go Aug 14, 2018 • edited Loading

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

michael-go Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lili2311 Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

michael-go Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

miiila Aug 14, 2018 • edited Loading

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

michael-go Aug 14, 2018 • edited Loading

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

miiila Aug 15, 2018 • edited Loading

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

michael-go left a comment

Choose a reason for hiding this comment

snyksec commented Aug 16, 2018

miiila commented Aug 9, 2018 •

edited by lili2311

Loading

CLAassistant commented Aug 9, 2018 •

edited

Loading

michael-go left a comment •

edited

Loading

gjvis Aug 14, 2018 •

edited

Loading

michael-go Aug 14, 2018 •

edited

Loading

michael-go Aug 14, 2018 •

edited

Loading

lili2311 Aug 14, 2018 •

edited

Loading

michael-go Aug 14, 2018 •

edited

Loading

gjvis Aug 14, 2018 •

edited

Loading

miiila Aug 14, 2018 •

edited

Loading

michael-go Aug 14, 2018 •

edited

Loading

miiila Aug 15, 2018 •

edited

Loading