fix(packages): validate manager name against allowlist in restore/diff

CHANGELOG.md

@@ -267,6 +267,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `asdfDataDir()` already used and the official asdf docs.
   Updated `system_node_test.go`'s "asdf env wins" case to drive
   the new variable. Closes #50.
+- `internal/cli/packages.go` (`runRestore` and `runDiff`): the
+  `<manager>` positional argument is now validated against a
+  canonical allowlist (`detector.IsAllowedManagerName`) before any
+  filesystem path is constructed. Pre-fix, the manager name was
+  interpolated verbatim into a snapshot filename
+  (`fmt.Sprintf("%s-%s.json", managerName, version)`) and the
+  result was `filepath.Join`'d into the snapshots dir — and
+  `filepath.Join` collapses `..` segments, so a manager name like
+  `../../tmp/evil` resolved outside the snapshots directory. An
+  attacker with a local file-placement primitive (a shared temp
+  directory, a cloned repo containing a payload filename like
+  `<prefix>../../tmp/evil-1.0.0.json`) could have the resulting
+  snapshot's `Packages` list piped straight into
+  `npm install -g <name>@<version>` with no validation. The
+  allowlist is built from `detector.AllowedManagerNames()` (a new
+  helper that derives the list from `detector.All()`), so the
+  set stays in sync with the per-platform build files; the match
+  is byte-for-byte case-sensitive (matching the `--manager` flag),
+  and the error message surfaces the offender and the allowlist
+  so typos remain user-fixable. `internal/detector/manager_names.go`
+  (new file) holds the helpers; `manager_names_test.go` pins
+  AllowedManagerNames ↔ All() parity and IsAllowedManagerName
+  behaviour (incl. traversal payloads, case-fold negativity, and
+  near-miss strings). Closes #51.
 
 ## [0.0.0] - 2024-07-01
 

internal/cli/packages.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/Masterminds/semver/v3"
 	"github.com/spf13/cobra"
