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 to get rid of security vulnerabilities #1547

Merged
merged 8 commits into from Sep 25, 2018
Merged

Update dependencies to get rid of security vulnerabilities #1547

merged 8 commits into from Sep 25, 2018

Conversation

stefan-guggisberg
Copy link
Contributor

@stefan-guggisberg stefan-guggisberg commented Sep 18, 2018

running npm install on v0.22.2 ends with the following console output:

found 35 vulnerabilities (6 low, 16 moderate, 12 high, 1 critical)

updating the following dependencies:

  • node-gyp -> ^3.8.0
  • node-pre-gyp -> ^0.11.0
  • coveralls -> ^3.0.2
  • mocha -> ^5.2.0

fixes the following vulnerabilities:

  • hoek < 4.2.1
  • cryptiles < 4.1.2
  • sshpk < 1.13.2
  • growl < 1.10.0

@trieloff
Copy link

@stefan-guggisberg Mocha 3.0.0 changed the behavior for async tests (mochajs/mocha#2407), you cannot return a Promise and call done() at the same time. This causes two of the tests to fail. The solution seems to be remove the return statements here:

https://github.com/stefan-guggisberg/nodegit/blob/bump_node-pre-gyp/test/tests/filter.js#L107

and here:

https://github.com/stefan-guggisberg/nodegit/blob/bump_node-pre-gyp/test/tests/note.js#L61

Not sure what is causing the third test failure.

@stefan-guggisberg
Copy link
Contributor Author

@trieloff thanks. i've fixed those tests in a6a4aab.

now running npm test on the branch gives the exact same result as on the latest release (v0.22.2):

  381 passing (4m)
  4 pending
  3 failing

  1) Clone
       can clone with ssh:
     Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/stefan/projects/nodegit-fork/test/tests/clone.js)


  2) Clone
       can clone with git:
     Error: Error receiving socket data: Operation timed out


  3) Remote
       can reject fetching from private repository without valid credentials:

      AssertionError [ERR_ASSERTION]: Should not be able to find repository.
      + expected - actual

      -callback failed to initialize SSH credentials
      +ERROR: Repository not found.

the same tests are failing on master, they're not broken by this PR.

@Croydon
Copy link
Contributor

Croydon commented Sep 20, 2018

This is redundant of my pull request #1519 which I have referenced in #1499

@Croydon Croydon mentioned this pull request Sep 20, 2018
@maxkorp
Copy link
Collaborator

maxkorp commented Sep 22, 2018

Maybe its redundant @Croydon but is there a reason we can't separate these changes out, to break down the size of that monster a bit? Like would that cause undue conflicts in your branch if we were to merge this then that?

CC @tbranyen @implausible

@Croydon
Copy link
Contributor

Croydon commented Sep 22, 2018

It's fine with me when this gets merged first. I just noted that because it sounds like duplication of already done work.

@implausible
Copy link
Member

I definitely feel like we should probably target these branches instead of having one mega PR, as well. It's a bit intimidating to look at your PR @Croydon.

@Croydon
Copy link
Contributor

Croydon commented Sep 24, 2018

@implausible Yes, I totally agree. I did start my pull request a long time ago and it kept growing as I never got Node 10 to actually work.

Please go ahead and merge this.

@implausible
Copy link
Member

Rerunning the test that failed. Looked like a weird failure to occur as everything passed except the deploy docs noop step.

@implausible implausible merged commit 8d43950 into nodegit:master Sep 25, 2018
@stefan-guggisberg stefan-guggisberg deleted the bump_node-pre-gyp branch September 26, 2018 07:36
@stefan-guggisberg
Copy link
Contributor Author

@implausible thanks! any chance this gets published on npm anytime soon?

@implausible
Copy link
Member

I am hoping to get the node 10 branch wrapped up before we publish the next version, but yes, hopefully soon.

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

5 participants