From 4c0859bff4152147db6fa376e40e23d87519a58c Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 22:22:32 +0800 Subject: [PATCH] fix(engine): reject path-traversal repo segments on the governor-ledger write path normalizeOptionalRepoFullName in the engine only checked for exactly two non-empty segments, so "../evilrepo" normalized unchanged (owner "..", repo "evilrepo") and was persisted by appendGovernorEvent's SQLite INSERT. The miner-lib siblings already reject this class via repo-clone.ts's isValidRepoSegment (#5831/#7525/#7795), and miner-lib's own guarded copy covers its read/purge paths -- but the write path delegates to this engine function, which never got the guard. Restates the guard locally (the engine must not import from the miner package, which depends on it) with isValidRepoSegment's exact semantics: /^[A-Za-z0-9._-]+$/ plus rejecting a bare "."/".." segment. Signature, return type, and the null-passthrough are unchanged. Closes #8350 --- .../loopover-engine/src/governor-ledger.ts | 14 +++++++++ .../test/governor-ledger.test.ts | Bin 0 -> 1878 bytes test/unit/governor-ledger.test.ts | 29 ++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100644 packages/loopover-engine/test/governor-ledger.test.ts 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 0000000000000000000000000000000000000000..fc134163077b8c4bac230cabe258303be60d9bab GIT binary patch literal 1878 zcmdUv!D<^Z5Qcm9Qw-Zf?c&`HltLSmLMdqpghCV3LPIdJG_jRvrL82}af08yzjos| zaod1%>&0u0Mw*##{*kU~=Oc*_7@rI8D$1-=yx&^N5WSW$8+3H*cuSE(q}4;$WP!)V z#X4UJqi^_&o3qF2nAL>+;hgQ}Xi=0(hgfQ>_`0|Z=?3X6E+31)bU?QqB2Gt!%bNEo zQ*1S>47pO{*T=q)Q9Jv|AX|92n(e6eS=v!7b--SGaM{>Ai_loEv0NcuS<=gQZ{F_h z(~~LdKyk|S~NVSMl>Q>sa&y60x3M6X8RqI;YDis@^2#}%ZNQ12NDVmOfPQM-N z$aM7e^zdZZ>yxl5c{pp9B&RH|SW(8;C=G_VaLCQ(T-9+&fhQG!gpBf>f+IoE3m^`{ ztoO+1D^8&@0Oq+io@Qgkcaw_ZuQBvduaMrj1tzcEq5Ovo z=S!9mG=M}V>NM9PMN3g~X`JOE1~Ad#I~1T!U_Nb=9yAQuPC$;Nu+~Mo6IGmwc!v~y zOE3d)Q0v=A?kQqZl!qXEU{FK#sV6~H_~A;}s3r3Bj8sAdc}2!aVZbnzS!-)ynzHAj zlLXs^%%4zcTPV<=+xZ4^DsxRbP`43JFY8mcM+YAo#iG&FZFQWSKhKQ$nsRg(boQtv zeaeKaxGV~s&x$fF(!bW2o%fpSAn|52i#YXe5xQ-lr&90UQc)CXhWbrc?lV|{8m4WY z`Z7wnjV_Sf$RfM)v1@OD0a|rd`0m2$F`R|QoIq(FLPpQX&0^4s>Fi_vQ{*>$`TJs& zpYP64NJ7F3jTZ%jum*A`zuAprSW|pQB!cIeA z(MKpXgd~#L1kcm1!fZsh#Wa_$td^tJnjYxHx*Y$ZO#U?!ru$4u77vHyuPVHsfak}- Z^PUK;dRx(wwbA { ).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",