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", () => {