A bearer-authenticated admin metadata write is stamped `actor: 'system'` — `req.user` / `req.userId` are unset on the `/meta` PUT path

[huangyiirene]

Symptom

A metadata write made by an authenticated admin is attributed to nobody.

• PUT /api/v1/meta/<type>/<name> with an admin bearer token → the audit row records actor: 'system' and the history row records actor: null.
• The real identity appears only when the caller sends X-Actor by hand.

The practical consequence: every audit row written through the normal console/API path names system, so the trail cannot answer "who changed this" for any client that does not know to add a non-standard header.

Root cause

packages/rest/src/rest-server.ts computes the actor as a three-way fallback, at five sites (lines 5846-5847, 5965-5966, 6110-6111, 6164-6165, 6516-6517 on origin/main):

const actorHeader = req.headers?.['x-actor'] ?? req.headers?.['X-Actor']
 ?? req.user?.id ?? req.userId;

The header is the only limb that resolves on this route: req.user and req.userId are unset on the /meta PUT path, so with no X-Actor the expression yields undefined and the downstream default ('system' for audit, null for history) takes over. The bearer token is validated — it just never lands on req.user for these handlers.

Stale-premise check: re-verified on objectstack origin/main (00e9196) — all five sites still carry the identical fallback chain.

Reproduction

1. Boot the showcase with writable runtime packages; obtain an admin bearer token.
2. PUT /api/v1/meta/<type>/<name> with Authorization: Bearer … and no X-Actor.
3. GET /api/v1/meta/<type>/<name>/audit → the row reads actor: 'system'; /history reads actor: null.
4. Repeat with X-Actor: <id> → the row carries that id, proving the write path itself records whatever the fallback resolves.

Scope

Separate from #7748 (publish/rollback write no audit row at all): different file, different failure, and this one survives that fix — once publish and rollback start writing rows, they will write them stamped system for the same reason.

Source

Extracted from the QA run #7695 (framework 92f26f7, console 09987b680).

[hotlong]

Claim: PM loop round 7 — domain:cli seat.

• Session: session_01B3Kurx8qufrDzNjk4rag7V (account hotlong)
• Branch: claude/issue-7749-meta-put-actor-identity
• Worktree: dedicated (cloud container)
• Domain: domain:cli
• File surface: packages/rest/src/rest-server.ts — the /meta PUT/publish/rollback handler region and the five actor-resolution sites (~:5846, :5965, :6110, :6164, :6516 on origin/main, line numbers will have drifted) + tests. Stop on breach; explain in the report.
• Container & model: M, mode:cloud, model: claude-opus-5.
• Serial constraints cleared: the only in-flight cli claim is finding: the runtime dispatcher's capabilities.search is still presence-gated, so it over-promises the moment the slot is filled #7602 (packages/runtime/src/http-dispatcher.ts). packages/rest/src/rest-server.ts is otherwise free — fix(rest): name the conflicting field in the UNIQUE_VIOLATION 409 body (#7821) #7930 (mapDataError) and fix(meta): gate _drafts outlets as authoring surfaces (ADR-0106 D5(4), #6599) #7907 (_drafts) both merged. ⚠️ Two queued cards also land in this file (suggested-binding-loop (b): unknown ?status on /security/suggested-bindings returns 200 empty instead of 400 (live REST route skips validation) #7678 suggested-bindings, and draft-publish-lifecycle: the metadata audit trail records only save — publish, rollback and the 409 conflict denial never write a row #7748's family) — they are NOT dispatched and will be serialized behind this one by region.

裁决:修生产者,⛔ 不要再加一条 fallback

req.user / req.userId are unset on this path and the header is the only limb that ever resolves. The fix is to make the authenticated identity actually reach these handlers, not to widen the fallback chain — adding another ?? limb would leave the same "declared identity that nobody produces" shape the card is reporting, one level down. This is a no-producer defect: the bearer token is validated, its identity just never lands on the request the handler reads.

Fix it at the one place that serves all five sites if the code allows; if the sites genuinely need separate treatment, say why in the report rather than copy-pasting a fix five times — one rule in five copies is what this lane has spent the shift undoing.

⚠️ Report on this, do NOT change it unilaterally

Read the chain as written:

req.headers['x-actor'] ?? req.user?.id ?? req.userId

