asdf-vm / asdf-nodejs

Node.js plugin for asdf version manager
https://github.com/asdf-vm/asdf
MIT License
908 stars 144 forks source link

npm shim should work without $HOME #371

Closed gabrielf closed 9 months ago

gabrielf commented 1 year ago

package-manager-manager is a npm package for identifying the package manager used (npm, yarn, pnpm) etc. It does this by running commands such as npm --version using the npm package shellac which, for security reasons I guess, only exposes a very limited number of ENV variables to the command that is run.

HOME is not one of the default ENV variables and it is not explicitly set by package-manager-manager either. This results in the error HOME: unbound variable in the npm shim for users using asdf and the nodejs plugin.

The same error can be triggered from the command line, without the above tools, by running:

$ env -u HOME /Users/<user>/.asdf/plugins/nodejs/shims/npm
/Users/<user>/.asdf/plugins/nodejs/shims/npm: line 14: HOME: unbound variable

It seems reasonable that the npm shim should work without access to the HOME variable as all package managers work without it.

augustobmoura commented 1 year ago

I can't reproduce the issue on Linux, running env -u HOME npm --version works for me. Can you send the output of asdf info, and could you also check for updates on asdf and asdf-nodejs?

AaronFriel commented 10 months ago

This issue also affects running scripts via Turborepo, which filters environment variables to the commands it runs.

The npm shim should not rely on any environment variables that the npm binary doesn't require. That may mean that the asdf shim needs to be a dynamic file.

@augustobmoura are you running the shimmed NPM (which npm === shimmed script) using Bash? A minimal repro is:

env -u HOME -- /bin/bash -c 'set -u; echo "${ASDF_DATA_DIR:-$HOME/.asdf}"'

That quoted string is copied verbatim from nodejs/shims/npm, and set -u causes BASH to report the unbound variable.

gabrielf commented 10 months ago

@augustobmoura, sorry for the late reply! Below is the output. I updated asdf and the plugin, removed my fix and verified that the problem remains.

> asdf info                                   
OS:
Darwin beatnik.local 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:55:06 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6020 arm64

SHELL:
zsh 5.9 (x86_64-apple-darwin23.0)

BASH VERSION:
5.2.21(1)-release

ASDF VERSION:
v0.14.0-ccdd47d

ASDF INTERNAL VARIABLES:
ASDF_DEFAULT_TOOL_VERSIONS_FILENAME=.tool-versions
ASDF_DATA_DIR=/Users/gabrielfalkenberg/.asdf
ASDF_DIR=/Users/gabrielfalkenberg/.asdf
ASDF_CONFIG_FILE=/Users/gabrielfalkenberg/.asdfrc

ASDF INSTALLED PLUGINS:
golang                       ssh://git@github.com/kennyp/asdf-golang.git master 33b1f6d
java                         ssh://git@github.com/halcyon/asdf-java.git master cf25c5f
liquibase                    http://github.com/ASM88/asdf-liquibase master 9590261
nodejs                       ssh://git@github.com/asdf-vm/asdf-nodejs.git master c5b7c40
pnpm                         ssh://git@github.com/jonathanmorley/asdf-pnpm.git master 305baff
python                       ssh://git@github.com/asdf-community/asdf-python.git master 5e277e2
sbt                          ssh://git@github.com/bram2000/asdf-sbt.git master 33f9637
scala                        ssh://git@github.com/asdf-community/asdf-scala.git master 6de4edb

I realize now that the shim script is run by bash so here is my bash version:

> /usr/bin/env bash -version
GNU bash, version 5.2.21(1)-release (aarch64-apple-darwin23.0.0)
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Here is minimal reproduction using just bash and the relevant lines from the npm shim script:

> env -u HOME bash -c 'set -eu

asdf_data_dir=$(cd "${ASDF_DATA_DIR:-$HOME/.asdf}" && pwd -P)'

If this does not trigger bash: line 3: HOME: unbound variable then maybe env doesn't unset variables using -u on your machine?

augustobmoura commented 9 months ago

I understand that if you unset HOME, because of set -u, you would get an error on arbitrary scripts that depend on HOME. What I can't reproduce is calling the asdf shim without HOME reaching an error, unless you are using the npm shim directly, it does still work:

# This works
env -i $(which npm) --version

# THIS doesn't work, but is not technically supported, you shouldn't call our npm shim wrapper manually
env -i $(asdf which npm) --version

My question is, in the real world, on which occasions are you reaching this unbound variable error?

To me, it doesn't make any sense to hide the HOME variable from any script, given how easy it is to get $HOME by other means. HOME, USER and UID shouldn't be understood as "environment variables that the npm binary doesn't require". Both HOME and USER are basic posix variables that are easily available everywhere (meaning they are not really secrets). This looks more like an incompatibility problem from these tools that are unsetting these variables before calling their scripts.

As a proof we can easily get the $HOME value from expanding ~ in bash. Or the whole "initial env" by running bash on --login mode: env --ignore-environment bash --login -c env

gabrielf commented 9 months ago

My question is, in the real world, on which occasions are you reaching this unbound variable error?

It was when using package-manager-manager (see original issue comment) or rather using some tool that used package-manager-manager. However, it turns out that package-manager-manager has since been updated to pass the env to shellac: https://github.com/james-elicx/package-manager-manager/commit/69fef3247b189365d38b3e6f55675b486642d9a6

I agree that it doesn't make sense to not providing HOME but I, mistakenly, assumed there were reasons 😅

I'll close this issue and my PR now! 🥳