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

use fs.lchown rather than fs.chown and thereby fix #14 #15

Closed
wants to merge 5 commits into from

Conversation

simevo
Copy link

@simevo simevo commented Aug 10, 2018

fixes the symlinks problem #3 while not causing the TOCTOU vulnerability #14

The patch in libuv 1.21.0 that undeprecates fs.lchown has been incorporated in nodejs Version 10.6.0.

So I specified the minimum nodejs version in package.json with the engine key: https://docs.npmjs.com/files/package.json#engines

fixes the symlinks problem isaacs#3 while not causing the TOCTOU vulnerability isaacs#14

The [patch in libuv 1.21.0](https://github.com/libuv/libuv/releases/tag/v1.21.0) that undeprecates `fs.lchown` [has been incorporated in nodejs Version 10.6.0](https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V10.md#2018-07-04-version-1060-current-targos).

So I specified the minimum nodejs version in `package.json` with the `engine` key: https://docs.npmjs.com/files/package.json#engines
@@ -19,5 +19,6 @@
"scripts": {
"test": "tap test/*.js"
},
"license": "ISC"
"license": "ISC",
"engines": { "node" : ">=10.6.0" }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to change the .travis.yml to match.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe update that list to match the maintained Node versions?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The job exceeded the maximum time limit for jobs, and has been terminated.

no wonder, it decided to build nodejs 10.6.0 from source !

@simevo
Copy link
Author

simevo commented Aug 11, 2018

Ok now the tests are running on nodejs 10.8 and we get several errors, of 3 types:

  • Error: TypeError [ERR_INVALID_ARG_TYPE]: The "gid" argument must be of type number. Received type undefined
  • Error: gid should be undefined
  • Error: uid not should be 0

the very same 30 tests are failing on master; see: #16

@morkro
Copy link

morkro commented Sep 6, 2018

Since this seems to get no attention from the project maintainer, is there a way to help?
What guarantees this will even be merged if all checks are successful again?

@isaacs isaacs closed this Sep 16, 2018
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.

4 participants