Skip to content

fix(admin): disclose boundary denial channel in audit tab and docs (#851) - #958

Closed
De-pitcher wants to merge 2 commits into
libredb:mainfrom
De-pitcher:fix/admin-audit-proxy-denial-disclosure
Closed

De-pitcher wants to merge 2 commits into
libredb:mainfrom
De-pitcher:fix/admin-audit-proxy-denial-disclosure

Conversation

@De-pitcher

Copy link
Copy Markdown

Summary

Resolves #851 by implementing Option A (Disclosure) as outlined in the issue:

  1. Admin Audit Tab Disclosure: Added an explicit disclosure message to 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 by src/proxy.ts (e.g. origin_mismatch, unauthorized routes) are emitted directly to process stdout (libredb.audit.v1).
  2. emitAuditEvent Docblock Update: Updated the docblock in src/lib/audit.ts to document that proxy-emitted events land in the proxy's own runtime instance rather than the application runtime that serves /api/admin/audit.
  3. Security Architecture Documentation: Updated 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.
  4. Unit Test Coverage: Added a unit test in tests/components/admin/AuditTab.test.tsx verifying 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)

@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:47 and :153, emitting only when notice.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_mismatch returns an unconditional 403 at src/proxy.ts:78, it is not a redirect. Only insufficient_role redirects.
  • src/lib/audit.ts: "boundary permission_denied or origin_mismatch" reads as two event types. origin_mismatch is a reason on a permission_denied event.

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.

@De-pitcher

Copy link
Copy Markdown
Author

Thanks for the thorough review and precise feedback @cevheri!

I have addressed all four points in the latest commit (07c190b1):

  1. Narrowed boundary denial disclosure in UI & docs: Updated both src/components/admin/tabs/AuditTab.tsx and docs/SECURITY.md to explicitly state the proxy boundary scope ("cross-origin rejections and non-admin access to /admin pages") so it does not conflate with application runtime role denials.
  2. Log streaming scope: Softened docs/SECURITY.md to describe stdout (libredb.audit.v1) as the primary stream for server-generated audit events without claiming absolute unmetered completeness.
  3. Clarified HTTP 403 vs Redirect: Clarified that origin_mismatch returns an unconditional 403 while insufficient_role redirects non-admin sessions from /admin pages.
  4. Audit docstring: Clarified in src/lib/audit.ts that origin_mismatch is a reason on boundary permission_denied events.

All local quality gates, component tests, and typechecks pass cleanly.

@cevheri

cevheri commented Sep 18, 2026

Copy link
Copy Markdown
Member

Checked 07c190b1 against the four points, and all four land correctly. The disclosure now names the proxy's real scope, docs/SECURITY.md no longer promises completeness, the 403 and the redirect are separated, and origin_mismatch reads as a reason rather than an event type. I re-read src/proxy.ts:78 and :143 alongside the new wording and they agree. Thanks for the careful turnaround.

One small thing left before I merge, and it is in the test rather than the fix.

tests/components/admin/AuditTab.test.tsx asserts Boundary denials, libredb.audit.v1 and stdout. All three were already present in the wording my review asked you to change, so the test passes equally on the old text and the new one. The correction it exists to protect is not pinned, and a future edit could widen the scope again without turning it red.

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 docs/API_DOCS.md and export-disclosure items stay out of this PR. I will open a follow-up issue for them.

@cevheri cevheri closed this Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Admin Audit tab silently omits every denial recorded by src/proxy.ts

2 participants