From e064ab81042273d2fabc8f0c03065bb466dde124 Mon Sep 17 00:00:00 2001 From: nghetienhiep <13849419+nghetienhiep@users.noreply.github.com> Date: Wed, 15 Jul 2026 10:15:54 +0000 Subject: [PATCH] fix(miner): share repo-clone.js's path-safety validation across owner/repo CLI parsers Closes #5831 --- packages/loopover-miner/lib/attempt-cli.js | 2 ++ .../loopover-miner/lib/claim-ledger-cli.js | 3 ++- packages/loopover-miner/lib/claim-ledger.js | 2 ++ .../lib/cross-repo-evaluation.js | 11 ++-------- .../loopover-miner/lib/event-ledger-cli.js | 3 ++- packages/loopover-miner/lib/repo-clone.d.ts | 6 ++++++ packages/loopover-miner/lib/repo-clone.js | 14 +++++++++---- test/unit/miner-attempt-cli.test.ts | 18 +++++++++++++++++ test/unit/miner-claim-ledger-cli.test.ts | 15 ++++++++++++++ test/unit/miner-claim-ledger.test.ts | 11 ++++++++++ test/unit/miner-cross-repo-evaluation.test.ts | 10 ++++++++++ test/unit/miner-event-ledger-cli.test.ts | 20 +++++++++++++++++++ 12 files changed, 100 insertions(+), 15 deletions(-) diff --git a/packages/loopover-miner/lib/attempt-cli.js b/packages/loopover-miner/lib/attempt-cli.js index 8f9a2713a8..ec7ac74c81 100644 --- a/packages/loopover-miner/lib/attempt-cli.js +++ b/packages/loopover-miner/lib/attempt-cli.js @@ -27,6 +27,7 @@ import { initEventLedger } from "./event-ledger.js"; import { initAttemptLog } from "./attempt-log.js"; import { initGovernorLedger } from "./governor-ledger.js"; import { openWorktreeAllocator } from "./worktree-allocator.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { REJECTION_REASON_AI_USAGE_POLICY_BAN, REJECTION_REASON_OWN_SUBMISSION_REJECTED, resolveRejectionSignaled } from "./rejection-signal.js"; import { cleanupAttemptWorktree, prepareAttemptWorktree } from "./attempt-worktree.js"; import { fetchSelfReviewContext } from "./self-review-context.js"; @@ -46,6 +47,7 @@ function parseRepoTarget(value) { const trimmed = typeof value === "string" ? 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/claim-ledger-cli.js b/packages/loopover-miner/lib/claim-ledger-cli.js index 1b8aa48de9..5d186b4301 100644 --- a/packages/loopover-miner/lib/claim-ledger-cli.js +++ b/packages/loopover-miner/lib/claim-ledger-cli.js @@ -1,5 +1,6 @@ import { CLAIM_STATUSES, openClaimLedger } from "./claim-ledger.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; +import { isValidRepoSegment } from "./repo-clone.js"; const CLAIM_CLAIM_USAGE = "Usage: loopover-miner claim claim [--note ] [--api-base-url ] [--dry-run] [--json]"; @@ -12,7 +13,7 @@ function parseRepoArg(value, usage) { if (!value) return { error: usage }; const trimmed = value.trim(); const [owner, repo, extra] = trimmed.split("/"); - if (!owner || !repo || extra !== undefined) { + if (!owner || !repo || extra !== undefined || !isValidRepoSegment(owner) || !isValidRepoSegment(repo)) { return { error: "Repository must be in owner/repo form." }; } return { repoFullName: `${owner}/${repo}` }; diff --git a/packages/loopover-miner/lib/claim-ledger.js b/packages/loopover-miner/lib/claim-ledger.js index 5275be2611..b448458046 100644 --- a/packages/loopover-miner/lib/claim-ledger.js +++ b/packages/loopover-miner/lib/claim-ledger.js @@ -1,6 +1,7 @@ import { DatabaseSync } from "node:sqlite"; import { DEFAULT_FORGE_CONFIG } from "./forge-config.js"; import { normalizeLocalStoreDbPath, openLocalStoreDb, resolveLocalStoreDbPath } from "./local-store.js"; +import { isValidRepoSegment } from "./repo-clone.js"; import { applySchemaMigrations } from "./schema-version.js"; import { CLAIM_LEDGER_PURGE_SPEC, purgeStoreByRepo } from "./store-maintenance.js"; @@ -27,6 +28,7 @@ function normalizeRepoFullName(repoFullName) { 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/cross-repo-evaluation.js b/packages/loopover-miner/lib/cross-repo-evaluation.js index 120152962a..e5e03050f5 100644 --- a/packages/loopover-miner/lib/cross-repo-evaluation.js +++ b/packages/loopover-miner/lib/cross-repo-evaluation.js @@ -8,7 +8,7 @@ import { existsSync } from "node:fs"; import { join } from "node:path"; import { buildCodingTaskSpec } from "./coding-task-spec.js"; import { resolveMinerGoalSpec } from "./miner-goal-spec.js"; -import { resolveRepoCloneDir } from "./repo-clone.js"; +import { isValidRepoSegment, resolveRepoCloneDir } from "./repo-clone.js"; import { detectRepoStack } from "./stack-detection.js"; /** Failure taxonomy surfaced in per-repo reports (#4788). */ @@ -33,23 +33,16 @@ export const DEFAULT_CROSS_REPO_MANIFEST_RELATIVE_PATH = "benchmarks/cross-repo/ export const MAX_CROSS_REPO_MANIFEST_BYTES = 65_536; export const MAX_CROSS_REPO_MANIFEST_REPOS = 100; -const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; - function cloneEmptyManifest(warnings = []) { return { present: false, manifest: { repos: [] }, warnings }; } -function isPathTraversalSegment(segment) { - return segment === "." || segment === ".."; -} - /** Canonical `owner/repo` with exactly one slash and safe segments; anything else → null. */ export function normalizeCrossRepoFullName(value) { if (typeof value !== "string") return null; const [owner, repo, extra] = value.trim().split("/"); if (!owner || !repo || extra !== undefined) return null; - if (!REPO_SEGMENT_PATTERN.test(owner) || !REPO_SEGMENT_PATTERN.test(repo)) return null; - if (isPathTraversalSegment(owner) || isPathTraversalSegment(repo)) return null; + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null; return `${owner}/${repo}`; } diff --git a/packages/loopover-miner/lib/event-ledger-cli.js b/packages/loopover-miner/lib/event-ledger-cli.js index 1c74c727d9..32ce6a5d71 100644 --- a/packages/loopover-miner/lib/event-ledger-cli.js +++ b/packages/loopover-miner/lib/event-ledger-cli.js @@ -1,5 +1,6 @@ import { initEventLedger } from "./event-ledger.js"; import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js"; +import { isValidRepoSegment } from "./repo-clone.js"; const LEDGER_LIST_USAGE = "Usage: loopover-miner ledger list [--repo ] [--since ] [--type ] [--json]"; @@ -8,7 +9,7 @@ function parseRepoArg(value, usage) { if (!value) return { error: usage }; const trimmed = value.trim(); const [owner, repo, extra] = trimmed.split("/"); - if (!owner || !repo || extra !== undefined) { + if (!owner || !repo || extra !== undefined || !isValidRepoSegment(owner) || !isValidRepoSegment(repo)) { return { error: "Repository must be in owner/repo form." }; } return { repoFullName: `${owner}/${repo}` }; diff --git a/packages/loopover-miner/lib/repo-clone.d.ts b/packages/loopover-miner/lib/repo-clone.d.ts index 744f985372..8701705d91 100644 --- a/packages/loopover-miner/lib/repo-clone.d.ts +++ b/packages/loopover-miner/lib/repo-clone.d.ts @@ -2,6 +2,12 @@ export function resolveRepoCloneBaseDir(env?: Record export function resolveRepoCloneDir(repoFullName: string, env?: Record): string; +export const REPO_SEGMENT_PATTERN: RegExp; + +export function isPathTraversalSegment(segment: string): boolean; + +export function isValidRepoSegment(segment: unknown): boolean; + export type EnsureRepoClonedResult = { ok: boolean; repoPath: string; error?: string }; export type RunGitFn = (args: string[], cwd: string, timeoutMs: number) => Promise<{ ok: boolean; stdout: string; stderr: string }>; diff --git a/packages/loopover-miner/lib/repo-clone.js b/packages/loopover-miner/lib/repo-clone.js index b0fd88f96f..b9fff4b0a6 100644 --- a/packages/loopover-miner/lib/repo-clone.js +++ b/packages/loopover-miner/lib/repo-clone.js @@ -31,12 +31,19 @@ export function resolveRepoCloneBaseDir(env = process.env) { // 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). -const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; +// 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). +export const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; -function isPathTraversalSegment(segment) { +export function isPathTraversalSegment(segment) { return segment === "." || segment === ".."; } +export function isValidRepoSegment(segment) { + return typeof segment === "string" && REPO_SEGMENT_PATTERN.test(segment) && !isPathTraversalSegment(segment); +} + // Reject values that git would interpret as options when passed as argv (e.g. `--upload-pack=...`). function isUnsafeGitArgValue(value) { return typeof value === "string" && value.startsWith("-"); @@ -46,8 +53,7 @@ function normalizeRepoFullName(repoFullName) { 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 (!REPO_SEGMENT_PATTERN.test(owner) || !REPO_SEGMENT_PATTERN.test(repo)) throw new Error("invalid_repo_full_name"); - if (isPathTraversalSegment(owner) || isPathTraversalSegment(repo)) throw new Error("invalid_repo_full_name"); + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name"); return { owner, repo, repoFullName: `${owner}/${repo}` }; } diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index 7d71d9316a..10a023863d 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -165,6 +165,24 @@ describe("parseAttemptArgs (#5132)", () => { }); }); + // #5831: repo-clone.js's path-safety validation (character set + no "."/".." segments) must also gate + // this CLI's own early parser, not just the downstream prepareWorktree -> repo-clone.js call -- for + // both the owner and repo segment independently. + it("rejects a repo target with an unsafe owner or repo segment", () => { + expect(parseAttemptArgs(["../etc", "7", "--miner-login", "alice"])).toEqual({ + error: "Repository must be in owner/repo form: ../etc", + }); + expect(parseAttemptArgs(["owner/..", "7", "--miner-login", "alice"])).toEqual({ + error: "Repository must be in owner/repo form: owner/..", + }); + expect(parseAttemptArgs(["owner baz/repo", "7", "--miner-login", "alice"])).toEqual({ + error: "Repository must be in owner/repo form: owner baz/repo", + }); + expect(parseAttemptArgs(["owner/repo baz", "7", "--miner-login", "alice"])).toEqual({ + error: "Repository must be in owner/repo form: owner/repo baz", + }); + }); + it("rejects a non-positive or non-integer issue number", () => { expect(parseAttemptArgs(["acme/widgets", "0", "--miner-login", "alice"])).toEqual({ error: "Issue number must be a positive integer: 0", diff --git a/test/unit/miner-claim-ledger-cli.test.ts b/test/unit/miner-claim-ledger-cli.test.ts index a7b5ebf1e3..b56bed8592 100644 --- a/test/unit/miner-claim-ledger-cli.test.ts +++ b/test/unit/miner-claim-ledger-cli.test.ts @@ -58,6 +58,21 @@ describe("loopover-miner claim ledger CLI (#4290)", () => { expect(parseClaimClaimArgs(["acme", "42"])).toEqual({ error: "Repository must be in owner/repo form.", }); + // #5831: an unsafe path-traversal/character-set segment must be rejected here too, matching + // repo-clone.js's own validation, instead of being persisted as a claim-ledger key unvalidated -- + // for both the owner and repo segment independently. + expect(parseClaimClaimArgs(["../etc", "42"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseClaimClaimArgs(["acme/..", "42"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseClaimClaimArgs(["acme baz/widgets", "42"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseClaimClaimArgs(["acme/widgets baz", "42"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); expect(parseClaimClaimArgs(["acme/widgets", "0"])).toEqual({ error: "issue number must be a positive integer.", }); diff --git a/test/unit/miner-claim-ledger.test.ts b/test/unit/miner-claim-ledger.test.ts index dd28856de6..99a527e496 100644 --- a/test/unit/miner-claim-ledger.test.ts +++ b/test/unit/miner-claim-ledger.test.ts @@ -131,6 +131,17 @@ describe("loopover-miner claim ledger (#2314)", () => { expect(() => ledger.listClaims({ status: "bogus" as never })).toThrow("invalid_status"); }); + // #5831: 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(); + expect(() => ledger.recordClaim({ repoFullName: "../etc", issueNumber: 1 })).toThrow("invalid_repo_full_name"); + expect(() => ledger.recordClaim({ repoFullName: "o/..", issueNumber: 1 })).toThrow("invalid_repo_full_name"); + expect(() => ledger.recordClaim({ repoFullName: "o baz/a", issueNumber: 1 })).toThrow("invalid_repo_full_name"); + expect(() => ledger.recordClaim({ repoFullName: "o/a baz", issueNumber: 1 })).toThrow("invalid_repo_full_name"); + }); + it("claim-then-list, then release, excludes released rows from the active-only filter (#3354)", () => { const ledger = tempLedger(); ledger.recordClaim({ repoFullName: "o/a", issueNumber: 10 }); diff --git a/test/unit/miner-cross-repo-evaluation.test.ts b/test/unit/miner-cross-repo-evaluation.test.ts index 3ad84f3673..7cf8a26563 100644 --- a/test/unit/miner-cross-repo-evaluation.test.ts +++ b/test/unit/miner-cross-repo-evaluation.test.ts @@ -55,6 +55,16 @@ describe("cross-repo evaluation harness (#4788)", () => { expect(normalizeCrossRepoFullName("../evil/repo")).toBeNull(); expect(normalizeCrossRepoFullName(12)).toBeNull(); }); + + // #5831: this file's own copy of the path-safety check now comes from repo-clone.js's shared + // isValidRepoSegment -- exercise a traversal/invalid-character segment in both the owner and repo + // position (a "one slash" value, unlike "../evil/repo" above which is rejected earlier for having two). + it("rejects an unsafe owner or repo segment even with exactly one slash", () => { + expect(normalizeCrossRepoFullName("../foo")).toBeNull(); + expect(normalizeCrossRepoFullName("foo/..")).toBeNull(); + expect(normalizeCrossRepoFullName("ac me/widgets")).toBeNull(); + expect(normalizeCrossRepoFullName("acme/wid gets")).toBeNull(); + }); }); describe("parseCrossRepoEvaluationManifest", () => { diff --git a/test/unit/miner-event-ledger-cli.test.ts b/test/unit/miner-event-ledger-cli.test.ts index 6348a1480a..f591fd0880 100644 --- a/test/unit/miner-event-ledger-cli.test.ts +++ b/test/unit/miner-event-ledger-cli.test.ts @@ -66,6 +66,26 @@ describe("loopover-miner event ledger CLI (#2290)", () => { }); }); + // #5831: --repo's own parser must reject the same class of malformed/unsafe identifier repo-clone.js + // already rejects, not just "missing slash" -- for both the owner and repo segment independently. + it("rejects an unsafe --repo value", () => { + expect(parseLedgerListArgs(["--repo", "acme"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseLedgerListArgs(["--repo", "../etc"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseLedgerListArgs(["--repo", "acme/.."])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseLedgerListArgs(["--repo", "acme baz/widgets"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + expect(parseLedgerListArgs(["--repo", "acme/widgets baz"])).toEqual({ + error: "Repository must be in owner/repo form.", + }); + }); + it("filterLedgerEvents and renderLedgerTable format rows", () => { const events: LedgerEntry[] = [ {