Add snyk for npm package security

[fhemberger]

This is a cleaned up version of #821. Please check the discussion over there.

/cc @nodejs/build

[fhemberger]

/cc @nodejs/website

Can we merge this one? Yay? Nay?

[lpinca]

I'm still -0.

[Starefossen]

What about nsp?

[guypod]

@Starefossen I believe the conversation here is primarily about fixing the issues, which - for these vulnerabilities - is only doable by using Snyk's patches.

There are various other reasons to use Snyk over nsp for testing purposes (clearly I'm biased...), but either way nsp wouldn't help with the remediation discussed here.

[ghost]

i'd say merge it now to avoid security loopholes and contemplate on it later on. LGTM

[lpinca]

Did we ever try to see if we were actually affected by the minimatch vulnerability?

[lpinca]

Please correct me if I'm wrong but given

├─┬ chokidar@1.6.0
│ ├─┬ fsevents@1.0.14
│ │ └─┬ node-pre-gyp@0.6.29
│ │   └─┬ rimraf@2.5.3
│ │     └─┬ glob@7.0.5
│ │       └── minimatch@3.0.2 
│ └─┬ readdirp@2.1.0
│   └── minimatch@3.0.3 
├─┬ htmllint-cli@0.0.5
│ └─┬ htmllint@0.3.0
│   └─┬ bulk-require@0.2.1
│     └─┬ glob@3.2.11
│       └── minimatch@0.3.0 
├─┬ metalsmith@2.1.0
│ └─┬ recursive-readdir@1.3.0
│   └── minimatch@0.3.0 
├─┬ metalsmith-collections@0.7.0
│ └── minimatch@0.2.14 
└─┬ metalsmith-stylus@2.0.0
  ├── minimatch@2.0.10 
  └─┬ stylus@0.52.4
    └─┬ glob@3.2.11
      └── minimatch@0.3.0

I doubt we were vulnerable as I can't see how a malicious input could be provided to minimatch once the site was built. The site is static and once built is served via nginx.
I think we've only added bloat right now but automatic patching is still nice for future vulnerabilities.

[lpinca]

@guypod is running the wizard the only way to update the policy file?

[Starefossen]

Will snyk block/patch vulnerabilities automatically during runtine, or will we have to update the policy file and then redeploy?

On 11. aug. 2016, at 15:16, Luigi Pinca notifications@github.com wrote:

@guypod is running the wizard the only way to update the policy file?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[guypod]

@lpinca the wizard is the only tool that automatically edits it, but you can manually edit it too. Format is probably self explanatory but happy to answer questions about it.

You can also run snyk policy after any manual edits to confirm it worked well and see the human readable flow.

[guypod]

@Starefossen to patch, Snyk adds the snyk protect command as a prepublish hook. This means every time this project installs its dependencies, or every time someone installs this project via npm install <proj>, it will run.

By the time snyk protect runs, the original (vulnerable) package is already installed, and Snyk applies the (open-source) patches to it to fix the vulnerability. It will only apply the patches specified in the .snyk policy file. These are actual patches, you can see links to the patches used for every version range at the bottom of the vulnerability page: https://snyk.io/vuln/npm:minimatch:20160620

More details on our docs: https://snyk.io/docs/using-snyk/#protect

[guypod]

@Starefossen I realized I didn't explicitly answer your question: the short answer is you need to update the .snyk file, ensure you run snyk protect in the prepublish or (a bit more error prone) postinstall phase, and redeploy.

[lpinca]

This means updating the whole dependency tree and the policy file before each commit for us. Also I guess the policy file is based on the dependency tree so the npm version used for development must match with the npm version used on the remote server during the deployment phase.

[guypod]

@lpinca I might be missing something here... the .snyk file specifies a dependency path, and will only patch that. You can choose to include a more loose path (e.g. A -> B -> C, * -> B -> C or * -> C), to clarify which cases you want to patch. We default to the more cautious path of only patching the specific paths we knew. It's not sensitive to the specific version.

The patching happens each time you run snyk protect, and won't happen twice.
You can run snyk protect yourself in a deployed env and it'll patch what's needed, or you can run npm install to just update stuff and it'll also run the snyk protect through the prepublish hook.

In other words, there shouldn't be any need for you to refresh your deploys any more than you did before. You just need to ensure snyk protect runs.

[lpinca]

@guypod I may be the one missing something here 😄 but I'll try to clarify my thoughts.

1. Assume that I have A -> B -> C and C is vulnerable so I build the policy file and that path is added to it. After some time C is updated so it should no longer be patched. When this happens a new policy file should be built or and attempt to patch C will be made even if it is no longer vulnerable.
2. Again assume that I have A -> B -> C and C is vulnerable. The policy file includes that path. Now I deploy and npm builds a different tree (because it's npm 2.x) where the path A -> B -> C does not exists. Is C patched in this case?

[guypod]

You're right that the solution is a moving target. Which is the reason we created Snyk :)

My advice would be to "Watch" this project with Snyk (free for open source), and you'll get pull requests both on fixes to new vulns and (still a bit of a WIP) when a better remediation is made available.

Maybe try that out?

[guypod]

@lpinca regarding the specific scenarios:

1. If you upgraded your direct dependency, and now C no longer has the vulnerability, Snyk will identify it as a non-vulnerable path and not apply the patch. Ideally you'd also remove the patch from the .snyk file (hence my comment on using Snyk), but it won't apply a patch on a version that has no fix (each patch is associated with a version range, and the fixed versions have no patch).

2. In this case, C isn't patched. One option is to expand the policy file to patch all instances of C. The better path is to integrate Snyk with your project, spot the fact you're adding a vulnerable path before merging, and either patching there or upgrading/not-adding the new dependency.

Makes sense?

[lpinca]

@guypod yes it does but AFAIK we can't grant access to the Synk application. See #821 (comment).

As I see it, right now a lot of effort is necessary to keep the .snyk file in a consistent and useful state.

[Starefossen]

I second that @lpinca. Imho it looks like the work of maintaining an effective snyk configuration is greater than the received benefit.

[guypod]

@lpinca @phillipj can I ask what are the reason you don't want to integrate Snyk to the org?

I understand you removed Greenkeeper, but Snyk is quite different, notably sending far fewer PRs (only vuln related ones) and has controls stating what you do and don't want reported on.

Happy to do a quick hangout to talk through it if it's a bit much for GitHub comment conversation

[phillipj]

@guypod it's a nodejs org security policy to not add 3rd party github applications which requests private access. All the gh apps we have been wanting to add so far, has been asking for private access.

It would be okey for us if we could setup a webhook against snyk manually for this repo alone, instead of org wide permissions. Would that be a possibility?

P.S. we didn't turn off greenkeeper cause we wanted to, it required org wide permissions which we couldn't grant it.

[guypod]

@phillipj understood. What others have done to achieve this is to:

• Create a new GitHub user
• Give that user access only to the public repos you want tested
• Sign up to Snyk via that user

Snyk will then use only this user's token, and so will not have access to the private repos.

Would this work here?