The header wins over the authenticated identity. Once req.user is populated, that ordering means an authenticated caller can still attribute a metadata write to somebody else by sending X-Actor — the audit trail would name whoever the header says. Today that is masked because the other two limbs are always undefined; fixing the producer makes the precedence live and load-bearing for the first time.

⛔ Do not silently reorder or drop the header limb as part of this card. Some service-to-service or impersonation flow may legitimately depend on it, and changing whose name lands in an audit row is a security-semantics decision, not a bug fix. Measure and report: who sets X-Actor in this repo and in the console, whether any caller depends on it out-ranking the session, and what the audit contract says it means. If it turns out nothing legitimate sets it, say so — that makes it a clean follow-up card rather than a guess. I will take the ordering question to the maintainer with your measurement.

Pin the consequence

• An admin bearer-authenticated PUT /api/v1/meta/<type>/<name> with no X-Actor records that admin's id — in the audit row (not 'system') and the history row (not null). Both, since they default differently and a fix that only satisfies one is half-done.
• Negative: a genuinely unauthenticated / internal system write still records system — ⛔ the fix must not start stamping a real user onto machine writes.
• Regression: an explicit X-Actor still behaves exactly as it does today (whatever the reported ordering turns out to be) — so this PR is separable from the precedence decision.

Premise-first: the filer verified the five sites on 00e9196; re-verify on current origin/main and find them by text, not line number (many PRs merged since). #7748 is a different defect (publish/rollback writes no audit row at all) — ⛔ do not fix it here, but note that once it lands its rows inherit this bug, so a shared fix location is worth naming if you see one.

---

Generated by Claude Code

[hotlong]

