fix(platform): single-quote paths in QuotePath to neutralize NVM_DIR injection

CHANGELOG.md

@@ -139,6 +139,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   carrying the user's globals forward to the new default exactly
   once. Added a regression test pinning the
   `RestoreFromSnapshot`-reads-source-path contract. Closes #42.
+- `internal/platform/shell.go` (`QuotePath`): on POSIX the quoting
+  rule for paths embedded in `bash -c` scripts now uses **single
+  quotes** with `'\''` to escape embedded `'` bytes. The previous
+  rule used double quotes and only escaped `"` and `\\`, so a path
+  containing `$USER`, `$(...)`, or backticks was still subject to
+  shell expansion. `NVM.Version()` builds a `source <path> && nvm
+  --version` script using whatever `NVM_DIR` (or `~/.nvm`) provides,
+  so setting `NVM_DIR='/tmp/$(touch /tmp/PWNED)'` would expand
+  inside bash's double quotes before reaching nvm. Single-quote
+  wrapping disables bash variable expansion, command substitution,
+  and backtick substitution entirely. The Windows branch
+  (cmd.exe-unaware double-quote wrapping for paths-with-spaces) is
+  unchanged. The test table for `QuotePath` now expects single quotes
+  on POSIX, plus a dedicated `TestQuotePathNeutralizesShellInjection`
+  regression test, plus a `TestNVM_Version_NVMDirShellInjectionIsNeutralized`
+  detector-level test that points `NVM_DIR` at a payload string and
+  asserts no unquoted `$(touch` occurrence in the emitted script.
+  Closes #43.
 
 ## [0.0.0] - 2024-07-01
 

internal/detector/nvm_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -333,6 +334,84 @@ func TestNVM_Version_NoNVMScript(t *testing.T) {
 	}
 }
 
