From 681d7d9b551bd9e9886d125921db566848984e37 Mon Sep 17 00:00:00 2001 From: tryeverything24 <114252040+tryeverything24@users.noreply.github.com> Date: Wed, 29 Jul 2026 03:32:13 +0300 Subject: [PATCH] fix(engine): validate repoFullName and prNumber before composing the customer-facing results payload buildResultsPayload interpolated caller-supplied repoFullName and prNumber into the customer-facing prLink and summary with no validation, so a traversal-shaped repo like acme/widgets/../../evil produced a link a browser resolves to an unrelated GitHub path, and prNumber 0 or -3 took the has-PR branch. Now the repo must split into exactly two valid segments (the shared isValidRepoSegment rule, restated locally like governor-ledger.ts) or the summary renders the literal 'unknown repository' and prLink stays null; hasPr additionally requires a positive integer; and per-file additions/deletions are normalized with the sibling modules' finiteNonNegativeInt rule before they reach diffPreview and totals. Payload is still always returned (never throws); the preview cap, status default, and redactSecrets scrub are unchanged. Closes #9611 --- .../loopover-engine/src/results-payload.ts | 42 +++++++++-- test/unit/results-payload.test.ts | 72 +++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/packages/loopover-engine/src/results-payload.ts b/packages/loopover-engine/src/results-payload.ts index 15e2d1765e..bfe2bbacf1 100644 --- a/packages/loopover-engine/src/results-payload.ts +++ b/packages/loopover-engine/src/results-payload.ts @@ -28,6 +28,30 @@ export type IterationResult = { export type DiffPreviewFile = { path: string; additions: number; deletions: number }; +// #5831/#7525's path-safety guard, restated locally (same deliberate duplication as governor-ledger.ts and +// miner/deny-hook-synthesis.ts: this engine package must not import the miner package's repo-clone.ts): a +// segment must be entirely [A-Za-z0-9._-] and must not be a bare "." or ".." traversal segment. Without it, +// a caller-supplied repoFullName like "acme/widgets/../../evil" interpolates into a customer-facing prLink +// that a browser resolves to an unrelated GitHub path (#9611). +const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; + +function isValidRepoSegment(segment: string): boolean { + return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== ".."; +} + +/** True only when `repoFullName` splits on `/` into exactly two valid repo segments. */ +function isValidRepoFullName(repoFullName: string): boolean { + const segments = repoFullName.split("/"); + return segments.length === 2 && segments.every(isValidRepoSegment); +} + +// Normalize any numeric input to a non-negative integer (a non-finite or negative value becomes 0) — the +// same rule the sibling Rent-a-Loop modules apply (loop-consumption.ts / tenant-quota.ts), so a +// caller-supplied negative or fractional count can never reach the rendered diff preview or totals (#9611). +function finiteNonNegativeInt(value: number): number { + return Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0; +} + export type ResultsPayload = { /** Canonical PR URL, or null when no PR was opened. */ prLink: string | null; @@ -43,19 +67,27 @@ export type ResultsPayload = { export function buildResultsPayload(result: IterationResult): ResultsPayload { const normalized: DiffPreviewFile[] = (result.changedFiles ?? []).map((f) => ({ path: f.path, - additions: f.additions ?? 0, - deletions: f.deletions ?? 0, + additions: finiteNonNegativeInt(f.additions ?? 0), + deletions: finiteNonNegativeInt(f.deletions ?? 0), })); const totals = normalized.reduce( (acc, f) => ({ files: acc.files + 1, additions: acc.additions + f.additions, deletions: acc.deletions + f.deletions }), { files: 0, additions: 0, deletions: 0 }, ); - const hasPr = result.prNumber !== null && result.prNumber !== undefined; - const prLink = hasPr ? `https://github.com/${result.repoFullName}/pull/${result.prNumber}` : null; + // Both values interpolated into the customer-facing link get validated (#9611): the repo must be exactly + // two valid segments, and the pull number must be a positive integer (same rule as + // parse-pull-request-target-key.ts). An invalid repo renders as the literal "unknown repository" and never + // reaches prLink; a non-positive/non-integer prNumber takes the no-PR branch. Pure contract preserved: + // the payload is still returned, never thrown. + const repoValid = isValidRepoFullName(result.repoFullName); + const repoRef = repoValid ? result.repoFullName : "unknown repository"; + const hasPr = + result.prNumber !== null && result.prNumber !== undefined && Number.isInteger(result.prNumber) && result.prNumber > 0; + const prLink = hasPr && repoValid ? `https://github.com/${result.repoFullName}/pull/${result.prNumber}` : null; const status: LoopResultStatus = result.status ?? "open"; - const prPart = hasPr ? `Opened PR #${result.prNumber} in ${result.repoFullName}` : `No pull request was opened for ${result.repoFullName}`; + const prPart = hasPr ? `Opened PR #${result.prNumber} in ${repoRef}` : `No pull request was opened for ${repoRef}`; const changePart = totals.files === 0 ? "no file changes" diff --git a/test/unit/results-payload.test.ts b/test/unit/results-payload.test.ts index a0766df17e..2360cc1798 100644 --- a/test/unit/results-payload.test.ts +++ b/test/unit/results-payload.test.ts @@ -74,3 +74,75 @@ describe("buildResultsPayload — packages a completed loop iteration (#4801)", expect(p.summary).toBe(`Opened PR #12 in acme/widgets: ${title}. no file changes. Status: open.`); }); }); + +describe("buildResultsPayload — validates link inputs and normalizes counts (#9611)", () => { + it("regression: a traversal-shaped repoFullName never becomes a customer-facing prLink", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets/../../evil", prNumber: 1, title: "t" }); + expect(p.prLink).toBeNull(); + expect(p.summary).toContain("unknown repository"); + expect(p.summary).not.toContain("../"); + }); + + it("renders 'unknown repository' for a malformed repoFullName while still returning the payload", () => { + for (const repoFullName of ["no-slash", "acme/", "/widgets", "acme/wid gets", "./widgets", "acme/..", "../widgets"]) { + const p = buildResultsPayload({ repoFullName, prNumber: 5, title: "t" }); + expect(p.prLink).toBeNull(); + expect(p.summary).toBe("Opened PR #5 in unknown repository: t. no file changes. Status: open."); + } + }); + + it("regression: prNumber 0 takes the no-PR branch", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 0, title: "t" }); + expect(p.prLink).toBeNull(); + expect(p.summary).toBe("No pull request was opened for acme/widgets: t. no file changes. Status: open."); + }); + + it("treats a negative or non-integer prNumber as no PR", () => { + for (const prNumber of [-3, 2.5, Number.NaN]) { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber, title: "t" }); + expect(p.prLink).toBeNull(); + expect(p.summary).toBe("No pull request was opened for acme/widgets: t. no file changes. Status: open."); + } + }); + + it("an invalid repoFullName with a valid prNumber still yields prLink === null", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets/../../evil", prNumber: 42, title: "t" }); + expect(p.prLink).toBeNull(); + expect(p.summary).toContain("Opened PR #42 in unknown repository"); + }); + + it("a valid repoFullName with an invalid prNumber still renders the repo name in the summary", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber: -1, title: "t" }); + expect(p.prLink).toBeNull(); + expect(p.summary).toContain("No pull request was opened for acme/widgets"); + }); + + it("a valid repoFullName and positive-integer prNumber still produce the canonical link", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 42, title: "t" }); + expect(p.prLink).toBe("https://github.com/acme/widgets/pull/42"); + expect(p.summary).toContain("Opened PR #42 in acme/widgets"); + }); + + it("normalizes negative and fractional per-file counts to non-negative integers", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 1, title: "t", changedFiles: [{ path: "a", additions: -5, deletions: 2.7 }] }); + expect(p.diffPreview[0]).toEqual({ path: "a", additions: 0, deletions: 2 }); + expect(p.totals).toEqual({ files: 1, additions: 0, deletions: 2 }); + }); + + it("normalizes non-finite per-file counts to zero and passes finite non-negative integers through", () => { + const p = buildResultsPayload({ + repoFullName: "acme/widgets", + prNumber: 1, + title: "t", + changedFiles: [ + { path: "a", additions: Number.NaN, deletions: Number.POSITIVE_INFINITY }, + { path: "b", additions: 3, deletions: 0 }, + ], + }); + expect(p.diffPreview).toEqual([ + { path: "a", additions: 0, deletions: 0 }, + { path: "b", additions: 3, deletions: 0 }, + ]); + expect(p.totals).toEqual({ files: 2, additions: 3, deletions: 0 }); + }); +});