Uh oh!
There was an error while loading. Please reload this page.
fix(objectql): privileged driver-level reads join the ambient transaction - #11435
Conversation
…tion The engine's three privileged read verbs — resolveSecret, resolveSecretField and resolveInternalField — read at DRIVER level on purpose: that is the only layer where a masked or `internal: true`-omitted value still exists, and bypassing hooks, field-level security and sharing is the declared trust each places in its in-process caller. What they also bypassed, not by design, was the connection the surrounding transaction is holding: buildDriverOptions threads the ambient handle (ADR-0034) onto every ordinary read, while these three passed the driver NO options, so their read went to a FRESH pooled connection. Invisible on a roomy pool; a deadlock on a single-connection one. SQLite's knex pool is max=1 (driver-sqlite-wasm and driver-sql/better-sqlite3 both), which encodes SQLite's single-writer model rather than a tuning choice. Where the two met: AuthManager.handleRequest runs SESSION_ERASURE_PATHS inside engine.transaction(...), the vendor's session re-read reaches resolveInternalField through plugin-auth's internal-field readback, and the read waited for a connection that could not be freed until the transaction waiting on the read finished. knex's acquire timeout fired and the route degraded the block into an authentication refusal. Measured on the default datasource before this change: a caller better-auth's own admin gate ADMITS answered 401 after 120,196ms with the target row still present, and a signed-in member got the same 401 after 120,025ms instead of 403 YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS. After: 200 with the row deleted, 403, and an anonymous caller's 401 unchanged — all prompt. Reads only; the privileged write paths are untouched. The #5351 same-origin gate still decides whether the handle is this object's driver's to use, so a privileged read resolving to a different datasource keeps its own connection. The dogfood admin-route sweep's remove-user carve-out (which accepted UNAUTHENTICATED as an additional denial code) is deleted with the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
…BT ratchet `check:type-check-debt --re-measure` counts raw `tsc --noEmit` errors per package with the test exclusion lifted, so a package's own green `typecheck` says nothing about the files it hides. Both new files landed inside that hidden layer and drifted the shrink-only ledger: @objectstack/objectql 354 -> 364 (+10) and @objectstack/verify 8 -> 9 (+1). Every one of the 11 was in the new files, so the drift is attributable, not inherited. Fixed rather than re-baselined — raising a shrink-only entry is maintainer-only and hands back what an earlier PR paid to press it down: - registerObject(schema, packageId, ...) requires 2 arguments; the three calls passed 1. Supplied '__test__', the packageId the sibling engine transaction tests already use. - Array.prototype.at sits above the `lib` this program targets. Replaced the seven `.at(-1)!` sites with a local indexed `last()` helper rather than widening the compiler configuration for a test convenience. - './harness' -> './harness.js'; NodeNext needs the explicit extension, and two sibling verify tests already spell it that way. Re-measured after the repair: objectql 354 = ledger 354, verify 8 = ledger 8, zero errors in either new file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 14 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 56882c7d38aba84aa479cf5ec8ef6ece41688291 && git checkout 56882c7d38aba84aa479cf5ec8ef6ece41688291
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin cccbe51bf7b0cde74e208f086d9593d3983c4238 c0aaeb3c57f36665c85b6c9a8c2f80431c8bb9cf && git checkout -B drift-repro cccbe51bf7b0cde74e208f086d9593d3983c4238 && git merge --no-ff c0aaeb3c57f36665c85b6c9a8c2f80431c8bb9cf
node scripts/docs-audit/affected-docs.mjs --json cccbe51bf7b0cde74e208f086d9593d3983c4238
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#10792
Ruled direction B (
5386675441): a signed-in caller inside a transaction-capable flow must get the proper authorization answer on SQLite too. Directions A (unwrap the erasure transaction) and C (raise the SQLite pool depth) were not taken and are not approached here.The dispatch's Zone-2 assumption 1 was falsified — the fix is re-aimed at what was measured
The dispatch assumed the second connection was acquired by better-auth's own session re-read. It is not, and no vendor fork or vendor hook was needed.
The engine has three privileged, driver-level read verbs —
resolveSecret,resolveSecretField,resolveInternalField. They read at driver level on purpose: that is the only layer where a masked orinternal: true-omitted value still exists, and bypassing hooks, field-level security and sharing is the declared trust each places in its in-process caller. What they also bypassed, not by design, was the connection the surrounding transaction holds:buildDriverOptionsthreads the ambient handle onto every ordinary read (ADR-0034), while these three passed the driver no options at all, so their read went to a fresh pooled connection.Invisible on a roomy pool. On a single-connection pool it is a deadlock: the erasure transaction holds the one connection, and the privileged read waits for a connection that cannot be freed until the transaction waiting on that read finishes. knex's acquire timeout eventually fires and the vendor route degrades the block into an authentication refusal — which is why
remove-useranswered a signed-in caller the same401it answers an anonymous one, while its unwrapped siblingsset-roleandupdate-useranswered the same bearer correctly.New private
privilegedReadDriverOptions(object)returns{ transaction }only when there is an ambient transaction andtransactionCoversDriverFor(object, tx)passes, so the #5351 same-origin gate still decides whether the handle is this object's driver's to use. A privileged read resolving to a different datasource keeps its own connection. Reads only — the privileged write paths are untouched.packages/objectql/src/engine.tsis engine surface)This lane's usual carrier is
plugin-auth, and this PR reaches outside it. Stated per path rather than assumed:packages/objectql/src/engine.ts— the defect is here, and only here. The three verbs that omit driver options are engine methods; the omission is in their bodies.plugin-authis a caller that reachesresolveInternalFieldthrough its internal-field readback — it passes no options because the engine's privileged read signature never accepted the ambient handle from callers in the first place. A repair inplugin-authcould only have been a consumer-side workaround around an engine defect, which is precisely what contract-first forbids.txStore; nothing outside the engine can read it, andtransactionCoversDriverFor(the ambient 事务句柄跨数据源泄漏:凡在事务中执行的被审计写入,合规审计行全部静默丢失(#5226 的真实根因) #5351 gate that must still decide) is likewise engine-private. A fix anywhere else would have had to re-derive both.privilegedReadDriverOptionsreturnsundefinedthere, which is the pre-existing shape byte for byte.The other four files are tests and the changeset. Surface is exactly 5 files.
Acceptance matrix — the ruling names it, so it is binding
Re-measured on the merged head, not inherited from the pre-merge commit.
remove-usermax=1)YOU_ARE_NOT_ALLOWED_TO_DELETE_USERS, target survivesUNAUTHENTICATEDmax >= 10)Both halves are asserted separately on purpose: a fix that returned fast but still answered
401to the admitted admin would have repaired the exhaustion shape and left the capability dead. Status, elapsed, and the row's absence are three distinct assertions.The admitted caller is a fixture: better-auth's admin plugin authorizes on the legacy
user.role === 'admin'scalar that ADR-0068 D2 deliberately stopped synthesizing, so no caller on a stock boot passes the vendor gate. Without the fixture the admitted-admin row cannot be observed at all and the file would only ever measure refusals. A control test fires two unwrapped neighbouring/admin/routes with the same bearer first, so a caller the vendor refuses everywhere cannot masquerade as a pass.What was measured here, and what was not
packages/verify/src/erasure-transaction-authorization.test.ts, booting the default datasource. Whole@objectstack/verifysuite: 9 files, 40 tests, all passing.better-sqlite3→min=1 max=1;pg→min=2 max=10;mysql2→min=2 max=10.driver-sqlite-wasmpins{ min: 1, max: 1 }in its own source.driver-sqlsets nopool.maxitself — it inherits knex's per-dialect defaults, which is why the blast radius is dialect-shaped rather than configuration-shaped.OS_TEST_POSTGRES_URL/OS_TEST_MYSQL_URLis configured, so the PG/MySQL row is carried from the earlier rounds' measurement, not re-confirmed here. CI's live PG + MySQL conformance job is the check that actually re-runs it.Non-regression
The behaviour change on a roomy pool is real and worth naming: a privileged read inside an ambient transaction now runs on that transaction's connection instead of a separate one, so it observes the transaction's own uncommitted writes. That is not a new hazard — it is the same visibility every ordinary read has had under ADR-0034 since that ADR landed. The three privileged verbs were the anomaly; this removes the anomaly rather than creating one.
Swept the consumers that actually exercise these verbs, all green:
@objectstack/objectqlinternal-fields+secret-fields— 2 files, 59 tests@objectstack/plugin-authinternal-field-readback+auth-manager— 5 files, 319 tests@objectstack/plugin-webhookswebhook-secret-at-rest+webhook-drop-durable-record— 2 files, 46 testsThe dogfood carve-out is deleted with the defect
admin-route-nonadmin-refusal.dogfood.test.tscarriedremove-useras the single exception to the member-arm denial vocabulary, acceptingUNAUTHENTICATEDas an additional code. Removing it is part of closing the issue. The vocabulary is now/^YOU_ARE_NOT_ALLOWED/for every route in the bucket with no exception, so no route can drift into the same state silently. Sweep passes: 1 file, 6 tests.Verification
All commands run on
c0aaeb3c57(the merged head,mainmerged in atcbf8b2c8af; the 5 commits that landed onmainafterwards touch none of these paths).node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, no hand-supplied paths. 21 path-matched + 6 convention-triggered families, pluscheck:nul-bytes. 27/27 green, exit codes captured before any pipe.pnpm lint(eslint . --no-inline-config, whole repo) — green, 2m11s. Not narrowed, so nothing to declare.pnpm --filter @objectstack/objectql|@objectstack/verify|@objectstack/dogfood typecheck— all green on realtsc --noEmit.One gate was red and is now fixed — recorded rather than quietly repaired
check:type-check-debt --re-measurecounts rawtsc --noEmiterrors per package with the test exclusion lifted, so a package's own greentypechecksays nothing about the files it hides. Both new test files landed in that hidden layer and drifted the shrink-only ledger:@objectstack/objectql354 → 364 (+10),@objectstack/verify8 → 9 (+1). All 11 errors were in the new files, so the drift was attributable rather than inherited.Fixed, not re-baselined — raising a shrink-only entry is maintainer-only and hands back what an earlier PR paid to press it down.
registerObjectwas given the'__test__'packageId its sibling engine-transaction tests already use; the seven.at(-1)!sites became a local indexedlast()helper rather than widening the compilerlibfor a test convenience;'./harness'became'./harness.js'for NodeNext. Re-measured after the repair: objectql 354 = ledger 354, verify 8 = ledger 8, zero errors in either new file.check-engine-split-ratio.mjsalso came back non-zero on the first pass, but that was a shallow-clone refusal (cannot compute the ADR-0076 D7 trigger metric), i.e. NOT MEASURED rather than a failure. Deepened the clone as the gate itself instructs and re-ran: green, ratio 97.6% over 289 engine-core commits.Review posture
⛔ Draft, and stays draft.
needs:contract-reviewis on the card and on this PR. This seat will not flip ready, will not enqueue, will not arm auto-merge, and will not clear the label — clause ② applies (a request refused today with401now succeeds with200and a deleted row), and it measures belowCONTRACT_REVIEW_TIER.Generated by Claude Code
Generated by Claude Code