fix(objectql): stamp created_by from the session on an ordinary create, so a caller cannot forge the audit attribution through a plain POST - #16809
Conversation
The audit binder's beforeInsert read `record.created_by = record.created_by ?? session.userId` — client-preferred on every insert, with no flag and no privilege — while its sibling one line down was already the `preserveAudit` ternary. Since the static-readonly strip moved inside `engine.insert` (#15395) it runs after the before-phase hooks, and #14259's guard treats a key a hook assigned as the hook's write rather than a caller forgery, so the `??` laundered the caller's bytes past it. `created_by` now takes the same shape as `updated_by`, the shape PR #16313 landed for `created_at` one field over. Both audit-user assignments stay inside `if (session?.userId)`: with no session the hook must assign nothing so the strip takes the forgery, and that branch is pinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 16 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 385e4564eb1ef9d9cbe042e98136ec6e80a5477b && git checkout 385e4564eb1ef9d9cbe042e98136ec6e80a5477b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 73709893f1b269870e8c08a405a4604106521e24 22ef04956309528974be5fdcef9292c379cad904 && git checkout -B drift-repro 73709893f1b269870e8c08a405a4604106521e24 && git merge --no-ff 22ef04956309528974be5fdcef9292c379cad904
node scripts/docs-audit/affected-docs.mjs --json 73709893f1b269870e8c08a405a4604106521e24 |
落地前检三条全部满足 —— 已转 ready 并挂 auto-merge
① 不适用 —— ② ✅ exit 0: ⭐ 阴性对照发火:同一份文档、只删掉本席 07:09:04Z 那条重述 claim ⇒ exit 4。⇒ exit 0 是那条评论换来的,⛔ 不是文档凑出来的。 文档来源如实交代:dev 自己从 MCP 取的 ③ ✅ 10 个 workflow run 全部 翻 ready 前的正文亲扫:首行
⭐ 顺带记一笔本轮的更正:本席一小时前在 PR #16808 上写过「② 在这条通道上满足不了」。那是一个理由,不是一次尝试。 dev 把它从 MCP 取来的 Generated by Claude Code |
Fixes #16311
The audit binder's
beforeInsertlaundered a caller-suppliedcreated_byon an ordinary create, one field over from thecreated_athole whose landed remedy is PR #16313 (merged 2026-09-06, card #15964). This lands the same shape, in the same function, deliberately: two fields treated two ways inside oneifblock is how this second card came to exist at all.The stop condition was checked first, and it is NOT met
The triage ruling made this conditional: stop and report if decision batch #54 on #15964 was explicitly scoped to
created_atonly, with a stated reason not to extrapolate. Both conjuncts were checked against the ruling comment itself, not against a paraphrase.The ruling names
created_atbecause that is the card it was ruling. It states no reason against extrapolation, and the same triage seat had, in the very comment that sent #15964 to the decision box, asked the taker to bring this field into the same drive and recorded it as untested. The triage ruling on this card reads the substance as field-independent and routed itpm:queue, not to the decision box. PR #16313's own words for why it did not ride along are procedural — a different field's write contract deserves its own card, and this field carries a second question of its own — and that second question is answered below rather than left open.The change
packages/objectql/src/plugin.ts, the audit binder'sbeforeInsertstamp:which is now byte-symmetric with the line under it,
record.updated_by = preserveAudit ? (record.updated_by ?? session.userId) : session.userId;.Why the
preserveAuditternary and not a bare= session.userId. Same reason as the twin:treatAsHistoricalreaches this field through this very hook.runImport({ treatAsHistorical: true })setspreserveAudit: trueon the write context (packages/rest/src/import-runner.ts) and has no separate path to the audit stamp, so a bare assignment would close the hole and break the ruled channel in one line.Pre-fix and post-fix, side by side — same pin file, same head, one line apart
Both tables are printed by the same new pin (
packages/objectql/src/plugin-audit-created-by-create-side.test.ts). The rig is the real ingress: anObjectKernelwithObjectQLPlugin, so the shippedsys_stamp_audit_inserthook is bound throughbindHooksToEngineandengine.insertruns the static-readonly strip after it, over a capturing driver — so what reachesdriver.createis the stored row.BEFORE. Taken with
plugin.tsproved byte-identical toorigin/mainbefore the run:git hash-objectgave8d0178ae0, equal togit rev-parse BASE:packages/objectql/src/plugin.ts, with the old spelling counted at 1 on disk and the new spelling at 0. No restore leg is owed for this one and that is a fact, not an omission — nothing was mutated. The file was pristine at the merge base and the reading was taken before the first edit of the round.That is the card's rig reproduced verbatim, row for row, and the pin fails on exactly one assertion.
AFTER, at the final head
22ef049563:updated_byin the same payload is the in-experiment control on both legs: it proves the create-side strip IS running on those rows and DOES take the sibling audit field, socreated_bysurviving was "the strip ran and spared exactly this one", never "the strip did not run".idandrun_atstrip on both legs too, and a change that closedcreated_bywhile opening either of them would be a regression on a security card.row1 is an acceptance criterion, and it is why the guard was not touched
The card's carve-out is the load-bearing half of this change, and the triage ruling weighted it:
The answer this PR gives: the session-less insert is left exactly as it is, and both audit-user assignments stay inside
if (session?.userId). With no session the hook assigns nothing, the engine strip takes the caller's value, and the key is absent. A shape that assignedsession.userIdunconditionally would writeundefinedinto the key, making it one the hook "wrote"; #14259's guard would then spare it, and a branch that is correct today would become a new hole. row1 pinsundefinedon bothcreated_byandupdated_byso that regression cannot land silently, and a fourth case pins thatpreserveAuditwithout a session does not resurrect the forgery either — the guard still wins over the flag.The creators that supplied a
created_by— the enumeration, and why this side is structurally safer than the twinThe predicate is exact: this change alters behaviour only where
session.userIdis truthy AND the caller supplied acreated_bydiffering from it ANDpreserveAuditis unset. Every non-generated source underpackages/,apps/andexamples/carryingcreated_byas an object-literal key was classified against it. Two production creators exist; both are cleared by source, and both for the same structural reason.plugins/plugin-sharing/src/share-link-service.ts:558context.userId ?? null, inserted underSYSTEM_CTXSYSTEM_CTXis{ isSystem: true, positions: [], permissions: [] }with nouserId, so the hook's guard was false before this change too and never touched the key; theisSystemstrip exemption is what carries the valuerest/src/rest-server.ts:9042(sys_import_job)createdBy, derived at:9026fromcontext.userId ?? context.user?.id, inserted with{ ...context, isSystem: true }context.userIdexists,createdByis that same value, so?? session.userIdand= session.userIdagree byte for byte; where onlycontext.user?.idexists,session.userIdis falsy and the hook never enters the branch at allThe dogfood seeds that back-date attribution (
showcase-invoice-cbp,analytics-label-scope) driveql.insert(..., { context: { isSystem: true } })— again nouserId, so the hook never entered that branch before or after. The two public-form fixtures that sendcreated_by: 'usr_victim'assert the forgery must not land; this change moves in their direction, never against it.Worth stating rather than smoothing over: this is the asymmetry the card named, and it cuts in favour of the change. #16313 broke a legitimate consumer because the
created_atstamp is ungated, so every back-dating seed rode the??.created_bywrites are gated onsession.userId, so every path in this tree that must attribute a row to somebody other than the caller already runs session-less through a system context — a channel this hook never reaches. That is why the twin needed a fixture adaptation and this one needs none. The measured consequence is that no consumer in the repository changes behaviour, and the tests below are what checks that rather than the reasoning.Clause-②: no
Clause-② re-derived from this diff, not inherited from the precedent
PR #16313 declared
nofor the twin and re-derived it rather than carrying the ruling's expectation over; the same method was re-run here against this diff. Build at head, swap onlypackages/objectql/src/plugin.tsback to the merge base, rebuild, and compare every declaration file the package publishes by blob and by exported-name count.index.d.ts0585fe30f0585fe30findex.d.mtse33f349ede33f349edcore.d.tsac7e40431ac7e40431core.d.mts611ed987c611ed987cutil-Dw5ZTIII.d.ts/.d.mts6c82fcacb6c82fcacbThe rebuild is proved to have re-run rather than been skipped:
dist/index.jsmtime advanced1788848035 -> 1788848052 -> 1788848068across the three legs.The instrument is proved live, because an identical manifest is otherwise indistinguishable from a blind spot. A public member injected on the exported
ObjectQLPluginclass moveddist/index.d.tsfrom0585fe30ftoe929ab428andindex.d.mtsfrome33f349edtoec478a84e, and the probe appears in the published declaration. It was aimed inside the published set from the start, on #16313's finding that a new top-levelexport constinplugin.tsreaches no published entry point becausepackages/objectql/src/index.tsre-exportsObjectQLPluginby name.The measurement was taken before the
origin/mainmerge, and it still describes this PR exactly: the merge touched no file underpackages/objectql(git diff origin/main...HEADlists two objectql paths, both mine). The mutation and its restore were proved on disk in both directions — the swapped file matched the BASE blob with the old spelling at 1 and the new at 0, and the restore returned it to the HEAD blob withgit status --porcelainclean. The control build left indist/afterwards was cleared and that was verified, not assumed:ablation-dist-preflight --absentreports the probe absent from all 14 built files, and the same tool reports the fix present in 2 of them.So: no exported symbol moves, and the accept set only narrows — to the
readonlycontractAUDIT_FIELD_DEFS.created_byalready documents. A caller may still sendcreated_by; it is now overwritten with the session user on an ordinary create rather than honoured. Under the rule that uncertainty resolves toyes, there is none to resolve: the mechanical floor does not fire, the published declarations are byte-identical under a live instrument, and the landed precedent for the identical act declaredno.Changeset level
patch, and the governing text is the WHICH LEVEL block in.github/workflows/pr-automation.yml: "A purely additive widening of a published package's public surface (a new exported symbol on anindex, a new accepted key or value) takes at leastminor... afix(that changes no public surface stayspatch." This diff is afix(that changes no public surface, measured above, and it narrows the accept set rather than widening it, so neither half of theminortrigger is present.The text rejected, named rather than left implicit.
AGENTS.md:1028says a bug fix in a released package takes apatchand nevernone— that is the floor againstnone, not the level-choosing text, and a PR citing only it has cited the wrong paragraph. Route 2 of the same block (theskip-changesetlabel) is also rejected:@objectstack/objectqlcarries noprivatekey, so it publishes, and one entry is owed. It is the only touched package.Verification
Everything below was re-run at the final head
22ef049563, after mergingorigin/main(the gate deriver flagged the pre-merge tree as 8 commits stale with 3 of the files it derives from changed in that range). Exit codes were captured by redirect-then-read, never through a pipe.New pin
packages/objectql/src/plugin-audit-created-by-create-side.test.ts— 4 cases: the card's three-row rig with its controls; a create sending nocreated_by; thepreserveAudithistorical control; andpreserveAuditwith no session.@objectstack/objectqlfull package suite — 286 files, 4815 tests, all passing. Not a narrowed selection:pnpm --filter @objectstack/objectql testin full.pnpm --filter @objectstack/objectql typecheckclean, includingcheck:test-typecheck— 44 files / 242 errors / 69 pinned signatures held, the same counts as before this diff, so the new test file owes no ledger entry.@objectstack/restconsumer pins — 5 files, 56 tests, all passing, includingimport-runner-historical-readonly-insert.test.ts(the pin that drivestreatAsHistoricalthrough the real insert ingress), both public-form forgery pins and the two tenancy matrices that assert a landedcreated_by.The 57 gate families derived for this diff by
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack— the deriver takes the change set from the merge base itself, so no hand-built list can narrow it. Re-derived after the merge and found byte-identical to the pre-merge derivation, then re-run in full on the merged tree. Reconciliation, verbatim:✓ dispatch-gates --ran: 57 derived famil(ies) accounted for — 57 run, 0 NOT-MEASURED.That line accounts for coverage, not verdicts, so the verdicts are stated separately: 55 of the 57 exit 0. Two are NOT MEASURED and are reported as that rather than as passes:
pnpm check:dual-build-cjs-loads— exit 3,PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/, naming 44 packages. Its own words: "This is NOT a pass: nothing was measured."pnpm check:type-check-debt— exit 3,--re-measure cannot run: 7 workspace dependenc(ies) of the ledgered packages have no built type entry point on disk. Its own words: "This is NOT a pass and NOT a finding: nothing was measured."Both need
pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*'over the whole workspace, which CI runs before its own step and this seat deliberately did not run on a shared box. Neither is structurally reachable by this diff — no entry point, no package and no declaration file moves — but that reasoning is offered beside the reading, not in place of it.check:dts-closurereports26 built package(s) swept - 96/96 declared declaration file(s) present; it names none, so the swept set was the packages carrying adist/at that moment, and@objectstack/objectqlwas one of them (itsdist/had just been rebuilt for the measurement above).check:nul-bytesgreen, plus an independent control-character scan of the three changed files that returns nothing.Acceptance notes
Noted, not filed.
packages/objectql/src/plugin.ts's audit binder now spells all four audit fields the same way, so the asymmetry that produced two cards is gone from this function. The remaining??in the same block isrecord.tenant_id = record.tenant_id ?? session.organizationId, which is a different question — a tenant anchor, not an audit anchor, reached under a different guard — and it is left alone rather than swept into a security card's diff. It is an observation, not a measured defect, so it is written here rather than filed.Generated by Claude Code