diff --git a/CHANGELOG.md b/CHANGELOG.md index 469a425..d1b25d5 100644 --- a/CHANGELOG.md +++ b/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 + `` 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 + `../../tmp/evil-1.0.0.json`) could have the resulting + snapshot's `Packages` list piped straight into + `npm install -g @` 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 diff --git a/internal/cli/packages.go b/internal/cli/packages.go index 269406a..93d7e52 100644 --- a/internal/cli/packages.go +++ b/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 + // /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) diff --git a/internal/cli/packages_test.go b/internal/cli/packages_test.go new file mode 100644 index 0000000..afb1ff6 --- /dev/null +++ b/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 /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) + } +} diff --git a/internal/detector/manager_names.go b/internal/detector/manager_names.go new file mode 100644 index 0000000..0f8888f --- /dev/null +++ b/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 `` slot +// in `nodeup packages restore `, and the +// `` slot in `nodeup packages diff ...`. +// +// 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 +} diff --git a/internal/detector/manager_names_test.go b/internal/detector/manager_names_test.go new file mode 100644 index 0000000..0306219 --- /dev/null +++ b/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 `` +// 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) + } + } +}