Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/loopover-miner/lib/attempt-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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}`;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/loopover-miner/lib/claim-ledger-cli.js
Original file line number Diff line number Diff line change
@@ -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 <owner/repo> <issue#> [--note <text>] [--api-base-url <url>] [--dry-run] [--json]";
Expand All @@ -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}` };
Expand Down
2 changes: 2 additions & 0 deletions packages/loopover-miner/lib/claim-ledger.js
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -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}`;
}

Expand Down
11 changes: 2 additions & 9 deletions packages/loopover-miner/lib/cross-repo-evaluation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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). */
Expand All @@ -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}`;
}

Expand Down
3 changes: 2 additions & 1 deletion packages/loopover-miner/lib/event-ledger-cli.js
Original file line number Diff line number Diff line change
@@ -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 <owner/repo>] [--since <seq>] [--type <eventType>] [--json]";
Expand All @@ -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}` };
Expand Down
6 changes: 6 additions & 0 deletions packages/loopover-miner/lib/repo-clone.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ export function resolveRepoCloneBaseDir(env?: Record<string, string | undefined>

export function resolveRepoCloneDir(repoFullName: string, env?: Record<string, string | undefined>): 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 }>;
Expand Down
14 changes: 10 additions & 4 deletions packages/loopover-miner/lib/repo-clone.js
Original file line number Diff line number Diff line change
Expand Up @@ -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("-");
Expand All @@ -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}` };
}

Expand Down
18 changes: 18 additions & 0 deletions test/unit/miner-attempt-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
15 changes: 15 additions & 0 deletions test/unit/miner-claim-ledger-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
});
Expand Down
11 changes: 11 additions & 0 deletions test/unit/miner-claim-ledger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
10 changes: 10 additions & 0 deletions test/unit/miner-cross-repo-evaluation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
20 changes: 20 additions & 0 deletions test/unit/miner-event-ledger-cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [
{
Expand Down