+// TestNVM_Version_NVMDirShellInjectionIsNeutralized is the regression
+// test for #43. Even when NVM_DIR contains shell metacharacters like
+// `$(touch …)` or backticks, the script that runs through runScript
+// must keep those characters LITERAL — bash must not perform command
+// substitution inside the single-quoted region that QuotePath emits.
+//
+// The test sets NVM_DIR to a string that contains a command-substitution
+// payload but does NOT require a real directory to exist (nvm.sh is
+// stubbed via runScript), captures the script that NVM.Version() builds,
+// and asserts that the payload characters appear inside a single-quoted
+// span rather than as bare command substitution.
+func TestNVM_Version_NVMDirShellInjectionIsNeutralized(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("cmd.exe unquote rules differ; QuotePath's Windows branch is covered in internal/platform tests")
+	}
+
+	// Pick a payload directory NAME that QuotePath must absorb
+	// verbatim — we never create it on disk, but nvmDir()/nvmScriptPath()
+	// only return the strings, never validate them, and the runScript
+	// stub returns canned stdout without touching the filesystem.
+	//
+	// The bytes `$(touch /tmp/PWND_BY_NVM_DIR)` are just path bytes,
+	// not actual shell substitution. NVM_DIR is set to a path that
+	// looks like `/tmp/<tmpdir>/$(touch /tmp/PWND_BY_NVM_DIR)`.
+	maliciousDirName := "$(touch /tmp/PWND_BY_NVM_DIR)"
+	t.Setenv("NVM_DIR", "/tmp/anything-at-all/"+maliciousDirName)
+	// Make sure no leftover sentinel from a previous run.
+	_ = os.Remove("/tmp/PWND_BY_NVM_DIR")
+	defer os.Remove("/tmp/PWND_BY_NVM_DIR")
+
+	rec := withStubScript(t, "0.40.5\n", nil)
+	if _, err := NewNVM().Version(); err != nil {
+		t.Fatalf("Version(): %v", err)
+	}
+
+	// The script MUST contain the payload inside single quotes, not
+	// as bare `$(touch …)`. Otherwise bash would expand it the moment
+	// the script runs in production.
+	script := rec.script
+
+	// 1. The malicious substring must be inside a single-quoted span.
+	//    nvmScriptPath() appends `/nvm.sh`, so the full quoted form
+	//    is `…/$(touch /tmp/PWND_BY_NVM_DIR)/nvm.sh`. We find the
+	//    payload's byte offset and walk backwards/forward to the
+	//    nearest `'` on each side; both must surround the payload,
+	//    proving it lives inside a single-quoted region.
+	idx := strings.Index(script, "$(touch")
+	if idx < 0 {
+		t.Fatalf("payload substring %q not present in script — the malicious NVM_DIR was not used. script=%q",
+			"$(touch", script)
+	}
+	leftQuote := strings.LastIndex(script[:idx], "'")
+	rightQuote := strings.Index(script[idx:], "'")
+	if leftQuote < 0 || rightQuote < 0 {
+		t.Fatalf("payload not surrounded by single quotes on both sides. leftQuote=%d rightQuote=%d script=%q",
+			leftQuote, rightQuote, script)
+	}
+	// If the script also contains double-quotes around the payload,
+	// bash would still perform command-substitution inside them.
+	// We tolerate overall double-quotes elsewhere in the script;
+	// the regression check is specifically that the payload bytes are
+	// not in an unquoted region.
+
+	// 2. The payload substring `$(touch` (if present in the script
+	//    as bare bytes) must appear only inside a single-quoted span.
+	//    If QuotePath passed it through bare, that's a regression.
+	if idx := strings.Index(script, "$(touch"); idx >= 0 {
+		// Count single quotes BEFORE the payload occurrence: odd
+		// count means we are inside a single-quoted span.
+		upTo := script[:idx]
+		singleQuoteCount := strings.Count(upTo, "'")
+		if singleQuoteCount%2 == 0 {
+			t.Errorf("payload substring %q at offset %d is OUTSIDE single quotes — bash would expand it. script=%q",
+				"$(touch", idx, script)
+		}
+	}
+}
+
 // --- NVM.ListInstalled -------------------------------------------------
 
 func TestNVM_ListInstalled_HappyPath(t *testing.T) {

internal/platform/platform_test.go

@@ -2,6 +2,7 @@ package platform
 
 import (
 	"context"
+	"os"
 	"path/filepath"
 	"runtime"
 	"testing"
@@ -83,12 +84,21 @@ func TestPlatformHelpers(t *testing.T) {
 	}
 }
 
-// TestQuotePathNoSpecials verifies that paths without spaces or shell
-// metacharacters pass through unchanged (no spurious quoting).
+// TestQuotePathNoSpecials documents that all paths now go through
+// single-quote wrapping (POSIX) or selective double-quote wrapping
+// (Windows). The output is always a shell-safe literal of the input.
 func TestQuotePathNoSpecials(t *testing.T) {
 	in := "/usr/local/bin"
-	if got := QuotePath(in); got != in {
-		t.Errorf("QuotePath(%q) = %q, want unchanged", in, got)
+	got := QuotePath(in)
+	if runtime.GOOS == "windows" {
+		if got != in {
+			t.Errorf("QuotePath(%q) = %q, want unchanged on Windows", in, got)
+		}
+		return
+	}
+	want := "'/usr/local/bin'"
+	if got != want {
+		t.Errorf("QuotePath(%q) = %q, want %q", in, got, want)
 	}
 }
 
@@ -106,20 +116,29 @@ func TestQuotePathWithSpace(t *testing.T) {
 	if got == in {
 		t.Error("QuotePath left a space-containing path unquoted")
 	}
-	// Must start with a quote.
-	if got[0] != '"' {
-		t.Errorf("QuotePath did not wrap in double quotes: %q", got)
+	// POSIX: must start with a single quote (and end with one).
+	// Windows: must start with a double quote.
+	if runtime.GOOS == "windows" {
+		if got[0] != '"' {
+			t.Errorf("QuotePath did not wrap in double quotes (Windows): %q", got)
+		}
+	} else {
+		if got[0] != '\'' {
+			t.Errorf("QuotePath did not wrap in single quotes (POSIX): %q", got)
+		}
 	}
 }
 
 // TestQuotePathTable exercises QuotePath across the cases that matter for
 // cross-platform support: spaces, embedded quotes, backslashes (Windows
-// path separators), shell metacharacters on unix ($, `, |), and edges
+// path separators), shell metacharacters on unix ($, `, |, '), and edges
 // (empty, single space, trailing slash).
 //
 // Expected output is computed by branching on runtime.GOOS because
-// QuotePath uses a different unsafe-char set on Windows vs unix: on
-// Windows only `"` is unsafe; on unix ` "$`<>|'` are all unsafe.
+// QuotePath uses a different quoting strategy on Windows vs unix: on
+// Windows we wrap paths-with-spaces in double quotes (the cmd.exe
+// unquote rules); on unix we ALWAYS wrap in single quotes so that `$`,
+// backticks, and `\\` are not interpreted by bash. See #43.
 //
 // We run the same table on every OS so CI catches regressions on all
 // three platforms (ubuntu, macos, windows in our matrix).
@@ -130,26 +149,32 @@ func TestQuotePathTable(t *testing.T) {
 		want string
 	}
 
-	// Unix-style cases (forward slashes, no backslashes). On Windows the
-	// unsafe-char set is narrower — backslash is treated as a regular
-	// char and not escaped — so paths containing backslashes are only
-	// valid inputs on Windows. We split the table by OS to keep the
-	// expectations unambiguous.
+	// Unix-style cases. On POSIX we always single-quote; embedded `'`
+	// becomes `'\''`. We deliberately do NOT preserve `$USER` or
+	// backticks verbatim — that was the command-injection bug.
 	unixCases := []tc{
 		{"empty", "", `""`},
-		{"plain_unix_path", "/usr/local/bin", "/usr/local/bin"},
-		{"single_space", " ", `" "`},
-		{"trailing_space", "/Users/dipto/My App ", `"/Users/dipto/My App "`},
-		{"embedded_space", "/Users/dipto/My App/bin", `"/Users/dipto/My App/bin"`},
-		{"multiple_spaces", "/Users/My  App/bin", `"/Users/My  App/bin"`},
-		{"dollar_sign", "/home/$USER/foo", `"/home/$USER/foo"`},
-		{"backtick", "/tmp/`whoami`/x", "\"/tmp/`whoami`/x\""},
-		{"pipe", "/tmp/a|b/c", `"/tmp/a|b/c"`},
-		{"redirect", "/tmp/a>b/c", `"/tmp/a>b/c"`},
-		{"single_quote", "/tmp/it's/x", `"/tmp/it's/x"`},
-		{"embedded_dquote", `/tmp/oops"quote/x`, `"/tmp/oops\"quote/x"`},
-		{"trailing_slash", "/var/log/", "/var/log/"},
+		{"plain_unix_path", "/usr/local/bin", "'/usr/local/bin'"},
+		{"single_space", " ", `' '`},
+		{"trailing_space", "/Users/dipto/My App ", "'/Users/dipto/My App '"},
+		{"embedded_space", "/Users/dipto/My App/bin", "'/Users/dipto/My App/bin'"},
+		{"multiple_spaces", "/Users/My  App/bin", "'/Users/My  App/bin'"},
+		// Regression cases for #43 — these previously passed through
+		// unchanged (or with bare double quotes that bash would expand).
+		{"dollar_sign", "/home/$USER/foo", "'/home/$USER/foo'"},
+		{"backtick", "/tmp/`whoami`/x", "'/tmp/`whoami`/x'"},
+		{"command_subst", "/tmp/$(id)/x", "'/tmp/$(id)/x'"},
+		{"pipe", "/tmp/a|b/c", "'/tmp/a|b/c'"},
+		{"redirect", "/tmp/a>b/c", "'/tmp/a>b/c'"},
+		// Single quote inside the path becomes the canonical
+		// close/reopen/escape dance.
+		{"single_quote", "/tmp/it's/x", `'/tmp/it'\''s/x'`},
+		{"embedded_dquote", `/tmp/oops"quote/x`, `'/tmp/oops"quote/x'`},
+		{"trailing_slash", "/var/log/", "'/var/log/'"},
 	}
+	// Windows: same logic as before — only `"` (and spaces) trigger
+	// double-quote wrapping; `\\` inside the path gets escaped for
+	// cmd.exe double-quote semantics.
 	windowsCases := []tc{
 		{"empty", "", `""`},
 		{"plain_windows_path", `C:\node`, `C:\node`},
@@ -177,37 +202,79 @@ func TestQuotePathTable(t *testing.T) {
 	}
 }
 
+// TestQuotePathNeutralizesShellInjection is the regression test for #43:
+// a path containing `$(touch /tmp/PWNED)` or `\`whoami\“ must NOT be
+// expanded when fed back through bash. We embed the quoted form in a
+// shell script that touches a side-channel file if (and only if)
+// expansion happens.
+func TestQuotePathNeutralizesShellInjection(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("cmd.exe unquote rules differ; Windows path is covered by TestQuotePathTable")
+	}
+	// A path that, if expanded, would `touch` a sentinel file. After
+	// running the test, the sentinel MUST NOT exist.
+	malicious := "/tmp/nodeup-quote-path-test-$$(touch /tmp/nodeup-pwned-$$)"
+	sentinel := "/tmp/nodeup-pwned-$$"
+	// Clean up any leftover from a previous failed run.
+	_ = os.Remove(sentinel)
+	defer func() {
+		// `$$` is process-id; multiple test runs may target the same
+		// sentinel if the test reuses one. Clean using the literal
+		// basename pattern.
+		_ = os.Remove("/tmp/nodeup-pwned-$$")
+	}()
+
+	// Compose a shell script that runs `id -un` after sourcing a
+	// non-existent file (a no-op for our purposes) but that, if the
+	// path were expanded by bash, would touch the sentinel. We then
+	// compare the printed value to the literal input — they must match.
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	// `printf '%s'` echoes the literal value of the quoted form; if
+	// bash expanded `$(touch …)`, the touch side-effect would run AND
+	// the printed value would be the empty string (touch produces no
+	// stdout).
+	script := "printf '%s' " + QuotePath(malicious)
+	res, err := RunShellScript(ctx, script)
+	if err != nil {
+		t.Fatalf("RunShellScript: %v", err)
+	}
+	if res.Stdout != malicious {
+		t.Errorf("round-trip mismatch: in=%q quoted=%q got=%q (bash expanded the quoted path)",
+			malicious, QuotePath(malicious), res.Stdout)
+	}
+	if _, err := os.Stat("/tmp/nodeup-pwned-$$"); err == nil {
+		t.Errorf("sentinel file /tmp/nodeup-pwned-$$ exists — bash expanded a metacharacter from the path, command injection is NOT neutralized")
+	}
+}
+
 // TestQuotePathRoundTrip confirms that the output of QuotePath, when
 // embedded in a shell script, re-evaluates to the original path. We
 // shell out via RunShellScript and `printf '%s'` the quoted form, then
 // compare to the original input.
 //
-// We only test cases that QuotePath is *designed* to protect: paths
-// with spaces, embedded double-quotes, and shell metacharacters that
-// are not subject to variable expansion (|, <, >). Paths containing
-// literal `$` or backticks are intentionally NOT tested here because
-// QuotePath uses double-quotes (preserving shell variable expansion),
-// so a `$` in the path will still be expanded by the shell. See the
-// comment on QuotePath itself for this trade-off.
+// On POSIX we now expect round-trip success for the full set of
+// metacharacters ($, `, |) because QuotePath uses single quotes that
+// disable shell expansion entirely. On Windows, the round-trip is
+// not meaningful for cmd.exe (different unquote rules) — TestQuotePath
+// covers per-OS expected output instead.
 func TestQuotePathRoundTrip(t *testing.T) {
 	if runtime.GOOS == "plan9" {
 		t.Skip("plan9 shell semantics differ; skip")
 	}
-	// The round-trip uses RunShellScript, which on Windows invokes
-	// cmd.exe rather than bash. cmd.exe's unquote rules differ from
-	// bash (notably, cmd.exe drops the leading backslash from a
-	// double-quoted path and treats `\"` as a literal `"` followed by
-	// the next char). The test's purpose is to verify QuotePath's
-	// bash-mode quoting, so it is not meaningful on Windows. The
-	// table-driven TestQuotePath already covers the per-OS quoting
-	// logic for both unix and Windows.
 	if runtime.GOOS == "windows" {
 		t.Skip("cmd.exe unquote rules differ from bash; TestQuotePath covers per-OS expected output")
 	}
 	paths := []string{
 		"/Users/dipto/My App",
 		"/Users/Double  Space/bin",
 		"/tmp/oops\"quote/x",
+		// Regression cases for #43: these now round-trip correctly
+		// because single-quote wrapping disables expansion.
+		"/home/$USER/foo",
+		"/tmp/`whoami`/x",
+		"/tmp/it's/a/path",
+		"/tmp/$(id -u)/x",
 	}
 	for _, p := range paths {
 		p := p