Audit remediation: 12 findings, plus the pb-nebula bump that makes Nebula revocation real

[skeeeon]

A full-codebase review produced a 12-item ordered fix list. All twelve are here, one commit each, plus a dependency bump that turns item 4 from correct-but-inert into a working control.

52 files, +4405 / −391. No feature work — the freeze holds.

Verification

Gate	Result
go build (server + leaf-sync), go vet	ok
go test -count=1 ./...	ok
./scripts/test-authz.sh	176 passed, 0 failed (was 155)
./scripts/check-sort-fields.sh	58 sort terms resolve
cd ui && npm test	140 passed (was 0 — no runner existed)
vue-tsc --noEmit, vite build	clean
go mod tidy -diff, gofmt on every blob git stores	clean

The findings, most severe first

1. A blank organization matched a blank organization context — cross-tenant reads

f903acf. Every inventory read rule was organization = @request.auth.current_organization. Both sides are TEXT whose zero value is the empty string, and in PocketBase an empty string equals an empty string. No role was bypassed and no rule was mis-written — two sentinels compared equal.

Both halves were ordinary product states. organizations.deleteRule allowed owner = @request.auth.id, and 16 of the 18 relations into organizations are non-cascade and non-required, which PocketBase blanks rather than deletes (SaveNoValidate) — so an owner deleting their own org orphaned its entire inventory at a blank organization. Meanwhile hooks/membership_lifecycle.go blanks current_organization on the way out of an org. The leaf_nodes branches had the same hole on @request.auth.organization.

18 rules across 8 collections now require a non-blank context and a correlated membership. The membership clause is the load-bearing half: memberships.organization is required and cascade, so it can never be blank — that kills the sentinel by construction rather than by a comparison someone later tidies away. organizations.deleteRule is now operator-only, and users.createRule refuses current_organization outright, because that branch is anonymous and cannot check membership even in principle.

Bounded precisely: credentials were not exposed (memberships.organization is required+cascade), and org ids were not enumerable (organizations.listRule requires membership). Every write rule already carried the membership clause, which is exactly why only reads were exposed.

The review question is not "does this rule name the right roles" but "what does this rule do when both sides are the zero value."

2. leaf-sync zombied ~2 minutes after the local bus restarted

b016e2f. nats.Connect passed no reconnect options, so the nats.go default MaxReconnects: 60 × ReconnectWait: 2s closed the connection permanently while Run looped forever against a dead handle. Now MaxReconnects(-1). The initial dial still fails fast, deliberately — a misconfigured agent should exit, not retry silently.

3. The twin relay dropped failed hub writes, and a short fetch purged live rows

ba90e15. Two independent bugs.

The relay claimed a watcher replay would re-offer a failed write. It cannot: the watcher is on the local bucket, which does not die when the WAN does. Now a pending set is retried on a timer, re-reading the current value from the source (so a stale value is never resurrected) and relaying a delete for locally-removed keys. Retry-and-continue, not return-error — a poison key must not block every other key.

Separately, syncCollection purged any local row absent from the fetch, so a partial page meant deleting live records. Now expected is captured from the first page's TotalItems and a skipPurge flag guards only the delete loop — not the upserts, and not the failed > 0 error return. I caught both of those myself after a first attempt returned early and skipped the upserts too.

4. Nebula certificates were never revoked — plus 989762c

a626930 + 989762c. Setting a device inactive closed the console door and the NATS door; the overlay network stayed open until the certificate expired. pb-nebula v0.1.0 had no revocation concept at all, so this needed skeeeon/pb-nebula#2 first, now released as v0.2.0.

Nebula has no CRL and no OCSP: revocation is pki.blocklist, a fingerprint list in every peer's config, so it is a fan-out and it takes effect on config redeploy (SIGHUP is enough). Two consequences are now documented rather than discovered: deactivate, do not delete — a fingerprint absent from the database cannot be blocklisted, so deleting a host leaves its certificate trusted until expiry — and the config is not the delivery, which matches the NATS boundary of minting a credential and not policing what connects with it. The delivery path is deliberately out of scope: client responsibility, same as NATS.

hooks/active_flag.go now runs the NATS and Nebula cascades independently — a device may hold either identity, both, or neither, so neither may short-circuit the other. It mirrors active, not revoke: in pb-nats revoke is the "credentials leaked" button that hands back a working replacement, and it is checked first and returns early, so setting both in one save silently re-issues a deactivated device's credential.

The bump adds one behaviour that is a dependency's, not ours: a nebula_hosts record created without an active field now lands active, because a host born inactive is one every peer blocklists at birth. Nothing here relied on the old default — hooks/thing_routes.go:473 and the console form both always sent it — but the suite now pins that contract, since a silent upstream revert would provision devices that can never join the mesh.

5. The test harness bound 5 of 13 hooks, and demo-seed produced no decommissioned devices

78499ce. internal/testutil did not bind RegisterActiveFlag or RegisterMembershipLifecycle, so tests passed against behaviour the binary does not have. It now binds both, and documents why the 6 route registrars plus RegisterObservability are deliberately excluded (they bind only OnServe).

demoseed set the inactive flag at create time, which does nothing — a just-created record has an empty Original(), so the update hook sees no flip. It now creates active and re-reads before flipping, so the cascade actually fires. Its revokeNatsUser helper was deleted: it set the wrong flag, for the reason in item 4.

6. Encryption boundary — documentation, not code

