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