Audit records now attribute registered-operation writes to the authenticated user#1592
Merged
Conversation
…ecords carry attribution (#1591) Writes performed inside operation handlers via the static Resource API (table.put/delete with no explicit context) produced audit records with user: undefined, because nothing bridged the authenticated req.body.hdb_user into the AsyncLocalStorage context that the transactional wrappers fall back to. This affected component-registered operations (registerOperation), while built-in CRUD constructs its context explicitly. Fix at the dispatch layer (invariant, not per-handler): processLocalTransaction now runs the operation function inside contextStorage.run({ user: hdb_user }). Explicit contexts passed by handlers still take precedence, an existing ambient context already carrying the same user (server.operation() called within a request) is preserved, and requests without hdb_user (e.g. internal bypass_auth dispatch, catchup) get no ambient context — behavior unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses issue #1591 by bridging the authenticated user into the ambient async context during local transaction processing, ensuring static Resource API calls inherit user attribution for audit records. The review feedback highlights a critical issue where replacing the ambient context discards other existing context properties (such as transaction or cache state), potentially breaking transactional safety. It is recommended to merge the existing context instead of replacing it entirely, and to add a corresponding unit test to verify that other context properties are preserved.
Contributor
|
Reviewed; no blockers found. |
…odel review follow-up)
Pins the guard's fourth quadrant: a request hdb_user differing from the
ambient user gets a fresh minimal { user } context (no outer transaction
carried over, which is the mechanism that detaches its writes), and an
explicit hdb_user: null (unauthenticated-allowed path in serverHandlers.js)
establishes no ambient context, same as absent.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eserving transaction and request state (review feedback)
Adopts the review suggestion on the different-user quadrant: instead of
replacing the ambient context with a minimal { user }, spread-merge it so
other ambient properties (open transaction, signal, caches) survive and
only the user is swapped for attribution. Audit attribution is captured
per-write from the write-time context, so this yields both correct
per-user attribution and preserved atomicity; it also fixes the
userless-ambient-with-open-transaction case, which previously detached.
The outer context object is never mutated.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 8, 2026
ldt1996
pushed a commit
that referenced
this pull request
Jul 9, 2026
…1720) * fix: only join an ambient transaction if it is genuinely still OPEN resources/Resource.ts's static-API dispatcher (backing Table.get/put/patch/ delete) decides whether to join an existing transaction on the ambient context or start a fresh one purely via `if (context?.transaction)` truthiness. This is safe only when ambient contexts are fresh, one-shot objects per call — which was always true before #1591/#1592, since `context = contextStorage.getStore() ?? {}` fell back to a brand-new `{}` whenever nothing had populated contextStorage for the current call chain. #1591/#1592 (establish ambient user context for operation handlers so audit records carry attribution) changed that: processLocalTransaction now wraps an entire operation handler invocation in one `contextStorage.run({ user: hdbUser }, handler)` call, installing a single, long-lived, mutable context object as the ambient store for the whole handler. resources/transaction.ts's transaction() helper mutates that shared object in place (`context.transaction = new DatabaseTransaction()`) as a side effect of servicing the first static Resource API write made with no explicit context. Because the object is shared, that `.transaction` reference is then visible to any later, logically-independent static Resource API call in the same handler — and Resource.ts wrongly joins it instead of starting its own, even though the first call's transaction may already be closing/lingering. The two writes get folded into one transaction whose commit lifecycle was only ever driven by the first (or second) call, and the other write can be silently dropped. Confirmed via a live 2-node cluster-formation regression: harper-pro's `add_node`/`add_node_back` operation handlers (replication/ subscriptionManager.ts's ensureNode(), and security/certificate.ts's signCertificate()) each make 2+ sequential static Resource API calls with no explicit context under one ambient hdb_user-carrying operation. Bisected against harper-pro's integrationTests/cluster/replicationTopology.test.mjs (swapping only the core submodule pointer, harder-pro code held constant) to core commit af64622 as first-bad: peer nodes' CA certs silently failed to persist into hdb_nodes, producing SELF_SIGNED_CERT_IN_CHAIN on subsequent mesh TLS connections and "Timed out waiting for cluster to connect". Instrumented tracing showed two sequential ensureNode() calls (self-record, then peer-record) sharing the identical DatabaseTransaction instance. Fix: only treat an ambient `.transaction` as reusable when it is still TRANSACTION_STATE.OPEN — the exact same self-consistency check resources/transaction.ts's own transaction() helper already applies before deciding to reuse vs. create a transaction. This restores the pre-#1591 isolation for independent calls while leaving genuine nested/explicit transaction chaining (a caller holding the same still-open Context/txn across multiple calls) unaffected, since `.open` only transitions away from OPEN once the transaction's owning transaction() call has fully completed. Verified against harper-pro's replicationTopology.test.mjs "connect nodes" test (4-node star topology): fails consistently (timeout, 0/3 runs) with core at af64622 and this fix absent; passes consistently and at pre-regression speed (3/3 runs, ~3.2s each, matching a pre-af646222d core baseline) with this fix applied — even without any harper-pro-side change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * style: prettier formatting * test: assert transaction-instance identity, not just persistence, in leak regression cb1kenobi (PR #1720 review) found that the existing regression test doesn't actually discriminate buggy vs. fixed code: reverting Resource.ts's fix back to bare `if (context?.transaction)` truthiness and re-running still passes 3/3, because DatabaseTransaction's addWrite()/save() takes an immediate-commit path when a write joins an already-closed transaction, so persistence succeeds regardless in this isolated unit setup. Add a test that asserts the mechanism directly: each independent, no-explicit-context write inside one ambient operation handler must be serviced by its own DatabaseTransaction instance (captured via contextStorage.getStore()?.transaction after each write). Verified this invariant fails on the pre-fix bare-truthiness check and holds with the `.open === TRANSACTION_STATE.OPEN` fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: propagate a timeout-aborted ambient transaction instead of silently restarting #1720 fixed the ambient-transaction-leak bug by requiring context.transaction.open === OPEN before joining it, otherwise starting a fresh transaction. That treats a committed and an aborted ambient transaction the same way, but they need opposite handling: a committed transaction is safe to silently replace, while a transaction aborted by the long-transaction monitor (#1411, DatabaseTransaction.abortDueToTimeout(), marked via the `timedOut` poison flag) must not be silently superseded — the independent write that follows it needs to fail too, so the whole logical operation rolls back atomically instead of quietly landing on a brand-new transaction while the earlier write was discarded. Widen the join condition to also join a `timedOut` transaction: addWrite()/commit() already throw transactionOpenTooLongError() once a transaction is poisoned, so joining it here correctly propagates the abort to the caller. Restores integrationTests/resources/txn-overtime-atomicity.test.ts (broken by #1720) without regressing unitTests/resources/operationContextTransactionLeak.test.js (#1720's own regression test). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Operation handlers that write via the static Resource API without an explicit context (
table.put(id, record)etc.) produced audit records withuser: undefined, because nothing bridged the authenticatedreq.body.hdb_userinto the ambient AsyncLocalStorage context the transactional wrappers fall back to. This affectedregisterOperation-based (component-registered) operations; built-in CRUD was unaffected (ResourceBridge builds explicit contexts).Fix at the dispatch layer (invariant, not per-handler):
processLocalTransactionnow runs the operation function inside a user-bearing ambient context. Closes #1591 (full root-cause chain there).Semantics (where to look)
The small change in
serverUtilities.tshas four deliberate behaviors, each pinned by a regression test:contextStoragewhen no context argument is passed.server.operation()called from inside a component request handler keeps the exact request context object (open transaction, response), avoiding any semantics change on that embedded path (currentStore?.user !== hdbUser, object identity — the embedded path passes the samehdb_userreference).{ ...currentStore, user: hdbUser }): the ambient transaction, signal, and caches are preserved (atomicity intact), while attribution — which is captured per-write — follows the new user. The outer context object is never mutated. (This was replace-not-merge in the first revision; gemini-code-assist's review correctly flagged that replacing discards an ambient transaction, breaking atomicity for nested dispatch — adopted in050e70c9c.)hdb_user→ no wrap — both absent and the explicithdb_user: nullset by the unauthenticated-allowed path; internalbypass_authdispatch and legacy-clusteringcatchupare unchanged (catchup writes attribute explicitly through the dataLayer anyway).All
contextStorage.getStore()consumers in core were audited for dependence on an empty ambient context during op execution; none found (details in #1591). Behavioral note: overlapping no-context writes within one handler (e.g.Promise.all) now share one ambient context and can join one transaction — same machinery and semantics as the REST per-request path; sequential writes still get separate transactions.Known residual (pre-existing, out of scope):
jobProcess.tsinvokes built-in job ops bare; registered ops cannot dispatch via the job process today, but if that's ever added it needs the same wrap.Tests
unitTests/server/serverHelpers/serverUtilities.test.js— the ambient-context seam, all quadrants: wraps withhdb_user; no wrap when absent; no wrap when explicitlynull; same-user ambient preserved (exact object); different-user merge (transaction identity + request state preserved, user swapped, outer object unmutated); userless-ambient merge (transaction preserved, user added).unitTests/resources/operationContextUser.test.js(new) — end-to-end: static put inside an op is attributed in the audit record; explicit context wins; unattributed withouthdb_user.auditLog,transaction, MCPoperations); full unit suites left to CI.No docs changes needed: bug fix to existing audit behavior, no new API/config surface.
Review
Generated by an LLM (Claude — Fable 5), reviewed by Kris. Cross-model review complete (thorough: Codex leg + Gemini leg + Harper-domain adjudication): no blockers in the adjudicated pass; gemini-code-assist's PR review then correctly identified that the first revision's different-user path replaced rather than merged the ambient context — adopted (see Semantics above; threads resolved with rationale). Remaining suggestions were applied (
hdb_user: nullcontract test, merge-preservation tests) or rejected with rationale (optional chaining onreq.body— unreachable; ALS-overhead concern — ops dispatch is not the nanosecond-hot path andtransaction()already runscontextStorage.runper write).🤖 Generated with Claude Code