counters/review/reviewer-b-security-contracts.md

# Review B — security, contracts and operability

- Date: 2026-10-13/14 (interrupted by an API usage limit, resumed)
- Scope: the whole repository at the state of the first implementation; read-only
- Lens: HTTP validation layer, input normalisation and keys, web-app notification contract, config and secrets, counterctl, metrics and /health, docs vs code, test quality
- Proof tests: `_proof_tests/b/` (see `_proof_tests/README.md`)
- Verification by the coordinating agent and the merged ranking: `README.md`

The report below is the reviewer's text as delivered. Paths under `/tmp/review_b` refer to its scratch copy, which no longer exists; the fixture `bad.cdb` now lives at `_proof_tests/b/bad.cdb`.

---

## Findings (most severe first)

Scratch copy: `/tmp/review_b`. My proof tests are `internal/server/review_b*_test.go`, `cmd/counterctl/review_b_test.go`, `internal/config/review_b_test.go` and the fixture `/tmp/review_b/bad.cdb`. Nothing under /Users/dice/Work was modified. No Redis server was started; every Redis was miniredis, so there are no processes to kill.

---

**F1 — high — contract / security (limit bypass)**
- location: `internal/api/validate.go:35,49` (optional `mail_from` goes through `requiredEmail`), `:32`, `:78-103` (`NormalizeEmail`: ASCII-only domain, ≤254, no whitespace); `internal/server/server.go:115-118,133-136`
- summary: If any field fails the strict syntax check, the whole request is rejected with 400. That includes `mail_from`, which is never counted. So the sender alone decides whether the recipient's increments happen, and on /out a recipient address can drop the sender's outgoing increments.
- failure scenario: An external sender floods `b@yagolivas.com` with `MAIL FROM:<"john smith"@x.com>`. mizu's go-smtp fork (`migadu/go-smtp parse.go:208-224`) unquotes this to `john smith@x.com`. mailqueuer never validates X-Mail-From. Other spellings work the same way: `<"a@b"@x.com>`, `<info@bücher.de>` (mizu sets `EnableSMTPUTF8=true`), `<postmaster@[192.0.2.1]>`, `<x@mail_relay.example.com>`, or any path over 254 chars. Every /in call then returns 400 and the recipient's A/D/O incoming counters never move, so the incoming limit never fires. Legitimate SMTPUTF8 senders on IDN domains are not counted either. On /out, a recipient like `x@[192.0.2.1]`, `x@bücher.de` or `x@host_name.example.com` makes the call 400, so the authenticated sender's A/D/O outgoing increments are lost. A 400 also writes nothing to the counting log (see F9), so the loss is invisible there.
- evidence: `TestReviewB_InMailFromDropsRecipientCount`: all 6 mail_from variants return `400 {"error":"mail_from: invalid e-mail address"}` and leave `keys written: []`. `TestReviewB_OutRecipientDropsSenderCount`: all 3 return `400 recipient: invalid e-mail address`, and `cnt/A/a@yagolivas.com/.../o` is never created.
- verdict: CONFIRMED for the service behaviour. The end-to-end path is PLAUSIBLE, based on reading the go-smtp parser and confirming that mailqueuer does not validate the from header.
- fix: On /in, and on /out for `mail_from`, never reject because of `mail_from`: sanitise it for logging and treat it as non-exempt. Convert U-label domains with `idna.Lookup.ToASCII`. On /out, validate sender and recipient independently and count whichever side is valid. Keep 400 only for a missing or garbage *counted* address.

