Skip to content

Commit 16ff1cc

Browse files
tech0328davion-knight
authored andcommitted
fix(miner): reject path-traversal repo segments in the 4 remaining normalizeRepoFullName parsers
repo-clone.ts's isValidRepoSegment rejects a ./../control-char owner or repo segment before it is persisted to SQLite or echoed through a CLI. #5831 and #7525 rolled it out to ten sibling parsers, but four were missed and still only checked 'exactly one slash, both halves non-empty': contribution-profile-cache.ts, prediction-ledger.ts, replay-snapshot.ts, and run-state.ts. Inputs like 'owner/..', '../repo', or segments with tab/newline characters passed these four while being correctly rejected everywhere else. Import and call isValidRepoSegment on both owner and repo in all four, matching the exact call shape of the ten already-fixed siblings. Adds a per-file regression test asserting a path-traversal/invalid-character segment throws invalid_repo_full_name, covering both operands, mirroring test/unit/miner-claim-ledger.test.ts. Closes #7795
1 parent 2656eaa commit 16ff1cc

8 files changed

Lines changed: 65 additions & 0 deletions

packages/loopover-miner/lib/contribution-profile-cache.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
openLocalStoreAdapter,
1414
resolveLocalStoreDbPath,
1515
} from "./local-store.js";
16+
import { isValidRepoSegment } from "./repo-clone.js";
1617
import { applySchemaMigrations } from "./schema-version.js";
1718
import {
1819
CONTRIBUTION_PROFILE_CACHE_PURGE_SPEC,
@@ -58,6 +59,7 @@ function normalizeRepoFullName(repoFullName: unknown): string {
5859
const [owner, repo, extra] = repoFullName.trim().split("/");
5960
if (!owner || !repo || extra !== undefined)
6061
throw new Error("invalid_repo_full_name");
62+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
6163
return `${owner}/${repo}`;
6264
}
6365

packages/loopover-miner/lib/prediction-ledger.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { DatabaseSync } from "node:sqlite";
22
import { normalizeLocalStoreDbPath, openLocalStoreAdapter, resolveLocalStoreDbPath } from "./local-store.js";
3+
import { isValidRepoSegment } from "./repo-clone.js";
34
import { applySchemaMigrations } from "./schema-version.js";
45
import {
56
PREDICTION_LEDGER_PURGE_SPEC,
@@ -87,6 +88,7 @@ function normalizeRepoFullName(repoFullName: string): string {
8788
if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
8889
const [owner, repo, extra] = repoFullName.trim().split("/");
8990
if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
91+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
9092
return `${owner}/${repo}`;
9193
}
9294

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { join } from "node:path";
22
import { removeWorktree } from "@loopover/engine";
33
import type { WorktreeExecFn, WorktreeRemoveResult } from "@loopover/engine";
44
import { openLocalStoreAdapter, resolveLocalStoreDbPath, normalizeLocalStoreDbPath } from "./local-store.js";
5+
import { isValidRepoSegment } from "./repo-clone.js";
56

67
// Freeze/snapshot mechanism for historical replay targets (#3010). Given a repo and a commit SHA T, exports:
78
// (a) the full working tree checked out AT T via a DETACHED git worktree -- the same isolation primitive
@@ -72,6 +73,7 @@ function normalizeRepoFullName(repoFullName: string): string {
7273
if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
7374
const [owner, repo, extra] = repoFullName.trim().split("/");
7475
if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
76+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
7577
return `${owner}/${repo}`;
7678
}
7779

packages/loopover-miner/lib/run-state.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { DatabaseSync } from "node:sqlite";
22
import { DEFAULT_FORGE_CONFIG } from "./forge-config.js";
33
import { normalizeLocalStoreDbPath, openLocalStoreAdapter, resolveLocalStoreDbPath } from "./local-store.js";
4+
import { isValidRepoSegment } from "./repo-clone.js";
45
import { applySchemaMigrations } from "./schema-version.js";
56
import { RUN_STATE_PURGE_SPEC, purgeStoreByRepo } from "./store-maintenance.js";
67

@@ -57,6 +58,7 @@ function normalizeRepoFullName(repoFullName: string): string {
5758
const trimmed = repoFullName.trim();
5859
const [owner, repo, extra] = trimmed.split("/");
5960
if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
61+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
6062
return `${owner}/${repo}`;
6163
}
6264

test/unit/miner-contribution-profile-cache.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,20 @@ describe("contribution-profile cache store (#6797)", () => {
160160
);
161161
});
162162

163+
// #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching
164+
// repo-clone.js's own validation, instead of being silently accepted and persisted as a cache key --
165+
// for both the owner and repo segment independently.
166+
it("rejects a repoFullName with a path-traversal or invalid-character segment", () => {
167+
const store = tempStore();
168+
// Reads (get) and writes (put) both funnel through normalizeRepoFullName.
169+
expect(() => store.get("../etc")).toThrow("invalid_repo_full_name"); // owner ".." invalid
170+
expect(() => store.get("o/..")).toThrow("invalid_repo_full_name"); // repo ".." invalid
171+
expect(() => store.get("o baz/a")).toThrow("invalid_repo_full_name");
172+
expect(() => store.get("o/a baz")).toThrow("invalid_repo_full_name");
173+
expect(() => store.put(profile("../etc"), AT_MS)).toThrow("invalid_repo_full_name");
174+
expect(() => store.put(profile("o/.."), AT_MS)).toThrow("invalid_repo_full_name");
175+
});
176+
163177
it("exposes module-level get/put helpers backed by the default DB path", () => {
164178
vi.stubEnv(
165179
"LOOPOVER_MINER_CONTRIBUTION_PROFILE_CACHE_DB",

test/unit/miner-prediction-ledger.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,20 @@ describe("miner prediction ledger (#4263)", () => {
7575
expect(() => ledger.appendPrediction({ ...VALID, readinessScore: Number.NaN })).toThrow(/invalid_readiness_score/);
7676
});
7777

78+
// #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching
79+
// repo-clone.js's own validation, instead of being silently accepted and persisted as a ledger key --
80+
// for both the owner and repo segment independently.
81+
it("rejects a repoFullName with a path-traversal or invalid-character segment", () => {
82+
const ledger = tempLedger();
83+
// Both appendPrediction (write) and readPredictions (read) funnel through normalizeRepoFullName.
84+
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "../etc" })).toThrow("invalid_repo_full_name"); // owner ".." invalid
85+
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "o/.." })).toThrow("invalid_repo_full_name"); // repo ".." invalid
86+
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "o baz/a" })).toThrow("invalid_repo_full_name");
87+
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "o/a baz" })).toThrow("invalid_repo_full_name");
88+
expect(() => ledger.readPredictions({ repoFullName: "../etc" })).toThrow("invalid_repo_full_name");
89+
expect(() => ledger.readPredictions({ repoFullName: "o/.." })).toThrow("invalid_repo_full_name");
90+
});
91+
7892
it("scopes readPredictions by repo, preserving insertion order", () => {
7993
const ledger = tempLedger();
8094
ledger.appendPrediction({ ...VALID, repoFullName: "owner/repo-a", targetId: 1 });

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,4 +456,15 @@ describe("openReplaySnapshotStore (#3010) — round-trip persistence", () => {
456456
const store = tempStore();
457457
expect(store.getSnapshot("acme/widgets", "nope")).toBeNull();
458458
});
459+
460+
// #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching
461+
// repo-clone.js's own validation, instead of being silently accepted and used as a snapshot key --
462+
// for both the owner and repo segment independently.
463+
it("rejects a repoFullName with a path-traversal or invalid-character segment", () => {
464+
const store = tempStore();
465+
expect(() => store.getSnapshot("../etc", "abc123")).toThrow("invalid_repo_full_name"); // owner ".." invalid
466+
expect(() => store.getSnapshot("o/..", "abc123")).toThrow("invalid_repo_full_name"); // repo ".." invalid
467+
expect(() => store.getSnapshot("o baz/a", "abc123")).toThrow("invalid_repo_full_name");
468+
expect(() => store.getSnapshot("o/a baz", "abc123")).toThrow("invalid_repo_full_name");
469+
});
459470
});

