Skip to content

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

Merged
dipto0321 merged 1 commit into
mainfrom
fix/packages/manager-name-allowlist
Jul 3, 2026
Merged

fix(packages): validate manager name against allowlist in restore/diff#85
dipto0321 merged 1 commit into
mainfrom
fix/packages/manager-name-allowlist

Conversation

@dipto0321

@dipto0321dipto0321 commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Summary

nodeup packages restore <manager> <version> and nodeup packages diff <manager> … interpolated the user-supplied <manager> slot straight into a snapshot filename (fmt.Sprintf("%s-%s.json", managerName, version)) and then filepath.Join'd it into <DataDir>/snapshots. Because filepath.Join collapses .. segments, a manager name like ../../tmp/evil resolved outside the snapshots directory.

That's a confused-deputy surface: the read snapshot's Packages list is fed straight into npm install -g <name>@<version> with no validation, so an attacker with a local file-placement primitive (a shared temp directory, a cloned repo with a payload filename like <prefix>../../tmp/evil-1.0.0.json) could redirect the migration to install arbitrary packages onto the user's machine.

This PR validates <manager> against a canonical allowlist (detector.IsAllowedManagerName) before any filesystem path is constructed. The allowlist is derived from detector.All(), so it stays in sync with the per-platform registry_*.go build files (e.g. nvm-windows only appears on Windows).

Linked issues

Fixes#51

Type of change

  • fix — bug fix (PATCH bump)
  • feat — new feature (MINOR bump)
  • refactor — no behavior change
  • perf — performance improvement
  • docs — documentation only
  • test — tests only
  • chore — maintenance / dependency bump
  • ci — CI/CD only
  • build — build system only
  • Breaking change (MAJOR bump) — explain below

Checklist

  • Title follows Conventional Commits (feat(scope): subject)
  • I ran make ci locally and it passes
  • I added or updated tests for the change
  • I updated relevant docs (README, docs/, inline godoc)
  • No new linter warnings
  • If breaking: I documented the migration path in the PR body and updated CHANGELOG.md

Screenshots / output

Negative-path before/after on nodeup packages restore:

$ nodeup packages restore ../../tmp/evil 1.0.0
# before: silent path resolution, then `read snapshot: open /etc/tmp/evil-1.0.0.json: …`
# after:
Error: invalid manager name "../../tmp/evil" (allowed: asdf, fnm, mise, n, nodenv, nvm, nvm-windows, volta)

Negative-path on nodeup packages diff:

$ nodeup packages diff ../../tmp/evil 1.0.0 2.0.0
Error: invalid manager name "../../tmp/evil" (allowed: asdf, fnm, mise, n, nodenv, nvm, nvm-windows, volta)

Positive-path stays unchanged:

$ nodeup packages restore fnm 20.0.0
Restored packages for fnm 20.0.0

Files touched

  • internal/detector/manager_names.go — new file. AllowedManagerNames() (derived from All()) and IsAllowedManagerName(name) (case-sensitive lookup). Doc comments explain the security rationale and reference fix(packages): path traversal via unsanitized manager name in packages restore/diff #51.
  • internal/detector/manager_names_test.go — new file. TestAllowedManagerNames_MatchesAll pins AllowedManagerNames() ↔ All() parity; TestIsAllowedManagerName covers traversal payloads (../etc/passwd, /etc/passwd, ../../tmp/evil, ..\..\evil, ./fnm), case-fold negativity (FNM, FnM), near-miss strings (fnmm, fn, lol, fnm, fnm/), and a sanity check that every name in AllowedManagerNames() passes the boolean helper.
  • internal/cli/packages.go — runRestore (positional branch) and runDiff both bail with detector.IsAllowedManagerName(managerName) before constructing any snapshot path. Error message surfaces the offender and the full allowlist so typos remain user-fixable.
  • internal/cli/packages_test.go — new file. End-to-end cobra cmd.Execute() tests pinning the runRestore/runDiff error message (TestRunRestore_RejectsPathTraversal, TestRunDiff_RejectsPathTraversal) plus TestRunRestore_AcceptsCanonicalName to catch a regression that flipped the boolean to reject everything.
  • CHANGELOG.md — ### Fixed entry under [Unreleased] documenting the change with the fix(packages): path traversal via unsanitized manager name in packages restore/diff #51 reference.

