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.

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

Update dependencies and add nsp check to tests #32

Merged
merged 3 commits into from Mar 15, 2018
Merged

Conversation

mvolz
Copy link
Collaborator

@mvolz mvolz commented Mar 15, 2018

  • Update dependencies
  • Add a security check to tests
  • Increase timeout for all tests to 3s
  • Update minor version number
  • Commit package-lock.json

* Update dependencies
* Add a security check to tests
* Increase timeout for all tests to 3s
* Update minor version number
* Commit package-lock.json
@mvolz mvolz requested a review from Pchelolo March 15, 2018 13:48
@Pchelolo
Copy link
Contributor

Hm... In general, we don't really use shrinkwrap or package-lock and I'm not sure we should begin doing that..

@Pchelolo
Copy link
Contributor

Also seems like NSP uses some es6 syntax, so you need to remove older node versions from the travis.yaml. node 6 and 8 is enough

@mvolz
Copy link
Collaborator Author

mvolz commented Mar 15, 2018

Yes - I've noticed. I think we should start committing them.

For one, nsp is crippled without package-lock.json. I've noticed that I can get repositories to pass nsp that would normally fail if package-lock.json is committed. This is because package-lock has a more specific and complete list of what is actually being installed, including nested repositories.

The other reason is just that it's a standard part of node now, node prompts you to commit it, and everyone else is doing it :). https://docs.npmjs.com/files/package-lock.json

@Pchelolo
Copy link
Contributor

I think it's a bigger discussion regarding package-lock.json, cause if/when we decide to commit them it will affect all the node repos we have so it's a bigger endeavor then just adding it to a single repo. Could you perhaps better file a ticket to discuss this separately from this PR?

What I have against it is that up until now we've relied on specifying all dependencies the ^ so we get minor updates automagically and we don't need to go through all of our packages from time to time, run updates and recommit the package-lock.json. It seems a bit risky but in practice, there was no instance when this approach failed us and something got broken. If we start committing package-lock.json we will be forced to spend a significant amount of time upgrading dependencies from time to time.

As for the NSP - is that because NSP itself is flawed or maybe because of us setting all dependencies with ^ and getting newer already patched versions where the vulnerabilities are fixed?

@mvolz
Copy link
Collaborator Author

mvolz commented Mar 15, 2018

I can open a separate ticket on phabricator, definitely, but I will say that committing it shouldn't represent any extra time - package-lock just represents the last time someone installed it and it was working.

You don't have to increment package.json because of it, in fact the whole point of package-lock.json is that it shows what was the last installed working version was without having to manually update package.json when you are using ^. For packages that pin their repos to a particular version, package-lock.json is actually largely pointless (except for the whole nsp thing).

I haven't debugged the nsp thing extensively but to me it looks like nsp maybe isn't doing a full breadth test on dependencies from package.json. I may open a ticket over there!

@Pchelolo
Copy link
Contributor

Oh, ok, I didn't realize package-lock.json doesn't affect what's actually being installed..

@Pchelolo Pchelolo merged commit 320cf23 into wikimedia:master Mar 15, 2018
@mvolz
Copy link
Collaborator Author

mvolz commented Mar 15, 2018

Yeah, it's just a record of what's been installed and gets recreated every time you do npm install... should I add it back in? Because I did just remove it... :)

@Pchelolo
Copy link
Contributor

Let's do a separate PR cause I've just merged this one :)

@mvolz
Copy link
Collaborator Author

mvolz commented Mar 15, 2018

https://github.com/nodesecurity/nsp/issues/243 for the nsp thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants