From 01c528a3278d0ea65c4e828cdbf50761df39394d Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 09:33:32 +0600 Subject: [PATCH] fix(detector): replace n.Current()'s silent-download call site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix, N.Current() ran `n current` — an undocumented subcommand in upstream tj/n (bin/n). `n current` is not a first-class case arm in the dispatch block; it falls through to the default `*` arm which calls `install "$1"`. Inside `install`, the string "current" matches the version-label branch in display_remote_versions (same branch as "latest"), and the function downloads the latest Node.js tarball, extracts it, and calls activate. So every Current() invocation would silently side-effect-install the newest available Node version behind the user's back — every run, on every n-using machine. Callers treat Current() errors as "active version unknown, don't exclude it" (the safe-by-convention default per the Manager interface doc), so this also silently defeated the cleanup safety net on every n install. Replacement: shell out to `$N_PREFIX/bin/node --version` and parse the output. n's `activate` function copies the active node binary into $N_PREFIX/bin/node, so that binary's --version IS the active version. Side-effect-free, matches every supported n install, and the parser handles both vX.Y.Z and bare X.Y.Z output shapes (the latter is defensive against n forks). Tests: - TestParseNNodeVersion_* (4): parser correctness for v-prefixed, bare, whitespace-padded, empty, and unparseable input. The pre-fix parser's name was parseNCurrent — the rename is the most visible signal that the contract changed. - TestN_Current_InvokesNodeVersionNotNCurrent: pins the new behavior at the integration level. The literal "n current" request is the ONE THING the fixture asserts MUST NOT appear; any regression that re-introduces the bug fails immediately with a t.Fatalf citing the request. - TestN_Current_PropagatesRunShellError: missing node binary or exec failure surfaces as an error so callers treat it as "active unknown". - TestN_Current_PropagatesParseError: blank --version output errors rather than returning the zero semver. The pre-fix tests (TestParseNCurrent_*, TestNCurrent_InvokesShell) are deleted — they codified the buggy behavior. Refs #59 --- CHANGELOG.md | 31 +++++++++++ internal/detector/n.go | 74 ++++++++++++++++++------- internal/detector/n_test.go | 105 +++++++++++++++++++++++++++++++----- 3 files changed, 179 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85702d5..4a045f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -146,6 +146,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 for the full rationale. ### Fixed +- `internal/detector/n.go`: `N.Current()` no longer shells out + to `n current` — an undocumented subcommand that upstream + `tj/n` resolves as a label equivalent to "latest" and + side-effects a download of the newest available Node.js + version. Pre-fix, every `Current()` invocation would have + silently mutated the user's machine by running the + catch-all arm `install "$1"` for "current", which fetches + the latest semver from nodejs.org, downloads the tarball, + extracts it, and activates it. Since callers treat + `Current()` errors as "active version unknown, don't + exclude it" (the safe-by-convention default per the + `Manager` interface doc), every n-using machine ran the + cleanup step without active-version exclusion on every + invocation — and would now also have the latest version + installed behind the user's back. The replacement reads + the active version off `$N_PREFIX/bin/node --version` + instead: n's `activate` function copies the active + node binary into that path, so its `--version` is the + authoritative source for "what's active". Side-effect-free, + matches every supported n install, and the parser + (`parseNNodeVersion`) handles both `vX.Y.Z` and bare + `X.Y.Z` shapes. Doc comments at the top of `n.go` and + the mutation-methods block both spell out the "do not + use `n current`" reasoning. New tests in `n_test.go` pin + the new path (`TestN_Current_InvokesNodeVersionNotNCurrent` + explicitly fails if the literal `n current` request re- + appears), parser cases (`TestParseNNodeVersion_*`), and + error propagation (`TestN_Current_PropagatesRunShellError`, + `TestN_Current_PropagatesParseError`). The pre-fix tests + (`TestParseNCurrent_*`, `TestNCurrent_InvokesShell`) are + deleted — they codified the buggy behavior. Closes #59. - `internal/cli` (`upgrade.go`): `Manager.Current()` failure no longer leaves the active Node.js version unprotected from cleanup deletion. Pre-fix, the post-upgrade cleanup step diff --git a/internal/detector/n.go b/internal/detector/n.go index 375b207..c359c29 100644 --- a/internal/detector/n.go +++ b/internal/detector/n.go @@ -238,7 +238,8 @@ func parseNInstalled(stdout string) ([]semver.Version, error) { // --- Mutation methods ---------------------------------------------------- // // Install, Uninstall, Use, SetDefault, GlobalNpmPrefix, and Current -// for n all shell out to the `n` binary through runShell. +// for n all shell out to either the `n` binary or to the active +// node binary through runShell. // // Important n specifics: // - Install / Use take a bare ``: `n install `, `n `. @@ -250,9 +251,13 @@ func parseNInstalled(stdout string) ([]semver.Version, error) { // - GlobalNpmPrefix follows n's on-disk layout: each version is // in $N_PREFIX/n/versions/node// with the global prefix // at .../lib/node_modules. -// - Current is derived from `n current` (added in n 8+); older -// n versions return a non-zero exit, which we propagate as an -// error. +// - Current is derived from `/bin/node --version` — +// n's `activate` copies the active node binary into that +// path, so its `--version` reports the active version. We +// deliberately do NOT use `n current`: that string falls +// through to upstream's default dispatch arm which calls +// `install`, side-effecting a download of the latest +// version. See #59. // Install runs `n install `. n auto-uses the newly-installed // version, so there's no separate "default" step. @@ -338,29 +343,60 @@ func nPrefix() string { } // Current returns the version n currently has active for the user. -// Source: `n current` (added in n 8+). The output is a bare semver -// like "22.11.0". On older n versions that don't have `n current`, -// the call returns a non-zero exit code and we propagate that error. +// +// Strategy: invoke `/bin/node --version` and parse the +// output. n's `activate` function (bin/n) copies the active +// version's `bin/node` into `$N_PREFIX/bin/node`, so the active +// node binary's `--version` IS the active version — without +// requiring a separate "what's active?" subcommand. +// +// Why not `n current`? That subcommand doesn't exist as a +// first-class dispatch arm in upstream `tj/n` (bin/n's case +// statement). `n current` falls through to the default `*)` +// arm which calls `install "$1"`, which resolves "current" as +// a label equivalent to "latest" and **downloads and installs +// the newest available Node.js version**. So calling +// `N.Current()` would have silently mutated the user's machine +// on every invocation — defeating the cleanup safety net on +// every n install, every run. See #59. +// +// On platforms where `$N_PREFIX` is empty (the helper returned +// ""), or `$N_PREFIX/bin/node` is missing, or the subprocess +// fails, we return an error — the caller treats that as +// "active version unknown, don't exclude it" (the safe-by- +// convention default per the Manager interface doc). func (n *N) Current() (semver.Version, error) { - res, err := runShell(context.Background(), "n", "current") + prefix := nPrefix() + if prefix == "" { + return semver.Version{}, errors.New("n: cannot resolve N_PREFIX or /usr/local") + } + nodeBin := filepath.Join(prefix, "bin", "node") + // We don't pre-Stat nodeBin here — let the shell-out error + // surface the "not installed" case in a single round-trip + // (the runShell error message will already name the missing + // path), rather than paying for a stat that we'd race + // against a concurrent uninstall. + res, err := runShell(context.Background(), nodeBin, "--version") if err != nil { - return semver.Version{}, fmt.Errorf("n current: %w", err) + return semver.Version{}, fmt.Errorf("n current: %s --version: %w", nodeBin, err) } - return parseNCurrent(res.Stdout) + return parseNNodeVersion(res.Stdout) } -// parseNCurrent extracts the active version from `n current` output. -// Exposed (lowercase) for direct unit testing. +// parseNNodeVersion extracts the active version from +// `/bin/node --version` output. Exposed (lowercase) +// for direct unit testing. // -// Real observed output (n 10.2.0): +// Real observed output (node 22.11.0): // -// 22.11.0 +// v22.11.0 // // We take the first non-empty line, trim whitespace, strip an -// optional "v" prefix, and feed the remainder to semver.NewVersion. -// Older n versions that don't support `current` exit non-zero -// before stdout — that error is propagated by runShell. -func parseNCurrent(stdout string) (semver.Version, error) { +// optional "v" prefix, and feed the remainder to +// semver.NewVersion. node(1) has been emitting the +// `vX.Y.Z` form since its earliest public releases, so this is +// stable across every n-supported Node version. +func parseNNodeVersion(stdout string) (semver.Version, error) { for _, raw := range strings.Split(stdout, "\n") { line := strings.TrimSpace(raw) if line == "" { @@ -372,5 +408,5 @@ func parseNCurrent(stdout string) (semver.Version, error) { } return *v, nil } - return semver.Version{}, errors.New("n current returned empty output") + return semver.Version{}, errors.New("n current: returned empty output") } diff --git a/internal/detector/n_test.go b/internal/detector/n_test.go index dbe91b6..f20eeab 100644 --- a/internal/detector/n_test.go +++ b/internal/detector/n_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/Masterminds/semver/v3" @@ -530,10 +531,30 @@ func TestN_Uninstall_PropagatesError(t *testing.T) { } } -// --- parseNCurrent ---------------------------------------------------- +// --- parseNNodeVersion (the new Current() parser) --------------------- +// +// These tests cover the parser that reads `/bin/node +// --version` output. The pre-fix code parsed `n current` output, +// which no longer exists in the upstream-detector flow — that +// subcommand was undocumented and side-effecting. See #59. + +func TestParseNNodeVersion_VPrefixedReal(t *testing.T) { + // Real observed output of `node --version`: + // $ node --version + // v22.11.0 + v, err := parseNNodeVersion("v22.11.0\n") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if v.String() != "22.11.0" { + t.Errorf("got %q, want %q", v.String(), "22.11.0") + } +} -func TestParseNCurrent_Bare(t *testing.T) { - v, err := parseNCurrent("22.11.0\n") +func TestParseNNodeVersion_Bare(t *testing.T) { + // Defensive: some node builds / forks may omit the "v" + // prefix. The parser must still produce a valid semver. + v, err := parseNNodeVersion("22.11.0\n") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -542,8 +563,8 @@ func TestParseNCurrent_Bare(t *testing.T) { } } -func TestParseNCurrent_WithPrefix(t *testing.T) { - v, err := parseNCurrent("v22.11.0\n") +func TestParseNNodeVersion_LeadingTrailingWhitespace(t *testing.T) { + v, err := parseNNodeVersion(" v22.11.0 \n") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -552,23 +573,45 @@ func TestParseNCurrent_WithPrefix(t *testing.T) { } } -func TestParseNCurrent_Empty(t *testing.T) { - _, err := parseNCurrent("") +func TestParseNNodeVersion_Empty(t *testing.T) { + _, err := parseNNodeVersion("") if err == nil { t.Error("expected error on empty input") } } -func TestNCurrent_InvokesShell(t *testing.T) { +func TestParseNNodeVersion_Unparseable(t *testing.T) { + // Anything other than a bare semver (with or without "v") + // must error, not silently return zero value. + _, err := parseNNodeVersion("not-a-version\n") + if err == nil { + t.Error("expected error on unparseable input, got nil") + } +} + +// --- N.Current() tests ------------------------------------------------ +// +// These tests pin that Current() (a) does NOT call `n current` +// — the silent-download foot-gun — and (b) does call the active +// node binary at $N_PREFIX/bin/node --version. The fixture stubs +// `runShell` so we can assert the EXACT shell-out string without +// needing a real n install in CI. + +func TestN_Current_InvokesNodeVersionNotNCurrent(t *testing.T) { + // The bug: pre-fix code called `n current`, which falls + // through to upstream's default dispatch arm and downloads + // the latest Node. A regression that re-introduced that + // would be caught HERE: the literal `n current` request is + // the one thing this test asserts MUST NOT appear. var captured string withStubShell(t, nil, func(req string) (*platform.RunResult, error) { captured = req - if req != "n current" { - t.Errorf("unexpected runShell call: %q", req) + if req == "n current" { + t.Fatalf("Current() must not invoke `n current` — it side-effects a download; got %q", req) } - return &platform.RunResult{Stdout: "22.11.0\n"}, nil + return &platform.RunResult{Stdout: "v22.11.0\n"}, nil }, ) @@ -580,6 +623,44 @@ func TestNCurrent_InvokesShell(t *testing.T) { t.Errorf("got %q, want %q", got.String(), "22.11.0") } if captured == "" { - t.Error("expected n current to be invoked") + t.Error("expected a runShell call (the new path resolves /bin/node --version)") + } + // Sanity: the captured request should end in `--version` + // (we don't pin the absolute path because N_PREFIX is + // platform-dependent — /usr/local on Linux/macOS, whatever + // the test env sets, etc.). + if !strings.HasSuffix(captured, "--version") { + t.Errorf("captured request = %q, want one that ends in `--version`", captured) + } +} + +func TestN_Current_PropagatesRunShellError(t *testing.T) { + // If /bin/node is missing or fails to exec, we + // must propagate the error so callers treat Current() as + // "active version unknown" — the safe default. + wantErr := errors.New("simulated missing node binary") + withStubShell(t, nil, func(req string) (*platform.RunResult, error) { + return nil, wantErr + }) + + _, err := NewN().Current() + if err == nil { + t.Fatal("expected error, got nil") + } + if !errors.Is(err, wantErr) { + t.Errorf("error %v should wrap %v", err, wantErr) + } +} + +func TestN_Current_PropagatesParseError(t *testing.T) { + // The shell-out succeeded but the body is unparseable. We + // must return an error, not the zero semver.Version. + withStubShell(t, nil, func(req string) (*platform.RunResult, error) { + return &platform.RunResult{Stdout: ""}, nil + }) + + _, err := NewN().Current() + if err == nil { + t.Error("expected parsing error from blank --version output, got nil") } }