Skip to content

Commit 434aada

Browse files
RealDiligentRealDiligent
andauthored
fix(engine): reject path-traversal repo segments on the governor-ledger write path (#8469)
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 Co-authored-by: RealDiligent <nft.gold.eth@gmail.com>
1 parent 5d34f85 commit 434aada

3 files changed

Lines changed: 43 additions & 0 deletions

File tree

packages/loopover-engine/src/governor-ledger.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,25 @@ function normalizeRequiredString(value: unknown, code: string): string {
3636
return trimmed;
3737
}
3838

39+
// #5831/#7525's path-safety guard, restated locally. The miner package's parsers share
40+
// repo-clone.ts's isValidRepoSegment, but this engine package must not import from the miner package
41+
// (miner depends on engine, not the reverse), so the semantics are duplicated here deliberately:
42+
// a segment must be entirely [A-Za-z0-9._-] and must not be a bare "." or ".." traversal segment.
43+
const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/;
44+
45+
function isValidRepoSegment(segment: string): boolean {
46+
return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== "..";
47+
}
48+
3949
function normalizeOptionalRepoFullName(repoFullName: unknown): string | null {
4050
if (repoFullName === undefined || repoFullName === null) return null;
4151
if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
4252
const [owner, repo, extra] = repoFullName.trim().split("/");
4353
if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
54+
// This is the WRITE path (normalizeGovernorLedgerEvent -> appendGovernorEvent's SQLite INSERT). Without
55+
// this, "../evilrepo" normalized unchanged -- owner ".." and repo "evilrepo" both pass the
56+
// non-empty/one-slash check -- and reached persistence, the exact value class #7525 exists to stop.
57+
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
4458
return `${owner}/${repo}`;
4559
}
4660

1.83 KB
Binary file not shown.

test/unit/governor-ledger.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,35 @@ describe("governor ledger normalization (#2328)", () => {
6060
).toThrow(/invalid_event_type/);
6161
});
6262

63+
it("REGRESSION (#8350): rejects a path-traversal or invalid-character repo segment on the WRITE path", () => {
64+
// normalizeGovernorLedgerEvent backs appendGovernorEvent's SQLite INSERT. It only checked "exactly two
65+
// non-empty segments", so "../evilrepo" normalized unchanged (owner "..", repo "evilrepo") and reached
66+
// persistence -- the value class #5831/#7525 already guard against in every miner-lib sibling parser.
67+
const base = {
68+
eventType: "denied",
69+
actionClass: "open_pr",
70+
decision: "deny",
71+
reason: "x",
72+
};
73+
for (const repoFullName of [
74+
"../evilrepo", // traversal owner
75+
"acme/..", // traversal repo
76+
"./acme", // bare-dot owner
77+
"acme/.", // bare-dot repo
78+
"ac me/widgets", // space -> outside [A-Za-z0-9._-]
79+
"acme/wid;gets", // shell metacharacter
80+
"acme/wid\u0000gets", // control character
81+
]) {
82+
expect(() => normalizeGovernorLedgerEvent({ ...base, repoFullName }), repoFullName).toThrow(
83+
/invalid_repo_full_name/,
84+
);
85+
}
86+
// Legitimate slugs (including the dots/dashes/underscores real repos use) still normalize.
87+
for (const repoFullName of ["acme/widgets", "acme-co/my_widget.js", "a/b"]) {
88+
expect(normalizeGovernorLedgerEvent({ ...base, repoFullName }).repoFullName).toBe(repoFullName);
89+
}
90+
});
91+
6392
it("rejects malformed repo slugs, blank required strings, and lossy payloads", () => {
6493
const base = {
6594
eventType: "throttled",

0 commit comments

Comments
 (0)