From d1637008dcdc956cac442f66033a39e90f133f4e Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:50:35 +0000 Subject: [PATCH] fix(miner): wire isValidRepoSegment into the seven remaining owner/repo parsers discover-cli/loop-cli's parseRepoTarget, manage-poll/purge-cli/run-state-cli's parseRepoArg, and event-ledger/ranked-candidates's normalize helpers all split an owner/repo string without checking either segment against the shared path-traversal guard, so a `.`/`..`/control-char value could still reach resolveRepoCloneDir, persist into the event ledger, or land in the ranked-candidates snapshot. Each now rejects through its own existing error shape, matching the pattern already used in portfolio-queue-cli.ts and claim-ledger.ts. Also drops the stale file list from repo-clone.ts's guard comment now that every parser in the package shares it. --- packages/loopover-miner/lib/discover-cli.ts | 2 ++ packages/loopover-miner/lib/event-ledger.ts | 2 ++ packages/loopover-miner/lib/loop-cli.ts | 2 ++ packages/loopover-miner/lib/manage-poll.ts | 4 ++++ packages/loopover-miner/lib/purge-cli.ts | 4 ++++ packages/loopover-miner/lib/ranked-candidates.ts | 2 ++ packages/loopover-miner/lib/repo-clone.ts | 5 ++--- packages/loopover-miner/lib/run-state-cli.ts | 4 ++++ test/unit/miner-cli-run-state.test.ts | 15 +++++++++++++++ test/unit/miner-discover-cli.test.ts | 9 +++++++++ test/unit/miner-event-ledger.test.ts | 12 ++++++++++++ test/unit/miner-loop-cli.test.ts | 9 +++++++++ test/unit/miner-manage-poll.test.ts | 9 +++++++++ test/unit/miner-purge-cli.test.ts | 5 +++++ test/unit/miner-ranked-candidates.test.ts | 15 +++++++++++++++ 15 files changed, 96 insertions(+), 3 deletions(-) diff --git a/packages/loopover-miner/lib/discover-cli.ts b/packages/loopover-miner/lib/discover-cli.ts index 22fc990cf8..bfa18dd5e6 100644 --- a/packages/loopover-miner/lib/discover-cli.ts +++ b/packages/loopover-miner/lib/discover-cli.ts @@ -23,6 +23,7 @@ import { initPolicyDocCacheStore } from "./policy-doc-cache.js"; import type { PolicyDocCacheStore } from "./policy-doc-cache.js"; import { initPolicyVerdictCacheStore } from "./policy-verdict-cache.js"; import type { PolicyVerdictCacheStore } from "./policy-verdict-cache.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { enqueueRankedDiscovery } from "./portfolio-discovery.js"; import { AMS_MIN_RANK_SHIPPED, readMinRankAutotuneEnabled, readMinRankOverride } from "./ams-calibration.js"; import { initEventLedger, resolveEventLedgerDbPath } from "./event-ledger.js"; @@ -212,6 +213,7 @@ function parseRepoTarget(value: string): FanoutTarget | null { const trimmed = value.trim(); const [owner, repo, extra] = trimmed.split("/"); if (!owner || !repo || extra !== undefined) return null; + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null; return { owner, repo }; } diff --git a/packages/loopover-miner/lib/event-ledger.ts b/packages/loopover-miner/lib/event-ledger.ts index 52f3517aef..b85fc4f29f 100644 --- a/packages/loopover-miner/lib/event-ledger.ts +++ b/packages/loopover-miner/lib/event-ledger.ts @@ -2,6 +2,7 @@ import type { DatabaseSync, SQLOutputValue } from "node:sqlite"; import { isDeepStrictEqual } from "node:util"; import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; import { applySchemaMigrations } from "./schema-version.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { EVENT_LEDGER_PURGE_SPEC, EVENT_LEDGER_RETENTION_SPEC, @@ -83,6 +84,7 @@ function normalizeOptionalRepoFullName(repoFullName: unknown): string | null { 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/loop-cli.ts b/packages/loopover-miner/lib/loop-cli.ts index a388ceba9f..5d21d3ebbe 100644 --- a/packages/loopover-miner/lib/loop-cli.ts +++ b/packages/loopover-miner/lib/loop-cli.ts @@ -49,6 +49,7 @@ import { buildLoopClosureSummary } from "./loop-closure.js"; import { attemptLoopReentry } from "./loop-reentry.js"; import { parsePrNumberFromExecResult } from "./pr-number-parse.js"; import { resolveGitHubToken } from "./github-token-resolution.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { DEFAULT_AMS_POLICY_SPEC } from "@loopover/engine"; import type { GovernorCapUsage } from "@loopover/engine"; @@ -118,6 +119,7 @@ function parseRepoTarget(value: string): string | null { const trimmed = value.trim(); const [owner, repo, extra] = trimmed.split("/"); if (!owner || !repo || extra !== undefined) return null; + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null; return `${owner}/${repo}`; } diff --git a/packages/loopover-miner/lib/manage-poll.ts b/packages/loopover-miner/lib/manage-poll.ts index 1ae68c3408..856c692beb 100644 --- a/packages/loopover-miner/lib/manage-poll.ts +++ b/packages/loopover-miner/lib/manage-poll.ts @@ -11,6 +11,7 @@ import type { PortfolioQueueStore } from "./portfolio-queue.js"; import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; import { resolveGitHubToken } from "./github-token-resolution.js"; +import { isValidRepoSegment } from "./repo-clone.js"; const MANAGE_POLL_USAGE = "Usage: loopover-miner manage poll [--branch ] [--dry-run] [--json]"; @@ -54,6 +55,9 @@ function parseRepoArg(value: string): { repoFullName: string } | { error: string if (!owner || !repo || extra !== undefined) { return { error: "Repository must be in owner/repo form." }; } + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) { + return { error: "Repository must be in owner/repo form." }; + } return { repoFullName: `${owner}/${repo}` }; } diff --git a/packages/loopover-miner/lib/purge-cli.ts b/packages/loopover-miner/lib/purge-cli.ts index e0e83dc956..26f99dc738 100644 --- a/packages/loopover-miner/lib/purge-cli.ts +++ b/packages/loopover-miner/lib/purge-cli.ts @@ -60,6 +60,7 @@ import { } from "./store-maintenance.js"; import type { LedgerPurgeSpec } from "./store-maintenance.js"; import { argsWantJson, reportCliFailure } from "./cli-error.js"; +import { isValidRepoSegment } from "./repo-clone.js"; const PURGE_USAGE = "Usage: loopover-miner purge --repo [--dry-run] [--json]"; @@ -146,6 +147,9 @@ function parseRepoArg(value: string | undefined, usage: string): ParsedRepoArg { if (!owner || !repo || extra !== undefined) { return { error: "Repository must be in owner/repo form." }; } + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) { + return { error: "Repository must be in owner/repo form." }; + } return { repoFullName: `${owner}/${repo}` }; } diff --git a/packages/loopover-miner/lib/ranked-candidates.ts b/packages/loopover-miner/lib/ranked-candidates.ts index 71d45f1b17..efcb1148ed 100644 --- a/packages/loopover-miner/lib/ranked-candidates.ts +++ b/packages/loopover-miner/lib/ranked-candidates.ts @@ -2,6 +2,7 @@ import type { SQLOutputValue } from "node:sqlite"; import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; import { applySchemaMigrations } from "./schema-version.js"; import { RANKED_CANDIDATES_PURGE_SPEC, purgeStoreByRepo } from "./store-maintenance.js"; +import { isValidRepoSegment } from "./repo-clone.js"; // Last-discover-run ranked-candidates snapshot (#4859 prerequisite): `discover-cli.js`'s runDiscover already // computes the FULL per-issue ranking breakdown (rankScore/laneFit/freshness/potential/feasibility/dupRisk, via @@ -110,6 +111,7 @@ function normalizeRepoFullName(value: unknown, error: string): string { const repoFullName = typeof value === "string" ? value.trim() : ""; const [owner, repo, extra] = repoFullName.split("/"); if (!owner || !repo || extra !== undefined) throw new Error(error); + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error(error); return `${owner}/${repo}`; } diff --git a/packages/loopover-miner/lib/repo-clone.ts b/packages/loopover-miner/lib/repo-clone.ts index ecd2725e1b..f85116bca9 100644 --- a/packages/loopover-miner/lib/repo-clone.ts +++ b/packages/loopover-miner/lib/repo-clone.ts @@ -66,9 +66,8 @@ export function resolveRepoCloneBaseDir(env?: Record // GitHub owner/repo names are restricted to alphanumerics, hyphens, underscores, and periods, and are never // exactly "." or ".." -- both are rejected here so a value like "../foo" can't make resolveRepoCloneDir's // join(cloneBaseDir, owner, repo) escape the intended clone directory (a real path-traversal finding). -// Exported so every other owner/repo parser in this package (#5831) shares this one definition instead of -// duplicating it (cross-repo-evaluation.js) or skipping it entirely (attempt-cli.js, claim-ledger-cli.js, -// event-ledger-cli.js, claim-ledger.js). +// Exported so every owner/repo parser in this package (#5831) shares this one definition instead of +// duplicating it or skipping it entirely. export const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; export function isPathTraversalSegment(segment: string): boolean { diff --git a/packages/loopover-miner/lib/run-state-cli.ts b/packages/loopover-miner/lib/run-state-cli.ts index 90c9ca7390..5d3eb3a7cf 100644 --- a/packages/loopover-miner/lib/run-state-cli.ts +++ b/packages/loopover-miner/lib/run-state-cli.ts @@ -1,6 +1,7 @@ import { RUN_STATES, getRunState, setRunState } from "./run-state.js"; import type { RunState } from "./run-state.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; +import { isValidRepoSegment } from "./repo-clone.js"; const STATE_GET_USAGE = "Usage: loopover-miner state get [--api-base-url ] [--json]"; const STATE_SET_USAGE = @@ -35,6 +36,9 @@ function parseRepoArg(value: string | undefined, usage: string): ParsedRepoArg { if (!owner || !repo || extra !== undefined) { return { error: "Repository must be in owner/repo form." }; } + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) { + return { error: "Repository must be in owner/repo form." }; + } return { repoFullName: `${owner}/${repo}` }; } diff --git a/test/unit/miner-cli-run-state.test.ts b/test/unit/miner-cli-run-state.test.ts index f45d522ead..ba6a3ef488 100644 --- a/test/unit/miner-cli-run-state.test.ts +++ b/test/unit/miner-cli-run-state.test.ts @@ -172,6 +172,21 @@ describe("loopover-miner state CLI", () => { }); }); + it("REGRESSION (#9684): parseStateGetArgs and parseStateSetArgs reject a path-traversal repo segment", () => { + expect(parseStateGetArgs(["../acme"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseStateGetArgs(["acme/.."])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseStateSetArgs(["../acme", "idle"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseStateSetArgs(["acme/..", "idle"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + }); + it("runStateGet returns exit code 2 for malformed repositories", () => { const error = vi.spyOn(console, "error").mockImplementation(() => undefined); expect(runStateGet(["not-a-repo"])).toBe(2); diff --git a/test/unit/miner-discover-cli.test.ts b/test/unit/miner-discover-cli.test.ts index 9a23e1deb5..e025596f71 100644 --- a/test/unit/miner-discover-cli.test.ts +++ b/test/unit/miner-discover-cli.test.ts @@ -171,6 +171,15 @@ describe("parseDiscoverArgs (#4247)", () => { }); }); + it("REGRESSION (#9684): rejects a path-traversal repo target segment", () => { + expect(parseDiscoverArgs(["../acme"])).toEqual({ + error: "Repository must be in owner/repo form: ../acme", + }); + expect(parseDiscoverArgs(["acme/.."])).toEqual({ + error: "Repository must be in owner/repo form: acme/..", + }); + }); + it("rejects mixing repo targets with --search", () => { expect(parseDiscoverArgs(["acme/widgets", "--search", "x"])).toEqual({ error: "Pass either repository targets or --search, not both.", diff --git a/test/unit/miner-event-ledger.test.ts b/test/unit/miner-event-ledger.test.ts index d846dd4aa7..601f8d4cf7 100644 --- a/test/unit/miner-event-ledger.test.ts +++ b/test/unit/miner-event-ledger.test.ts @@ -149,6 +149,18 @@ describe("loopover-miner event ledger (#2290)", () => { ); }); + // #9684: a `.`/`..` segment must not persist into miner_event_ledger.repo_full_name -- both the owner and + // repo positions are checked, matching every other owner/repo parser in the package (#5831). + it("REGRESSION (#9684): rejects a repoFullName with a path-traversal segment", () => { + const ledger = tempLedger(); + expect(() => ledger.appendEvent({ type: "x", repoFullName: "../etc", payload: {} })).toThrow( + "invalid_repo_full_name", + ); + expect(() => ledger.appendEvent({ type: "x", repoFullName: "o/..", payload: {} })).toThrow( + "invalid_repo_full_name", + ); + }); + it("rejects a payload JSON would not round-trip verbatim, and accepts a nested JSON-safe one", () => { const ledger = tempLedger(); // Values JSON drops or coerces would make the audit entry differ from what was appended. diff --git a/test/unit/miner-loop-cli.test.ts b/test/unit/miner-loop-cli.test.ts index 107b4718ec..41c3d237f4 100644 --- a/test/unit/miner-loop-cli.test.ts +++ b/test/unit/miner-loop-cli.test.ts @@ -157,6 +157,15 @@ describe("parseLoopArgs (#5135)", () => { }); }); + it("REGRESSION (#9684): rejects a path-traversal repo target segment", () => { + expect(parseLoopArgs(["../acme", "--miner-login", "alice"])).toEqual({ + error: "Repository must be in owner/repo form: ../acme", + }); + expect(parseLoopArgs(["acme/..", "--miner-login", "alice"])).toEqual({ + error: "Repository must be in owner/repo form: acme/..", + }); + }); + it("rejects a non-integer or negative --max-cycles / --cycle-delay-ms", () => { expect(parseLoopArgs(["acme/widgets", "--miner-login", "alice", "--max-cycles", "abc"])).toHaveProperty("error"); expect(parseLoopArgs(["acme/widgets", "--miner-login", "alice", "--max-cycles", "-1"])).toHaveProperty("error"); diff --git a/test/unit/miner-manage-poll.test.ts b/test/unit/miner-manage-poll.test.ts index 73826d549f..32d96beafc 100644 --- a/test/unit/miner-manage-poll.test.ts +++ b/test/unit/miner-manage-poll.test.ts @@ -338,6 +338,15 @@ describe("loopover-miner manage poll (#2323/#2325)", () => { }); }); + it("REGRESSION (#9684): parseManagePollArgs rejects a path-traversal repo segment", () => { + expect(parseManagePollArgs(["../acme", "42"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseManagePollArgs(["acme/..", "42"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + }); + it("parseManagePollArgs rejects a malformed --branch flag", () => { expect(parseManagePollArgs(["acme/widgets", "42", "--branch"])).toEqual({ error: expect.stringContaining("Usage: loopover-miner manage poll"), diff --git a/test/unit/miner-purge-cli.test.ts b/test/unit/miner-purge-cli.test.ts index e20b8a1bd1..9f726d51f8 100644 --- a/test/unit/miner-purge-cli.test.ts +++ b/test/unit/miner-purge-cli.test.ts @@ -79,6 +79,11 @@ describe("parsePurgeArgs (#5564)", () => { expect(parsePurgeArgs(["--repo", "no-slash"])).toEqual({ error: "Repository must be in owner/repo form." }); }); + it("REGRESSION (#9684): rejects a path-traversal --repo segment", () => { + expect(parsePurgeArgs(["--repo", "../acme"])).toEqual({ error: "Repository must be in owner/repo form." }); + expect(parsePurgeArgs(["--repo", "acme/.."])).toEqual({ error: "Repository must be in owner/repo form." }); + }); + it("rejects a --repo flag missing its value", () => { expect(parsePurgeArgs(["--repo"])).toEqual({ error: expect.stringContaining("Usage: loopover-miner purge") }); expect(parsePurgeArgs(["--repo", "--json"])).toEqual({ error: expect.stringContaining("Usage: loopover-miner purge") }); diff --git a/test/unit/miner-ranked-candidates.test.ts b/test/unit/miner-ranked-candidates.test.ts index b47bf951c5..4afb22719c 100644 --- a/test/unit/miner-ranked-candidates.test.ts +++ b/test/unit/miner-ranked-candidates.test.ts @@ -197,6 +197,21 @@ describe("loopover-miner ranked-candidates store (#4859 prerequisite)", () => { } }); + it("REGRESSION (#9684): rejects a candidate repoFullName with a path-traversal segment", () => { + const dbPath = join(tempRoot(), "ranked-candidates.sqlite3"); + const store = initRankedCandidatesStore(dbPath); + try { + expect(() => store.saveRankedCandidates([{ ...fullCandidate, repoFullName: "../etc" }])).toThrow( + "invalid_ranked_candidate", + ); + expect(() => store.saveRankedCandidates([{ ...fullCandidate, repoFullName: "acme/.." }])).toThrow( + "invalid_ranked_candidate", + ); + } finally { + store.close(); + } + }); + it("purgeByRepo deletes only the given repo's snapshot rows and returns the count (#8009)", () => { const dbPath = join(tempRoot(), "ranked-candidates.sqlite3"); const store = initRankedCandidatesStore(dbPath);