**F2 — medium — input normalisation (two spellings, two counters)**
- location: `internal/counting/counting.go:164-198` (A id = raw `address`; the override lookup uses the raw local part); `internal/api/validate.go:78`
- summary: Plus-addressed recipients get their own A key per tag. The mailbox's own A counter, the one the web app reads and charts, is never incremented, and the per-mailbox `local@/i` override is never applied.
- failure scenario: rcptd strips `+tag` when validating (see `rcptd/internal/tables/regex_plustag_test.go`), so mizu accepts `b+1@…`, `b+2@…` and so on. Mail sent to `b+N@yagolivas.com` creates `cnt/A/b+N@yagolivas.com/...` keys, each starting at 0. The mailbox `b@yagolivas.com` never reaches A/i. When a tag key does cross, the web app gets `id=b+N@…`, `get_active_mailbox_by_address` returns nil, and it only sends a "mailbox not found" Telegram canary. An attacker can create unbounded keys, each with a 31-day TTL. The Lua plugin had the same behaviour, but the plan's goal is keys the web app reads.
- evidence: `TestReviewB_PlusTagSeparateCounters`: keys = `cnt/A/b+1@…/i, cnt/A/b+2@…/i, cnt/A/b+3@…/i, cnt/D/…, cnt/O/1049/…`; `cnt/A/b@yagolivas.com/.../i` is missing.
- verdict: CONFIRMED
- fix: Canonicalise the A-level id and the override local part by stripping `+…` from the local part (rcptd's recipient_delimiter), and apply the same rule in `/counters`.

**F3 — medium — correctness / notification contract**
- location: `internal/store/store.go:123-143`, `internal/server/server.go:155-167`, `internal/txlog/txlog.go:111`, `internal/counting/counting.go:204-206`
- summary: If Redis applies MULTI/EXEC but the reply is lost (read timeout, connection reset, handler timeout), the service answers 503 and logs "nothing counted". The counters were incremented, though. If that increment was the crossing one, no later increment can cross again, so the web app is never notified for that level/id/flow/day.
- failure scenario: A/o limit 20, counter at 19. The master stalls longer than `timeout_ms` (fork, failover). Redis applies the INCR (value 20), go-redis returns `i/o timeout` (txPipeline reads are not retried), and the client gets 503. The counting log says `redis error … (nothing counted)`. The next mail gives count 21 with `reached:false`, and no notification is ever sent.
- evidence: `TestReviewB_LostCrossingOnReplyTimeout` uses a delaying TCP proxy in front of miniredis. Request 1 returns `503 redis incr: read tcp …: i/o timeout`, and Redis then holds `20`. Request 2 returns `"count":21,"limit":20,"reached":false`. Zero notifications.
- verdict: CONFIRMED
- fix: For a failed or ambiguous EXEC, do not claim "nothing counted". Make crossing detection recoverable: on the next successful increment, notify when `old >= limit` and a per-day marker (`SET NX`) is absent; or use a marker key set together with the INCR. Log "outcome unknown" instead of "nothing counted".

**F4 — medium — config / auth**
- location: `internal/server/middleware.go:140-142`; `internal/config/config.go:296-300` (empty token is only a warning), `:151-153` (unknown keys are only warnings)
- summary: An empty `auth_token` turns off authentication on /in, /out and /counters, and a typo in the key name produces that state with only a log warning. The plan's step 4 makes the bearer token mandatory.
- failure scenario: `auth-token = "s3cret"` and `allowed_cidr = ["10.0.0.0/8"]` (typos), or `MIGADU_COUNTERS_AUTH_TOKEN` missing from the rc environment. The daemon starts, anyone who can reach :8095 can inflate any customer's counters (which triggers the web app's limit actions) or read counters, and the allowlist is silently empty.
- evidence: `TestReviewB_MisconfigStarts` "typo token": Load OK with `AuthToken="" prefixes=[]` and three warnings only.
- verdict: CONFIRMED
- fix: Make an empty `auth_token` a validation error, or require an explicit `insecure_no_auth = true`. Make unknown keys fatal, or at least fatal for security-relevant near-misses.

**F5 — medium/low — CDB robustness / spec deviation**
- location: `internal/domains/domains.go:116-124` (`ParseRecord` errors on a bad org id), `internal/counting/counting.go:172-180`, `internal/server/server.go:145-151`
- summary: One CDB row with a non-numeric org id causes 503 for every request that touches that domain. On /out it also drops the other party's valid increments. The plan says malformed rows are skipped, not fatal; Lua treated such a row as not hosted.
- failure scenario: The row `bad.com x12|A/i=5` is deployed. `/out a@good.com → b@bad.com` returns 503, and `a@good.com`'s outgoing is not counted. Every `/in` to bad.com returns 503.
- evidence: `TestReviewB_MalformedOrgIDDropsOtherParty` (fixture `/tmp/review_b/bad.cdb`): `503 {"error":"domain lookup failed: … invalid org id \"x12\""} keys=[]`.
- verdict: CONFIRMED (the real fixture has no such rows today)
- fix: Have `Lookup` return `ErrNotFound`, or a dedicated skip error, for unparsable records, log once, and count the other party.

