fix(engine): validate repoFullName and prNumber before composing the customer-facing results payload

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"

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 });
+  });
+});