fix: reject path-traversal segments in dependency aliases#11954
Conversation
📝 WalkthroughWalkthroughThis PR implements a security fix that rejects dependency aliases containing path-traversal segments across symlink creation and manifest reading in both TypeScript and Rust codebases. Validation rules prevent malicious registry packages from escaping ChangesPath-traversal dependency alias rejection
Sequence Diagram(s)sequenceDiagram
participant getWantedDependencies
participant assertValidDependencyAliases
participant isValidDependencyAlias
getWantedDependencies->>assertValidDependencyAliases: validate deps/dev/optional/peer
assertValidDependencyAliases->>isValidDependencyAlias: check alias
isValidDependencyAlias-->>assertValidDependencyAliases: valid/invalid
assertValidDependencyAliases-->>getWantedDependencies: throw on invalid / continue
sequenceDiagram
participant symlinkDependency
participant safeJoinModulesDir
participant Filesystem
symlinkDependency->>safeJoinModulesDir: modulesDir + alias
safeJoinModulesDir->>Filesystem: path.resolve & containment check
Filesystem-->>safeJoinModulesDir: resolved path
safeJoinModulesDir-->>symlinkDependency: safe path / throw ERR_PNPM_INVALID_DEPENDENCY_NAME
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11954 +/- ##
==========================================
- Coverage 87.94% 87.91% -0.03%
==========================================
Files 227 228 +1
Lines 27720 27783 +63
==========================================
+ Hits 24377 24426 +49
- Misses 3343 3357 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.95818051508,
"stddev": 0.035556901196774286,
"median": 1.9651763528800001,
"user": 2.8140999599999996,
"system": 3.2582006199999993,
"min": 1.89515174638,
"max": 2.01994219838,
"times": [
1.93930913138,
1.9810207803800002,
1.89515174638,
1.9672322793800001,
1.96864919538,
1.95237013538,
1.9631204263800002,
1.9809415913800001,
2.01994219838,
1.91406766638
]
},
{
"command": "pacquet@main",
"mean": 1.96683812958,
"stddev": 0.07016845457288398,
"median": 1.9538168843800001,
"user": 2.7912932599999998,
"system": 3.2959650199999997,
"min": 1.8511664753800001,
"max": 2.10299755638,
"times": [
1.8511664753800001,
1.93551273338,
2.10299755638,
1.9232083963800002,
1.95451731138,
2.01555324338,
1.9856543753800002,
1.91694993038,
1.9531164573800002,
2.02970481638
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6597742123600001,
"stddev": 0.013591103190721952,
"median": 0.65742019776,
"user": 0.37280006,
"system": 1.35919912,
"min": 0.64472460576,
"max": 0.68431639876,
"times": [
0.68431639876,
0.6516630987600001,
0.6594301367600001,
0.6517710507600001,
0.65574463076,
0.64472460576,
0.66411041276,
0.64553668076,
0.65909576476,
0.68134934376
]
},
{
"command": "pacquet@main",
"mean": 0.6916908322600002,
"stddev": 0.02954030629238518,
"median": 0.68958187276,
"user": 0.37921225999999997,
"system": 1.3430714199999998,
"min": 0.63960640376,
"max": 0.7527580057600001,
"times": [
0.7527580057600001,
0.6737167617600001,
0.69126526976,
0.69847250876,
0.68740496376,
0.63960640376,
0.6883946457600001,
0.6754621817600001,
0.7190584817600001,
0.69076909976
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3061674414000004,
"stddev": 0.04341973689891585,
"median": 2.2961654904,
"user": 3.93782328,
"system": 3.0469047,
"min": 2.2733918774000004,
"max": 2.4183190034,
"times": [
2.3230439314,
2.4183190034,
2.3132737054000003,
2.2737090514,
2.2791378774,
2.3116531524000004,
2.3033781734,
2.2889528074000003,
2.2733918774000004,
2.2768148344
]
},
{
"command": "pacquet@main",
"mean": 2.2949637178,
"stddev": 0.04697037589983299,
"median": 2.3124699794000003,
"user": 3.89964978,
"system": 3.0319968,
"min": 2.2206985494,
"max": 2.3445672404,
"times": [
2.3005612314,
2.2437504814,
2.2206985494,
2.2947927094000002,
2.3243787274,
2.3354171364000003,
2.3312429924,
2.2282733154,
2.3259547944000003,
2.3445672404
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.47999714798,
"stddev": 0.02318765092117898,
"median": 1.4805123129800002,
"user": 1.7610468999999997,
"system": 1.85264296,
"min": 1.44459821898,
"max": 1.50938939098,
"times": [
1.50938939098,
1.44459821898,
1.50210260598,
1.5059666919799999,
1.47756063598,
1.4474723629800001,
1.47513757098,
1.4920260489800001,
1.46225396298,
1.4834639899800002
]
},
{
"command": "pacquet@main",
"mean": 1.4836913176800002,
"stddev": 0.0254444235031174,
"median": 1.4746256774800002,
"user": 1.7550808999999998,
"system": 1.8377085599999998,
"min": 1.45297654598,
"max": 1.5350665869800002,
"times": [
1.47255949998,
1.45297654598,
1.47669185498,
1.5350665869800002,
1.51832510898,
1.47154424198,
1.46878439198,
1.47736454898,
1.46672381298,
1.49687658398
]
}
]
} |
|
Bencher Report| Branch | pr/11954 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,306.17 ms(-26.59%)Baseline: 3,141.32 ms | 3,769.59 ms (61.18%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,480.00 ms(-33.46%)Baseline: 2,224.06 ms | 2,668.87 ms (55.45%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,958.18 ms(-7.45%)Baseline: 2,115.89 ms | 2,539.07 ms (77.12%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 659.77 ms(+0.17%)Baseline: 658.67 ms | 790.41 ms (83.47%) |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fs/symlink-dependency/test/safeJoinModulesDir.test.ts (1)
8-34: ⚡ Quick winAdd regression coverage for aliases that resolve to
node_modulesitself.The guard branch that rejects
resolvedLink === resolvedDiris security-relevant and currently untested. Please add''/'.'alias cases.Proposed test addition
+test.each(['', '.'])('symlinkDependency refuses alias %p when it resolves to node_modules root', async (alias) => { + const tmp = tempDir(false) + const destModulesDir = path.join(tmp, 'node_modules') + fs.mkdirSync(destModulesDir) + await expect( + symlinkDependency(path.join(tmp, 'dep'), destModulesDir, alias) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fs/symlink-dependency/test/safeJoinModulesDir.test.ts` around lines 8 - 34, Add regression tests that exercise the guard branch which rejects when resolvedLink === resolvedDir by adding alias cases that point at the node_modules directory itself (e.g. '' and '.') for symlinkDependency, symlinkDependencySync and symlinkDirectRootDependency; for each of the three existing tests (the async symlinkDependency, the sync symlinkDependencySync, and symlinkDirectRootDependency) add additional assertions that calling the functions with alias '' and '.' rejects/throws with an error containing code 'ERR_PNPM_INVALID_DEPENDENCY_NAME', using the same tmp/node_modules setup as the existing tests so the new cases cover the path-equals-dir check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@fs/symlink-dependency/test/safeJoinModulesDir.test.ts`:
- Around line 8-34: Add regression tests that exercise the guard branch which
rejects when resolvedLink === resolvedDir by adding alias cases that point at
the node_modules directory itself (e.g. '' and '.') for symlinkDependency,
symlinkDependencySync and symlinkDirectRootDependency; for each of the three
existing tests (the async symlinkDependency, the sync symlinkDependencySync, and
symlinkDirectRootDependency) add additional assertions that calling the
functions with alias '' and '.' rejects/throws with an error containing code
'ERR_PNPM_INVALID_DEPENDENCY_NAME', using the same tmp/node_modules setup as the
existing tests so the new cases cover the path-equals-dir check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a664aa9b-246b-4300-8e88-ab7762927083
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/reject-traversal-dependency-aliases.mdfs/symlink-dependency/src/index.tsfs/symlink-dependency/src/safeJoinModulesDir.tsfs/symlink-dependency/src/symlinkDirectRootDependency.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/package.jsoninstalling/deps-resolver/src/getNonDevWantedDependencies.tsinstalling/deps-resolver/src/getWantedDependencies.tsinstalling/deps-resolver/src/validateDependencyAlias.tsinstalling/deps-resolver/test/validateDependencyAlias.test.tspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
fs/symlink-dependency/src/index.tsinstalling/deps-resolver/test/validateDependencyAlias.test.tsinstalling/deps-resolver/src/validateDependencyAlias.tsinstalling/deps-resolver/src/getWantedDependencies.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/src/getNonDevWantedDependencies.tsfs/symlink-dependency/src/symlinkDirectRootDependency.tsfs/symlink-dependency/src/safeJoinModulesDir.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use util.types.isNativeError() instead of instanceof Error for error type checking in Jest tests
Files:
installing/deps-resolver/test/validateDependencyAlias.test.tsfs/symlink-dependency/test/safeJoinModulesDir.test.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
🧠 Learnings (7)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
fs/symlink-dependency/src/index.tsinstalling/deps-resolver/test/validateDependencyAlias.test.tsinstalling/deps-resolver/src/validateDependencyAlias.tsinstalling/deps-resolver/src/getWantedDependencies.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/src/getNonDevWantedDependencies.tsfs/symlink-dependency/src/symlinkDirectRootDependency.tsfs/symlink-dependency/src/safeJoinModulesDir.ts
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
installing/deps-resolver/package.json
🔇 Additional comments (15)
fs/symlink-dependency/src/safeJoinModulesDir.ts (1)
1-19: LGTM!fs/symlink-dependency/src/index.ts (1)
4-4: LGTM!Also applies to: 13-13, 23-23
fs/symlink-dependency/src/symlinkDirectRootDependency.ts (1)
11-11: LGTM!Also applies to: 49-49
installing/deps-resolver/package.json (1)
73-73: LGTM!Also applies to: 85-86
installing/deps-resolver/src/validateDependencyAlias.ts (1)
1-31: LGTM!installing/deps-resolver/test/validateDependencyAlias.test.ts (1)
1-68: LGTM!installing/deps-resolver/src/getNonDevWantedDependencies.ts (1)
4-5: LGTM!Also applies to: 15-25
installing/deps-resolver/src/getWantedDependencies.ts (1)
9-10: LGTM!Also applies to: 28-31
pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
14-20: LGTM!pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
59-60: LGTM!Also applies to: 82-83
pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs (1)
1-60: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
106-118: LGTM!Also applies to: 160-170, 601-605, 791-800, 802-822, 824-830
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
158-170: LGTM!pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
320-364: LGTM!.changeset/reject-traversal-dependency-aliases.md (1)
1-7: LGTM!
A transitive registry package can use a dependency-alias key like `@x/../../../../../.git/hooks` to make `pnpm install` create a symlink outside the intended `node_modules` directory, since pnpm passes the alias straight into `path.join(modulesDir, alias)` without checking that the joined path stays inside `modulesDir`. Reject aliases that aren't a single `name` or `@scope/name` shape at manifest-read time (both the importer's manifest and every transitive package manifest) and re-check at the symlink layer as defense in depth. Mirror the fix in pacquet's deps-resolver. --- Written by an agent (Claude Code, claude-opus-4-7).
Perfectionist's `prefer-raw-string` lint rejects the two backslash-escaped test inputs. --- Written by an agent (Claude Code, claude-opus-4-7).
…name An alias is the directory name pnpm creates inside `node_modules`, so the only valid shapes are a single `name` or `@scope/name` consisting of URL-friendly characters with no leading `.` / `_`, and not equal to reserved names such as `node_modules`. That's the same `validForOldPackages` rule `parseWantedDependency` already applies to CLI-given names — the manifest-read path should match. Route both stacks through it so `.bin`, `.pnpm`, `node_modules`, `favicon.ico`, whitespace, and non-URL-friendly characters are all rejected alongside the path-traversal shapes the narrow validator caught. --- Written by an agent (Claude Code, claude-opus-4-7).
…odulesDir The two-step pattern of "assert the alias stays in the dir" then "join the dir and the alias" left it possible for a caller to use the join without the assertion. Fold them into a single `safeJoinModulesDir` that returns the joined path and throws on escape, so the check is unmissable. --- Written by an agent (Claude Code, claude-opus-4-7).
The earlier tests only exercised the `!startsWith` branch with `'../sibling'` and `'@x/../../../etc'`. Add `''` and `'.'` as alias cases — both resolve to the modules dir itself and hit the `resolvedLink === resolvedDir` branch of `safeJoinModulesDir`. --- Written by an agent (Claude Code, claude-opus-4-7).
809c5d2 to
96695d0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
installing/deps-resolver/src/validateDependencyAlias.ts (1)
11-31: ⚡ Quick winMove function declarations to match repo hoisting style.
Place
isValidDependencyAliasafterassertValidDependencyAliasesso declarations follow usage order per project TS style.♻️ Proposed reorder
-export function isValidDependencyAlias (alias: string): boolean { - return typeof alias === 'string' && validateNpmPackageName(alias).validForOldPackages -} - export function assertValidDependencyAliases ( deps: Record<string, unknown> | undefined, parentPkgDescription: string ): void { @@ } } + +export function isValidDependencyAlias (alias: string): boolean { + return typeof alias === 'string' && validateNpmPackageName(alias).validForOldPackages +}As per coding guidelines, “Follow Standard Style … declaring functions after they are used (relying on hoisting)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@installing/deps-resolver/src/validateDependencyAlias.ts` around lines 11 - 31, Reorder the two exported functions so that assertValidDependencyAliases appears before isValidDependencyAlias: move the isValidDependencyAlias declaration below assertValidDependencyAliases without changing their signatures or behavior; ensure the export names remain the same and that assertValidDependencyAliases still calls isValidDependencyAlias (relying on hoisting) so the repo’s style of declaring functions after use is followed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@installing/deps-resolver/src/validateDependencyAlias.ts`:
- Around line 11-31: Reorder the two exported functions so that
assertValidDependencyAliases appears before isValidDependencyAlias: move the
isValidDependencyAlias declaration below assertValidDependencyAliases without
changing their signatures or behavior; ensure the export names remain the same
and that assertValidDependencyAliases still calls isValidDependencyAlias
(relying on hoisting) so the repo’s style of declaring functions after use is
followed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d92b147-5e8d-4c3d-ac99-1b6e3a997177
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/reject-traversal-dependency-aliases.mdfs/symlink-dependency/src/index.tsfs/symlink-dependency/src/safeJoinModulesDir.tsfs/symlink-dependency/src/symlinkDirectRootDependency.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/package.jsoninstalling/deps-resolver/src/getNonDevWantedDependencies.tsinstalling/deps-resolver/src/getWantedDependencies.tsinstalling/deps-resolver/src/validateDependencyAlias.tsinstalling/deps-resolver/test/validateDependencyAlias.test.tspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/reject-traversal-dependency-aliases.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Audit dependencies
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Cargo Deny
- GitHub Check: Dylint
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
fs/symlink-dependency/src/safeJoinModulesDir.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/src/validateDependencyAlias.tsinstalling/deps-resolver/src/getWantedDependencies.tsinstalling/deps-resolver/src/getNonDevWantedDependencies.tsinstalling/deps-resolver/test/validateDependencyAlias.test.tsfs/symlink-dependency/src/index.tsfs/symlink-dependency/src/symlinkDirectRootDependency.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use util.types.isNativeError() instead of instanceof Error for error type checking in Jest tests
Files:
fs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/test/validateDependencyAlias.test.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
🧠 Learnings (7)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
fs/symlink-dependency/src/safeJoinModulesDir.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-resolver/src/validateDependencyAlias.tsinstalling/deps-resolver/src/getWantedDependencies.tsinstalling/deps-resolver/src/getNonDevWantedDependencies.tsinstalling/deps-resolver/test/validateDependencyAlias.test.tsfs/symlink-dependency/src/index.tsfs/symlink-dependency/src/symlinkDirectRootDependency.ts
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
installing/deps-resolver/package.json
🔇 Additional comments (14)
installing/deps-resolver/package.json (1)
73-73: LGTM!Also applies to: 85-86
installing/deps-resolver/src/getNonDevWantedDependencies.ts (1)
4-5: LGTM!Also applies to: 15-18, 21-25
installing/deps-resolver/src/getWantedDependencies.ts (1)
9-10: LGTM!Also applies to: 28-31
installing/deps-resolver/test/validateDependencyAlias.test.ts (1)
1-67: LGTM!fs/symlink-dependency/src/safeJoinModulesDir.ts (1)
1-19: LGTM!fs/symlink-dependency/src/index.ts (1)
4-5: LGTM!Also applies to: 13-13, 23-23
fs/symlink-dependency/src/symlinkDirectRootDependency.ts (1)
11-12: LGTM!Also applies to: 49-49
fs/symlink-dependency/test/safeJoinModulesDir.test.ts (1)
1-37: LGTM!pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
14-20: LGTM!pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
59-59: LGTM!Also applies to: 82-82
pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs (1)
1-60: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
106-118: LGTM!Also applies to: 160-170, 601-606, 791-830
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
158-170: LGTM!pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
320-364: LGTM!
Benchmark Results
Run 26454586679 · 10 runs per scenario · triggered by @zkochan |
A transitive registry package can use a dependency-alias key like `@x/../../../../../.git/hooks` to make `pnpm install` create a symlink outside the intended `node_modules` directory, since pnpm passes the alias straight into `path.join(modulesDir, alias)` without checking that the joined path stays inside `modulesDir`. Reject aliases that aren't valid npm package names at manifest-read time (both the importer's manifest and every transitive package manifest) and re-check at the symlink layer as defense in depth. Backport of #11954 to the release/10 line. --- Written by an agent (Claude Code, claude-opus-4-7).
Summary
A transitive registry package can use a dependency-alias key like
@x/../../../../../.git/hooksto makepnpm installcreate a symlink outside the intendednode_modulesdirectory. pnpm joins the alias straight intopath.join(modulesDir, alias)without checking that the joined path stays insidemodulesDir, so the resulting symlink can land at an attacker-chosen path under any predictable prefix (the global store, the user's home, the project directory if the depth matches up).--ignore-scriptsdoes not block this: no lifecycle script runs; only the symlink is created during install, and the payload executes the next time the victim runs anything that loads from the replaced path (git commitinvoking a hook, a CI step using a local action,pnpm publishpackaging a replaceddist/, etc.).Reproduced on
main(bundled pnpm 11.3.0): abad@1.0.0package whose manifest contains"dependencies": { "@x/../../../../../.git/hooks": "npm:payload-hooks@1.0.0" }, pulled in transitively by anormal@1.0.0install, caused pnpm to create~/Library/pnpm/store/v11/links/@/.git/hooksas a symlink to the payload package.Fix
Reject aliases that aren't a single
nameor@scope/nameshape:installing/deps-resolver/src/validateDependencyAlias.ts— newisValidDependencyAliaspredicate +assertValidDependencyAliasesasserter. Rejects empty strings,../.segments, embedded slashes beyond a single scope separator, backslashes, and null bytes.getNonDevWantedDependencies.ts— validates every transitivedependencies/optionalDependenciesentry. Error names the offending parent package.getWantedDependencies.ts— same check for the importer's own manifest.fs/symlink-dependency/src/assertAliasStaysInDir.ts— defense-in-depth check at the symlink layer. Re-validates afterpath.resolve(destModulesDir, alias)that the target still lives underdestModulesDir. Wired intosymlinkDependency,symlinkDependencySync, andsymlinkDirectRootDependency.Mirrored in pacquet:
pacquet/crates/resolving-deps-resolver/src/validate_dependency_alias.rs— same predicate.ResolveDependencyTreeError::InvalidDependencyNamevariant with miette codeINVALID_DEPENDENCY_NAME, surfaced fromcollect_deps(transitive aliases) and from both initial-wanted constructions (resolve_dependency_treeandresolve_importer).The new error code is
ERR_PNPM_INVALID_DEPENDENCY_NAMEon both stacks.Test plan
..,./foo,foo/bar,@scope/name/extra, backslashes, null bytes, absolute paths).InvalidDependencyNamenaming the offending parent.@pnpm/fs.symlink-dependencythatsymlinkDependency,symlinkDependencySync, andsymlinkDirectRootDependencyall throwERR_PNPM_INVALID_DEPENDENCY_NAMEwhen handed an escape alias.pnpm --filter @pnpm/installing.deps-resolver test— 84/84 passing.pnpm --filter @pnpm/fs.symlink-dependency test— 4/4 passing.cargo nextest run -p pacquet-resolving-deps-resolver— 60/60 passing.cargo clippy --workspace --all-targets -- --deny warnings— clean.cargo fmt -- --check— clean.[ERR_PNPM_INVALID_DEPENDENCY_NAME] Package "bad@1.0.0" contains a dependency with an invalid name: "@x/../../../../../.git/hooks", and no escape symlink is created.pnpm run lintat repo root (not yet run; let me know if you'd like that before flipping to ready).Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests