fix(admin): disclose boundary denial channel in audit tab and docs (#851) - #958
De-pitcher wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
left a comment
There was a problem hiding this comment.
Thanks for taking this on, and for choosing Option A with the reasoning stated in the description. The approach and the placement are right, and the empty-state case is handled well: the note shows even when the table has no rows, which is exactly where an operator would otherwise conclude nothing was denied.
One thing needs a change before I can merge, and it is in the disclosure itself.
The paragraph says boundary denials "such as origin mismatches and unauthorized route access" are emitted to the process log only. Unauthorized route access is not proxy-only. auditRoleDenial in src/lib/api/require-session.ts:107 emits permission_denied / insufficient_role from the application runtime, and those events do reach this buffer and are rendered in this table. Its callers are GET/POST /api/admin/audit, POST /api/db/maintenance and POST /api/admin/fleet-health. The proxy's own branch is narrower: pathname.startsWith("/admin") page navigation only, at src/proxy.ts:143. Measurement 4 in #851 is that case, and it was the one denial that did appear in the tab.
So as written the note tells an admin that a row currently on their screen is a kind of row this tab does not show. Naming the proxy's actual scope fixes it: cross-origin rejections, and non-admin access to the /admin pages. The same sentence in docs/SECURITY.md needs the same narrowing.
Three smaller ones worth folding into the same push:
docs/SECURITY.md: "the authoritative record for all server-generated audit events" is stronger than the code. Both proxy emit sites are metered (src/proxy.ts:47and:153, emitting only whennotice.allowed || notice.tripped), so under a burst the denials past the trip point reach neither channel. The metering itself is out of scope here, but the sentence should not promise completeness.docs/SECURITY.md:origin_mismatchreturns an unconditional 403 atsrc/proxy.ts:78, it is not a redirect. Onlyinsufficient_roleredirects.src/lib/audit.ts: "boundarypermission_deniedororigin_mismatch" reads as two event types.origin_mismatchis areasonon apermission_deniedevent.
Optional, and fine as a follow-up issue rather than in this PR: docs/API_DOCS.md still describes GET /api/admin/audit as "Returns audit events" with no scope note, and the CSV/JSON export carries no equivalent disclosure, which is the artifact that ends up attached to a ticket.
|
Thanks for the thorough review and precise feedback @cevheri! I have addressed all four points in the latest commit (
All local quality gates, component tests, and typechecks pass cleanly. |
|
Checked One small thing left before I merge, and it is in the test rather than the fix.
Two lines in that same test close it: expect(disclosure.textContent).toContain("/admin");
expect(disclosure.textContent).not.toContain("unauthorized route access");The first pins the narrowed scope, the second keeps the old wording from coming back. The |
Summary
Resolves #851 by implementing Option A (Disclosure) as outlined in the issue:
src/components/admin/tabs/AuditTab.tsx(data-testid="audit-boundary-disclosure") clarifying that the tab displays in-memory runtime events and that boundary denials recorded bysrc/proxy.ts(e.g.origin_mismatch, unauthorized routes) are emitted directly to process stdout (libredb.audit.v1).emitAuditEventDocblock Update: Updated the docblock insrc/lib/audit.tsto document that proxy-emitted events land in the proxy's own runtime instance rather than the application runtime that serves/api/admin/audit.docs/SECURITY.md(section 3.2) explaining that process stdout is the authoritative channel for boundary denials and why the in-app ring buffer is scoped to application runtime events.tests/components/admin/AuditTab.test.tsxverifying the presence and content of the boundary channel disclosure.Verification
bun tests/run-tests.ts tests/components/admin/AuditTab.test.tsx(23/23 tests pass)bun run security:check(20 controls documented, 21 security tests accounted for)bun run typecheck(tsc --noEmit— 0 errors)bun run format(biome format .— checked 1180 files, clean)bun run lint:oxc(oxlint— 0 errors)