Skip to content

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

Merged
os-musk merged 3 commits into
mainfrom
claude/issue-16311-created-by-launder
Sep 8, 2026
Merged

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
os-musk merged 3 commits into
mainfrom
claude/issue-16311-created-by-launder

Conversation

@os-musk

@os-musk os-musk commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16311

The audit binder's beforeInsert laundered a caller-supplied created_by on an ordinary create, one field over from the created_at hole 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 one if block 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_at only, with a stated reason not to extrapolate. Both conjuncts were checked against the ruling comment itself, not against a paraphrase.

The ruling names created_at because 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 it pm: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's beforeInsert stamp:

-          record.created_by = record.created_by ?? session.userId;
+          record.created_by = preserveAudit ? (record.created_by ?? session.userId) : session.userId;

which is now byte-symmetric with the line under it, record.updated_by = preserveAudit ? (record.updated_by ?? session.userId) : session.userId;.

Why the preserveAudit ternary and not a bare = session.userId. Same reason as the twin: treatAsHistorical reaches this field through this very hook. runImport({ treatAsHistorical: true }) sets preserveAudit: true on 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: an ObjectKernel with ObjectQLPlugin, so the shipped sys_stamp_audit_insert hook is bound through bindHooksToEngine and engine.insert runs the static-readonly strip after it, over a capturing driver — so what reaches driver.create is the stored row.

BEFORE. Taken with plugin.ts proved byte-identical to origin/main before the run: git hash-object gave 8d0178ae0, equal to git 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.

  row0: title=a created_by=forged_user updated_by=real_user   session { userId: 'real_user' }
  row1: title=b created_by=undefined   updated_by=undefined   session {} (no userId)
  row2: title=c created_by=forged_user updated_by=real_user   session { userId: 'real_user' }

  Test Files  1 failed (1)        Tests  1 failed | 3 passed (4)
  AssertionError: expected 'forged_user' not to be 'forged_user'

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:

  row0: title=a created_by=real_user updated_by=real_user   session { userId: 'real_user' }
  row1: title=b created_by=undefined updated_by=undefined   session {} (no userId)
  row2: title=c created_by=real_user updated_by=real_user   session { userId: 'real_user' }

  Test Files  1 passed (1)        Tests  4 passed (4)

updated_by in 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, so created_by surviving was "the strip ran and spared exactly this one", never "the strip did not run". id and run_at strip on both legs too, and a change that closed created_by while 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 created_at stamp is unconditional (driver-sql provisions the column on every table), while created_by is guarded by hasField and by session.userId. A shape decision has to say what an insert with no session should do — today it leaves the caller's forgery for the strip to take, which is the correct outcome and happens for a different reason.

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 assigned session.userId unconditionally would write undefined into 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 pins undefined on both created_by and updated_by so that regression cannot land silently, and a fourth case pins that preserveAudit without 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 twin

The predicate is exact: this change alters behaviour only where session.userId is truthy AND the caller supplied a created_by differing from it AND preserveAudit is unset. Every non-generated source under packages/, apps/ and examples/ carrying created_by as an object-literal key was classified against it. Two production creators exist; both are cleared by source, and both for the same structural reason.

site what it supplies verdict
plugins/plugin-sharing/src/share-link-service.ts:558 context.userId ?? null, inserted under SYSTEM_CTX unaffectedSYSTEM_CTX is { isSystem: true, positions: [], permissions: [] } with no userId, so the hook's guard was false before this change too and never touched the key; the isSystem strip exemption is what carries the value
rest/src/rest-server.ts:9042 (sys_import_job) createdBy, derived at :9026 from context.userId ?? context.user?.id, inserted with { ...context, isSystem: true } unaffected in both branches — where context.userId exists, createdBy is that same value, so ?? session.userId and = session.userId agree byte for byte; where only context.user?.id exists, session.userId is falsy and the hook never enters the branch at all

The dogfood seeds that back-date attribution (showcase-invoice-cbp, analytics-label-scope) drive ql.insert(..., { context: { isSystem: true } }) — again no userId, so the hook never entered that branch before or after. The two public-form fixtures that send created_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_at stamp is ungated, so every back-dating seed rode the ??. created_by writes are gated on session.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 no for 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 only packages/objectql/src/plugin.ts back to the merge base, rebuild, and compare every declaration file the package publishes by blob and by exported-name count.

file HEAD BASE verdict
index.d.ts 0585fe30f 0585fe30f identical blob
index.d.mts e33f349ed e33f349ed identical blob
core.d.ts ac7e40431 ac7e40431 identical blob
core.d.mts 611ed987c 611ed987c identical blob
util-Dw5ZTIII.d.ts / .d.mts 6c82fcacb 6c82fcacb identical blob, and the content-hashed chunk name did not move

The rebuild is proved to have re-run rather than been skipped: dist/index.js mtime advanced 1788848035 -> 1788848052 -> 1788848068 across 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 ObjectQLPlugin class moved dist/index.d.ts from 0585fe30f to e929ab428 and index.d.mts from e33f349ed to ec478a84e, 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-level export const in plugin.ts reaches no published entry point because packages/objectql/src/index.ts re-exports ObjectQLPlugin by name.

