test(pdf-server): bridge coverage gaps and switch fixture to gated loopback HTTP

ochafik · ochafik · commit ce4600f18937 · 2026-04-24T20:09:26.000+01:00
PdfCacheRangeTransport.deliver(): pdf.js's reader is keyed by the original begin and removed after one delivery, so accumulate slices and call onDataRange once with the full buffer (the previous multi-call approach threw inside pdfjs). Covered by a new integration test that drives getDocument()/getPage(1) on a >1MB PDF through a clamping in-memory readPdfRange. validateUrl: allow http://127.0.0.1|localhost|[::1] only when PDF_SERVER_ALLOW_LOOPBACK_HTTP is set, so a remote deploy can't be made to probe its own ports. Covered by env-on/off unit tests. Fixture switched to plain HTTP (no openssl, no NODE_TLS_REJECT_UNAUTHORIZED). Adds /error.pdf (500s after 50KB) and two e2e tests: page 2 renders after stall release (>512KB object path), and display_pdf returns gracefully on mid-load 500. Existing <30% test now samples stats before the iframe loads. New unit tests: display_pdf returns (not hangs) on mid-load fetch failure via in-memory MCP client; computeDiff/serializeDiff contract tests pinning the restoredRemovedIds tombstone-preservation behaviour.
diff --git a/examples/pdf-server/server.test.ts b/examples/pdf-server/server.test.ts
@@ -295,7 +295,7 @@ describe("PDF Cache with Timeouts", () => {
 });
 
 describe("PdfCacheRangeTransport", () => {
-  it("delivers ranges larger than MAX_CHUNK_BYTES in multiple onDataRange calls", async () => {
+  it("accumulates ranges larger than MAX_CHUNK_BYTES into one onDataRange call", async () => {
     const big = MAX_CHUNK_BYTES * 2 + 100;
     const reads: Array<[number, number]> = [];
     const t = new PdfCacheRangeTransport("u", big, async (_u, off, n) => {
@@ -311,7 +311,11 @@ describe("PdfCacheRangeTransport", () => {
     );
     t.requestDataRange(0, big);
     await new Promise((r) => setTimeout(r, 10));
-    expect(delivered).toEqual([
+    // pdf.js's reader is keyed by the original begin and removed after one
+    // delivery, so deliver() must call onDataRange exactly once with the
+    // accumulated buffer — multiple calls would throw inside pdfjs.
+    expect(delivered).toEqual([[0, big]]);
+    expect(reads).toEqual([
       [0, MAX_CHUNK_BYTES],
       [MAX_CHUNK_BYTES, MAX_CHUNK_BYTES],
       [MAX_CHUNK_BYTES * 2, 100],
@@ -339,6 +343,127 @@ describe("PdfCacheRangeTransport", () => {
     t.requestDataRange(0, 100);
     await expect(t.failed).rejects.toThrow(/empty range/);
   });
+
+  it("getDocument resolves on a >1MB PDF when readPdfRange clamps to MAX_CHUNK_BYTES", async () => {
+    // pdfjs coalesces adjacent missing chunks into one requestDataRange that
+    // can exceed MAX_CHUNK_BYTES. deliver() must accumulate clamped reads and
+    // hand pdfjs a single onDataRange(begin, fullBuffer). This test fails if
+    // deliver() either truncates or calls onDataRange more than once per
+    // requestDataRange (pdf.mjs _onReceiveData matches by exact begin).
+    function makeRandomJpeg(len: number): Uint8Array {
+      const header = Uint8Array.from([
+        0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46, 0x49, 0x46, 0x00, 0x01,
+        0x01, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0xff, 0xc0, 0x00, 0x0b,
+        0x08, 0x00, 0x08, 0x00, 0x08, 0x01, 0x01, 0x11, 0x00, 0xff, 0xc4, 0x00,
+        0x14, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xda, 0x00, 0x08, 0x01,
+        0x01, 0x00, 0x00, 0x3f, 0x00,
+      ]);
+      const scan = new Uint8Array(len);
+      for (let i = 0; i < len; i++) {
+        const b = (i * 1103515245 + 12345) & 0xff;
+        scan[i] = b === 0xff ? 0xfe : b;
+      }
+      const eoi = Uint8Array.from([0xff, 0xd9]);
+      const out = new Uint8Array(header.length + len + 2);
+      out.set(header, 0);
+      out.set(scan, header.length);
+      out.set(eoi, header.length + len);
+      return out;
+    }
+
+    const d = await PDFDocument.create();
+    const img = await d.embedJpg(makeRandomJpeg(1_100_000));
+    const page = d.addPage([612, 792]);
+    page.drawImage(img, { x: 36, y: 36, width: 540, height: 720 });
+    const bytes = await d.save();
+    expect(bytes.length).toBeGreaterThan(2 * MAX_CHUNK_BYTES);
+
+    let maxReadLen = 0;
+    const readClamped: PdfCache["readPdfRange"] = async (_u, off, n) => {
+      const len = Math.min(n, MAX_CHUNK_BYTES, bytes.length - off);
+      maxReadLen = Math.max(maxReadLen, len);
+      return { data: bytes.slice(off, off + len), totalBytes: bytes.length };
+    };
+    const transport = new PdfCacheRangeTransport(
+      "mem://big",
+      bytes.length,
+      readClamped,
+    );
+
+    const doc = await Promise.race([
+      getDocument({
+        range: transport,
+        length: bytes.length,
+        disableAutoFetch: true,
+        disableStream: true,
+        rangeChunkSize: 64 * 1024,
+      }).promise,
+      transport.failed,
+      new Promise<never>((_, rej) =>
+        setTimeout(() => rej(new Error("getDocument hung")), 5000),
+      ),
+    ]);
+    const p1 = await Promise.race([
+      doc.getPage(1),
+      transport.failed,
+      new Promise<never>((_, rej) =>
+        setTimeout(() => rej(new Error("getPage hung")), 5000),
+      ),
+    ]);
+    expect(p1).toBeDefined();
+    expect(maxReadLen).toBeLessThanOrEqual(MAX_CHUNK_BYTES);
+    doc.destroy();
+  });
+});
+
+describe("display_pdf transport-error handling", () => {
+  it("returns (does not hang) when range fetches fail mid-load", async () => {
+    // First fetch = the 1-byte size probe → 206 with Content-Range so
+    // display_pdf gets totalBytes. Every subsequent fetch (made by
+    // PdfCacheRangeTransport via readPdfRange) rejects, which must surface
+    // through transport.failed → orFail() → outer catch, not hang.
+    let calls = 0;
+    const mockFetch = spyOn(globalThis, "fetch").mockImplementation(
+      async () => {
+        if (calls++ === 0) {
+          return new Response(new Uint8Array(1), {
+            status: 206,
+            headers: { "Content-Range": "bytes 0-0/50000" },
+          });
+        }
+        throw new Error("network down");
+      },
+    );
+
+    const server = createServer();
+    const client = new Client({ name: "t", version: "1" });
+    const [ct, st] = InMemoryTransport.createLinkedPair();
+    await Promise.all([server.connect(st), client.connect(ct)]);
+
+    try {
+      const result = await Promise.race([
+        client.callTool({
+          name: "display_pdf",
+          arguments: { url: "https://arxiv.org/pdf/err-test" },
+        }),
+        new Promise<never>((_, rej) =>
+          setTimeout(
+            () => rej(new Error("display_pdf hung on transport error")),
+            3000,
+          ),
+        ),
+      ]);
+      expect(result.isError).toBeFalsy();
+      const sc = result.structuredContent as { formFields?: unknown };
+      expect(sc.formFields).toBeUndefined();
+      expect(calls).toBeGreaterThan(1);
+    } finally {
+      mockFetch.mockRestore();
+      await client.close();
+      await server.close();
+    }
+  });
 });
 
 describe("extractFormSchema field-tree handling", () => {
@@ -419,6 +544,24 @@ describe("extractFormSchema field-tree handling", () => {
   });
 });
 
+describe("validateUrl loopback HTTP allow (PDF_SERVER_ALLOW_LOOPBACK_HTTP)", () => {
+  it("rejects http://127.0.0.1 by default", () => {
+    expect(validateUrl("http://127.0.0.1:9999/x.pdf").valid).toBe(false);
+  });
+
+  it("accepts http://127.0.0.1 only when the env gate is set, and never non-loopback http", () => {
+    const prev = process.env.PDF_SERVER_ALLOW_LOOPBACK_HTTP;
+    process.env.PDF_SERVER_ALLOW_LOOPBACK_HTTP = "1";
+    try {
+      expect(validateUrl("http://127.0.0.1:9999/x.pdf").valid).toBe(true);
+      expect(validateUrl("http://169.254.169.254/").valid).toBe(false);
+    } finally {
+      if (prev === undefined) delete process.env.PDF_SERVER_ALLOW_LOOPBACK_HTTP;
+      else process.env.PDF_SERVER_ALLOW_LOOPBACK_HTTP = prev;
+    }
+  });
+});
+
 describe("validateUrl with MCP roots (allowedLocalDirs)", () => {
   const savedFiles = new Set(allowedLocalFiles);
   const savedDirs = new Set(allowedLocalDirs);
diff --git a/examples/pdf-server/server.ts b/examples/pdf-server/server.ts
@@ -641,10 +641,19 @@ export function validateUrl(url: string): {
   // Remote URL - require HTTPS
   try {
     const parsed = new URL(url);
-    if (parsed.protocol !== "https:") {
-      return { valid: false, error: `Only HTTPS URLs are allowed: ${url}` };
+    if (parsed.protocol === "https:") return { valid: true };
+    // Loopback HTTP is opt-in (test fixtures, local dev). Off by default so a
+    // remotely-deployed server can't be made to probe its own internal ports.
+    if (
+      process.env.PDF_SERVER_ALLOW_LOOPBACK_HTTP &&
+      parsed.protocol === "http:" &&
+      (parsed.hostname === "127.0.0.1" ||
+        parsed.hostname === "localhost" ||
+        parsed.hostname === "[::1]")
+    ) {
+      return { valid: true };
     }
-    return { valid: true };
+    return { valid: false, error: `Only HTTPS URLs are allowed: ${url}` };
   } catch {
     return { valid: false, error: `Invalid URL: ${url}` };
   }
@@ -914,20 +923,23 @@ export class PdfCacheRangeTransport extends PDFDataRangeTransport {
 
   /**
    * pdf.js coalesces adjacent missing chunks into one unbounded request, but
-   * readPdfRange clamps each call to MAX_CHUNK_BYTES. Deliver in slices so
-   * the worker marks every requested chunk as loaded.
+   * readPdfRange clamps each call to MAX_CHUNK_BYTES. Its reader is keyed by
+   * the original `begin` and removed after one delivery, so we must accumulate
+   * slices and call onDataRange exactly once with the full buffer.
    */
   private async deliver(begin: number, end: number): Promise<void> {
-    let off = begin;
-    while (off < end) {
-      const want = Math.min(end - off, MAX_CHUNK_BYTES);
-      const { data } = await this.readPdfRange(this.url, off, want);
+    const buf = new Uint8Array(end - begin);
+    let off = 0;
+    while (off < buf.length) {
+      const want = Math.min(buf.length - off, MAX_CHUNK_BYTES);
+      const { data } = await this.readPdfRange(this.url, begin + off, want);
       if (data.length === 0) {
-        throw new Error(`empty range at ${off} for ${this.url}`);
+        throw new Error(`empty range at ${begin + off} for ${this.url}`);
       }
-      this.onDataRange(off, data);
+      buf.set(data.subarray(0, Math.min(data.length, buf.length - off)), off);
       off += data.length;
     }
+    this.onDataRange(begin, buf);
   }
 }
 
diff --git a/examples/pdf-server/src/pdf-annotations.test.ts b/examples/pdf-server/src/pdf-annotations.test.ts
@@ -367,6 +367,92 @@ describe("computeDiff", () => {
       current.map((a) => a.id).sort(),
     );
   });
+
+  // Backs the post-computeDiff union step in mcp-app.ts persistAnnotations /
+  // getAnnotatedPdfBytes: with the lazy per-page baseline scan, computeDiff
+  // alone cannot produce `removed` for natives on pages not yet visited, so
+  // callers must union restoredRemovedIds. These tests pin the contract those
+  // call sites depend on.
+  describe("partial baseline (lazy-scan tombstone preservation)", () => {
+    const tombstoned = "pdf-5-0";
+    const userNote: PdfAnnotationDef = {
+      type: "note",
+      id: "u1",
+      page: 1,
+      x: 10,
+      y: 10,
+      content: "unrelated edit",
+    };
+
+    /** Mirrors the union loop in mcp-app.ts persistAnnotations. */
+    function unionRestored(
+      diff: AnnotationDiff,
+      restored: Iterable<string>,
+      currentIds: Set<string>,
+    ): void {
+      for (const id of restored) {
+        if (!currentIds.has(id) && !diff.removed.includes(id)) {
+          diff.removed.push(id);
+        }
+      }
+    }
+
+    it("computeDiff alone drops tombstones for unscanned pages; the union step preserves them", () => {
+      const baseline: PdfAnnotationDef[] = []; // page with the native not yet scanned
+      const current = [userNote];
+      const diff = computeDiff(baseline, current, new Map());
+      expect(diff.removed).toEqual([]); // proves the union step is load-bearing
+
+      unionRestored(diff, [tombstoned], new Set(current.map((a) => a.id)));
+
+      expect(diff.removed).toEqual([tombstoned]);
+      expect(isDiffEmpty(diff)).toBe(false);
+      expect(deserializeDiff(serializeDiff(diff)).removed).toEqual([
+        tombstoned,
+      ]);
+    });
+
+    it("a tombstone the user re-added is excluded from the union", () => {
+      const reAdded: PdfAnnotationDef = {
+        type: "note",
+        id: tombstoned,
+        page: 3,
+        x: 0,
+        y: 0,
+        content: "back",
+      };
+      const current = [userNote, reAdded];
+      const diff = computeDiff([], current, new Map());
+      unionRestored(diff, [tombstoned], new Set(current.map((a) => a.id)));
+      expect(diff.removed).toEqual([]);
+    });
+
+    it("union does not duplicate ids once the page is scanned and computeDiff produces them", () => {
+      const native: PdfAnnotationDef = {
+        type: "note",
+        id: tombstoned,
+        page: 3,
+        x: 0,
+        y: 0,
+        content: "native",
+      };
+      const diff = computeDiff([native], [userNote], new Map());
+      expect(diff.removed).toEqual([tombstoned]);
+      unionRestored(diff, [tombstoned], new Set([userNote.id]));
+      expect(diff.removed).toEqual([tombstoned]);
+    });
+
+    it("removedRefs from restored tombstones parse for buildAnnotatedPdfBytes; non-ref ids are skipped", () => {
+      const restored = new Set(["pdf-5-0", "pdf-12R", "pdf-2-idx-7"]);
+      const removedRefs = [...restored]
+        .map(parseAnnotationRef)
+        .filter((r): r is NonNullable<typeof r> => r !== null);
+      expect(removedRefs).toEqual([
+        { objectNumber: 5, generationNumber: 0 },
+        { objectNumber: 12, generationNumber: 0 },
+      ]);
+    });
+  });
 });
 
 // =============================================================================
diff --git a/playwright.config.ts b/playwright.config.ts
@@ -47,14 +47,10 @@ export default defineConfig({
     env: {
       ...process.env,
       EXAMPLE: process.env.EXAMPLE ?? "",
-      // pdf-incremental-load.spec.ts serves test PDFs over self-signed HTTPS
-      // and pdf-server's outbound fetch must accept that cert. This disables
-      // TLS verification for the entire e2e webServer process tree (all
-      // example servers launched by examples:start), not just pdf-server —
-      // run-all.ts inherits env to every child. Acceptable for the e2e
-      // sandbox; never set in production. Tightening to pdf-server alone
-      // requires a validateUrl localhost allow (tracked separately).
-      NODE_TLS_REJECT_UNAUTHORIZED: "0",
+      // Let pdf-server fetch from the http://127.0.0.1 range-counting fixture
+      // (validateUrl rejects loopback HTTP unless this is set). Scoped to this
+      // server's check only — does not touch Node's TLS verification.
+      PDF_SERVER_ALLOW_LOOPBACK_HTTP: "1",
     },
   },
   // Snapshot configuration
diff --git a/tests/e2e/pdf-incremental-load.spec.ts b/tests/e2e/pdf-incremental-load.spec.ts
diff --git a/tests/helpers/range-counting-server.ts b/tests/helpers/range-counting-server.ts
