docs(changes): record the adversarial input-validation audit (sink-first negative-path)

docs/changes/2026-07-08-adversarial-input-audit.md

@@ -0,0 +1,129 @@
+# Adversarial input-validation audit — every dangerous sink fails closed
+
+- **Type:** negative-path / input-validation audit (no production code change)
+- **Date:** 2026-07-08
+- **Area:** `internal/api`, `internal/naming`, `internal/submit`, `internal/rcon`
+- **Task:** a different question from the #82 / round-2 mutation audits. Those asked
+  *do the TESTS catch a gate regression?* This asks *does the CODE reject hostile
+  INPUT, or does bad data PASS?* — feed the real endpoints malformed, boundary, and
+  hostile bodies (故意加错误数据) and confirm they fail closed (4xx) rather than letting
+  the garbage reach a sink.
+
+## Method — sink-first, not fuzz-everything
+
+The low-hanging garbage (oversized body, unknown field, wrong content-type) is already
+caught by the universal body guards, so a blanket "fuzz all ~80 handlers" would burn
+effort where the answer is known. The real "can bad data PASS?" risk lives at the
+**sinks** — the few places a request string is concatenated into an RCON command, used
+as a K8s object name, joined into a filesystem/archive path, put in a SQL query, or
+accepted as an enum/quantity **without a validator in front**. So the audit traces each
+dangerous sink class from its handler entry to the sink, and for the crown-jewel class
+(text → RCON) **mutation-verifies** the guard is non-vacuously pinned: loosen the guard
+in source, run the package tests, confirm the specifically-named negative test reddens
+(`--- FAIL: <subtest>`), then revert. Oracle: WSL Fedora-44, go1.26.4.
+
+## The universal belt (caught before any sink)
+
+`decodeJSON` (`internal/api/util.go`) wraps every body in
+`http.MaxBytesReader(w, r.Body, 1<<20)` (1 MiB cap), sets `DisallowUnknownFields()`, and
+rejects trailing data after the first JSON value — all → `400 bad_request`.
+`requireJSONContentType` returns `415 unsupported_media_type` on credential writes
+(a CSRF belt). So oversized, unknown-field, multi-document, and wrong-type bodies never
+reach a handler body at all.
+
+## The dangerous sinks — each traced fail-closed
+
+### 1. Text → RCON (the lead). Two vectors, both fenced.
+
+**(a) Structured commands** — `handlers_access.go` (LuckPerms permission/group,
+whitelist/ban/kick). Every operand is validated against an anchored allow-list charset
+*before* it is concatenated: `mcNameRe = ^[A-Za-z0-9_]{1,16}$` (player),
+`lpNodeRe = ^[A-Za-z0-9_.*-]{1,64}$` (node), `lpCtxRe = ^[A-Za-z0-9_-]{1,48}$`
+(world/group). No space, separator, or control character can appear in a validated
+operand, and **there is no free-text field anywhere** — a ban/kick deliberately carries
+no reason string (that would be the one splice vector). Go's `$` is `\z` (absolute end,
+not `\Z`), so even a single trailing `\n` is rejected.
+*Mutation-verified:* loosening `mcNameRe` to admit a space
+(`^[A-Za-z0-9_ ]{1,16}$`) reddens
+`TestAccessInjectionRejected/{whitelist,ban,kick,permission}_player_space` — the guard
+is real, not vacuous.
+
+**(b) Free-text passthrough** — `handlers_console.go` `handleCommand`, POST
+`/servers/{name}/command`. This is the *one deliberate* free-text → RCON vector, and it
+is **owner/admin-gated** (403 for a stranger, 404 for an unknown server). Its input
+fence: trim + strip a single leading `/`, reject empty, cap at 1024 bytes, and
+`strings.IndexFunc(command, func(c rune) bool { return c < 0x20 }) >= 0 → 400` — every
+C0 control (incl. `\n`) is rejected so one request cannot splice a second command.
+*Mutation-verified:* disabling the scan (`c < 0x20` → `c < 0x00`) reddens
+`TestConsoleCommand/control_character_(newline)_->_400,_no_RCON_call`, whose input is
+literally `{"command":"say hi\nop attacker"}` and whose assertion is 400 **and**
+`console.calls == 0`. (Severity note: because this vector is owner-gated by design, the
+scan is an audit-integrity measure — one request = one command — not a privilege
+boundary; a splice on your *own* server escalates nothing, since the owner may already
+run any RCON command. The fence exists regardless.)
+
+### 2. Break-glass / internal-face (the newest code — scrutinised specifically)
+
+This surface runs under a "service-token-authed / local-root, inputs trusted" posture,
+the classic place a field-level guard gets skipped. Traced end to end:
+
+- **Server name** — every internal handler (`handleReady`, `handleJoinEvent`,
+  `handleInternalWake`, `handleInternalClaim`, `handleInternalMenuStatus`) validates the
+  path name with `naming.ValidateServerName` before use.
+- **`mc_uuid`** (join/wake/claim bodies) — checked non-empty, then flows *only* to
+  DB-parameterized calls (`RecordJoin`, `UserByMCUUID`, `UUIDInAllowlist`). The
+  "allowlist" is a **DB table**, not a live RCON `whitelist add` — there is no
+  `mc_uuid` → RCON path.
+- **Break-glass "OP-create"** (`performAddOperator` → `provisionOperator` →
+  `InsertOperator(ctx, id, username, email)`) is a **parameterized DB INSERT** creating a
+  *panel staff account*, **not** a Minecraft `op` RCON command. The hypothesised
+  name → RCON `op` sink was checked and **does not exist** in this shape; the username is
+  `TrimSpace`d and reaches only `$N`-parameterized SQL, from a local-root caller.
+- **`os_user`** (internal backup attribution) — `TrimSpace`d, sets only the audit actor
+  (a DB row); shown non-vacuous in the round-2 backup/restore audit (`b7b4a3b`).
+
+### 3. SQL injection — dismissed.
+
+`pgrepo.go` uses uniform `$1/$2/$3` parameterization throughout
+(`QueryRowContext`/`ExecContext(ctx, q, args…)`); no request string is `Sprintf`'d into a
+query.
+
+### 4. Path traversal (submit) — dismissed.
+
+`internal/submit` validates the submission id (rejects `..`, path separators, uppercase,
+space, empty), and the on-disk blob name is a **fixed** constant (`contextBlobName`) — no
+attacker-supplied filename is ever joined. The hostile-id matrix
+`{"../evil","sub/../../etc","SUB-UPPER","has space","","a/b"}` is test-pinned in both the
+local and S3 backends. The one free-form field a submission carries (`DisplayName`) is
+charset-constrained by `displayNameRE` and rejects control chars
+(`submit_test.go` "control chars" case).
+
+### 5. K8s object names — validated at every cluster write.
+
+`ValidateServerName` / `ValidateSystemServerName` (`^[a-z0-9-]{3,32}$`, no
+leading/trailing dash, reserved-name set) and `ValidateHostname`
+(`dnsLabelRE`, single label under the configured root domain) gate every create/patch.
+`cluster.go` documents the invariant: "there is no free-form YAML path — every field is a
+typed, validated value," so no raw CRD field can be smuggled through a create/patch body.
+
+### 6. Numeric / enum — fail-closed.
+
+`resolveResources` routes **every** quantity (memory, `resources.cpu/memory`, requests)
+through `parsePositiveQuantity`, which rejects `q.Sign() <= 0` (negative *and* zero) with
+a field-named 400, plus a request>limit guard; `parseStorageSize` carries the same guard.
+Enums are closed sets: `parseAutostartPolicy` (ownerOnly/public/allowlist), the
+access-action switch, and the image `ImageAdmitted` allow-list (no free image string).
+
+## Verdict
+
+**Bad data does not pass.** Every dangerous sink is fail-closed — including the
+break-glass / internal-face surface, where the hypothesised `mc_uuid`/OP-create → RCON
+paths were traced and found not to exist (parameterized DB, not RCON). Both text → RCON
+vectors — structured (`handlers_access`) and free-text (`handleCommand`) — are
+mutation-pinned by their named negative tests. No gap was found and no production code
+changed; the honest result of "故意加错误数据" is that the input surface rejects it.
+
+Scope is deliberately bounded to the dangerous sinks and the newest (break-glass) code,
+not an exhaustive fuzz of all ~80 handlers — the claim proven is "every place user input
+reaches a dangerous sink validates before the sink," by trace plus two mutations, not
+"every handler was fuzzed."