Skip to content

Commit 8cf3570

Browse files
bidobirdJSONbored
authored andcommitted
fix(miner): guard repo segments in 4 more normalizeRepoFullName parsers
contribution-profile-cache.ts, prediction-ledger.ts, replay-snapshot.ts, and run-state.ts each define a normalizeRepoFullName that only checks "exactly one slash, both halves non-empty" — they never call repo-clone.ts's isValidRepoSegment, so a `.`/`..`/control-char owner or repo segment (e.g. "../repo", "owner/..") is accepted and persisted as a SQLite key / echoed through the sibling CLIs. Issues #5831 and #7525 already rolled this exact guard out to the other ten sibling parsers; these four were missed. Add the same `isValidRepoSegment(owner)/isValidRepoSegment(repo)` check each already-fixed sibling uses, and extend each file's existing malformed-repo test with path-traversal cases for both the owner and repo segment. Closes #7795
1 parent 299c842 commit 8cf3570

8 files changed

Lines changed: 28 additions & 0 deletions

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
resolveLocalStoreDbPath,
1515
} from "./local-store.js";
1616
import { applySchemaMigrations } from "./schema-version.js";
17+
import { isValidRepoSegment } from "./repo-clone.js";
1718
import {
1819
CONTRIBUTION_PROFILE_CACHE_PURGE_SPEC,
1920
purgeStoreByRepo,
@@ -58,6 +59,9 @@ 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+
// #7795: reject `.`/`..`/control-char segments (via repo-clone.js's shared guard) before this value backs a
63+
// SQLite key or is echoed through a CLI -- the same path-safety check #5831/#7525 rolled out to every sibling.
64+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
6165
return `${owner}/${repo}`;
6266
}
6367

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { DatabaseSync } from "node:sqlite";
22
import { normalizeLocalStoreDbPath, openLocalStoreAdapter, resolveLocalStoreDbPath } from "./local-store.js";
33
import { applySchemaMigrations } from "./schema-version.js";
4+
import { isValidRepoSegment } from "./repo-clone.js";
45
import {
56
PREDICTION_LEDGER_PURGE_SPEC,
67
PREDICTION_LEDGER_RETENTION_SPEC,
@@ -87,6 +88,9 @@ 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+
// #7795: reject `.`/`..`/control-char segments (via repo-clone.js's shared guard) before this value backs a
92+
// SQLite key or is echoed through a CLI -- the same path-safety check #5831/#7525 rolled out to every sibling.
93+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
9094
return `${owner}/${repo}`;
9195
}
9296

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

Lines changed: 4 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,9 @@ 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+
// #7795: reject `.`/`..`/control-char segments (via repo-clone.js's shared guard) before this value backs a
77+
// SQLite key or is echoed through a CLI -- the same path-safety check #5831/#7525 rolled out to every sibling.
78+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
7579
return `${owner}/${repo}`;
7680
}
7781

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

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

78
export type RunState = "idle" | "discovering" | "planning" | "preparing";
@@ -57,6 +58,9 @@ 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+
// #7795: reject `.`/`..`/control-char segments (via repo-clone.js's shared guard) before this value backs a
62+
// SQLite key or is echoed through a CLI -- the same path-safety check #5831/#7525 rolled out to every sibling.
63+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
6064
return `${owner}/${repo}`;
6165
}
6266

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,9 @@ describe("contribution-profile cache store (#6797)", () => {
158158
expect(() => store.put({ repoFullName: 42 } as never)).toThrow(
159159
"invalid_repo_full_name",
160160
);
161+
// #7795: `.`/`..`/control-char segments must be rejected too (both owner and repo), not just missing/extra slashes.
162+
expect(() => store.get("../etc")).toThrow("invalid_repo_full_name");
163+
expect(() => store.get("owner/..")).toThrow("invalid_repo_full_name");
161164
});
162165

163166
it("exposes module-level get/put helpers backed by the default DB path", () => {

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,9 @@ describe("miner prediction ledger (#4263)", () => {
6868
it("rejects invalid inputs field by field", () => {
6969
const ledger = tempLedger();
7070
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "no-slash" })).toThrow(/invalid_repo_full_name/);
71+
// #7795: `.`/`..`/control-char segments must be rejected too (both owner and repo), not just missing/extra slashes.
72+
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "../etc" })).toThrow(/invalid_repo_full_name/);
73+
expect(() => ledger.appendPrediction({ ...VALID, repoFullName: "owner/.." })).toThrow(/invalid_repo_full_name/);
7174
expect(() => ledger.appendPrediction({ ...VALID, targetId: 0 })).toThrow(/invalid_target_id/);
7275
expect(() => ledger.appendPrediction({ ...VALID, conclusion: "" })).toThrow(/invalid_conclusion/);
7376
expect(() => ledger.appendPrediction({ ...VALID, engineVersion: "" })).toThrow(/invalid_engine_version/);

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,9 @@ describe("exportReplaySnapshot (#3010)", () => {
341341

342342
await expect(exportReplaySnapshot({ repoPath: "/repo", repoFullName: "noslash", commitSha: "a" }, deps)).rejects.toThrow("invalid_repo_full_name");
343343
await expect(exportReplaySnapshot({ repoPath: "/repo", repoFullName: "a/b/c", commitSha: "a" }, deps)).rejects.toThrow("invalid_repo_full_name");
344+
// #7795: `.`/`..`/control-char segments must be rejected too (both owner and repo), not just missing/extra slashes.
345+
await expect(exportReplaySnapshot({ repoPath: "/repo", repoFullName: "../etc", commitSha: "a" }, deps)).rejects.toThrow("invalid_repo_full_name");
346+
await expect(exportReplaySnapshot({ repoPath: "/repo", repoFullName: "owner/..", commitSha: "a" }, deps)).rejects.toThrow("invalid_repo_full_name");
344347
});
345348

346349
it("assertExecResult falls back to a generic exit-code message when stderr is entirely absent", async () => {

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,9 @@ describe("loopover-miner run-state store (#2289)", () => {
129129
try {
130130
expect(() => store.getRunState("not-a-full-name")).toThrow("invalid_repo_full_name");
131131
expect(() => store.setRunState("owner/repo/extra", "idle")).toThrow("invalid_repo_full_name");
132+
// #7795: `.`/`..`/control-char segments must be rejected too (both owner and repo), not just missing/extra slashes.
133+
expect(() => store.getRunState("../etc")).toThrow("invalid_repo_full_name");
134+
expect(() => store.setRunState("owner/..", "idle")).toThrow("invalid_repo_full_name");
132135
expect(() => store.setRunState("owner/repo", "blocked" as never)).toThrow("invalid_run_state");
133136
expect(store.getRunState("owner/repo")).toBeNull();
134137
} finally {

0 commit comments

Comments
 (0)