From 09ad4e1d980819b4c6be20662016cee65140b589 Mon Sep 17 00:00:00 2001 From: davion-knight Date: Wed, 22 Jul 2026 12:43:48 -0500 Subject: [PATCH] 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's persisted to SQLite or echoed through a CLI. #5831 and #7525 rolled it out to ten sibling parsers, but four were missed: contribution-profile-cache.ts, prediction-ledger.ts, replay-snapshot.ts, and run-state.ts still only checked 'exactly one slash, both halves non-empty'. Inputs like 'owner/..', '../repo', or tab/newline segments passed these four while being rejected everywhere else -- and each backs a SQLite key and is echoed through its sibling CLI, the exact rationale #5831/#7525 used. 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 --- .../lib/contribution-profile-cache.ts | 2 ++ .../loopover-miner/lib/prediction-ledger.ts | 2 ++ packages/loopover-miner/lib/replay-snapshot.ts | 2 ++ packages/loopover-miner/lib/run-state.ts | 2 ++ .../miner-contribution-profile-cache.test.ts | 14 ++++++++++++++ test/unit/miner-prediction-ledger.test.ts | 14 ++++++++++++++ test/unit/miner-replay-snapshot.test.ts | 11 +++++++++++ test/unit/miner-run-state.test.ts | 18 ++++++++++++++++++ 8 files changed, 65 insertions(+) diff --git a/packages/loopover-miner/lib/contribution-profile-cache.ts b/packages/loopover-miner/lib/contribution-profile-cache.ts index 46f00660c1..d0ac746a01 100644 --- a/packages/loopover-miner/lib/contribution-profile-cache.ts +++ b/packages/loopover-miner/lib/contribution-profile-cache.ts @@ -13,6 +13,7 @@ import { openLocalStoreAdapter, resolveLocalStoreDbPath, } from "./local-store.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { applySchemaMigrations } from "./schema-version.js"; import { CONTRIBUTION_PROFILE_CACHE_PURGE_SPEC, @@ -58,6 +59,7 @@ function normalizeRepoFullName(repoFullName: unknown): string { const [owner, repo, extra] = repoFullName.trim().split("/"); if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name"); + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name"); return `${owner}/${repo}`; } diff --git a/packages/loopover-miner/lib/prediction-ledger.ts b/packages/loopover-miner/lib/prediction-ledger.ts index ad4f4bda80..1f3928a10d 100644 --- a/packages/loopover-miner/lib/prediction-ledger.ts +++ b/packages/loopover-miner/lib/prediction-ledger.ts @@ -1,5 +1,6 @@ import type { DatabaseSync } from "node:sqlite"; import { normalizeLocalStoreDbPath, openLocalStoreAdapter, resolveLocalStoreDbPath } from "./local-store.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { applySchemaMigrations } from "./schema-version.js"; import { PREDICTION_LEDGER_PURGE_SPEC, @@ -87,6 +88,7 @@ function normalizeRepoFullName(repoFullName: string): string { if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name"); const [owner, repo, extra] = repoFullName.trim().split("/"); if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name"); + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name"); return `${owner}/${repo}`; } diff --git a/packages/loopover-miner/lib/replay-snapshot.ts b/packages/loopover-miner/lib/replay-snapshot.ts index 03b80b77ad..42876f417c 100644 --- a/packages/loopover-miner/lib/replay-snapshot.ts +++ b/packages/loopover-miner/lib/replay-snapshot.ts @@ -2,6 +2,7 @@ import { join } from "node:path"; import { removeWorktree } from "@loopover/engine"; import type { WorktreeExecFn, WorktreeRemoveResult } from "@loopover/engine"; import { openLocalStoreAdapter, resolveLocalStoreDbPath, normalizeLocalStoreDbPath } from "./local-store.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { REPLAY_SNAPSHOT_PURGE_SPEC, purgeStoreByRepo } from "./store-maintenance.js"; // Freeze/snapshot mechanism for historical replay targets (#3010). Given a repo and a commit SHA T, exports: @@ -75,6 +76,7 @@ function normalizeRepoFullName(repoFullName: string): string { if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name"); const [owner, repo, extra] = repoFullName.trim().split("/"); if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name"); + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name"); return `${owner}/${repo}`; } diff --git a/packages/loopover-miner/lib/run-state.ts b/packages/loopover-miner/lib/run-state.ts index ec8de4d4a4..2538f62865 100644 --- a/packages/loopover-miner/lib/run-state.ts +++ b/packages/loopover-miner/lib/run-state.ts @@ -1,6 +1,7 @@ import type { DatabaseSync } from "node:sqlite"; import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { normalizeLocalStoreDbPath, openLocalStoreAdapter, resolveLocalStoreDbPath } from "./local-store.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { applySchemaMigrations } from "./schema-version.js"; import { RUN_STATE_PURGE_SPEC, purgeStoreByRepo } from "./store-maintenance.js"; @@ -57,6 +58,7 @@ function normalizeRepoFullName(repoFullName: string): string { const trimmed = repoFullName.trim(); const [owner, repo, extra] = trimmed.split("/"); if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name"); + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name"); return `${owner}/${repo}`; } diff --git a/test/unit/miner-contribution-profile-cache.test.ts b/test/unit/miner-contribution-profile-cache.test.ts index 621b23c536..6eb9f3fae2 100644 --- a/test/unit/miner-contribution-profile-cache.test.ts +++ b/test/unit/miner-contribution-profile-cache.test.ts @@ -160,6 +160,20 @@ describe("contribution-profile cache store (#6797)", () => { ); }); + // #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching + // repo-clone.js's own validation, instead of being silently accepted and persisted as a cache key -- + // for both the owner and repo segment independently. + it("rejects a repoFullName with a path-traversal or invalid-character segment", () => { + const store = tempStore(); + // Reads (get) and writes (put) both funnel through normalizeRepoFullName. + expect(() => store.get("../etc")).toThrow("invalid_repo_full_name"); // owner ".." invalid + expect(() => store.get("o/..")).toThrow("invalid_repo_full_name"); // repo ".." invalid + expect(() => store.get("o baz/a")).toThrow("invalid_repo_full_name"); + expect(() => store.get("o/a baz")).toThrow("invalid_repo_full_name"); + expect(() => store.put(profile("../etc"), AT_MS)).toThrow("invalid_repo_full_name"); + expect(() => store.put(profile("o/.."), AT_MS)).toThrow("invalid_repo_full_name"); + }); + it("exposes module-level get/put helpers backed by the default DB path", () => { vi.stubEnv( "LOOPOVER_MINER_CONTRIBUTION_PROFILE_CACHE_DB", diff --git a/test/unit/miner-prediction-ledger.test.ts b/test/unit/miner-prediction-ledger.test.ts index 404ae0d385..7c8fbc1df1 100644 --- a/test/unit/miner-prediction-ledger.test.ts +++ b/test/unit/miner-prediction-ledger.test.ts @@ -75,6 +75,20 @@ describe("miner prediction ledger (#4263)", () => { expect(() => ledger.appendPrediction({ ...VALID, readinessScore: Number.NaN })).toThrow(/invalid_readiness_score/); }); + // #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching + // repo-clone.js's own validation, instead of being silently accepted and persisted as a ledger key -- + // for both the owner and repo segment independently. + it("rejects a repoFullName with a path-traversal or invalid-character segment", () => { + const ledger = tempLedger(); + // Both appendPrediction (write) and readPredictions (read) funnel through normalizeRepoFullName. + expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "../etc" })).toThrow("invalid_repo_full_name"); // owner ".." invalid + expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "o/.." })).toThrow("invalid_repo_full_name"); // repo ".." invalid + expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "o baz/a" })).toThrow("invalid_repo_full_name"); + expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "o/a baz" })).toThrow("invalid_repo_full_name"); + expect(() => ledger.readPredictions({ repoFullName: "../etc" })).toThrow("invalid_repo_full_name"); + expect(() => ledger.readPredictions({ repoFullName: "o/.." })).toThrow("invalid_repo_full_name"); + }); + it("scopes readPredictions by repo, preserving insertion order", () => { const ledger = tempLedger(); ledger.appendPrediction({ ...VALID, repoFullName: "owner/repo-a", targetId: 1 }); diff --git a/test/unit/miner-replay-snapshot.test.ts b/test/unit/miner-replay-snapshot.test.ts index ac9dd2899d..f546c89df2 100644 --- a/test/unit/miner-replay-snapshot.test.ts +++ b/test/unit/miner-replay-snapshot.test.ts @@ -362,6 +362,17 @@ describe("exportReplaySnapshot (#3010)", () => { await expect(exportReplaySnapshot({ repoPath: "/repo", repoFullName: "a/b/c", commitSha: "a" }, deps)).rejects.toThrow("invalid_repo_full_name"); }); + // #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching + // repo-clone.js's own validation, instead of being silently accepted and used as a snapshot key -- + // for both the owner and repo segment independently. + it("rejects a repoFullName with a path-traversal or invalid-character segment", () => { + const store = tempStore(); + expect(() => store.getSnapshot("../etc", "abc123")).toThrow("invalid_repo_full_name"); // owner ".." invalid + expect(() => store.getSnapshot("o/..", "abc123")).toThrow("invalid_repo_full_name"); // repo ".." invalid + expect(() => store.getSnapshot("o baz/a", "abc123")).toThrow("invalid_repo_full_name"); + expect(() => store.getSnapshot("o/a baz", "abc123")).toThrow("invalid_repo_full_name"); + }); + it("assertExecResult falls back to a generic exit-code message when stderr is entirely absent", async () => { const { exec } = scriptedExec(happyPathScripts([{ match: isWorktreeAdd, result: { code: 1 } }])); const store = tempStore(); diff --git a/test/unit/miner-run-state.test.ts b/test/unit/miner-run-state.test.ts index cbbe71c202..409b1a05e2 100644 --- a/test/unit/miner-run-state.test.ts +++ b/test/unit/miner-run-state.test.ts @@ -136,6 +136,24 @@ describe("loopover-miner run-state store (#2289)", () => { } }); + // #7795: an unsafe path-traversal/invalid-character segment must be rejected here too, matching + // repo-clone.js's own validation, instead of being silently accepted and persisted as a state key -- + // for both the owner and repo segment independently. + it("rejects a repoFullName with a path-traversal or invalid-character segment", () => { + const store = initRunStateStore(join(tempRoot(), "run-state.sqlite3")); + try { + // Both getRunState (read) and setRunState (write) funnel through normalizeRepoFullName. + expect(() => store.getRunState("../etc")).toThrow("invalid_repo_full_name"); // owner ".." invalid + expect(() => store.getRunState("o/..")).toThrow("invalid_repo_full_name"); // repo ".." invalid + expect(() => store.setRunState("o baz/a", "idle")).toThrow("invalid_repo_full_name"); + expect(() => store.setRunState("o/a baz", "idle")).toThrow("invalid_repo_full_name"); + expect(() => store.setRunState("../etc", "idle")).toThrow("invalid_repo_full_name"); + expect(() => store.setRunState("o/..", "idle")).toThrow("invalid_repo_full_name"); + } finally { + store.close(); + } + }); + it("fails closed to null when a legacy table contains an unknown state", () => { const dbPath = join(tempRoot(), "legacy.sqlite3"); const legacy = new DatabaseSync(dbPath);