fix(git-fetcher): reject non-SHA commit values before invoking git by zkochan · Pull Request #11967 · pnpm/pnpm

zkochan · 2026-05-26T18:15:59Z

Summary

fetching/git-fetcher/src/index.ts passed the lockfile-controlled resolution.commit value straight to git fetch --depth 1 origin <commit> and git checkout <commit> with no -- separator and no format validation. A malicious pnpm-lock.yaml could put a value such as --upload-pack=touch /tmp/pwned in resolution.commit; git parses anything starting with - as an option, and on SSH or local-file transports --upload-pack runs the supplied command as the user running pnpm install. HTTPS ignores --upload-pack, but the SSH/file paths are enough to reach code execution.

The fix validates resolution.commit against /^[0-9a-f]{40}$/i at the entry of the fetcher and throws INVALID_GIT_COMMIT otherwise. This is strictly stronger than adding a -- separator — a validated value cannot start with - or contain shell-significant characters at all.

Pacquet's pacquet-git-fetcher crate shells out to git along the same code path (pacquet/crates/git-fetcher/src/fetcher.rs) and had the identical issue. Ported the same check there, with a new GitFetcherError::InvalidCommit variant carrying the INVALID_GIT_COMMIT diagnostic code.

Reported by AutoFyn.

Test plan

pnpm --filter @pnpm/fetching.git-fetcher test -t reject — new reject a partial commit before invoking git and reject a commit value that looks like a git option tests pass; both assert execa is never invoked.
cargo nextest run -p pacquet-git-fetcher is_valid_commit_hash — helper unit tests pass.
cargo nextest run -p pacquet-git-fetcher fetcher_rejects_option_shaped_commit — integration test passes.
cargo clippy --locked -p pacquet-git-fetcher --all-targets -- --deny warnings — clean.

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

Bug Fixes
- Rejects non-40-character hex git commit references, preventing malformed inputs from being used and returning a clear error when a commit is invalid.
Tests
- Added coverage to verify invalid and option-shaped commit values are rejected before any git operations.

coderabbitai · 2026-05-26T18:16:17Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c5868b6-7de1-4ea9-81be-45450626e972

📥 Commits

Reviewing files that changed from the base of the PR and between e122c4c and ac396a2.

📒 Files selected for processing (6)

.changeset/git-fetcher-reject-non-sha-commits.md
fetching/git-fetcher/src/index.ts
fetching/git-fetcher/test/index.ts
pacquet/crates/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/tests.rs

✅ Files skipped from review due to trivial changes (1)

.changeset/git-fetcher-reject-non-sha-commits.md

📜 Recent 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). (5)

GitHub Check: Lint and Test (macos-latest)
GitHub Check: Lint and Test (windows-latest)
GitHub Check: Lint and Test (ubuntu-latest)
GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Compile & Lint

🧰 Additional context used

📓 Path-based instructions (2)

pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

pacquet/crates/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/tests.rs