@@ -175,6 +176,16 @@ func runRestore(cmd *cobra.Command, args []string) error {
 	managerName := args[0]
 	versionStr := args[1]
 
+	// Validate the manager name against the canonical allowlist before
+	// it touches any file path. A name like `../../tmp/evil` would
+	// otherwise pass straight into snapshotPath and, after
+	// `filepath.Join` collapses the `..` segments, resolve outside
+	// <DataDir>/snapshots — letting an attacker with a local
+	// file-placement primitive redirect the snapshot read. See #51.
+	if !detector.IsAllowedManagerName(managerName) {
+		return fmt.Errorf("invalid manager name %q (allowed: %s)", managerName, strings.Join(detector.AllowedManagerNames(), ", "))
+	}
+
 	v, err := semver.NewVersion(versionStr)
 	if err != nil {
 		return fmt.Errorf("invalid version: %w", err)
@@ -201,6 +212,12 @@ func newDiffCmd() *cobra.Command {
 func runDiff(cmd *cobra.Command, args []string) error {
 	managerName := args[0]
 
+	// Validate before constructing any snapshot path — same reasoning
+	// as runRestore above. See #51.
+	if !detector.IsAllowedManagerName(managerName) {
+		return fmt.Errorf("invalid manager name %q (allowed: %s)", managerName, strings.Join(detector.AllowedManagerNames(), ", "))
+	}
+
 	s1, err := packages.LoadSnapshot(managerName, args[1])
 	if err != nil {
 		return fmt.Errorf("load snapshot %s: %w", args[1], err)

internal/cli/packages_test.go

@@ -0,0 +1,92 @@
+package cli
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+)
+
+// TestRunRestore_RejectsPathTraversal exercises the manager-name
+// allowlist added for #51. Calling runRestore with a positional
+// manager arg like `../../tmp/evil` must fail BEFORE any
+// snapshotPath is computed — the traversal closes the
+// confused-deputy surface where the user controls the manager name
+// and the result string is used to build a filesystem path.
+func TestRunRestore_RejectsPathTraversal(t *testing.T) {
+	cmd := newRestoreCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+	cmd.SetArgs([]string{"../../tmp/evil", "1.0.0"})
+
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatalf("runRestore(%q): expected error, got nil", "../../tmp/evil")
+	}
+	if !strings.Contains(err.Error(), "invalid manager name") {
+		t.Fatalf("runRestore: err = %v, want message containing 'invalid manager name'", err)
+	}
+	if !strings.Contains(err.Error(), "../../tmp/evil") {
+		t.Fatalf("runRestore: err = %v, want the offender name in the message", err)
+	}
+	// The allowlist must be surfaced verbatim so the user can fix
+	// the typo without grepping our docs.
+	if !strings.Contains(err.Error(), "fnm") {
+		t.Errorf("runRestore: err = %v, want allowlist (e.g. 'fnm') in message", err)
+	}
+}
+
+// TestRunDiff_RejectsPathTraversal mirrors the restore test for the
+// diff subcommand. The diff codepath also passes the user-supplied
+// manager name straight into snapshotPath via LoadSnapshot, with
+// the same path-traversal exposure.
+func TestRunDiff_RejectsPathTraversal(t *testing.T) {
+	cmd := newDiffCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+	cmd.SetArgs([]string{"../../tmp/evil", "1.0.0", "2.0.0"})
+
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatalf("runDiff(%q): expected error, got nil", "../../tmp/evil")
+	}
+	if !strings.Contains(err.Error(), "invalid manager name") {
+		t.Fatalf("runDiff: err = %v, want message containing 'invalid manager name'", err)
+	}
+}
+
+// TestRunRestore_AcceptsCanonicalName verifies the validator doesn't
+// reject every input — a regression that flipped the boolean would
+// be caught by the negative cases above, but a regression that
+// rejected all names would slip past them.
+//
+// We give cobra a single positional arg per the leaf's ExactArgs(2)
+// validator (matches the real CLI shape), and assert: the error path
+// must NOT come from the allowlist. Whether the command succeeds,
+// fails at the snapshot read, or fails at the npm install, doesn't
+// matter — only that the validator let the canonical name through.
+func TestRunRestore_AcceptsCanonicalName(t *testing.T) {
+	// Build the leaf directly so we don't trigger the parent's
+	// Args validator via SetArgs.
+	cmd := newRestoreCmd()
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+	cmd.SetArgs([]string{"fnm", "20.0.0"})
+
+	err := cmd.Execute()
+	if err == nil {
+		// On a machine with fnm installed and a real snapshot
+		// file at <DataDir>/snapshots/fnm-20.0.0.json, the
+		// command could in principle succeed. We don't depend on
+		// that — we only care that the error (if any) is not the
+		// allowlist message.
+		return
+	}
+	if strings.Contains(err.Error(), "invalid manager name") {
+		t.Errorf("runRestore(\"fnm\"): allowlist wrongly rejected canonical name: %v", err)
+	}
+}

internal/detector/manager_names.go

@@ -0,0 +1,45 @@
+package detector
+
+// AllowedManagerNames returns the canonical set of manager names
+// nodeup accepts from user input — `--manager`, the `<manager>` slot
+// in `nodeup packages restore <manager> <version>`, and the
+// `<manager>` slot in `nodeup packages diff <manager> ...`.
+//
+// This is the *allowlist* the cli uses to block path-traversal
+// attempts: a manager name like `../../tmp/evil` would otherwise be
+// interpolated into a snapshot filename via `fmt.Sprintf` and then
+// `filepath.Join`'d into the snapshots dir — and `filepath.Join`
+// collapses `..` segments, taking the result outside the snapshots
+// dir. Validating the name against this list before any path is
+// constructed closes that confused-deputy surface. See issue #51.
+//
+// Built dynamically from All() so the set stays in sync with
+// per-platform registry_*.go build files (e.g. nvm-windows on
+// Windows only). Kept in its own file (no build tag) so the
+// allowlist works identically on every platform — Windows's
+// nvm-windows stays in the list on Linux builds where it would
+// never install, but the cost of an extra name in a 9-element list
+// is nil and the consistency helps audit.
+func AllowedManagerNames() []string {
+	all := All()
+	out := make([]string, 0, len(all))
+	for _, m := range all {
+		out = append(out, m.Name())
+	}
+	return out
+}
+
+// IsAllowedManagerName reports whether `name` is one of the canonical
+// manager-name strings nodeup accepts from user input. The match is
+// case-sensitive (matches All()[i].Name() byte-for-byte) — the
+// `--manager` flag uses the same case-sensitive lookup, and silently
+// accepting "FNM" / "FnM" would just create another inconsistency
+// to debug later. See #51.
+func IsAllowedManagerName(name string) bool {
+	for _, candidate := range AllowedManagerNames() {
+		if candidate == name {
+			return true
+		}
+	}
+	return false
+}

internal/detector/manager_names_test.go

@@ -0,0 +1,85 @@
+package detector
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+)
+
+// TestAllowedManagerNames_MatchesAll covers the platform-correct set:
+// the allowlist mirrors All()[i].Name() exactly. nvm-windows only
+// appears on Windows builds (registry_windows.go), so we don't
+// hard-code the expected slice — we derive it from All() and assert
+// the two stay synchronized.
+func TestAllowedManagerNames_MatchesAll(t *testing.T) {
+	want := make([]string, 0, len(All()))
+	for _, m := range All() {
+		want = append(want, m.Name())
+	}
+	sort.Strings(want)
+
+	got := append([]string(nil), AllowedManagerNames()...)
+	sort.Strings(got)
+
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("AllowedManagerNames = %v, want %v", got, want)
+	}
+}
+
+// TestIsAllowedManagerName pins the case-sensitive lookup contract.
+//
+// Case-sensitive matters: the `--manager` flag and the `<manager>`
+// positional in `packages restore` / `packages diff` all do
+// byte-for-byte case-sensitive matches, and a silent case-fold
+// here would just paper over the next inconsistency instead of
+// surfacing it. The negative cases include both genuine traversal
+// payloads and harmless-looking-but-unrelated strings so a future
+// regression that flips the case-fold would be caught.
+func TestIsAllowedManagerName(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+		want bool
+	}{
+		// Positive: every name in the canonical allowlist. We don't
+		// hard-code the list — we iterate AllowedManagerNames() so
+		// a future addition flows through this test automatically.
+		{"fnm", "fnm", true},
+		{"empty string", "", false},
+
+		// Negative: traversal payloads (the bug from #51).
+		{"dotdot traversal", "../etc/passwd", false},
+		{"absolute path", "/etc/passwd", false},
+		{"mixed traversal", "../../tmp/evil", false},
+		{"backslash on windows-style path", "..\\..\\evil", false},
+		{"dot segment hidden", "./fnm", false},
+
+		// Negative: case-folding must NOT match (see comment above).
+		{"uppercase FNM", "FNM", false},
+		{"title case FnM", "FnM", false},
+
+		// Negative: strings that look manager-shaped but aren't.
+		{"near-miss fnmm", "fnmm", false},
+		{"near-miss fn", "fn", false},
+		{"garbage", "lol", false},
+		{"space-padded", " fnm", false},
+		{"trailing slash", "fnm/", false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.in, func(t *testing.T) {
+			if got := IsAllowedManagerName(tc.in); got != tc.want {
+				t.Errorf("IsAllowedManagerName(%q) = %v, want %v", tc.in, got, tc.want)
+			}
+		})
+	}
+
+	// Sanity: every name in AllowedManagerNames() must also pass the
+	// boolean helper. Catches an off-by-one in the iteration order
+	// (e.g. if someone typed `if candidate > name` instead of `==`).
+	for _, name := range AllowedManagerNames() {
+		if !IsAllowedManagerName(name) {
+			t.Errorf("AllowedManagerNames lists %q but IsAllowedManagerName says false", name)
+		}
+	}
+}