libredb / libredb/libredb-studio
Admin Audit tab silently omits every denial recorded by src/proxy.ts
- Dominant language
- TypeScript
- Stars
- 726
- Forks
- 119
- Avg merge
- 7h 47m
- Merged PRs (30d)
- 265
Description
`src/lib/audit.ts:495-500` already warns that an event emitted from `proxy()` "may land in a different instance than the admin API reads, because the proxy is a separately compiled entry and instance sharing is unverified".
It is now verified: it always lands in a different instance. Every denial `src/proxy.ts` records is missing from the Admin Audit tab, and nothing in the UI says so.
## Why this matters
The stdout line is the authoritative channel and it is correct, so no record is lost.
The problem is the Admin Audit tab, which is the only audit surface most operators will ever look at, and which presents itself as the audit log rather than as a partial view of it.
The events it silently drops are exactly the security-relevant ones: an unauthenticated or cross-origin request rejected at the boundary, and a non-admin session reaching for `/admin`.
## Measurement
Studio on `main` at 2703ddc6, dev server, OIDC sessions for an admin (`alice@probe.local`) and two non-admins (`bob@probe.local`, `carol@probe.local`).
Three denials were provoked, two of them reachable only through `proxy()`:
1. `carol` requests `GET /admin` (proxy-only, `src/proxy.ts:138-158`)
2. `bob` requests `GET /admin` (proxy-only, same branch)
3. `bob` sends `POST /api/storage/connections` with `Origin: https://evil.example` (proxy-only, `src/proxy.ts:45-80`)
and one reachable through a route handler:
4. `bob` requests `GET /api/admin/audit` (route-level, `src/app/api/admin/audit/route.ts:11`)
All four appear on stdout with the correct actor:
```
{"schema":"libredb.audit.v1","event":"permission_denied","actor":"carol@probe.local","route":"GET /admin","reason":"insufficient_role"}
{"schema":"libredb.audit.v1","event":"permission_denied","actor":"bob@probe.local","route":"GET /api/admin/audit","reason":"insufficient_role"}
{"schema":"libredb.audit.v1","event":"permission_denied","actor":"anonymous","route":"POST /api/storage/connections","reason":"origin_mismatch"}
{"schema":"libredb.audit.v1","event":"permission_denied","actor":"bob@probe.local","route":"GET /admin","reason":"insufficient_role"}
```
`GET /api/admin/audit`, read by alice immediately afterwards, returned 7 events: three `login_success` for the three users, three more from a second login round, and exactly one `permission_denied`, number 4.
The three proxy-originated events are absent, while the route-level one from the same second is present.
That is the control arm: the only variable separating the event that arrived from the three that did not is which module called `emitAuditEvent`.
The mechanism is the one the comment names. `emitAuditEvent` (`src/lib/audit.ts:510-515`) pushes into `getServerAuditBuffer()`, a module-level singleton (`src/lib/audit.ts:241-249`). Next compiles `proxy.ts` as its own entry, and the framework documentation is explicit that it "can run outside of your application's main runtime and handle requests before they reach your app" (`node_modules/next/dist/docs/01-app/03-api-reference/03-file-conventions/proxy.md:779`). Two entries, two module graphs, two buffers.
## What to change
The ring buffer is documented as a convenience view and that design is fine. What is not fine is a view that is silently incomplete. Pick one of these, in this order of preference:
**Option A, disclose it.** Cheapest and honest. The Admin Audit tab states that it shows events recorded by the application and that boundary denials recorded by the proxy are on the process log only, with a pointer to the `libredb.audit.v1` stdout channel. Also correct the docblock at `src/lib/audit.ts:495-500` from "may land in a different instance" and "unverified" to the measured fact, so the next reader does not re-derive this.
**Option B, make the proxy's events reach the buffer.** The proxy would have to hand its event to the app runtime, for example by posting it to an internal route that calls `emitAuditEvent`. Weigh this carefully before taking it: it adds a request per denial on a path whose whole point is to be cheap, it needs its own authentication so it does not become a way to forge audit entries, and it must not be able to turn a broken sink into a failed denial. Every existing emit site is already wrapped in its own try/catch for that reason.
**Option C, a shared durable sink.** Write audit events through the storage provider when one is configured, so both entries append to the same place and the record survives a restart. This is the largest change and the one that actually fixes the underlying weakness, since today the in-memory buffer is lost on every restart in every deployment. It should not be bolted onto this issue; if this is the direction, this issue becomes Option A and the sink gets its own design issue.
Do not pick a direction without saying why in the PR. If in doubt, take Option A.
## Done when
- The Admin Audit tab no longer presents an incomplete list as complete, by whichever option is chosen.
- The docblock in `src/lib/audit.ts` states the measured behaviour rather than an unverified suspicion.
- A test asserts the chosen behaviour. For Option A that is a component test on the tab's disclosure. For Option B, an integration test that provokes a proxy-level denial and asserts it appears in `GET /api/admin/audit`, with a control case asserting a route-level denial still does.
- 100% line coverage on touched files.
- `docs/SECURITY.md` says which channel is authoritative for boundary denials, if it does not already.
## Not in scope
Changing what the proxy records, when it records it, or the rate limiting around those emits. Those are deliberate and documented at each call site. This issue is only about the gap between what is recorded and what the Admin Audit tab shows.
Contributor guide
Research direction
Read the audit buffer and warning in src/lib/audit.ts:495-515, then inspect proxy denials in src/proxy.ts:45-80 and 138-158, the admin audit route, and the Admin Audit tab component and its tests. Decide and document the chosen disclosure or delivery behavior, update the stated channel behavior, add the corresponding component or integration test, update docs/SECURITY.md if needed, and verify 100% coverage on touched files.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- next.js, typescript
- Domain
- backend, frontend, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100