v1.11.10: stream-cap response body at 5MB, abort on overflow

apps/server/src/services/__tests__/web_tools.test.ts

@@ -295,9 +295,10 @@ describe('executeWebFetch — size + truncation', () => {
     // 1.5M U+1F600 emojis: each is length 2 in UTF-16 (surrogate pair) and
     // 4 bytes in UTF-8. body.length = 3,000,000 chars (~2.86 MiB by
     // UTF-16 count) but Buffer.byteLength = 6,000,000 bytes (>5 MiB).
-    // Pre-fix the char-count comparison let this through; the byte-count
+    // v1.11.10: streaming reader catches this as body_too_large (was
-    // check now rejects. No Content-Length header so the pre-flight
+    // response_too_large in the post-consumption check). No
-    // guard doesn't fire — we're testing the POST-consumption check.
+    // Content-Length header so the pre-flight pass and the streaming
+    // path is the one that rejects.
     const heavy = '😀'.repeat(1_500_000);
     const fakeFetch = vi.fn().mockResolvedValue(
       new Response(heavy, { status: 200, headers: { 'content-type': 'text/plain' } }),
@@ -308,9 +309,8 @@ describe('executeWebFetch — size + truncation', () => {
     );
     expect('error' in result).toBe(true);
     if ('error' in result) {
-      expect(result.error).toBe('response_too_large');
+      expect(result.error).toBe('body_too_large');
-      // Error reason should reference bytes, not character count.
+      expect(result.reason).toMatch(/exceeded/);
-      expect(result.reason).toMatch(/bytes/);
     }
   });
 
@@ -343,3 +343,248 @@ describe('executeWebFetch — size + truncation', () => {
     expect('content' in result && result.truncated).toBe(false);
   });
 });
