From 6e89eb9ec1eb5e797e015b8868c75e802035ffb1 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 9 Aug 2026 04:06:23 +0200 Subject: [PATCH] fix(web-tabs): bound time-to-headers, not the whole proxied exchange (#237, #238) Reported by @DodgyBadger. #237: the proxy wrapped each upstream fetch in a 30s `AbortSignal.timeout`, which bounded the ENTIRE exchange rather than the wait for response headers. A dashboard endpoint doing model inference, and any actively streaming response, both died at 30s as a generic 502 that Codeman never logged, so it read as an intermittent network error. The timeout now bounds time-to-headers only and is cleared the moment headers arrive, so a slow endpoint and a long stream both survive. The default moves to 300s because "the app is thinking" is normal for the dashboards people proxy; abandoned upstreams are reclaimed by the client-hangup abort rather than by this value. A browser that navigates away mid-request now aborts the upstream fetch, guarded by `writableFinished` for the same reason as `abortOnClientHangUp` in session-routes: `close` also fires after a completed response and must not abort anything. Header timeouts are logged as a warning with a sanitized identity (method plus origin plus path, never the query string, which can carry the dashboard's tokens), and a client hangup is deliberately not warned since nobody is listening and it would read as the dashboard being broken. The WebSocket handshake keeps its own 30s budget (`CODEMAN_WEBVIEW_WS_HANDSHAKE_TIMEOUT_MS`), decoupled from the request timeout: a handshake is connection establishment, and waiting minutes on one only delays the browser's reconnect logic. #238: the web-tab guide covered sandboxed dashboards having no cookies, but not cookie authentication in front of Codeman itself (Cloudflare Access and similar), where a sandboxed frame's asset and API requests carry no auth cookie, bounce to the login provider, and leave the embedded app looking unstyled or broken while trusted mode works. Documented, and the Test button's result now says it probes server-to-upstream reachability only, not how the page behaves in a sandboxed frame. Co-Authored-By: Claude Opus 5 (1M context) --- docs/web-tabs.md | 26 +++++- src/config/webview-limits.ts | 21 ++++- src/web/public/index.html | 5 +- src/web/public/webview-tabs.js | 8 +- src/web/routes/webview-routes.ts | 59 +++++++++++- test/webview-proxy-timeout.test.ts | 141 +++++++++++++++++++++++++++++ 6 files changed, 252 insertions(+), 8 deletions(-) create mode 100644 test/webview-proxy-timeout.test.ts diff --git a/docs/web-tabs.md b/docs/web-tabs.md index 28438727..2560218d 100644 --- a/docs/web-tabs.md +++ b/docs/web-tabs.md @@ -46,7 +46,11 @@ Codeman, including a phone that is not on the tailnet. `direct` mode (a plain cross-origin iframe) still exists and is cheaper, but it only works for an HTTPS dashboard that permits framing. The **Test** button probes from -the server and tells you which mode applies. +the server and tells you which mode applies. Note what Test actually verifies: +**server-to-upstream reachability, nothing else**. It does not exercise the browser +sandbox, cookies, CORS, CSP, or any reverse proxy sitting in front of Codeman, so a +passing Test does not guarantee the embedded page will render (see the +cookie-authenticated reverse proxy caveat below). ## The sandbox, and when to turn it off @@ -66,6 +70,17 @@ Even in trusted mode, Codeman never forwards its own credentials upstream: the `Authorization` header and the `codeman_session` cookie are stripped on the way out, so `CODEMAN_PASSWORD` cannot leak into a dashboard. +⚠️ **Sandboxed tabs may not work when Codeman itself is behind a +cookie-authenticated reverse proxy** (Cloudflare Access, Authelia, oauth2-proxy and +similar). The sandboxed frame is opaque-origin, so its stylesheet, script, and API +requests do not carry the proxy's authentication cookie; the proxy redirects them to +the login provider, where CORS/CSP kills them, and the embedded app renders +unstyled or broken while the Codeman page around it works fine. Trusted mode +(**Open sandboxed** off) keeps a real origin and the cookie, so it works. The +**Test** button cannot catch this: it checks that the Codeman *server* can reach the +upstream, not that a sandboxed *browser* frame can load assets through the public +authentication layer. + ## How the proxy authenticates A sandboxed iframe is opaque-origin, so every request it makes is cross-site: the @@ -137,6 +152,15 @@ then every API call fails, which looks like the dashboard being broken. - **Login-protected dashboards need trusted mode**, since a sandboxed frame has no cookie jar. A server-side per-dashboard cookie jar would lift this and is the natural next step if it becomes annoying. +- **Cookie-authenticated reverse proxies in front of Codeman break sandboxed tabs** + (#238). The sandboxed frame's requests carry no auth cookie, so the proxy bounces + them to its login provider and the app loads broken while Test reports reachable. + Use trusted mode behind Cloudflare Access and friends; see the warning above. +- **Slow endpoints and the upstream timeout** (#237). The proxy waits + `CODEMAN_WEBVIEW_TIMEOUT_MS` (default 300s) for the upstream's response *headers*, + then streams the body without any time bound; a header timeout is logged + server-side and answered as a 502 that names the limit. WebSocket handshakes use + the separate `CODEMAN_WEBVIEW_WS_HANDSHAKE_TIMEOUT_MS` (default 30s). - **Not a security boundary.** The proxy reaches whatever the Codeman server can reach. That is not an escalation for someone who already commands `--dangerously-skip-permissions` agents, but in multi-user mode it does mean a diff --git a/src/config/webview-limits.ts b/src/config/webview-limits.ts index 9fe546d3..8524cdb0 100644 --- a/src/config/webview-limits.ts +++ b/src/config/webview-limits.ts @@ -29,12 +29,29 @@ export const WEBVIEW_CAPABILITY_TTL_MS = envInt('CODEMAN_WEBVIEW_CAPABILITY_TTL_ /** Max concurrent capabilities held in memory before the oldest are dropped. */ export const MAX_WEBVIEW_CAPABILITIES = 200; -/** Upstream request timeout for a proxied HTTP request. */ -export const WEBVIEW_UPSTREAM_TIMEOUT_MS = envInt('CODEMAN_WEBVIEW_TIMEOUT_MS', 30_000); +/** + * How long a proxied HTTP request waits for the upstream's RESPONSE HEADERS. + * + * This bounds time-to-headers only, never an actively streaming body: the proxy + * clears the timer the moment headers arrive (issue #237: the old 30s + * `AbortSignal.timeout` bounded the whole fetch and killed slow AI/model endpoints + * and long streams alike, as a silent 502). 300s because "the app is thinking" is + * normal for the dashboards people proxy; abandoned upstreams are reclaimed by the + * client-hangup abort, not by this value, so a generous default costs nothing. + */ +export const WEBVIEW_UPSTREAM_TIMEOUT_MS = envInt('CODEMAN_WEBVIEW_TIMEOUT_MS', 300_000); /** Shorter timeout for the editor's "Test" probe, which a human is waiting on. */ export const WEBVIEW_PROBE_TIMEOUT_MS = envInt('CODEMAN_WEBVIEW_PROBE_TIMEOUT_MS', 8_000); +/** + * WebSocket upgrade handshake timeout. Deliberately decoupled from + * WEBVIEW_UPSTREAM_TIMEOUT_MS: a handshake is connection establishment, and waiting + * minutes on one only delays the browser's reconnect logic. Matches the pre-#237 + * behavior (the handshake used to ride the 30s upstream timeout). + */ +export const WEBVIEW_WS_HANDSHAKE_TIMEOUT_MS = envInt('CODEMAN_WEBVIEW_WS_HANDSHAKE_TIMEOUT_MS', 30_000); + /** * Max bytes of an HTML response buffered for `` injection and link * rewriting. Larger HTML documents stream through untouched: the rewrite is a diff --git a/src/web/public/index.html b/src/web/public/index.html index aca5bac5..48cf978f 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -708,7 +708,10 @@ Recommended. A proxied dashboard is served from Codeman's own address, so unchecking this lets its JavaScript read this page and call the API that starts agents. Uncheck - only for a dashboard you fully trust, or one whose own login needs cookies. + only for a dashboard you fully trust, or one whose own login needs cookies. Also + uncheck it if Codeman itself sits behind a cookie-authenticated reverse proxy + (e.g. Cloudflare Access): a sandboxed frame carries no auth cookie, so its asset + and API requests bounce to the login provider and the page loads broken.
diff --git a/src/web/public/webview-tabs.js b/src/web/public/webview-tabs.js index 0962d469..eb58ab87 100644 --- a/src/web/public/webview-tabs.js +++ b/src/web/public/webview-tabs.js @@ -396,9 +396,13 @@ Object.assign(CodemanApp.prototype, { out.textContent = 'Test failed (invalid URL?).'; return; } + // #238: the probe runs server-to-upstream; say so, or a passing Test reads as + // "the embedded page will work" when the browser sandbox / a cookie-auth + // reverse proxy in front of Codeman can still break it. out.textContent = probe.reachable - ? `Reachable (HTTP ${probe.status}). ${probe.reason}` - : `Not reachable. ${probe.reason}`; + ? `Reachable (HTTP ${probe.status}) from the Codeman server. ${probe.reason} ` + + `(Tests server-to-upstream reachability only, not how the page behaves in a sandboxed frame.)` + : `Not reachable from the Codeman server. ${probe.reason}`; out.className = 'form-hint webview-probe-result ' + (probe.reachable ? 'ok' : 'bad'); }, diff --git a/src/web/routes/webview-routes.ts b/src/web/routes/webview-routes.ts index 6421b7a5..16eac81b 100644 --- a/src/web/routes/webview-routes.ts +++ b/src/web/routes/webview-routes.ts @@ -43,6 +43,7 @@ import { WEBVIEW_PROBE_TIMEOUT_MS, WEBVIEW_PROXY_PREFIX, WEBVIEW_UPSTREAM_TIMEOUT_MS, + WEBVIEW_WS_HANDSHAKE_TIMEOUT_MS, } from '../../config/webview-limits.js'; import { readWebviews, writeWebviews } from '../../webview-store.js'; import { webviewCapabilities } from '../../webview-capabilities.js'; @@ -420,6 +421,34 @@ async function proxyRequest( refererPath: typeof req.headers.referer === 'string' ? stripProxyPrefix(req.headers.referer, cap) : undefined, }); + // #237: the timeout bounds TIME-TO-HEADERS only. A plain AbortSignal.timeout on + // the fetch bounded the entire exchange, so a legitimately slow endpoint (AI + // inference behind the dashboard) and an actively streaming response both died at + // 30s as an unlogged generic 502. The timer is cleared the moment headers arrive; + // what reclaims an abandoned upstream afterwards is the client hangup below. + const startedAt = Date.now(); + const abort = new AbortController(); + let headerTimedOut = false; + let clientGone = false; + const headerTimer = setTimeout(() => { + headerTimedOut = true; + abort.abort(); + }, WEBVIEW_UPSTREAM_TIMEOUT_MS); + // A browser that navigates away mid-request (or mid-stream) must abort the + // upstream fetch, or slow endpoints accumulate as orphaned upstream sockets. + // Guarded by writableFinished, same as abortOnClientHangUp in session-routes: + // `close` also fires after a completed response, which must not abort anything. + reply.raw.on('close', () => { + if (!reply.raw.writableFinished) { + clientGone = true; + abort.abort(); + } + }); + + // Sanitized request identity for logs: method + origin + path, never the query + // string (it can carry the dashboard's tokens). + const logTarget = `${req.method} ${upstream.origin}${upstream.pathname}`; + let response: Response; try { response = await fetch(upstream.href, { @@ -431,11 +460,37 @@ async function proxyRequest( // Redirects are rewritten into the proxy prefix instead of followed, so the // browser's URL stays inside the frame and relative assets keep resolving. redirect: 'manual', - signal: AbortSignal.timeout(WEBVIEW_UPSTREAM_TIMEOUT_MS), + signal: abort.signal, } as RequestInit); } catch (err) { + const elapsed = Date.now() - startedAt; + if (clientGone) { + // Nobody is listening; the abort was ours and intentional. Not an upstream + // failure, so no warn (it would read as the dashboard being broken). + return reply; + } + if (headerTimedOut) { + console.warn( + `[Webview] upstream sent no response headers within ${WEBVIEW_UPSTREAM_TIMEOUT_MS}ms: ` + + `${logTarget} (webview "${webview.name}")` + ); + return reply + .code(502) + .type('text/plain') + .send( + `Dashboard unreachable: upstream sent no response headers within ${WEBVIEW_UPSTREAM_TIMEOUT_MS}ms ` + + `(CODEMAN_WEBVIEW_TIMEOUT_MS raises this limit)` + ); + } const message = err instanceof Error ? err.message : String(err); + console.warn( + `[Webview] upstream fetch failed after ${elapsed}ms: ${logTarget} (webview "${webview.name}"): ${message}` + ); return reply.code(502).type('text/plain').send(`Dashboard unreachable: ${message}`); + } finally { + // Headers arrived (or the fetch failed): from here on the timeout must never + // fire, a streaming body is allowed to take as long as it takes. + clearTimeout(headerTimer); } const secureContext = req.protocol === 'https'; @@ -581,7 +636,7 @@ function proxyWebSocket(socket: WebSocket, req: FastifyRequest<{ Params: ProxyPa origin: upstream.origin, ...(webview.trusted && req.headers.cookie ? { cookie: String(req.headers.cookie) } : {}), }, - handshakeTimeout: WEBVIEW_UPSTREAM_TIMEOUT_MS, + handshakeTimeout: WEBVIEW_WS_HANDSHAKE_TIMEOUT_MS, } ); diff --git a/test/webview-proxy-timeout.test.ts b/test/webview-proxy-timeout.test.ts new file mode 100644 index 00000000..6ce50dbc --- /dev/null +++ b/test/webview-proxy-timeout.test.ts @@ -0,0 +1,141 @@ +/** + * @fileoverview Issue #237: the webview proxy's upstream timeout bounds + * TIME-TO-HEADERS only, is logged when it fires, and never kills a response that + * is actively streaming. + * + * Uses app.inject() against the real registerWebviewRoutes with a real local + * upstream http server on an ephemeral port (inject fakes only the inbound + * request; the proxy's outbound fetch is real). Port: ephemeral (server.listen(0)). + * + * The timeout is shrunk via CODEMAN_WEBVIEW_TIMEOUT_MS inside vi.hoisted(), which + * runs before the module graph loads (webview-limits reads the env at import). + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +import { createServer, type Server } from 'node:http'; + +const TIMEOUT_MS = vi.hoisted(() => { + process.env.CODEMAN_WEBVIEW_TIMEOUT_MS = '400'; + return 400; +}); + +import { registerWebviewRoutes } from '../src/web/routes/webview-routes.js'; +import { webviewCapabilities } from '../src/webview-capabilities.js'; +import { writeWebviews } from '../src/webview-store.js'; +import { getDataDir } from '../src/config/instance.js'; +import type { EventPort } from '../src/web/ports/index.js'; + +const WEBVIEW_ID = 'wv-timeout-test'; + +let upstream: Server; +let upstreamPort: number; +let app: FastifyInstance; +let capability: string; + +const eventPortStub: EventPort = { + broadcast: vi.fn(), + sendPushNotifications: vi.fn(), + batchTerminalData: vi.fn(), + broadcastSessionStateDebounced: vi.fn(), + batchTaskUpdate: vi.fn(), + getSseClientCount: vi.fn(() => 0), +}; + +beforeAll(async () => { + upstream = createServer((req, res) => { + if (req.url === '/fast') { + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('quick'); + return; + } + if (req.url === '/slow-headers') { + // Headers arrive AFTER the proxy's limit: this is the #237 repro shape + // (an API endpoint thinking for longer than the timeout). + setTimeout(() => { + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('finally'); + }, TIMEOUT_MS + 700); + return; + } + if (req.url === '/stream') { + // Headers immediately, then a body that takes ~3x the limit to finish. + // Under the old whole-fetch AbortSignal.timeout this died mid-stream. + res.writeHead(200, { 'content-type': 'text/plain' }); + res.write('start;'); + let chunks = 0; + const timer = setInterval(() => { + chunks++; + res.write(`chunk${chunks};`); + if (chunks >= 4) { + clearInterval(timer); + res.end('done'); + } + }, TIMEOUT_MS * 0.75); + return; + } + res.writeHead(404).end(); + }); + await new Promise((resolve) => upstream.listen(0, '127.0.0.1', resolve)); + const address = upstream.address(); + if (typeof address === 'object' && address) upstreamPort = address.port; + + await writeWebviews(getDataDir(), [ + { + id: WEBVIEW_ID, + name: 'timeout-under-test', + url: `http://127.0.0.1:${upstreamPort}/`, + embedMode: 'proxy', + trusted: false, + createdAt: Date.now(), + }, + ]); + capability = webviewCapabilities.mint(WEBVIEW_ID, undefined); + + app = Fastify({ logger: false }); + registerWebviewRoutes(app, eventPortStub); + await app.ready(); +}); + +afterAll(async () => { + await app.close(); + webviewCapabilities.revokeWebview(WEBVIEW_ID); + await new Promise((resolve) => upstream.close(() => resolve())); +}); + +describe('webview proxy upstream timeout (#237)', () => { + it('proxies a fast request untouched', async () => { + const res = await app.inject({ method: 'GET', url: `/webview/${capability}/fast` }); + expect(res.statusCode).toBe(200); + expect(res.body).toBe('quick'); + }); + + it('502s with an actionable, logged message when headers never arrive in time', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const res = await app.inject({ method: 'GET', url: `/webview/${capability}/slow-headers` }); + expect(res.statusCode).toBe(502); + // The body names the limit and the env var that raises it; the old message + // was an opaque "The operation was aborted due to timeout". + expect(res.body).toContain(`no response headers within ${TIMEOUT_MS}ms`); + expect(res.body).toContain('CODEMAN_WEBVIEW_TIMEOUT_MS'); + // And it is logged server-side (the old path was completely silent), with + // the sanitized target and the webview's name. + const logged = warn.mock.calls.map((args) => args.join(' ')).join('\n'); + expect(logged).toContain('no response headers'); + expect(logged).toContain(`/slow-headers`); + expect(logged).toContain('timeout-under-test'); + } finally { + warn.mockRestore(); + } + }); + + it('never aborts a response that is actively streaming past the timeout', async () => { + const started = Date.now(); + const res = await app.inject({ method: 'GET', url: `/webview/${capability}/stream` }); + expect(res.statusCode).toBe(200); + expect(res.body).toBe('start;chunk1;chunk2;chunk3;chunk4;done'); + // Sanity: the exchange really did outlive the header timeout. + expect(Date.now() - started).toBeGreaterThan(TIMEOUT_MS); + }); +});