diff --git a/.changeset/audit-binder-created-by-session-on-create.md b/.changeset/audit-binder-created-by-session-on-create.md new file mode 100644 index 0000000000..5dd5b0266a --- /dev/null +++ b/.changeset/audit-binder-created-by-session-on-create.md @@ -0,0 +1,21 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): the audit binder stamps `created_by` from the session on an ordinary create, so a caller-supplied value no longer survives a plain `POST` (#16311) + +The `beforeInsert` audit stamp was `record.created_by = record.created_by ?? session.userId` — client-preferred on every insert, with no flag and no privilege required — while its sibling one line down was already the `preserveAudit` ternary. Since the static-`readonly` strip moved INSIDE `engine.insert` (2026-09-03 ruling, option C) it runs AFTER the before-phase hooks, and its guard treats a key a `beforeInsert` hook ASSIGNED as the hook's write rather than a caller forgery. The `??` therefore laundered the caller's bytes past that strip: an authenticated `POST /api/v1/data/OBJECT` carrying `created_by: 'forged_user'` stored exactly that, on an object whose `created_by` is the registry-injected `AUDIT_FIELD_DEFS` shape (`readonly: true`), while `updated_by` in the same payload was correctly overwritten with the session user. A row could claim it was created by a user who did not create it — audit integrity, not privilege escalation. + +The stamp now takes the same shape as `updated_by`, one field over, and the same shape #15964 landed for `created_at`: + +```ts +record.created_by = preserveAudit ? (record.created_by ?? session.userId) : session.userId; +``` + +**What changes for a caller.** An ordinary create no longer preserves a supplied `created_by` — the value is overwritten with the session user rather than deleted, so the column is still a real attribution stamp. This narrows the accept set to the `readonly` contract the field already documents; no exported symbol, schema or config key moves. + +**The session-less insert is deliberately unchanged, and that is load-bearing.** Both audit-user assignments stay inside `if (session?.userId)`. With no session the hook assigns nothing and the engine's readonly strip takes the caller's value, so the key is absent — already the correct outcome today, reached by a different path. A shape that assigned `session.userId` unconditionally would write `undefined` into the key, making it one the hook "wrote", and the strip would then spare it: a branch that is correct today would become a new hole. That row is pinned. + +**The historical-import channel is unchanged and pinned.** `runImport({ treatAsHistorical: true })` sets `preserveAudit: true` on the write context (`@objectstack/rest`), and that branch still reinstates an original `created_by`, exactly as it has for `updated_by` since #3493. This is why the fix is the `preserveAudit` ternary rather than a bare `= session.userId`. + +**A creator that supplied a non-session `created_by` under an authenticated session must now ask for it** via `preserveAudit: true`. Creators that write an arbitrary `created_by` through a session-less system context (`{ isSystem: true }` with no `userId`) are untouched: the hook never entered that branch before this change either, and the `isSystem` strip exemption is what carries their value. diff --git a/packages/objectql/src/plugin-audit-created-by-create-side.test.ts b/packages/objectql/src/plugin-audit-created-by-create-side.test.ts new file mode 100644 index 0000000000..0a48411aaa --- /dev/null +++ b/packages/objectql/src/plugin-audit-created-by-create-side.test.ts @@ -0,0 +1,239 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #16311 — on an ORDINARY create the audit binder stamps `created_by` from the +// SESSION, so a caller-supplied value never survives a plain REST `POST`. +// +// This is the one-field-over twin of #15964 / PR #16313 (`created_at`), and it +// is deliberately shaped the same way: two fields fixed two ways inside one +// function is how this card came to exist in the first place. +// +// ## The hole +// +// The audit binder's `beforeInsert` read +// +// record.created_by = record.created_by ?? session.userId; // client-preferred, no flag +// record.updated_by = preserveAudit ? (record.updated_by ?? session.userId) : session.userId; // server-set +// +// — two spellings in one `if` block, one line apart. Since #15395 the +// static-`readonly` strip runs INSIDE `engine.insert`, AFTER the `beforeInsert` +// hooks, and #14259's guard reads a key a hook ASSIGNED as the hook's write +// rather than a caller forgery (`rowHookWrittenKeys`). The `??` therefore +// LAUNDERED the caller's bytes past that strip: an ordinary authenticated POST +// stored `created_by: 'forged_user'` on an object whose `created_by` is the +// registry-injected `AUDIT_FIELD_DEFS` shape, `readonly: true`. +// +// ## The card's rig, reproduced verbatim in the first case below +// +// 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' } +// +// row0/row2 carry the reading, and `updated_by` in the SAME payload is the +// in-experiment control: it proves the strip ran on this row and took the +// sibling audit field, so `created_by` surviving was "the strip ran and spared +// exactly this one", never "the strip did not run". +// +// ## row1 is an ACCEPTANCE criterion, not a nicety +// +// `created_by` is NOT symmetric with `created_at`. The `created_at` stamp is +// unconditional (driver-sql provisions that column on every table), while +// `created_by` sits inside `if (session?.userId)` and behind `hasField`. With no +// session the hook assigns nothing and the engine strip then takes the caller's +// forgery correctly — the right outcome, reached by a path this card is not +// about. So the fix stays INSIDE that guard. +// +// A shape that assigned `session.userId` unconditionally would write `undefined` +// into the key on a session-less insert, making it a key the hook "wrote"; the +// strip would then spare it, and a branch that is correct today would become a +// NEW hole. row1 pins `undefined` so that regression cannot land silently. +// +// ## Ruling +// +// Direction settled by the maintainer ruling of 2026-09-06 on #15964 (decision +// batch #54, option A, verbatim 「同意」) and applied to this sibling field per +// the triage ruling on #16311: the audit anchor may not be supplied by the +// caller on an ordinary create, and the historical-import channel keeps working +// through `preserveAudit` — what `runImport({ treatAsHistorical: true })` sets +// on the write context (`packages/rest/src/import-runner.ts`). That is why the +// fix is the `preserveAudit` ternary and not a bare `= session.userId`. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQLPlugin } from './plugin.js'; +import { ObjectQL } from './engine.js'; + +const FORGED_USER = 'forged_user'; +const REAL_USER = 'real_user'; +const FORGED_AT = '1999-01-01T00:00:00.000Z'; +const FORGED_ID = 'conv_REST_FORGED'; + +describe('audit binder: create-side `created_by` (#16311)', () => { + let kernel: ObjectKernel; + + beforeEach(() => { + kernel = new ObjectKernel({ logger: { level: 'silent' }, gracefulShutdown: false }); + }); + + afterEach(async () => { + if (kernel.getState() === 'running') await kernel.shutdown(); + }); + + /** + * Boots the REAL ingress this card is about: `ObjectQLPlugin` binds its + * `sys_stamp_audit_insert` hook through `bindHooksToEngine`, and + * `engine.insert` runs the static-readonly strip after it. What reaches the + * driver's `create` IS the stored row, so the payload is the verdict. + */ + async function boot(objectName: string) { + const captured: Record[] = []; + const mockDriver = { + name: 'audit-capture', version: '1.0.0', + connect: async () => {}, disconnect: async () => {}, + find: async () => [], findOne: async () => null, + create: async (_o: string, d: any) => { + captured.push({ ...d }); + return { id: d.id ?? 'minted_id', ...d }; + }, + update: async (_o: string, i: any, d: any) => ({ id: i, ...d }), + delete: async () => true, syncSchema: async () => {}, + }; + await kernel.use({ + name: 'audit-capture-plugin', type: 'driver', version: '1.0.0', + init: async (ctx: any) => { ctx.registerService('driver.audit-capture', mockDriver); }, + } as any); + await kernel.use(new ObjectQLPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql'); + // `created_by` / `updated_by` are NOT declared here: the registry injects + // the whole audit family from `AUDIT_FIELD_DEFS`, where `created_by` is a + // `readonly: true` lookup to `sys_user` — the declaration the card's rig + // describes, and the contract this fix pulls back to. + const schema = { + name: objectName, + label: 'Repro Object', + datasource: 'audit-capture', + fields: { + id: { name: 'id', label: 'Id', type: 'text', readonly: true }, + title: { name: 'title', label: 'Title', type: 'text' }, + run_at: { name: 'run_at', label: 'Run At', type: 'datetime', readonly: true }, + }, + } as any; + objectql.registry.registerObject(schema, 'test', 'test'); + return { objectql, captured }; + } + + const forgedPayload = (title: string) => ({ + id: FORGED_ID, + title, + run_at: FORGED_AT, + created_by: FORGED_USER, + updated_by: FORGED_USER, + }); + + /** Prints the card's own three-row table for the rows that reached the driver. */ + function printRig(rows: Record[], sessions: string[]) { + const cell = (v: unknown) => String(v); + // eslint-disable-next-line no-console + console.log( + `\n[#16311 rig — what reached driver.create]\n` + + rows + .map( + (r, i) => + ` row${i}: title=${cell(r.title)} created_by=${cell(r.created_by)} ` + + `updated_by=${cell(r.updated_by)} ${sessions[i]}`, + ) + .join('\n') + + '\n', + ); + } + + it("the card's three-row rig: a forged `created_by` does NOT survive an ordinary create, and the session-less row stays `undefined`", async () => { + const { objectql, captured } = await boot('repro_conversations'); + + // row0 and row2 — an ordinary authenticated caller. + await objectql.insert('repro_conversations', forgedPayload('a'), { + context: { userId: REAL_USER }, + }); + // row1 — the SECOND control: no `session.userId` at all. + await objectql.insert('repro_conversations', forgedPayload('b'), { context: {} }); + await objectql.insert('repro_conversations', forgedPayload('c'), { + context: { userId: REAL_USER }, + }); + + expect(captured.length).toBe(3); + const [row0, row1, row2] = captured; + printRig(captured, [ + `session { userId: '${REAL_USER}' }`, + 'session {} (no userId)', + `session { userId: '${REAL_USER}' }`, + ]); + + for (const row of [row0, row2]) { + // The in-experiment controls — both were already correct BEFORE this + // change, and a fix that closes `created_by` while opening either of them + // is a regression on a security card. + expect(row.id).not.toBe(FORGED_ID); + expect(row.run_at).not.toBe(FORGED_AT); + expect(row.updated_by).toBe(REAL_USER); + + // The card's row. Overwritten by the binder rather than deleted, so the + // column is still a real attribution stamp. + expect(row.created_by).not.toBe(FORGED_USER); + expect(row.created_by).toBe(REAL_USER); + } + + // row1 — the acceptance criterion the triage ruling makes mandatory. With + // no session the hook must assign NOTHING, so the engine strip takes the + // forgery and the key is absent. A shape that assigned `session.userId` + // unconditionally would store `undefined` as a hook write and the strip + // would spare it, turning a correct branch into a new hole. + expect(row1.created_by).toBeUndefined(); + expect(row1.updated_by).toBeUndefined(); + // …and its own controls, proving the strip really did run on this row. + expect(row1.id).not.toBe(FORGED_ID); + expect(row1.run_at).not.toBe(FORGED_AT); + }); + + it('a create that sends no `created_by` is still stamped from the session (the binder keeps doing its job)', async () => { + const { objectql, captured } = await boot('repro_plain'); + + await objectql.insert('repro_plain', { title: 'x' }, { context: { userId: REAL_USER } }); + + const row = captured[0]; + expect(row.created_by).toBe(REAL_USER); + expect(row.updated_by).toBe(REAL_USER); + }); + + it('`preserveAudit` (what `treatAsHistorical` sets) still reinstates the original `created_by`', async () => { + const { objectql, captured } = await boot('repro_historical'); + + await objectql.insert('repro_historical', forgedPayload('h'), { + context: { userId: REAL_USER, preserveAudit: true }, + }); + + const row = captured[0]; + expect(row.created_by).toBe(FORGED_USER); + // Symmetric with `updated_by`, which has had this branch since #3493. + expect(row.updated_by).toBe(FORGED_USER); + // …and the exemption is the audit binder's, not the strip's: an ordinary + // author-declared readonly field is still taken on the create side + // (2026-08-08 ruling, unchanged by this card). + expect(row.run_at).not.toBe(FORGED_AT); + }); + + it('`preserveAudit` without a session does NOT resurrect the forgery — the guard still wins', async () => { + const { objectql, captured } = await boot('repro_historical_anon'); + + await objectql.insert('repro_historical_anon', forgedPayload('n'), { + context: { preserveAudit: true }, + }); + + // Both audit-user keys stay inside `if (session?.userId)`, so with no + // session there is nobody to attribute the row to and nothing is written — + // the strip takes the caller's value on both, exactly as on row1. + const row = captured[0]; + expect(row.created_by).toBeUndefined(); + expect(row.updated_by).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index 8d0178ae00..f397398d2a 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -1100,10 +1100,10 @@ export class ObjectQLPlugin implements Plugin { ) => { const now = stamp(); // A "historical" import (#3493) reinstates the ORIGINAL timeline, so a - // client-supplied created_at/updated_at/updated_by is CLIENT-PREFERRED - // under `preserveAudit` — instead of being overwritten with the import - // instant. Opt-in and server-set only; a normal write leaves - // `preserveAudit` unset and still stamps now. + // client-supplied created_at/created_by/updated_at/updated_by is + // CLIENT-PREFERRED under `preserveAudit` — instead of being overwritten + // with the import instant. Opt-in and server-set only; a normal write + // leaves `preserveAudit` unset and still stamps now. // // [#15964] `created_at` takes the SAME SHAPE as `updated_at`, on the // maintainer ruling of 2026-09-06 (decision batch #54, option A). It was @@ -1123,9 +1123,28 @@ export class ObjectQLPlugin implements Plugin { record.created_at = preserveAudit ? (record.created_at ?? now) : now; } record.updated_at = preserveAudit ? (record.updated_at ?? now) : now; + // [#16311] `created_by` takes the SAME SHAPE as `updated_by`, one field + // over — the identical laundering, measured on the same rig and closed + // the same way, because two fields fixed two ways inside one function is + // how this second card came to exist at all. It was + // `record.created_by ?? session.userId`: client-preferred with no flag, + // so an ordinary authenticated POST stored `created_by: 'forged_user'` + // while `updated_by` in the SAME payload was correctly overwritten with + // the session user — the control proving the strip ran on that row and + // took the sibling audit field. + // + // Both audit-user assignments stay INSIDE `if (session?.userId)`, and + // that guard is load-bearing rather than incidental. `created_by` is not + // symmetric with `created_at`: with no session the hook must assign + // NOTHING, so the engine strip takes a caller's forgery and the key is + // absent. Assigning `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. Pinned as row1 of + // `plugin-audit-created-by-create-side.test.ts`. if (session?.userId) { if (isInsert && hasField(objectName, 'created_by')) { - record.created_by = record.created_by ?? session.userId; + record.created_by = preserveAudit ? (record.created_by ?? session.userId) : session.userId; } if (hasField(objectName, 'updated_by')) { record.updated_by = preserveAudit ? (record.updated_by ?? session.userId) : session.userId;