tech debt audit and cleanup specs for milestones 15-19

specs/milestone-17.md

@@ -0,0 +1,427 @@
+# Milestone 17: contract repairs
+
+Goal: make the operator-facing contract true. Four decided items — a real
+per-query deadline, the upstream editor milestone 8 promised, an opt-in
+trusted-proxy setting, and a field-level contract guard between the server
+and the frontend — plus BADVERS, the validator holes, and the doc claims
+the code contradicts. `TECH_DEBT.md` Theme 4. All four decisions are made;
+do not re-litigate them.
+
+**PROVISIONAL.** Written before milestones 15-16 were built. m15 touches
+`docs_drift_test.zig` and `cli.zig`; m16 touches `pool.zig`'s neighbors,
+the listeners and the settings handler. Re-verify line references before
+each session starts.
+
+## Rulings (binding)
+
+### 1. `total_timeout_ms` becomes a real per-query deadline
+
+Today the key feeds `Pool.attempt_timeout` (app.zig:283-288 →
+pool.zig:110) and bounds one attempt; with N upstreams down a query takes
+N × 5000 ms. The reference docs contradict each other (configuration.md
+says "per-query budget", cli.md says "per-attempt") and PLAN promises a
+total budget. Decision: the total budget becomes real.
+
+- `model.Upstream` gains `attempt_timeout_ms: u32 = 2500`;
+  `total_timeout_ms` keeps its name and default (5000) and becomes the
+  whole-exchange deadline. `read_timeout_ms` is untouched (it feeds the
+  forward-zone client only, app.zig:412).
+- `pool.zig`: `attempt` keeps its race, now budgeted by the new key. The
+  two-pass failover loop is extracted into a private
+  `exchangeLoopLen(self, io, query, response_buf) transport.ExchangeError!usize`
+  (the `exchangeLen` precedent, pool.zig:690-693: `Io.concurrent` stores
+  the future's return value, so the raced loop returns a length and
+  `exchange` rebuilds the slice). A new outer race wraps that helper with
+  the total budget, using the select idiom already in the file. The outer
+  cancel unwinds an in-flight attempt — the `entry.busy` lock is taken
+  cancelably on purpose (:169-174) and released by defer. Outer expiry
+  returns `error.Timeout`, **unconditionally**: the fast-failure paths
+  (all upstreams disabled → `ConnectFailed`) finish long before the
+  budget, so no attempted-marker is needed, and a test pins that an
+  all-disabled pool still fails immediately rather than waiting out the
+  deadline.
+- `validate.zig`: `checkTimeout` both keys; the cross-check becomes
+  `attempt_timeout_ms <= total_timeout_ms` (the old `total >= read` check,
+  which related knobs of different subsystems, is deleted).
+- Settings plumbing for the new key (the reflective walk carries it; the
+  hand-written mirrors do not): `expected_keys` (model.zig:449, sorted),
+  the round-trip test with a non-default value, the `SettingsView`
+  upstream struct in web_integration_test.zig:528, `types.ts`
+  `Settings.upstream`, `openapi.yaml` Settings + SettingsPatch,
+  SettingsPage `SECTIONS[0]`, the configuration.md row (drift-guarded),
+  and the cli.md/configuration.md prose — both now say: total is the
+  per-query budget, attempt bounds one upstream try.
+- Tests: a pool test with two stalling upstreams proves the exchange
+  returns within the total budget, not 2 × attempt; the existing
+  per-attempt tests keep passing against the new key.
+
+### 2. The validator closes its two verified holes
+
+- **Hostname `tls://` upstreams.** `Endpoint.parse` accepts any host
+  text; the DoT client then refuses non-literals on every dial
+  (`resolveAddress`, dot_client.zig:74-80), so a hostname `tls://` config
+  validates clean and fails at query time as a peer fault. Milestone 3
+  explicitly ordered this check carried into the validator and it was
+  dropped. Correction to the audit: the function to mirror is **not**
+  `parseResolver` itself (validate.zig:257 — that is the forward-zone
+  resolver check); the upstream block in `checkCollections`
+  (validate.zig:603-639) gains, for `.dot` endpoints,
+  `net.IpAddress.parse(endpoint.host, endpoint.port) catch` → a new
+  `error.UpstreamHostNotIpLiteral` diagnostic ("tls:// upstreams take an
+  IP literal host"). `faults.zig` picks the variant up automatically.
+  Fix the broken example at configuration.md:348 — the hostname NextDNS
+  `tls://` config can never complete an exchange; show an IP-literal
+  `tls://` example and move the hostname form to `https://`.
+- **Boot-allocation bounds.** `query_log_buffer_max` is floor-checked
+  only (an Entry is ~420 bytes; `maxInt(u32)` asks for ~1.8 TB at boot),
+  and `cache.size` is not validated at all (grep confirms). Both get the
+  file's ceiling-only idiom: `1..1_000_000` with the "must be at most"
+  message shape. No full memory-fit guarantee — out of reach, not needed.
+
+### 3. The upstream editor is built
+
+Milestone 8 ruling 9 requires upstream CRUD in the UI ("the Settings page
+must edit them"); milestone 9 dropped the obligation when the SPA was
+built. The entire data layer exists with zero importers
+(`upstreamsQuery`, three mutation factories at queries.ts:251-264, four
+api.ts wrappers). Decision: build it. Placement deviates from m8's
+literal text: a dedicated **Upstreams page** (nav entry), matching the
+house resource-page pattern — record the deviation beside m8 ruling 9.
+
+- New `web/src/features/upstreams/UpstreamsPage.tsx` +
+  `UpstreamForm.tsx`, modeled on the blocklists pair (the closest
+  analogue; copy its named idioms: editing-state-as-row, two mutation
+  instances of one factory so row-toggle state never bleeds into the
+  form, `mutateAsync` submit, full-row resend on toggle because **PUT is
+  a replace**, `key={editing?.id ?? "add"}` remount, `window.confirm`
+  delete, split error routing).
+- Fields: url, priority (number), enabled (toggle), tls_name. 400s carry
+  the real validator's text (the handler validates via
+  `mutations.checkUpstream`); surface the three 409 conflicts verbatim
+  ("an upstream with that url already exists", "the last enabled
+  upstream cannot be disabled/removed").
+- Every mutation response carries `restart_required: true` and today
+  nothing raises the banner outside SettingsPage. Generalize the restart
+  banner copy to "Changes saved. Restart nxdns to apply." and raise it on
+  upstream create/update/delete success.
+- Wiring is three edits: `createRoute` in routes.tsx, the `routeTree`
+  entry, the AppShell nav item. The health table stays on the Dashboard
+  (health rows have no `id` — url is the only join key — and reflect the
+  running pool, so a new upstream appears there only after restart; the
+  page states this beside the restart banner).
+- Dead-layer cleanup in the same stroke (the same finding): delete the
+  seven unused single-row `get*` wrappers (correction: seven, not five —
+  `getGroup` and `getUpstream` are shadowed by longer names in grep),
+  plus `getMetrics`, `getOpenapiYaml`, and `ruleUpdateMutation`. The
+  editor reads rows from the list query per house idiom, so none gains a
+  caller. Git history keeps them.
+- Page tests in the stubbed-fetch house style: list render, add, toggle
+  resends the full row, delete confirms, a 409 renders inline, the
+  banner raises.
+
+### 4. Trusted proxies restore real client identity
+
+Behind the documented same-box reverse proxy every request is loopback:
+`localhost_exempt = true` (default) then disables the API limiter — the
+only brake on argon2 brute force — and all remote users share one
+address's 3-stream SSE cap. No X-Forwarded-For handling exists anywhere
+(verified repo-wide). Decision: opt-in trusted-proxy setting.
+
+- New setting `web.trusted_proxies: []const u8 = ""` — a comma-separated
+  list of IP literals in one string (the settings codec supports scalars
+  only; a list type is a compile error at model.zig:358). Empty means
+  off: behavior today.
+- Validation: each comma-separated element must parse as an IP literal →
+  new `error.BadTrustedProxy` diagnostic in the web block of
+  `checkScalars`.
+- Mechanics: when the socket peer is in the trusted set, the effective
+  client address is the **last** valid IP literal in the request's
+  `X-Forwarded-For` (the entry our proxy appended); a **missing** header
+  falls back to the socket peer. Implementation follows the
+  copy-before-dispatch constraint (http_util.zig:10-14): a new
+  `max_xff_len = 256` budget, a new Conn buffer, and a new `Request`
+  field `client_addr: address.NetAddress` computed once in
+  `handleRequest`. The copy keeps the **tail**, not the head: a new
+  bounded suffix-copy helper beside `copyHeader`, because `copyHeader`
+  returns empty on overflow (server.zig:608) and an empty result would
+  fall back to the socket peer — loopback behind the intended same-box
+  proxy, which `api_localhost_exempt = true` (model.zig:89, the default)
+  then exempts. That is the exact bypass this ruling closes: a client
+  ships an oversized XFF through the proxy and regains the exemption.
+  The proxy appends `, <client-ip>` last, so the real entry always fits
+  in the 256-byte tail. Fail closed on the residue: header present but
+  no valid last IP literal in the tail means the trusted proxy violated
+  its contract — respond 400, never fall back to the exemptible peer.
+  Only a misconfigured proxy can trigger that arm; a spoofed prefix
+  cannot, because the proxy's appended entry always terminates the
+  chain. `bucketLimit` (server.zig:201-207) and the
+  SSE acquire/release in `live.zig:82-87` both read `client_addr` — the
+  SSE pair must use the same key or releases leak. `api_limiter.Config`
+  is untouched: `WebState.web` already carries every web field
+  (app.zig:472), so the trust check reads `state.web`.
+- With trust configured, a proxied remote address is not loopback, so
+  the exemption no longer swallows it — the limiter and the per-address
+  SSE cap bind per real client. Requests arriving at the socket from
+  non-trusted, non-loopback peers are unaffected.
+- Settings plumbing checklist (same list as ruling 1, plus the
+  hand-written web mirrors the fact-finding flagged): `expected_keys`,
+  round-trip test, `WebView` + `view()` in handlers/settings.zig:207-248,
+  `restart_required_keys` balance test, `SettingsView` web struct in the
+  integration test, `types.ts` `Settings.web`, openapi web schemas
+  (including the `required` array), SettingsPage web section, the
+  configuration.md row, the api.md rate-limiting section, and a warning
+  in the proxy/authentication how-to: when proxying without
+  `trusted_proxies`, set `api_localhost_exempt = false`.
+- Tests: unit tests on the effective-address derivation (trusted peer +
+  XFF chain, untrusted peer + spoofed XFF ignored, trusted peer + no
+  XFF, trusted peer + an XFF over 256 bytes whose valid last entry is
+  still honored from the tail, trusted peer + a present-but-invalid
+  chain → 400); an integration test proving a proxied remote address is
+  rate-limited while the proxy itself stays exempt.
+
+### 5. A field-level contract guard between server and frontend
+
+The REST contract lives in three hand-synced copies. The Zig side is
+genuinely well guarded (56-entry contract table parsing live responses
+with `.ignore_unknown_fields = false`; route-level openapi guards — which
+live in `web_integration_test.zig:1494-1590` and `openapi.zig`, not in
+`docs_drift_test.zig` as the audit said). **TypeScript is guarded by
+nothing**: every frontend test stubs fetch, and types.ts is strictly
+narrower than the wire in places (literal unions like
+`Health.status: "ok" | "degraded"`), so re-parsing into Zig structs can
+never catch an out-of-union string. Decision: one integration layer
+asserting the frontend's consumed shapes against real responses.
+
+Mechanism — captured samples validated by `tsc`, no new dependencies:
+
+- A new `-Dintegration` test beside the contract walk drives the real
+  `Env` server through every route whose contract-table kind is
+  `.json` **and** which the frontend consumes through an `api.ts` wrapper
+  — the GETs **and** the JSON-returning POST/PUT wrappers (login, the
+  group/client/rule/upstream/blocklist writes, refresh, pause, the
+  settings PUT), driven with deterministic seed payloads so the mutating
+  responses are stable too (plus one sample per shared error response
+  class). GET-only would leave every write-path response contract
+  unguarded. Explicitly excluded:
+  the `.raw` routes (`/metrics`, `/api/openapi.yaml` — not JSON, and
+  ruling 3 deletes their unused wrappers), the `.sse` route, and the
+  `.none` deletes. The endpoint-to-type mapping is derived from `api.ts`,
+  not invented. Captured bodies are **canonicalized**: keys sorted, every
+  number replaced with 0 (type-preserving; strings and booleans kept —
+  they come from the deterministic seed). Canonicalization is what makes
+  the output byte-stable across runs.
+- The canonical samples render into a committed file
+  `web/src/lib/contractSamples.gen.ts`: for each endpoint,
+  `export const sample_<name>: <TsType> = <json>;` with the matching
+  import from `types.ts`. TypeScript object literals get excess-property
+  checking, so a server field missing from types.ts, a types.ts field
+  missing from the wire, and an out-of-union literal all fail
+  `npm run typecheck` — which already runs in CI's frontend job.
+- Embedding: an anonymous-import root must be Zig, not TypeScript, so a
+  wrapper `web/src/lib/contract_samples.zig` exports
+  `pub const bytes = @embedFile("contractSamples.gen.ts");` and build.zig
+  registers it — the exact `docs/docs.zig` pattern. The Zig test
+  byte-compares the embedded bytes against the freshly rendered content
+  and fails on mismatch.
+- Regeneration is explicit, not write-beside-the-embed (embedded bytes
+  carry no source-tree path): a build option `-Dcontract-samples-out`
+  (absolute path, wired through `build_options`) makes the test write the
+  fresh content there instead of comparing. The pinned command, stated in
+  the failure message and the docs:
+  `zig build test -Dintegration -Dcontract-samples-out="$PWD/web/src/lib/contractSamples.gen.ts"`.
+  Golden-file discipline: the server side keeps the samples current;
+  `tsc` keeps types.ts honest against them.
+- `types.ts` gains `ErrorEnvelope` matching the shared error responses
+  (verify the exact envelope shape against the handlers before writing
+  it) so the error samples are typed too.
+- Recorded residual risk: the server↔openapi seam stays route-level
+  only. The yaml is served verbatim and never parsed; field-level yaml
+  drift remains possible and is accepted — revisit only if it bites.
+
+### 6. BADVERS becomes expressible
+
+RFC 6891 §6.1.3: a version-1 EDNS query must get RCODE 16 (BADVERS) with
+version 0 in the reply OPT. Nothing checks `opt.version` anywhere
+(verified), and the write path cannot express it: `addOptEcho` hardcodes
+`extended_rcode = 0` (packet.zig:354) and `synthesize` takes a 4-bit
+`types.Rcode`. Negligible operational impact; the mechanism is the
+finding.
+
+- `edns.zig` gains the write-side splitter beside the read-side
+  `extendedRcode` (:272): a 12-bit rcode splits into the header's four
+  bits and the OPT's upper eight. `extendedRcode` stays — it is the
+  read-side pair (milestone 19 assumes it survives).
+- `packet.zig` gains `addOptWithRcode(request_opt, do_bit,
+  extended_rcode: u8)`; `addOptEcho` becomes a call with 0.
+- `handler.zig`: the version check sits directly after the successful
+  `parseOpt` (:227-233), before the opcode check — `if (o.version != 0)`
+  → a BADVERS synthesis (header `.no_error` + OPT `extended_rcode = 1,
+  version = 0`), counted in a new `Handler.Stats.badvers` (the metrics
+  reflection exports it; extend the name test).
+- Tests: a version-1 query yields composite rcode 16 (assert via
+  `edns.extendedRcode`) and version 0 in the reply OPT; a version-0
+  query is unaffected.
+
+### 7. The stale phase comments state the as-built contract
+
+- `rate_limiter.zig:5-6` ("Phase 7 decides the locking"): rewrite to the
+  as-built truth — the handler serializes with `Handler.limiter_mutex`
+  (handler.zig:124), uncancelable on the query path, cancelable in the
+  `runMaintenance` sweep (app.zig:702-731 already states the rationale
+  to adopt).
+- `shutdown.zig` `trigger` doc ("what a Phase 8 restart endpoint would
+  call"): no restart endpoint exists or is planned — m8 shipped
+  `restart_required` echoes instead. Rewrite to "what a test uses".
+- `pool.zig:83-84`: tense only — the health endpoint exists; "Feeds
+  `GET /api/upstream/health`."
+
+### 8. Doc transcripts stop hardcoding the version
+
+Four sites print `0.1.0-dev` (tutorial/first-run.md:108,
+install-with-docker.md:126, install-with-systemd.md:206,
+upgrade.md:142 — three how-to plus one tutorial, two different
+transcript shapes). Correct today; silently wrong at the first tag.
+Milestone 14 mandates a placeholder and a guard but names neither; the
+milestone-13 executed-transcript rule is in tension.
+
+- Ruling on the tension, explicit: milestone-13 ruling 3 constrains
+  **command lines**; pasted **output lines** may carry placeholders.
+  The orchestrator adds one clarifying sentence to milestone-13.md
+  beside ruling 3.
+- The four output lines replace the version with `<version>` (e.g.
+  `nxdns <version> serving on ...`).
+- The guard: `docs/docs.zig` additionally embeds the tutorial and
+  how-to pages; a new test in `docs_drift_test.zig` asserts no embedded
+  doc page contains the string `0.1.0-dev` and that the four transcript
+  pages contain `nxdns <version>`. (Note m15 rewrote this file's CLI
+  test — rebase, don't collide.)
+
+### 9. The log docs stop claiming append mode
+
+`files-and-directories.md:143` claims the log file is "opened for
+append". It is not: `.mode = .write_only` plus `seekTo(state.file_pos)`
+per line, offset seeded once at open — the source comment at
+logging.zig:502 says so outright ("0.16.0 has no append mode"). Under
+external logrotate the divergence is destructive (copytruncate → sparse
+NUL-prefixed file; rename+create → unbounded writes to the renamed
+inode, `max_size_mb` unenforceable).
+
+Docs only: correct the row (positional writes, offset tracked
+in-process), and add an operator warning — nxdns owns rotation for this
+file; do not point external logrotate at it. A per-line stat re-check
+was considered and rejected: a syscall per log line on the hot path to
+defend against a misconfiguration the docs now forbid. Record that
+decision here.
+
+## Sessions
+
+Dependency graph: S1 → S2 → S5 (they share model.zig, validate.zig,
+types.ts, openapi.yaml, SettingsPage.tsx, web_integration_test.zig — the
+later session rebases on the earlier). S3, S4, S6 run in parallel with
+the chain and each other.
+
+### Session S1: deadline and validator
+
+Owns `src/upstream/pool.zig`, `src/config/model.zig`,
+`src/config/validate.zig`, `src/app.zig`, plus the ruling-1 plumbing
+trail (types.ts, openapi.yaml, SettingsPage SECTIONS[0], the
+integration-test SettingsView, configuration.md, cli.md). Rulings 1, 2,
+and the pool comment from ruling 7.
+
+### Session S2: trusted proxy (after S1)
+
+Owns `src/web/server.zig`, `src/web/http_util.zig`,
+`src/web/handlers/live.zig`, `src/web/handlers/settings.zig`, and its
+plumbing trail through the same shared files S1 touched. Ruling 4.
+
+### Session S3: upstream editor
+
+Owns `web/src/features/upstreams/*` (new), `web/src/routes.tsx`,
+`web/src/shell/AppShell.tsx`, `web/src/lib/api.ts`,
+`web/src/lib/queries.ts`, `web/src/features/settings/RestartBanner.tsx`
+and `restartBanner.ts`, `specs/milestone-8.md` (the deviation note).
+Ruling 3.
+
+### Session S4: BADVERS
+
+Owns `src/dns/packet.zig`, `src/dns/edns.zig`, `src/server/handler.zig`,
+the metrics name test in `src/web/metrics.zig`. Ruling 6.
+
+### Session S5: contract samples (after S1 and S2)
+
+Owns the new generator test in `src/web/web_integration_test.zig`,
+`build.zig` (the anonymous import), `src/tests.zig` (if a new file needs
+an entry), `web/src/lib/contractSamples.gen.ts` (committed golden),
+`web/src/lib/types.ts` (`ErrorEnvelope` only). Ruling 5.
+
+### Session S6: docs and comments
+
+Owns `src/server/rate_limiter.zig` and `src/server/shutdown.zig`
+(headers only), `docs/docs.zig`, `src/docs_drift_test.zig`, the four
+transcript pages, `docs/reference/files-and-directories.md`,
+`specs/milestone-13.md` (the clarifying sentence). Rulings 7 (except the
+pool comment), 8, 9.
+
+### Orchestrator
+
+Strikes the closed findings in `TECH_DEBT.md`; confirms PLAN's
+total-budget promise now matches the code; records the m8 placement
+deviation.
+
+## Module layout
+
+New files: `web/src/features/upstreams/UpstreamsPage.tsx`,
+`web/src/features/upstreams/UpstreamForm.tsx`,
+`web/src/lib/contractSamples.gen.ts` (generated, committed).
+
+Deleted surface: the seven unused `get*` wrappers, `getMetrics`,
+`getOpenapiYaml`, `ruleUpdateMutation`.
+
+## Acceptance (milestone complete)
+
+- [ ] Two stalled upstreams: the exchange returns within
+      `total_timeout_ms` (test). `attempt_timeout_ms` exists end to end:
+      settings key, validator range + cross-check, docs row, SettingsPage
+      field, SettingsView, types.ts, openapi.
+- [ ] A hostname `tls://` upstream fails `nxdns check` with the new
+      diagnostic; the configuration.md example connects.
+- [ ] `query_log_buffer_max = 4000000000` and `cache.size = 0` both fail
+      validation with ceiling/floor messages.
+- [ ] The Upstreams page lists, adds, edits, toggles (full-row resend),
+      deletes; the three 409 texts render; the restart banner raises with
+      the generalized copy; nav and route exist; page tests pass.
+- [ ] The ten dead wrappers/factories are gone; `npm run typecheck` and
+      `lint` pass.
+- [ ] With `trusted_proxies` set, a proxied remote address is
+      rate-limited and holds its own SSE budget while the proxy stays
+      exempt; a spoofed XFF from an untrusted peer is ignored; an
+      oversized XFF through the trusted proxy does not regain the
+      loopback exemption (tests). The how-to carries the no-trust
+      warning.
+- [ ] `contractSamples.gen.ts` is committed and contains samples for the
+      JSON-returning POST/PUT wrappers, not GETs only; the Zig generator
+      test passes against it; deliberately removing a field from a
+      types.ts type fails `npm run typecheck` (proven, then reverted).
+- [ ] A version-1 EDNS query answers composite RCODE 16 with version 0;
+      `nxdns_dns_badvers_total` appears in /metrics.
+- [ ] Grep: "Phase 7" and "Phase 8" appear in no source doc comment;
+      the rewritten headers state the mutex and the maintenance task.
+- [ ] Grep: `0.1.0-dev` appears in no docs/ page; the new drift test
+      passes and was proven able to fail (reinsert the literal, watch it
+      fail, revert).
+- [ ] files-and-directories.md describes positional writes and warns off
+      external logrotate.
+- [ ] Full suite green: `zig build test -Dintegration`, `npm run test`,
+      `npm run typecheck`.
+
+## Anti-requirements
+
+- No restart endpoint — the `restart_required` echo model stands.
+- No hostname resolution for `tls://` upstreams — the validator rejects;
+  the client's own check stays as defense in depth.
+- No openapi codegen and no runtime schema validation library — the
+  contract guard is captured samples + tsc, nothing more.
+- No proxy protocol (PROXY/haproxy) support; X-Forwarded-For only.
+- No per-line stat re-check in the logger (ruling 9 records why).
+- No upstream health join in the editor beyond the stated restart note.
+- No `Forwarded` (RFC 7239) header parsing — XFF only, recorded.