f235d0b. The proposal was to encrypt creds_file and config_yaml. Dropped after discussion, and that was the right call. A .creds file contains the user seed, Nebula needs the host key inline, and ui/src/stores/nats.ts reads creds_file to open the browser's own NATS connection — a browser can never hold a decryption key. It would have required a decrypting route inside pb-nats and pb-nebula, hard-coupling those libraries to this platform, to protect material that rotates cheaply on the NATS side anyway.

SECURITY.md now states the boundary precisely: a stolen database with the key held elsewhere yields no ability to mint new identities and every existing credential.

7. CI ran none of the checks that already existed

53a1104. Added the frontend npm test step, go test -count=1, ./scripts/check-sort-fields.sh, go mod tidy -diff, a pb_public placeholder guard before the build, a maplibre-gl 5 pin guard, and a gofmt -l . gate.

The gofmt gate was previously written off as impossible — gofmt rewrites a doubled single-quote in a Go doc comment into a typographic quote. It turned out to be fixable by rewording two comments rather than accepting the corruption, so the gate is now real. -race is deliberately absent: it needs cgo and I had no gcc to verify it with, and an unverified gate that first fails on someone else's PR is worse than a missing one. Reasoning is in ci.yml for whoever has a Linux box.

8. Every rule rejection was counted as a server error

4131abe. statusClass read re.Status, which is zero when a handler returns an error before writing. So every 400 and 404 — including every authz denial — landed in the 5xx bucket, making the error-rate metric useless as an alert. It now resolves router.ToApiError(err).Status, with a test asserting the bucket matches what the router would actually send.

9. Widget defaults had two sources, and the newer one lost

8b00869. I proposed this as deleting a duplicate. It was not a duplicate — it was stale, and it ran second, so it reverted newer defaults: kvtable lost its twin bucket, and button/switch/slider lost their command and desired-state subjects.

I said so before touching it, pulled item 11's runner forward to pin createDefaultWidget first, then merged only the additive parts (per-type titles, $.value jsonPaths). User-visible defaults are unchanged. WIDGET_TYPES is now a runtime as const array with WidgetType derived from it, and configComponents is typed Record<WidgetType, Component> so a new widget type fails to compile until its config form exists.

10. Documentation asserted things the code does not do

0f6852a + 12f1bf8 + platform-docs. The headline feature list still sold message schemas — the worst place for a stale claim, since a buyer demos what they were sold. Also: a pocketbase widget with no component behind it, the role count, the agent's binary name, and "bootstrap is three commands" where it is four.

The agent reports were partly stale, citing line numbers that had moved and claims already fixed. I verified every doc claim against the code before editing.

11. No frontend test runner existed

3359610. Vitest, node environment by default, jsdom opt-in per file. 140 tests over the five files vue-tsc && vite build cannot protect: twinDrift, useSubscriptionManager, the can capability map, dashboard import/export, createDefaultWidget.

12. The gate in front of every destructive action was not a dialog

7a44700. ConfirmDialog was a plain div: no role, no aria-modal, no labelling, no Escape, no focus trap, and autofocus on the destructive button — so Enter on an unread dialog deleted the thing. Now a proper dialog; focus lands on the container, not a button, so nothing is pre-selected.

The habit that mattered most

Every high-severity fix was verified by reverting it and confirming the new test fails. That caught four tests of mine that passed against deliberately broken code:

1. A leaf-node authz check that restored its fixture before the assertion ran → fixed with a dedicated permanent ORPHAN record.
2. A relay-retry test that cleared the injected fault on a poll, so the retry path never ran → fixed with a writeFails counter.
3. A Nebula cascade check whose fixture host was already inactive (the payload omits the flag; PocketBase bools have no schema default) → fixed by forcing and asserting it immediately before deactivating.
4. A focus-trap test that caught a bug in my own implementation — filtering on offsetParent !== null is null for every element under jsdom and inside position: fixed subtrees, so the trap was a no-op.

The new born-active check in 989762c was verified the same way: it fails against pb-nebula v0.1.0 and passes against v0.2.0.

Also worth knowing for the next authz change: PocketBase tokenizes quote characters in a rule string before comments are stripped, so one apostrophe in a comment re-pairs every quote after it and the collection fails to import with invalid quoted text. My first hypothesis about the mechanism was wrong; I isolated it empirically by restoring the literal and rebuilding. Written up in CLAUDE.md.

Known gaps

• -race is not in CI (needs cgo; no gcc available to verify with).
• STONE_AGE_BOOTSTRAP_PASSWORD has no file-based indirection and outlives its single use in the container's environment. Flagged, out of the 12-item scope.
• The HEALTHCHECK change in 53a1104 is unexercised at runtime. I listed this as "the image was never built", which was wrong: CI's pre-existing Container image builds step passes, so the Dockerfile is valid and the image builds. But building an image never runs its healthcheck, and I had no container runtime locally to start one — so the port-expansion fix itself is still unproven.
• The console was never opened in a browser. ConfirmDialog is covered by 14 jsdom tests instead; that is a better regression guard but it is not the same as clicking it.
• Frontend refactors deliberately excluded under the freeze: useListView (~2,700 duplicated lines), a widget registry (28 edit sites per new type), FormField (~640 lines), splitting ScannerWidget. Vitest now existing is what makes those safe to attempt later.
• Unrelated: GitHub reports 3 Dependabot vulnerabilities on main (2 critical, 1 low), untouched by this branch.

Merge order

skeeeon/pb-nebula#2 and #3 are merged and tagged v0.2.0, so this branch is unblocked. platform-docs fix/docs-truth-pass is pushed but has no PR yet — it pairs with item 10 and can land independently.

🤖 Generated with Claude Code