fix: reject oversized checkout diffs (#2488)

* fix: reject oversized checkout diffs * fix(diff): keep large previews from disconnecting clients The physical socket limit is an OOM backstop, not a frame-size contract. Bound structured diff construction incrementally and report oversized previews without closing the client's socket. * fix(app): preserve oversized diff state * fix(diff): stay within relay frame limit --------- Co-authored-by: Mohamed Boudra <boudra.moha@gmail.com>
2026-07-30 00:48:05 +08:00
parent 16bbead5a4
commit 9e5accee3c
24 changed files with 321 additions and 81 deletions
@@ -14,11 +14,13 @@ import { join } from "path";
 import { win32 } from "node:path";
 import { tmpdir } from "os";
 import pino from "pino";
+import { base64EncryptedWireByteLength } from "@getpaseo/relay";
 import {
  __resetCheckoutShortstatCacheForTests,
  __resetPullRequestStatusCacheForTests,
  __setPullRequestStatusCacheTtlForTests,
  commitAll,
+  CHECKOUT_DIFF_MAX_STRUCTURED_BYTES,
  createPullRequest,
  getCachedCheckoutShortstat,
  getCheckoutSnapshotFacts,
@@ -1333,6 +1335,19 @@ const x = 1;
    );
  });

+  it("keeps the structured diff cap below the relay frame limit", () => {
+    const relayFrameBytes = 32 * 1024 * 1024;
+    const frameEnvelopeHeadroomBytes = 1024 * 1024;
+    const diffWireBytes = base64EncryptedWireByteLength(CHECKOUT_DIFF_MAX_STRUCTURED_BYTES);
+    const maximumFrameWireBytes = base64EncryptedWireByteLength(
+      CHECKOUT_DIFF_MAX_STRUCTURED_BYTES + frameEnvelopeHeadroomBytes,
+    );
+
+    expect(CHECKOUT_DIFF_MAX_STRUCTURED_BYTES).toBe(24_117_208);
+    expect(diffWireBytes).toBeLessThan(relayFrameBytes);
+    expect(maximumFrameWireBytes).toBe(relayFrameBytes);
+  });
+
  it("marks tracked generated one-line diffs as too_large by content size", async () => {
    writeFileSync(join(repoDir, "generated.js"), `const data = "old";\n`);
    execFileSync("git", ["add", "generated.js"], { cwd: repoDir });
@@ -3,9 +3,14 @@ import { existsSync, realpathSync } from "fs";
 import { open as openFile, readFile, stat as statFile } from "fs/promises";
 import { TTLCache } from "@isaacs/ttlcache";
 import type { CheckoutCommit, CheckoutCommitFile } from "@getpaseo/protocol/messages";
+import { maxBase64EncryptedPlaintextByteLength } from "@getpaseo/relay";
 import type { Logger } from "pino";
 import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js";
-import { parseAndHighlightDiff } from "../server/utils/diff-highlighter.js";
+import {
+  highlightDiffWithFileContent,
+  parseAndHighlightDiff,
+  parseDiff,
+} from "../server/utils/diff-highlighter.js";
 import { parseGitHubRepoFromRemote } from "../server/workspace-git-metadata.js";
 import { createGitHubService } from "../services/github-service.js";
 import type {
@@ -583,6 +588,7 @@ function buildGitDiffArgs(args: { ignoreWhitespace?: boolean; extra: string[] })
 }

 const TRACKED_DIFF_NUMSTAT_MAX_BYTES = 2 * 1024 * 1024; // 2MB
+const TRACKED_DIFF_BATCH_SIZE = 8;
 const EMPTY_TREE_OBJECT_ID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";

 function isUnbornHeadDiffError(error: unknown): boolean {
@@ -759,10 +765,9 @@ export type CheckoutStatusGit = CheckoutStatusGitNonPaseo | CheckoutStatusGitPas

 export type CheckoutStatusResult = CheckoutStatus | CheckoutStatusGit;

-export interface CheckoutDiffResult {
-  diff: string;
-  structured?: ParsedDiffFile[];
-}
+export type CheckoutDiffResult =
+  | { diff: string; structured?: ParsedDiffFile[]; diffTooLarge?: false }
+  | { diff: ""; structured: []; diffTooLarge: true };

 export interface CheckoutDiffCompare {
  mode: "uncommitted" | "base";
@@ -1823,6 +1828,37 @@ export async function getCheckoutSnapshotFacts(

 const PER_FILE_DIFF_MAX_BYTES = 1024 * 1024; // 1MB
 const TOTAL_DIFF_MAX_BYTES = 2 * 1024 * 1024; // 2MB
+const RELAY_MAX_FRAME_BYTES = 32 * 1024 * 1024;
+const CHECKOUT_DIFF_FRAME_HEADROOM_BYTES = 1024 * 1024;
+// Temporary until diffs load lazily per file. The Paseo relay's 32 MiB frame limit is
+// binding: string frames are encrypted and base64-encoded. Reserve 1 MiB plaintext for
+// the surrounding WebSocket JSON envelope after inverting that exact wire expansion.
+export const CHECKOUT_DIFF_MAX_STRUCTURED_BYTES =
+  maxBase64EncryptedPlaintextByteLength(RELAY_MAX_FRAME_BYTES) - CHECKOUT_DIFF_FRAME_HEADROOM_BYTES;
+
+interface StructuredDiffAccumulator {
+  files: ParsedDiffFile[];
+  serializedBytes: number;
+}
+
+function createStructuredDiffAccumulator(): StructuredDiffAccumulator {
+  return { files: [], serializedBytes: Buffer.byteLength("[]", "utf8") };
+}
+
+function appendStructuredFile(
+  structured: StructuredDiffAccumulator,
+  file: ParsedDiffFile,
+): boolean {
+  const separatorBytes = structured.files.length > 0 ? 1 : 0;
+  const fileBytes = Buffer.byteLength(JSON.stringify(file), "utf8");
+  const nextBytes = structured.serializedBytes + separatorBytes + fileBytes;
+  if (nextBytes > CHECKOUT_DIFF_MAX_STRUCTURED_BYTES) {
+    return false;
+  }
+  structured.files.push(file);
+  structured.serializedBytes = nextBytes;
+  return true;
+}
 const UNTRACKED_BINARY_SNIFF_BYTES = 16 * 1024;

 async function isLikelyBinaryFile(absolutePath: string): Promise<boolean> {
@@ -2543,27 +2579,62 @@ export function warmCheckoutShortstatInBackground(
 interface AppendStructuredTrackedDiffsInput {
  cwd: string;
  trackedChanges: CheckoutFileChange[];
-  trackedChangeByPath: Map<string, CheckoutFileChange>;
  trackedNumstatByPath: Map<string, FileStat>;
  trackedPlaceholderByPath: Map<string, { status: "binary" | "too_large"; stat: FileStat }>;
  trackedDiffText: string;
  refsForDiff: CheckoutDiffRefs;
  ignoreWhitespace: boolean;
-  structured: ParsedDiffFile[];
-  appendDiff: (text: string) => void;
+  structured: StructuredDiffAccumulator;
  appendTrackedPlaceholderComment: (
    change: CheckoutFileChange,
    status: "binary" | "too_large",
  ) => void;
 }

+async function buildHighlightedTrackedDiffFile(input: {
+  cwd: string;
+  change: CheckoutFileChange;
+  parsedFile: ParsedDiffFile;
+  refsForDiff: CheckoutDiffRefs;
+}): Promise<ParsedDiffFile> {
+  const { cwd, change, parsedFile, refsForDiff } = input;
+  const refPath = change.oldPath ?? change.path;
+  const [oldFileContent, newFileContent] = await Promise.all([
+    change.isNew ? null : readGitFileContentAtRef(cwd, refsForDiff.baseRef, refPath),
+    refsForDiff.targetRef ? readGitFileContentAtRef(cwd, refsForDiff.targetRef, change.path) : null,
+  ]);
+  const highlightedFile = await highlightDiffWithFileContent(parsedFile, cwd, {
+    oldFileContent,
+    newFileContent,
+  });
+  return {
+    ...highlightedFile,
+    path: change.path,
+    isNew: change.isNew,
+    isDeleted: change.isDeleted,
+    status: "ok",
+  };
+}
+
+function isWhitespaceOnlyTrackedChange(input: {
+  change: CheckoutFileChange;
+  stat: FileStat;
+  ignoreWhitespace: boolean;
+}): boolean {
+  const { change, stat, ignoreWhitespace } = input;
+  return (
+    ignoreWhitespace &&
+    change.status.startsWith("M") &&
+    (!stat || (!stat.isBinary && stat.additions === 0 && stat.deletions === 0))
+  );
+}
+
 async function appendStructuredTrackedDiffs(
  input: AppendStructuredTrackedDiffsInput,
-): Promise<void> {
+): Promise<boolean> {
  const {
    cwd,
    trackedChanges,
-    trackedChangeByPath,
    trackedNumstatByPath,
    trackedPlaceholderByPath,
    trackedDiffText,
@@ -2573,36 +2644,19 @@ async function appendStructuredTrackedDiffs(
    appendTrackedPlaceholderComment,
  } = input;

-  const parsedTrackedFiles =
-    trackedDiffText.length > 0
-      ? await parseAndHighlightDiff(trackedDiffText, cwd, {
-          getOldFileContent: async (file) => {
-            const change = trackedChangeByPath.get(file.path);
-            if (!change || change.isNew) {
-              return null;
-            }
-            const refPath = change.oldPath ?? change.path;
-            return readGitFileContentAtRef(cwd, refsForDiff.baseRef, refPath);
-          },
-          getNewFileContent: async (file) => {
-            if (!refsForDiff.targetRef) {
-              return null;
-            }
-            return readGitFileContentAtRef(cwd, refsForDiff.targetRef, file.path);
-          },
-        })
-      : [];
+  const parsedTrackedFiles = trackedDiffText.length > 0 ? parseDiff(trackedDiffText) : [];
  const parsedTrackedByPath = new Map(parsedTrackedFiles.map((file) => [file.path, file]));

  for (const change of trackedChanges) {
    const placeholder = trackedPlaceholderByPath.get(change.path);
    if (placeholder) {
-      structured.push(
-        buildPlaceholderParsedDiffFile(change, {
-          status: placeholder.status,
-          stat: placeholder.stat,
-        }),
-      );
+      const file = buildPlaceholderParsedDiffFile(change, {
+        status: placeholder.status,
+        stat: placeholder.stat,
+      });
+      if (!appendStructuredFile(structured, file)) {
+        return false;
+      }
      appendTrackedPlaceholderComment(change, placeholder.status);
      continue;
    }
@@ -2610,28 +2664,26 @@ async function appendStructuredTrackedDiffs(
    const stat = trackedNumstatByPath.get(change.path) ?? null;
    const parsedFile = parsedTrackedByPath.get(change.path);
    if (parsedFile) {
-      structured.push({
-        ...parsedFile,
-        path: change.path,
-        isNew: change.isNew,
-        isDeleted: change.isDeleted,
-        status: "ok",
+      const file = await buildHighlightedTrackedDiffFile({
+        cwd,
+        change,
+        parsedFile,
+        refsForDiff,
      });
+      if (!appendStructuredFile(structured, file)) {
+        return false;
+      }
      continue;
    }

    // `git diff -w --name-status` can still report a modified path even when the
    // whitespace-filtered patch and numstat are both empty. Skip emitting a
    // structured placeholder in that case so whitespace-only edits truly disappear.
-    if (
-      ignoreWhitespace &&
-      change.status.startsWith("M") &&
-      (!stat || (!stat.isBinary && stat.additions === 0 && stat.deletions === 0))
-    ) {
+    if (isWhitespaceOnlyTrackedChange({ change, stat, ignoreWhitespace })) {
      continue;
    }

-    structured.push({
+    const file = {
      path: change.path,
      isNew: change.isNew,
      isDeleted: change.isDeleted,
@@ -2639,8 +2691,13 @@ async function appendStructuredTrackedDiffs(
      deletions: stat?.deletions ?? 0,
      hunks: [],
      status: "ok",
-    });
+    } satisfies ParsedDiffFile;
+    if (!appendStructuredFile(structured, file)) {
+      return false;
+    }
  }
+
+  return true;
 }

 interface ProcessUntrackedChangeInput {
@@ -2648,11 +2705,11 @@ interface ProcessUntrackedChangeInput {
  change: CheckoutFileChange;
  ignoreWhitespace: boolean;
  includeStructured: boolean;
-  structured: ParsedDiffFile[];
+  structured: StructuredDiffAccumulator;
  appendDiff: (text: string) => void;
 }

-async function processUntrackedChange(input: ProcessUntrackedChangeInput): Promise<void> {
+async function processUntrackedChange(input: ProcessUntrackedChangeInput): Promise<boolean> {
  const { cwd, change, ignoreWhitespace, includeStructured, structured, appendDiff } = input;
  const { text, truncated, stat } = await getUntrackedDiffText(cwd, change, ignoreWhitespace);

@@ -2664,19 +2721,33 @@ async function processUntrackedChange(input: ProcessUntrackedChangeInput): Promi
    } else {
      appendDiff(text);
    }
-    return;
+    return true;
  }

  if (stat?.isBinary) {
-    structured.push(buildPlaceholderParsedDiffFile(change, { status: "binary", stat }));
+    if (
+      !appendStructuredFile(
+        structured,
+        buildPlaceholderParsedDiffFile(change, { status: "binary", stat }),
+      )
+    ) {
+      return false;
+    }
    appendDiff(`# ${change.path}: binary diff omitted\n`);
-    return;
+    return true;
  }

  if (truncated) {
-    structured.push(buildPlaceholderParsedDiffFile(change, { status: "too_large", stat }));
+    if (
+      !appendStructuredFile(
+        structured,
+        buildPlaceholderParsedDiffFile(change, { status: "too_large", stat }),
+      )
+    ) {
+      return false;
+    }
    appendDiff(`# ${change.path}: diff too large omitted\n`);
-    return;
+    return true;
  }

  appendDiff(text);
@@ -2692,13 +2763,14 @@ async function processUntrackedChange(input: ProcessUntrackedChangeInput): Promi
      hunks: [],
    } satisfies ParsedDiffFile);

-  structured.push({
+  const file = {
    ...parsedFile,
    path: change.path,
    isNew: change.isNew,
    isDeleted: change.isDeleted,
    status: "ok",
-  });
+  } satisfies ParsedDiffFile;
+  return appendStructuredFile(structured, file);
 }

 interface ProcessTrackedChangesInput {
@@ -2710,7 +2782,6 @@ interface ProcessTrackedChangesInput {
 }

 interface ProcessTrackedChangesResult {
-  trackedChangeByPath: Map<string, CheckoutFileChange>;
  trackedNumstatByPath: Map<string, FileStat>;
  trackedPlaceholderByPath: Map<string, { status: "binary" | "too_large"; stat: FileStat }>;
  trackedDiffText: string;
@@ -2720,7 +2791,6 @@ async function processTrackedChanges(
  input: ProcessTrackedChangesInput,
 ): Promise<ProcessTrackedChangesResult> {
  const { cwd, refsForDiff, trackedChanges, ignoreWhitespace, appendDiff } = input;
-  const trackedChangeByPath = new Map(trackedChanges.map((change) => [change.path, change]));
  const trackedNumstatByPath =
    trackedChanges.length > 0
      ? await getTrackedNumstatByPath(cwd, refsForDiff, ignoreWhitespace)
@@ -2742,9 +2812,10 @@ async function processTrackedChanges(

  let trackedDiffText = "";
  let trackedDiffBytes = 0;
-  if (trackedDiffPaths.length > 0) {
+  for (let start = 0; start < trackedDiffPaths.length; start += TRACKED_DIFF_BATCH_SIZE) {
+    const paths = trackedDiffPaths.slice(start, start + TRACKED_DIFF_BATCH_SIZE);
    const trackedDiffs = await Promise.all(
-      trackedDiffPaths.map((path) =>
+      paths.map((path) =>
        getTrackedDiffTextForPath({
          cwd,
          refsForDiff,
@@ -2754,7 +2825,6 @@ async function processTrackedChanges(
      ),
    );

-    const visibleTrackedDiffs: string[] = [];
    for (const fileDiff of trackedDiffs) {
      if (fileDiff.truncated) {
        trackedPlaceholderByPath.set(fileDiff.path, {
@@ -2772,15 +2842,12 @@ async function processTrackedChanges(
        continue;
      }
      trackedDiffBytes += diffBytes;
-      visibleTrackedDiffs.push(fileDiff.text);
+      trackedDiffText += fileDiff.text;
    }
-
-    trackedDiffText = visibleTrackedDiffs.join("");
-    appendDiff(trackedDiffText);
  }
+  appendDiff(trackedDiffText);

  return {
-    trackedChangeByPath,
    trackedNumstatByPath,
    trackedPlaceholderByPath,
    trackedDiffText,
@@ -2840,7 +2907,7 @@ export async function getCheckoutDiff(
    return a.path < b.path ? -1 : 1;
  });

-  const structured: ParsedDiffFile[] = [];
+  const structured = createStructuredDiffAccumulator();
  let diffText = "";
  let diffBytes = 0;
  const appendDiff = (text: string) => {
@@ -2881,19 +2948,20 @@ export async function getCheckoutDiff(
  };

  if (compare.includeStructured) {
-    await appendStructuredTrackedDiffs({
+    const didAppendTrackedDiffs = await appendStructuredTrackedDiffs({
      cwd,
      trackedChanges,
-      trackedChangeByPath: trackedDiff.trackedChangeByPath,
      trackedNumstatByPath: trackedDiff.trackedNumstatByPath,
      trackedPlaceholderByPath: trackedDiff.trackedPlaceholderByPath,
      trackedDiffText: trackedDiff.trackedDiffText,
      refsForDiff: effectiveRefsForDiff,
      ignoreWhitespace,
      structured,
-      appendDiff,
      appendTrackedPlaceholderComment,
    });
+    if (!didAppendTrackedDiffs) {
+      return { diff: "", structured: [], diffTooLarge: true };
+    }
  } else {
    for (const change of trackedChanges) {
      const placeholder = trackedDiff.trackedPlaceholderByPath.get(change.path);
@@ -2907,7 +2975,7 @@ export async function getCheckoutDiff(
    if (diffBytes >= TOTAL_DIFF_MAX_BYTES) {
      break;
    }
-    await processUntrackedChange({
+    const didAppendUntrackedDiff = await processUntrackedChange({
      cwd,
      change,
      ignoreWhitespace,
@@ -2915,10 +2983,13 @@ export async function getCheckoutDiff(
      structured,
      appendDiff,
    });
+    if (!didAppendUntrackedDiff) {
+      return { diff: "", structured: [], diffTooLarge: true };
+    }
  }

  if (compare.includeStructured) {
-    return { diff: diffText, structured };
+    return { diff: diffText, structured: structured.files };
  }
  return { diff: diffText };
 }