From 90d1ce6b60c00827c460409acd694e7763c8b4a4 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 26 May 2026 22:56:49 +0200 Subject: [PATCH] fix(git-fetcher): reject non-SHA commit values before invoking git (#11967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fetching/git-fetcher/src/index.ts` passed the lockfile-controlled `resolution.commit` value straight to `git fetch --depth 1 origin ` and `git checkout ` 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](https://github.com/SignalPilot-Labs/AutoFyn). --- .../git-fetcher-reject-non-sha-commits.md | 6 +++ fetching/git-fetcher/src/index.ts | 7 +++ fetching/git-fetcher/test/index.ts | 21 +++++++- pacquet/crates/git-fetcher/src/error.rs | 11 ++++ pacquet/crates/git-fetcher/src/fetcher.rs | 13 +++++ .../crates/git-fetcher/src/fetcher/tests.rs | 51 ++++++++++++++++++- 6 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 .changeset/git-fetcher-reject-non-sha-commits.md diff --git a/.changeset/git-fetcher-reject-non-sha-commits.md b/.changeset/git-fetcher-reject-non-sha-commits.md new file mode 100644 index 0000000000..258ded7ed8 --- /dev/null +++ b/.changeset/git-fetcher-reject-non-sha-commits.md @@ -0,0 +1,6 @@ +--- +"@pnpm/fetching.git-fetcher": patch +"pnpm": patch +--- + +Reject git resolutions whose `commit` field is not a 40-character hexadecimal SHA before invoking `git`. A malicious lockfile could otherwise smuggle a value such as `--upload-pack=` through `git fetch` / `git checkout`, which on SSH or local-file transports executes the supplied command. diff --git a/fetching/git-fetcher/src/index.ts b/fetching/git-fetcher/src/index.ts index 2a402e07bf..1d35259886 100644 --- a/fetching/git-fetcher/src/index.ts +++ b/fetching/git-fetcher/src/index.ts @@ -26,6 +26,9 @@ export function createGitFetcher (createOpts: CreateGitFetcherOptions): { git: G const ignoreScripts = createOpts.ignoreScripts ?? false const gitFetcher: GitFetcher = async (cafs, resolution, opts) => { + if (!isValidCommitHash(resolution.commit)) { + throw new PnpmError('INVALID_GIT_COMMIT', `Invalid git commit hash "${resolution.commit}" for repository "${resolution.repo}". Expected a 40-character hexadecimal SHA.`) + } const tempLocation = await cafs.tempDir() if (allowedHosts.size > 0 && shouldUseShallow(resolution.repo, allowedHosts)) { await execGit(['init'], { cwd: tempLocation }) @@ -78,6 +81,10 @@ export function createGitFetcher (createOpts: CreateGitFetcherOptions): { git: G } } +function isValidCommitHash (commit: string): boolean { + return /^[0-9a-f]{40}$/i.test(commit) +} + function shouldUseShallow (repoUrl: string, allowedHosts: Set): boolean { try { const { host } = new URL(repoUrl) diff --git a/fetching/git-fetcher/test/index.ts b/fetching/git-fetcher/test/index.ts index 1abd567538..fdc11afe28 100644 --- a/fetching/git-fetcher/test/index.ts +++ b/fetching/git-fetcher/test/index.ts @@ -227,7 +227,7 @@ test('fail when preparing a git-hosted package', async () => { ).rejects.toThrow('Failed to prepare git-hosted package fetched from "https://github.com/pnpm-e2e/prepare-script-fails.git": @pnpm.e2e/prepare-script-fails@1.0.0 npm-install: `npm install`') }) -test('fail when preparing a git-hosted package with a partial commit', async () => { +test('reject a partial commit before invoking git', async () => { const storeDir = temporaryDirectory() const fetch = createGitFetcher({ storeIndex: createStoreIndex(storeDir), @@ -241,7 +241,24 @@ test('fail when preparing a git-hosted package with a partial commit', async () }, { filesIndexFile: path.join(storeDir, 'index.json'), }) - ).rejects.toThrow(/received commit [0-9a-f]{40} does not match expected value deadbeef/) + ).rejects.toThrow('Invalid git commit hash "deadbeef"') + expect(jest.mocked(execa)).not.toHaveBeenCalled() +}) + +test('reject a commit value that looks like a git option', async () => { + const storeDir = temporaryDirectory() + const fetch = createGitFetcher({ storeIndex: createStoreIndex(storeDir) }).git + await expect( + fetch(createCafsStore(storeDir), + { + commit: '--upload-pack=touch /tmp/pwned', + repo: 'file:///tmp/repo.git', + type: 'git', + }, { + filesIndexFile: path.join(storeDir, 'index.json'), + }) + ).rejects.toThrow('Invalid git commit hash "--upload-pack=touch /tmp/pwned"') + expect(jest.mocked(execa)).not.toHaveBeenCalled() }) test('do not build the package when scripts are ignored', async () => { diff --git a/pacquet/crates/git-fetcher/src/error.rs b/pacquet/crates/git-fetcher/src/error.rs index 5a617a975d..2545d66160 100644 --- a/pacquet/crates/git-fetcher/src/error.rs +++ b/pacquet/crates/git-fetcher/src/error.rs @@ -89,6 +89,17 @@ pub enum GitFetcherError { #[diagnostic(code(GIT_CHECKOUT_FAILED))] CheckoutMismatch { expected: String, received: String }, + /// `resolution.commit` is not a 40-character hexadecimal SHA. A + /// commit value beginning with `-` would otherwise be parsed by + /// `git fetch` / `git checkout` as an option (e.g. `--upload-pack`), + /// allowing a malicious lockfile to execute arbitrary commands on + /// SSH or local-file transports. + #[display( + "Invalid git commit hash {commit:?} for repository {repo:?}. Expected a 40-character hexadecimal SHA." + )] + #[diagnostic(code(INVALID_GIT_COMMIT))] + InvalidCommit { commit: String, repo: String }, + #[display("I/O error during git fetch: {_0}")] #[diagnostic(code(pacquet_git_fetcher::io))] Io(#[error(source)] std::io::Error), diff --git a/pacquet/crates/git-fetcher/src/fetcher.rs b/pacquet/crates/git-fetcher/src/fetcher.rs index 1552df0c89..b34f3fddf6 100644 --- a/pacquet/crates/git-fetcher/src/fetcher.rs +++ b/pacquet/crates/git-fetcher/src/fetcher.rs @@ -106,6 +106,12 @@ impl<'a> GitFetcher<'a> { } fn run_sync(self) -> Result { + if !is_valid_commit_hash(self.commit) { + return Err(GitFetcherError::InvalidCommit { + commit: self.commit.to_string(), + repo: self.repo.to_string(), + }); + } let temp = tempfile::tempdir().map_err(GitFetcherError::Io)?; let temp_location = temp.path(); @@ -226,6 +232,13 @@ fn wrap_prepare_error(_repo: &str, err: PreparePackageError) -> GitFetcherError GitFetcherError::Prepare(err) } +/// True iff `commit` is exactly a 40-character hexadecimal git SHA. +/// Rejects everything else (short SHAs, ref names, option-shaped +/// strings like `--upload-pack=…`) before the value reaches `git`. +fn is_valid_commit_hash(commit: &str) -> bool { + commit.len() == 40 && commit.bytes().all(|b| b.is_ascii_hexdigit()) +} + /// True iff `repo` parses to a host that pacquet should clone via the /// shallow `init` + `fetch --depth 1` path. Mirrors upstream's /// [`shouldUseShallow`](https://github.com/pnpm/pnpm/blob/94240bc046/fetching/git-fetcher/src/index.ts#L81-L91). diff --git a/pacquet/crates/git-fetcher/src/fetcher/tests.rs b/pacquet/crates/git-fetcher/src/fetcher/tests.rs index 819337926b..e9f20b50a1 100644 --- a/pacquet/crates/git-fetcher/src/fetcher/tests.rs +++ b/pacquet/crates/git-fetcher/src/fetcher/tests.rs @@ -1,4 +1,4 @@ -use super::{GitFetcher, exec_git, extract_host, should_use_shallow}; +use super::{GitFetcher, exec_git, extract_host, is_valid_commit_hash, should_use_shallow}; use crate::error::GitFetcherError; use pacquet_executor::ScriptsPrependNodePath; use pacquet_reporter::SilentReporter; @@ -84,6 +84,55 @@ fn should_use_shallow_matches_known_host() { assert!(!should_use_shallow("https://example.com/x/y.git", &hosts)); } +#[test] +fn is_valid_commit_hash_accepts_full_sha() { + assert!(is_valid_commit_hash("c9b30e71d704cd30fa71f2edd1ecc7dcc4985493")); + assert!(is_valid_commit_hash("C9B30E71D704CD30FA71F2EDD1ECC7DCC4985493")); +} + +#[test] +fn is_valid_commit_hash_rejects_short_or_option_shaped_values() { + assert!(!is_valid_commit_hash("deadbeef")); + assert!(!is_valid_commit_hash("")); + assert!(!is_valid_commit_hash("--upload-pack=touch /tmp/pwned")); + assert!(!is_valid_commit_hash("c9b30e71d704cd30fa71f2edd1ecc7dcc4985493 ")); + // 40 chars but contains a non-hex digit. + assert!(!is_valid_commit_hash("c9b30e71d704cd30fa71f2edd1ecc7dcc498549z")); +} + +#[tokio::test(flavor = "multi_thread")] +async fn fetcher_rejects_option_shaped_commit() { + let store_root = tempdir().unwrap(); + let store_dir = StoreDir::from(store_root.path().to_path_buf()); + let err = GitFetcher { + repo: "file:///tmp/githost", + commit: "--upload-pack=touch /tmp/pwned", + path: None, + git_shallow_hosts: &[], + allow_build: deny_all_builds(), + ignore_scripts: false, + unsafe_perm: true, + user_agent: None, + scripts_prepend_node_path: ScriptsPrependNodePath::Never, + script_shell: None, + node_execpath: None, + npm_execpath: None, + store_dir: &store_dir, + package_id: "pkg@1.0.0", + requester: "/test", + store_index_writer: None, + files_index_file: "pkg@1.0.0\tbuilt", + git_bin: None, + } + .run::() + .await + .unwrap_err(); + assert!( + matches!(err, GitFetcherError::InvalidCommit { .. }), + "expected InvalidCommit, got {err:?}", + ); +} + #[test] fn extract_host_handles_user_authority_and_port() { assert_eq!(extract_host("https://github.com/foo/bar"), Some("github.com"));