Skip to content

[node] - Install pnpm as non-root user to prevent root-owned npm cache #1625

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.

Sign up for GitHub

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

Open
brianhelba wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[node] - Install pnpm as non-root user to prevent root-owned npm cache #1625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e50ea02
[node] - Install pnpm as non-root user to prevent root-owned npm cache
brianhelba Apr 16, 2026
1ddefad
Merge branch 'main' into node-root
Kaniska244 Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/node/devcontainer-feature.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"id": "node",
"version": "1.7.1",
"version": "1.7.2",
"name": "Node.js (via nvm), yarn and pnpm.",
"documentationURL": "https://github.com/devcontainers/features/tree/main/src/node",
"description": "Installs Node.js, nvm, yarn, pnpm, and needed dependencies.",
Expand Down
8 changes: 1 addition & 7 deletions src/node/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,13 +386,7 @@ if [ ! -z "${PNPM_VERSION}" ] && [ "${PNPM_VERSION}" = "none" ]; then
echo "Ignoring installation of PNPM"
else
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"
Copy link
Copy Markdown
Contributor

@abdurriq abdurriq Apr 28, 2026

Choose a reason for hiding this comment

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

Others may need these proxies; could they be added back?

Copy link
Copy Markdown
Author

@brianhelba brianhelba Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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 "$https_proxy" ] && npm set https-proxy="$https_proxy"
[ ! -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"
Copy link
Copy Markdown
Contributor

@abdurriq abdurriq Apr 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@brianhelba brianhelba Apr 29, 2026

Choose a reason for hiding this comment

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

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 to EACCES errors 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.

else
echo "Skip installing pnpm because npm is missing"
fi
Expand Down
Loading