From ea9aca567f9cbaeb1357bdf36432f5f215d89a47 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 16:09:42 +0000 Subject: [PATCH 1/2] fix(objectql): refuse undeclared insert fields at the schema, and keep bound values out of the write-path logs (#8682) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Half A — an undeclared write key is refused by the object's field map before `applyFieldDefaults`, so no default, no summary seeding, no beforeInsert hook, no secret write, no validation and no AUTONUMBER runs for a request that was already going to be refused. Measured on origin/main, one mistyped key made a valid create numbered 0001 be followed by 0003; it is now followed by 0002. The wire answer is unchanged: 400 INVALID_FIELD with the same message. Half B — the insert/update/delete loggers no longer write the driver's bound statement. `Logger` serializes exactly `message` and `stack`, and the statement opened both, so a single mistyped field name wrote an entire row's values to disk at ERROR. The level, the message, the object and the database's own diagnostic (which names the failing column) are all kept; the rethrown error is untouched so the caller's 400 does not move. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- .../src/driver-fault-redaction.test.ts | 251 ++++++++++++++++++ .../objectql/src/driver-fault-redaction.ts | 153 +++++++++++ .../objectql/src/engine-insert-many.test.ts | 9 +- .../engine-undeclared-field-preflight.test.ts | 248 +++++++++++++++++ packages/objectql/src/engine.ts | 171 +++++++++++- .../src/protocol-recorded-by-null.test.ts | 10 + .../src/protocol-registry-shadow.test.ts | 10 + ...ol-save-meta-repo-path-real-engine.test.ts | 7 + .../publish-meta-response-conformance.test.ts | 10 + .../save-meta-response-conformance.test.ts | 10 + 10 files changed, 872 insertions(+), 7 deletions(-) create mode 100644 packages/objectql/src/driver-fault-redaction.test.ts create mode 100644 packages/objectql/src/driver-fault-redaction.ts create mode 100644 packages/objectql/src/engine-undeclared-field-preflight.test.ts diff --git a/packages/objectql/src/driver-fault-redaction.test.ts b/packages/objectql/src/driver-fault-redaction.test.ts new file mode 100644 index 0000000000..8153f030e2 --- /dev/null +++ b/packages/objectql/src/driver-fault-redaction.test.ts @@ -0,0 +1,251 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #8682 half B — a driver-level write fault is logged WITHOUT the caller's +// values. +// +// Measured on `origin/main` @ 3508678 with planted canaries, a single insert +// carrying one misspelled field name: +// +// message carries SENSITIVE-CANARY-9f3a2b true +// stack carries SENSITIVE-CANARY-9f3a2b true ← the same statement twice +// +// `Logger.error(msg, error, meta)` serializes exactly `error.message` and +// `error.stack` (`ObjectLogger.write`, and both `@objectstack/observability` +// loggers do the same), so those two fields ARE the exposure — redacting one +// and not the other would have moved the leak rather than closed it. +// +// ⛔ What this suite must never be read as licence for: LOWERING the level or +// DROPPING the entry. Every case below that asserts a value is gone has a +// sibling asserting the line is still there, at `error`, still saying `Insert +// operation failed`, still naming the object AND the failing column the +// database itself named. "Stopped leaking" and "stopped reporting" are the two +// outcomes this file exists to tell apart — a driver fault nobody can debug is +// the tolerant-fallback direction, not the loud one. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { redactBoundStatement, redactStatementFromMessage } from './driver-fault-redaction.js'; + +/** The canaries the card planted, kept verbatim so a leak is unmistakable. */ +const SECRET = 'SENSITIVE-CANARY-9f3a2b'; +const DESCRIPTION = 'DESCRIPTION-VALUE-CANARY'; + +/** knex's shape: the fully bound statement, ` - `, then the database's own words. */ +const BOUND_INSERT = + 'insert into `crm_account` (`description`, `name`, `zzz_secret_field`) values ' + + `('${DESCRIPTION}', 'P689 probe', '${SECRET}')` + + ' returning * - table crm_account has no column named zzz_secret_field'; + +describe('redactStatementFromMessage', () => { + it('keeps the database`s diagnostic and drops the bound statement', () => { + const out = redactStatementFromMessage(BOUND_INSERT); + + expect(out).toContain('table crm_account has no column named zzz_secret_field'); + expect(out).not.toContain(SECRET); + expect(out).not.toContain(DESCRIPTION); + expect(out).not.toContain('insert into'); + }); + + it('cuts at the LAST separator, so a value containing " - " leaves no fragment', () => { + // The reason the cut is the last separator and not the first: cutting early + // would leave the tail of the value standing in what we then log as "the + // diagnostic". + const out = redactStatementFromMessage( + "insert into `t` (`label`) values ('2026 - Q3 secret plan') returning * - table t has no column named label", + ); + + expect(out).toContain('table t has no column named label'); + expect(out).not.toContain('Q3 secret plan'); + }); + + it.each([ + ['postgres', 'insert into "t" ("c") values (\'boundvalue\') - column "c" of relation "t" does not exist', 'column "c" of relation "t" does not exist'], + ['mysql via knex', "insert into `t` (`c`) values ('boundvalue') - Unknown column 'c' in 'field list'", "Unknown column 'c' in 'field list'"], + ['update — values ride the `set` clause', "update `t` set `c` = 'boundvalue' where `id` = 'r1' - table t has no column named c", 'table t has no column named c'], + ['delete — values ride the `where` clause', "delete from `t` where `email` = 'boundvalue@example.com' - no such column: email", 'no such column: email'], + ])('%s', (_dialect, message, diagnostic) => { + const out = redactStatementFromMessage(message); + + expect(out).toContain(diagnostic); + // The bound literal, and the statement that carried it, are both gone. + expect(out).not.toContain('boundvalue'); + expect(out).not.toMatch(/insert into|update `|delete from/); + }); + + it('leaves a driver dump that carries no statement exactly as it was', () => { + // Nothing to cut: these are already the diagnostic, and the column they + // name is the operator's whole answer. + for (const message of [ + 'UNIQUE constraint failed: sys_user.email', + 'NOT NULL constraint failed: sys_team.organization_id', + 'SQLITE_CONSTRAINT_NOTNULL: NOT NULL constraint failed: t.c', + ]) { + expect(redactStatementFromMessage(message)).toBe(message); + } + }); + + it('leaves ordinary business and validation prose alone, dashes included', () => { + // The verdict comes from `looksLikeInternalErrorLeak`, which is pinned from + // both directions in `@objectstack/types`. A hook's own message is not a + // driver dump however it is punctuated, and mangling it would replace a + // real answer with a redaction notice. + for (const message of [ + 'name is required', + 'Order 4711 - cannot be closed while lines are open', + "Object 'ghost' is not registered", + '删除被阻断:该客户下仍有未结订单', + ]) { + expect(redactStatementFromMessage(message)).toBe(message); + } + }); +}); + +describe('redactBoundStatement', () => { + it('redacts `stack` too — the statement opened it a second time', () => { + const original = new Error(BOUND_INSERT); + original.name = 'SqliteError'; + original.stack = `SqliteError: ${BOUND_INSERT}\n at Database.prepare (/x/better-sqlite3.js:1:1)\n at create (/x/driver.js:2:2)`; + + const redacted = redactBoundStatement(original) as Error; + + expect(redacted.message).not.toContain(SECRET); + expect(redacted.stack).not.toContain(SECRET); + expect(redacted.stack).not.toContain(DESCRIPTION); + // The frames survive: the redaction narrows WHAT is written, it does not + // take the operator's stack away. + expect(redacted.stack).toContain('at Database.prepare (/x/better-sqlite3.js:1:1)'); + expect(redacted.stack).toContain('at create (/x/driver.js:2:2)'); + expect(redacted.stack).toContain('SqliteError: table crm_account has no column named zzz_secret_field'); + }); + + it('rebuilds a header that spanned several lines', () => { + // A bound statement can contain newlines, so the header is not reliably one + // line — which is why the frames are found rather than the message matched. + const original = new Error("insert into `t`\n(`c`)\nvalues ('multi\nline secret') - table t has no column named c"); + original.name = 'SqliteError'; + original.stack = `SqliteError: ${original.message}\n at only (/x/y.js:1:1)`; + + const redacted = redactBoundStatement(original) as Error; + + expect(redacted.stack).not.toContain('line secret'); + expect(redacted.stack).toBe('SqliteError: table t has no column named c [statement and bound values redacted]\n at only (/x/y.js:1:1)'); + }); + + it('returns the SAME error when there is nothing to redact', () => { + // Identity, not equality: a validation failure, a hook's business error and + // a bare `Error` from our own code must reach the log untouched, and the + // cheapest proof of that is that no new object was made. + const untouched = new Error('name is required'); + + expect(redactBoundStatement(untouched)).toBe(untouched); + }); + + it('passes a non-Error through unchanged', () => { + expect(redactBoundStatement('a thrown string')).toBe('a thrown string'); + expect(redactBoundStatement(undefined)).toBe(undefined); + }); +}); + +/** + * The engine-level half: the SAME insert the card measured, but on a DECLARED + * field whose physical column is missing — schema drift, the one shape that + * still reaches the driver after half A's door closed the undeclared-key route. + * This is the case the redaction exists for, and the case where the ERROR line + * must survive intact. + */ +describe('#8682 half B — the write-path loggers', () => { + function makeCapturingLogger() { + const lines: Array<{ level: string; msg: string; err?: any; meta?: any }> = []; + const logger: any = { + lines, + trace() {}, fatal() {}, + debug() {}, info() {}, + warn(msg: string) { lines.push({ level: 'warn', msg: String(msg) }); }, + error(msg: string, err?: any, meta?: any) { lines.push({ level: 'error', msg: String(msg), err, meta }); }, + child() { return logger; }, + }; + return logger; + } + + async function insertAgainstADriftedColumn() { + const logger = makeCapturingLogger(); + const engine = new ObjectQL({ logger }); + const driver: any = { + name: 'drifted', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { return []; }, + async findOne() { return null; }, + async create(object: string, data: Record) { + const cols = Object.keys(data).sort(); + const stmt = `insert into \`${object}\` (${cols.map((c) => `\`${c}\``).join(', ')}) values (${cols.map((c) => `'${String(data[c])}'`).join(', ')}) returning *`; + const e: any = new Error(`${stmt} - table ${object} has no column named secret_note`); + e.name = 'SqliteError'; + e.code = 'SQLITE_ERROR'; + e.stack = `SqliteError: ${stmt} - table ${object} has no column named secret_note\n at Database.prepare (/x/better-sqlite3.js:1:1)`; + throw e; + }, + async update() { return {}; }, async updateMany() { return 0; }, + async delete() { return true; }, async deleteMany() { return 0; }, async count() { return 0; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'crm_account', + fields: { + id: { name: 'id', type: 'text', primaryKey: true, readonly: true }, + name: { name: 'name', type: 'text' }, + description: { name: 'description', type: 'text' }, + // DECLARED — and the table does not have it. The door in half A cannot + // see this and must not: it is drift, not a caller mistake. + secret_note: { name: 'secret_note', type: 'text' }, + }, + } as any, 'test'); + + let thrown: any = null; + try { + await engine.insert('crm_account', { + name: 'P689 probe', description: DESCRIPTION, secret_note: SECRET, + } as any); + } catch (e) { thrown = e; } + const line = logger.lines.find((l: any) => l.msg === 'Insert operation failed'); + return { line, thrown }; + } + + it('the entry survives — same level, same message, same object', async () => { + const { line } = await insertAgainstADriftedColumn(); + + expect(line).toBeDefined(); + expect(line!.level).toBe('error'); + expect(line!.meta).toEqual({ object: 'crm_account' }); + }); + + it('the failing column is still named — the fault stays debuggable', async () => { + const { line } = await insertAgainstADriftedColumn(); + + expect(String(line!.err?.message)).toContain('has no column named secret_note'); + expect(String(line!.err?.stack)).toContain('at Database.prepare'); + }); + + it('neither `message` nor `stack` carries a caller value', async () => { + const { line } = await insertAgainstADriftedColumn(); + + for (const field of [String(line!.err?.message), String(line!.err?.stack)]) { + expect(field).not.toContain(SECRET); + expect(field).not.toContain(DESCRIPTION); + expect(field).not.toContain('insert into'); + } + }); + + it('the RETHROWN error is untouched — the caller`s 400 must not move', async () => { + // `mapDataError` reads the driver's raw message to extract the failing + // field and answer `400 INVALID_FIELD`. Redacting what we THROW would break + // that answer; the redaction is one argument at one call site. + const { thrown } = await insertAgainstADriftedColumn(); + + expect(String(thrown?.message)).toContain('insert into'); + expect(String(thrown?.message)).toContain(SECRET); + }); +}); diff --git a/packages/objectql/src/driver-fault-redaction.ts b/packages/objectql/src/driver-fault-redaction.ts new file mode 100644 index 0000000000..3fdda839f4 --- /dev/null +++ b/packages/objectql/src/driver-fault-redaction.ts @@ -0,0 +1,153 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8682] Keep the CALLER'S VALUES out of the server log when a driver-level + * write fault is logged. + * + * ## The exposure this closes + * + * A SQL driver builds its error message by prefixing the offending statement — + * fully bound, values inlined — to the database's own diagnostic. knex's shape + * is ` - `. The engine's write paths log that error, + * and `Logger` serializes exactly two of its fields (`message`, `stack`), so a + * single mistyped field name in a client request wrote an entire row's values + * to disk at ERROR level, twice: + * + * ``` + * ERROR Insert operation failed {"object":"crm_account","error":{"message": + * "insert into `crm_account` (`account_number`, …, `zzz_nonexistent_field`) + * values ('ACC-000011', …, 'SENSITIVE-CANARY-9f3a2b') returning * + * - table crm_account has no column named zzz_nonexistent_field", "stack": + * "SqliteError: insert into `crm_account` (…) values (…) returning * - …"}} + * ``` + * + * Measured with planted canaries: the row's values, the organization id and + * the acting user id all landed in the log, and `stack` re-opened with the + * whole statement a second time — so redacting `message` alone would have + * moved the leak rather than closed it. Both fields are rebuilt here. + * + * ## What is KEPT, deliberately + * + * The driver's own diagnostic — the tail — is the half that names the failing + * column and the condition (`table crm_account has no column named + * zzz_nonexistent_field`, `NOT NULL constraint failed: sys_team.name`). It is + * kept verbatim, and the log site keeps the `object` it already carried. ⛔ The + * remedy for this exposure is NOT to lower the level or drop the entry: a + * driver-level fault that logs nothing is a fault nobody can debug, which is + * strictly worse than one logged too loudly. This narrows WHAT is written; the + * level, the message and the entry are untouched. + * + * ## Why the cut is at the separator, and not at a statement keyword + * + * "Is this a driver dump?" already has ONE owner in this repo — + * {@link looksLikeInternalErrorLeak} in `@objectstack/types`, applied by both + * HTTP boundaries and tested from both directions (it catches dialect codes, + * bare statements and constraint dumps; it deliberately does NOT fire on + * ordinary business or validation prose). Re-deriving a second statement-head + * keyword list here is exactly the drift that module exists to prevent, so this + * asks it the verdict and adds only the one thing it does not answer: WHERE the + * statement ends. + * + * That answer is structural, not lexical. Every value a driver inlines sits in + * the statement, and the statement always comes FIRST — so the last ` - ` in a + * message the shared predicate has already called a driver dump separates the + * part that may carry values from the part that may not. The cut takes the + * LAST separator rather than the first on purpose: a bound value may itself + * contain ` - ` (`'2026 - Q3 plan'`), and cutting at the first would leave a + * fragment of that value standing in the tail. Cutting at the last can only + * ever discard MORE than necessary, which is the safe direction — and when a + * native message legitimately contains ` - `, what is lost is a prefix of the + * diagnostic, never the failing identifier the database names at the end. + * + * A driver dump with no separator carries no statement to cut (`UNIQUE + * constraint failed: sys_user.email`, `SQLITE_CONSTRAINT_NOTNULL: …`) and is + * returned untouched — there is nothing there but the diagnostic already. + * + * ## Not a change to the thrown error + * + * This never mutates and never replaces what the engine rethrows. The REST + * boundary reads the driver's raw message to answer `400 INVALID_FIELD` with + * the failing field name (`mapDataError`), and that answer is correct and must + * stay byte-identical. The redaction applies to the LOG SLOT only — one + * argument at one call site — so the caller's answer and the operator's log + * diverge exactly where they should. + */ + +import { looksLikeInternalErrorLeak } from '@objectstack/types'; + +/** + * knex joins the bound statement to the database's own message with this + * separator. It is the only structural marker the shape offers, and it is + * stable across the dialects this repo runs (`driver-sql`, `driver-turso`, + * `driver-sqlite-wasm` all reach knex). + */ +const STATEMENT_SEPARATOR = ' - '; + +/** What replaces a statement that carried nothing but values. */ +const REDACTED_STATEMENT = '[statement and bound values redacted]'; + +/** + * The first stack FRAME line (` at …`). Everything above it is the header + * that repeats `name: message` — and therefore repeats the statement. + */ +const FIRST_STACK_FRAME = /^[ \t]*at\s/m; + +/** + * Strip the bound statement from a driver error's `message` and `stack`, + * keeping the database's own diagnostic. + * + * Returns the input UNCHANGED (same reference) when there is nothing to + * redact — not a driver dump, or a dump that carries no statement — so a + * validation error, a hook's business error and a bare `Error` from our own + * code all reach the log exactly as before, stack frames included. + * + * Otherwise returns a NEW `Error` carrying the redacted text and the original + * frames. It must be a real `Error`: `ObjectLogger.error` routes its second + * argument by `instanceof Error`, and a plain object would silently land in + * the meta slot instead. + * + * @param error - the thrown value, of any shape. + */ +export function redactBoundStatement(error: unknown): unknown { + if (!(error instanceof Error)) return error; + const redactedMessage = redactStatementFromMessage(error.message); + if (redactedMessage === error.message) return error; + + const redacted = new Error(redactedMessage); + redacted.name = error.name; + redacted.stack = redactStack(error.stack, error.name, redactedMessage); + return redacted; +} + +/** + * The message half, exported for the cases that pin the cut directly. + * Returns the input string unchanged when nothing is cut. + */ +export function redactStatementFromMessage(message: string): string { + if (!message || !looksLikeInternalErrorLeak(message)) return message; + const cut = message.lastIndexOf(STATEMENT_SEPARATOR); + if (cut === -1) return message; + const diagnostic = message.slice(cut + STATEMENT_SEPARATOR.length).trim(); + // A dump whose tail is empty still had its head removed: report the + // redaction rather than an empty message, so the entry never reads as a + // fault with no detail at all. + return diagnostic.length > 0 + ? `${diagnostic} ${REDACTED_STATEMENT}` + : REDACTED_STATEMENT; +} + +/** + * Rebuild `stack` so its header carries the redacted message instead of the + * statement, keeping every frame. + * + * The header is located by the first FRAME line, not by matching the message: + * a bound statement can contain newlines, so the header is not reliably one + * line and a `replace(message, …)` would leave the remainder standing. + */ +function redactStack(stack: string | undefined, name: string, redactedMessage: string): string | undefined { + const header = `${name}: ${redactedMessage}`; + if (stack === undefined) return undefined; + const frame = FIRST_STACK_FRAME.exec(stack); + if (!frame) return header; + return `${header}\n${stack.slice(frame.index)}`; +} diff --git a/packages/objectql/src/engine-insert-many.test.ts b/packages/objectql/src/engine-insert-many.test.ts index 07326115dc..2ac4c12f49 100644 --- a/packages/objectql/src/engine-insert-many.test.ts +++ b/packages/objectql/src/engine-insert-many.test.ts @@ -88,7 +88,14 @@ describe('engine.insertMany — partial-success batch insert (framework#3172)', }, { object: 'task' }); const outcomes = await engine.insertMany('task', [ - { name: 'good1' }, { slug: 'no-name' }, { name: 'good2' }, + // [#8682] The middle row's defect is the MISSING REQUIRED `name` — which + // is what this case is about, and what makes the hook run for it. It used + // to be spelled `{ slug: 'no-name' }`, which carried a second, undeclared + // defect nobody meant to test: `slug` is not a field of `task`, so the + // declared-field door now refuses that row BEFORE the hooks, and the + // assertion below would be measuring the wrong refusal. An empty row says + // exactly the one thing intended. + { name: 'good1' }, {}, { name: 'good2' }, ]); // The #3152 acceptance criterion: hooks fired ONCE per row. diff --git a/packages/objectql/src/engine-undeclared-field-preflight.test.ts b/packages/objectql/src/engine-undeclared-field-preflight.test.ts new file mode 100644 index 0000000000..294ca9028e --- /dev/null +++ b/packages/objectql/src/engine-undeclared-field-preflight.test.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #8682 half A — an undeclared field must be refused by the SCHEMA, before the +// insert path produces anything for a request that is already refused. +// +// The client answer was never the defect: a mistyped field name has always come +// back as a clean `400 INVALID_FIELD`, because the DRIVER refused the statement +// and `mapDataError` translated `table X has no column named c` into that +// envelope. What this suite is about is everything that ran BEHIND that answer. +// +// Measured on `origin/main` @ 3508678, real `ObjectQL` + the recording driver +// below, one mistyped key on a single insert: +// +// autonumber before the bad request 0001 +// the bad request refused — SQLITE_ERROR, from the driver +// autonumber after the bad request 0003 ← gap of 1, permanently +// beforeInsert hooks fired ok-1, bad, ok-2 ← ran for the refused row +// driver create() calls 3 ← the refused row reached the driver +// +// After the door: `0001` → refused → `0002`, hooks `ok-1, ok-2`, two creates. +// +// ## Why the AUTONUMBER is the pin and the log is not +// +// The card's own strongest evidence is the sequence gap, and it is the only +// observable here that does not depend on log contents — which matters +// especially in this PR, whose other half REWRITES what the write-path loggers +// emit. A pin that grepped log text would have rotted the moment half B landed, +// in the same commit that made it pass. So the refusal is asserted on the +// caller's error envelope (ADR-0112 `code` + `status`) and on three +// side-effect counters, and never on a log line. +// +// A sequence gap is not bookkeeping: `ACC-000014` is a document number an end +// user reads on an invoice, and nothing ever reissues it. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; + +/** Records everything that reached the driver — the counts are the point. */ +function makeRecordingDriver(missingColumns: readonly string[] = []) { + const writes: Array<{ fn: string; data: Record }> = []; + const driver: any = { + name: 'recording', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { return []; }, + async findOne() { return null; }, + async create(object: string, data: Record) { + writes.push({ fn: 'create', data: { ...data } }); + const bad = missingColumns.find((c) => c in data); + // The shape knex produces: the bound statement, then ` - `, then the + // database's own diagnostic. + if (bad) throw new Error(`insert into \`${object}\` (\`${bad}\`) values ('v') returning * - table ${object} has no column named ${bad}`); + return { id: `rec_${writes.length}`, ...data }; + }, + async update(_o: string, id: string, data: Record) { writes.push({ fn: 'update', data: { ...data } }); return { id, ...data }; }, + async updateMany() { return 0; }, + async delete() { return true; }, + async deleteMany() { return 0; }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => driver.create(object, r))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, writes }; +} + +/** + * `account_number` is the autonumber whose gap is this suite's observable, and + * the `beforeInsert` hook DERIVES a value the caller never sent — the + * `period_label = 'Q3 2026'` shape the card measured on `crm_forecast`, which + * is what makes "the hook ran" a fact about app behaviour and not just a + * counter. + */ +async function makeEngine(options: { missingColumns?: readonly string[]; registerObject?: boolean } = {}) { + const engine = new ObjectQL({ logger: silentLogger() }); + const { driver, writes } = makeRecordingDriver(options.missingColumns ?? []); + engine.registerDriver(driver, true); + await engine.init(); + if (options.registerObject !== false) { + engine.registry.registerObject({ + name: 'acct', + fields: { + id: { name: 'id', type: 'text', primaryKey: true, readonly: true }, + name: { name: 'name', type: 'text' }, + description: { name: 'description', type: 'text' }, + account_number: { name: 'account_number', type: 'autonumber' }, + }, + } as any, 'test'); + } + const hookRuns: string[] = []; + engine.registerHook('beforeInsert', (ctx: any) => { + hookRuns.push(String(ctx.input.data?.name ?? '?')); + ctx.input.data.description = `derived-for-${ctx.input.data?.name}`; + }, { object: 'acct' }); + return { engine, writes, hookRuns }; +} + +function silentLogger() { + const logger: any = { + trace() {}, debug() {}, info() {}, warn() {}, error() {}, fatal() {}, + child() { return logger; }, + }; + return logger; +} + +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e; + } + return null; +} + +describe('#8682 half A — the declared-field door', () => { + it('REPRODUCED then FIXED: the rejected request consumes no autonumber', async () => { + const { engine } = await makeEngine(); + + const before: any = await engine.insert('acct', { name: 'ok-1' }); + await refusalOf(() => engine.insert('acct', { name: 'bad', zzz_nonexistent_field: 'x' } as any)); + const after: any = await engine.insert('acct', { name: 'ok-2' }); + + // On `origin/main` this read 0001 → 0003. The refused request took 0002 + // into the void; the gap was permanent. + expect(before.account_number).toBe('0001'); + expect(after.account_number).toBe('0002'); + }); + + it('the app`s beforeInsert hook does not run for the refused row', async () => { + const { engine, hookRuns } = await makeEngine(); + + await engine.insert('acct', { name: 'ok-1' }); + await refusalOf(() => engine.insert('acct', { name: 'bad', zzz_nonexistent_field: 'x' } as any)); + await engine.insert('acct', { name: 'ok-2' }); + + // `'bad'` between them on `origin/main`: the hook computed a derived value + // for a request the server had already decided to refuse. + expect(hookRuns).toEqual(['ok-1', 'ok-2']); + }); + + it('nothing reaches the driver', async () => { + const { engine, writes } = await makeEngine(); + + await refusalOf(() => engine.insert('acct', { name: 'bad', zzz_nonexistent_field: 'x' } as any)); + + expect(writes).toHaveLength(0); + }); + + it('refuses in the ADR-0112 envelope, with the wire answer unchanged', async () => { + const { engine } = await makeEngine(); + + const refusal = await refusalOf(() => engine.insert('acct', { name: 'bad', zzz_nonexistent_field: 'x' } as any)); + + // `code` AND `status` — the envelope, not merely "it threw". + expect(refusal?.code).toBe('INVALID_FIELD'); + expect(refusal?.status).toBe(400); + expect(refusal?.field).toBe('zzz_nonexistent_field'); + expect(refusal?.object).toBe('acct'); + // Byte-identical to the message `mapDataError`'s driver-string branch + // produced before this door existed — the refusal moved, the answer did + // not. `@objectstack/rest` re-emits this verbatim as the 400 body's + // `error`, so a change here is a change to the public wire answer. + expect(refusal?.message).toBe("Unknown field 'zzz_nonexistent_field' on object 'acct'"); + }); + + it('names every undeclared key, not only the first', async () => { + const { engine } = await makeEngine(); + + const refusal = await refusalOf(() => engine.insert('acct', { + name: 'bad', zzz_one: 1, zzz_two: 2, + } as any)); + + expect(refusal?.field).toBe('zzz_one'); + expect(refusal?.fields).toEqual(['zzz_one', 'zzz_two']); + }); + + it('a key holding `undefined` is still an undeclared key', async () => { + // A spread of a partial object is the common way this arrives from code + // rather than from JSON, and a mistyped key is a mistyped key whatever it + // holds — the AI-authoring case this door is for. Declared fields holding + // `undefined` are untouched: only the NAME is judged here. + const { engine } = await makeEngine(); + + const refusal = await refusalOf(() => engine.insert('acct', { name: 'bad', zzz_typo: undefined } as any)); + + expect(refusal?.code).toBe('INVALID_FIELD'); + expect(refusal?.field).toBe('zzz_typo'); + }); + + it('a declared field whose physical column is missing still reaches the driver', async () => { + // The door's scope is the SCHEMA's field map, so schema drift — declared + // here, absent in the table — is invisible to it by construction and stays + // the driver's to refuse, exactly as before. `mapDataError`'s driver-string + // branch is what turns that into the caller's 400, and it is still needed. + const { engine, writes } = await makeEngine({ missingColumns: ['description'] }); + + const refusal = await refusalOf(() => engine.insert('acct', { name: 'ok', description: 'v' } as any)); + + expect(writes).toHaveLength(1); + expect(String(refusal?.message)).toContain('has no column named description'); + }); + + it('a batch refuses on the first bad row and writes none of it', async () => { + const { engine, writes, hookRuns } = await makeEngine(); + + const refusal = await refusalOf(() => engine.insert('acct', [ + { name: 'a' }, { name: 'b', zzz_nonexistent_field: 'x' }, { name: 'c' }, + ] as any)); + + expect(refusal?.code).toBe('INVALID_FIELD'); + expect(refusal?.status).toBe(400); + expect(writes).toHaveLength(0); + expect(hookRuns).toEqual([]); + }); + + it('insertMany culls the bad row and writes the good ones', async () => { + // Partial-success mode's whole contract is that one bad row costs the + // others nothing — and before this door an undeclared key defeated it + // completely: the row travelled to `bulkCreate` with the rest and the + // driver failed the WHOLE batch that partial mode exists to protect. + const { engine, writes, hookRuns } = await makeEngine(); + + const outcomes = await engine.insertMany('acct', [ + { name: 'a' }, { name: 'b', zzz_nonexistent_field: 'x' }, { name: 'c' }, + ] as any); + + expect(outcomes.map((o: any) => o.ok)).toEqual([true, false, true]); + expect((outcomes[1] as any).error?.code).toBe('INVALID_FIELD'); + expect((outcomes[1] as any).error?.status).toBe(400); + // The culled row ran no hook and consumed no sequence value: the two good + // rows took 0001 and 0002 with nothing skipped between them. + expect(hookRuns).toEqual(['a', 'c']); + expect(writes.map((w) => w.data.account_number)).toEqual(['0001', '0002']); + }); + + it('a registry-less host gets no verdict — the driver stays the backstop', async () => { + // Same discipline as the read path's doors: a door that cannot see the + // field map must not invent an opinion about it. + const { engine, writes } = await makeEngine({ registerObject: false, missingColumns: ['zzz_nonexistent_field'] }); + + const refusal = await refusalOf(() => engine.insert('acct', { name: 'x', zzz_nonexistent_field: 'x' } as any)); + + expect(writes).toHaveLength(1); + expect(String(refusal?.message)).toContain('has no column named zzz_nonexistent_field'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 98dd40fad9..0b817c26c3 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -112,6 +112,8 @@ import { isMissingTableError } from '@objectstack/metadata/errors'; // engine is the consumer-side tolerant parsing PD #12 forbids and precedent // #5841 retired. import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types'; +// [#8682] The write-path loggers' redaction — bound values never reach the log. +import { redactBoundStatement } from './driver-fault-redaction.js'; /** * Per-row outcome of {@link ObjectQL.insertMany} (framework#3172). One entry @@ -977,6 +979,115 @@ function assertProjectionHasNoDottedPaths( throw err; } +/** + * [#8682] The DECLARED-FIELD DOOR on the insert path: a key the object does not + * declare is refused by the SCHEMA, before anything is produced for a request + * that is about to be refused anyway. + * + * ## What ran before this door existed + * + * Nothing rejected an undeclared write key. The row travelled the whole insert + * path and was refused at the very end by the DRIVER — `table crm_account has + * no column named zzz_nonexistent_field` — which `mapDataError` then translated + * into the (correct, unchanged) `400 INVALID_FIELD` the caller sees. The client + * answer was never the defect; what happened behind it was. Measured on + * `origin/main` for one mistyped key: + * + * - an id, a normalized name, owner/creator resolution and the column + * defaults were all produced; + * - the app's `beforeInsert` hooks RAN, and their derived values reached the + * statement (`period_label = 'Q3 2026'`, computed for a request nobody + * would ever store); + * - an AUTONUMBER was issued and CONSUMED — the log-independent observable, + * and the one this door is pinned on: a valid create before the bad request + * numbered `ACC-000013`, the bad request took `ACC-000014` into the void, + * and the next valid create came back `ACC-000015`. A sequence gap is + * permanent and visible to end users on a document number. + * + * A hook is not a pure function — it writes rows, calls out, stamps ledgers — + * so "it ran and we threw the result away" is not a no-op, it is a side effect + * of a request the server had already decided to refuse. + * + * ## Why HERE and not one step later + * + * This runs as the first act inside the middleware body: after middleware (a + * middleware may legitimately rewrite `data`, and judging the caller's keys + * before it ran would refuse rows a sanitizing middleware had already fixed), + * and before `applyFieldDefaults` — hence before the defaults, the summary + * seeding, the hooks, the strips, validation, the autonumber and the statement. + * The same placement, and the same argument, as {@link assertWriteAllowed} and + * `enforceTransactionOrigin` one level up: a refusal this early costs nothing. + * Like those two it throws without logging — the caller is told exactly what is + * wrong, and a client typo is not a server fault to record at ERROR. + * + * ## The verdict is the SCHEMA's field map, and the wire answer is unchanged + * + * `INVALID_FIELD` + 400, and the message is byte-identical to the one + * `mapDataError`'s driver-string branch produced (`Unknown field 'x' on object + * 'y'`) — so this moves WHERE the refusal is decided without moving what the + * caller reads. One condition keeps one wire shape however it was noticed, the + * same rule the read path's dotted-projection and unmaterializable-sort doors + * record. The driver-string branch stays where it is: it still backstops schema + * drift (a DECLARED field whose physical column is missing), which this door by + * construction cannot see. + * + * ## Where this door deliberately has NO opinion + * + * A door that cannot see the field map must not invent an opinion about it — + * the read-path doors' rule, and it decides two cases here: + * + * - **`schema.fields` absent** (a registry-less host), and **`schema.fields` + * EMPTY**. An empty map is indistinguishable from an unpopulated one: a real + * registered object always carries at least its primary key, and the + * registry injects the tenancy and audit columns on top, so zero declared + * fields means the host did not fill the map in — not that the object + * forbids every key. Refusing everything on that reading would be a verdict + * made from an absence, which is what this clause exists to prevent. + * - **`id` / `created_at` / `updated_at`**, which are tolerated even when the + * map does not list them. This mirrors `find()` / `findOne()`, which add + * exactly these three to their known set for exactly this reason: they are + * provisioned by the platform rather than authored, so an object may legally + * omit them from its declaration while the physical table has them. Same + * three names, same rationale, so a key accepted by a read is not refused by + * a write. + * + * In every one of those cases the driver remains the backstop it has always + * been — nothing is widened, the refusal simply moves back to where it was. + * + * @returns one entry per row: an `Error` for a row carrying undeclared keys, + * `undefined` for a row that passes. Per row rather than throw-on-first + * because the partial-success path (`insertMany`) reports per row and must + * cull the bad rows instead of failing the batch around them. + */ +const PLATFORM_PROVISIONED_COLUMNS = ['id', 'created_at', 'updated_at'] as const; + +function undeclaredInsertFieldErrors( + object: string, + schema: { fields?: unknown } | undefined, + rows: readonly unknown[], +): Array { + const out: Array = new Array(rows.length); + const fields = schema?.fields; + if (!fields || typeof fields !== 'object' || Array.isArray(fields)) return out; + const declared = new Set(Object.keys(fields as Record)); + if (declared.size === 0) return out; + for (const provisioned of PLATFORM_PROVISIONED_COLUMNS) declared.add(provisioned); + for (let i = 0; i < rows.length; i++) { + const row = rows[i]; + if (!row || typeof row !== 'object' || Array.isArray(row)) continue; + const undeclared = Object.keys(row as Record).filter((key) => !declared.has(key)); + if (undeclared.length === 0) continue; + const err: any = new Error(`Unknown field '${undeclared[0]}' on object '${object}'`); + err.status = 400; + err.code = 'INVALID_FIELD'; + err.field = undeclared[0]; + err.fields = undeclared; + err.object = object; + out[i] = err as Error; + } + return out; +} + /** * Evaluate formula virtual fields against the raw rows a driver handed back — * the read path (`find` / `findOne`) and, since #5504, the write path's @@ -7959,6 +8070,23 @@ export class ObjectQL implements IObjectQLEngine { // untouched, hooks run after and may override. const nowSnap = new Date(); const isBatch = Array.isArray(opCtx.data); + // [#8682] The declared-field door — see `undeclaredInsertFieldErrors` for + // what used to run below it for a request that was already refused. + // FIRST, so nothing downstream (defaults, summary seeding, the hooks, the + // secret writes, validation, the autonumber) happens for a row the schema + // rejects. Non-partial callers get the refusal thrown here; the + // partial-success path (`insertMany`) carries it per row into `rowErrors` + // below, where the culled rows also skip the hooks and every producer. + const undeclaredPerRow = undeclaredInsertFieldErrors( + object, + this._registry.getObject(object) as { fields?: unknown } | undefined, + isBatch ? (opCtx.data as unknown[]) : [opCtx.data], + ); + const partialRowMode = isBatch && (options as any)?.__partialRowErrors === true; + if (!partialRowMode) { + const refusal = undeclaredPerRow.find((e) => e !== undefined); + if (refusal) throw refusal; + } // [#4441] The RAW caller payload per row — before `applyFieldDefaults` // resolves any `defaultValue` / `current_user` token and before the // beforeInsert hooks stamp `owner_id` / `organization_id` / @@ -8027,8 +8155,14 @@ export class ObjectQL implements IObjectQLEngine { ql: this, }), ); - for (const rowCtx of rowHookContexts) { - await this.triggerHooks('beforeInsert', rowCtx); + // [#8682] A row the declared-field door refused runs NO hook. In + // non-partial mode the throw above already returned, so this skip only + // ever fires for a culled row of a partial batch — where the whole point + // is that one bad row costs the others nothing, and a hook that stamps a + // ledger or calls out must not fire for a row that will never be written. + for (let i = 0; i < rowHookContexts.length; i++) { + if (undeclaredPerRow[i] !== undefined) continue; + await this.triggerHooks('beforeInsert', rowHookContexts[i]); } // Thread the open transaction (if any) into the driver-facing // options so that knex's `.transacting(trx)` is honoured. Without @@ -8058,9 +8192,16 @@ export class ObjectQL implements IObjectQLEngine { // whole-batch degradation that re-runs beforeInsert hooks on the good // rows. rowErrors[i] set = row i is dead; only live rows reach the // driver / afterInsert / summaries. - const partialMode = isBatch && (options as any)?.__partialRowErrors === true; + const partialMode = partialRowMode; const rowErrors: (unknown | undefined)[] = new Array(rows.length); + // [#8682] Rows the declared-field door refused are already dead on + // arrival — seeded BEFORE the credential loop below, which is the first + // pass with a real side effect (`encryptSecretFields` writes a + // `sys_secret` row), so a culled row cannot mint a secret for a write + // that will never happen. + for (let i = 0; i < rows.length; i++) rowErrors[i] = undeclaredPerRow[i]; for (let i = 0; i < rows.length; i++) { + if (rowErrors[i] !== undefined) continue; try { // [#8559] Both halves of the credential write door, per row: the // password refusal at its own seam, then the secret channel (which @@ -8378,7 +8519,14 @@ export class ObjectQL implements IObjectQLEngine { if (summaryFailures.length > 0) throw new SummaryRecomputeError(summaryFailures, written); return written; } catch (e) { - this.logger.error('Insert operation failed', e as Error, { object }); + // [#8682] Same message, same ERROR level, same `object` — only the + // driver's inlined statement and its bound values are cut, from BOTH + // `message` and `stack` (the logger serializes exactly those two, and + // the stack re-opened with the statement a second time). What the + // database itself said, including the failing column, is kept; the + // error rethrown below is untouched, so the caller's answer does not + // move. See `redactBoundStatement`. + this.logger.error('Insert operation failed', redactBoundStatement(e) as Error, { object }); throw e; } }); @@ -9441,7 +9589,12 @@ export class ObjectQL implements IObjectQLEngine { if (summaryFailures.length > 0) throw new SummaryRecomputeError(summaryFailures, hookContext.result); return hookContext.result; } catch (e) { - this.logger.error('Update operation failed', e as Error, { object }); + // [#8682] The insert logger's twin, and measured to carry the same + // exposure: an UPDATE statement inlines the caller's values in its + // `set` clause exactly as an INSERT does in its `values` list. Same + // redaction, same one argument — the message, the level and the + // `object` are unchanged. + this.logger.error('Update operation failed', redactBoundStatement(e) as Error, { object }); throw e; } }); @@ -10193,8 +10346,14 @@ export class ObjectQL implements IObjectQLEngine { // `deleteBehavior:'cascade'` remedy) would reach no channel at all, // and the server log of a zh-CN deployment would read in Chinese. An // error that carries no `developerMessage` logs exactly as before. + // [#8682] Same redaction as the insert/update loggers. A DELETE's + // statement carries the caller's values in its `where` clause — which + // is how a record is addressed, so it is the same class of payload. + // `developerMessage` is read off the ORIGINAL error: it is written by + // our own throw sites (#7307), never by a driver, so it carries no + // statement and the redaction has no opinion about it. const devDetail = (e as any)?.developerMessage; - this.logger.error('Delete operation failed', e as Error, { + this.logger.error('Delete operation failed', redactBoundStatement(e) as Error, { object, ...(typeof devDetail === 'string' && devDetail.length > 0 ? { developerMessage: devDetail } : {}), }); diff --git a/packages/objectql/src/protocol-recorded-by-null.test.ts b/packages/objectql/src/protocol-recorded-by-null.test.ts index 0f6acf66ff..27bfc776ee 100644 --- a/packages/objectql/src/protocol-recorded-by-null.test.ts +++ b/packages/objectql/src/protocol-recorded-by-null.test.ts @@ -42,6 +42,16 @@ const sysMetadataObject = { type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + // [#8682] Declared on the real `sys_metadata` + // (`metadata-core/src/objects/sys-metadata.object.ts`), where it is part + // of the row's uniqueness key `(type, name, organization_id, + // package_id)`, and written by `SysMetadataRepository` — but omitted by + // this minimal stub. Nothing noticed while an undeclared write key just + // travelled on to the driver; the declared-field door judges the payload + // against this map, so the omission now shows up as the fixture defect + // it always was. (`sys_metadata_history` genuinely carries none, so its + // sibling stub below is left alone.) + package_id: { name: 'package_id', label: 'Package', type: 'text' as const }, metadata: { name: 'metadata', label: 'Body', type: 'longtext' as const }, checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, state: { name: 'state', label: 'State', type: 'text' as const }, diff --git a/packages/objectql/src/protocol-registry-shadow.test.ts b/packages/objectql/src/protocol-registry-shadow.test.ts index 688190a7fe..e302e2a0e6 100644 --- a/packages/objectql/src/protocol-registry-shadow.test.ts +++ b/packages/objectql/src/protocol-registry-shadow.test.ts @@ -38,6 +38,16 @@ const sysMetadataObject = { type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + // [#8682] Declared on the real `sys_metadata` + // (`metadata-core/src/objects/sys-metadata.object.ts`), where it is part + // of the row's uniqueness key `(type, name, organization_id, + // package_id)`, and written by `SysMetadataRepository` — but omitted by + // this minimal stub. Nothing noticed while an undeclared write key just + // travelled on to the driver; the declared-field door judges the payload + // against this map, so the omission now shows up as the fixture defect + // it always was. (`sys_metadata_history` genuinely carries none, so its + // sibling stub below is left alone.) + package_id: { name: 'package_id', label: 'Package', type: 'text' as const }, metadata: { name: 'metadata', label: 'Body', type: 'longtext' as const }, checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, state: { name: 'state', label: 'State', type: 'text' as const }, diff --git a/packages/objectql/src/protocol-save-meta-repo-path-real-engine.test.ts b/packages/objectql/src/protocol-save-meta-repo-path-real-engine.test.ts index 2248fd4435..4a5f406083 100644 --- a/packages/objectql/src/protocol-save-meta-repo-path-real-engine.test.ts +++ b/packages/objectql/src/protocol-save-meta-repo-path-real-engine.test.ts @@ -22,6 +22,13 @@ const sysMetadataObject = { type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + // [#8682] The real `sys_metadata` carries this — it is part of the + // row's uniqueness key `(type, name, organization_id, package_id)` and + // `SysMetadataRepository` writes it — but this minimal stub had omitted + // it. Nothing noticed while an undeclared write key simply travelled to + // the driver; the declared-field door judges the payload against this + // map, so the omission now shows up as the fixture defect it always was. + package_id: { name: 'package_id', label: 'Package', type: 'text' as const }, metadata: { name: 'metadata', label: 'Body', type: 'longtext' as const }, checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, state: { name: 'state', label: 'State', type: 'text' as const }, diff --git a/packages/objectql/src/publish-meta-response-conformance.test.ts b/packages/objectql/src/publish-meta-response-conformance.test.ts index 3cfe11a590..9cbfd42cb4 100644 --- a/packages/objectql/src/publish-meta-response-conformance.test.ts +++ b/packages/objectql/src/publish-meta-response-conformance.test.ts @@ -43,6 +43,16 @@ const sysMetadataObject: ServiceObject = { type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + // [#8682] Declared on the real `sys_metadata` + // (`metadata-core/src/objects/sys-metadata.object.ts`), where it is part + // of the row's uniqueness key `(type, name, organization_id, + // package_id)`, and written by `SysMetadataRepository` — but omitted by + // this minimal stub. Nothing noticed while an undeclared write key just + // travelled on to the driver; the declared-field door judges the payload + // against this map, so the omission now shows up as the fixture defect + // it always was. (`sys_metadata_history` genuinely carries none, so its + // sibling stub below is left alone.) + package_id: { name: 'package_id', label: 'Package', type: 'text' as const }, metadata: { name: 'metadata', label: 'Body', type: 'textarea' as const }, checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, state: { name: 'state', label: 'State', type: 'text' as const }, diff --git a/packages/objectql/src/save-meta-response-conformance.test.ts b/packages/objectql/src/save-meta-response-conformance.test.ts index 0325e6510a..a17c48cc64 100644 --- a/packages/objectql/src/save-meta-response-conformance.test.ts +++ b/packages/objectql/src/save-meta-response-conformance.test.ts @@ -56,6 +56,16 @@ const sysMetadataObject: ServiceObject = { type: { name: 'type', label: 'Type', type: 'text' as const, required: true }, name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, organization_id: { name: 'organization_id', label: 'Org', type: 'text' as const }, + // [#8682] Declared on the real `sys_metadata` + // (`metadata-core/src/objects/sys-metadata.object.ts`), where it is part + // of the row's uniqueness key `(type, name, organization_id, + // package_id)`, and written by `SysMetadataRepository` — but omitted by + // this minimal stub. Nothing noticed while an undeclared write key just + // travelled on to the driver; the declared-field door judges the payload + // against this map, so the omission now shows up as the fixture defect + // it always was. (`sys_metadata_history` genuinely carries none, so its + // sibling stub below is left alone.) + package_id: { name: 'package_id', label: 'Package', type: 'text' as const }, metadata: { name: 'metadata', label: 'Body', type: 'textarea' as const }, checksum: { name: 'checksum', label: 'Checksum', type: 'text' as const, maxLength: 71 }, state: { name: 'state', label: 'State', type: 'text' as const }, From d99df41dc0229ea5b8cad3656b1a8fd095a75799 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 16:50:47 +0000 Subject: [PATCH 2/2] docs(changeset): frame #8682 on the security consequence, not tidiness Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- .changeset/undeclared-field-preflight.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .changeset/undeclared-field-preflight.md diff --git a/.changeset/undeclared-field-preflight.md b/.changeset/undeclared-field-preflight.md new file mode 100644 index 0000000000..3b1aaaf5ab --- /dev/null +++ b/.changeset/undeclared-field-preflight.md @@ -0,0 +1,11 @@ +--- +"@objectstack/objectql": patch +--- + +Refuse undeclared fields on insert at the schema, and keep bound values out of the write-path logs (#8682) + +**A single mistyped field name in a client request no longer writes an entire row's values to disk.** A driver-level write fault is logged by prefixing the fully bound SQL statement — values inlined — to the database's own message, and the logger serializes both `message` and `stack`, so the statement was written twice at ERROR level. Confirmed with planted canaries: the row's values landed in the log alongside the organization id and the acting user id. The insert, update and delete loggers now write the database's own diagnostic — which still names the failing column and the object — with the statement and its bound values cut from both fields. The level, the message and the entry itself are unchanged: a driver fault nobody can debug would be a worse outcome than one logged too loudly. + +**An undeclared field is now refused by the object's field map, before anything runs for a request that was already going to be refused.** Previously an unknown key was caught only at the very end, by the driver, after an id, an auto-number, a normalized name, owner/creator resolution, the column defaults and the app's `beforeInsert` hooks had all been produced for it. The auto-number was the durable damage: the refused request consumed a sequence value and left a permanent gap in a document number an end user reads. `insertMany` now culls such a row per row instead of letting it fail the whole batch. + +The client-facing answer is deliberately unchanged — the same `400 INVALID_FIELD`, with the same message and the same `field` / `object` — and the rethrown error is untouched, so only what reaches the log has moved. Objects whose field map is absent or empty get no verdict at all, and `id` / `created_at` / `updated_at` stay accepted even when a declaration omits them, matching what the read path already tolerates; in every one of those cases the driver remains the backstop it has always been.