Uh oh!
There was an error while loading. Please reload this page.
fix(rest): attribute a bearer-authenticated metadata write to its caller (#7749) - #7940
Conversation
…ler (#7749) An admin's ordinary `PUT /api/v1/meta/<type>/<name>` was attributed to nobody: the `sys_metadata_audit` row recorded the sentinel `actor: 'system'` and the `sys_metadata_history` row recorded `recorded_by: NULL`. The real identity appeared only when the caller hand-set a non-standard `X-Actor` header, so the audit trail could not answer "who changed this" for any normal console or API client. The cause was a fallback chain with no producer. Five `/meta` write sites — save, delete, publish, rollback and the compound save — each resolved the actor inline as req.headers['x-actor'] ?? req.headers['X-Actor'] ?? req.user?.id ?? req.userId and nothing on this transport ever sets `req.user` or `req.userId`: REST resolves identity through `resolveExecCtx` (better-auth → `resolveAuthzContext`), which puts it on the returned ExecutionContext and never back onto the raw request. The bearer token was validated — its identity simply never reached the handlers that read for it. Rather than widen the chain with a third limb (which would leave the same "a value everything reads and nothing writes" shape one level down), the two dead limbs are replaced by a single shared producer, `resolveMetaWriteActor`, reading the SAME identity resolution the route's own `manage_metadata` capability gate reads a few lines earlier. The caller a write is ATTRIBUTED to can no longer drift from the caller it was AUTHORIZED against, and all five sites share one rule instead of five copies — which also means the audit rows #7748 will add to publish and rollback inherit the fix rather than the bug. Deliberately unchanged: `X-Actor` still outranks the authenticated identity, exactly as the original expression read. That ordering was masked while the other limbs were always `undefined` and becomes load-bearing now; whether an authenticated caller may keep attributing a write to somebody else is a security-semantics decision for the audit contract, measured and reported on the issue rather than settled as a side effect here. Also unchanged: anonymous and internal system writes resolve no principal, so they still record `'system'` / `NULL` — a machine write is never stamped with a real user. Tests: `meta-write-actor-identity.test.ts` boots a real better-sqlite3 engine, the real `sys_metadata*` objects and a real protocol, then asserts the PERSISTED rows rather than the call arguments — because the two defaults that swallowed the identity differ ('system' vs NULL) and a fix satisfying only one of them would otherwise pass. Reverse-verified: with the producer reverted the admin case reads `{ audit: 'system', history: null }` while the system-write, anonymous and explicit-`X-Actor` cases stay green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WVchQDTf3UjRFWY3JPkdki
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also reference the affected code. These are read-only:
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#7749
Symptom
An admin's ordinary
PUT /api/v1/meta/<type>/<name>was attributed to nobody: thesys_metadata_auditrow recorded the sentinelactor: 'system'and thesys_metadata_historyrow recordedrecorded_by: NULL. The real identity appeared only if the caller hand-set a non-standardX-Actorheader — so the audit trail could not answer "who changed this" for any normal console or API client.Premise re-verified
The filer verified five sites on
00e9196. Re-verified by text on currentorigin/main(2daafe1) — all five still carry the identical chain, at lines 5931 / 6050 / 6195 / 6249 / 6601:ctxin scope already?PUT /meta/:type/:nameDELETE /meta/:type/:namePOST /meta/:type/:name/publishPOST /meta/:type/:name/rollbackPUT /meta/:type/:section/:nameRoot cause — a fallback chain with no producer
Each site resolved the actor inline as
and nothing on this transport ever sets
req.userorreq.userId. REST resolves identity throughresolveExecCtx(better-auth →resolveAuthzContext), which puts it on the returnedExecutionContextand never back onto the raw request. So with no header the expression yieldedundefinedand the protocol's own defaults took over —recordMetadataAudit'sactor ?? 'system'and #4556'sactor ?? null. The bearer token was validated; its identity simply never reached the handlers reading for it.The fix — one producer, not a sixth limb
The two dead limbs are replaced, not widened with a third (which would leave the same "a value everything reads and nothing writes" shape one level down). All five sites now call one shared private producer,
resolveMetaWriteActor, which reads the same identity resolution the route's ownmanage_metadatacapability gate reads a few lines earlier. Two consequences worth naming:save— publish, rollback and the 409 conflict denial never write a row #7748 will add to publish/rollback inherit the fix rather than the bug.resolveMetaWriteActoris the shared location that serves both cards.resolveExecCtxis memoized per request, so sites 1/2/5 pay nothing extra; sites 3/4 (which had no gate) resolve once.Deliberately NOT changed — the precedence question
X-Actorstill outranks the authenticated identity, exactly as the original expression read. That ordering was masked while the other limbs were alwaysundefined; fixing the producer makes it load-bearing for the first time, which means an authenticated caller can attribute a metadata write to somebody else by sending a header.Changing whose name lands in an audit row is a security-semantics decision, not a bug fix, so it is measured and reported on the issue rather than settled here. Measurement: nothing in this repository sets
X-Actoroutside tests — the only non-test references are the five consumer sites themselves, twoCHANGELOGentries, and the QA checklist, which describes the intended contract as "resolved fromX-Actor/ the session identity, never anonymous". The console (objectui) is a separate repository and is not present in this checkout, so its half of the measurement is unverified here. Full detail is in the report on #7749.Tests
New
packages/rest/src/meta-write-actor-identity.test.tsboots a real better-sqlite3:memory:engine, the realsys_metadata*object definitions, a realObjectStackProtocolImplementationand the real route, then asserts the persisted rows rather than the call arguments. That matters: the two defaults that swallowed the identity live downstream of REST and differ ('system'vsNULL), so a mock-protocol test asserting "anactorfield was passed" would prove neither row, and a fix satisfying only one of them would pass.X-Actor→ both rows carry the admin's id (asserted together in one object, so half a fix cannot pass on the first assertion alone);isSystem, no principal) still records'system'/NULL, and an anonymous caller is still refused outright (401) with no row written;X-Actorbehaves exactly as before — the test to change when the maintainer rules on the ordering;resolveExecCtxstub and drives a realauthServiceProvider, pinning the bearer → session →execCtx.userId→ actor chain end to end.One pre-existing assertion in
rest.test.ts(expect(arg).not.toHaveProperty('actor')) encoded the bug and is updated to assert the session identity instead.Reverse-verification (measured)
With the producer fix reverted and the tests unchanged:
X-Actor{ audit: 'system', history: null }{ audit: 'usr_admin_7749', history: 'usr_admin_7749' }'system'/NULLX-Actor'user_42'The reverted admin case reproduces the issue's symptom exactly.
Gates
packages/restsuite: 93 files / 1524 tests, all greenpnpm check:authz-resolver,check:cross-package-test-inputs,check:meta-type-normalized,check:route-envelope,check:nul-bytes— all pass (route-envelope ratchet unmoved: stringError 44 / siblingCode 77)pnpm check:type-check-debt(theTypeScript Type Checkratchet, run with the full closure built): the new test file initially added +5 to@objectstack/restTEST_DEBT (155 → 160). Those five were fixed in the test — an explicit.jsimport specifier and the requiredpackageIdargument toregistry.registerObject— bringing the package back to exactly 155. No ledger entry was raised. Final:OK — 36 ledger entries re-measured, none above its recorded number.eslint --no-inline-configon all changed files: cleanFile surface
packages/rest/src/rest-server.ts,packages/rest/src/rest.test.ts, the new test file, and the changeset. Nothing outside the declared surface;packages/runtime/src/http-dispatcher.tswas not touched.🤖 Generated with Claude Code
https://claude.ai/code/session_01WVchQDTf3UjRFWY3JPkdki
Generated by Claude Code