+
+// ============================================================================
+// v1.11.9: manual redirect handling — re-run URL guard on each hop
+// ============================================================================
+
+// Helper: build a 30x redirect Response. status 302 by default; tests
+// pass other codes (or omit the Location header) when they need to.
+function redirect(loc: string | null, status = 302): Response {
+  const headers: Record<string, string> = {};
+  if (loc !== null) headers['location'] = loc;
+  return new Response('', { status, headers });
+}
+
+describe('executeWebFetch — redirect handling', () => {
+  it('blocks a redirect target that resolves to a private IP (AWS IMDS)', async () => {
+    // Public-IP origin 302s into 169.254.169.254 (link-local). Pre-v1.11.9
+    // `redirect: 'follow'` would silently follow this; the new manual
+    // loop re-runs isPublicUrl on the resolved target and blocks.
+    const fakeFetch = vi
+      .fn<typeof fetch>()
+      .mockResolvedValueOnce(redirect('http://169.254.169.254/latest/meta-data/'));
+    const result = await executeWebFetch(
+      { url: 'https://example.com/redirect' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('error' in result).toBe(true);
+    if ('error' in result) {
+      expect(result.error).toBe('blocked_by_url_guard');
+      // Reason should make it clear this was a REDIRECT hop, not the
+      // initial URL — so logs can distinguish the two failure modes.
+      expect(result.reason).toMatch(/redirect target/);
+    }
+    // Critical: the second fetch (the private target) must NOT happen.
+    expect(fakeFetch).toHaveBeenCalledTimes(1);
+  });
+
+  it('follows a public-to-public redirect and returns the final body', async () => {
+    const fakeFetch = vi
+      .fn<typeof fetch>()
+      .mockResolvedValueOnce(redirect('https://example.org/final'))
+      .mockResolvedValueOnce(mockResponse('ok body', { contentType: 'text/plain' }));
+    const result = await executeWebFetch(
+      { url: 'https://example.com/start' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('content' in result).toBe(true);
+    if ('content' in result) {
+      expect(result.content).toBe('ok body');
+      // Final URL is reported back so the model knows where the body came from.
+      expect(result.url).toBe('https://example.org/final');
+    }
+    expect(fakeFetch).toHaveBeenCalledTimes(2);
+  });
+
+  it('bails after MAX_REDIRECTS hops with a Too many redirects error', async () => {
+    // Chain 6 redirects — one more than the loop allows. Each Location
+    // points at a distinct public host so the URL guard stays happy and
+    // we exercise the redirectCount > MAX_REDIRECTS branch specifically.
+    const fakeFetch = vi
+      .fn<typeof fetch>()
+      .mockResolvedValueOnce(redirect('https://a.example/'))
+      .mockResolvedValueOnce(redirect('https://b.example/'))
+      .mockResolvedValueOnce(redirect('https://c.example/'))
+      .mockResolvedValueOnce(redirect('https://d.example/'))
+      .mockResolvedValueOnce(redirect('https://e.example/'))
+      .mockResolvedValueOnce(redirect('https://f.example/'));
+    const result = await executeWebFetch(
+      { url: 'https://start.example/' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('error' in result).toBe(true);
+    if ('error' in result) {
+      expect(result.error).toBe('too_many_redirects');
+      expect(result.reason).toMatch(/Too many redirects/);
+    }
+  });
+
+  it('errors when a 30x response omits the Location header', async () => {
+    const fakeFetch = vi
+      .fn<typeof fetch>()
+      .mockResolvedValueOnce(redirect(null, 302));
+    const result = await executeWebFetch(
+      { url: 'https://example.com/' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('error' in result).toBe(true);
+    if ('error' in result) {
+      expect(result.error).toBe('redirect_missing_location');
+      expect(result.reason).toMatch(/no Location/);
+    }
+  });
+
+  it('resolves a relative Location against the current URL', async () => {
+    // Server sends `Location: /foo` (relative) on a request to
+    // https://example.com/path. RFC 9110 says resolve against the
+    // request URL, so the next hop is https://example.com/foo. Assert
+    // the second fetch was called with the absolute resolved URL.
+    const fakeFetch = vi
+      .fn<typeof fetch>()
+      .mockResolvedValueOnce(redirect('/foo'))
+      .mockResolvedValueOnce(mockResponse('final', { contentType: 'text/plain' }));
+    const result = await executeWebFetch(
+      { url: 'https://example.com/path' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('content' in result && result.content).toBe('final');
+    expect(fakeFetch).toHaveBeenCalledTimes(2);
+    expect(fakeFetch.mock.calls[1]![0]).toBe('https://example.com/foo');
+  });
+});
+
+// ============================================================================
+// v1.11.10: streaming body cap — abort the response stream at MAX_BYTES
+// ============================================================================
+
+// MAX_BYTES is 5 * 1024 * 1024 = 5_242_880. Repeating this here (rather
+// than importing) so a change to the cap surfaces as a test failure —
+// the limit is part of the public contract.
+const MAX_BYTES_TEST = 5 * 1024 * 1024;
+
+// Build a Response whose body is a real ReadableStream. Uses pull() (not
+// start()) so chunks are produced lazily — without backpressure, an
+// unbounded start() enqueues everything and calls controller.close()
+// before the consumer reads, which means a subsequent reader.cancel()
+// finds the stream already closed and the cancel callback never fires.
+// `cancelFlag` lets the test observe whether reader.cancel() reached the
+// underlying source mid-stream.
+function streamedResponse(
+  chunks: Uint8Array[],
+  init: { contentType?: string; contentLength?: number | null; cancelFlag?: { cancelled: boolean } } = {},
+): Response {
+  let idx = 0;
+  const stream = new ReadableStream({
+    pull(controller) {
+      if (idx >= chunks.length) {
+        controller.close();
+        return;
+      }
+      controller.enqueue(chunks[idx]!);
+      idx += 1;
+    },
+    cancel() {
+      if (init.cancelFlag) init.cancelFlag.cancelled = true;
+    },
+  });
+  const headers: Record<string, string> = {};
+  if (init.contentType) headers['content-type'] = init.contentType;
+  if (init.contentLength !== undefined && init.contentLength !== null) {
+    headers['content-length'] = String(init.contentLength);
+  }
+  return new Response(stream, { status: 200, headers });
+}
+
+describe('executeWebFetch — streaming body cap (v1.11.10)', () => {
+  it('aborts the stream when a server lies about Content-Length and emits over the cap', async () => {
+    // Honest header would have failed the pre-flight check. The lie is
+    // the point: pre-flight passes (100 < 5MB) and the streaming reader
+    // has to be the thing that catches the oversized body.
+    //
+    // Chunk count is deliberately higher than what the reader will
+    // consume (10 × 1MB available, but the reader will cancel after ~6
+    // chunks land it over 5MB). That headroom keeps the stream in
+    // 'readable' state at the moment reader.cancel() runs — otherwise
+    // a pull-then-close race could make the source close the stream
+    // before cancel reaches it, and the cancel() callback wouldn't fire.
+    const oneMB = new Uint8Array(1024 * 1024).fill(65); // 'A'
+    const tenMBInChunks = Array.from({ length: 10 }, () => oneMB);
+    const cancelFlag = { cancelled: false };
+    const fakeFetch = vi.fn().mockResolvedValue(
+      streamedResponse(tenMBInChunks, {
+        contentType: 'text/plain',
+        contentLength: 100,
+        cancelFlag,
+      }),
+    );
+    const result = await executeWebFetch(
+      { url: 'https://example.com/lying-server' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('error' in result).toBe(true);
+    if ('error' in result) {
+      expect(result.error).toBe('body_too_large');
+      expect(result.reason).toMatch(/exceeded/);
+    }
+    // Critical: reader.cancel() actually fired so the underlying
+    // connection / stream got released. Otherwise the abort would be
+    // notional and the server could keep streaming.
+    expect(cancelFlag.cancelled).toBe(true);
+  });
+
+  it('catches an oversized stream when Content-Length is omitted entirely', async () => {
+    // Many real servers (chunked transfer-encoding, dynamic responses)
+    // never send Content-Length. The pre-flight check has nothing to
+    // gate on; the streaming reader is the only line of defense.
+    // 10 chunks vs the ~6 the reader will consume — same headroom
+    // rationale as the lying-Content-Length test above.
+    const oneMB = new Uint8Array(1024 * 1024).fill(66); // 'B'
+    const tenMBInChunks = Array.from({ length: 10 }, () => oneMB);
+    const fakeFetch = vi.fn().mockResolvedValue(
+      streamedResponse(tenMBInChunks, { contentType: 'text/plain' }),
+    );
+    const result = await executeWebFetch(
+      { url: 'https://example.com/no-length' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    expect('error' in result && result.error).toBe('body_too_large');
+  });
+
+  it('passes a multi-chunk body that totals just under the cap', async () => {
+    // Boundary case: MAX_BYTES - 1 bytes split across N chunks. The
+    // streaming reader's `total > maxBytes` check is strict-greater so
+    // exactly MAX_BYTES would still succeed; MAX_BYTES + 1 would fail.
+    // - 1 leaves clear headroom without coinciding with the boundary.
+    const targetTotal = MAX_BYTES_TEST - 1;
+    const chunkSize = 256 * 1024; // 256 KiB chunks
+    const chunks: Uint8Array[] = [];
+    let remaining = targetTotal;
+    while (remaining > 0) {
+      const size = Math.min(chunkSize, remaining);
+      chunks.push(new Uint8Array(size).fill(67)); // 'C'
+      remaining -= size;
+    }
+    const fakeFetch = vi.fn().mockResolvedValue(
+      streamedResponse(chunks, { contentType: 'text/plain' }),
+    );
+    const result = await executeWebFetch(
+      { url: 'https://example.com/right-at-cap' },
+      fakeFetch as unknown as typeof fetch,
+    );
+    // The streaming reader succeeded — we got a content shape, not an
+    // error. (Downstream truncate() will clamp the final string to
+    // MAX_CHARS_CAP=32000 and set truncated:true; that's the existing
+    // truncation logic and is exercised by its own test. The point of
+    // THIS test is that readBodyCapped didn't trip on a body that
+    // sits just under its byte limit.)
+    expect('content' in result).toBe(true);
+    if ('content' in result) {
+      expect(result.content.length).toBeGreaterThan(0);
+      // All ASCII 'C's, so the leading 200 chars before any truncation
+      // marker should be all C — proves we read real bytes through the
+      // streaming reader rather than getting an empty buffer.
+      expect(result.content.slice(0, 200)).toBe('C'.repeat(200));
+    }
+  });
+});

apps/server/src/services/web_fetch.ts

@@ -22,6 +22,9 @@ const DEFAULT_MAX_CHARS = 8_000;
 const MAX_CHARS_CAP = 32_000;
 const FETCH_TIMEOUT_MS = 15_000;
 const MAX_BYTES = 5 * 1024 * 1024;
+// v1.11.9: cap redirect chains. Each hop re-runs isPublicUrl on the
+// resolved target so a public-IP origin can't 302 us into a private IP.
+const MAX_REDIRECTS = 5;
 
 // Output shape. Each variant uses a discriminator the LLM can branch on.
 export type WebFetchOutput =
@@ -59,6 +62,39 @@ function stripHtml(html: string): { text: string; title: string | undefined } {
   return { text, title };
 }
 
+// v1.11.10: streaming body reader. Aborts the response stream the instant
+// cumulative bytes cross maxBytes, so a server that lies about
+// Content-Length (or omits it entirely) can't make us buffer gigabytes
+// before the post-read check fires. reader.cancel() releases the
+// underlying connection on the spot.
+async function readBodyCapped(
+  res: Response,
+  maxBytes: number,
+): Promise<{ ok: true; body: string } | { ok: false; bytesRead: number }> {
+  if (!res.body) return { ok: true, body: '' };
+  const reader = res.body.getReader();
+  const chunks: Uint8Array[] = [];
+  let total = 0;
+  try {
+    while (true) {
+      const { done, value } = await reader.read();
+      if (done) break;
+      total += value.byteLength;
+      if (total > maxBytes) {
+        // Best-effort cancel — surfaces on the server side as a closed
+        // connection and (in our tests) fires the ReadableStream's
+        // cancel() callback so we can assert the abort happened.
+        await reader.cancel();
+        return { ok: false, bytesRead: total };
+      }
+      chunks.push(value);
+    }
+  } finally {
+    try { reader.releaseLock(); } catch { /* already released by cancel() */ }
+  }
+  return { ok: true, body: Buffer.concat(chunks).toString('utf8') };
+}
+
 function truncate(text: string, max: number): { content: string; truncated: boolean } {
   if (text.length <= max) return { content: text, truncated: false };
   const omitted = text.length - max;
@@ -74,89 +110,136 @@ export async function executeWebFetch(
   input: WebFetchInputT,
   fetcher: typeof fetch = fetch,
 ): Promise<WebFetchOutput> {
-  const guard = isPublicUrl(input.url);
-  if (!guard.ok) {
-    return { error: 'blocked_by_url_guard', reason: guard.reason ?? 'unknown' };
-  }
-
   const maxChars = Math.min(input.max_chars ?? DEFAULT_MAX_CHARS, MAX_CHARS_CAP);
 
-  const controller = new AbortController();
+  // v1.11.9: manual redirect handling. `redirect: 'follow'` in fetch
-  const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS);
+  // doesn't expose intermediate hops — a public-IP origin that 302s us
-  try {
+  // to 169.254.169.254 would silently bypass isPublicUrl. We follow each
-    const res = await fetcher(input.url, {
+  // hop ourselves, re-running the URL guard on the resolved target so a
-      signal: controller.signal,
+  // mid-chain hostile redirect gets blocked.
-      // TODO(v1.11.9): redirect: 'manual' + re-run isPublicUrl on Location header.
+  //
-      // Current 'follow' allows redirect-to-private-IP bypass of URL guard.
+  // Timeout semantics changed from v1.11.8: AbortSignal.timeout fires
-      redirect: 'follow',
+  // per fetch hop (vs. one 15s budget shared across the whole call). In
-      headers: { 'User-Agent': 'BooCode/1.11.8', Accept: 'text/html,text/plain,application/json,*/*' },
+  // the worst case a 5-hop chain can take ~5×15s before erroring — still
-    });
+  // bounded; trades a longer cap for simpler code.
-    if (!res.ok) {
+  let currentUrl = input.url;
-      return { error: 'upstream_status', reason: `HTTP ${res.status}` };
+  let res: Response | undefined;
-    }
+  let redirectCount = 0;
-    // Pre-flight size check via Content-Length when the server provides it.
-    const lenHeader = res.headers.get('content-length');
-    if (lenHeader) {
-      const len = Number(lenHeader);
-      if (Number.isFinite(len) && len > MAX_BYTES) {
-        return { error: 'response_too_large', reason: `Content-Length ${len} > ${MAX_BYTES}` };
-      }
-    }
-    const contentType = (res.headers.get('content-type') ?? '').toLowerCase();
-    // Read body. We rely on the 5MB cap by checking length after consumption
-    // — most malicious or accidental large responses also exceed it via the
-    // Content-Length pre-flight above. A truly hostile server that lies
-    // about length AND streams gigabytes would defeat that; for v1.11.8
-    // the 15s timeout is the secondary fence.
-    const body = await res.text();
-    // v1.11.8 review: byte-count, not char-count. A 5MB cap on
-    // body.length (UTF-16 code units) lets a multi-byte payload (emoji,
-    // CJK) pass when its wire size already exceeded MAX_BYTES. Compute
-    // once and reuse for the error message.
-    const bodyBytes = Buffer.byteLength(body, 'utf8');
-    if (bodyBytes > MAX_BYTES) {
-      return { error: 'response_too_large', reason: `body ${bodyBytes} bytes > ${MAX_BYTES}` };
-    }
 
-    let textRaw: string;
+  while (true) {
-    let title: string | undefined;
+    const guard = isPublicUrl(currentUrl);
-    if (contentType.includes('text/html') || contentType.includes('application/xhtml')) {
+    if (!guard.ok) {
-      const stripped = stripHtml(body);
-      textRaw = stripped.text;
-      title = stripped.title;
-    } else if (
-      contentType.includes('text/plain') ||
-      contentType.includes('text/markdown') ||
-      contentType.includes('application/json') ||
-      contentType.includes('text/xml') ||
-      contentType.includes('application/xml')
-    ) {
-      textRaw = body;
-    } else {
       return {
-        error: 'unsupported_content_type',
+        error: 'blocked_by_url_guard',
-        reason: `content-type ${contentType || '(none)'} not supported`,
+        reason: redirectCount === 0
-        content_type: contentType,
+          ? (guard.reason ?? 'unknown')
+          : `redirect target ${currentUrl} blocked: ${guard.reason ?? 'unknown'}`,
       };
     }
 
-    const truncated = truncate(textRaw, maxChars);
+    try {
-    return {
+      res = await fetcher(currentUrl, {
-      url: input.url,
+        method: 'GET',
-      title,
+        redirect: 'manual',
-      content: truncated.content,
+        signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
-      content_type: contentType,
+        headers: {
-      truncated: truncated.truncated,
+          'User-Agent': 'BooCode/1.11.9',
-    };
+          Accept: 'text/html,text/plain,application/json,*/*',
-  } catch (err) {
+        },
-    const msg = err instanceof Error ? err.message : String(err);
+      });
-    if (err instanceof Error && err.name === 'AbortError') {
+    } catch (err) {
-      return { error: 'timeout', reason: `aborted after ${FETCH_TIMEOUT_MS}ms` };
+      const msg = err instanceof Error ? err.message : String(err);
+      // AbortSignal.timeout fires a DOMException with name 'TimeoutError';
+      // older runtimes / polyfills may surface 'AbortError'. Treat both.
+      if (err instanceof Error && (err.name === 'TimeoutError' || err.name === 'AbortError')) {
+        return { error: 'timeout', reason: `aborted after ${FETCH_TIMEOUT_MS}ms` };
+      }
+      return { error: 'fetch_failed', reason: msg };
     }
-    return { error: 'fetch_failed', reason: msg };
+
-  } finally {
+    if (res.status >= 300 && res.status < 400) {
-    clearTimeout(timer);
+      const loc = res.headers.get('location');
+      if (!loc) {
+        return {
+          error: 'redirect_missing_location',
+          reason: `${res.status} redirect with no Location header`,
+        };
+      }
+      redirectCount += 1;
+      if (redirectCount > MAX_REDIRECTS) {
+        return {
+          error: 'too_many_redirects',
+          reason: `Too many redirects (exceeded ${MAX_REDIRECTS} hops)`,
+        };
+      }
+      // Resolve relative Location against the URL we just hit (RFC 9110).
+      // The next loop iteration re-runs isPublicUrl on the new currentUrl.
+      currentUrl = new URL(loc, currentUrl).toString();
+      continue;
+    }
+    break;
   }
+
+  if (!res.ok) {
+    return { error: 'upstream_status', reason: `HTTP ${res.status}` };
+  }
+  // Pre-flight size check via Content-Length when the server provides it.
+  const lenHeader = res.headers.get('content-length');
+  if (lenHeader) {
+    const len = Number(lenHeader);
+    if (Number.isFinite(len) && len > MAX_BYTES) {
+      return { error: 'response_too_large', reason: `Content-Length ${len} > ${MAX_BYTES}` };
+    }
+  }
+  const contentType = (res.headers.get('content-type') ?? '').toLowerCase();
+  // v1.11.10: stream the body with a hard byte cap. Previously we read
+  // res.text() in one shot and then byte-length-checked — a server that
+  // lies about Content-Length (or omits it) could make us buffer
+  // gigabytes before the post-check fired. readBodyCapped aborts the
+  // stream the instant total bytes cross MAX_BYTES. The Content-Length
+  // pre-flight above stays as a cheap early reject for honest servers.
+  const read = await readBodyCapped(res, MAX_BYTES);
+  if (!read.ok) {
+    return {
+      error: 'body_too_large',
+      reason: `Response body exceeded ${MAX_BYTES} bytes (read ${read.bytesRead} before abort)`,
+    };
+  }
+  const body = read.body;
+
+  let textRaw: string;
+  let title: string | undefined;
+  if (contentType.includes('text/html') || contentType.includes('application/xhtml')) {
+    const stripped = stripHtml(body);
+    textRaw = stripped.text;
+    title = stripped.title;
+  } else if (
+    contentType.includes('text/plain') ||
+    contentType.includes('text/markdown') ||
+    contentType.includes('application/json') ||
+    contentType.includes('text/xml') ||
+    contentType.includes('application/xml')
+  ) {
+    textRaw = body;
+  } else {
+    return {
+      error: 'unsupported_content_type',
+      reason: `content-type ${contentType || '(none)'} not supported`,
+      content_type: contentType,
+    };
+  }
+
+  const truncated = truncate(textRaw, maxChars);
+  // Report the FINAL URL (post-redirects) so the LLM knows where the body
+  // came from — useful for citations and for the model to reason about
+  // domain trust.
+  return {
+    url: currentUrl,
+    title,
+    content: truncated.content,
+    content_type: contentType,
+    truncated: truncated.truncated,
+  };
 }
 
 export const webFetch: ToolDef<WebFetchInputT> = {