**F6 — low — counterctl date warning**
- location: `cmd/counterctl/main.go:88-90`
- summary: The comparison is `now − midnight(D) > 31d`, so the warning fires for a date exactly 31 days back and claims its keys "will read as 0". The plan says "older than 31 days".
- failure scenario: The current time is 2026-10-13 12:00 UTC and the operator runs `--date 2026-09-12`. That day's keys were last refreshed around 23:59 and live until about 2026-10-13 23:59, so real non-zero numbers print alongside "more than 31 days ago … will read as 0".
- evidence: `TestReviewB_31DayWarningOffByOne`: the warning is printed; a key last incremented at 09-12 23:59 expires 10-13 23:59. The existing test covers only 12 and 42 days back.
- verdict: CONFIRMED
- fix: Warn only when `today − D ≥ 32 days` (always expired). At exactly 31 days, say the counts may be partial.

**F7 — low — secrets in logs**
- location: `internal/config/config.go:279-287` (userinfo in notify URLs accepted, `http://` with basic auth accepted without warning), `internal/notify/notify.go:147`, `internal/txlog/txlog.go:125,133`, `internal/config/logger.go:110` (mode 0644)
- summary: Credentials given as URL userinfo work, because Go sends them as basic auth. The counting log then prints the full URL, password included, on every sent or failed notification, into a world-readable file.
- evidence: `TestReviewB_NotifyURLCredentialsLogged`: the server sees `Basic YWRtaW46aHVudGVyMg==`, and the log line is `sent limit exceeded notification to url "http://admin:hunter2@127.0.0.1:…/api/limits?…"`.
- verdict: CONFIRMED
- fix: Reject userinfo in the notify URLs, or redact with `url.Redacted()` before logging. Warn or fail when `user` is set with an `http://` URL.

**F8 — low — metrics**
- location: `internal/server/middleware.go:73-104`, `server.go:96` (`recoverer(observe(timed))`; `observe` has no defer)
- summary: A recovered panic is answered 500 but never counted in `requests_total`. The plan says every rejection or recovery increments `requests_total{status}`.
- evidence: `TestReviewB_PanicNotInMetrics`: status 500, `requests_total{/in,500}=0`.
- verdict: CONFIRMED
- fix: Record in a deferred function in `observe`, or put `observe` outside `recoverer`.

**F9 — low — operability / spec deviation**
- location: `internal/server/middleware.go:46-49`
- summary: Rejection logs carry no `trace_id`, although the plan requires it "if parsable". The counting log gets nothing for 4xx rejections, despite README's "Every request writes one block" and `grep '<trace_id>'` returns one transaction.
- evidence: `TestReviewB_RejectLogHasNoTraceID`: the log line is `msg="request rejected" route=/in remote=… status=400 reason="recipient: invalid e-mail address"`, without TRACE-XYZ. This combines with F1 to make the dropped counts invisible.
- verdict: CONFIRMED
- fix: Pass the sanitised trace_id and route into `reject`, and write a `rejected: <reason>` line to the txlog.

**F10 — low — information exposure**
- location: `internal/server/server.go:150,165,236,263,301`; `internal/config/config.go:117` (default `admin_addr=":9095"`, all interfaces, no auth, no allowlist)
- summary: 503 bodies, and `/health` (unauthenticated), return raw Redis and CDB errors that include internal addresses and paths. The default admin bind is public, while the code comment says to "bind admin_addr locally".
- evidence: `TestReviewB_ErrorLeak`: API returns `{"error":"redis unavailable: redis incr: dial tcp 127.0.0.1:52115: connect: connection refused"}`, and `/health` returns `{"error":"redis: dial tcp 127.0.0.1:52115: …"}`.
- verdict: CONFIRMED
- fix: Return generic messages and keep the details in the app log. Default `admin_addr` to `127.0.0.1:9095`, or warn when it is not loopback.

**F11 — low — notification contract (idempotency)**
- location: `internal/notify/notify.go:157-213`
- summary: Up to 5 attempts, with a retry on client timeout, transport error, 429 or 5xx, against a non-idempotent web action. The A branch (`Core.Email.incoming/outgoing_limit_exceeded!`, `core/email.ex:3572-3584`) emails the customer on every call. The canary Telegram message is also sent before the O-level action can raise.
- failure scenario: The web app takes more than 5 s (the per-attempt timeout) but completes. The service retries, and the customer gets 2–5 "limit exceeded" emails plus duplicate canaries. Lua sent once.
- verdict: PLAUSIBLE
- fix: Retry only on connect errors and 5xx-before-processing, or add an idempotency key (trace_id+level+id+flow+day) that the web app can dedupe on. Consider a longer timeout with fewer retries.

