Skip to content

Commit 2656eaa

Browse files
authored
fix(miner): reject path-traversal-shaped commitSha in replay-snapshot path planner (#7996)
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 <path>` 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
1 parent 162b7cb commit 2656eaa

2 files changed

Lines changed: 31 additions & 3 deletions

File tree

packages/loopover-miner/lib/replay-snapshot.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,18 @@ function normalizeRepoFullName(repoFullName: string): string {
7575
return `${owner}/${repo}`;
7676
}
7777

78+
// A commit SHA is joined straight into REPLAY_SNAPSHOT_SUBDIR (and later passed to git as a bare revision),
79+
// so a value like "../../../tmp/evil" (or one containing a path separator) would escape the intended snapshot
80+
// directory via path.join (#7796). Constrain it to a single safe path segment -- the same restricted charset
81+
// repo-clone.ts's isValidRepoSegment guard uses for owner/repo (#5831), plus an explicit "."/".." rejection.
82+
// A genuine commit SHA is hex and always satisfies this, so no legitimate caller regresses.
83+
const COMMIT_SHA_PATTERN = /^[A-Za-z0-9._-]+$/;
84+
7885
function normalizeCommitSha(commitSha: string): string {
7986
if (typeof commitSha !== "string" || !commitSha.trim()) throw new Error("invalid_commit_sha");
80-
return commitSha.trim();
87+
const trimmed = commitSha.trim();
88+
if (trimmed === "." || trimmed === ".." || !COMMIT_SHA_PATTERN.test(trimmed)) throw new Error("invalid_commit_sha");
89+
return trimmed;
8190
}
8291

8392
/** Worktree exports live under this dir inside the repo, mirroring worktree-allocator.ts's WORKTREE_SUBDIR. */

test/unit/miner-replay-snapshot.test.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,20 @@ import { tmpdir } from "node:os";
33
import { join } from "node:path";
44
import { afterEach, describe, expect, it, vi } from "vitest";
55

6-
import {
6+
// Import the .ts SOURCE (not the build-time .js) via a non-literal specifier. The committed lib is .ts-only,
7+
// but once `build:miner` has produced the artifact, a plain `.js`/extensionless import loads that .js, leaving
8+
// coverage.include's `.ts` entry at 0% -- that .js-vs-.ts mismatch is why this file's new guard kept reporting
9+
// 0% patch coverage. A variable specifier loads (and instruments) the .ts while dodging TS5097 (#7796).
10+
const REPLAY_SNAPSHOT_MODULE = "../../packages/loopover-miner/lib/replay-snapshot.ts";
11+
const {
712
closeDefaultReplaySnapshotStore,
813
exportReplaySnapshot,
914
openReplaySnapshotStore,
1015
planReplaySnapshotPath,
1116
removeReplaySnapshotWorktree,
1217
REPLAY_SNAPSHOT_SUBDIR,
1318
validateSnapshotFreshness,
14-
} from "../../packages/loopover-miner/lib/replay-snapshot.js";
19+
} = (await import(REPLAY_SNAPSHOT_MODULE)) as typeof import("../../packages/loopover-miner/lib/replay-snapshot.js");
1520

1621
const FIELD_SEP = "\x1f";
1722

@@ -77,6 +82,20 @@ describe("planReplaySnapshotPath (#3010) — pure, deterministic", () => {
7782
expect(planReplaySnapshotPath({ repoPath: "/repo", commitSha: "abc123" })).toBe(a);
7883
expect(planReplaySnapshotPath({ repoPath: "/repo", commitSha: "def456" })).not.toBe(a);
7984
});
85+
86+
it("rejects a commit SHA that would escape the snapshot subdir via path traversal or a separator (#7796)", () => {
87+
// Without the guard, join()ing these into REPLAY_SNAPSHOT_SUBDIR escapes the repo entirely (e.g.
88+
// ".../../../../../tmp/evil"). Each must be rejected up front rather than producing an out-of-sandbox path.
89+
for (const commitSha of ["../../../../tmp/evil", "..", ".", "a/b", "a\\b", "../abc123", "foo/../..", " ../x "]) {
90+
expect(() => planReplaySnapshotPath({ repoPath: "/repo", commitSha })).toThrow("invalid_commit_sha");
91+
}
92+
});
93+
94+
it("still confines a genuine hex commit SHA to the snapshot subdir (#7796)", () => {
95+
const sha = "0a1b2c3d4e5f60718293a4b5c6d7e8f901234567";
96+
const p = planReplaySnapshotPath({ repoPath: "/repo", commitSha: ` ${sha} ` }).replaceAll("\\", "/");
97+
expect(p).toBe(`/repo/${REPLAY_SNAPSHOT_SUBDIR}/${sha}`);
98+
});
8099
});
81100

82101
describe("validateSnapshotFreshness (#3010) — pure fail-fast check", () => {

0 commit comments

Comments
 (0)