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

Stop using process.umask() #1103

Closed
bnoordhuis opened this issue Apr 3, 2020 · 2 comments · Fixed by #2444
Closed

Stop using process.umask() #1103

bnoordhuis opened this issue Apr 3, 2020 · 2 comments · Fixed by #2444
Assignees
Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes

Comments

@bnoordhuis
Copy link
Contributor

Refs: nodejs/node#32321

Summary: process.umask() (no args) will be deprecated and removed.

I couldn't quite divine what lib/config/defaults.js uses process.umask() for but in most cases you don't need to deal with the umask directly - the operating system will apply it automatically.

Example:

const mode = 0o777 & ~process.umask();
fs.mkdirSync(dir, mode);

Computing the file mode that way is superfluous, it can be replaced with just this:

fs.mkdirSync(dir, 0o777);
@isaacs
Copy link
Contributor

isaacs commented Apr 3, 2020

The umask argument defaults to process.umask(). This is passed to pacote, bin-links, and node-tar, which use it to determine the appropriate modes for files and folders in unpacked packages.

For pacote and bin-links, we can default it to 0, since as you say, the process will enforce its own by default. But for node-tar, it's a bit more tricky, as being able to read the default umask allows us to skip a lot of unnecessary stat and chmod calls.

Once the dependencies are no longer using it, we can remove the process.umask() calls from the npm cli without any problem, and just set 0 as the default.

@isaacs
Copy link
Contributor

isaacs commented Apr 3, 2020

One extremely hacky solution we could use to make things work for older npm clients where we might not want to do all the work to unwind old deps depending on process.umask() is just shim it at the start of the CLI process if it's not present.

if (typeof process.umask !== 'function')
  process.umask = () => 0o22

Would mean that debian users don't get group-writable files, but safer than accidentally creating world-writable bins, and better than crashing entirely.

Trott added a commit to Trott/cli that referenced this issue Sep 27, 2020
This has the benefit of removing a `process.umask()` that causes a
runtime deprecation warning if commands such as `npm ls` are run either
with Node.js pending deprecations enabled or with the current 15.0.0 RC.

Refs: nodejs/node#35332 (comment)
Refs: npm#1103
Trott added a commit to Trott/cli that referenced this issue Sep 27, 2020
This has the benefit of removing a `process.umask()` that causes a
runtime deprecation warning if commands such as `npm ls` are run either
with Node.js pending deprecations enabled or with the current 15.0.0 RC.

Refs: nodejs/node#35332 (comment)
Refs: npm#1103
@darcyclarke darcyclarke added the Bug thing that needs fixing label Sep 29, 2020
@darcyclarke darcyclarke removed the Bug thing that needs fixing label Oct 1, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 16 milestone Oct 1, 2020
@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes labels Oct 1, 2020
isaacs added a commit to isaacs/node-tar that referenced this issue Jan 5, 2021
This allows a way to suppress the call to process.umask() while
still being as compliant as possible with the modes as defined
in the tarball entries.

Re: npm/cli#1103
isaacs added a commit to isaacs/node-tar that referenced this issue Jan 6, 2021
This allows a way to suppress the call to process.umask() while
still being as compliant as possible with the modes as defined
in the tarball entries.

Re: npm/cli#1103
isaacs added a commit to isaacs/node-tar that referenced this issue Jan 6, 2021
This allows a way to suppress the call to process.umask() while
still being as compliant as possible with the modes as defined
in the tarball entries.

Re: npm/cli#1103
isaacs added a commit that referenced this issue Jan 6, 2021
Since we now are using pacote/tar in a way that will rely on the default
process umask setting, and we set file/directory modes explicitly
anyway, there's no need to have a default umask setting that calls
process.umask()

As this method is not worker-thread safe, and is deprecated, it's best
for us to stop using it.

Fix: #1103
isaacs added a commit to isaacs/node-tar that referenced this issue Jan 6, 2021
This allows a way to suppress the call to process.umask() while
still being as compliant as possible with the modes as defined
in the tarball entries.

Re: npm/cli#1103
isaacs added a commit that referenced this issue Jan 7, 2021
Since we now are using pacote/tar in a way that will rely on the default
process umask setting, and we set file/directory modes explicitly
anyway, there's no need to have a default umask setting that calls
process.umask()

As this method is not worker-thread safe, and is deprecated, it's best
for us to stop using it.

Fix: #1103
isaacs added a commit that referenced this issue Jan 7, 2021
Since we now are using pacote/tar in a way that will rely on the default
process umask setting, and we set file/directory modes explicitly
anyway, there's no need to have a default umask setting that calls
process.umask()

As this method is not worker-thread safe, and is deprecated, it's best
for us to stop using it.

Fix: #1103
ruyadorno pushed a commit to isaacs/node-tar that referenced this issue Jan 7, 2021
This allows a way to suppress the call to process.umask() while
still being as compliant as possible with the modes as defined
in the tarball entries.

Re: npm/cli#1103

PR-URL: #270
Credit: @isaacs
Close: #270
Reviewed-by: @ruyadorno
isaacs added a commit that referenced this issue Jan 7, 2021
Since we now are using pacote/tar in a way that will rely on the default
process umask setting, and we set file/directory modes explicitly
anyway, there's no need to have a default umask setting that calls
process.umask()

As this method is not worker-thread safe, and is deprecated, it's best
for us to stop using it.

Fix: #1103
isaacs added a commit that referenced this issue Jan 7, 2021
Since we now are using pacote/tar in a way that will rely on the default
process umask setting, and we set file/directory modes explicitly
anyway, there's no need to have a default umask setting that calls
process.umask()

As this method is not worker-thread safe, and is deprecated, it's best
for us to stop using it.

Fix: #1103
@isaacs isaacs closed this as completed in d01746a Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants