Repository navigation
bug(detector): command injection via NVM_DIR env var in nvm detection #43
Description
Describe the bug
The lock-file subsystem (internal/platform/lock_unix.go, lock_windows.go, AcquireLock/Release) is fully implemented — flock on POSIX, LockFileEx on Windows, stale-PID detection — but has zero call sites anywhere in internal/cli, internal/config, internal/packages, or cmd/.
README states as fact: "Concurrent runs are blocked via a lock file at ~/.nodeup/nodeup.lock." This is currently false.
Concrete impact
- Two concurrent
nodeup upgradeinvocations can snapshot/install/migrate at the same time with no protection whatsoever. - Two concurrent
nodeup config setinvocations race: each does an independent read-modify-write, and the second writer's atomic rename silently clobbers the first writer's change (a lost update, not a crash).
Suggested fix
Wire platform.AcquireLock()/Release() into runUpgrade (around the snapshot/install/migrate critical section) and into internal/config's Save() path (or at least config set/init). Also fix the incidental doc bug while here: README says the lock lives at ~/.nodeup/nodeup.lock; the real path is <DataDir>/nodeup.lock, which resolves to OS-specific XDG/AppData locations per CLAUDE.md's on-disk layout table, never literally ~/.nodeup/ on any of the three supported OSes.
Severity
High — advertised safety feature does not exist in the wired product.
Activity
dipto0321 commentedon Jul 1, 2026
Update: PR #56 (merged, closing #41) added 5 new nvm mutation methods —
Install,Uninstall,Use,SetDefault,Current— that all funnel through the samenvmScriptInvocation()helper (internal/detector/nvm.go) as the originalVersion()method this issue was filed against. Verified: the version argument itself passed into these new methods is safe (semver's anchored parser can't produce a string containing$, backticks, or shell metacharacters), but the underlyingNVM_DIR-derived script path is still built the same vulnerable way — so this bug is now reachable from 6 call paths instead of 1, not fixed or worsened in kind, just wider in exposure. Worth fixing centrally innvmScriptPath()/QuotePath(or by passingNVM_DIRvia subprocess environment instead of string interpolation) so all 6 methods get fixed at once.fix(platform): single-quote paths in QuotePath to neutralize NVM_DIR …