diff --git a/agents/dewey/work/queue-74/build.md b/agents/dewey/work/queue-74/build.md index 327d1f5b8..43d208454 100644 --- a/agents/dewey/work/queue-74/build.md +++ b/agents/dewey/work/queue-74/build.md @@ -1,15 +1,66 @@ -# Row 74 (#1568): WebUI preview behind a sign-in proxy, round 1 +# Row 74 (#1568): WebUI preview behind a sign-in proxy, round 2 -Owner: Dewey. Reviewers: Darkwing and Filbert. Brief: -`docs/plans/2026-10-11_webui-preview.md`, blob `1fe76f32` (rev 487; the -change from `59fd510f` is the out-of-scope line saying Jason starts the -preview, and nothing in the code depends on it). Base `bc8e1ee7`. None of -the four files changed between the base and `refactor` HEAD when the -packet was built. +Owner: Dewey. Reviewers: Darkwing and Filbert (Filbert re-reviews the +delta). Brief: `docs/plans/2026-10-11_webui-preview.md`, blob `1fe76f32` +(rev 487; the change from `59fd510f` is the out-of-scope line saying Jason +starts the preview, and nothing in the code depends on it). Round 2 scope: +Sage's queue notes rev 521 (Filbert's findings, comment 27433) and rev 523 +(Darkwing's findings). Base `17ea0dbc`. None of the four files changed +between `bc8e1ee7` (round 1's base) and `17ea0dbc`, or between `17ea0dbc` +and `refactor` HEAD when the packet was built. -`build.patch` is `git diff --binary bc8e1ee7 -- packages/webui`. Apply it -with `git apply --index` to a detached worktree of the base; -`candidate-manifest.sha256` then checks 4/4 OK. +`build.patch` is `git diff --binary 17ea0dbc -- packages/webui`, the whole +row, not a delta on round 1. Apply it with `git apply --index` to a +detached worktree of the base; `candidate-manifest.sha256` then checks 4/4 +OK. `cli.mjs` is unchanged from round 1 (same digest). + +## Round 2 changes + +Round 1 was the packet in `ea07e19c`, candidate `67515db5…76a0`, request +comment 27428. Filbert approved it (27433); Darkwing sent findings instead +of a verdict. The delta, all in `serve.mjs`, the README and the tests: + +1. **A repeated Host is 400 `repeated Host refused`**, before any other + check, in preview mode and without it. Node keeps the first of repeated + Host headers, so Filbert's `Host: 127.0.0.1:PORT` then `Host: NAME` with + no auth header landed on the loopback path and got 200 with inbox rows. + No edge proxy sends two, and nothing else in the Console has a use for + them, so the check is not limited to preview mode. Test: Filbert's case + exactly, the reverse order, the same Host twice, each with and without + HEADER, with and without the flags. +2. **HEADER with any Host but NAME is 403 `proxy Host refused`**, with + `message: 'the proxy must forward Host unchanged'`. nginx's default + `proxy_set_header Host $proxy_host` sends the upstream's Host, which is a + loopback Host on the Console's port, so the request skipped the user + check. Darkwing's case, `Host: 127.0.0.1:PORT` with a wrong user, got + 200 on `/api/config`. The operator's browser never sends HEADER, so it + still gets through. Test: `127.0.0.1`, `localhost` and `[::1]` on the + port, with a wrong user, the right one, the header in mixed case and an + empty value, across a bus read, `/api/config`, HEAD and a POST; nothing + reaches the bus or board. This check runs before the loopback check, so + a wrong Host carrying HEADER now gets `proxy Host refused` rather than + `non-local Host refused`; both are 403. +3. **Tests only** (the code already did these): HEAD unsigned is 401 and + signed is 200; HEAD is refused wherever GET is refused for Host and + Origin; `x.NAME` and `xNAME` are refused; `--auth-header` refuses + `remote_user`, `_` and `-remote-user`. The comma-joined value Sage + skipped as equivalent is not added. `send()` now leaves an empty HEAD + body unparsed, and a raw-socket helper sends the two Host lines + `node:http` won't. +4. **README**: the general security line names the repeated-Host 400; the + preview bullets name the loopback forms, the proxy-Host 403 and the + repeated Host; the proxy contract says Host is forwarded unchanged, + names nginx's `proxy_set_header Host $host;` (Caddy and Traefik keep it + by default), says any loopback Host on the port is the operator path, + `localhost` included, so a tunnel whose local end uses the same port + number reaches it, and binds every hop on this host that reaches the + loopback port, not only the edge. +5. **Kept**: the `NAME:443` refusal (both reviewers). The `X-Authentik-Uid` + mixed-case test stays (Sage, rev 523). + +Not in round 2, by Sage's scope: Filbert's N4 (`--allow-user` has no +leading-character check) and the wrong "Delivery may be unknown" text on a +403 Reply, which row 76 fixes before the send route opens. ## What ships @@ -30,7 +81,8 @@ refused and a POST still reaches the board). prints nothing on stdout. With all three it prints a `Preview:` line. - A request is a NAME request when its raw Host, lowercased, equals NAME exactly. Anything else goes through the existing loopback Host check - unchanged, so `NAME:443` and `NAME.evil.example` get the existing 403. + unchanged, so `NAME:443` and `NAME.evil.example` get the existing 403 + (round 2: with HEADER they get `proxy Host refused`, also 403). - The Origin, when sent, must be `https://NAME` for a NAME request and the existing `http://` for a loopback one. - A NAME request must carry HEADER exactly once @@ -46,7 +98,8 @@ refused and a POST still reaches the board). unchanged. Order of checks for a NAME request: Host, Origin, header (401), method -(403). An unsigned write gets 401, not 403, so nothing about the routes is +(403). Round 2 puts two checks ahead of all of these, for every request: a +repeated Host (400), then HEADER with a Host other than NAME (403). An unsigned write gets 401, not 403, so nothing about the routes is answered before sign-in. ## Decisions and tradeoffs @@ -66,10 +119,12 @@ answered before sign-in. reading. A comma is refused in ID for the same reason: a proxy that joins values with a comma would make `ID,ID` look like one value. 4. **HEAD is allowed** with GET. It reads nothing GET doesn't. -5. **Provider-neutral.** No identity provider is named in code, tests or - README. The tests use `remote-user`/`jason` and, for the first - deployment's shape, `X-authentik-uid` with a 64-hex stand-in id. The - README example uses `X-Example-Uid`. +5. **Provider-neutral.** No identity provider is named in the code or the + README; the README example uses `X-Example-Uid`. The tests use + `remote-user`/`jason` and, for the first deployment's shape as the brief + asks, `X-authentik-uid` with a 64-hex stand-in id, so a provider's + header name does appear in the tests and the demo. (Round 1 said "code, + tests or README", which overstated it; Filbert's N3.) ## Where the brief and the code differ (for Sage) @@ -94,10 +149,10 @@ answered before sign-in. | File | Change | |---|---| -| `packages/webui/src/serve.mjs` | `previewOptions`, the `preview` option, the NAME, Origin, header and read-only checks | +| `packages/webui/src/serve.mjs` | `previewOptions`, the `preview` option, the repeated-Host, proxy-Host, NAME, Origin, header and read-only checks | | `packages/webui/src/cli.mjs` | the three flags, checked before config; the `Preview:` line | -| `packages/webui/README.md` | the preview section, the proxy contract, the local exposure; two lines of the old "do not expose" text replaced | -| `packages/webui/tests/preview.test.mjs` | new, 9 tests | +| `packages/webui/README.md` | the preview section, the proxy contract, the local exposure; two lines of the old "do not expose" text replaced; the repeated Host in the general security line | +| `packages/webui/tests/preview.test.mjs` | new, 11 tests | The README's bus-reads exposure paragraph is untouched, to keep the overlap with row 60 small. @@ -105,33 +160,66 @@ overlap with row 60 small. ## Tests (`preview.test.mjs`) 1. Options are all three or none, and each is checked (hosts, headers, - users; `startServer` with a partial set or `{}`). + users; `startServer` with a partial set or `{}`). Round 2 adds + `remote_user`, `_` and `-remote-user` to the refused headers. 2. The CLI refuses a partial or invalid flag set at start: exit 2, `refused:`, empty stdout. 3. The CLI with all three flags serves preview mode: the `Preview:` line, 401 unsigned, 200 signed in, 403 for a loopback POST. 4. 401 with nothing read: no header, another user, a suffix, a different - header, two values, across a bus read, a trail, the board and `/`. + header, two values, across a bus read, a trail, the board and `/`, and + a HEAD for each case (round 2). 5. Header name case and exact value: `X-authentik-uid` on the flag with a 64-hex id; lower, mixed and upper names pass; an uppercased value, a truncated value, a suffix, or `ID,ID` gets 401. 6. Signed-in reads of inbox, tasks, agents, a trail, the board and `/`; - NAME in mixed case; wrong Host, `NAME:443` and `NAME.evil.example` get - 403; wrong Origins get 403. + NAME in mixed case; a signed HEAD of `/` is 200. Wrong Host, `NAME:443`, + `NAME.evil.example`, `x.NAME` and `xNAME` get 403 for GET and HEAD: + `non-local Host refused` without HEADER, `proxy Host refused` with it. + Wrong Origins get 403 for GET and HEAD. 7. Every write gets 403 read-only from NAME and from loopback, with and without Origin, for both write routes, a bus route and `/`; PUT, DELETE, PATCH and OPTIONS too; an unsigned write gets 401; nothing reaches the board or bus. 8. Loopback is unchanged in preview mode, and with the board down its route answers the existing 502 while the bus views answer. -9. Without the flags NAME is refused and a POST still reaches the board. +9. A proxy that rewrites Host to the loopback port gets 403 `proxy Host + refused` (round 2, Darkwing's case and its variants, listed above). +10. A repeated Host is 400 before any other check, with and without the + flags (round 2, Filbert's case and its variants). +11. Without the flags NAME is refused and a POST still reaches the board. + +Without the round 2 `serve.mjs` change, tests 6, 9 and 10 fail and the other +eight pass. ## Mutants -22 single-site mutants in a separate worktree, run against -`preview.test.mjs` and `serve.test.mjs` (`mutants.py`, output in -`mutants.txt`). **22 of 22 -killed**; the worktree was clean after the run. +Round 2: 34 single-site mutants in a separate worktree (`17ea0dbc` plus +`build.patch`), run against `preview.test.mjs` and `serve.test.mjs` +(`mutants.py`, output in `mutants.txt`). **34 of 34 killed**; the worktree +was clean after the run. They are round 1's 22 (one site pinned more +narrowly, since the header lookup now appears twice) and 12 new: + +| Mutant | Edit | +|---|---| +| `repeated-host-allowed` | two Hosts pass; only three are refused | +| `repeated-host-preview-only` | the repeated-Host check only with the flags | +| `proxy-host-skipped` | the proxy-Host check removed | +| `proxy-host-wrong-user-only` | refused only when the value isn't ID | +| `proxy-host-nonempty-only` | an empty HEADER value passes | +| `proxy-host-get-only` | the proxy-Host check only for GET | +| `proxy-host-loopback-only` | the check only for a loopback-looking Host | +| `proxy-host-raw-name` | the check looks HEADER up by the raw flag | +| `header-check-skips-head` | Darkwing's: HEAD skips the sign-in check | +| `head-refused` | Darkwing's: HEAD gets 403 read-only | +| `header-leading-dash` | `--auth-header` accepts a leading `-` | +| `header-underscore` | `--auth-header` accepts `_` | + +The last four edit code round 1 already had. Against round 1's tests all +four survive (`mutants-round1-tests.txt`, `mutants.py` pointed at a +worktree of `bc8e1ee7` with round 1's patch); round 2's tests kill them. + +Round 1's 22, for reference: Partial set allowed; NAME not lowercased; HEADER not lowercased; IP allowed; `host` allowed as HEADER; comma allowed in ID; `preview: {}` @@ -153,6 +241,12 @@ passed. ## Demonstration +Round 2 adds four refusals to `demo.sh`: a wrong Host without and with +HEADER, a Host rewritten to `127.0.0.1:PORT` with a wrong user and to +`localhost:PORT` with the right one, and the two Hosts (over a raw socket, +since curl sends one Host however many are given). `demo.txt` was run on +the build worktree, whose four files match the manifest. + `demo.sh` starts the CLI in preview mode on loopback port 0 with no system config and a closed board port, then drives it with `curl`, showing each refusal and the allowed reads. `demo.txt` is its output, run on the gate @@ -162,14 +256,14 @@ got past the sign-in check; tests 6 and 8 show the bus reads themselves. ## Gate (my run on the candidate) -All suites ran one after another in a detached worktree of `bc8e1ee7` with -`build.patch` applied, `TMPDIR` in my scratch and Docker on, from -2026-10-11T22:26:06Z to 22:35:49Z. The manifest checked 4/4 OK before the -run, and `core.hooksPath` stayed unset. +Round 2: all suites ran one after another in a detached worktree of +`17ea0dbc` with `build.patch` applied, `TMPDIR` in my scratch and Docker +on, from 2026-10-11T23:15:06Z to 23:22:54Z. The manifest checked 4/4 OK +before the run, and `core.hooksPath` stayed unset. No flake, no rerun. | Suite | Result | |---|---| -| `packages/webui` | 49 pass, 0 fail (9 new), the browser tests included | +| `packages/webui` | 61 pass, 0 fail (11 in `preview.test.mjs`), the browser tests included | | `packages/control-board` | 124 pass, 0 fail | | `packages/cli` | 112 pass, 0 fail | | bus, business, conversation, discord, harness, ledger, mosaic, queue, runs, seat, tasks | all 0 fail (80, 60, 182, 178, 57, 78, 69, 150, 41, 27, 71 pass) | @@ -178,9 +272,16 @@ run, and `core.hooksPath` stayed unset. | test-release | 14 passed, 0 failed (Docker, nothing skipped) | | test-task | 98 passed, 0 failed (Docker, nothing skipped) | +Round 1's gate ran on `bc8e1ee7` from 22:26:06Z to 22:35:49Z, also all +green (webui 49 then). + ## Questions for reviewers -1. Is refusing `NAME:443` (decision 2) right, or should the default port be - accepted in case the proxy adds it? -2. Anything in the README's proxy contract that ops-01 would need and - doesn't have? +Round 1's two are answered: keep refusing `NAME:443`, and the contract +lines are added. One for round 2: + +1. The repeated-Host 400 applies without the flags too. Without them only + the first Host is ever read, and it must pass the loopback check by + itself, so the 400 closes nothing there. I kept it + unconditional because a repeated Host has no legitimate sender and one + rule is simpler to state. Would you rather it be preview-only? diff --git a/agents/dewey/work/queue-74/build.patch b/agents/dewey/work/queue-74/build.patch index b059e00b7..0c873a5c1 100644 --- a/agents/dewey/work/queue-74/build.patch +++ b/agents/dewey/work/queue-74/build.patch @@ -1,8 +1,8 @@ diff --git a/packages/webui/README.md b/packages/webui/README.md -index e43118d5..ff277b76 100644 +index e43118d5..883b5a6d 100644 --- a/packages/webui/README.md +++ b/packages/webui/README.md -@@ -16,12 +16,63 @@ Open http://127.0.0.1:7330/. Optional `--port N` and +@@ -16,12 +16,78 @@ Open http://127.0.0.1:7330/. Optional `--port N` and `--business ID` picks the business the bus views read; without it they read the running bus host's business (`mosaic bus start `). Port 0 picks a free port. Ctrl-C stops each foreground server. No daemon, @@ -11,7 +11,9 @@ index e43118d5..ff277b76 100644 +checks one header a sign-in proxy sets; it signs nobody in itself. Only HTTP loopback board origins are accepted. The WebUI binds to IPv4 loopback - by default, rejects nonlocal Host and cross-origin requests, sends no CORS +-by default, rejects nonlocal Host and cross-origin requests, sends no CORS ++by default, rejects a repeated Host (400), nonlocal Host and cross-origin ++requests, sends no CORS headers, refuses redirects and accepts only JSON object POST bodies up to 4096 -bytes. Do not expose either unauthenticated server through a public proxy. +bytes. Do not put the board behind a public proxy. Put the Console behind one @@ -39,9 +41,16 @@ index e43118d5..ff277b76 100644 + equal to ID. Missing, different or repeated answers 401 `sign-in required` + before any route runs, so no bus read or board call is made. An Origin, + when sent, must be `https://NAME`. -+- A request whose Host is loopback on the Console's port is checked as -+ before and needs no HEADER, so the operator's browser on the host still -+ works. Any other Host is 403. ++- A request whose Host is loopback on the Console's port (`127.x`, ++ `localhost` or `[::1]`) is checked as before and needs no HEADER, so the ++ operator's browser on the host still works. Any other Host is 403. ++- A request that carries HEADER with any Host but NAME answers 403 `proxy ++ Host refused`. The operator's browser never sends HEADER; a proxy that ++ rewrote Host does, and it fails on its first request instead of reaching ++ the loopback path unchecked. ++- More than one Host header answers 400 before any other check, in this ++ mode or not. Node keeps the first, so a loopback Host followed by ++ `Host: NAME` would otherwise skip the sign-in check. +- Every method other than GET and HEAD answers 403 `read-only`, whatever the + Host. `/api/seen` and `/api/reply` never reach the board, so Seen and Reply + fail in the page with that refusal. @@ -56,10 +65,17 @@ index e43118d5..ff277b76 100644 + from off the host. +- It strips HEADER from every client request and sets it only after the + identity provider signs the user in. -+- It forwards only requests whose Host is NAME, and preserves that Host. A -+ loopback Host is the operator's path and skips the header check, so a -+ proxy that forwarded a client's `Host: 127.0.0.1:7332` would let anyone -+ through without signing in. ++- It forwards only requests whose Host is NAME, and forwards that Host ++ unchanged. nginx rewrites Host to the upstream by default; set ++ `proxy_set_header Host $host;` (or the literal NAME). Caddy and Traefik ++ keep the client's Host by default. ++- Any loopback Host on the Console's port, `localhost` included, is the ++ operator's path and skips the header check, so a proxy that forwarded a ++ client's `Host: 127.0.0.1:7332` would let anyone through without signing ++ in. A tunnel whose local end uses the same port number sends such a Host ++ and reaches the Console unchecked. ++- Every hop on this host that reaches the loopback port is bound by this ++ contract too, not only the edge. + +The local exposure doesn't change ("Data and boundaries"): any process on +the host can still connect to the port and send `Host: NAME` with HEADER and @@ -119,10 +135,10 @@ index 64b3e7b4..5c6a5931 100644 } catch (err) { console.error(`refused: ${err.message}`); diff --git a/packages/webui/src/serve.mjs b/packages/webui/src/serve.mjs -index 4d51942a..34c4aca4 100644 +index 4d51942a..a1861d5d 100644 --- a/packages/webui/src/serve.mjs +++ b/packages/webui/src/serve.mjs -@@ -78,22 +78,54 @@ async function consoleRead(res, read) { +@@ -78,22 +78,60 @@ async function consoleRead(res, read) { return json(res, readStatus(code), { error: code, message: err instanceof ReadError ? err.message : code }); } } @@ -157,8 +173,14 @@ index 4d51942a..34c4aca4 100644 res.setHeader('referrer-policy', 'no-referrer'); res.setHeader('content-security-policy', "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; connect-src 'self'; font-src 'self'; object-src 'none'; base-uri 'none'; frame-ancestors 'none'; form-action 'self'"); let path, search; ++ // Node keeps the first of repeated Host headers; a second one (a loopback Host, then NAME) is ++ // never legitimate, so it is refused before any check reads the Host. ++ if ((req.headersDistinct.host?.length ?? 0) > 1) return json(res, 400, { error: 'repeated Host refused' }); + // Preview mode: a request for the public name, exactly, as the proxy forwards it. + const named = preview !== null && (req.headers.host ?? '').toLowerCase() === preview.host; ++ // HEADER with any other Host comes from a proxy that rewrote Host (nginx's default) and would ++ // otherwise land on the loopback path unchecked. The operator's browser never sends it. ++ if (preview !== null && !named && req.headersDistinct[preview.header]) return json(res, 403, { error: 'proxy Host refused', message: 'the proxy must forward Host unchanged' }); try { - const authority = new URL(`http://${req.headers.host}`); - if (!isLoopback(authority.hostname.replace(/^\[|\]$/g, '')) || Number(authority.port || 80) !== server.address().port) return json(res, 403, { error: 'non-local Host refused' }); @@ -183,16 +205,19 @@ index 4d51942a..34c4aca4 100644 const allowed = path === '/api/board' || conversation ? 'GET' : ['/api/seen', '/api/reply'].includes(path) ? 'POST' : null; diff --git a/packages/webui/tests/preview.test.mjs b/packages/webui/tests/preview.test.mjs new file mode 100644 -index 00000000..97abba89 +index 00000000..662add64 --- /dev/null +++ b/packages/webui/tests/preview.test.mjs -@@ -0,0 +1,203 @@ +@@ -0,0 +1,268 @@ +// Row 74 (#1568, lead decision 93): the preview mode. A request for the public name needs the +// proxy's header equal to the one user id, else 401 with nothing read; in preview mode nothing -+// but GET and HEAD gets through, whatever the Host; loopback requests work as they did. ++// but GET and HEAD gets through, whatever the Host; loopback requests work as they did. Round 2: ++// a repeated Host is 400, and HEADER with any Host but NAME is 403, so a proxy that rewrites Host ++// fails closed instead of reaching the loopback path. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { createServer, request } from 'node:http'; ++import { connect } from 'node:net'; +import { spawn, spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { startServer, previewOptions } from '../src/serve.mjs'; @@ -210,12 +235,23 @@ index 00000000..97abba89 + const req = request({ host: '127.0.0.1', port, method, path, headers: { host: NAME, ...length, ...headers } }, res => { + let text = ''; + res.on('data', c => (text += c)); -+ res.on('end', () => resolve({ status: res.statusCode, body: (res.headers['content-type'] ?? '').startsWith('application/json') ? JSON.parse(text) : text })); ++ res.on('end', () => resolve({ status: res.statusCode, body: text && (res.headers['content-type'] ?? '').startsWith('application/json') ? JSON.parse(text) : text })); + }); + req.on('error', reject); + req.end(body); + }); +} ++// A raw request, for what node:http won't send: two Host lines. ++function raw(port, lines) { ++ return new Promise((resolve, reject) => { ++ const socket = connect(port, '127.0.0.1', () => socket.end([...lines, 'connection: close', '', ''].join('\r\n'))); ++ let text = ''; ++ socket.on('data', c => (text += c)); ++ // json() answers chunked: one chunk, then the zero chunk. ++ socket.on('end', () => resolve({ status: Number(text.split(' ')[1]), body: JSON.parse(text.split('\r\n\r\n')[1].split('\r\n')[1]) })); ++ socket.on('error', reject); ++ }); ++} +const signedIn = { [HEADER]: ID }; +const json = { 'content-type': 'application/json' }; + @@ -254,7 +290,7 @@ index 00000000..97abba89 + await refused({}); + for (const host of ['https://' + NAME, NAME + ':443', NAME + '/', 'localhost', '127.0.0.1', '::1', 'preview', '', 'a..b.com', '-a.example.com']) + assert.throws(() => previewOptions({ publicHost: host, authHeader: HEADER, allowUser: ID }), /public host/, host); -+ for (const header of ['host', 'Origin', 'remote user', 'x:y', '', 'a'.repeat(65)]) ++ for (const header of ['host', 'Origin', 'remote user', 'x:y', '', 'a'.repeat(65), 'remote_user', '_', '-remote-user']) + assert.throws(() => previewOptions({ publicHost: NAME, authHeader: header, allowUser: ID }), /auth header/, header); + for (const user of ['', 'jason woltje', 'a,b', 'é', 'a'.repeat(257)]) + assert.throws(() => previewOptions({ publicHost: NAME, authHeader: HEADER, allowUser: user }), /allowed user/, user); @@ -304,6 +340,8 @@ index 00000000..97abba89 + assert.equal(r.status, 401, `${why} ${path}`); + assert.deepEqual(r.body, { error: 'sign-in required' }); + } ++ // HEAD is held to the same check; its answer has no body. ++ assert.equal((await send(port, { method: 'HEAD', path: '/', headers })).status, 401, `${why} HEAD`); + } + assert.deepEqual(busCalls, []); + assert.deepEqual(boardHits, []); @@ -332,16 +370,28 @@ index 00000000..97abba89 + // A hostname compares without regard to case. + assert.equal((await send(port, { path: '/healthz', headers: { ...signedIn, host: 'Mosaic-Preview.Example.COM' } })).status, 200); + assert.equal((await send(port, { path: '/', headers: { 'Remote-User': ID } })).status, 200); ++ assert.equal((await send(port, { method: 'HEAD', path: '/', headers: signedIn })).status, 200); + assert.deepEqual(busCalls, [['inbox', undefined], ['tasks', undefined], ['agents', undefined], ['trail', 'row-1']]); + assert.deepEqual(boardHits, ['GET /api/board']); -+ // Wrong Host, the public name with a port, and the wrong Origin refuse even with the header. -+ for (const headers of [{ host: 'evil.example' }, { host: NAME + ':443' }, { host: `${NAME}.evil.example` }]) { -+ const r = await send(port, { headers: { ...signedIn, ...headers } }); -+ assert.equal(r.status, 403, headers.host); assert.deepEqual(r.body, { error: 'non-local Host refused' }); ++ // Any Host but NAME exactly is refused: without the header as non-local, with it as a proxy ++ // that didn't forward Host unchanged. GET and HEAD alike. ++ for (const host of ['evil.example', NAME + ':443', `${NAME}.evil.example`, `x.${NAME}`, `x${NAME}`]) { ++ for (const method of ['GET', 'HEAD']) { ++ const path = method === 'GET' ? '/api/bus/inbox' : '/'; ++ const bare = await send(port, { method, path, headers: { host } }); ++ assert.equal(bare.status, 403, `${method} ${host}`); ++ const signed = await send(port, { method, path, headers: { ...signedIn, host } }); ++ assert.equal(signed.status, 403, `${method} ${host} signed`); ++ if (method === 'GET') { ++ assert.deepEqual(bare.body, { error: 'non-local Host refused' }); ++ assert.deepEqual(signed.body, { error: 'proxy Host refused', message: 'the proxy must forward Host unchanged' }); ++ } ++ } + } + for (const origin of [`http://${NAME}`, 'https://evil.example', `https://${NAME}:443`, `http://127.0.0.1:${port}`]) { + const r = await send(port, { headers: { ...signedIn, origin } }); + assert.equal(r.status, 403, origin); assert.deepEqual(r.body, { error: 'cross-origin request refused' }); ++ assert.equal((await send(port, { method: 'HEAD', path: '/', headers: { ...signedIn, origin } })).status, 403, `HEAD ${origin}`); + } + assert.equal(busCalls.length, 4); +}); @@ -382,6 +432,43 @@ index 00000000..97abba89 + assert.deepEqual(busCalls.map(c => c[0]), ['inbox', 'agents']); +}); + ++test('a proxy that rewrites Host to the loopback port gets 403 on its first request, whatever the user', async t => { ++ // nginx's default proxy_set_header Host $proxy_host sends the upstream's Host, the operator's path. ++ const { port, boardHits, busCalls } = await setup(t); ++ for (const host of [`127.0.0.1:${port}`, `localhost:${port}`, `[::1]:${port}`]) { ++ for (const [name, value] of [[HEADER, 'mallory'], [HEADER, ID], ['Remote-User', ID], [HEADER, '']]) { ++ for (const [method, path] of [['GET', '/api/bus/inbox'], ['GET', '/api/config'], ['HEAD', '/'], ['POST', '/api/seen']]) { ++ const r = await send(port, { method, path, headers: { host, [name]: value, ...(method === 'POST' ? json : {}) }, body: method === 'POST' ? '{}' : undefined }); ++ assert.equal(r.status, 403, `${host} ${name}=${value} ${method} ${path}`); ++ if (method !== 'HEAD') assert.deepEqual(r.body, { error: 'proxy Host refused', message: 'the proxy must forward Host unchanged' }); ++ } ++ } ++ // The operator's browser sends no HEADER and still gets through. ++ assert.equal((await send(port, { headers: { host } })).status, 200, host); ++ } ++ assert.deepEqual(busCalls.map(c => c[0]), ['inbox', 'inbox', 'inbox']); ++ assert.deepEqual(boardHits, []); ++}); ++ ++test('a repeated Host is 400 before any other check, in preview mode and without it', async t => { ++ for (const options of [{}, { preview: null }]) { ++ const { port, boardHits, busCalls } = await setup(t, options); ++ const loopback = `host: 127.0.0.1:${port}`, named = `host: ${NAME}`; ++ // Filbert's case: a loopback Host, then NAME, with no header; before round 2 this was 200. ++ for (const hosts of [[loopback, named], [named, loopback], [named, named], [loopback, loopback]]) { ++ for (const extra of [[], [`${HEADER}: ${ID}`]]) { ++ const r = await raw(port, ['GET /api/bus/inbox HTTP/1.1', ...hosts, ...extra]); ++ assert.equal(r.status, 400, hosts.concat(extra).join(' | ')); ++ assert.deepEqual(r.body, { error: 'repeated Host refused' }); ++ } ++ } ++ // One Host still works. ++ assert.equal((await raw(port, ['GET /api/bus/inbox HTTP/1.1', loopback])).status, 200); ++ assert.deepEqual(busCalls.map(c => c[0]), ['inbox']); ++ assert.deepEqual(boardHits, []); ++ } ++}); ++ +test('without the preview flags the public name is refused and writes still reach the board', async t => { + const { port, boardHits } = await setup(t, { preview: null }); + const r = await send(port, { headers: signedIn }); diff --git a/agents/dewey/work/queue-74/candidate-manifest.sha256 b/agents/dewey/work/queue-74/candidate-manifest.sha256 index e4f78f607..a76b0027c 100644 --- a/agents/dewey/work/queue-74/candidate-manifest.sha256 +++ b/agents/dewey/work/queue-74/candidate-manifest.sha256 @@ -1,4 +1,4 @@ -35fb02a5bcb0625a40f7aaf80e588c7acf7de1ac4ef7986c492bb9d097c0d5e4 packages/webui/README.md +c3b9e04e44e4beb1736d90191b0c72cbedabfdedea039e447e17b3e3b8e707eb packages/webui/README.md e0970dfdb3c930b622ec40d5fb60ca3ca0d95f05f604f561a1a97b1192a84d2a packages/webui/src/cli.mjs -fe27eaadfee4b2c1df89304f4b562420634fec948648e36d7ef3677e4d64e24f packages/webui/src/serve.mjs -50db4c28ff6c4f299bb03cfe194404843beb0fb8166ee63929d28833833680d7 packages/webui/tests/preview.test.mjs +8cd31c3fc95dc966a0252f7a45776d016ffd34578099263c649d41a0209dbdd5 packages/webui/src/serve.mjs +52885a39224cfba006a691765bafae42a035c02e19445a377ec875bb47ade8e0 packages/webui/tests/preview.test.mjs diff --git a/agents/dewey/work/queue-74/demo.sh b/agents/dewey/work/queue-74/demo.sh index 971f0a0ef..2d960f09f 100644 --- a/agents/dewey/work/queue-74/demo.sh +++ b/agents/dewey/work/queue-74/demo.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Row 74 (#1568): the Console in preview mode, driven with curl on loopback only. +# Row 74 (#1568): the Console in preview mode, driven with curl on loopback only (round 2). # Run from a worktree with the candidate applied: bash demo.sh . # No system config (MOSAIC_CONFIG names a missing file), so the bus routes answer # not-configured once past the sign-in check; the board flag names a closed port. @@ -21,11 +21,19 @@ PORT=$(sed -n 's|^Mosaic Console: http://127.0.0.1:\([0-9]*\)/$|\1|p' "$LOG") U=http://127.0.0.1:$PORT c() { local what=$1 out; shift; out=$(curl -s -w '\n%{http_code}' "$@"); printf '%-42s %s %.110s\n' "$what" "${out##*$'\n'}" "$(printf '%s' "${out%$'\n'*}" | tr -d '\n')"; } echo "== refusals" -c "wrong Host" -H "Host: evil.example" -H "$HEADER: $ID" "$U/api/bus/inbox" +c "wrong Host, no header" -H "Host: evil.example" "$U/api/bus/inbox" +c "wrong Host, signed in" -H "Host: evil.example" -H "$HEADER: $ID" "$U/api/bus/inbox" c "public name with a port" -H "Host: $NAME:443" -H "$HEADER: $ID" "$U/api/bus/inbox" c "wrong Origin" -H "Host: $NAME" -H "$HEADER: $ID" -H "Origin: https://evil.example" "$U/api/bus/inbox" c "missing header" -H "Host: $NAME" "$U/api/bus/inbox" c "wrong user" -H "Host: $NAME" -H "$HEADER: ${ID%?}0" "$U/api/bus/inbox" +c "Host rewritten to loopback, wrong user" -H "Host: 127.0.0.1:$PORT" -H "$HEADER: mallory" "$U/api/config" +c "Host rewritten to localhost, right user" -H "Host: localhost:$PORT" -H "$HEADER: $ID" "$U/api/config" +# curl sends one Host however many are given, so the repeated Host goes over a raw socket. +exec 3<>"/dev/tcp/127.0.0.1/$PORT" +printf 'GET /api/bus/inbox HTTP/1.1\r\nHost: 127.0.0.1:%s\r\nHost: %s\r\nConnection: close\r\n\r\n' "$PORT" "$NAME" >&3 +out=$(tr -d '\r' <&3); exec 3<&- +printf '%-42s %s %.110s\n' "two Hosts, loopback then the public name" "$(sed -n '1s/^HTTP\/1.1 \([0-9]*\).*/\1/p' <<<"$out")" "$(grep '^{' <<<"$out")" c "POST /api/seen, signed in" -H "Host: $NAME" -H "$HEADER: $ID" -H 'Content-Type: application/json' -d '{"seen":true}' "$U/api/seen" c "POST /api/reply, signed in" -H "Host: $NAME" -H "$HEADER: $ID" -H 'Content-Type: application/json' -d '{"agent":"p/a","text":"x"}' "$U/api/reply" c "POST /api/reply, loopback Host" -H 'Content-Type: application/json' -d '{"agent":"p/a","text":"x"}' "$U/api/reply" diff --git a/agents/dewey/work/queue-74/demo.txt b/agents/dewey/work/queue-74/demo.txt index 183532071..ac6994491 100644 --- a/agents/dewey/work/queue-74/demo.txt +++ b/agents/dewey/work/queue-74/demo.txt @@ -1,17 +1,21 @@ == partial flag set, refused at start refused: preview mode needs --public-host, --auth-header and --allow-user together exit 2 -Mosaic Console: http://127.0.0.1:44305/ +Mosaic Console: http://127.0.0.1:34273/ Board: http://127.0.0.1:9 Bus: not configured (system config problem (mosaic-config exit 3): mosaic-config: configuration not found: /no-config.json (run scripts/bootstrap.sh to create it)) Preview: https://mosaic-preview.woltje.com/ through the proxy, signed-in user from x-authentik-uid, read-only Ctrl-C stops this server. == refusals -wrong Host 403 {"error":"non-local Host refused"} -public name with a port 403 {"error":"non-local Host refused"} +wrong Host, no header 403 {"error":"non-local Host refused"} +wrong Host, signed in 403 {"error":"proxy Host refused","message":"the proxy must forward Host unchanged"} +public name with a port 403 {"error":"proxy Host refused","message":"the proxy must forward Host unchanged"} wrong Origin 403 {"error":"cross-origin request refused"} missing header 401 {"error":"sign-in required"} wrong user 401 {"error":"sign-in required"} +Host rewritten to loopback, wrong user 403 {"error":"proxy Host refused","message":"the proxy must forward Host unchanged"} +Host rewritten to localhost, right user 403 {"error":"proxy Host refused","message":"the proxy must forward Host unchanged"} +two Hosts, loopback then the public name 400 {"error":"repeated Host refused"} POST /api/seen, signed in 403 {"error":"read-only","message":"the Console preview is read-only"} POST /api/reply, signed in 403 {"error":"read-only","message":"the Console preview is read-only"} POST /api/reply, loopback Host 403 {"error":"read-only","message":"the Console preview is read-only"} diff --git a/agents/dewey/work/queue-74/mutants-round1-tests.txt b/agents/dewey/work/queue-74/mutants-round1-tests.txt new file mode 100644 index 000000000..edf8b5f6c --- /dev/null +++ b/agents/dewey/work/queue-74/mutants-round1-tests.txt @@ -0,0 +1,6 @@ +header-check-skips-head SURVIVED +head-refused SURVIVED +header-leading-dash SURVIVED +header-underscore SURVIVED +worktree diff after: clean +summary 0 of 4 killed diff --git a/agents/dewey/work/queue-74/mutants.py b/agents/dewey/work/queue-74/mutants.py index b5c837925..be479ccc5 100644 --- a/agents/dewey/work/queue-74/mutants.py +++ b/agents/dewey/work/queue-74/mutants.py @@ -1,11 +1,13 @@ #!/usr/bin/env python3 # Dewey row 74 mutants: single-site edits in the mutant worktree (never the build tree), -# each run against the preview and serve tests, then restored from the index. +# each run against the preview and serve tests, then restored from the index. Round 2 runs in +# mut2 (17ea0dbc plus the round 2 patch) and adds the repeated-Host, proxy-Host, HEAD and HEADER +# mutants after the round 1 list. import os, subprocess, sys R = os.path.expanduser('~/dewey-scratch/r74') -T = R + '/mut' -OUT = R + '/out' +T = R + '/mut2' +OUT = R + '/out2' SERVE = 'packages/webui/src/serve.mjs' CLI = 'packages/webui/src/cli.mjs' TESTS = ['node', '--test', 'packages/webui/tests/preview.test.mjs', 'packages/webui/tests/serve.test.mjs'] @@ -27,12 +29,25 @@ MUTANTS = [ ('two-values-allowed', SERVE, "values.length !== 1 || values[0] !== preview.user", "!values.includes(preview.user)", TESTS), ('value-prefix', SERVE, "values[0] !== preview.user", "!values[0].startsWith(preview.user)", TESTS), ('value-case-insensitive', SERVE, "values[0] !== preview.user", "values[0].toLowerCase() !== preview.user.toLowerCase()", TESTS), - ('header-check-raw-name', SERVE, "req.headersDistinct[preview.header]", "req.headersDistinct[preview.header.toUpperCase()]", TESTS), + ('header-check-raw-name', SERVE, "const values = req.headersDistinct[preview.header];", "const values = req.headersDistinct[preview.header.toUpperCase()];", TESTS), ('readonly-named-only', SERVE, "if (preview !== null && req.method !== 'GET' && req.method !== 'HEAD') return", "if (named && req.method !== 'GET' && req.method !== 'HEAD') return", TESTS), ('readonly-post-only', SERVE, "if (preview !== null && req.method !== 'GET' && req.method !== 'HEAD') return", "if (preview !== null && req.method === 'POST') return", TESTS), ('readonly-always', SERVE, "if (preview !== null && req.method !== 'GET' && req.method !== 'HEAD') return", "if (req.method !== 'GET' && req.method !== 'HEAD') return", TESTS), ('cli-preview-dropped', CLI, "bus, reads, preview: options.preview });", "bus, reads });", TESTS), ('cli-checked-late', CLI, " options.preview = previewOptions(preview);\n", "", TESTS), + # Round 2. + ('repeated-host-allowed', SERVE, "(req.headersDistinct.host?.length ?? 0) > 1", "(req.headersDistinct.host?.length ?? 0) > 2", TESTS), + ('repeated-host-preview-only', SERVE, "if ((req.headersDistinct.host?.length ?? 0) > 1)", "if (preview !== null && (req.headersDistinct.host?.length ?? 0) > 1)", TESTS), + ('proxy-host-skipped', SERVE, "if (preview !== null && !named && req.headersDistinct[preview.header])", "if (false && preview !== null && !named && req.headersDistinct[preview.header])", TESTS), + ('proxy-host-wrong-user-only', SERVE, "!named && req.headersDistinct[preview.header])", "!named && req.headersDistinct[preview.header]?.some(v => v !== preview.user))", TESTS), + ('proxy-host-nonempty-only', SERVE, "!named && req.headersDistinct[preview.header])", "!named && req.headersDistinct[preview.header]?.[0])", TESTS), + ('proxy-host-get-only', SERVE, "if (preview !== null && !named && req.headersDistinct", "if (preview !== null && !named && req.method === 'GET' && req.headersDistinct", TESTS), + ('proxy-host-loopback-only', SERVE, "if (preview !== null && !named && req.headersDistinct", "if (preview !== null && !named && /^(127\\.|localhost|\\[)/.test(req.headers.host ?? '') && req.headersDistinct", TESTS), + ('proxy-host-raw-name', SERVE, "!named && req.headersDistinct[preview.header])", "!named && req.headersDistinct[preview.header.toUpperCase()])", TESTS), + ('header-check-skips-head', SERVE, " if (named) {\n // One value", " if (named && req.method !== 'HEAD') {\n // One value", TESTS), + ('head-refused', SERVE, "req.method !== 'GET' && req.method !== 'HEAD') return json(res, 403, { error: 'read-only'", "req.method !== 'GET') return json(res, 403, { error: 'read-only'", TESTS), + ('header-leading-dash', SERVE, "const HEADER = /^[a-z0-9][a-z0-9-]{0,63}$/;", "const HEADER = /^[a-z0-9-]{1,64}$/;", TESTS), + ('header-underscore', SERVE, "const HEADER = /^[a-z0-9][a-z0-9-]{0,63}$/;", "const HEADER = /^[a-z0-9_][a-z0-9_-]{0,63}$/;", TESTS), ] def run(cmd): diff --git a/agents/dewey/work/queue-74/mutants.txt b/agents/dewey/work/queue-74/mutants.txt index 0d1f526e8..8781d4775 100644 --- a/agents/dewey/work/queue-74/mutants.txt +++ b/agents/dewey/work/queue-74/mutants.txt @@ -20,5 +20,17 @@ readonly-post-only killed readonly-always killed cli-preview-dropped killed cli-checked-late killed +repeated-host-allowed killed +repeated-host-preview-only killed +proxy-host-skipped killed +proxy-host-wrong-user-only killed +proxy-host-nonempty-only killed +proxy-host-get-only killed +proxy-host-loopback-only killed +proxy-host-raw-name killed +header-check-skips-head killed +head-refused killed +header-leading-dash killed +header-underscore killed worktree diff after: clean -summary 22 of 22 killed +summary 34 of 34 killed