diff --git a/CHANGELOG.md b/CHANGELOG.md index d1b25d5..a0f34bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -146,6 +146,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 for the full rationale. ### Fixed +- `internal/cli` (`upgrade.go`): `--yes --no-cleanup` no longer + silently auto-deletes every eligible old Node.js version. The + `if yes { ... }` block at upgrade.go:107-111 ran unconditionally + AFTER the `switch` that was supposed to make `--no-cleanup` win, + so it flipped `NonInteractive` back to `false` and set + `AutoDeleteAll=true`, regardless of `noCleanup`. A CI script + that habitually passes `-y`/`--yes` (the documented + `--no-cleanup` contract is "never prompt, never delete") would + therefore lose Node installs it never asked to remove. The + cleanup-toggle resolution is now factored into a pure helper + `resolveCleanupConfig(noCleanup, autoCleanup, yes, versions, cfg)` + in `cleanup.go` so it's testable in isolation; the `--yes` + block is guarded by `&& !noCleanup`, restoring `--no-cleanup`'s + precedence. New tests in `cleanup_config_test.go` pin the + precedence table (including the negative-path matrix where + `--no-cleanup` beats every other knob). Closes #57. +- `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. - `internal/cli`: upgrade loop's restore step was looking up its snapshot by the **new** installed Node version (`packages.Restore(ctx, mgr, *v)` where `v` came from `toInstall`), diff --git a/internal/cli/cleanup.go b/internal/cli/cleanup.go index ac6313f..aeb1ee1 100644 --- a/internal/cli/cleanup.go +++ b/internal/cli/cleanup.go @@ -8,6 +8,7 @@ import ( "github.com/Masterminds/semver/v3" + "github.com/dipto0321/nodeup/internal/config" "github.com/dipto0321/nodeup/internal/detector" ) @@ -64,6 +65,54 @@ type cleanupConfig struct { Prefiltered []semver.Version } +// resolveCleanupConfig maps the post-upgrade cleanup toggles (CLI flags +// + config-file values) into the cleanupConfig struct that +// runCleanupPrompt consumes. +// +// Precedence, highest first: +// +// --no-cleanup → NonInteractive=true; nothing else applies. +// --no-cleanup's documented contract is +// "never prompt, never delete" and it beats +// every other toggle, including --yes. A CI +// script that always passes `-y`/`--yes` +// must NOT silently delete Node installs +// the user told it to keep. See #57. +// --cleanup → AutoDeleteAll=true. +// cfg.Cleanup.Auto → AutoDeleteAll=true. +// --yes (and !noCleanup) → AutoDeleteAll=true, PerVersion=false, +// NonInteractive=false. So a non-interactive +// run still makes progress without hanging +// on an unserviced prompt. +// +// PerVersion defaults to cfg.Cleanup.Prompt (true by default), and +// Prefiltered is whatever --cleanup-version passed. +func resolveCleanupConfig(noCleanup, autoCleanup, yes bool, cleanupVersions []semver.Version, cfg config.CleanupConfig) cleanupConfig { + out := cleanupConfig{ + NonInteractive: noCleanup, + PerVersion: cfg.Prompt, + Prefiltered: cleanupVersions, + } + switch { + case noCleanup: + // Already set; no other knobs apply. --no-cleanup wins. + case autoCleanup: + out.AutoDeleteAll = true + case cfg.Auto: + out.AutoDeleteAll = true + } + if yes && !noCleanup { + // --yes implies auto-delete-all so non-interactive runs don't + // hang on an unserviced prompt. Guarded by !noCleanup so a user + // who explicitly says "never delete" doesn't have that contract + // silently overridden by a CI script's `-y` flag. See #57. + out.NonInteractive = false + out.AutoDeleteAll = true + out.PerVersion = false + } + return out +} + // cleanupResult summarizes what happened during a cleanup run. The // upgrade command reports this to the user after the prompt loop // completes (success or partial-failure). diff --git a/internal/cli/cleanup_config_test.go b/internal/cli/cleanup_config_test.go new file mode 100644 index 0000000..24c7805 --- /dev/null +++ b/internal/cli/cleanup_config_test.go @@ -0,0 +1,182 @@ +package cli + +import ( + "testing" + + "github.com/Masterminds/semver/v3" + + "github.com/dipto0321/nodeup/internal/config" +) + +// TestResolveCleanupConfig_NoCleanupBeatsYes pins the precedence fix +// for #57. Before the fix, --yes ran unconditionally AFTER the switch +// that was supposed to make --no-cleanup win — the body flipped +// NonInteractive back to false and set AutoDeleteAll=true, so +// `nodeup upgrade --yes --no-cleanup` silently deleted every old +// Node.js version. After the fix, --no-cleanup's documented contract +// ("never prompt, never delete") survives any combination of the +// other flags. +func TestResolveCleanupConfig_NoCleanupBeatsYes(t *testing.T) { + cfg := config.CleanupConfig{Auto: false, Prompt: true} + + got := resolveCleanupConfig( + /*noCleanup*/ true, + /*autoCleanup*/ false, + /*yes*/ true, + nil, + cfg, + ) + + if !got.NonInteractive { + t.Errorf("--no-cleanup + --yes: NonInteractive = false, want true (--no-cleanup must win)") + } + if got.AutoDeleteAll { + t.Errorf("--no-cleanup + --yes: AutoDeleteAll = true, want false (--no-cleanup must win)") + } + // Sanity: a regression that flipped the `&& !noCleanup` guard + // back to a bare `yes` would also need to leave the per-version + // prompt flag alone. Pin both. + if !got.PerVersion { + t.Errorf("--no-cleanup + --yes: PerVersion = false, want true (cfg.Cleanup.Prompt default)") + } +} + +// TestResolveCleanupConfig_YesAutoDeletesWithoutNoCleanup verifies +// that --yes still works as documented when --no-cleanup is NOT set. +// A regression that guarded the block with `!yes` instead of `&& !noCleanup` +// would silently break every CI script relying on --yes. +func TestResolveCleanupConfig_YesAutoDeletesWithoutNoCleanup(t *testing.T) { + cfg := config.CleanupConfig{Auto: false, Prompt: true} + + got := resolveCleanupConfig( + /*noCleanup*/ false, + /*autoCleanup*/ false, + /*yes*/ true, + nil, + cfg, + ) + + if got.NonInteractive { + t.Errorf("--yes alone: NonInteractive = true, want false (we're not skipping cleanup, just auto-confirming)") + } + if !got.AutoDeleteAll { + t.Errorf("--yes alone: AutoDeleteAll = false, want true") + } + if got.PerVersion { + t.Errorf("--yes alone: PerVersion = true, want false (auto-confirm should skip per-version y/N)") + } +} + +// TestResolveCleanupConfig_CleanupFlagBeatsYesAndCfgAuto documents +// that --cleanup and cfg.Cleanup.Auto both map to AutoDeleteAll=true. +// Both are subsumed under --yes's "skip the per-version confirm" +// behavior — they're the same end state. +func TestResolveCleanupConfig_CleanupFlagSetsAutoDeleteAll(t *testing.T) { + cfg := config.CleanupConfig{Auto: false, Prompt: true} + + got := resolveCleanupConfig(false, true, false, nil, cfg) + if !got.AutoDeleteAll { + t.Errorf("--cleanup: AutoDeleteAll = false, want true") + } + if got.NonInteractive { + t.Errorf("--cleanup: NonInteractive = true, want false") + } +} + +func TestResolveCleanupConfig_ConfigAutoSetsAutoDeleteAll(t *testing.T) { + cfg := config.CleanupConfig{Auto: true, Prompt: true} + + got := resolveCleanupConfig(false, false, false, nil, cfg) + if !got.AutoDeleteAll { + t.Errorf("cfg.Cleanup.Auto=true: AutoDeleteAll = false, want true") + } +} + +// TestResolveCleanupConfig_PrefilteredPropagates covers --cleanup-version: +// the parsed semver slice must pass through unchanged. +func TestResolveCleanupConfig_PrefilteredPropagates(t *testing.T) { + cfg := config.CleanupConfig{Auto: false, Prompt: true} + + pre := []semver.Version{ + mustVer(t, "20.18.0"), + mustVer(t, "18.20.4"), + } + + got := resolveCleanupConfig(false, false, false, pre, cfg) + if len(got.Prefiltered) != len(pre) { + t.Fatalf("Prefiltered length = %d, want %d", len(got.Prefiltered), len(pre)) + } + for i := range pre { + if got.Prefiltered[i].String() != pre[i].String() { + t.Errorf("Prefiltered[%d] = %s, want %s", i, got.Prefiltered[i], pre[i]) + } + } +} + +// TestResolveCleanupConfig_DefaultsAreInteractive covers the default +// state: no flags, default config (Auto=false, Prompt=true). Cleanup +// should run interactively with per-version y/N prompts. +func TestResolveCleanupConfig_DefaultsAreInteractive(t *testing.T) { + cfg := config.CleanupConfig{Auto: false, Prompt: true} + + got := resolveCleanupConfig(false, false, false, nil, cfg) + if got.NonInteractive { + t.Errorf("defaults: NonInteractive = true, want false") + } + if got.AutoDeleteAll { + t.Errorf("defaults: AutoDeleteAll = true, want false") + } + if !got.PerVersion { + t.Errorf("defaults: PerVersion = false, want true (cfg.Cleanup.Prompt default)") + } + if len(got.Prefiltered) != 0 { + t.Errorf("defaults: Prefiltered = %v, want empty", got.Prefiltered) + } +} + +// TestResolveCleanupConfig_CfgPromptFalseSkipsPerVersion covers +// cfg.Cleanup.Prompt=false: per-version y/N is skipped but we still +// hit the all-or-nothing prompt (unless Auto is also true). +func TestResolveCleanupConfig_CfgPromptFalseSkipsPerVersion(t *testing.T) { + cfg := config.CleanupConfig{Auto: false, Prompt: false} + + got := resolveCleanupConfig(false, false, false, nil, cfg) + if got.PerVersion { + t.Errorf("cfg.Cleanup.Prompt=false: PerVersion = true, want false") + } + if got.AutoDeleteAll { + t.Errorf("cfg.Cleanup.Prompt=false: AutoDeleteAll = true, want false (only Prompt was false, not Auto)") + } +} + +// TestResolveCleanupConfig_NoCleanupBeatsEverythingElse is the +// negative-path matrix for #57. Each row sets one knob that would +// otherwise drive AutoDeleteAll=true and asserts --no-cleanup +// wins on every one. The bug from #57 was a single unguarded +// `if yes` block; this test pins all the other places the same +// regression could re-appear. +func TestResolveCleanupConfig_NoCleanupBeatsEverythingElse(t *testing.T) { + cases := []struct { + name string + autoCleanup bool + yes bool + cfg config.CleanupConfig + }{ + {"noCleanup + autoCleanup", true, false, config.CleanupConfig{Auto: false, Prompt: true}}, + {"noCleanup + yes", false, true, config.CleanupConfig{Auto: false, Prompt: true}}, + {"noCleanup + cfg.Auto", false, false, config.CleanupConfig{Auto: true, Prompt: true}}, + {"noCleanup + every knob", true, true, config.CleanupConfig{Auto: true, Prompt: true}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := resolveCleanupConfig(true, tc.autoCleanup, tc.yes, nil, tc.cfg) + if !got.NonInteractive { + t.Errorf("NonInteractive = false, want true (--no-cleanup must always win)") + } + if got.AutoDeleteAll { + t.Errorf("AutoDeleteAll = true, want false (--no-cleanup must always win)") + } + }) + } +} diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 97466b7..8225448 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -79,37 +79,11 @@ func runUpgrade(cmd *cobra.Command, args []string) error { skipMigrate := noMigrate || !cfg.Packages.Migrate _ = skipMigrate // referenced below in snapshot/restore sections - // Cleanup behavior toggles. Resolution order (highest first): - // --no-cleanup : never prompt, never delete - // --cleanup : auto-delete all (no all-or-nothing prompt) - // --cleanup-version : delete only these versions - // cfg.Cleanup.Auto : auto-delete all (no prompt) - // cfg.Cleanup.Prompt=false: skip the per-version confirm - // default : prompt, one y/N per cleanup action - // - // The cleanupConfig struct is what runCleanupPrompt consumes. - cleanupCfg := cleanupConfig{ - NonInteractive: noCleanup, - PerVersion: cfg.Cleanup.Prompt, - Prefiltered: cleanupVersions, - } - switch { - case noCleanup: - // already set; no other knobs apply - case autoCleanup: - cleanupCfg.AutoDeleteAll = true - cleanupCfg.Prefiltered = cleanupVersions // combine with --cleanup-version if both set - case cfg.Cleanup.Auto: - cleanupCfg.AutoDeleteAll = true - cleanupCfg.Prefiltered = cleanupVersions - } - // --yes implies auto-delete-all so non-interactive runs don't hang. - if yes { - cleanupCfg.NonInteractive = false - cleanupCfg.AutoDeleteAll = true - cleanupCfg.PerVersion = false - } - _ = yes + // Resolve the cleanup behavior. The full precedence table lives + // on resolveCleanupConfig; the key contract is that --no-cleanup + // beats --yes ("never prompt, never delete" must not be silently + // overridden by a CI script's -y). See #57. + cleanupCfg := resolveCleanupConfig(noCleanup, autoCleanup, yes, cleanupVersions, cfg.Cleanup) // From here on, anything that mutates disk in a way that needs // replay-by-sentinel bookkeeping is wrapped in a sentinel lifecycle.