Files
stack/agents/dewey/work/queue-74/build.md
T
jason.woltjeandClaude Opus 5.5 fce04c1556 review(dewey): row 74 round 2 packet, repeated-Host 400 and proxy-Host 403 (#1568)
Round 2 per Sage's notes revs 521 and 523: a repeated Host is 400 before
any check; HEADER with a Host other than NAME is 403 proxy Host refused;
HEAD, x.NAME and --auth-header tests; the README proxy contract lines.
Candidate manifest 17b6d7f6, base 17ea0dbc. 34 of 34 mutants killed;
gate green.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-11 18:23:26 -05:00

288 lines
16 KiB
Markdown

# Row 74 (#1568): WebUI preview behind a sign-in proxy, round 2
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 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
`serve --public-host NAME --auth-header HEADER --allow-user ID`, all three
or none. Off by default, and without the flags the Console behaves as
before (`serve.test.mjs` unchanged and green, plus a test that NAME is
refused and a POST still reaches the board).
- `previewOptions` (serve.mjs) validates and lowercases NAME and HEADER.
NAME is a DNS name with at least one dot: no scheme, port, path or IP
literal, and not `localhost`. HEADER is a header name, 1 to 64 of
`[a-z0-9-]`, not `host` or `origin`. ID is 1 to 256 visible ASCII
characters without a comma. A partial set throws. `startServer`
revalidates its `preview` option the same way, so a direct caller can't
skip the check, and `preview: {}` refuses instead of serving without it.
- The CLI collects the three flags and calls `previewOptions` before any
config is read, so a partial or bad set exits 2 with `refused: …` and
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
(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://<Host>` for a loopback one.
- A NAME request must carry HEADER exactly once
(`req.headersDistinct`), with a value equal to ID. Otherwise 401
`{error: 'sign-in required'}`, before routing, so no bus read or board
call is made. Node lowercases header names and trims surrounding
whitespace from values; the compare is otherwise exact.
- In preview mode every method other than GET and HEAD answers 403
`{error: 'read-only', message: 'the Console preview is read-only'}`,
whatever the Host, after the sign-in check. `/api/seen` and `/api/reply`
never reach the board.
- Loopback requests need no header and read as before. The GET routes are
unchanged.
Order of checks for a NAME request: Host, Origin, header (401), method
(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
1. **Every non-GET/HEAD method gets 403, not only POST.** The brief says
"every POST". The Console's only write routes are POST today, and every
other non-GET/HEAD method already gets 404 or 405 on the base, so the
superset changes no allowed request. It
means a write route added later is refused in preview mode until
someone opens it on purpose (row 76 does, under `--send`).
2. **A port on NAME is refused.** The README tells the proxy to preserve
`Host: NAME`. A proxy that sends `NAME:443` fails closed with a 403
rather than being matched loosely. If ops-01's proxy adds the port,
the fix is in the proxy config, not here.
3. **Repeated header is 401.** Two values could come from a client copy
the proxy failed to strip plus the proxy's own. Refusing is the safe
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 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)
- The superset in decision 1.
- With row 60 landed, `/api/bus/credentials` is also readable through the
preview by the signed-in user: credential states and dates, no tokens.
`/api/bus/decisions` is readable today. Both are GET routes that "stay as
they are"; I mention them because the brief lists inbox, tasks, agents
and trails as the minimum.
- Overlap with row 60: both change `packages/webui/src/serve.mjs` (row 60
only the `BUS_VERBS` line) and `packages/webui/README.md`. The two
patches apply on top of each other in either order without conflict, but
whichever lands second has different digests for those files, so its
manifest needs a new round.
- A refused Reply in the page shows `read-only Delivery may be unknown;
check the seat before sending again.` The second sentence is wrong for a
403. It comes from `src/public/app.js`, which isn't in this row's files,
and row 76 changes that path.
- The brief's line references (`serve.mjs` 82, 92, 96) match the base.
## Files
| File | Change |
|---|---|
| `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; 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.
## Tests (`preview.test.mjs`)
1. Options are all three or none, and each is checked (hosts, headers,
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 `/`, 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; 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. 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
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: {}`
served; NAME compared case-sensitively; NAME as a prefix; NAME with any
port; `http://NAME` Origin allowed; `https://NAME` Origin allowed on
loopback; header check skipped; any of several values accepted; value as
a prefix; value compared without case; header looked up by the raw flag;
read-only only for NAME; read-only only for POST; read-only without the
flags; CLI drops the option; CLI never validates.
The first run found two faults in the tests, not the code, both fixed
before the gate. `setup()` registered the fake board's cleanup after
`startServer`, so a mutant that made `startServer` throw left the board
open and the run hung instead of failing. And test 1 left open any server
a mutant let start. Both now close what they open, and the mutants that
had hung fail in about a second. Test 3 was added because no test started
the CLI with all three flags, so a CLI that dropped the option would have
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
worktree, with the server's start lines. Nothing leaves the host. With no
bus configured the inbox read answers 503 `not-configured`, which shows it
got past the sign-in check; tests 6 and 8 show the bus reads themselves.
## Gate (my run on the candidate)
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` | 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) |
| build-tokens `--check` | exit 0 |
| test-auth, test-conductor, test-config, test-discord, test-extension-package, test-foundation, test-queue | exit 0 (15, 17, 24, 66, 18, 44, 27 passed) |
| 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
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?