The measurement was taken before the origin/main merge, and it still describes this PR exactly: the merge touched no file under packages/objectql (git diff origin/main...HEAD lists 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 with git status --porcelain clean. The control build left in dist/ afterwards was cleared and that was verified, not assumed: ablation-dist-preflight --absent reports 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 readonly contract AUDIT_FIELD_DEFS.created_by already documents. A caller may still send created_by; it is now overwritten with the session user on an ordinary create rather than honoured. Under the rule that uncertainty resolves to yes, 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 declared no.

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 an index, a new accepted key or value) takes at least minor ... a fix( that changes no public surface stays patch." This diff is a fix( that changes no public surface, measured above, and it narrows the accept set rather than widening it, so neither half of the minor trigger is present.

The text rejected, named rather than left implicit. AGENTS.md:1028 says a bug fix in a released package takes a patch and never none — that is the floor against none, not the level-choosing text, and a PR citing only it has cited the wrong paragraph. Route 2 of the same block (the skip-changeset label) is also rejected: @objectstack/objectql carries no private key, 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 merging origin/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 no created_by; the preserveAudit historical control; and preserveAudit with no session.

  • @objectstack/objectql full package suite — 286 files, 4815 tests, all passing. Not a narrowed selection: pnpm --filter @objectstack/objectql test in full.

  • pnpm --filter @objectstack/objectql typecheck clean, including check: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/rest consumer pins — 5 files, 56 tests, all passing, including import-runner-historical-readonly-insert.test.ts (the pin that drives treatAsHistorical through the real insert ingress), both public-form forgery pins and the two tenancy matrices that assert a landed created_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-closure reports 26 built package(s) swept - 96/96 declared declaration file(s) present; it names none, so the swept set was the packages carrying a dist/ at that moment, and @objectstack/objectql was one of them (its dist/ had just been rebuilt for the measurement above).

  • check:nul-bytes green, 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 is record.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

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
@github-actions github-actions Bot added the size/m label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot 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
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 16 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 73709893f1b269870e8c08a405a4604106521e24packageMentionDocs.

Which tree this was computed on

This run read content/docs from 385e4564eb1ef9d9cbe042e98136ec6e80a5477b — the merge of head 22ef04956309528974be5fdcef9292c379cad904 into base 73709893f1b269870e8c08a405a4604106521e24, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# 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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

os-musk commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

落地前检三条全部满足 —— 已转 ready 并挂 auto-merge

domain:engine execution PM seat。

不适用 —— Clause-②: no,⛔ 从未挂过载体。

exit 0✓ check-clause2-carriers: PR #16809 / card #16311 — the clause-② declaration is readable in the fixed spelling and both carriers agree.

阴性对照发火:同一份文档、只删掉本席 07:09:04Z 那条重述 claim ⇒ exit 4。⇒ exit 0 是那条评论换来的,⛔ 不是文档凑出来的。

文档来源如实交代:dev 自己从 MCP 取的 pair.json(PR 正文原文在内,⛔ 本席未重打),本席只做两件在安全方向上的事 —— filesgit diff --name-status 从 merge base 87fad14b86 重新生成(.changeset/audit-binder-created-by-session-on-create.md A · plugin-audit-created-by-create-side.test.ts A · plugin.ts M),并追加本席那条重述 claim 的逐字开头三行。断言文档里的 head 仍等于分支当刻 head 22ef049563

✅ 10 个 workflow run 全部 completed全部钉在 22ef049563:9 success(CI · Lint & Type Check · PR Automation · Governed Surface Guard · Docs Drift Check · Check Links · Duplicate Fix Guard · Single-Claim Path Guard · Part-of Closing-Keyword Guard)· 1 skipped(Pack Smoke,opt-in)· 0 failure / 0 in_progress

翻 ready 前的正文亲扫:首行 Fixes #16311(本 PR 应当关卡,⇒ 这是有意的)。全文按「闭合关键词 40 字符内是否有编号」扫,唯一命中就是首行那一处;正文另外引用的 #14259 / #15964 / #16313 ⛔ 无一挨着关键词。阳性对照发火(Fixes #1 为真、Part of #1 为假)。

⚠️ 落地待办已记Fixes 会自动关卡,但 pm:dispatched 标签不会自动摘 —— 合并后本席手工摘除。


⭐ 顺带记一笔本轮的更正:本席一小时前在 PR #16808 上写过「② 在这条通道上满足不了」。那是一个理由,不是一次尝试。 dev 把它从 MCP 取来的 pair.json 留在了 scratchpad 里,⇒ 本席不需要重打任何东西。#16808 与本 PR 都是用同一套办法量出来的,两次都带发火的阴性对照。


Generated by Claude Code

@os-musk
os-musk added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 7783738 Sep 8, 2026
35 checks passed
@os-musk
os-musk deleted the claude/issue-16311-created-by-launder branch September 8, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The audit binder launders a caller-supplied created_by on an ordinary create, the same way #15964 does created_at

2 participants