From be1e5f77ff1152c2ee18849d5dec646459b42d3d Mon Sep 17 00:00:00 2001 From: kai392 Date: Wed, 29 Jul 2026 10:04:50 +0800 Subject: [PATCH] fix(engine): validate the repo and PR number behind buildResultsPayload's customer link (#9611) buildResultsPayload composes the customer-facing prLink and summary by raw interpolation of repoFullName and prNumber, neither validated: - repoFullName like "acme/widgets/../../evil" produced a link the browser resolves to github.com/evil. It's now treated as valid only when it splits into exactly two path-safe segments (the same isValidRepoSegment guard governor-ledger.ts keeps locally); otherwise prLink is null and the summary renders "unknown repository" instead of the raw value. - prNumber only had a non-null check, so 0/-3/2.5 took the has-PR branch and rendered ".../pull/0" etc. hasPr now additionally requires Number.isInteger && > 0. - additions/deletions now normalize with the finiteNonNegativeInt rule the sibling Rent-a-Loop modules use, so a negative or fractional count can't reach totals or the diff. title's redactSecrets scrub, MAX_DIFF_PREVIEW_FILES, the diff slice, the status default, and diffPreview ordering are unchanged; the function still never throws. Tests cover every branch in both suites: the engine's own node:test suite (packages/loopover-engine/test/results-payload.test.ts, the source the engine Codecov flag grades) at 100% on the changed code, plus the root vitest suite's required regression cases. Closes #9611 Co-Authored-By: Claude Opus 4.8 --- .../loopover-engine/src/results-payload.ts | 33 ++++++++-- .../test/results-payload.test.ts | 60 +++++++++++++++++++ test/unit/results-payload.test.ts | 26 ++++++++ 3 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 packages/loopover-engine/test/results-payload.test.ts diff --git a/packages/loopover-engine/src/results-payload.ts b/packages/loopover-engine/src/results-payload.ts index 15e2d1765e..b40ae66336 100644 --- a/packages/loopover-engine/src/results-payload.ts +++ b/packages/loopover-engine/src/results-payload.ts @@ -40,22 +40,45 @@ export type ResultsPayload = { /** Package a completed iteration into the customer-facing results payload (#4801). Pure: it formats * already-fetched iteration metadata, it does not fetch, open, or deliver anything. */ +// #9611: the same path-safety guard restated locally (this engine package must not import from the miner +// package, and governor-ledger.ts keeps its own copy for the same reason): a repo segment is entirely +// [A-Za-z0-9._-] and is not a bare "." / ".." traversal segment. +const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; +function isValidRepoSegment(segment: string): boolean { + return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== ".."; +} + +// #9611: same non-negative-integer normalization the sibling Rent-a-Loop modules use (tenant-quota.ts etc.), +// so a caller-supplied negative or fractional additions/deletions count can't flow into totals or the diff. +function finiteNonNegativeInt(value: number): number { + return Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0; +} + 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; + // #9611: `prLink` becomes a clickable customer-facing URL, so validate BOTH interpolated values. An + // unvalidated repoFullName like "acme/widgets/../../evil" resolves in the browser to github.com/evil; a + // non-positive or non-integer prNumber renders ".../pull/0" or ".../pull/-3". Treat repoFullName as real + // only when it is exactly two path-safe segments, and require a positive integer PR number. + const repoSegments = result.repoFullName.split("/"); + const validRepo = repoSegments.length === 2 && repoSegments.every((segment) => isValidRepoSegment(segment)); + const repoDisplay = validRepo ? result.repoFullName : "unknown repository"; + + const hasPr = + result.prNumber !== null && result.prNumber !== undefined && Number.isInteger(result.prNumber) && result.prNumber > 0; + const prLink = hasPr && validRepo ? `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 ${repoDisplay}` : `No pull request was opened for ${repoDisplay}`; const changePart = totals.files === 0 ? "no file changes" diff --git a/packages/loopover-engine/test/results-payload.test.ts b/packages/loopover-engine/test/results-payload.test.ts new file mode 100644 index 0000000000..9631aee7ea --- /dev/null +++ b/packages/loopover-engine/test/results-payload.test.ts @@ -0,0 +1,60 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { buildResultsPayload } from "../dist/index.js"; +import type { IterationResult } from "../dist/index.js"; + +const base = (over: Partial = {}): IterationResult => ({ + repoFullName: "acme/widgets", + prNumber: 1, + title: "t", + changedFiles: [], + ...over, +}); + +test("#9611: a valid repo + positive integer PR produces the canonical link and names the repo", () => { + const p = buildResultsPayload(base({ prNumber: 42 })); + assert.equal(p.prLink, "https://github.com/acme/widgets/pull/42"); + assert.ok(p.summary.includes("Opened PR #42 in acme/widgets")); +}); + +test("#9611: an invalid repoFullName (traversal / extra segment / bad char / missing half) yields no link + 'unknown repository'", () => { + for (const repoFullName of ["acme/widgets/../../evil", "a/b/c", "acme/wid gets", "acme/..", "acme/.", "./widgets", "/widgets", "acme/", "acme"]) { + const p = buildResultsPayload(base({ repoFullName, prNumber: 1 })); + assert.equal(p.prLink, null, `expected null prLink for ${repoFullName}`); + assert.ok(p.summary.includes("unknown repository"), `expected 'unknown repository' for ${repoFullName}`); + assert.ok(!p.summary.includes("../"), `summary must not leak raw traversal for ${repoFullName}`); + } +}); + +test("#9611: a non-positive / non-integer / absent prNumber takes the no-PR branch", () => { + for (const prNumber of [0, -3, 2.5, null, undefined]) { + const p = buildResultsPayload(base({ prNumber })); + assert.equal(p.prLink, null); + assert.ok(p.summary.includes("No pull request was opened for acme/widgets")); + } +}); + +test("#9611: a valid PR number against an invalid repo still yields prLink null", () => { + const p = buildResultsPayload(base({ repoFullName: "acme/../evil", prNumber: 7 })); + assert.equal(p.prLink, null); +}); + +test("#9611: additions/deletions normalize to non-negative integers (negative, fractional, non-finite)", () => { + const p = buildResultsPayload( + base({ + changedFiles: [ + { path: "a", additions: -5, deletions: 2.7 }, + { path: "b", additions: Number.NaN, deletions: 3 }, + ], + }), + ); + assert.deepEqual(p.diffPreview[0], { path: "a", additions: 0, deletions: 2 }); + assert.deepEqual(p.diffPreview[1], { path: "b", additions: 0, deletions: 3 }); + assert.deepEqual(p.totals, { files: 2, additions: 0, deletions: 5 }); +}); + +test("#9611: absent additions/deletions default to 0 (the ?? 0 arms)", () => { + const p = buildResultsPayload(base({ changedFiles: [{ path: "a" }] })); + assert.deepEqual(p.diffPreview[0], { path: "a", additions: 0, deletions: 0 }); +}); diff --git a/test/unit/results-payload.test.ts b/test/unit/results-payload.test.ts index a0766df17e..b471d24fb4 100644 --- a/test/unit/results-payload.test.ts +++ b/test/unit/results-payload.test.ts @@ -73,4 +73,30 @@ describe("buildResultsPayload — packages a completed loop iteration (#4801)", const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 12, title, changedFiles: [] }); expect(p.summary).toBe(`Opened PR #12 in acme/widgets: ${title}. no file changes. Status: open.`); }); + + it("#9611: a path-traversal repoFullName yields no link and an 'unknown repository' summary", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets/../../evil", prNumber: 1, title: "t", changedFiles: [] }); + expect(p.prLink).toBeNull(); + expect(p.summary).toContain("unknown repository"); + expect(p.summary).not.toContain("../"); + }); + + it("#9611: a non-positive or non-integer prNumber takes the no-PR branch (link null, valid repo still named)", () => { + for (const prNumber of [0, -3, 2.5]) { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber, title: "t", changedFiles: [] }); + expect(p.prLink).toBeNull(); + expect(p.summary).toContain("No pull request was opened for acme/widgets"); + } + }); + + it("#9611: negative/fractional additions and deletions normalize 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("#9611: a valid repo + positive integer PR still produces the canonical link", () => { + const p = buildResultsPayload({ repoFullName: "acme/widgets", prNumber: 42, title: "t", changedFiles: [] }); + expect(p.prLink).toBe("https://github.com/acme/widgets/pull/42"); + }); });