diff --git a/packages/loopover-engine/src/discovery-index-contract.ts b/packages/loopover-engine/src/discovery-index-contract.ts index b3d15cb28..6a0b8f06a 100644 --- a/packages/loopover-engine/src/discovery-index-contract.ts +++ b/packages/loopover-engine/src/discovery-index-contract.ts @@ -11,6 +11,8 @@ // miner-goal-spec.ts / fleet-run-manifest.ts: every field optional, malformed input degrades to a documented // default with a warning rather than throwing. +import { isValidRepoSegment } from "./repo-segment.js"; + export const DISCOVERY_INDEX_CONTRACT_VERSION = 1; const MAX_QUERY_ITEMS = 200; @@ -124,10 +126,12 @@ function normalizeStringList(value: unknown, transform: (entry: string) => strin return result; } -/** `owner/repo` with exactly one slash and non-empty halves; anything else → null (mirrors normalizeCandidate). */ +/** `owner/repo` with exactly one slash and non-empty, path-safe halves — a "." / ".." traversal segment + * fails the shared isValidRepoSegment guard; anything else → null (mirrors normalizeCandidate). */ function normalizeRepoFullName(value: string): string | null { const [owner, repo, extra] = value.trim().split("/"); if (!owner || !repo || extra !== undefined) return null; + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null; return `${owner}/${repo}`; } diff --git a/packages/loopover-engine/src/discovery-soft-claim.ts b/packages/loopover-engine/src/discovery-soft-claim.ts index 67527b396..94627171a 100644 --- a/packages/loopover-engine/src/discovery-soft-claim.ts +++ b/packages/loopover-engine/src/discovery-soft-claim.ts @@ -1,4 +1,5 @@ import { DISCOVERY_INDEX_CONTRACT_VERSION } from "./discovery-index-contract.js"; +import { isValidRepoSegment } from "./repo-segment.js"; // Soft-claim coordination request builder (#4302). The local soft-claim ledger (claim-ledger.js) is 100% // client-side — "never uploads, syncs, or phones home" — and duplicate-cluster adjudication @@ -53,12 +54,14 @@ export function softClaimActionForStatus(status: SoftClaimStatus): SoftClaimActi return status === "active" ? "claim" : "release"; } -/** `owner/repo` with exactly one slash and non-empty halves; anything else → null (mirrors the discovery-index - * contract / claim-ledger repo validation). */ +/** `owner/repo` with exactly one slash and non-empty, path-safe halves; anything else → null (mirrors the + * discovery-index contract / claim-ledger repo validation, including their shared "." / ".." traversal- + * segment rejection). */ function normalizeRepoFullName(value: unknown): string | null { if (typeof value !== "string") return null; const [owner, repo, extra] = value.trim().split("/"); if (!owner || !repo || extra !== undefined) return null; + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return null; return `${owner}/${repo}`; } diff --git a/packages/loopover-engine/src/governor-ledger.ts b/packages/loopover-engine/src/governor-ledger.ts index b2fa15260..266b170d0 100644 --- a/packages/loopover-engine/src/governor-ledger.ts +++ b/packages/loopover-engine/src/governor-ledger.ts @@ -1,3 +1,5 @@ +import { isValidRepoSegment } from "./repo-segment.js"; + /** Immutable governor decision vocabulary — unknown values fail closed before insert. */ export const GOVERNOR_LEDGER_EVENT_TYPES = Object.freeze([ "allowed", @@ -36,16 +38,6 @@ function normalizeRequiredString(value: unknown, code: string): string { return trimmed; } -// #5831/#7525's path-safety guard, restated locally. The miner package's parsers share -// repo-clone.ts's isValidRepoSegment, but this engine package must not import from the miner package -// (miner depends on engine, not the reverse), so the semantics are duplicated here deliberately: -// a segment must be entirely [A-Za-z0-9._-] and must not be a bare "." or ".." traversal segment. -const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; - -function isValidRepoSegment(segment: string): boolean { - return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== ".."; -} - function normalizeOptionalRepoFullName(repoFullName: unknown): string | null { if (repoFullName === undefined || repoFullName === null) return null; if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name"); diff --git a/packages/loopover-engine/src/idea-intake.ts b/packages/loopover-engine/src/idea-intake.ts index 644b902b6..2b6013dd6 100644 --- a/packages/loopover-engine/src/idea-intake.ts +++ b/packages/loopover-engine/src/idea-intake.ts @@ -10,6 +10,7 @@ import { type FeasibilityGateInput, type FeasibilityVerdict, } from "./feasibility.js"; +import { isValidRepoSegment } from "./repo-segment.js"; // Intake bounds — mirror the manifest text-slot handling (focus-manifest.ts): a renter's freeform text is // length-capped so one submission can never dominate a public surface. @@ -101,8 +102,14 @@ export function validateIdeaSubmission(raw: unknown): IdeaValidationResult { // `go`). A `{ kind: "provision" }` object requests a not-yet-created repo (#7589). Anything else is missing. let resolvedTarget: IdeaTarget | undefined; if (isNonEmptyString(input.targetRepo)) { - if (/^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/.test(input.targetRepo)) resolvedTarget = { kind: "existing", repo: input.targetRepo }; - else errors.push("target_repo_malformed"); + // Same split-and-guard shape as repo-clone.ts's normalizeRepoFullName: exactly two segments, each a + // valid repo segment, so a bare "." / ".." traversal segment is rejected at intake too. + const [owner, repo, extra] = input.targetRepo.split("/"); + if (!owner || !repo || extra !== undefined || !isValidRepoSegment(owner) || !isValidRepoSegment(repo)) { + errors.push("target_repo_malformed"); + } else { + resolvedTarget = { kind: "existing", repo: input.targetRepo }; + } } else if (typeof input.targetRepo === "object" && input.targetRepo !== null && (input.targetRepo as Record).kind === "provision") { resolvedTarget = { kind: "provision" }; } else errors.push("target_repo_required"); diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 5989875d6..1f608c2ac 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -193,6 +193,7 @@ export { type GovernorLedgerEventType, type NormalizedGovernorLedgerEvent, } from "./governor-ledger.js"; +export { REPO_SEGMENT_PATTERN, isValidRepoSegment } from "./repo-segment.js"; export { MINER_TELEMETRY_EVENT_TYPES, MINER_TELEMETRY_OUTCOME_BUCKETS, diff --git a/packages/loopover-engine/src/repo-segment.ts b/packages/loopover-engine/src/repo-segment.ts new file mode 100644 index 000000000..2c08b3e8e --- /dev/null +++ b/packages/loopover-engine/src/repo-segment.ts @@ -0,0 +1,11 @@ +// Shared repo-segment path-safety guard (the #5831 -> #7525 -> #8350 fix family). The miner package's +// parsers share packages/loopover-miner/lib/repo-clone.ts's isValidRepoSegment, but this engine package +// must not import from the miner package (miner depends on engine, not the reverse), so the engine keeps +// its own single copy here: a segment must be entirely [A-Za-z0-9._-] and must not be a bare "." or ".." +// traversal segment. + +export const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; + +export function isValidRepoSegment(segment: string): boolean { + return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== ".."; +} diff --git a/packages/loopover-engine/test/repo-segment.test.ts b/packages/loopover-engine/test/repo-segment.test.ts new file mode 100755 index 000000000..07fcaf925 --- /dev/null +++ b/packages/loopover-engine/test/repo-segment.test.ts @@ -0,0 +1,64 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { + REPO_SEGMENT_PATTERN, + buildSoftClaimRequest, + isValidRepoSegment, + normalizeDiscoveryIndexCandidate, + normalizeDiscoveryIndexResponse, + validateIdeaSubmission, +} from "../dist/index.js"; + +// #9610: the "."/".." path-safety segment guard from the #5831 -> #7525 -> #8350 family now lives in one +// shared module (repo-segment.ts) and is applied by every engine-package owner/repo normalizer, not just +// governor-ledger's write path (whose own traversal tests live in governor-ledger.test.ts). + +test("repo-segment: isValidRepoSegment accepts GitHub-legal slugs and rejects traversal/non-slug segments (#9610)", () => { + for (const segment of ["acme", "a.b_c-d9", "...three-dots-are-a-slug..."]) { + assert.equal(isValidRepoSegment(segment), true, segment); + assert.equal(REPO_SEGMENT_PATTERN.test(segment), true, segment); + } + for (const segment of ["evil repo", "", "a/b", ".", ".."]) { + assert.equal(isValidRepoSegment(segment), false, segment); + } +}); + +test("idea-intake: a '.'/'..' traversal segment in a bare-string targetRepo is rejected at intake (#9610)", () => { + const idea = { id: "idea-1", title: "t", body: "b" }; + for (const targetRepo of ["../evilrepo", "./x", "a/..", "../..", "/x", "x/", "a/b/c", "no-slash", "evil repo/x"]) { + assert.deepEqual( + validateIdeaSubmission({ ...idea, targetRepo }), + { ok: false, errors: ["target_repo_malformed"] }, + targetRepo, + ); + } + const accepted = validateIdeaSubmission({ ...idea, targetRepo: "acme/widgets" }); + assert.equal(accepted.ok, true); + if (accepted.ok) assert.deepEqual(accepted.idea.targetRepo, { kind: "existing", repo: "acme/widgets" }); +}); + +test("discovery-index contract: a traversal-segment repoFullName fails candidate normalization (#9610)", () => { + for (const repoFullName of ["../evil", "evil/..", "./evil"]) { + assert.equal(normalizeDiscoveryIndexCandidate({ repoFullName, issueNumber: 1, title: "x" }), null, repoFullName); + } + const valid = normalizeDiscoveryIndexCandidate({ repoFullName: "owner/repo", issueNumber: 1, title: "x" }); + assert.equal(valid?.repoFullName, "owner/repo"); + + const parsed = normalizeDiscoveryIndexResponse({ + candidates: [ + { repoFullName: "../evil", issueNumber: 1, title: "x" }, + { repoFullName: "owner/repo", issueNumber: 2, title: "y" }, + ], + }); + assert.deepEqual(parsed.response.candidates.map((candidate) => candidate.repoFullName), ["owner/repo"]); + assert.ok(parsed.warnings.includes("DiscoveryIndexResponse dropped an invalid or boundary-violating candidate.")); +}); + +test("discovery soft-claim: a traversal-segment repoFullName never becomes a soft-claim request (#9610)", () => { + const claim = { id: 7, repoFullName: "owner/repo", issueNumber: 42, claimedAt: "2026-01-01T00:00:00Z", status: "active" as const }; + for (const repoFullName of ["../evil", "evil/..", "./evil"]) { + assert.equal(buildSoftClaimRequest({ ...claim, repoFullName }), null, repoFullName); + } + assert.equal(buildSoftClaimRequest(claim)?.repoFullName, "owner/repo"); +}); diff --git a/test/unit/discovery-index-contract.test.ts b/test/unit/discovery-index-contract.test.ts index 5c48c4d13..cabf91f61 100644 --- a/test/unit/discovery-index-contract.test.ts +++ b/test/unit/discovery-index-contract.test.ts @@ -196,3 +196,26 @@ describe("discovery-index API contract (#4300)", () => { }); }); }); + +describe("repo-segment path safety in candidate normalization (#9610)", () => { + it("returns null for a candidate whose repoFullName carries a '.'/'..' traversal segment", () => { + expect(normalizeDiscoveryIndexCandidate({ repoFullName: "../evil", issueNumber: 1, title: "x" })).toBeNull(); + expect(normalizeDiscoveryIndexCandidate({ repoFullName: "evil/..", issueNumber: 1, title: "x" })).toBeNull(); + expect(normalizeDiscoveryIndexCandidate({ repoFullName: "./evil", issueNumber: 1, title: "x" })).toBeNull(); + }); + + it("drops a traversal-segment candidate from a response with the existing invalid-candidate warning", () => { + const parsed = normalizeDiscoveryIndexResponse({ + candidates: [{ repoFullName: "../evil", issueNumber: 1, title: "x" }, VALID_CANDIDATE], + }); + expect(parsed.response.candidates.map((candidate) => candidate.repoFullName)).toEqual(["owner/repo"]); + expect(parsed.warnings).toContain("DiscoveryIndexResponse dropped an invalid or boundary-violating candidate."); + }); + + it("still round-trips a valid owner/repo unchanged through candidate normalization", () => { + const normalized = normalizeDiscoveryIndexCandidate(VALID_CANDIDATE); + expect(normalized?.repoFullName).toBe("owner/repo"); + expect(normalized?.owner).toBe("owner"); + expect(normalized?.repo).toBe("repo"); + }); +}); diff --git a/test/unit/discovery-soft-claim.test.ts b/test/unit/discovery-soft-claim.test.ts index f4444f030..d4901eae7 100644 --- a/test/unit/discovery-soft-claim.test.ts +++ b/test/unit/discovery-soft-claim.test.ts @@ -85,3 +85,15 @@ describe("soft-claim coordination request builder (#4302)", () => { } }); }); + +describe("repo-segment path safety in soft-claim requests (#9610)", () => { + it("returns null for a claim whose repoFullName carries a '.'/'..' traversal segment", () => { + expect(buildSoftClaimRequest({ ...ACTIVE_CLAIM, repoFullName: "../evil" })).toBeNull(); + expect(buildSoftClaimRequest({ ...ACTIVE_CLAIM, repoFullName: "evil/.." })).toBeNull(); + expect(buildSoftClaimRequest({ ...ACTIVE_CLAIM, repoFullName: "./evil" })).toBeNull(); + }); + + it("still round-trips a valid owner/repo unchanged", () => { + expect(buildSoftClaimRequest(ACTIVE_CLAIM)?.repoFullName).toBe("owner/repo"); + }); +}); diff --git a/test/unit/idea-intake-bridge.test.ts b/test/unit/idea-intake-bridge.test.ts index 4d5d84f37..86609c8df 100644 --- a/test/unit/idea-intake-bridge.test.ts +++ b/test/unit/idea-intake-bridge.test.ts @@ -320,3 +320,23 @@ describe("buildClaimPlan — routes a scored task-graph into a loop claim plan ( expect(plan.graphVerdict).toBe("avoid"); // least-favorable across the graph }); }); + +describe("validateIdeaSubmission targetRepo path safety (#9610)", () => { + it("rejects a '.'/'..' traversal segment in the bare-string targetRepo with target_repo_malformed", () => { + for (const targetRepo of ["../evilrepo", "./x", "a/..", "../.."]) { + expect(validateIdeaSubmission(rawIdea({ targetRepo }))).toEqual({ ok: false, errors: ["target_repo_malformed"] }); + } + }); + + it("still rejects empty halves, extra segments, and non-slug characters", () => { + for (const targetRepo of ["/x", "x/", "a/b/c", "no-slash", "evil repo/x", "acme/widg!ts"]) { + expect(validateIdeaSubmission(rawIdea({ targetRepo }))).toEqual({ ok: false, errors: ["target_repo_malformed"] }); + } + }); + + it("still accepts a well-formed owner/name and resolves it as an existing-repo target", () => { + const r = validateIdeaSubmission(rawIdea({ targetRepo: "acme/widgets" })); + expect(r.ok).toBe(true); + if (r.ok) expect(r.idea.targetRepo).toEqual({ kind: "existing", repo: "acme/widgets" }); + }); +}); diff --git a/test/unit/repo-segment.test.ts b/test/unit/repo-segment.test.ts new file mode 100644 index 000000000..35a692217 --- /dev/null +++ b/test/unit/repo-segment.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from "vitest"; + +import { REPO_SEGMENT_PATTERN, isValidRepoSegment } from "../../packages/loopover-engine/src/index"; + +describe("shared repo-segment path-safety guard (#9610)", () => { + it("re-exports the guard from the engine barrel", () => { + expect(REPO_SEGMENT_PATTERN).toBeInstanceOf(RegExp); + expect(typeof isValidRepoSegment).toBe("function"); + }); + + it("accepts GitHub-legal slug segments", () => { + expect(isValidRepoSegment("acme")).toBe(true); + expect(isValidRepoSegment("a.b_c-d9")).toBe(true); + expect(isValidRepoSegment("...three-dots-are-a-slug...")).toBe(true); + }); + + it("rejects a segment outside [A-Za-z0-9._-]", () => { + expect(isValidRepoSegment("evil repo")).toBe(false); + expect(isValidRepoSegment("")).toBe(false); + expect(isValidRepoSegment("a/b")).toBe(false); + }); + + it("rejects the bare '.' and '..' traversal segments", () => { + expect(isValidRepoSegment(".")).toBe(false); + expect(isValidRepoSegment("..")).toBe(false); + }); +});