Phase 60: lpm add source delivery from any registry#19
Merged
Conversation
Behavior-preserving refactor extracting the two private routed-tarball helpers from install.rs (download_tarball_routed, download_tarball_streaming_routed) onto RegistryClient as public methods. Both `lpm install` and the upcoming Phase 60 `lpm add` source- delivery flow consume the same Custom-route auth-attachment logic. - crates/lpm-registry/src/client.rs: add public methods - crates/lpm-cli/src/commands/install.rs: switch all 5 call sites to the new methods; delete the private helpers; remove the now-unused DownloadedTarball import All 602 install + npmrc tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a three-tier version-spec resolver on PackageMetadata covering dist-tag → exact-version → semver-range, mirroring the canonical pattern at install_global.rs:368-405 verbatim. Pre-Phase-60, `lpm add react@beta`, `next@canary`, `lodash@^4` all failed because PackageMetadata::version() is a pure HashMap lookup — none of those literal strings exist as concrete versions. The new helper closes the gap. Per D3 (preplan): both parse-failure and no-satisfying-version return LpmError::Script (matching install_global verbatim) so the Phase 60.1 migration of the four duplicate sites (install_global, install, update_global, global) is a true behavior-preserving refactor. 9 unit tests cover dist-tag (latest/beta/canary), exact match, caret/tilde range, no-satisfying error, parse-fail error, and empty-versions error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stry
Decouple `lpm add` from LPM-only package identity, mirror install's
full .npmrc setup, switch to file-spool tarball download, add
destination-side path containment, gate dep auto-install on
lpm.config.json presence, and surface external imports for the simple
path. End-to-end flow now works for any package on any registry the
rust client can reach (lpm.dev worker, npmjs.org direct, .npmrc-
declared private registries).
60.0.a + 60.0.b — Identity refactor + drop dotted-name auto-prepend
- New AddTarget enum: Lpm(PackageName) | Npm { spec: String }.
- New resolve_add_target replaces parse_package_ref. No rewriting
outside the @lpm.dev/ scope — `lodash.merge`, `tolga.foo`, etc.
resolve to AddTarget::Npm verbatim. Fixes a long-standing
correctness bug: pre-Phase-60 dotted bare names were silently
rewritten to @lpm.dev/<name> which doesn't exist on lpm.dev.
- All output / log / JSON sites render via target.display() /
target.json_name() — `name.scoped()` no longer used unconditionally.
- Skills branch type-encoded via `let AddTarget::Lpm(pkg) = &target`
pattern, with a why-comment (60.2) explaining the scope gate
(lpm.dev runs LLM scans on shipped skill content; arbitrary npm
packages are not scanned).
60.0.c — Mirror install's full .npmrc setup
- Build RouteTable::from_env_and_filesystem before any network call.
- Surface npmrc_warnings (non-JSON) and the strict-ssl=false security
warning (escapes --json). Clone the client with with_tls_overrides
so cafile= / strict-ssl=false take effect on metadata + tarball
fetches. Mirrors install.rs:3295-3445.
60.0.d — Routed metadata + file-spool tarball
- Metadata: AddTarget::Lpm uses get_package_metadata; AddTarget::Npm
uses get_npm_metadata_routed.
- Tarball: client.download_tarball_routed (D2 promoted helper) +
lpm_extractor::extract_tarball_from_file. Bounded memory via
MAX_COMPRESSED_TARBALL_SIZE (500 MB) for free; lpm add typescript
(~22 MB) and worst-case @scope/giant-fixture no longer load the
whole tarball into RAM.
60.0.f — Destination-side path containment (D6)
- New resolve_safe_dest helper canonicalizes target_dir once and
validates every write destination: refuses to follow existing
symlinks, rejects writes whose canonical parent escapes the target
root. Wired into the Step 8 file-copy loop. Closes the threat-model
gap that opened up when add expanded from "trusted lpm.dev
publishers" to "any npm publisher."
60.1 — Dep gate + bare-imports notice (D4)
- Tighten dep gate: `if !no_install_deps && lpm_config.is_some()`.
Simple path is download-manager: copy bytes, no auto-install.
- import_rewriter exports a sibling collect_bare_specifiers fn that
shares an internal SpecifierKind classifier with rewrite_imports
(anti-drift contract — "bare" means the same thing in both places).
- add.rs surfaces the collected externals as a non-JSON notice and
as a `external_imports` array in the JSON output.
60.1.5 — Non-interactive simple-path guard
- `lpm_config.is_none() && target_path.is_none() && (yes || json ||
!is_tty)` errors before the file-copy loop. Heuristically defaulting
components/ for arbitrary 3rd-party source under --yes/--json/non-TTY
is a CI/automation footgun.
Tests
- 15 unit tests in add.rs (resolve_add_target classification including
the dotted-name regression; resolve_safe_dest contracts including
symlink-refusal on Unix).
- 10 unit tests in import_rewriter.rs (classify_specifier,
collect_bare_specifiers).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…raversal Three new wiremock-driven integration tests covering the highest-value end-to-end scenarios for Phase 60: - add_simple_non_interactive_without_path.rs (4 sub-tests) — proves the 60.1.5 guard fires for --yes, --json, and non-TTY (stdin from /dev/null) without --path; positive control with --path succeeds. No package.json mutation in any failure case. - add_source_npm_simple.rs (2 sub-tests) — full simple-path pipeline via wiremock npm metadata + tarball: AddTarget::Npm resolves, file- spool download, extract, files copied flat (no auto-nest), bare- imports notice lists react + @radix-ui/react-slot, package.json NOT mutated, .lpm/skills/ NOT created. JSON sub-test asserts the package.name uses the npm-style identity (not @lpm.dev/-prefixed) and the new external_imports array is well-shaped. - add_path_traversal_dest_escape.rs — proves resolve_safe_dest is wired into the actual write loop, not just unit-tested in isolation. Tarball ships an lpm.config.json with files[0].dest = "../../escaped/evil.txt" — assertion: containment-violation error, exit non-zero, no file written outside target_dir. Other 60.3 specced tests are either (i) covered by the unit tests that landed alongside the implementation (#5 dotted-name, #9 version- spec, #11 symlink — see preplan v6 audit checklist) or (ii) deliberately deferred where the underlying machinery is already test-covered by Phase 58.x install tests (#1 lpm.dev rich, #2 npm rich, #6 npmrc auth, #7 strict-ssl, #8 missing-var fatal). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Update the lpm add one-liner in the Commands list. - Add a "How lpm add Works" section explaining: source delivery vs. install, the firm naming rule (@lpm.dev/owner.name only), the rich vs. simple paths, and the non-interactive --path requirement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audit reproduced (with a temp-dir filesystem probe) that the landed
resolve_safe_dest helper still created directories OUTSIDE the
target_dir for two attack vectors before the containment error fired:
1. `dest_rel = "../../escaped/evil.txt"` — `Path::join` resolves
lexically; `dest.parent()` lands outside target; `create_dir_all`
ran before the containment check, leaving `<target>/../escaped/`
on disk even though the file write was correctly blocked.
2. Absolute `dest_rel = "/tmp/elsewhere/evil.txt"` — `Path::join` of
an absolute path returns the absolute path verbatim; `parent =
/tmp/elsewhere/`; `create_dir_all` created it before the
containment check fired.
The original integration test only asserted no escaped FILE existed,
so the directory-side-effect bug passed CI.
Fix
- Reorder resolve_safe_dest so EVERY check that can reject the
destination runs BEFORE any filesystem mutation:
Step 1 (NEW) — reject absolute dest_rel up-front.
Step 2 (NEW) — reject any ParentDir / RootDir / Prefix component.
Step 3 — refuse existing-symlink destinations.
Step 4 (NEW) — pre-mkdir ancestor canonicalization: walk up to the
longest existing ancestor; canonicalize; require it under
target_root_canonical (catches symlinked intermediate dirs).
Step 5 — create_dir_all (NOW safe).
Step 6 — post-mkdir re-canonicalize as TOCTOU defense-in-depth.
The lexical bans in Steps 1-2 kill the entire `../escape` and
absolute-path attack classes before any mkdir runs. The longest-
existing-ancestor walk in Step 4 covers the symlinked-intermediate
case (target/foo → /tmp/elsewhere). Step 6 is paranoia.
Tests
- Strengthen unit tests:
- resolve_safe_dest_dotdot_in_path_rejected_with_no_external_dir_created
now asserts no escape directory was created.
- resolve_safe_dest_absolute_dest_rejected_with_no_external_dir_created
is new — covers the absolute-path attack.
- resolve_safe_dest_dotdot_in_middle_of_path_also_rejected covers
`foo/../bar.txt` (lexically resolves back inside but still
rejected up-front).
- Extend integration test:
- dest_escape_via_dotdot_is_refused_and_creates_no_external_directory
now snapshots target_dir entries before the run and asserts no
unexpected new top-level entries appeared, plus no escape dir.
- dest_escape_via_absolute_path_is_refused_and_creates_no_external_directory
is new — covers the absolute-path attack at the integration level.
Net: 4923 → 4926 workspace tests; clippy + fmt clean; all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tolgaergin
added a commit
that referenced
this pull request
Apr 29, 2026
Highlights since v0.27.0: - **Phase 60: `lpm add` source delivery from any registry** (#19). Decouples `lpm add` from the LPM-only package identity. `react`, `lodash.merge`, `@juggle/resize-observer`, `@private/internal-pkg` via `.npmrc` — anything on any registry works. Adds destination- side path containment, file-spool tarball download, version-spec resolution (dist-tags + semver ranges), and a non-interactive simple-path guard. ~44 new tests. - **Resolver default flipped to greedy-fusion** (#20). `lpm install` with no env vars now reaches the fast path (~1s on `bench/fixture-large`) directly. PubGrub-with-split-retry remains as `LPM_RESOLVER=pubgrub` opt-out; `LPM_GREEDY_FUSION=0` still falls back to the legacy walker arm. - **README benchmarks updated** (#21, #22). New round-robin lpm-vs-bun methodology (`bench/scripts/run-readme.sh`) + corrected `bun.lock` wipe in `bench/run.sh` give honest apples-to-apples numbers on `bench/fixture-large` (266 packages, the Phase 49+ ship-gate fixture). Cold install equal-footing: lpm 962ms vs bun 1005ms (0.96×); warm/up-to-date/script-overhead/lint/fmt unchanged. CI gate green on this commit: - cargo clippy --workspace --all-targets -- -D warnings: clean - cargo fmt --check: clean - cargo build --workspace: clean - cargo nextest run --workspace --exclude lpm-integration-tests: 4926/4926 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lpm add <pkg>now works for any package on any registry the rust client can reach (lpm.dev worker, npmjs.org direct,.npmrc-declared private registries) — not just@lpm.dev/owner.name. Two paths inside the command, decided after extraction:lpm.config.jsonpresent): existing behavior — schema prompts, conditional file filtering, conditional dependency installation, importAlias-aware rewrite.lpm.config.json): "download manager" — copy source files verbatim under--path, rewrite imports, NO automatic dep install. Surfaces external/bare imports the user needs to add to their ownpackage.json.What changed
Identity model — new
AddTarget { Lpm(PackageName) | Npm { spec } }enum decoupleslpm addfrom the LPM-onlyPackageNamevalidator. Bare/dotted/non-@lpm.dev/inputs flow toAddTarget::Npmverbatim.Correctness fix — drops the dotted-name auto-prepend. Pre-Phase-60,
lodash.merge/lodash.debounce/tolga.foowere silently rewritten to@lpm.dev/<name>(which doesn't exist on lpm.dev). Per the firm naming rule "only@lpm.dev/owner.nameidentifies an lpm.dev-hosted package," dotted bare names now flow through npm routing..npmrcsetup parity withinstall—RouteTable::from_env_and_filesystembuilds early; npmrc warnings + thestrict-ssl=falsesecurity warning surface unconditionally;clone_with_config().with_tls_overrides(...)ensurescafile=/strict-ssl=falsetake effect onadd's metadata + tarball fetches.Routed metadata + file-spool tarball —
client.download_tarball_routed(D2 promoted helper) +lpm_extractor::extract_tarball_from_file. Bounded memory (MAX_COMPRESSED_TARBALL_SIZE500 MB) for free;lpm add typescript(~22 MB) and worst-case@scope/giant-fixtureno longer load whole tarballs into RAM.Version-spec resolution — new
PackageMetadata::resolve_version_speccovers dist-tag (react@beta,next@canary), exact, and semver-range (lodash@^4). Pre-Phase-60 these all failed becausemetadata.version()was a pure HashMap lookup. Helper landed inlpm-registry;addconsumes it; install/install_global/update_global/global migration deferred to Phase 60.1 to avoid behavior drift.Destination-side path containment — new
resolve_safe_destrejects absolute paths,..components, existing-symlink destinations, and intermediate symlinks pointing outsidetarget_dir. Wired into the file-copy loop. Includes an audit fix (commit5b90507) reordering the helper so all rejects run BEFORE anycreate_dir_all— pre-fix,../absolute attacks left stray external directories on disk even though file writes were blocked.Dep gate + bare-imports notice — dep auto-install now requires
lpm.config.jsonto be present. Simple path is download-manager: copy bytes, surface external imports via the newimport_rewriter::collect_bare_specifiers(sibling torewrite_imports, sharing aSpecifierKindclassifier so "bare" never drifts between them).Non-interactive simple-path guard —
--yes/--json/ non-TTY without--patherrors before any file write. Heuristically defaulting tocomponents/for arbitrary 3rd-party source under non-interactive use is a CI/automation footgun.D2 prep refactor (behavior-preserving) — promotes
download_tarball_routedanddownload_tarball_streaming_routedfrom private install.rs helpers to public methods onRegistryClient. Bothinstallandaddconsume the same Custom-route auth-attachment logic. 602/602 install + npmrc tests still pass after the switch.Test plan
cargo clippy --workspace -- -D warningscleancargo fmt --checkcleancargo build --workspacecleancargo nextest run --workspace --exclude lpm-integration-tests --no-fail-fast— 4926/4926 pass (was 4882 pre-phase-60; +44 new tests)fancy-regexintroducedPhase 60.1 follow-ups (queued, not blocking merge)
add's own test surfaceinstall_global/install/update_global/globalversion-spec resolution to call the newPackageMetadata::resolve_version_spechelperlpm add owner.thingexamples (README/marketing/blog/vision docs) — owner-taskOut of scope (recommended as a separate PR)
Flipping the global install resolver default from PubGrub → greedy-fusion. Today's logic at install.rs:3825-3826 is
LPM_RESOLVER == "greedy" && LPM_GREEDY_FUSION != "0"— fusion is the default for the greedy arm, not globally. Phase 56 W4 made greedy-fusion the default for that arm (and it's the default in our bench harness). Flipping it as the global install default touches every user who today runslpm installwithout env vars — that's its own ship-gate (n=20+ bench, edge-case resolution audit on currently-PubGrub-default users) and shouldn't be bundled withlpm addsource-anywhere. Strongly recommend a follow-up PR.🤖 Generated with Claude Code