diff --git a/packages/loopover-engine/src/governor-ledger.ts b/packages/loopover-engine/src/governor-ledger.ts index 374542a26b..b2fa15260d 100644 --- a/packages/loopover-engine/src/governor-ledger.ts +++ b/packages/loopover-engine/src/governor-ledger.ts @@ -36,11 +36,25 @@ 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"); const [owner, repo, extra] = repoFullName.trim().split("/"); if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name"); + // This is the WRITE path (normalizeGovernorLedgerEvent -> appendGovernorEvent's SQLite INSERT). Without + // this, "../evilrepo" normalized unchanged -- owner ".." and repo "evilrepo" both pass the + // non-empty/one-slash check -- and reached persistence, the exact value class #7525 exists to stop. + if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name"); return `${owner}/${repo}`; } diff --git a/packages/loopover-engine/test/governor-ledger.test.ts b/packages/loopover-engine/test/governor-ledger.test.ts new file mode 100644 index 0000000000..fc13416307 Binary files /dev/null and b/packages/loopover-engine/test/governor-ledger.test.ts differ diff --git a/test/unit/governor-ledger.test.ts b/test/unit/governor-ledger.test.ts index 7ad1d55ff7..c7f3dc9f90 100644 --- a/test/unit/governor-ledger.test.ts +++ b/test/unit/governor-ledger.test.ts @@ -60,6 +60,35 @@ describe("governor ledger normalization (#2328)", () => { ).toThrow(/invalid_event_type/); }); + it("REGRESSION (#8350): rejects a path-traversal or invalid-character repo segment on the WRITE path", () => { + // normalizeGovernorLedgerEvent backs appendGovernorEvent's SQLite INSERT. It only checked "exactly two + // non-empty segments", so "../evilrepo" normalized unchanged (owner "..", repo "evilrepo") and reached + // persistence -- the value class #5831/#7525 already guard against in every miner-lib sibling parser. + const base = { + eventType: "denied", + actionClass: "open_pr", + decision: "deny", + reason: "x", + }; + for (const repoFullName of [ + "../evilrepo", // traversal owner + "acme/..", // traversal repo + "./acme", // bare-dot owner + "acme/.", // bare-dot repo + "ac me/widgets", // space -> outside [A-Za-z0-9._-] + "acme/wid;gets", // shell metacharacter + "acme/wid\u0000gets", // control character + ]) { + expect(() => normalizeGovernorLedgerEvent({ ...base, repoFullName }), repoFullName).toThrow( + /invalid_repo_full_name/, + ); + } + // Legitimate slugs (including the dots/dashes/underscores real repos use) still normalize. + for (const repoFullName of ["acme/widgets", "acme-co/my_widget.js", "a/b"]) { + expect(normalizeGovernorLedgerEvent({ ...base, repoFullName }).repoFullName).toBe(repoFullName); + } + }); + it("rejects malformed repo slugs, blank required strings, and lossy payloads", () => { const base = { eventType: "throttled",