**F12 — low — config validation**
- location: `internal/config/config.go:257-266`
- summary: Sentinel mode is accepted without `redis.password`, although the deployment has one Redis password.
- failure scenario: The password is missing or the env var is unset. The daemon starts, every INCR gets NOAUTH and returns 503, and mailqueuer ignores 503s, so nothing is counted fleet-wide. Only `/health` and `redis_errors_total` show it.
- evidence: `TestReviewB_MisconfigStarts` "sentinel without password": Load OK, `warnings=[]`.
- verdict: CONFIRMED
- fix: Require the password in sentinel mode, or at least warn. Make a NOAUTH/WRONGPASS reply to the startup PING fatal.

**F13 — low — test-suite gaps (tests pass for the wrong reasons)**
- `cmd/counterctl/main_test.go:180-208` never tests the 31/32-day boundary, which hides F6.
- `server_test.go:482-497` checks the panic answer but not the metric (F8).
- `:422-454` checks that rejections are logged but not the trace_id (F9).
- `:372-395` never covers the empty-token open mode (F4).
- Nothing covers an ambiguous Redis failure (F3), plus-tags (F2), or a malformed org id (F5).
- `TestValidation_JSONAndFields` asserts as correct the 400 on a `mail_from` the system never counts (F1).
- Watcher tests use a 16-slot buffered `done` channel plus sleeps, so a stale signal from an earlier event could satisfy `c.wait` in `TestWatcher_ReloadsOnAtomicReplaceTwice`. This is a mild flakiness and false-pass risk.
- verdict: CONFIRMED (by inspection and by the F6/F8/F9 tests above)

## Checked, no issue found
- Middleware order matches the plan's table: method → declared size → token → source IP → content-type → MaxBytesReader. Note that auth runs before the allowlist, so a non-allowed source can tell a valid token (403) from an invalid one (401); this is minor.
- Body size: chunked 100 KB body → 413 (`TestReviewB_ChunkedTooLarge`). The declared Content-Length check sits before auth.
- Auth: missing, `Basic` and empty `Bearer ` → 401 with `WWW-Authenticate`. ConstantTimeCompare leaks only the length. The `Bearer` scheme is case-sensitive, which is fine for mailqueuer (`routing.go:257`).
- Content-Type: parameters are ignored, other types → 415.
- JSON: syntax errors, two objects and trailing data → 400. `null` / null fields → "missing" 400. Wrong types → 400 (the message exposes the Go type name). Duplicate and case-insensitive keys: last wins. That is harmless because the caller is trusted.
- allowed_cidrs: `Unmap()` handles IPv4-mapped clients. Zoned and IPv6-mapped config entries fail closed.
- Unknown paths → 404 JSON. HEAD/PUT/OPTIONS → 405 with `Allow`. `//counters` → 301 → works. Metric labels: route is bounded to 4 values, and status/level/flow/op/result are bounded.
- Log injection: the app log uses slog (values quoted). The counting text log can't receive newlines or ESC: addresses reject Cc/whitespace and trace_id strips controls. trace_id still allows spaces, `>` and non-ASCII, which deviates from the plan's "printable ASCII" but has low impact because mizu generates it.
- Redis key injection: an address has exactly one `@`, the domain is restricted to `[a-z0-9.-]` and the org id is numeric, so no collision across customers or levels is possible. `/counters?domain=` is unvalidated but read-only and confined to `cnt/D/…`.
- Notify URL contract: all six params are in the default templates. `id` is the address for A, the domain for D and the org id for O. Values are QueryEscaped. `/api/limits` routes to `InternalApi.NotificationController` with no route collision. `accepts json` is fine because there is no Accept header. COUNT is now the new value (= limit) where Lua sent limit−1; the plan intends this, and the web app only uses it in canary text.
- Exempt via `/out` mail_from: mitigated by mizu `ResolveSender` / rcptd `allowed_from`. On `/in` it behaves the same as Lua.
- go-redis txPipeline: read errors are not retried, so there is no double-increment.
- Config: db≠0 is rejected, relative `domains_cdb` is resolved against the config dir, env overrides apply only when non-empty, notify URLs are checked for an http(s) scheme.
- counterctl: config discovery, env token override, `:8095` → `http://localhost:8095`, trailing slash on `--url` works, `--url …/api` → 404 (expected), `!` uses ≥ (no zero limits exist in the CDB), exit codes are 0/1.
- `/health` checks CDB readability and a 200 ms write-side PING. The README's "drain ≤ 30 s" matches the code; the plan said 10+10 s, a documentation-only difference.
- CDB fixture: 93,549 rows, every row has all 6 base keys, there are 22,995 lowercase overrides, no non-numeric org ids, no zero or negative limits, and 112 punycode domains.