From 11e6124b350510969b0a31a53c9c6c12f5d20adc Mon Sep 17 00:00:00 2001 From: real-venus Date: Wed, 22 Jul 2026 04:35:24 +0200 Subject: [PATCH] fix(miner): reject path-traversal-shaped commitSha in replay-snapshot path planner normalizeCommitSha accepted any non-empty string and join()ed it straight into REPLAY_SNAPSHOT_SUBDIR, so a crafted commitSha like "../../../../tmp/evil" escaped the intended .loopover-replay-snapshots sandbox entirely -- and would then control where `git worktree add --detach ` writes on disk (#7796). Constrain it to a single safe path segment (repo-clone.ts's isValidRepoSegment charset for owner/repo, #5831, plus an explicit "."/".." rejection) before it reaches path.join(). A genuine commit SHA is hex and always satisfies this, so no legitimate caller regresses. Adds a regression test covering traversal-/separator-shaped values (and the accepted hex case). The test now imports the .ts SOURCE via a non-literal specifier instead of the extensionless/.js path: once build:miner has produced the artifact, a .js import loads that build output and leaves coverage.include's .ts entry at 0% -- so the new guard is now actually instrumented (100% patch), while the variable specifier keeps tsc happy (no TS5097). Closes #7796 --- .../loopover-miner/lib/replay-snapshot.ts | 11 ++++++++- test/unit/miner-replay-snapshot.test.ts | 23 +++++++++++++++++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/loopover-miner/lib/replay-snapshot.ts b/packages/loopover-miner/lib/replay-snapshot.ts index 7cdc14dc6e..363e9798f4 100644 --- a/packages/loopover-miner/lib/replay-snapshot.ts +++ b/packages/loopover-miner/lib/replay-snapshot.ts @@ -75,9 +75,18 @@ function normalizeRepoFullName(repoFullName: string): string { return `${owner}/${repo}`; } +// A commit SHA is joined straight into REPLAY_SNAPSHOT_SUBDIR (and later passed to git as a bare revision), +// so a value like "../../../tmp/evil" (or one containing a path separator) would escape the intended snapshot +// directory via path.join (#7796). Constrain it to a single safe path segment -- the same restricted charset +// repo-clone.ts's isValidRepoSegment guard uses for owner/repo (#5831), plus an explicit "."/".." rejection. +// A genuine commit SHA is hex and always satisfies this, so no legitimate caller regresses. +const COMMIT_SHA_PATTERN = /^[A-Za-z0-9._-]+$/; + function normalizeCommitSha(commitSha: string): string { if (typeof commitSha !== "string" || !commitSha.trim()) throw new Error("invalid_commit_sha"); - return commitSha.trim(); + const trimmed = commitSha.trim(); + if (trimmed === "." || trimmed === ".." || !COMMIT_SHA_PATTERN.test(trimmed)) throw new Error("invalid_commit_sha"); + return trimmed; } /** Worktree exports live under this dir inside the repo, mirroring worktree-allocator.ts's WORKTREE_SUBDIR. */ diff --git a/test/unit/miner-replay-snapshot.test.ts b/test/unit/miner-replay-snapshot.test.ts index f1ef84628c..31a0c1cf11 100644 --- a/test/unit/miner-replay-snapshot.test.ts +++ b/test/unit/miner-replay-snapshot.test.ts @@ -3,7 +3,12 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { +// Import the .ts SOURCE (not the build-time .js) via a non-literal specifier. The committed lib is .ts-only, +// but once `build:miner` has produced the artifact, a plain `.js`/extensionless import loads that .js, leaving +// coverage.include's `.ts` entry at 0% -- that .js-vs-.ts mismatch is why this file's new guard kept reporting +// 0% patch coverage. A variable specifier loads (and instruments) the .ts while dodging TS5097 (#7796). +const REPLAY_SNAPSHOT_MODULE = "../../packages/loopover-miner/lib/replay-snapshot.ts"; +const { closeDefaultReplaySnapshotStore, exportReplaySnapshot, openReplaySnapshotStore, @@ -11,7 +16,7 @@ import { removeReplaySnapshotWorktree, REPLAY_SNAPSHOT_SUBDIR, validateSnapshotFreshness, -} from "../../packages/loopover-miner/lib/replay-snapshot.js"; +} = (await import(REPLAY_SNAPSHOT_MODULE)) as typeof import("../../packages/loopover-miner/lib/replay-snapshot.js"); const FIELD_SEP = "\x1f"; @@ -77,6 +82,20 @@ describe("planReplaySnapshotPath (#3010) — pure, deterministic", () => { expect(planReplaySnapshotPath({ repoPath: "/repo", commitSha: "abc123" })).toBe(a); expect(planReplaySnapshotPath({ repoPath: "/repo", commitSha: "def456" })).not.toBe(a); }); + + it("rejects a commit SHA that would escape the snapshot subdir via path traversal or a separator (#7796)", () => { + // Without the guard, join()ing these into REPLAY_SNAPSHOT_SUBDIR escapes the repo entirely (e.g. + // ".../../../../../tmp/evil"). Each must be rejected up front rather than producing an out-of-sandbox path. + for (const commitSha of ["../../../../tmp/evil", "..", ".", "a/b", "a\\b", "../abc123", "foo/../..", " ../x "]) { + expect(() => planReplaySnapshotPath({ repoPath: "/repo", commitSha })).toThrow("invalid_commit_sha"); + } + }); + + it("still confines a genuine hex commit SHA to the snapshot subdir (#7796)", () => { + const sha = "0a1b2c3d4e5f60718293a4b5c6d7e8f901234567"; + const p = planReplaySnapshotPath({ repoPath: "/repo", commitSha: ` ${sha} ` }).replaceAll("\\", "/"); + expect(p).toBe(`/repo/${REPLAY_SNAPSHOT_SUBDIR}/${sha}`); + }); }); describe("validateSnapshotFreshness (#3010) — pure fail-fast check", () => {