{
 "issue": 7749,
 "status": "complete",
 "branch": "claude/issue-7749-meta-put-actor-identity",
 "pr": "https://github.com/objectstack-ai/objectstack/pull/7940",
 "premise_still_valid": true,
 "summary": {
 "premise_recheck": "Re-verified BY TEXT on current origin/main (2daafe1, not the filer's 00e9196). All five sites still carry the identical chain `req.headers['x-actor'] ?? req.headers['X-Actor'] ?? req.user?.id ?? req.userId`, now at lines 5931 / 6050 / 6195 / 6249 / 6601: PUT /meta/:type/:name, DELETE /meta/:type/:name, POST .../publish, POST .../rollback, PUT /meta/:type/:section/:name. Sites 1, 2 and 5 already resolved an ExecutionContext ~20 lines earlier for their `manage_metadata` capability gate and simply ignored it; 3 and 4 had no gate and no ctx.",
 "root_cause": "A fallback chain with no producer. Nothing on this transport ever sets `req.user` / `req.userId` — REST resolves identity through `resolveExecCtx` (better-auth -> resolveAuthzContext), which puts it on the RETURNED ExecutionContext and never back onto the raw request. With no X-Actor the expression yielded undefined and the protocol's own defaults took over: metadata-protocol/src/protocol.ts:9095 `actor ?? 'system'` (audit) and :10474 `actor ?? null` (history). Both rows derive from the SINGLE `actor` field on the saveMetaItem request, which is why one fix serves both.",
 "fix": "ONE shared private producer on RestServer — `resolveMetaWriteActor(environmentId, req)` — replacing the two dead limbs at all five sites (NOT widening the chain with a third limb). It reads the SAME identity resolution the route's own capability gate reads, so the caller a write is ATTRIBUTED to cannot drift from the caller it was AUTHORIZED against. No site needed separate treatment: every one of the five has `environmentId` in scope, so one call signature served all five. resolveExecCtx is memoized per request, so sites 1/2/5 pay nothing extra; sites 3/4 resolve once.",
 "shared_location_for_7748": "`RestServer.resolveMetaWriteActor` IS the shared fix location the PM asked me to name. #7748 (publish/rollback write no audit row at all) was NOT touched here, but sites 3 and 4 are exactly its two routes and they now resolve a real actor — so when #7748 starts writing rows, those rows inherit the fix rather than this bug. No coordination needed beyond not reverting this helper.",
 "behaviour_note": "One deliberate micro-change, called out for review: a PRESENT-but-unusable X-Actor (repeated header -> array, or empty string) previously produced `undefined` -> 'system'; it now falls through to the authenticated identity. A well-formed header is byte-identical to before. Rationale: recording the real caller beats recording 'system' for a malformed request, and it keeps 'no usable header -> the session identity' a single rule. Reversible in one line if the maintainer prefers strict parity.",
 "files_changed": [
 "packages/rest/src/rest-server.ts (helper + five call sites + one stale comment)",
 "packages/rest/src/rest.test.ts (one pre-existing assertion that encoded the bug)",
 "packages/rest/src/meta-write-actor-identity.test.ts (new)",
 ".changeset/meta-write-actor-identity.md (new)"
 ],
 "file_surface_respected": true
 },
 "tests": {
 "new_file": "packages/rest/src/meta-write-actor-identity.test.ts — real better-sqlite3 :memory: engine, real sys_metadata/sys_metadata_history/sys_metadata_audit object definitions, real ObjectStackProtocolImplementation, real route. Asserts the PERSISTED ROWS, not the call arguments: the two defaults that swallowed the identity live downstream of REST and DIFFER ('system' vs NULL), so a mock-protocol test asserting 'an actor field was passed' would prove neither row and a half-fix would pass.",
 "consequence_1_the_fix": "GREEN — admin bearer-authenticated PUT with NO X-Actor records the admin id in BOTH rows. Asserted together in one object so a fix satisfying only one cannot pass on the first assertion alone.",
 "consequence_2_negative": "GREEN — an internal system write (isSystem, no principal) still records audit 'system' / history NULL; an anonymous caller is still refused outright (401 from the /meta umbrella auth gate, before the capability gate) with no row written.",
 "consequence_3_regression": "GREEN — an explicit X-Actor still wins over the session identity and lands in both rows unchanged. Commented in-file as pinned-not-endorsed, and named as THE test to change when the ordering is ruled on.",
 "extra": "A fifth case removes even the resolveExecCtx stub and drives a real authServiceProvider, pinning the bearer -> session -> execCtx.userId -> actor chain end to end with no stub in the identity path.",
 "reverse_verification": {
 "method": "git diff > patch; git checkout -- rest-server.ts; run suite; git apply patch (no git stash — shared stack)",
 "measured_before": "{ audit: 'system', history: null } — reproduces the issue's symptom exactly",
 "measured_after": "{ audit: 'usr_admin_7749', history: 'usr_admin_7749' }",
 "isolation_confirmed": "With the producit reverted ONLY consequence 1 went red; consequences 2 and 3 stayed green throughout. (The fifth case also went red, expected — it calls the new helper by name.)"
 },
 "suite": "packages/rest — 93 files / 1524 tests, all green.",
 "pre_existing_test_updated": "rest.test.ts 'omits parentVersion when no If-Match header is present' carried a rider assertion `expect(arg).not.toHaveProperty('actor')` while stubbing resolveExecCtx to `{ userId: 'test-user' }`. That absence WAS the defect, not the contract; it now asserts `arg.actor === 'test-user'`. This was the only pre-existing failure the fix produced.",
 "gates": {
 "dispatch_gates_matched": "check:authz-resolver, check:cross-package-test-inputs, check:meta-type-normalized, check:route-envelope — all PASS. check:nul-bytes also run (any-edit convention gate) — PASS. check:route-envelope ratchet unmoved (stringError 44 / siblingCode 77); no new error responses added.",
 "type_check_debt": "PASS, and it DID bite as warned. Run with the full closure built (pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*'), the first measurement reported @objectstack/rest TEST_DEBT 155 -> 160 (+5) — all five in the new test file: one TS2835 (relative import needs an explicit .js specifier) and four TS2554 (registry.registerObject requires `packageId`, which every neighbouring test omits). Fixed IN THE TEST, ledger untouched (#6376). Re-measured: exactly 155, and the full run reports 'OK — 36 ledger entries re-measured, none above its recorded number.'",
 "build_closure": "@objectstack/spec and @objectstack/types built first, then the full workspace closure; all 70 turbo build tasks successful.",
 "lint": "eslint --no-inline-config on all three changed source files — clean."
 }
 },
 "open_questions": [
 {
 "id": "x-actor-precedence",
 "question": "Should X-Actor keep out-ranking the authenticated identity on /meta writes, now that the authenticated identity actually exists?",
 "status": "MEASURED AND REPORTED — deliberately NOT changed in this PR, per the dispatch ruling. The header limb is untouched and still first.",
 "why_it_matters_now": "The ordering was masked while the other two limbs were always undefined. This PR makes it live and load-bearing for the first time: an authenticated caller can now attribute a metadata write to somebody else by sending one header, and the audit row will name whoever the header says. Nothing about the header handling changed — only that it now has a real competitor to out-rank.",
 "measurement_who_sets_it": "NOTHING IN THIS REPOSITORY SETS X-Actor OUTSIDE TESTS. A repo-wide case-insensitive search for 'x-actor' returns exactly nine non-CHANGELOG hits: the five CONSUMER sites in rest-server.ts themselves, two in rest.test.ts (a test that sends 'x-actor: user_42', and its section header), and two in docs/qa/platform-checklist/areas/studio-authoring.json (prose describing the intended contract). No client, SDK, CLI, service-to-service caller or impersonation flow in this repo emits the header. The two CHANGELOG references (packages/rest, packages/objectql) are historical PR-10d notes describing this same plumbing.",
 "measurement_console": "NOT VERIFIED — the console is the separate `objectui` repository. This checkout has only the `packages/console` package stub (package.json / README / CHANGELOG, no dist and no sources — the built artifact is pulled in by `pnpm objectui:refresh`), and objectui is not a sibling on disk in this container. A grep of everything present returns nothing. This is the ONE gap in the measurement and it needs someone with the objectui checkout to close it.",
 "measurement_audit_contract": "docs/qa/platform-checklist/areas/studio-authoring.json states the contract as: 'audit rows carry the actor ... each stamped with the acting user, resolved from X-Actor / the session identity, never anonymous'. It names both sources as legitimate but does NOT state which wins when both are present — so the checklist does not settle the precedence either way. Notably, the checklist's own step 327 instructs a tester to 'send X-Actor OR drive it under an authenticated session so the actor is attributable', which reads as the two being interchangeable rather than ranked; before this PR only the first half of that instruction actually worked.",
 "assessment": "On the in-repo evidence this is a clean follow-up card, not a guess: nothing legitimate in this repository depends on the header out-ranking the session, so demoting it (session wins; header honoured only for an isSystem/service principal, or dropped entirely) would break no known caller here. I did NOT act on that — it is a security-semantics decision and the objectui half of the measurement is unverified. Recommended framing for the maintainer: (a) demote X-Actor below the session identity, (b) gate it to isSystem/service principals only, or (c) keep it first and document it as a deliberate impersonation surface. The regression test in this PR pins today's behaviour precisely so whichever way it goes is a one-test, one-line change."
 },
 {
 "id": "malformed-header-fallthrough",
 "question": "Is the micro-change to malformed-header handling acceptable?",
 "status": "Decided myself and documented in-code, flagged here for review rather than left silent.",
 "detail": "A repeated (array) or empty X-Actor previously yielded undefined -> 'system'; it now falls through to the authenticated identity. A well-formed header is byte-identical to before. I judged recording the real caller better than recording 'system' for a malformed request, and it keeps the rule single. Note this file already has a `refuseRepeatedQueryParams` helper for the analogous QUERY-param multiplicity problem (#6877) but no header equivalent; if the maintainer would rather REFUSE a repeated X-Actor than fall through, that is a small follow-up and would pair naturally with the precedence decision above."
 }
 ],
 "out_of_scope_findings": [
 {
 "id": "publish-rollback-ungated",
 "severity": "worth a look",
 "detail": "POST /meta/:type/:name/publish and POST /meta/:type/:name/rollback have NO `manage_metadata` capability gate, while PUT and DELETE next door do (#6603). Both mutate live metadata — rollback restores an arbitrary historical version as the new live overlay. They were the only two of the five actor sites with no ExecutionContext in scope, which is precisely BECAUSE no gate had ever resolved one. I did not add a gate (out of surface, and it is an authorization change, not this bug), but the asymmetry looks unintended rather than deliberate, and it is adjacent to #7748's territory on the same two routes."
 },
 {
 "id": "actor-lookup-integrity",
 "severity": "informational",
 "detail": "sys_metadata_history.recorded_by is a lookup('sys_user') (#4556 chose NULL over the 'system' sentinel precisely because the sentinel resolves to no user row). Real user ids now flow into it on every authenticated write. In the integration test no sys_user rows exist and the engine accepted the value without complaint, so referential integrity is not enforced on this column — worth knowing, not obviously worth changing."
 },
 {
 "id": "audit-actor-column-untyped",
 "severity": "informational",
 "detail": "sys_metadata_audit.actor is a plain text column carrying either a real user id or the literal 'system'. Any consumer joining it to sys_user must special-case the sentinel. Not introduced here — but this PR makes real ids the COMMON case for the first time, so a report that previously saw only 'system' will now see a mix."
 }
 ]
}

---

Generated by Claude Code

[hotlong]

ACCEPTED — PR #7940. Landing on gate-green.

Reviewed by the domain:cli seat (hotlong, session session_01B3Kurx8qufrDzNjk4rag7V) against the diff.

The producer was fixed once, and wired to the right identity

resolveMetaWriteActor replaces the two dead limbs at all five sites — save, delete, publish, rollback, compound save — rather than adding a third limb or being copy-pasted five times. And it resolves through resolveExecCtx, the same identity resolution the route's own manage_metadata capability gate reads a few lines earlier, with the consequence stated better than my dispatch put it:

the caller a write is ATTRIBUTED to can never drift from the caller it was AUTHORIZED against.

That is the property worth having. resolveExecCtx is memoized per request, so the routes that already resolved a context for their gate pay nothing extra.

The test boots the real stack, for a stated reason

The two defaults that swallowed the identity live downstream of REST and differ — sys_metadata_audit.actor falls back to the string 'system', sys_metadata_history.recorded_by to SQL NULL. So a mock asserting "REST passed an actor field" would have proven neither row, and a fix satisfying only one would have passed. Instead: a real better-sqlite3 :memory: engine, the real sys_metadata* object definitions, a real protocol, the real route — and the assertions read the persisted rows.

Both rows are asserted together in one object ({audit, history} → {ADMIN, ADMIN}), explicitly so that a half-fix cannot pass on the strength of the first assertion. That is a small thing that matters: two sequential expects would have let a 'system'-in-history regression hide behind a green audit assertion.

The last case is the one I would have accepted the PR without and been slightly poorer for: it removes the execCtx stub entirely and drives the production bearer → getSession → resolveAuthzContext → execCtx chain against a real auth service, then asserts resolveMetaWriteActor turns exactly that into the recorded actor. The other three cases stub the auth boundary; this one closes the loop and proves the link the card said was missing.

Negatives are pinned as required: an internal isSystem write still records 'system' / NULL (⛔ the fix does not invent a user for machine writes), and an anonymous caller is refused 401 with no row at all.

The modified existing assertion was changed for the right reason

rest.test.ts previously asserted expect(arg).not.toHaveProperty('actor'). That is now expect(arg.actor).toBe('test-user'), with the justification that the absence was the defect, not the contract — it encoded the very bug this card reports. Changing an existing expectation to accommodate your own diff is the move I check hardest, and this one explains why the old expectation was wrong rather than merely noting that it changed.

One edge handled and documented: a present-but-unusable header (repeated → array, or empty string) falls through to the session rather than suppressing attribution — recording the real caller beats recording 'system' for a malformed request.

⚠️ The X-Actor precedence is correctly left alone, and is now mine to carry

X-Actor still outranks the authenticated identity, exactly as before — and the PR says plainly that this becomes load-bearing for the first time now that the other limb produces a value, i.e. an authenticated caller can attribute a metadata write to somebody else. Leaving it unchanged was the instruction and the right call: whose name lands in an audit row is a security-semantics decision for the audit contract, not a side effect of fixing a producer. The test that pins today's ordering is even labelled as the test to change when the maintainer rules.

That question does not close with this PR. I am filing it separately with the dev's measurement attached, so the ordering is decided deliberately rather than inherited by default — and so this fix is not read as an endorsement of it.

Scope

4 files: packages/rest/src/rest-server.ts, a new row-level test, one existing-test update, changeset. No docs/adr/**, no skills paths, no content/docs/releases/.

⛔ Not flipped yet: ESLint and TypeScript Type Check must both read conclusion: success. ⚠️ The new test registers objects with an explicit packageId specifically to avoid adding to packages/rest's frozen type-error pile — the right instinct, and the ratchet will confirm it.

---

Generated by Claude Code