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

Remove pstree.remy #1454

Closed
wants to merge 2 commits into
base: master
from

Conversation

3 participants
@FallingSnow
Copy link

FallingSnow commented Nov 21, 2018

This is a bloated dependency that (down the stream) depends on a dependency that is running unknown and clearly malicious code on everyone's system.

FallingSnow added some commits Nov 21, 2018

fix: Update audit dependencies and remove pstree.remy
pstree.remy has a dependency with a compromised dependency

Fixes: #1442
Fixes: #1451
@jaydenseric

This comment has been minimized.

Copy link

jaydenseric commented Nov 21, 2018

Please lets expedite a review and merge! It's unbelievable a hack affecting millions of installations is not being acknowledged or dealt with by maintainers 🤯

@remy

This comment has been minimized.

Copy link
Owner

remy commented Nov 22, 2018

maintainer - singular.

You've commented out a bunch of tests. Can you explain why? (I don't know why the PR is on nodemon and not my pstree, but I'll review the change to make sure the swapped in module does the same)

@remy

This comment has been minimized.

Copy link
Owner

remy commented Nov 22, 2018

You've also bumped the semantic release several major versions, and the auto deployment depends entirely on that code - and has not been tested with major versions out...

Bulking together these changes is delaying the release.

@remy

This comment has been minimized.

Copy link
Owner

remy commented Nov 22, 2018

Right - reviewing this properly, which I'm guessing never happened, this can't be merged.

The reason I'm using pstree.remy - my own fork, is (as it clearly states in the description of the module) is that it's able to get the process tree without ps - which a bunch of systems (usually inside docker) use. kill-tree relies on ps so this would break again.

The fix is to PR against pstree.remy and to remove the ps-tree dep there.

@remy remy closed this Nov 22, 2018

@FallingSnow

This comment has been minimized.

Copy link

FallingSnow commented Nov 22, 2018

You've commented out a bunch of tests.

I've only commented out one test, should disable colour for the logger. It was continuously failing and in my debugging seemed to be caused by the async nature of javascript and how the test was written (The bus.onces were receiving the wrong log, I assumed because of this process.nextTick), therefore it seemed the test would need to be rewritten to accommodate for this. This wasn't my MO so I just removed it.

You've also bumped the semantic release several major versions

This was to address npm audit which reported:

found 42 vulnerabilities (21 low, 15 moderate, 5 high, 1 critical)
  run `npm audit fix` to fix them, or `npm audit` for details

The bump to the dependencies results in

found 3 low severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details

I can remove the major version bumps to mocha and semantic-release if you'd like.

it's able to get the process tree without ps - which a bunch of systems (usually inside docker) use. kill-tree relies on ps so this would break again.

I see. Then I suppose the actual removal of ps-tree should happen there.

@remy

This comment has been minimized.

Copy link
Owner

remy commented Nov 22, 2018

npm audit is great, except it's kinda not. It covers dev deps which are a very different kind of dependency than production (which is why I use snyk instead).

I'm pushing a release now that removes the deep dep of ps-tree (fixed in pstree.remy).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment