599 lines
35 KiB
Markdown
599 lines
35 KiB
Markdown
# Milestone 13: restructure the documentation to Diátaxis
|
|
|
|
Goal: replace the four mixed-mode documents with the four Diátaxis modes, add the
|
|
missing tutorial, and prove every instruction by running it.
|
|
|
|
## Rulings (binding)
|
|
|
|
1. **Four directories plus an index.** `docs/tutorial/`, `docs/how-to/`,
|
|
`docs/reference/`, `docs/explanation/`, and `docs/README.md` — the index names
|
|
the four modes, says who each is for, and links every page. The old
|
|
`docs/{operator,architecture,config-reference,api,performance}.md` are deleted
|
|
by the orchestrator once the new pages exist; no redirect stubs (greenfield
|
|
repo, AGENTS.md forbids compatibility leftovers).
|
|
|
|
2. **A document serves one mode.** The defect being fixed is `operator.md`, which
|
|
interleaves install procedure, CLI reference, exit-code tables and
|
|
troubleshooting in 418 lines. Reference pages state what is; how-to pages state
|
|
what to do; explanation states why; the tutorial teaches. When porting text,
|
|
move a paragraph to the mode it belongs to rather than keeping it where it was.
|
|
|
|
3. **Every command block in `tutorial/` and `how-to/` is executed verbatim by the
|
|
session that writes it**, on this host, before the session reports. A command
|
|
that cannot run here (needs a Raspberry Pi, root, a domain, hardware TLS) is
|
|
marked in the page itself as not verified on this host, with the reason. This
|
|
ruling exists because milestone 11's docs were written from source-reading
|
|
alone: the first real blocklist download aborted the process (commit 35f2324)
|
|
and a stale SPA bundle crashed the settings page — both would have surfaced if
|
|
the documented paths had been run. No session may report a page complete on the
|
|
strength of having read the code.
|
|
|
|
4. **Content is verified against `src/`, not copied from the old pages.** Defaults,
|
|
flags, exit codes, paths and route names come from the code at HEAD. Report any
|
|
discrepancy found; do not fix source in this milestone (docs-only, ruling 9).
|
|
|
|
5. **The tutorial has one guaranteed outcome.** `docs/tutorial/first-run.md` takes
|
|
a reader from a clean checkout to a running nxdns that answers a query and
|
|
blocks a domain from a real blocklist, on unprivileged ports in a scratch
|
|
directory, then stops it cleanly. It teaches by doing and states what the reader
|
|
will have at the end. No branching, no options menu, no "you may also".
|
|
|
|
6. **File map.**
|
|
- `docs/tutorial/first-run.md` — new.
|
|
- `docs/how-to/`: `install-with-systemd.md` (incl. the Raspberry Pi 5 aarch64
|
|
binary), `install-with-docker.md`, `upgrade.md`, `troubleshoot.md`,
|
|
`enable-doh-and-dot.md`, `set-up-admin-authentication.md`,
|
|
`back-up-and-restore.md`, `measure-performance.md`.
|
|
- `docs/reference/`: `configuration.md` (every section, field, default, range),
|
|
`api.md` (all 56 operations), `cli.md` (six subcommands, every flag, exit
|
|
codes), `files-and-directories.md` (data dir layout, file modes),
|
|
`performance.md` (targets + measured numbers).
|
|
- `docs/explanation/`: `architecture.md`, `configuration-model.md` (the file
|
|
seeds the database once, the database is truth, export/import round trip),
|
|
`performance-and-testing.md` (why the targets exist, why CI does not gate on
|
|
them, what hermetic tests do and do not prove).
|
|
|
|
7. **Drift guards repointed and kept honest.** `docs/docs.zig` embeds
|
|
`reference/api.md`, `reference/configuration.md`, `reference/cli.md`.
|
|
`src/docs_drift_test.zig` keeps its three guards against those paths: every
|
|
served route as a full `| METHOD | \`pattern\` |` row; every `toSettings` key
|
|
verbatim; every subcommand as its own heading. `reference/cli.md` gives each
|
|
subcommand a `## \`name\`` heading so the guard anchors on structure, not prose.
|
|
|
|
8. **Style.** Plain sentences. No banned vocabulary (leverage, seamless, robust as
|
|
filler, rule-of-three padding, "it's important to note"). No badges, no
|
|
marketing. Code blocks are copy-pasteable and use the scratch paths the
|
|
tutorial establishes, not `/var/lib/nxdns`, unless the page is about a real
|
|
install.
|
|
|
|
9. **Docs-only.** The only non-docs edits are the orchestrator's: `docs/docs.zig`,
|
|
`src/docs_drift_test.zig`, `README.md` links, and PLAN.md's repo-layout line.
|
|
No behavior changes.
|
|
|
|
## Sessions
|
|
|
|
X1, X2, X3, X4, X5 run in parallel — every session owns distinct files.
|
|
|
|
## Session X1: index + tutorial + README links
|
|
|
|
Owns `docs/README.md`, `docs/tutorial/first-run.md`, `README.md` (links section
|
|
only). Rulings 1, 5, 3 (the tutorial is executed end to end).
|
|
|
|
## Session X2: how-to, install and operations
|
|
|
|
Owns `docs/how-to/{install-with-systemd,install-with-docker,upgrade,troubleshoot}.md`.
|
|
Rulings 2, 3, 4.
|
|
|
|
## Session X3: how-to, security and measurement
|
|
|
|
Owns `docs/how-to/{enable-doh-and-dot,set-up-admin-authentication,back-up-and-restore,measure-performance}.md`.
|
|
Rulings 2, 3, 4.
|
|
|
|
## Session X4: reference
|
|
|
|
Owns `docs/reference/{configuration,api,cli,files-and-directories,performance}.md`.
|
|
Rulings 2, 4, 7 (heading convention).
|
|
|
|
## Session X5: explanation
|
|
|
|
Owns `docs/explanation/{architecture,configuration-model,performance-and-testing}.md`.
|
|
Rulings 2, 4.
|
|
|
|
## File ownership
|
|
|
|
X1 docs/README.md + docs/tutorial/* + README.md; X2 and X3 disjoint files under
|
|
docs/how-to/; X4 docs/reference/*; X5 docs/explanation/*. Orchestrator:
|
|
docs/docs.zig, src/docs_drift_test.zig, PLAN.md, deletion of the old pages, spec.
|
|
|
|
## Delivered
|
|
|
|
Seventeen pages: `docs/README.md`, one tutorial, eight how-to guides, five
|
|
reference pages, three explanation pages. The five old documents are deleted.
|
|
|
|
The orchestrator repointed `docs/docs.zig` at `reference/{api,configuration,cli}.md`
|
|
and `src/docs_drift_test.zig` at the same three, moved the subcommand anchor from
|
|
`### \`name` to `## \`name`, fixed the README Quickstart link that pointed at the
|
|
deleted `operator.md`, and corrected PLAN.md's repo-layout line and its §13.2
|
|
OpenAPI paragraph (which still promised a renderer and contract tests that were
|
|
never built).
|
|
|
|
All three guards were proven able to fail: deleting one API table row, one
|
|
settings key row and one `## \`export` heading each produced a named build
|
|
failure. The settings guard was strengthened during review — it searched for a
|
|
bare key, which prose and the annotated example could satisfy after a field's row
|
|
was deleted; it now anchors on `| \`key\` |`.
|
|
|
|
Ruling 3 held. The tutorial ran end to end twice, the second time after the review
|
|
fixes. DoH and DoT answered real queries, `POST /api/certs/reload` returned both
|
|
its success and its failure payload, the export/import round trip was
|
|
byte-identical, a Docker image was built and its container served DNS, and the
|
|
offline password change was proven (old password 401, new 200). What could not run
|
|
here — root-only steps, a second LAN host, a Raspberry Pi, a registry push, the
|
|
full-scale bench, `dnsperf` — is marked in the page that documents it, with the
|
|
reason.
|
|
|
|
Five review rounds: 15 findings, then 8, then 1, then 1, then clean. One round-1
|
|
finding was rejected on evidence (`troubleshoot.md` already stated the verified
|
|
exit codes; the contradiction was in `reference/cli.md`).
|
|
|
|
## Fix wave (supersedes ruling 9)
|
|
|
|
Ruling 9 kept the documentation wave docs-only so that writing pages could not
|
|
churn behaviour underneath itself. That job is done and committed at 16c9de2.
|
|
The nine discrepancies the wave found are now fixed inside milestone 13, on the
|
|
user's direction; milestone 14 stays packaging and publishing.
|
|
|
|
Binding rulings for the fix wave:
|
|
|
|
F-a. **One definition of "the operator's configuration is wrong."** The
|
|
divergence in D1 exists because `app.isConfigFault` and `cli.failureExitCode`
|
|
each carry their own list. Neither list is the fix. Add `src/config/faults.zig`
|
|
with exactly:
|
|
|
|
```zig
|
|
pub fn isConfigFault(err: anyerror) bool
|
|
```
|
|
|
|
covering the seed and validation errors (`ParseZon`, `ConfigTooLarge`,
|
|
`MissingDefaultGroup`, `NoUpstreams`, `NoUsableUpstreams`, `BadBindAddress`,
|
|
`BadRateLimit`, `BadCertificate`, `PasswordAndHashBothSet`, and every
|
|
`validate.ValidateError`). `app.zig` and `cli.zig` both call it and keep no
|
|
private list. `run`, `check` and `import` then agree: a rejected configuration
|
|
file is exit 2 from every subcommand.
|
|
|
|
F-b. **Diagnostics carry severity.** `validate.Diagnostics` gains a severity per
|
|
item — `.fail` or `.warn`. `writeAll` prints `FAIL `/`WARN ` accordingly.
|
|
Counting splits: failures set exit 2, warnings never change an exit code. This
|
|
is the API D2 and D5 both need, so it is pinned here rather than invented twice.
|
|
|
|
F-c. **`check` never writes.** It opens `config.db` read-only and does not
|
|
migrate. A database behind the current schema is reported, not upgraded. If a
|
|
read-only open is impossible for a database needing WAL recovery, report that as
|
|
a failure naming `nxdns run` as the fix — do not silently fall back to a
|
|
writable open.
|
|
|
|
F-d. **`check` proves what it claims.** It parses the certificate and the key and
|
|
verifies they pair, through the same code the server uses, so a green `check`
|
|
cannot be followed by `run` exiting 2 on `BadCertificate`.
|
|
|
|
F-e. **Documentation follows behaviour in the same wave.** Every page that
|
|
documents the old behaviour is corrected: exit codes in `reference/cli.md`,
|
|
`how-to/troubleshoot.md`, `how-to/install-with-docker.md`, `tutorial/first-run.md`;
|
|
the certificate gap in `how-to/troubleshoot.md` and `how-to/enable-doh-and-dot.md`;
|
|
the "check writes" note in `reference/files-and-directories.md`. The three drift
|
|
guards stay green. Ruling 3 still binds: a changed command is re-run here.
|
|
|
|
F-f. **Every fix ships with a test that fails without it.**
|
|
|
|
## The nine discrepancies
|
|
|
|
1. `nxdns run` exits 1 for seed-file errors (`ParseZon`, `MissingDefaultGroup`,
|
|
`NoUpstreams`) while `check` and `import` exit 2 for the same file, because
|
|
`app.isConfigFault` lists only `NoUsableUpstreams`, `BadBindAddress`,
|
|
`BadRateLimit`, `BadCertificate`.
|
|
2. `nxdns check` prints `OK: no problems found` and exits 0 after emitting WARN
|
|
lines.
|
|
3. `nxdns check` never parses a certificate or tests that the key matches it — it
|
|
checks path readability and key mode only. A green `check` is followed by
|
|
`run` exiting 2 with `BadCertificate` on a mismatched pair.
|
|
4. `check --config <missing file>` exits 1; the implicit path prints `nothing to
|
|
check` and exits 2.
|
|
5. A blocklist source attached to no group is silently inert, and `validate.zig`
|
|
has no diagnostic for it.
|
|
6. `nxdns check` says it validates without writing, but the `config.db` branch
|
|
chmods, enables WAL and runs migrations.
|
|
7. `pruneOrphans` matches only `.list`/`.wild`, so an orphaned `.raw.tmp` is never
|
|
swept.
|
|
8. `isEmpty` counts auto-materialised client rows, so a server that has answered
|
|
one query ignores a seed file placed afterwards.
|
|
9. PLAN §18 and the docs say 10 MB / 15 MB; CI asserts 10 MiB / 15 MiB.
|
|
|
|
## Fix wave delivered
|
|
|
|
All nine are closed. Each shipped with a test its author watched fail with the
|
|
implementation reverted — ruling F-f was enforced by demanding the observed
|
|
failure output, not an assertion that a test would fail.
|
|
|
|
D1 is one `src/config/faults.zig` deriving the fault set by comptime reflection
|
|
over `validate.ValidateError`, with no exclusion list; `run`, `check` and
|
|
`import` all exit 2 on the same rejected file, for `MissingDefaultGroup` and for
|
|
`ParseZon`. D2 and D5 rest on `validate.Diagnostics` gaining `.fail`/`.warn`:
|
|
`check` prints `OK: no failures found, 1 warning` and exits 0. D3 proves the
|
|
certificate pair through `cert_store.CertStore.init`, the same code the
|
|
listeners use, so a green `check` cannot be followed by `run` exiting 2 on
|
|
`BadCertificate`. D4 fixes the missing-file exit at the root read. D6 opens
|
|
`config.db` with `OpenMode.immutable` and no migrate; a stale `-wal` is reported
|
|
naming `nxdns run`, never read past. D7 sweeps all five suffixes. D8 counts
|
|
`hand_edited = 1` only. D9 is MiB everywhere CI asserts MiB.
|
|
|
|
Four defects the wave found that were not among the nine:
|
|
|
|
1. **`pruneOrphans` had no production caller.** D7's widened matching was
|
|
unreachable at runtime. `Manager.sweepOrphans` now runs at startup, before
|
|
each interval pass, and on `DELETE /api/blocklists/:id`.
|
|
2. **An import destroyed a device's observed timestamps.** `first_seen` and
|
|
`last_seen` are runtime state, not configuration, and the model carries no
|
|
field for either — so a configured row was inserted with the import clock in
|
|
both columns. They now follow the address: a device the database already knew
|
|
keeps them, only an unseen address takes the import's clock, and a client the
|
|
file omits is removed with its history. Verified live, not only in tests.
|
|
3. **A blocklist or upstream url reached the log whole.** A signed url or an
|
|
`?apikey=` query persisted in journald. `src/safe_url.zig` exports `redact`,
|
|
which keeps scheme, host and port and drops userinfo, path, query and
|
|
fragment. It scans rather than parses, deliberately: `error.BadUrl` is one of the
|
|
failures these very lines report, so the inputs a parser refuses are exactly
|
|
the ones that must still redact. Twenty-two call sites across `manager.zig`,
|
|
`validate.zig`, `cli.zig` and `app.zig`.
|
|
4. **`src/main.zig` built both runner writers in positional mode.** With stderr
|
|
redirected to a regular file, runner output pwrote over what `std.log` had
|
|
already written at offset 0, and `2>>` was silently broken because pwrite
|
|
ignores `O_APPEND`. Both writers now use `writerStreaming`.
|
|
|
|
Two review findings were answered against the reviewer rather than by it, both
|
|
with evidence rather than argument:
|
|
|
|
- **A failed stderr write must not stop the server.** The reviewer wanted
|
|
`seedFromFile` to propagate an output failure as a runtime failure. Implementing
|
|
that proposal and running the suite showed it replacing
|
|
`error.MissingDefaultGroup` with `error.WriteFailed` — a broken stderr would
|
|
hide why the seed file was refused. The discards stay, and the reasoning now
|
|
sits above them, with the derived half of the claim labelled as derived.
|
|
- **The log-injection hole was already closed for `std.log`.** `logging.zig:342`
|
|
escapes control bytes in every log message, so the forged-line scenario the
|
|
reviewer described could not happen through that path. The live gap was
|
|
`cli.zig`'s stdout. Escaping stays in `safe_url` as well as the sink, because
|
|
`validate.Diagnostics` builds `Problem.message` as an allocated string that
|
|
`web/handlers/mutations.zig` returns as a 400 body — a channel no log sink can
|
|
escape.
|
|
|
|
Redaction ended stricter than it started. `redact` prints scheme, host and port
|
|
only: a NextDNS DoH upstream is `https://dns.nextdns.io/abcd12`, where the path
|
|
segment is the whole account identifier, so keeping the path kept the credential.
|
|
The rule has no exemption for `nxdns check`'s stdout, which is the output an
|
|
operator pastes into a bug report. Log lines identify a source by row id and name
|
|
instead, and the two duplicate diagnostics now name the other entry
|
|
(`duplicate of upstreams[0]`) rather than quoting a url that no longer shows why
|
|
the two collide.
|
|
|
|
One credential redaction cannot remove, pinned as a test and documented rather
|
|
than hidden: a NextDNS **DoT** upstream is `tls://abcd12.dns.nextdns.io`, which
|
|
carries the same identifier in the hostname. Removing it would leave no host and
|
|
no actionable line. A hostname is resolved publicly and offered as SNI in any
|
|
case, so it is not private the way a query string is, and dropping every host
|
|
would cost every operator a diagnostic to cover one vendor's choice. The
|
|
reference page names the mitigation the operator controls instead: NextDNS also
|
|
publishes a DoH endpoint whose identifier sits in the path and is redacted whole.
|
|
|
|
Four rounds of review hardened `redact` and each one found what the last missed.
|
|
Three of the four were the same root cause, which is worth naming because the
|
|
first two fixes treated it as bad luck: **the scanner tried to identify a host
|
|
inside text that is not a url, and guessed.** It guessed the query was safe to cut
|
|
before the userinfo, leaving `user:pa55` as the host. It guessed a `\` was not a
|
|
separator. And it guessed which side of an `@` was the host when a late delimiter
|
|
made both readings available.
|
|
|
|
That third one was recorded here, in an earlier revision of this section, as an
|
|
accepted cost: an `@` in a backslash path prints "a misleading host, never a
|
|
credential". **That claim was false and is retracted.** Running the shipped code
|
|
disproved it:
|
|
|
|
```
|
|
https://lists.example?token=prefix@hunter2 -> https://hunter2
|
|
https://lists.example#f@hunter2 -> https://hunter2
|
|
https:\\lists.example\p@hunter2 -> https://hunter2
|
|
```
|
|
|
|
A query string is the most likely place in a url for a token, so the text the
|
|
scanner promoted to "host" was the secret itself. The note claiming otherwise is
|
|
why three subsequent rounds passed over it.
|
|
|
|
The fix is a rule rather than a fourth special case: **the scan no longer guesses,
|
|
it declines.** The `/` cut now runs first and unconditionally, which is safe for
|
|
the reason the old ordering missed — everything after the `/` is dropped anyway,
|
|
so an `@` there never needed to be userinfo to stay out of the log. Where two
|
|
readings genuinely survive, the authority is omitted whole and `format` prints
|
|
`(ambiguous authority omitted)`, which is prose rather than a placeholder host so
|
|
an operator reads it as a statement about the line. `SafeUrl.authority` is
|
|
therefore `?[]const u8`: empty and `null` are different answers, one saying the
|
|
url names no authority and the other saying it names one that cannot be resolved.
|
|
The same rule caught a case nobody had raised — `https:a@hunter2`, where RFC 3986
|
|
reads `hunter2` as a path segment and WHATWG reads it as the host. The
|
|
disagreement between two parsers is itself the evidence of ambiguity.
|
|
|
|
A redesign was considered and rejected: parse with `std.Uri.parse` first and print
|
|
nothing but the scheme when the parse fails. It reaches the same "do not guess"
|
|
place, but it also discards the host for every url malformed in a harmless way,
|
|
and `redact` exists to be callable from the `error.BadUrl` paths that report
|
|
exactly those. The ordering fix gets the property without the cost.
|
|
|
|
Escaping is owned by the type that introduces the delimiter: `quoteText` writes
|
|
its own quotes and escapes `'` inside them, so a caller cannot reopen the hole by
|
|
adding quotes of its own. Escaping inside a plain helper would have left the
|
|
defect one caller away, which on a third review round is not a fix.
|
|
|
|
`SafeUrl` was then found to break that same rule from the other side. Its `format`
|
|
hardcoded the `none` delimiter, so it never escaped `'` — while eight call sites
|
|
wrapped it in `'{f}'` of their own. `https://ho'st/x` redacts to `https://ho'st`,
|
|
which inside a caller's quotes reads as `'ho'` followed by loose text. Two
|
|
independent findings converged on it: this review, and the `/metrics` work, which
|
|
hit the same shape with `"` instead of `'`.
|
|
|
|
`redactQuoted` closes it, and the choice between the two is a stated rule rather
|
|
than per-call-site judgement: **use the quoted form whenever anything follows the
|
|
url on the line**, because a redacted authority can still hold a space, a `:` and a
|
|
`'`, and unquoted it can impersonate whatever comes next — `upstream {f} failed:
|
|
{t}` with an authority of `ok failed: Timeout` reports a failure that did not
|
|
happen. Bare `redact` is for the two cases where that cannot arise: the url ends
|
|
the line (`cli.zig`'s `OK upstreams[N]`, `context.zig`'s missing-id line), or the
|
|
caller owns the escaping for a delimiter of its own (`metrics.zig`). Ten sites
|
|
moved to the quoted form. The output is byte-identical for any url without a `'`,
|
|
so no documented output changed.
|
|
|
|
One property is asserted rather than assumed, because it is what makes a `\` in the
|
|
output always this file's and never the operator's: a `\` ends an authority, so
|
|
unlike a source name it can never reach the value to be doubled.
|
|
|
|
Round five found two more, both in the same scanner, which is now five rounds and
|
|
five findings. Both were confirmed by running the code before being fixed.
|
|
|
|
The first is the ambiguity rule applied to only half its cases. `https:a@hunter2`
|
|
was withheld, but `https:hunter2` — the same opaque path with no `@` in it —
|
|
printed whole, because the check sat *after* the `@` lookup and a url with no `@`
|
|
returned before reaching it. The reading is ambiguous either way; the `@` was never
|
|
what made it so. The check now runs before the `@` lookup.
|
|
|
|
Moving it exposed the reason it had been placed there: `localhost` satisfies the
|
|
scheme production, so a rule keyed on the production alone withholds
|
|
`localhost:8080/x`, which is the shape an operator on a LAN is most likely to
|
|
write. A port is not an opaque path, so the digits after the colon are what
|
|
separate the two. This is checked against the trimmed authority, not the raw one,
|
|
or `localhost:8080?x` would fail the digit test on the query.
|
|
|
|
The second: a network-path reference (`//lists.example/hosts.txt`) redacted to the
|
|
empty string, because the unconditional `/` cut lands at byte zero. Not a leak —
|
|
nothing was printed — but the line named no source at all, and the authority in it
|
|
is not in doubt. A leading run of `/` is now consumed the way a scheme delimiter's
|
|
is, so the userinfo in `//user:pa55@lists.example/x` is dropped rather than the
|
|
whole authority withheld. An ambiguous one such as `//a?b@c` is still withheld: the
|
|
branch settles where the authority starts, not that every reading of it is
|
|
resolved.
|
|
|
|
Round six found three more in the same function, all the same class again: a
|
|
separator run that was read as an authority delimiter when it does not settle one.
|
|
|
|
`https:/hunter2` and `https:///hunter2` printed the path segment as the host. The
|
|
scan accepted a delimiter of *any* number of separators, and the comment recording
|
|
why was accurate when it was written and stale by the time it was read: the
|
|
tolerance existed so `https:/user:pass@host/list` would find an authority instead
|
|
of printing its userinfo. That reason expired when the `/` cut moved ahead of the
|
|
userinfo lookup in round four — the authority of a url with no `://` now ends at
|
|
its first `/`, so it holds no userinfo to print. Only a run of exactly two
|
|
introduces an authority; one leaves an absolute path and three or more is an empty
|
|
authority to RFC 3986 and a host to WHATWG. The four inputs that motivated the old
|
|
tolerance are still safe, now by being withheld rather than resolved, and that is
|
|
asserted where the old behaviour used to be.
|
|
|
|
`///lists.example/x` had the same defect in the network-path branch added one
|
|
round earlier, which consumed the whole run. Exactly two there too.
|
|
|
|
The third retracts something this section claimed one round ago. `isPort` was
|
|
introduced so a schemeless `localhost:8080` would keep resolving, on the reasoning
|
|
that a port is not an opaque path. It is not that simple: `https:123456` is an
|
|
opaque path whose digits are as much a token as any other text, and the exception
|
|
printed it whole. The reviewer proposed excluding known schemes from the
|
|
exception; the exception is gone instead, because a list of known schemes is the
|
|
kind of thing that goes stale silently and this file already has one rule that
|
|
covers it. `localhost:8080/x` is now withheld, which costs nothing an operator
|
|
needs: a url reaching this without a scheme is one the validator is rejecting, and
|
|
the field path beside it names which. An `IP:port` is unaffected — a leading digit
|
|
fails the scheme production, so `10.0.0.2:8080` and `[::1]:853` still resolve.
|
|
|
|
Round seven took the same rule one step further. "Exactly two separators" still
|
|
accepted `\\`, `/\` and `\/`, and WHATWG converts a `\` to a `/` only for a
|
|
*special* scheme — so `https:\\hunter2` has the same split reading as the cases
|
|
above, and `tls:\\hunter2` has no reading at all under which `hunter2` is a host,
|
|
`tls` not being special. RFC 3986 gives `\` no meaning anywhere. Only `//` opens
|
|
an authority now. A `\` still ends one, and still anchors the scheme scan so a
|
|
backslash-pasted url is reported by its scheme, but it opens nothing.
|
|
|
|
Round seven also caught a stale comment, which is the second time in two rounds
|
|
that a comment outlived the reason it recorded. Both said something true when
|
|
written and false when read, and both were load-bearing — the first is why the
|
|
any-length tolerance survived three rounds after its justification expired.
|
|
|
|
Seven rounds, nine findings, one function. Every one was a place where the scan
|
|
committed to a reading the text did not support. What finally holds is not a
|
|
sharper scan but a smaller claim: the authority is printed only where exactly one
|
|
reading survives, and withheld everywhere else. Concretely, an authority is read
|
|
only after a literal `//`, whether a scheme introduced it or not.
|
|
|
|
The cost is paid by malformed input alone, and it is paid in diagnostic detail
|
|
rather than in safety: a backslash-pasted or schemeless url now reports its scheme
|
|
and no host. Every such url is one the validator is already rejecting, and the
|
|
field path or row id beside it names which one. A sweep of eighteen shapes
|
|
carrying `hunter2`, `abcd12`, `s3cr3t`, `token`, `pa55` and `123456` finds none of
|
|
them in any output.
|
|
|
|
Round eight returned no critical and no important finding, and two minor ones that
|
|
cut in opposite directions. A run of three or more slashes was reported as an
|
|
authority the url does not have, when it is contested rather than absent — RFC
|
|
3986 reads an empty authority, WHATWG resolving against a special-scheme base
|
|
reads a host. `SafeUrl` exists to keep those two answers apart, so it is now
|
|
withheld rather than reported as empty.
|
|
|
|
The other corrected a claim this file made about the very thing it was withholding.
|
|
`localhost:8080` is **not** contested: `localhost` is not one of WHATWG's six
|
|
special schemes, so both standards read a scheme and an opaque path, and the
|
|
host-and-port an operator meant is a reading no parser offers. It is still
|
|
withheld — the text after the colon is a path segment under every reading — but the
|
|
marker calls it an authority that could not be resolved when the honest answer is
|
|
that there is none.
|
|
|
|
That wording defect was first recorded rather than fixed, on the grounds that
|
|
telling the two apart needs a scheme list and a scheme list is what the previous
|
|
round had just removed. Round nine rejected that and was right to: the list round
|
|
six removed described *what nxdns supports*, so it went stale whenever a transport
|
|
was added. WHATWG's special schemes are a closed set fixed by the URL Standard —
|
|
`ftp`, `file`, `http`, `https`, `ws`, `wss` — which never described nxdns and
|
|
cannot go stale with it. Conflating the two was the error, and knowingly shipping
|
|
a diagnostic that contradicts its own type's contract is the tech debt this
|
|
project does not take on.
|
|
|
|
So `redact` now distinguishes them. Only a special scheme can disagree with RFC
|
|
3986 about text no `//` introduced, so `https:hunter2` is withheld as contested
|
|
and `localhost:8080`, `mailto:ops@example.com` and `tls:\\host` are withheld as
|
|
naming no authority at all. Round nine's other finding was the same rule missing
|
|
from the schemeless branch: `\\lists.example\path` and `/\lists.example/path` were
|
|
reported as absent when they are contested. A run is settled when it is one
|
|
separator, or when the reading that looks for a host finds none —
|
|
`\\?\C:\lists\hosts.txt` ends its authority at the `?` under both — and contested
|
|
otherwise.
|
|
|
|
Round ten returned nothing critical and nothing important, four minor findings and
|
|
a nit. Two were taken: the nit, which was a comment claiming
|
|
`https:\\dns.nextdns.io\abcd12` "names a host" when the file's own reasoning is
|
|
that it is contested; and the rendering question — `tls:\\host` prints `tls://`
|
|
although the input held no `//`. That one is now contractual rather than
|
|
accidental. `format` renders `scheme://authority` canonically and does not quote
|
|
the input's syntax; nothing about which bytes separated the scheme survives
|
|
redaction, and nothing should, because the input is not meant to be reconstructible
|
|
from the output.
|
|
|
|
**Three findings were declined, and the reason is checked rather than asserted.**
|
|
`https:/user@/x`, `\path@hunter2` and `file:secret` are classified as contested
|
|
when both readings in fact find no host. Running them shows why that is tolerable:
|
|
each prints `(ambiguous authority omitted)`, so each says *less* than it could and
|
|
none prints the path segment. The mechanisms are named in the file — emptiness
|
|
tested before userinfo removal, a leading run of one reaching the late-delimiter
|
|
rule, and `file` having its own WHATWG parsing states that `isSpecialScheme` does
|
|
not model. Fixing them means modelling more of two standards for inputs no
|
|
accepted configuration can hold: every url this program takes carries `http`,
|
|
`https`, `tls`, `udp` or `tcp` and a `//`. All three are pinned by a test, because
|
|
the failure that would matter is the opposite one — if any of them ever starts
|
|
naming a host, that test fails.
|
|
|
|
That is where the review loop was stopped, on a judgement rather than on an empty
|
|
round. Rounds eight, nine and ten each returned nothing critical and nothing
|
|
important, and the findings had moved from "this prints a secret" to "this claims
|
|
more than it knows about a url no operator can configure". Round ten opened a new
|
|
sub-class of its own — WHATWG's `file:` state machine — which is the signal that
|
|
the remaining work is unbounded and no longer about safety.
|
|
|
|
Ten rounds, sixteen findings, one function. A sweep of twenty-five shapes carrying
|
|
`hunter2`, `abcd12`, `s3cr3t`, `token`, `pa55`, `123456`, `hosts.txt` and
|
|
`example.com` finds none of them in any output.
|
|
|
|
The last gap was found by sweeping every log site in the tree rather than
|
|
trusting the review's file list: `src/upstream/dot_client.zig` printed the
|
|
upstream url whole at four sites and `pool.zig` at a fifth. Three review rounds
|
|
and four agents had redacted urls across `manager.zig`, `validate.zig`, `cli.zig`
|
|
and `app.zig` without anyone asking which other modules logged the same values.
|
|
|
|
Deliberately not changed: `POST /api/blocklists` still ignores the
|
|
`SourceInNoGroup` warning. The web flow creates a source before any group link
|
|
can exist, so the warning is structural at that moment and a 400 would be wrong.
|
|
The three CLI paths carry it instead.
|
|
|
|
Claims in the wave that no test backs, recorded rather than buried: the two
|
|
`app.zig` `log.warn` redactions, because a `std.log` line is not observable from
|
|
a unit test under the default runner; `cli.zig`'s `not a usable DoH url`
|
|
redaction, argued unreachable by any credential-carrying url because
|
|
`Endpoint.parse` rejects `@?#` in the authority first; the two probe lines that
|
|
need a reachable upstream; and the WAL guard's residual window, which is argued
|
|
rather than observed.
|
|
|
|
## Round four
|
|
|
|
The `/metrics` family was the last place a credential still reached an
|
|
unauthenticated reader. `GET /metrics` is `.auth = .open` in `src/web/routes.zig`
|
|
and `web.bind` defaults to `0.0.0.0`, so an unauthenticated `curl` printed
|
|
`nxdns_upstream_up{url="https://dns.nextdns.io/abcd12"} 1` with HTTP 200. Every
|
|
other redaction in this wave was on a path that already required a session or a
|
|
shell on the host; this one was not.
|
|
|
|
Redacting it needed a second escaping layer rather than a call to `redact`.
|
|
`redact` cuts the authority at `/@?#\` and escapes every control byte, so it can
|
|
emit neither a raw newline nor a lone backslash — but `"` is in none of those cut
|
|
sets, so `https://ho"st/x` arrives at the label as `https://ho"st` and closes the
|
|
label value, letting the rest of the string write label pairs of its own.
|
|
`writeLabelValue` therefore runs *over* the redacted text. The ordering also
|
|
matters in the other direction: doubling the backslash turns the two characters
|
|
`redact` writes for a control byte into an unambiguous `\\n`, so a parser reads a
|
|
backslash rather than a newline.
|
|
|
|
The redaction then introduced a defect of its own, caught before it shipped: two
|
|
upstreams on one host redact to one label set, so `nxdns_upstream_up` rendered
|
|
twice with identical labels in a single exposition. Two NextDNS profiles is a
|
|
realistic configuration. The fix labels each sample with its array position in
|
|
`renderUpstreams`, so uniqueness holds by construction and a hand-built `Sample`
|
|
cannot forge a collision. The index is positional and therefore stable only while
|
|
the pool order is — `pool.Snapshot` carries no row id, and threading one through
|
|
the pool and the repository was judged out of scope for a review round. The
|
|
caveat is in the file. It only bites when two upstreams share an origin; for
|
|
distinct origins the url carries the identity and a reorder is harmless.
|
|
|
|
Claim scoping on that defect, since the three statements are not one: the
|
|
duplicate exposition text **was** reproduced, out of the same `render` function
|
|
`/metrics` calls. It was **not** reproduced over HTTP from a pre-fix server — every
|
|
live run had the index in place. And "a scrape carrying it is rejected" is
|
|
**reasoned, not observed**; nothing was fed to a Prometheus. Whether a scraper
|
|
rejects the pair or keeps the last sample, the down upstream is lost, which is the
|
|
part the fix rests on.
|
|
|
|
`src/storage/repositories/context.zig` printed a blocklist url whole and a group
|
|
name raw on its two `NotFound` paths. Both are invariant-failure paths that fire
|
|
rarely, which is why they outlived three rounds of sweeping.
|
|
|
|
One test file was never running. `src/config/faults.zig` was absent from
|
|
`src/tests.zig`, and Zig collects tests only from the root module's explicit
|
|
import list — a transitively imported file contributes none, even when its values
|
|
are used, which was confirmed against a scratch project rather than assumed. The
|
|
five reflection guards in that file are what catch a future `ValidateError`
|
|
variant being added and left unclassified; they had never executed. Its runtime
|
|
behaviour was covered — `cli.zig` has a test that classifies through it — so the
|
|
gap was in the regression guard, not in what shipped. A sweep of the tree found no
|
|
other unlisted file with tests; `dns/dns.zig` is a re-export barrel with none, and
|
|
its own comment documents this rule.
|
|
|
|
## Acceptance (milestone complete)
|
|
|
|
- [x] The five old `docs/*.md` files are gone; the four directories and the index
|
|
exist with every page in ruling 6.
|
|
- [x] Every command in `tutorial/` and `how-to/` was executed on this host, or is
|
|
marked in-page as unverified with a reason.
|
|
- [x] `zig build test` passes with the three drift guards pointing at the new
|
|
reference pages: 16/16 steps, 1254 pass, 115 skip (integration-gated), 0 failed.
|
|
- [x] `zig build test -Dintegration` passes: 16/16 steps, 1365 pass, 4 skip (the
|
|
live-network TLS tests excluded by milestone-1 design), 0 failed.
|
|
- [x] No page mixes modes: no procedure in `reference/`, no field table in any
|
|
`how-to/` or `tutorial/` page.
|
|
|
|
## Anti-requirements
|
|
|
|
- No new documentation subjects (no metrics-families reference, no security-model
|
|
page) — this milestone moves and splits existing content plus the tutorial.
|
|
- No redirect stubs or `docs/legacy/`.
|
|
- No source fixes; report discrepancies instead. (Held for the documentation
|
|
wave. Superseded by the fix wave above, which closes the nine it reported.)
|
|
- No doc generator, no site builder, no MkDocs.
|