**/*.{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:

fetching/git-fetcher/src/index.ts
fetching/git-fetcher/test/index.ts

🧠 Learnings (4)

📚 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/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/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/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/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/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/tests.rs

📚 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:

fetching/git-fetcher/src/index.ts
fetching/git-fetcher/test/index.ts

🔇 Additional comments (7)

pacquet/crates/git-fetcher/src/error.rs (1)

92-101: LGTM!

pacquet/crates/git-fetcher/src/fetcher.rs (1)

109-114: LGTM!

Also applies to: 235-240

pacquet/crates/git-fetcher/src/fetcher/tests.rs (1)

1-1: LGTM!

Also applies to: 87-101, 103-134

fetching/git-fetcher/src/index.ts (2)

29-31: LGTM!

84-86: LGTM!

fetching/git-fetcher/test/index.ts (2)

230-246: LGTM!

248-261: LGTM!

📝 Walkthrough

Walkthrough

This PR rejects git resolution commit values that are not 40-character hexadecimal SHAs before invoking git, implementing the check in Node.js and Rust git-fetchers, adding error diagnostics, tests, and a changeset documenting the patch release.

Changes

Git Commit Hash Validation Security Fix

Layer / File(s)	Summary
Node.js validation guard, helper, and tests `fetching/git-fetcher/src/index.ts`, `fetching/git-fetcher/test/index.ts`	Add `isValidCommitHash` and an early guard in `createGitFetcher` that throws `INVALID_GIT_COMMIT` for non-40-hex commits; tests assert rejection and that `execa`/git is not invoked for partial or option-shaped commits.
Rust error type for invalid commit `pacquet/crates/git-fetcher/src/error.rs`	Add `GitFetcherError::InvalidCommit { commit, repo }` with diagnostic code `INVALID_GIT_COMMIT` and display formatting.
Rust validation guard, helper, and tests `pacquet/crates/git-fetcher/src/fetcher.rs`, `pacquet/crates/git-fetcher/src/fetcher/tests.rs`	Add `is_valid_commit_hash` and early validation in `GitFetcher::run_sync` that returns `InvalidCommit` for non-40-hex values; unit and async tests cover valid/invalid inputs and option-shaped commits.
Release documentation `.changeset/git-fetcher-reject-non-sha-commits.md`	Add changeset entry for patch-level bumps to document the validation/security fix.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble codes and count each hex,
Forty characters—no tricks complex.
Option-shaped strings bounce off the gate,
Both Node and Rust now validate.
Secure fetch hops on, safe and vex-free.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main security fix: validating git commit values before passing them to git commands.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch vuln6

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

qodo-free-for-open-source-projects · 2026-05-26T18:16:26Z

Review Summary by Qodo

Reject non-SHA commit values before invoking git

🐞 Bug fix

Walkthroughs

Description

• Validate git commit hashes before passing to git commands
  - Rejects non-40-character hexadecimal SHA values
  - Prevents command injection via malicious lockfile entries
• Implements validation in both pnpm and pacquet git fetchers
• Adds comprehensive tests for partial commits and option-shaped values

Diagram

flowchart LR
  A["Malicious lockfile<br/>with commit option"] -->|"resolution.commit"| B["Git Fetcher Entry"]
  B -->|"Validate SHA"| C{"Is 40-char<br/>hex SHA?"}
  C -->|"No"| D["Throw INVALID_GIT_COMMIT"]
  C -->|"Yes"| E["Proceed to git fetch"]
  D -->|"Prevents"| F["Command Injection"]

File Changes

1. fetching/git-fetcher/src/index.ts 🐞 Bug fix +7/-0

Add commit hash validation to pnpm git fetcher

• Added isValidCommitHash() function validating 40-character hexadecimal SHA format
• Validates resolution.commit at fetcher entry point before git invocation
• Throws INVALID_GIT_COMMIT error for invalid commit values

fetching/git-fetcher/src/index.ts

2. fetching/git-fetcher/test/index.ts 🧪 Tests +19/-2

Add tests for commit validation before git invocation

• Renamed test to clarify validation occurs before git invocation
• Added test for partial commit rejection with execa verification
• Added test for option-shaped commit rejection (e.g., --upload-pack)
• Verifies git is never called for invalid commits

fetching/git-fetcher/test/index.ts

3. pacquet/crates/git-fetcher/src/error.rs Error handling +11/-0

Add InvalidCommit error variant for pacquet
• Added new InvalidCommit error variant to GitFetcherError enum
• Carries INVALID_GIT_COMMIT diagnostic code
• Includes commit and repo fields for error context
pacquet/crates/git-fetcher/src/error.rs

View more (3)

4. pacquet/crates/git-fetcher/src/fetcher.rs 🐞 Bug fix +13/-0

Add commit hash validation to pacquet git fetcher

• Added is_valid_commit_hash() helper function with byte-level validation
• Validates commit at run_sync() entry point before git operations
• Returns InvalidCommit error for non-SHA values
• Mirrors pnpm implementation with Rust idioms

pacquet/crates/git-fetcher/src/fetcher.rs

5. pacquet/crates/git-fetcher/src/fetcher/tests.rs 🧪 Tests +50/-1

Add comprehensive tests for commit validation

• Added unit tests for is_valid_commit_hash() accepting full SHAs
• Added unit tests rejecting short SHAs, empty strings, and option-shaped values
• Added integration test fetcher_rejects_option_shaped_commit() verifying error handling
• Tests validate both uppercase and lowercase hexadecimal acceptance

pacquet/crates/git-fetcher/src/fetcher/tests.rs

6. .changeset/git-fetcher-reject-non-sha-commits.md 📝 Documentation +6/-0

Add changeset for git fetcher security fix
• Changelog entry documenting security fix for both pnpm and pacquet
• Describes rejection of non-SHA commits before git invocation
• Explains prevention of command injection via malicious lockfiles
.changeset/git-fetcher-reject-non-sha-commits.md

github-actions · 2026-05-26T18:20:03Z

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.03      8.5±0.47ms   512.0 KB/sec    1.00      8.2±0.06ms   527.1 KB/sec

coderabbitai

Actionable comments posted: 1

🤖 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.

Inline comments:
In @.changeset/git-fetcher-reject-non-sha-commits.md:
- Line 2: Update the changeset so the package entry uses the correct package
name: replace the string "`@pnpm/fetching.git-fetcher`" with "`@pnpm/fetching`" in
the changeset header/body so the patch-level bump targets the intended package
(`@pnpm/fetching`) instead of the incorrect `@pnpm/fetching.git-fetcher`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b956b20c-8cdd-48b3-ad42-67d5dd8bdea1

📥 Commits

Reviewing files that changed from the base of the PR and between f107b1e and fbee8e9.

📒 Files selected for processing (6)

.changeset/git-fetcher-reject-non-sha-commits.md
fetching/git-fetcher/src/index.ts
fetching/git-fetcher/test/index.ts
pacquet/crates/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/tests.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). (7)

GitHub Check: Lint and Test (macos-latest)
GitHub Check: Lint and Test (windows-latest)
GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Lint and Test (ubuntu-latest)
GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Code Coverage
GitHub Check: Compile & Lint

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{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:

fetching/git-fetcher/src/index.ts
fetching/git-fetcher/test/index.ts

pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

pacquet/crates/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/tests.rs

🧠 Learnings (4)

📚 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:

fetching/git-fetcher/src/index.ts
fetching/git-fetcher/test/index.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/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/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/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/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/git-fetcher/src/error.rs
pacquet/crates/git-fetcher/src/fetcher.rs
pacquet/crates/git-fetcher/src/fetcher/tests.rs

🔇 Additional comments (5)

fetching/git-fetcher/src/index.ts (1)

29-31: LGTM!

Also applies to: 84-86

fetching/git-fetcher/test/index.ts (1)

230-246: LGTM!

Also applies to: 248-262

pacquet/crates/git-fetcher/src/error.rs (1)

92-101: LGTM!

pacquet/crates/git-fetcher/src/fetcher.rs (1)

109-114: LGTM!

Also applies to: 235-240

pacquet/crates/git-fetcher/src/fetcher/tests.rs (1)

1-1: LGTM!

Also applies to: 87-134

codecov-commenter · 2026-05-26T18:25:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.94%. Comparing base (f578281) to head (ac396a2).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11967   +/-   ##
=======================================
  Coverage   87.93%   87.94%           
=======================================
  Files         228      228           
  Lines       27810    27819    +9     
=======================================
+ Hits        24456    24466   +10     
+ Misses       3354     3353    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2026-05-26T18:43:12Z

Integrated-Benchmark Report (Linux)

Scenario: Isolated linker: fresh restore, cold cache + cold store

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	1.993 ± 0.151	1.891	2.401	1.02 ± 0.08
`pacquet@main`	1.962 ± 0.064	1.902	2.099	1.00

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.9934191823600003,
      "stddev": 0.15113710489693202,
      "median": 1.94583643336,
      "user": 2.72395318,
      "system": 3.3874479600000003,
      "min": 1.8912364698600002,
      "max": 2.40090932686,
      "times": [
        1.96199417286,
        1.93912603686,
        1.91565222086,
        1.95254682986,
        1.9157912138600002,
        1.8912364698600002,
        2.05183057586,
        2.40090932686,
        1.90427928886,
        2.00082568786
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.9618813851599999,
      "stddev": 0.06358985131235681,
      "median": 1.9294572198600002,
      "user": 2.7131698799999997,
      "system": 3.3822848599999995,
      "min": 1.90153662386,
      "max": 2.09855010086,
      "times": [
        1.90153662386,
        1.92518842886,
        2.09855010086,
        1.9183675528600002,
        1.93372601086,
        1.96193771386,
        1.92013155586,
        1.92224132686,
        2.0308081518599996,
        2.00632638586
      ]
    }
  ]
}

Scenario: Isolated linker: fresh restore, hot cache + hot store

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`pacquet@HEAD`	643.3 ± 34.4	625.5	739.9	1.03 ± 0.06
`pacquet@main`	626.4 ± 13.9	608.6	653.7	1.00

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6433479453400001,
      "stddev": 0.03441335633007782,
      "median": 0.6333702854400001,
      "user": 0.37326702,
      "system": 1.3371399800000001,
      "min": 0.6254698254400001,
      "max": 0.73993164044,
      "times": [
        0.73993164044,
        0.6424411374400001,
        0.62644076144,
        0.6373731634400001,
        0.6282762254400001,
        0.6280908564400001,
        0.6254698254400001,
        0.63871527244,
        0.63531923044,
        0.6314213404400001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6263564079400001,
      "stddev": 0.013898527365190021,
      "median": 0.6225422524400002,
      "user": 0.34785451999999994,
      "system": 1.3413618799999998,
      "min": 0.6085907194400001,
      "max": 0.6537090874400001,
      "times": [
        0.6537090874400001,
        0.6085907194400001,
        0.61829712944,
        0.61644874644,
        0.6282320264400001,
        0.6236219634400001,
        0.6214625414400001,
        0.62530075644,
        0.6473710144400001,
        0.6205300944400001
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + cold store

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	2.242 ± 0.062	2.182	2.413	1.02 ± 0.03
`pacquet@main`	2.192 ± 0.035	2.151	2.283	1.00

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.2415608180199995,
      "stddev": 0.06217088401457929,
      "median": 2.22576064202,
      "user": 3.755966779999999,
      "system": 3.10370642,
      "min": 2.18232304052,
      "max": 2.41323145952,
      "times": [
        2.22497562952,
        2.41323145952,
        2.18232304052,
        2.23008041252,
        2.22346582452,
        2.23709600052,
        2.22196419752,
        2.22189959352,
        2.23402636752,
        2.22654565452
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.19200466132,
      "stddev": 0.03519879166838471,
      "median": 2.18525905852,
      "user": 3.748655779999999,
      "system": 3.04067892,
      "min": 2.15096087952,
      "max": 2.28290347752,
      "times": [
        2.18106685352,
        2.28290347752,
        2.1723258525199998,
        2.19589295752,
        2.15096087952,
        2.17239639552,
        2.17783649152,
        2.19972323852,
        2.18945126352,
        2.19748920352
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, hot cache + hot store

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	1.391 ± 0.017	1.367	1.418	1.01 ± 0.03
`pacquet@main`	1.382 ± 0.032	1.346	1.455	1.00

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.39076634758,
      "stddev": 0.016823110034591863,
      "median": 1.3866068348799998,
      "user": 1.7011812,
      "system": 1.80626918,
      "min": 1.36685342088,
      "max": 1.41771772588,
      "times": [
        1.3898324148799999,
        1.36685342088,
        1.40117916388,
        1.41289037288,
        1.41771772588,
        1.38006122288,
        1.40071042488,
        1.3735719258799999,
        1.38338125488,
        1.38146554888
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.38246060998,
      "stddev": 0.031907527617307946,
      "median": 1.3759349693799998,
      "user": 1.6550117999999998,
      "system": 1.8133045799999998,
      "min": 1.3455251928799998,
      "max": 1.4554284228799999,
      "times": [
        1.34932366688,
        1.36444211088,
        1.39880496588,
        1.3455251928799998,
        1.4554284228799999,
        1.37365748588,
        1.40696648388,
        1.37417065588,
        1.3776992828799999,
        1.37858783188
      ]
    }
  ]
}

github-actions · 2026-05-26T18:43:14Z

Bencher Report

Branch	pr/11967
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,241.56 ms (-19.34%) Baseline: 2,779.05 ms	3,334.86 ms (67.22%)
isolated-linker.fresh-install.hot-cache.hot-store	📈 view plot 🚷 view threshold	1,390.77 ms (-26.67%) Baseline: 1,896.54 ms	2,275.85 ms (61.11%)
isolated-linker.fresh-restore.cold-cache.cold-store	📈 view plot 🚷 view threshold	1,993.42 ms (-3.47%) Baseline: 2,065.15 ms	2,478.18 ms (80.44%)
isolated-linker.fresh-restore.hot-cache.hot-store	📈 view plot 🚷 view threshold	643.35 ms (-2.27%) Baseline: 658.30 ms	789.96 ms (81.44%)

🐰 View full continuous benchmarking report in Bencher

The git fetcher passed the lockfile-controlled `resolution.commit` value to `git fetch --depth 1 origin <commit>` and `git checkout <commit>` without a `--` separator and without validating that the value is a SHA. A malicious lockfile could smuggle a value such as `--upload-pack=touch /tmp/pwned`, which `git` parses as an option. On SSH and local-file transports `--upload-pack` runs the supplied command. HTTPS ignores it, but the SSH/file paths are enough to execute code as the user running `pnpm install`. Validate `resolution.commit` against `/^[0-9a-f]{40}$/i` at the entry of the fetcher and throw `INVALID_GIT_COMMIT` otherwise. The check is strictly stronger than adding a `--` separator: a validated value cannot start with `-` or contain shell-significant characters at all. Ported the same fix to pacquet's `pacquet-git-fetcher` crate, which shells out to `git` along the same code path. Added a new `GitFetcherError::InvalidCommit` variant carrying `INVALID_GIT_COMMIT`, and unit + integration tests for both the helper and the fetcher entry-point. --- Written by an agent (Claude Code, claude-opus-4-7).

The validation throws before any host check, so wiring up `gitShallowHosts` and a matching `file://` URL host added nothing to the test. cspell also flagged the placeholder host name; using a real-looking `file:///tmp/repo.git` URL removes both issues at once. --- Written by an agent (Claude Code, claude-opus-4-7).

The git fetcher passed the lockfile-controlled `resolution.commit` value to `git fetch --depth 1 origin <commit>` and `git checkout <commit>` without a `--` separator and without validating that the value is a SHA. A malicious lockfile could smuggle a value such as `--upload-pack=touch /tmp/pwned`, which `git` parses as an option. On SSH and local-file transports `--upload-pack` runs the supplied command. HTTPS ignores it, but the SSH/file paths are enough to execute code as the user running `pnpm install`. Validate `resolution.commit` against `/^[0-9a-f]{40}$/i` at the entry of the fetcher and throw `INVALID_GIT_COMMIT` otherwise. The check is strictly stronger than adding a `--` separator: a validated value cannot start with `-` or contain shell-significant characters at all. Backport of #11967 to release/10. --- Written by an agent (Claude Code, claude-opus-4-7).

coderabbitai Bot requested changes May 26, 2026

View reviewed changes

Comment thread .changeset/git-fetcher-reject-non-sha-commits.md

coderabbitai Bot approved these changes May 26, 2026

View reviewed changes

zkochan mentioned this pull request May 26, 2026

ci(pacquet): pin cargo-dylint to 6.0.0 to unblock CI #11969

Merged

2 tasks

zkochan added 2 commits May 26, 2026 20:59

zkochan force-pushed the vuln6 branch from e122c4c to ac396a2 Compare May 26, 2026 18:59

zkochan merged commit 90d1ce6 into main May 26, 2026
28 checks passed

zkochan deleted the vuln6 branch May 26, 2026 20:56

This was referenced May 27, 2026

chore(release): 10.34.0 #11988

Merged

chore(release): 11.4.0 #11989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(git-fetcher): reject non-SHA commit values before invoking git#11967

fix(git-fetcher): reject non-SHA commit values before invoking git#11967
zkochan merged 2 commits into
mainfrom
vuln6

zkochan commented May 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 26, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented May 26, 2026

Uh oh!

github-actions Bot commented May 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov-commenter commented May 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

zkochan commented May 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented May 26, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

github-actions Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Micro-Benchmark Results

Linux

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Integrated-Benchmark Report (Linux)

Scenario: Isolated linker: fresh restore, cold cache + cold store

Scenario: Isolated linker: fresh restore, hot cache + hot store

Scenario: Isolated linker: fresh install, cold cache + cold store

Scenario: Isolated linker: fresh install, hot cache + hot store

Uh oh!

github-actions Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 26, 2026 •

edited

Loading

github-actions Bot commented May 26, 2026 •

edited

Loading

codecov-commenter commented May 26, 2026 •

edited

Loading

github-actions Bot commented May 26, 2026 •

edited

Loading

github-actions Bot commented May 26, 2026 •

edited

Loading