dipto0321
fix(packages): validate manager name against allowlist in restore/diff
45eba73
The <manager> positional argument in `nodeup packages restore` and
`nodeup packages diff` was interpolated verbatim into a snapshot
filename via `fmt.Sprintf("%s-%s.json", managerName, version)`, then
`filepath.Join`'d into <DataDir>/snapshots. Because `filepath.Join`
collapses `..` segments, a manager name like `../../tmp/evil`
resolved outside the snapshots directory.
Worse, the read snapshot's `Packages` list is piped straight into
`npm install -g <name>@<version>` with no validation, so an attacker
with a local file-placement primitive (a shared temp dir, a cloned
repo containing a payload filename like
`<prefix>../../tmp/evil-1.0.0.json`) could redirect the migration to
install arbitrary packages onto the user's machine.
Close the confused-deputy surface by validating `managerName` against
a canonical allowlist before any file path is constructed:
- New `detector.AllowedManagerNames()` derives the allowlist from
`detector.All()` so the set stays in sync with per-platform
registry_*.go build files (e.g. nvm-windows only on Windows).
- New `detector.IsAllowedManagerName(name)` does a byte-for-byte
case-sensitive lookup — matching the `--manager` flag's lookup
convention so a future regression that flips the case-fold
surfaces immediately.
- `runRestore` (positional branch) and `runDiff` both bail with
the offender plus the full allowlist in the error message, so a
typo stays user-fixable without grepping docs.
- New tests cover traversal payloads (`../../tmp/evil`,
`../etc/passwd`, `/etc/passwd`, `..\\..\\evil`, `./fnm`),
case-fold negativity (`FNM`, `FnM`), and near-miss strings
(`fnmm`, `fn`, ` fnm`, `fnm/`), plus an end-to-end cobra
`cmd.Execute()` test pinning the runRestore/runDiff error message.
Refs #51
@cocogitto-bot

cocogitto-botBot commented Jul 3, 2026

Copy link
Copy Markdown

✔️ 45eba73 - Conventional commits check succeeded.

@dipto0321
dipto0321 merged commit 2845dfa into mainJul 3, 2026
10 checks passed
@dipto0321
dipto0321 deleted the fix/packages/manager-name-allowlist branch July 3, 2026 03:16
@dipto0321dipto0321 mentioned this pull request Jul 5, 2026
Merged
dipto0321 pushed a commit that referenced this pull request Jul 5, 2026
dipto0321puku-ai-2.8
chore(detector): remove stale fnm not-implemented comment + sentinel/…
5592e84
…pkg-doc collision + dead FileOverlay
Residual stale-comment / dead-code cleanups from the 2026-07-03 re-review
that didn't belong in any of the active fix-wave PRs. All deletions /
docstring edits — no behavior change.
- internal/detector/fnm.go: delete ErrFNMNotImplemented (zero call
sites, no tests, mutations have been real since PR #56); rewrite
the FNM struct doc to describe the actual implemented surface
(detection + mutations + layout queries) instead of "Phase 1 only,
mutations not implemented"; drop the "left as stubs in Phase 1"
paragraph from the mutation section.
- internal/packages/sentinel.go: demote `// Package sentinel
implements…` to `// Sentinel file handling: …` — godoc attributes
package-doc comments to the actual package name (`packages`), so
the rendered godoc was wrong. Verified no canonical `// Package
packages …` comment exists elsewhere in the package, so this was
the sole package-level doc and the move doesn't leave the package
undocumented.
- internal/config/resolve.go: delete superseded FileOverlay() (the
coarse all-fields overlay that marked explicit-zero values as set
— exactly the bug FileOverlayFromNode was introduced to fix in
#85). Update the Overlay type's doc comment to point at
FileOverlayFromNode as the file-layer builder.
Closes#104.
Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
dipto0321 pushed a commit that referenced this pull request Jul 5, 2026
dipto0321puku-ai-2.8
chore(detector): stale fnm comment, sentinel pkg-doc, dead FileOverlay
b2a95b6
Residual stale-comment / dead-code cleanups from the 2026-07-03
re-review that didn't belong in any of the active fix-wave PRs. All
deletions / docstring edits — no behavior change.
- internal/detector/fnm.go: delete ErrFNMNotImplemented (zero call
sites, no tests, mutations real since PR #56); rewrite the FNM
struct doc to describe the actual surface (detection + mutations
+ layout queries) instead of "Phase 1 only, not implemented"; drop
the "left as stubs in Phase 1" paragraph from the mutation section.
- internal/packages/sentinel.go: demote "// Package sentinel
implements..." to "// Sentinel file handling: ..." — godoc
attributes package-doc comments to the actual package name
(`packages`), so the rendered godoc was wrong. Verified no
canonical "// Package packages ..." comment exists elsewhere, so
the package isn't left undocumented.
- internal/config/resolve.go: delete superseded FileOverlay() (the
coarse all-fields overlay that marked explicit-zero values as set
— exactly the bug FileOverlayFromNode was introduced to fix in
#85). Update the Overlay type's doc comment to point at
FileOverlayFromNode as the file-layer builder.
Closes#104.
Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(packages): path traversal via unsanitized manager name in packages restore/diff

1 participant

@dipto0321