diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index a50539c8..2cca18e9 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -541,9 +541,9 @@ Further detail on the Docker Compose supervisor (`docker-compose`), see also `do 1. **Root-absolute URLs built at runtime.** `` only governs URLs the HTML parser resolves; `fetch('/api/data')` bypasses it and lands on Codeman's root. That is how most dashboards talk to their own backend. The `Referer`-keyed 404 fallback is only a rescue (it fires after every real route missed, and only when the browser sends a usable `Referer`), so the fix is `runtimeUrlShim()`: a small script injected right after `` that rebases root-absolute and same-origin-absolute URLs into the prefix. It removes the whole class inside the iframe instead of trading security for it. ⚠️ It must be injected even when the page ships its OWN `` (an early return there silently breaks exactly the pages that need it most). ⚠️ **The DOM sinks are as load-bearing as `fetch`.** Patching only `fetch`/`XHR`/`WebSocket`/`EventSource` leaves `container.innerHTML = ''` and `img.src = '/api/slide'` untouched, and neither of the other layers can reach those either (`` never applies to root-absolute URLs, and `rewriteHtml()` only ever sees the INITIAL document, never markup built later by page script). The symptom is precise and easy to misdiagnose as an upstream fault: the dashboard's **data** loads while every **image** stays broken. So the shim also wraps `innerHTML`/`outerHTML`/`insertAdjacentHTML`, `setAttribute`/`setAttributeNS`, and the `src`/`srcset`/`href`/`poster`/`data`/`action` property setters, with a `MutationObserver` as a last net for sinks not patched above. Every rewrite routes through the same idempotent `rw()`, which matters because unlike the server-side rewrite this one sees markup that may ALREADY be proxied (a page re-injecting its own `outerHTML` would otherwise double-prefix). The DOM half is pinned in jsdom by `test/webview-proxy.test.ts`; `curl` cannot see any of it. 2. **CORS on same-host requests.** An opaque-origin document treats EVERY request as cross-origin, including to the very host it was served from, so its `fetch`/XHR are CORS-checked and its preflights carry `Origin: null`. Static subresources (script/css/img) are NOT CORS-checked, which is why the page renders while its API calls die with an opaque `net::ERR_FAILED`. `buildProxyCorsHeaders()` echoes the origin (omitting `allow-credentials` for `null`, which browsers reject in combination), upstream `access-control-*` headers are dropped (they describe the dashboard's origin, not the frame's), and the proxy answers preflights itself rather than relaying them. ⚠️ `registerSecurityHeaders` answers EVERY `OPTIONS` with a bare 204 before routing, and its CORS block only emits headers for localhost origins, so that short-circuit **must** exempt a valid webview capability or every preflight fails. `curl` cannot reproduce any of this because curl does not enforce CORS. -**Rewrites, each load-bearing** (pure + unit-tested in `src/web/webview-proxy.ts`): drop `x-frame-options` and the CSP `frame-ancestors` directive (the point of the proxy); drop `content-encoding`/`content-length` because undici's `fetch` already decoded the body (forwarding them makes the browser gunzip plaintext); rewrite `Location` for same-origin redirects only, handing CROSS-origin redirects back unchanged so this never becomes an open relay; rebase `Set-Cookie` `Path` onto the prefix and drop `Domain`; inject `` and rebase root-absolute `src`/`href`/`action`. `resolveUpstreamUrl()` returns null on anything escaping the upstream origin. +**Rewrites, each load-bearing** (pure + unit-tested in `src/web/webview-proxy.ts`): drop `x-frame-options` and the CSP `frame-ancestors` directive (the point of the proxy); drop the response's `content-encoding`/`content-length` because undici's `fetch` already decoded the body (forwarding them makes the browser gunzip plaintext); relay the REQUEST's `content-length` whenever a body goes upstream (`relayBodyLength`, no `transfer-encoding` present), see Fastify specifics; rewrite `Location` for same-origin redirects only, handing CROSS-origin redirects back unchanged so this never becomes an open relay; rebase `Set-Cookie` `Path` onto the prefix and drop `Domain`; inject `` and rebase root-absolute `src`/`href`/`action`. `resolveUpstreamUrl()` returns null on anything escaping the upstream origin. -**Fastify specifics.** The proxy lives in an **encapsulated plugin scope** with `removeAllContentTypeParsers()` + a `'*'` pass-through parser, so raw bodies relay byte-for-byte while the root instance keeps its JSON parsing (and keeps `text/plain` RAW, which was a real CSRF hole once). One `app.route({ method:'GET', handler, wsHandler })` serves both HTTP and the WebSocket upgrade; **HEAD must NOT be declared** on the sibling route because `exposeHeadRoutes` already derives it and the duplicate is a startup error. ⚠️ **Every exit path in the proxy handler RETURNS `reply.send(...)`**: the handler is `async`, and a bare `return` after `reply.send(stream)` resolves the handler promise to `undefined` before the stream is consumed, so Fastify answers with an EMPTY body. HTML survives that (synchronous string payload) while every streamed asset comes back zero-length, which is a genuinely confusing failure. The root-absolute fallback hangs off `setNotFoundHandler`, so real Codeman routes always win. +**Fastify specifics.** The proxy lives in an **encapsulated plugin scope** with `removeAllContentTypeParsers()` + a `'*'` pass-through parser, so raw bodies relay byte-for-byte while the root instance keeps its JSON parsing (and keeps `text/plain` RAW, which was a real CSRF hole once). ⚠️ **Bytes, and the FRAMING too**: undici sends a stream body with no known length as `Transfer-Encoding: chunked`, and a server that reads only `Content-Length` (Python's `http.server`, many small Go/C ones) then sees an EMPTY body. That broke every POST to such a dashboard until 2026-10-21 (a WoL page's Shut down lost its `{"confirm":true}` and was refused on every click, while curl straight at it worked), so `buildUpstreamRequestHeaders` puts the browser's `content-length` back, pinned end to end by `test/webview-proxy-body-framing.test.ts` against a length-only upstream. One `app.route({ method:'GET', handler, wsHandler })` serves both HTTP and the WebSocket upgrade; **HEAD must NOT be declared** on the sibling route because `exposeHeadRoutes` already derives it and the duplicate is a startup error. ⚠️ **Every exit path in the proxy handler RETURNS `reply.send(...)`**: the handler is `async`, and a bare `return` after `reply.send(stream)` resolves the handler promise to `undefined` before the stream is consumed, so Fastify answers with an EMPTY body. HTML survives that (synchronous string payload) while every streamed asset comes back zero-length, which is a genuinely confusing failure. The root-absolute fallback hangs off `setNotFoundHandler`, so real Codeman routes always win. **Frontend** (`src/web/public/webview-tabs.js`, load order 12.5): web tabs render into `#sessionTabs` carrying `data-webview-id` instead of `data-id`, so every session-tab path (drag-and-drop, alerts, badges) skips them. ⚠️ `_renderSessionTabsImmediate()`'s `canIncremental` check must include the web-tab comparison: the session-only comparison is vacuously "unchanged" whenever session count is stable, most visibly at ZERO sessions (`0 === 0`), where opening a dashboard would never draw its tab. ⚠️ `isActive` for a session tab is `id === activeSessionId && !activeWebviewId`: `activeSessionId` stays set while a web tab is showing (the terminal keeps streaming underneath), so without that clause the debounced re-render lights two tabs at once. Frames stay MOUNTED while hidden (LRU-evicted past `maxLiveFrames`) so tab switching never reloads a dashboard. diff --git a/src/web/routes/webview-routes.ts b/src/web/routes/webview-routes.ts index 99dc8420..579aada5 100644 --- a/src/web/routes/webview-routes.ts +++ b/src/web/routes/webview-routes.ts @@ -473,6 +473,7 @@ async function proxyRequest( sessionCookieName: AUTH_COOKIE_NAME, refererPath: typeof req.headers.referer === 'string' ? stripProxyPrefix(req.headers.referer, cap, basePath) : undefined, + relayBodyLength: hasBody && req.body !== undefined, }); // #237: the timeout bounds TIME-TO-HEADERS only. A plain AbortSignal.timeout on diff --git a/src/web/webview-proxy.ts b/src/web/webview-proxy.ts index 1b707306..bc05feed 100644 --- a/src/web/webview-proxy.ts +++ b/src/web/webview-proxy.ts @@ -21,9 +21,13 @@ * * - `x-frame-options` / CSP `frame-ancestors`: dropped, else the browser refuses * to render the frame. This is the whole point of the proxy. - * - `content-encoding` / `content-length`: dropped, because undici's `fetch` - * already decoded the body. Forwarding them makes the browser try to gunzip - * plaintext. + * - `content-encoding` / `content-length`: dropped from responses, because + * undici's `fetch` already decoded the body. Forwarding them makes the browser + * try to gunzip plaintext. + * - Request `content-length`: RELAYED with a body. The body reaches undici as a + * stream, which it sends chunked unless told the length, and a server that + * reads only Content-Length (Python's `http.server`, many small Go/C ones) + * then sees an EMPTY body: every POST to such a dashboard arrived blank. * - `authorization` + the `codeman_session` cookie: stripped on the way OUT. In * trusted mode the iframe is same-origin, so the browser attaches Codeman's own * Basic-auth header and session cookie to every proxied request. Forwarding @@ -57,8 +61,10 @@ const HOP_BY_HOP = new Set([ /** * Request headers dropped on the way to the upstream. `authorization` and `cookie` - * carry Codeman's own credentials on a same-origin (trusted) frame; `host`, - * `content-length` and `accept-encoding` are recomputed by undici. + * carry Codeman's own credentials on a same-origin (trusted) frame; `host` and + * `accept-encoding` are recomputed by undici. `content-length` is NOT recomputed + * for a streamed body; it is listed here only so the copy loop never relays it + * blindly, and `buildUpstreamRequestHeaders` puts it back when a body goes with it. */ const DROP_REQUEST_HEADERS = new Set([ ...HOP_BY_HOP, @@ -305,7 +311,7 @@ export function filterCookieHeader(cookie: string | undefined, drop: string[]): export function buildUpstreamRequestHeaders( incoming: Record, upstream: URL, - opts: { forwardCookies: boolean; sessionCookieName: string; refererPath?: string } + opts: { forwardCookies: boolean; sessionCookieName: string; refererPath?: string; relayBodyLength?: boolean } ): Record { const headers: Record = {}; @@ -316,6 +322,21 @@ export function buildUpstreamRequestHeaders( headers[lower] = Array.isArray(value) ? value.join(', ') : value; } + // The body is relayed as a stream, and undici frames a stream it has no length + // for as `Transfer-Encoding: chunked`. Hand it the browser's own length so the + // upstream sees the framing the browser sent. Only alongside a body (a length + // with none would leave the upstream waiting for bytes that never come), and + // never against a transfer-encoding, which would make the length a lie. + const length = incoming['content-length']; + if ( + opts.relayBodyLength && + typeof length === 'string' && + /^\d+$/.test(length) && + incoming['transfer-encoding'] === undefined + ) { + headers['content-length'] = length; + } + if (opts.forwardCookies) { const raw = incoming['cookie']; const cookie = filterCookieHeader(Array.isArray(raw) ? raw.join('; ') : raw, [opts.sessionCookieName]); diff --git a/test/webview-proxy-body-framing.test.ts b/test/webview-proxy-body-framing.test.ts new file mode 100644 index 00000000..d821cb80 --- /dev/null +++ b/test/webview-proxy-body-framing.test.ts @@ -0,0 +1,141 @@ +/** + * @fileoverview The webview proxy relays a request body with the browser's own + * Content-Length, not as `Transfer-Encoding: chunked`. + * + * The proxy hands undici the body as a stream, and undici frames a stream with + * no known length as chunked. Servers that read only Content-Length (Python's + * `http.server`, many small Go/C ones) then see an EMPTY body: a WoL dashboard's + * "Shut down" posted `{"confirm":true}` and was refused for missing the flag on + * every click through the web tab, while curl straight at it worked. + * + * Uses app.inject() against the real registerWebviewRoutes with a real upstream + * that parses requests the way such a server does: headers, then exactly + * Content-Length bytes, and nothing else. Port: ephemeral (server.listen(0)). + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +import { createServer, type Server } from 'node:net'; + +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-body-framing-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), +}; + +interface Seen { + method: string; + body: string; + contentLength: string | null; + transferEncoding: string | null; +} + +beforeAll(async () => { + // A Content-Length-only HTTP/1.0 server. It answers with what it saw, so a + // chunked body shows up as `body: ''` exactly as it did for the dashboard. + upstream = createServer((socket) => { + let buf = Buffer.alloc(0); + const onData = (chunk: Buffer) => { + buf = Buffer.concat([buf, chunk]); + const end = buf.indexOf('\r\n\r\n'); + if (end < 0) return; + const lines = buf.subarray(0, end).toString('latin1').split('\r\n'); + const headers = new Map( + lines.slice(1).map((l) => [l.slice(0, l.indexOf(':')).trim().toLowerCase(), l.slice(l.indexOf(':') + 1).trim()]) + ); + const length = Number(headers.get('content-length') ?? 0); + if (buf.length - (end + 4) < length) return; + socket.off('data', onData); + const seen: Seen = { + method: lines[0].split(' ')[0], + body: buf.subarray(end + 4, end + 4 + length).toString(), + contentLength: headers.get('content-length') ?? null, + transferEncoding: headers.get('transfer-encoding') ?? null, + }; + const payload = JSON.stringify(seen); + socket.end( + `HTTP/1.0 200 OK\r\ncontent-type: application/json\r\ncontent-length: ${Buffer.byteLength(payload)}\r\n\r\n${payload}` + ); + }; + socket.on('data', onData); + socket.on('error', () => {}); + }); + 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: 'body-framing-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 request body framing', () => { + it('relays a JSON POST body with its Content-Length, so a length-only server reads it', async () => { + const res = await app.inject({ + method: 'POST', + url: `/webview/${capability}/api/shutdown`, + headers: { 'content-type': 'application/json' }, + payload: '{"confirm":true}', + }); + expect(res.statusCode).toBe(200); + const seen = res.json() as Seen; + expect(seen.transferEncoding).toBeNull(); + expect(seen.contentLength).toBe('16'); + expect(JSON.parse(seen.body)).toEqual({ confirm: true }); + }); + + it('keeps a large body intact under the same framing', async () => { + const big = 'x'.repeat(256 * 1024); + const res = await app.inject({ + method: 'PUT', + url: `/webview/${capability}/upload`, + headers: { 'content-type': 'application/octet-stream' }, + payload: big, + }); + const seen = res.json() as Seen; + expect(seen.contentLength).toBe(String(big.length)); + expect(seen.body).toBe(big); + }); + + it('sends no length on a GET, which carries no body', async () => { + const res = await app.inject({ method: 'GET', url: `/webview/${capability}/status` }); + const seen = res.json() as Seen; + expect(seen.method).toBe('GET'); + expect(seen.contentLength).toBeNull(); + expect(seen.transferEncoding).toBeNull(); + }); +}); diff --git a/test/webview-proxy.test.ts b/test/webview-proxy.test.ts index 224ab86d..54b3f68d 100644 --- a/test/webview-proxy.test.ts +++ b/test/webview-proxy.test.ts @@ -247,17 +247,45 @@ describe('buildUpstreamRequestHeaders', () => { expect(headers.referer).toBe('http://127.0.0.1:4000/panel'); }); - it('drops hop-by-hop and recomputed headers', () => { + it('drops hop-by-hop headers, and a length that a transfer-encoding contradicts', () => { const headers = buildUpstreamRequestHeaders( { host: 'codeman.local', connection: 'keep-alive', 'transfer-encoding': 'chunked', 'content-length': '5' }, upstream, - { forwardCookies: false, sessionCookieName: 'codeman_session' } + { forwardCookies: false, sessionCookieName: 'codeman_session', relayBodyLength: true } ); expect(headers.host).toBeUndefined(); expect(headers.connection).toBeUndefined(); expect(headers['transfer-encoding']).toBeUndefined(); expect(headers['content-length']).toBeUndefined(); }); + + // A streamed body without a length goes upstream chunked, and a dashboard that + // reads only Content-Length (Python's http.server) then sees an empty body. + it('relays the browser content-length alongside a body', () => { + const headers = buildUpstreamRequestHeaders( + { 'content-length': '16', 'content-type': 'application/json' }, + upstream, + { forwardCookies: false, sessionCookieName: 'codeman_session', relayBodyLength: true } + ); + expect(headers['content-length']).toBe('16'); + }); + + it('never relays a content-length without a body, or one that is not a plain number', () => { + const opts = { forwardCookies: false, sessionCookieName: 'codeman_session' }; + expect(buildUpstreamRequestHeaders({ 'content-length': '16' }, upstream, opts)['content-length']).toBeUndefined(); + expect( + buildUpstreamRequestHeaders({ 'content-length': '16' }, upstream, { ...opts, relayBodyLength: false })[ + 'content-length' + ] + ).toBeUndefined(); + for (const bad of ['-1', '16, 16', ' 16', '0x10', '']) { + expect( + buildUpstreamRequestHeaders({ 'content-length': bad }, upstream, { ...opts, relayBodyLength: true })[ + 'content-length' + ] + ).toBeUndefined(); + } + }); }); describe('buildDownstreamResponseHeaders', () => {