test/unit/miner-run-state.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,24 @@ describe("loopover-miner run-state store (#2289)", () => {
136136
}
137137
});
138138

139+
// #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching
140+
// repo-clone.js's own validation, instead of being silently accepted and persisted as a state key --
141+
// for both the owner and repo segment independently.
142+
it("rejects a repoFullName with a path-traversal or invalid-character segment", () => {
143+
const store = initRunStateStore(join(tempRoot(), "run-state.sqlite3"));
144+
try {
145+
// Both getRunState (read) and setRunState (write) funnel through normalizeRepoFullName.
146+
expect(() => store.getRunState("../etc")).toThrow("invalid_repo_full_name"); // owner ".." invalid
147+
expect(() => store.getRunState("o/..")).toThrow("invalid_repo_full_name"); // repo ".." invalid
148+
expect(() => store.setRunState("o baz/a", "idle")).toThrow("invalid_repo_full_name");
149+
expect(() => store.setRunState("o/a baz", "idle")).toThrow("invalid_repo_full_name");
150+
expect(() => store.setRunState("../etc", "idle")).toThrow("invalid_repo_full_name");
151+
expect(() => store.setRunState("o/..", "idle")).toThrow("invalid_repo_full_name");
152+
} finally {
153+
store.close();
154+
}
155+
});
156+
139157
it("fails closed to null when a legacy table contains an unknown state", () => {
140158
const dbPath = join(tempRoot(), "legacy.sqlite3");
141159
const legacy = new DatabaseSync(dbPath);

0 commit comments

Comments
 (0)