[node] - Install pnpm as non-root user to prevent root-owned npm cache#1625
[node] - Install pnpm as non-root user to prevent root-owned npm cache#1625brianhelba wants to merge 2 commits intodevcontainers:mainfrom
Conversation
Kaniska244
left a comment
Kaniska244
left a comment
There was a problem hiding this comment.
Hi @brianhelba
Thank you for the contribution. Would you kindly bump the node feature version.
Currently, the pnpm installation block runs `npm install -g pnpm` in
a bare subshell as root, unlike every other npm/nvm operation in the
script which uses `su ${USERNAME}`. This causes the npm cache directory
to be created owned by `root:root`, leading to `EACCES` errors for the
non-root user on subsequent npm operations.
This is particularly reproducible on macOS with Rosetta 2 emulation,
where the cache directory may not already exist from prior steps.
Note, the explicit setting of proxy env vars (`http_proxy`, `https_proxy`,
`no_proxy`) is likely a workaround for npm/cli#6835.
No other commands in this script use that workaround anymore, so this change
uses the same `su` syntax as all other commands.
|
@Kaniska244 Done. This is simply a bugfix and doesn't add or remove features, so I bumped the patch version. |
Kaniska244
left a comment
Kaniska244
left a comment
There was a problem hiding this comment.
To be further reviewed by maintainers.
Hi @brianhelba Would you please accept the above license agreement. |
|
@Kaniska244 Sorry, I'm awaiting approval from my employer. I'm hoping this will be quick. |
|
@microsoft-github-policy-service agree company="Kitware, Inc." |
|
@Kaniska244 All done. |
| if bash -c ". '${NVM_DIR}/nvm.sh' && type npm >/dev/null 2>&1"; then | ||
| ( | ||
| . "${NVM_DIR}/nvm.sh" | ||
| [ ! -z "$http_proxy" ] && npm set proxy="$http_proxy" |
There was a problem hiding this comment.
Others may need these proxies; could they be added back?
There was a problem hiding this comment.
From my commit message:
Note, the explicit setting of proxy env vars (
http_proxy,https_proxy,no_proxy) is likely a workaround for npm/cli#6835. No other commands in this script use that workaround anymore, so this change uses the same su syntax as all other commands.
So, this doesn't change anything for users. They can still set $http_proxy and npm will directly respect it.
Every other install block works this way. The removed code just seems like leftovers of a workaround. If the workaround is actually necessary for some reason, than it's a bug that every other install block lacks it.
| [ ! -z "$no_proxy" ] && npm set noproxy="$no_proxy" | ||
| npm install -g pnpm@$PNPM_VERSION --force | ||
| ) | ||
| su ${USERNAME} -c "umask 0002 && . '${NVM_DIR}/nvm.sh' && npm install -g pnpm@${PNPM_VERSION} --force" |
There was a problem hiding this comment.
Are there any implications for switching to user-based vs root based? There may have been assumptions made by others based on this being root-based. If it could cause issues, we may want to consider adding an option and defaulting to the original approach.
There was a problem hiding this comment.
Every other install block installs things as the user USERNAME, instead of root. I am not aware of any reason that pnpm's files should be owned by root, but everything else should be owned by USERNAME.
Further, this doesn't just cause pnpm-specific things to be owned by root, it can impact other common Node directories if this block runs first. From my commit message:
This causes the npm cache directory to be created owned by
root:root, leading toEACCESerrors for the non-root user on subsequent npm operations.
The unconditional ownership of some files by root is the essence of the bug. Features are (I believe) supposed to respect the active user (and every other part of this script does). A flag would be a "would you like to to fix the bug" option.
Users can continue to set USERNAME to root (by setting USER in their Dockerfile or using other dev container features) if they want everything installed by the Node feature to be owned by root.
abdurriq
left a comment
abdurriq
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I've left a couple comments which I would appreciate if you could review and address.
3 participants
Currently, the pnpm installation block runs
npm install -g pnpmin a bare subshell as root, unlike every other npm/nvm operation in the script which usessu ${USERNAME}. This causes the npm cache directory to be created owned byroot:root, leading toEACCESerrors for the non-root user on subsequent npm operations.This is particularly reproducible on macOS with Rosetta 2 emulation, where the cache directory may not already exist from prior steps.
Note, the explicit setting of proxy env vars (
http_proxy,https_proxy,no_proxy) is likely a workaround for npm/cli#6835. No other commands in this script use that workaround anymore, so this change uses the samesusyntax as all other commands.