diff --git a/.changeset/harden-update-refusal-order.md b/.changeset/harden-update-refusal-order.md new file mode 100644 index 00000000000..5216a6e2acf --- /dev/null +++ b/.changeset/harden-update-refusal-order.md @@ -0,0 +1,15 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +harden(data): an update by id answers the same refusal whether or not the id exists, as delete does + +Clause-②: no +Metadata-protocol: none +Skills-docs: none — no file under `skills/**` teaches what a by-id update answers for an id that names no record (a `git grep` of `skills/` for `RECORD_NOT_FOUND` names only the approvals record lock), so no skill line changes. The Update Record section of `content/docs/protocol/kernel/http-protocol.mdx` states the order. + +The by-id update doors now judge the update before they look the record up — the order the by-id delete door already has. + +- **What changes.** A caller the update is refused to — no edit permission on the object, or an external data source that refuses writes — now gets that refusal (`403 PERMISSION_DENIED`, `403 EXTERNAL_WRITE_FORBIDDEN`) for an id that names no record too, where it got `404 RECORD_NOT_FOUND`. This holds on `PATCH /api/v1/data/:object/:id`, on each row of `POST /api/v1/data/:object/updateMany` and of `POST /api/v1/data/:object/batch` with `operation: "update"`, and on every door that updates through them (the runtime dispatcher's `/data`, the `/mcp` HTTP tools, declarative update actions). +- **What does not change.** A caller who may update the object still gets `404 RECORD_NOT_FOUND` for an id that names no record, and for a record its row-level access hides. Successful updates, the `405` for an operation the object does not expose, optimistic concurrency (`If-Match` / `expectedVersion`), upsert and system-context writes answer as before. +- **Embedders calling the protocol directly.** `updateData`, `updateManyData` and `batchData`'s `update` operation no longer read the record before calling `engine.update`. They ask whether it exists only after the engine resolves a `null` readback, so an engine that resolves `null` for an id that names no record still gets `404 RECORD_NOT_FOUND`, and an update that moved its record out of the caller's row scope still answers `200` with `record: null`. diff --git a/content/docs/protocol/kernel/http-protocol.mdx b/content/docs/protocol/kernel/http-protocol.mdx index 9ec8cd7b0f4..08674db433d 100644 --- a/content/docs/protocol/kernel/http-protocol.mdx +++ b/content/docs/protocol/kernel/http-protocol.mdx @@ -713,6 +713,14 @@ field is left unchanged. > lacks edit permission on is *rejected* with `403 PermissionDeniedError`, not > stripped. +**Refusals come before the record lookup:** +The update is judged before the record is looked up, as a delete is. A caller who may +not update the object (no edit permission, or an external data source that refuses +writes) gets the same `403` whether or not `{record_id}` names a record, and so does +each row of `updateMany` and of `batch` with `operation: "update"`. A caller who may +update the object gets `404 RECORD_NOT_FOUND` for an id that names no record, and for +a record its row-level access hides. + ### Delete Record Delete a record by ID. diff --git a/packages/metadata-protocol/src/protocol-batch-data.ts b/packages/metadata-protocol/src/protocol-batch-data.ts index 2b8aca7e2f2..c8edfa78d25 100644 --- a/packages/metadata-protocol/src/protocol-batch-data.ts +++ b/packages/metadata-protocol/src/protocol-batch-data.ts @@ -311,15 +311,16 @@ export async function runBatchDataLoop( } case 'update': { if (!record.id) throw rowRequiredIdError('update'); - // [#5088] Same existence gate as `updateMany` and the - // single-record PATCH — a row naming no record must not - // enter the write pipeline, where #4770's stored ⊕ - // payload merge has no stored side and #4775 blames the - // hook for the resulting gap. - await this.assertRecordExists(object, record.id); // [#3455] Collect the engine's LEGAL write strips per row. const dropped: DroppedFieldsEvent[] = []; const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any)); + // [#5088] Same existence answer as `updateMany` and the + // single-record PATCH, and in the same place: AFTER the + // engine has judged the write (its refusals, then its + // not-found gate ahead of every hook), so a refused row + // reads the same whether or not its id names a record — + // see `assertRecordExists`. + if (updated == null) await this.assertRecordExists(object, record.id); omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; @@ -335,8 +336,8 @@ export async function runBatchDataLoop( // duplicate-keyed — an authorization/update // scenario reported as a key collision — or, on a // store without a unique id constraint, wrote a - // second row. Same probe as the update branch above - // and the single-record path (#4620): it asks the + // second row. The same `probeRecord` the update branch + // above asks after its write (#4620): it asks the // database a fact, and whether the caller may WRITE // the row it proves stays #1994's decision inside // `engine.update`. @@ -739,17 +740,19 @@ export async function runUpdateManyLoop( // handling — at best a 404 with `undefined` interpolated into // the message, at worst a where-clause with no id at all. if (!record.id) throw rowRequiredIdError('update'); - // [#5088] Third gap, the same shape: no existence gate. A row - // naming no record went straight into `engine.update`, so the - // hook pipeline ran over a payload-only record and the row came - // back `INTERNAL_ERROR` from #4775's condition abort — blaming - // the hook for a caller's stale id. Probe first, per row, so - // this face answers what the single-record PATCH answers. - await this.assertRecordExists(object, record.id); const dropped: DroppedFieldsEvent[] = []; const opts: any = { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (context !== undefined) opts.context = context; const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, opts)); + // [#5088] Third gap, the same shape: no existence answer. A row + // naming no record came back `INTERNAL_ERROR` from #4775's + // condition abort — blaming the hook for a caller's stale id. It + // answers what the single-record PATCH answers now, asked where + // that door asks it: AFTER the engine has judged the write (its + // refusals, then its not-found gate ahead of every hook), so a + // refused row reads the same whether or not its id names a + // record — see `assertRecordExists`. + if (updated == null) await this.assertRecordExists(object, record.id); omitInternalFieldsFromWriteResponse(updateManySchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; diff --git a/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts b/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts index 9ac2db42976..7eea1a14ca4 100644 --- a/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts +++ b/packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts @@ -28,6 +28,7 @@ import { describe, it, expect, vi } from 'vitest'; import { BatchOperationResultSchema, BatchUpdateResponseSchema } from '@objectstack/spec/api'; import { ObjectStackProtocolImplementation } from './protocol.js'; import { assertEngineUpdateDispatch, assertEngineDeleteDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { recordNotFoundError } from '@objectstack/core'; const SCHEMA = { name: 'invoice', fields: { title: { name: 'title', type: 'text' } } }; @@ -56,11 +57,13 @@ function makeStoreEngine() { rows.set(rec.id, rec); return rec; }), - update: vi.fn(async (_object: string, data: any, options?: any) => { + update: vi.fn(async (object: string, data: any, options?: any) => { assertEngineUpdateDispatch(data, options); const id = options?.where?.id; const current = rows.get(id); - if (!current) throw new Error(`no such record: ${id}`); + // ObjectQL's by-id not-found gate: the engine refuses a row that is + // not there itself, after the write is judged and before any hook. + if (!current) throw recordNotFoundError(object, id); if (data?.title === POISON) throw new Error('update exploded'); const next = { ...current, ...data }; rows.set(id, next); @@ -156,10 +159,10 @@ describe('batchData rows conform to BatchOperationResultSchema (#4793)', () => { expectConformantResponse(res, 2); expect(res.results[0].data).toBeUndefined(); // [#5088] Was an unclassified engine throw (`no such record`, rendered - // INTERNAL_ERROR) because the row reached `engine.update` at all. The - // update branch now runs the same existence probe as the single-record - // PATCH, so a missing id is refused BEFORE the write with the catalogued - // 404 — see `protocol.bulk-record-not-found.test.ts`. + // INTERNAL_ERROR). The engine now refuses a missing id itself with the + // catalogued 404 — after it has judged the write, which is why the + // update branch asks nothing about existence ahead of it — see + // `protocol.bulk-record-not-found.test.ts`. expect(res.results[1].errors[0]).toMatchObject({ code: 'RECORD_NOT_FOUND', httpStatus: 404 }); expect(res.results[1].errors[0].message).toMatch(/not found/); }); diff --git a/packages/metadata-protocol/src/protocol.bulk-record-not-found.test.ts b/packages/metadata-protocol/src/protocol.bulk-record-not-found.test.ts index 0bcc2a6be62..7fe242d7cee 100644 --- a/packages/metadata-protocol/src/protocol.bulk-record-not-found.test.ts +++ b/packages/metadata-protocol/src/protocol.bulk-record-not-found.test.ts @@ -22,17 +22,25 @@ * of them deleted. Verbatim the defect #4435's own body describes, still * live ten lines from the `deleteMany` loop that fixed it. * - * The pins that matter most here are the ones about what the engine was ASKED - * to do: a 404 in the response body is cheap to produce, but "the write - * pipeline never ran for that row" is the actual guarantee — it is what keeps - * hooks, automation and audit rows from firing for a record that does not - * exist. So the fake engine below reproduces the #4775 abort verbatim on any - * write it receives for an unknown id, and each test asserts both the row's - * code and that the write was never attempted. + * A 404 in the response body is cheap to produce; "the write pipeline never ran + * for that row" is the actual guarantee — it is what keeps hooks, automation + * and audit rows from firing for a record that does not exist. ObjectQL now + * holds it itself: its by-id update refuses an id that names no row with + * `recordNotFoundError`, AFTER the write has been judged and BEFORE any + * `before*` hook (`engine-write-not-found-gate.test.ts` pins that order at the + * engine). So these faces no longer probe ahead of the engine — an update by id + * answers the same refusal whether or not the id exists, as delete does — and + * the fake engine below answers the way ObjectQL does, in ObjectQL's order: + * + * 1. the write is judged (a caller it refuses is refused, whatever the id); + * 2. an id that names no row is `recordNotFoundError`; + * 3. only then does the pipeline run — recorded on `pipeline`, so each test + * asserts both the row's answer and that the pipeline never saw that row. */ import { describe, it, expect, vi } from 'vitest'; import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { recordNotFoundError } from '@objectstack/core'; import { ObjectStackProtocolImplementation } from './protocol.js'; const SCHEMA = { @@ -43,17 +51,16 @@ const SCHEMA = { }, }; -/** - * What the real pipeline answers today when a write reaches it for an id with - * no stored row — quoted from the issue's repro. Unclassified (no `code`, no - * `status`), so `toRowApiError` renders it `INTERNAL_ERROR`: exactly the - * misdirection the probe exists to prevent. - */ -const HOOK_ABORT = - "Hook 'showcase_audit_task_completion' could not evaluate its condition " + - '(runtime: No such key: done) — operation aborted. The condition reads ' + - "'done', which this object does not declare — fix the hook's condition, or " + - 'declare the field.'; +/** A caller the engine's write judgement refuses — an object-level grant it lacks. */ +const REFUSED = { userId: 'u_refused' }; + +/** The refusal the engine's middleware answers that caller with, before the row is looked up. */ +function writeRefusal(): Error { + return Object.assign(new Error('You do not have permission to perform this action.'), { + code: 'PERMISSION_DENIED', + status: 403, + }); +} /** * In-memory store with real snapshot/rollback transaction semantics — the @@ -67,13 +74,18 @@ function makeStoreEngine() { ['t3', { id: 't3', progress: 0, done: false }], ]); const handle = { id: 'trx-1' }; + /** What a hook, automation or audit row would observe: the rows the write pipeline ran for. */ + const pipeline = vi.fn((_id: string) => {}); - const update = vi.fn(async (_object: string, data: any, options?: any) => { + const update = vi.fn(async (object: string, data: any, options?: any) => { assertEngineUpdateDispatch(data, options); const id = options?.where?.id; - // The write pipeline, reached for an id that names no row: the hook - // condition evaluates against a payload-only record and #4775 aborts. - if (!rows.has(id)) throw new Error(HOOK_ABORT); + // ObjectQL's order: the write is judged first (the middleware chain)… + if (options?.context?.userId === REFUSED.userId) throw writeRefusal(); + // …then its by-id not-found gate, ahead of every `before*` hook… + if (!rows.has(id)) throw recordNotFoundError(object, id); + // …and only then the pipeline. + pipeline(id); const next = { ...rows.get(id), ...data }; rows.set(id, next); return next; @@ -81,6 +93,7 @@ function makeStoreEngine() { // Contract per #4435: `false` is the positive not-found value. const del = vi.fn(async (_object: string, options?: any) => { assertEngineDeleteDispatch(options); + if (options?.context?.userId === REFUSED.userId) throw writeRefusal(); const id = options?.where?.id; if (!rows.has(id)) return false; rows.delete(id); @@ -111,7 +124,7 @@ function makeStoreEngine() { } }), }; - return { engine, rows, update, del, findOne }; + return { engine, rows, update, del, findOne, pipeline }; } /** @@ -143,8 +156,11 @@ describe('[#5088] updateManyData — a row naming no record is RECORD_NOT_FOUND' expect(res.succeeded).toBe(0); expect(res.failed).toBe(1); // THE CRUX. A refused row must not fire hooks, automation or an audit - // row for a record that does not exist — so the engine is never asked. - expect(t.update).not.toHaveBeenCalled(); + // row for a record that does not exist — so the pipeline never ran for + // it: the engine judged the write and refused the row before it. + expect(t.pipeline).not.toHaveBeenCalled(); + // …and nothing was asked ahead of the engine. + expect(t.findOne).not.toHaveBeenCalled(); }); it('non-atomic: a stale id fails alone — the real rows in the same batch still land', async () => { @@ -168,7 +184,7 @@ describe('[#5088] updateManyData — a row naming no record is RECORD_NOT_FOUND' expect(res.results[2]).toMatchObject({ id: 't3', success: true }); expect(t.rows.get('t1')).toMatchObject({ progress: 1 }); expect(t.rows.get('t3')).toMatchObject({ progress: 3 }); - expect(t.update).toHaveBeenCalledTimes(2); + expect(t.pipeline.mock.calls.map((c) => c[0])).toEqual(['t1', 't3']); }); it('atomic: the stale row is the CAUSAL row — later rows NOT_ATTEMPTED, nothing written', async () => { @@ -193,7 +209,7 @@ describe('[#5088] updateManyData — a row naming no record is RECORD_NOT_FOUND' ]); expectRecordNotFoundRow(res.results[0], 'definitely_missing'); expect(t.rows.get('t2')).toMatchObject({ progress: 0 }); - expect(t.update).not.toHaveBeenCalled(); + expect(t.pipeline).not.toHaveBeenCalled(); }); it('atomic: a stale row AFTER a real one still rolls the real one back', async () => { @@ -236,9 +252,9 @@ describe('[#5088] updateManyData — a row naming no record is RECORD_NOT_FOUND' expect(res.results[0].data).toMatchObject({ id: 't1', progress: 1 }); // Context is still threaded to every write (#3455). expect(t.update.mock.calls.every((c: any[]) => c[2]?.context?.userId === 'u1')).toBe(true); - // …and the probe asks EXISTENCE, not visibility (#4435): system context, - // so a row the caller cannot READ is still the by-id write policy's call. - expect(t.findOne.mock.calls.every((c: any[]) => c[1]?.context?.isSystem === true)).toBe(true); + // …and nothing is read ahead of the engine: a row the caller cannot + // READ is still the by-id write policy's call, made inside the engine. + expect(t.findOne).not.toHaveBeenCalled(); }); }); @@ -262,8 +278,8 @@ describe('[#5088] batchData update — the same gate as updateMany', () => { expect(res.succeeded).toBe(1); expect(res.failed).toBe(1); expectRecordNotFoundRow(res.results[1], 'definitely_missing'); - expect(t.update).toHaveBeenCalledTimes(1); - expect(t.update.mock.calls[0][2].where.id).toBe('t1'); + expect(t.pipeline.mock.calls.map((c) => c[0])).toEqual(['t1']); + expect(t.findOne).not.toHaveBeenCalled(); }); it('atomic: the stale row is causal, the earlier write is rolled back', async () => { @@ -663,3 +679,63 @@ describe('[#5088] the three by-id write faces answer the SAME thing', () => { expect(batch.results[0].errors[0].code).toBe(single.code); }); }); + +describe('an update by id answers the same refusal whether or not the id exists, as delete does', () => { + /** The refusal as the caller sees it: code, status and sentence. */ + const refusalOf = async (run: () => Promise) => { + try { + await run(); + } catch (err: any) { + return { code: err?.code, status: err?.status, message: err?.message }; + } + throw new Error('expected a refusal, but the call resolved'); + }; + const rowRefusal = (res: any) => { + const { code, httpStatus, message } = res.results[0].errors[0]; + return { success: res.results[0].success, code, httpStatus, message }; + }; + + it('the single-record faces: PATCH and DELETE refuse a real and a missing id alike', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + const patch = (id: string) => () => p.updateData({ object: 'showcase_task', id, data: { progress: 1 }, context: REFUSED } as any); + const remove = (id: string) => () => p.deleteData({ object: 'showcase_task', id, context: REFUSED } as any); + + const patchReal = await refusalOf(patch('t1')); + expect(patchReal).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(await refusalOf(patch('definitely_missing'))).toEqual(patchReal); + const deleteReal = await refusalOf(remove('t1')); + expect(await refusalOf(remove('definitely_missing'))).toEqual(deleteReal); + // Existence was never asked: the engine refused the write first. + expect(t.findOne).not.toHaveBeenCalled(); + expect(t.pipeline).not.toHaveBeenCalled(); + expect(t.rows.get('t1')).toMatchObject({ progress: 0 }); + }); + + it('the bulk faces: updateMany and batchData give one per-row refusal for a real and a missing id', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + const many = (id: string) => p.updateManyData({ + object: 'showcase_task', records: [{ id, data: { progress: 1 } }], context: REFUSED, + } as any); + const batch = (id: string) => p.batchData({ + object: 'showcase_task', request: { operation: 'update', records: [{ id, data: { progress: 1 } }] }, context: REFUSED, + } as any); + + const manyReal = rowRefusal(await many('t1')); + expect(manyReal).toMatchObject({ success: false, code: 'PERMISSION_DENIED', httpStatus: 403 }); + expect(rowRefusal(await many('definitely_missing'))).toEqual(manyReal); + expect(rowRefusal(await batch('definitely_missing'))).toEqual(rowRefusal(await batch('t1'))); + expect(t.findOne).not.toHaveBeenCalled(); + expect(t.pipeline).not.toHaveBeenCalled(); + }); + + it('CONTROL: a caller the write is open to still gets 404 for the missing id on every face', async () => { + const t = makeStoreEngine(); + const p = new ObjectStackProtocolImplementation(t.engine); + const single = await refusalOf(() => p.updateData({ object: 'showcase_task', id: 'definitely_missing', data: { progress: 1 }, context: { userId: 'u1' } } as any)); + expect(single).toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + const many: any = await p.updateManyData({ object: 'showcase_task', records: [{ id: 'definitely_missing', data: { progress: 1 } }], context: { userId: 'u1' } } as any); + expectRecordNotFoundRow(many.results[0], 'definitely_missing'); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.record-not-found.test.ts b/packages/metadata-protocol/src/protocol.record-not-found.test.ts index ebbd50e6780..43822f850a9 100644 --- a/packages/metadata-protocol/src/protocol.record-not-found.test.ts +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -26,25 +26,44 @@ import { ObjectStackProtocolImplementation } from './protocol.js'; const SCHEMA = { name: 'task', fields: { title: { name: 'title', type: 'text' } } }; +/** The refusal a write the engine judges unauthorized answers — before the row is looked up. */ +function writeRefusal(): Error { + return Object.assign(new Error('You do not have permission to perform this action.'), { + code: 'PERMISSION_DENIED', + status: 403, + }); +} + /** * An engine whose `delete` honours the driver contract * (`IDataDriver.delete` — "True if deleted, false if not found") and whose * `findOne` answers from the same row set, so existence means one thing here. + * + * `refuse` names a caller whose writes the engine refuses, in ObjectQL's order: + * the write is judged before the row is looked up. `leavesScope` names rows a + * landed update moves out of the caller's row scope, so their readback is + * `null` while the row is still there. */ -function makeProtocol(rows: Record = {}) { +function makeProtocol( + rows: Record = {}, + opts: { refuse?: string; leavesScope?: ReadonlySet } = {}, +) { const store = new Map(Object.entries(rows)); - const findOne = vi.fn(async (_object: string, opts: any) => { assertEngineFindOnePredicate(_object, opts); return store.get(String(opts?.where?.id)) ?? null; }); - const update = vi.fn(async (_object: string, data: any, opts: any) => { - assertEngineUpdateDispatch(data, opts); - const id = String(opts?.where?.id); + const refused = (o: any) => opts.refuse !== undefined && o?.context?.userId === opts.refuse; + const findOne = vi.fn(async (_object: string, o: any) => { assertEngineFindOnePredicate(_object, o); return store.get(String(o?.where?.id)) ?? null; }); + const update = vi.fn(async (_object: string, data: any, o: any) => { + assertEngineUpdateDispatch(data, o); + if (refused(o)) throw writeRefusal(); + const id = String(o?.where?.id); if (!store.has(id)) return null; const next = { ...store.get(id), ...data }; store.set(id, next); - return next; + return opts.leavesScope?.has(id) ? null : next; }); - const del = vi.fn(async (_object: string, opts: any) => { - assertEngineDeleteDispatch(opts); - return store.delete(String(opts?.where?.id)); + const del = vi.fn(async (_object: string, o: any) => { + assertEngineDeleteDispatch(o); + if (refused(o)) throw writeRefusal(); + return store.delete(String(o?.where?.id)); }); const engine = { registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, @@ -71,14 +90,21 @@ async function expectRecordNotFound(run: () => Promise, id: string) { describe('[#4435] updateData refuses an id that names no row', () => { it('PATCH of a nonexistent id is 404 RECORD_NOT_FOUND, not 200 { record: null }', async () => { - const { p, update } = makeProtocol({ real: { id: 'real', title: 'x' } }); + // This engine RESOLVES a `null` readback for a row that is not there (the + // shape `IDataEngine.update` declares), rather than refusing it the way + // ObjectQL's own not-found gate does — so the 404 here is the protocol's. + const { p, update, findOne } = makeProtocol({ real: { id: 'real', title: 'x' } }); await expectRecordNotFound( () => p.updateData({ object: 'task', id: 'definitely_not_a_row', data: { title: 'y' } } as any), 'definitely_not_a_row', ); - // …and the engine was never asked to write. A refused PATCH must not fire - // hooks, automation or an audit row for a record that does not exist. - expect(update).not.toHaveBeenCalled(); + // The write is judged FIRST, and existence is asked only after the engine + // resolved — the order `deleteData` has. (That no hook, automation or + // audit row fires for a row that is not there is the engine's to hold: + // its by-id branch refuses it ahead of every `before*` hook.) + expect(update).toHaveBeenCalledTimes(1); + expect(findOne).toHaveBeenCalledTimes(1); + expect(update.mock.invocationCallOrder[0]).toBeLessThan(findOne.mock.invocationCallOrder[0]); }); it('the SAME id answers 404 on the read path — the two verbs agree now', async () => { @@ -89,37 +115,61 @@ describe('[#4435] updateData refuses an id that names no row', () => { ); }); - it('an existing record still updates and returns the row', async () => { - const { p, update } = makeProtocol({ real: { id: 'real', title: 'x' } }); + it('an existing record still updates and returns the row — and nothing is read ahead of the engine', async () => { + const { p, update, findOne } = makeProtocol({ real: { id: 'real', title: 'x' } }); const res: any = await p.updateData({ object: 'task', id: 'real', data: { title: 'y' } } as any); expect(update).toHaveBeenCalledTimes(1); expect(res).toMatchObject({ object: 'task', id: 'real', record: { title: 'y' } }); + expect(findOne).not.toHaveBeenCalled(); }); - it('the existence probe asks EXISTENCE, not the caller\'s visibility', async () => { - // Load-bearing, and the first cut of this fix got it backwards. Probing - // with the CALLER's context turns the existence gate into an authorization - // gate: a row the caller cannot read comes back null and the PATCH 404s. - // That would (a) move an RLS decision out of the write policy where #1994 - // put it, and (b) disarm `@proof: rls-by-id-write` — the dogfood fixture - // whose RED half asserts that a member who cannot READ a row but has no - // write policy still mutates it by id. A caller-scoped probe makes that - // proof go green, so the gate could no longer prove it can go red, and a - // future revert of #1994 would be masked by this probe. + it('the existence check asks EXISTENCE, not the caller\'s visibility — a write that moved its row out of scope keeps its 200', async () => { + // Load-bearing, and the first cut of this fix got it backwards. Asking + // with the CALLER's context turns the existence check into an + // authorization gate: a row the caller cannot read comes back null and the + // PATCH 404s. That would (a) move an RLS decision out of the write policy + // where #1994 put it, and (b) disarm `@proof: rls-by-id-write` — the + // dogfood fixture whose RED half asserts that a member who cannot READ a + // row but has no write policy still mutates it by id. // - // So the probe runs as system: "does this row exist", nothing more. - // Authorization stays inside engine.update, exactly where it was. - const { p, findOne } = makeProtocol({ real: { id: 'real' } }); - await p.updateData({ object: 'task', id: 'real', data: {}, context: { userId: 'u1' } } as any); + // So it is asked as the system: "does this row exist", nothing more — + // and only after the engine has judged and resolved the write, on its + // `null` readback. Here that readback is a write that LANDED and moved + // the row out of the caller's row scope, so the row exists and the PATCH + // keeps its success. + const { p, findOne, store } = makeProtocol({ real: { id: 'real', owner: 'u1' } }, { leavesScope: new Set(['real']) }); + const res: any = await p.updateData({ object: 'task', id: 'real', data: { owner: 'u2' }, context: { userId: 'u1' } } as any); + expect(res).toMatchObject({ object: 'task', id: 'real', record: null }); + expect(store.get('real')).toMatchObject({ owner: 'u2' }); + expect(findOne).toHaveBeenCalledTimes(1); expect(findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'real' }, context: { isSystem: true }, }); }); - it('a PATCH the caller may not see is still decided by RLS, not by the probe', async () => { - // The row exists, so the probe passes it through to the engine — where the - // write policy answers, as it always has. The probe must not pre-empt it. + it('a write the engine refuses answers the same refusal whether or not the id exists, as delete does', async () => { + // The engine judges the write before it asks whether the row exists, so a + // caller it refuses is refused alike for both ids — and this door asks + // nothing ahead of it. + const { p, findOne, store } = makeProtocol({ real: { id: 'real', title: 'x' } }, { refuse: 'u1' }); + const answer = (run: () => Promise) => run().then( + () => { throw new Error('expected a refusal, but the call resolved'); }, + (e: any) => ({ code: e.code, status: e.status, message: e.message }), + ); + const patchReal = await answer(() => p.updateData({ object: 'task', id: 'real', data: { title: 'y' }, context: { userId: 'u1' } } as any)); + const patchMissing = await answer(() => p.updateData({ object: 'task', id: 'definitely_not_a_row', data: { title: 'y' }, context: { userId: 'u1' } } as any)); + const deleteMissing = await answer(() => p.deleteData({ object: 'task', id: 'definitely_not_a_row', context: { userId: 'u1' } } as any)); + expect(patchReal).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(patchMissing).toEqual(patchReal); + expect(deleteMissing).toEqual(patchReal); + expect(findOne).not.toHaveBeenCalled(); + expect(store.get('real')).toEqual({ id: 'real', title: 'x' }); + }); + + it('a PATCH the caller may not see is still decided by RLS, not by an existence read', async () => { + // The write goes to the engine — where the write policy answers, as it + // always has. Nothing at this door pre-empts it. const { p, update } = makeProtocol({ hidden: { id: 'hidden' } }); await p.updateData({ object: 'task', id: 'hidden', data: { title: 'x' }, context: { userId: 'nobody' } } as any); expect(update).toHaveBeenCalledOnce(); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index d02ea60cd0e..78abbab6ef4 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -14336,35 +14336,29 @@ export class ObjectStackProtocolImplementation implements async updateData(request: { object: string, id: string, data: any, expectedVersion?: string, context?: any }) { this.assertObjectRegistered(request.object); // [#3770] - // [#4435] ONE probe serves both gates. + // [#4435] A PATCH of an id that names no row answers `404 + // RECORD_NOT_FOUND`, as `getData` does — never `200 { record: null }`, + // a SUCCESS payload a client had to null-check to learn its write + // never landed. // - // A PATCH of an id that names no row answered `200 { record: null }` — - // the caller had to null-check a SUCCESS payload to discover its write - // never landed, which is exactly what a client that PATCHes a - // concurrently deleted record does not do. `getData` has always - // answered 404 for the same id; the two verbs now agree. + // ⭐ The write is judged FIRST and existence after, the order + // `deleteData` below has always had. `engine.update` answers its + // refusals — the security middleware's object- and row-level verdicts, + // the external datasource's write guard — before its by-id branch asks + // whether the row exists (the engine's not-found gate, which runs + // inside the middleware chain and ahead of every `before*` hook). So an + // update by id answers the same refusal whether or not the id exists, + // as delete does, and a caller the write is open to still gets the 404. // - // Existence is asked BEFORE the write rather than read off what comes - // back: the engine's update returns the post-write READBACK, which is - // also `null` when the row still exists but the write moved it out of - // the caller's row scope (reassigning `owner_id` away from yourself - // under an owner-scoped RLS policy). Reading that as "not found" would - // answer 404 to a write that succeeded. So ask existence directly. + // ⛔ No existence probe in front of the engine: existence is asked only + // once the write has been authorized, on this door (below the write) + // and on the by-id bulk faces ({@link assertRecordExists}). // - // The probe asks EXISTENCE, not visibility — see `probeRecord` for why - // that distinction is load-bearing (it keeps this fix out of the RLS - // model and keeps the #1994 by-id-write proof able to go red). - // - // OCC already had to read the same row for its `updated_at`, so the two - // gates share this single read instead of issuing a probe each. Two - // round-trips per PATCH would have been a performance regression no - // gate reports, and the second read could even disagree with the first. - const current = await this.probeRecord(request.object, request.id); - if (!current) throw recordNotFoundError(request.object, request.id); - // 404 wins over 409 when both could apply: OCC has always declined to - // treat a missing record as a concurrency conflict, and "this record - // does not exist" is the more specific answer. - await this.assertVersionOf(request.object, request.id, current, request.expectedVersion, request.context); + // OCC is asked the way `deleteData` asks it — a probe only when the + // caller sent a token ({@link assertVersionMatch}) — and a row that + // probe does not find is left to the engine's answer: OCC has never + // treated a missing record as a concurrency conflict. + await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context); const opts: any = { where: { id: request.id } }; if (request.context !== undefined) opts.context = request.context; // [#3407/#3431] Capture the engine's LEGAL write strips (static `readonly` @@ -14408,8 +14402,8 @@ export class ObjectStackProtocolImplementation implements // this caller has already named the row twice — in the URL and in `where` — // and the three gates around this line all judge THAT row: // - // probe → `probeRecord(object, request.id)` (existence, #4435) - // OCC → `assertVersionOf(…, request.id, …)` (If-Match / expectedVersion) + // OCC → `assertVersionMatch(…, request.id, …)` (If-Match / expectedVersion) + // engine → the by-id write, its refusals, then its not-found gate // receipt → `{ id: request.id, record: result }` // // Passing `request.data` verbatim let a body `{"id":"rec_2"}` on @@ -14446,6 +14440,18 @@ export class ObjectStackProtocolImplementation implements ? { ...(request.data as Record), id: request.id } : request.data; const result = byIdUpdateRecord(await this.engine.update(request.object, writeData, opts)); + // The engine resolved, so the write was authorized. A `null` readback + // is two answers by contract (`IDataEngine.update`): the row is not + // there, or the write moved it out of the caller's row scope + // (reassigning `owner_id` away from yourself under an owner-scoped + // policy) and LANDED — answering 404 to that would report a write that + // succeeded as one that never happened. Existence tells them apart, + // asked now, after authorization, through the system-context + // {@link probeRecord}. ObjectQL refuses a row that is not there + // itself, so this reads only on a `null` readback. + if (result == null && !(await this.probeRecord(request.object, request.id))) { + throw recordNotFoundError(request.object, request.id); + } // [#7823] The PATCH 200 body is the surface #7728's fourth measurement // caught: a client revoking a `sys_api_key` (apiMethods keeps `update` // open, #7727) got the stored hash back in this response. That closure @@ -14569,17 +14575,36 @@ export class ObjectStackProtocolImplementation implements * where it was. The one behaviour this adds is the 404 the issue asked for: * an id that names no row at all. * - * One place, because two gates need this row — the existence gate and OCC's - * `updated_at` comparison — and issuing a probe each would put two - * round-trips on every PATCH. + * ⛔ And it is never asked IN FRONT of the write's authorization. Its + * readers are OCC's `updated_at` comparison, asked only when the caller sent + * a token ({@link assertVersionMatch}), and the by-id update's existence + * check, asked only after `engine.update` has judged and resolved the write + * (`updateData`, and the bulk faces through {@link assertRecordExists}) — + * so an update by id answers the same refusal whether or not the id + * exists, as delete does. */ private async probeRecord(object: string, id: string): Promise { return this.engine.findOne(object, { where: { id }, context: { isSystem: true } } as any); } /** - * [#5088] The same existence gate {@link updateData} runs, for the BY-ID - * BULK write faces — `updateManyData` and `batchData`'s `update` branch. + * [#5088] The by-id update's existence answer for the BY-ID BULK write + * faces (`updateManyData` and `batchData`'s `update` branch), one per row — + * the same check {@link updateData} makes inline below its write. + * + * ⭐ Asked AFTER `engine.update` has judged the write, and only when it + * resolved a `null` readback — never in front of the engine. The engine + * answers its refusals (object- and row-level authorization, the external + * datasource's write guard) and then refuses a row that is not there + * itself, ahead of every `before*` hook, so a write the caller may not make + * is refused whether or not the id names a row, as a delete is. What is + * left for this check is the readback `IDataEngine.update` declares as two + * answers: a row that is not there (an engine that resolves rather than + * refuses), or a write that LANDED and moved the row out of the caller's + * row scope — which this tells apart by asking existence, so the second + * keeps its success. + * + * The history below is why the bulk faces carry the check at all. * * #4435's "a write that touched zero rows must not report success" landed on * 2 of the 5 write faces in this file: `updateData` (probe) and @@ -14594,14 +14619,13 @@ export class ObjectStackProtocolImplementation implements * undeclared field. Three contracts disagreeing because one of them never * ran; the probe is what makes them agree again. * - * Deliberately the same `probeRecord` the single-record path uses, for the + * Deliberately the same `probeRecord` the OCC comparison uses, for the * reason documented there: it asks EXISTENCE, not visibility, which keeps - * this gate out of the RLS model (the by-id write policy stays #1994's + * this check out of the RLS model (the by-id write policy stays #1994's * decision, inside `engine.update`) and keeps the `rls-by-id-write` proof - * able to go red. And deliberately BEFORE the write, never inferred from a - * null readback — `updateData`'s note explains why that inference would - * answer 404 to a write that succeeded by moving the row out of the - * caller's scope. + * able to go red. And never inferred from the `null` readback alone — + * `updateData`'s note explains why that inference would answer 404 to a + * write that succeeded by moving the row out of the caller's scope. * * Inside the atomic arm this still reads the batch's own uncommitted state: * `engine.transaction` runs its callback inside the ambient `txStore` @@ -14618,11 +14642,10 @@ export class ObjectStackProtocolImplementation implements } /** - * Optimistic Concurrency Control — the COMPARISON half, over the row the - * existence probe has already read. A token that agrees (or no token) - * issues no query of its own, which is what lets `updateData` run the gate - * on its existence probe's result rather than re-reading the record - * (#4435). Only a MISMATCH reads again — see the conflict paragraph below. + * Optimistic Concurrency Control — the COMPARISON half, over the row + * {@link assertVersionMatch}'s probe has already read. A token that agrees + * (or no token) issues no query of its own. Only a MISMATCH reads again — + * see the conflict paragraph below. * * When the caller passes a non-empty `expectedVersion` token (typically the * `updated_at` value they read), a mismatch throws `ConcurrentUpdateError`, @@ -14639,8 +14662,8 @@ export class ObjectStackProtocolImplementation implements * guard. Every other falsy shape (no header/field at all, `''`, * whitespace-only) is untouched by this and still opts out. * - Record not found → no check. We intentionally do not treat "missing - * record" as a concurrency conflict; `updateData` has already answered - * 404 by this point, and `deleteData` lets the driver report it. + * record" as a concurrency conflict; both `updateData` and `deleteData` + * leave it to the engine, which judges the write first. * - Record has no `updated_at` field (timestamps disabled) → no check. * Logging would be noisy here; OCC is opt-in and the absence of a * version column is an explicit "this object doesn't support OCC" @@ -14689,10 +14712,12 @@ export class ObjectStackProtocolImplementation implements } /** - * OCC gate for `deleteData`, which — unlike `updateData` — needs no - * existence probe of its own: the driver's own `delete` return reports - * whether a row matched (#4435). So this still probes ONLY when the caller - * actually opted into OCC, keeping a plain DELETE at zero extra reads. + * OCC gate for the two single-record by-id writes, `updateData` and + * `deleteData`. Neither asks existence ahead of the engine — the engine + * judges the write and then refuses a row that is not there (#4435, and + * `deleteData` reads the driver's `false` besides) — so this probes ONLY + * when the caller actually opted into OCC, keeping a plain PATCH or DELETE + * at zero extra reads. * * [#13576] The malformed-token check runs FIRST, before the probe — a * malformed client token is a request defect independent of whether the diff --git a/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts b/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts index b5195cea531..44e536ae6df 100644 --- a/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts +++ b/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts @@ -25,8 +25,13 @@ * asserted below is deliberately not "the fix line is present" but the * ingress-level fact a future fork would break: * - * **the probed row, the OCC-checked row, the written row and the receipt's - * `id`/`record` are the SAME row — the PATH row.** + * **the OCC-checked row, the written row (and the row a not-found answer + * names) and the receipt's `id`/`record` are the SAME row — the PATH row.** + * + * There is no existence probe AHEAD of the write any more: an update by id is + * judged first and its row's existence asked after, as a delete's is, so the + * "probed row" of the history above is now the row the post-write existence + * check reads — the same PATH row. * * ## What is deliberately NOT re-litigated here * @@ -147,7 +152,8 @@ describe('[#6479] the PATH row is the row — probe, OCC, write and receipt agre const { p, findOne, update, store } = makeProtocol(TWO_ROWS); const res = await p.updateData({ object: 'task', id: 'rec_1', data: { id: 'rec_1', title: 'x' } }); - expect(findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'rec_1' } }); + // No token and a row that is there: nothing is read beside the write. + expect(findOne).not.toHaveBeenCalled(); expect(boundRowOfCall(update)).toBe('rec_1'); expect(res).toMatchObject({ object: 'task', id: 'rec_1', record: { id: 'rec_1', title: 'x' } }); expect(store.get('rec_1')).toMatchObject({ id: 'rec_1', title: 'x' }); @@ -158,8 +164,8 @@ describe('[#6479] the PATH row is the row — probe, OCC, write and receipt agre const { p, findOne, update, store } = makeProtocol(TWO_ROWS); const res = await p.updateData({ object: 'task', id: 'rec_1', data: { id: 'rec_2', title: 'x' } }); - // The four gates, one row. - expect(findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'rec_1' } }); + // The gates, one row. + expect(findOne).not.toHaveBeenCalled(); expect(boundRowOfCall(update)).toBe('rec_1'); expect(res.id).toBe('rec_1'); expect(res.record).toMatchObject({ id: 'rec_1', title: 'x' }); @@ -189,10 +195,12 @@ describe('[#6479] the PATH row is the row — probe, OCC, write and receipt agre }); it('(d1) a path id that names no row is still 404, and the body id cannot rescue it', async () => { - // Unchanged by this fix and pinned as such: the probe has always asked the - // PATH id (#4435). What it adds here is that an existing row named by the - // BODY does not make the call succeed — and is not written. - const { p, update, store } = makeProtocol(TWO_ROWS); + // Unchanged by this fix and pinned as such: existence is asked of the + // PATH id (#4435) — now after the engine has judged the write, which + // this double resolves with a `null` readback for the PATH row it + // bound. What it adds here is that an existing row named by the BODY + // does not make the call succeed — and is not written. + const { p, findOne, update, store } = makeProtocol(TWO_ROWS); let caught: any; try { await p.updateData({ object: 'task', id: 'ghost', data: { id: 'rec_2', title: 'x' } }); @@ -203,7 +211,8 @@ describe('[#6479] the PATH row is the row — probe, OCC, write and receipt agre expect(caught.code).toBe('RECORD_NOT_FOUND'); expect(caught.status).toBe(404); expect(caught.message).toContain('ghost'); - expect(update).not.toHaveBeenCalled(); + expect(boundRowOfCall(update)).toBe('ghost'); + expect(findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'ghost' } }); expect(store.get('rec_2')).toEqual(TWO_ROWS[1]); }); }); diff --git a/packages/objectql/src/engine-write-not-found-gate.test.ts b/packages/objectql/src/engine-write-not-found-gate.test.ts index a22e50c705a..29f32cd08ef 100644 --- a/packages/objectql/src/engine-write-not-found-gate.test.ts +++ b/packages/objectql/src/engine-write-not-found-gate.test.ts @@ -52,8 +52,10 @@ * * ## Two sibling paths already had this gate; this is the third, not a fourth * - * - `protocol.updateData`/`deleteData` probe existence and throw - * `recordNotFoundError` (#4435) + * - `protocol.updateData`/`deleteData` throw `recordNotFoundError` (#4435) + * — no longer by probing ahead of the engine: both leave the answer to + * this gate, which runs after the write is judged (section 5 below), and + * read existence themselves only off the engine's own reply * - `callData`'s ObjectQL fallback does the same (#5138) * * All three now call the SAME `recordNotFoundError` — moved to @@ -436,3 +438,47 @@ describe('[#7867] the engine answers with the SAME error the protocol and callDa .toEqual({ code: b.code, status: b.status, object: b.object, message: b.message }); }); }); + +/* ──────────────────────────────────────────────────────────────────────────── + * 5. The write is judged BEFORE the gate asks whether the row exists + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('a by-id write the middleware chain refuses gets that refusal whether or not the id exists', () => { + // The gate sits inside the middleware chain's `next()`, so a refusal the + // chain answers — the security middleware's object-level verdict is the one + // that matters — is answered for an id that names no row exactly as for one + // that does. The data doors rely on this order: they ask nothing about + // existence ahead of the engine, so an update by id answers the same refusal + // whether or not the id exists, as delete does. + for (const verb of ['update', 'delete'] as const) { + it(`${verb}: one refusal for a real id and for a missing one, and nothing reaches the driver`, async () => { + const { engine, calls } = await bootEngine([]); + const row: any = await engine.insert('nf_task', { title: 'Ship it', done: false }); + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + if ((opCtx.operation === 'update' || opCtx.operation === 'delete') && opCtx.context?.userId === 'u_refused') { + throw Object.assign(new Error('You do not have permission to perform this action.'), { + code: 'PERMISSION_DENIED', + status: 403, + }); + } + await next(); + }); + const context = { userId: 'u_refused' }; + const run = (id: string) => + (verb === 'update' + ? engine.update('nf_task', { id, done: true }, { context } as any) + : engine.delete('nf_task', { where: { id }, context } as any) + ).then( + () => null, + (e: any) => ({ code: e?.code, status: e?.status, message: e?.message }), + ); + + const real = await run(row.id); + const ghost = await run('ghost'); + + expect(real).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(ghost).toEqual(real); + expect(calls.update + calls.delete).toBe(0); + }); + } +}); diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index 5c5de5606d2..cf4b7311635 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ObjectStackProtocolImplementation, recordNotFoundError } from '@objectstack/metadata-protocol'; /** * Tests for the Protocol Implementation's data methods (findData, getData). @@ -406,13 +406,12 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { describe('Optimistic Concurrency Control', () => { beforeEach(() => { // Both update and delete need `update` / `delete` on the engine, - // plus `findOne` for the record probe. + // plus `findOne` for the OCC probe. // - // [#4435] The record now has to EXIST for a PATCH to proceed at - // all: `updateData` refuses an id that names no row instead of - // answering `200 { record: null }`. The default probe result is - // therefore a real row — these cases are about OCC, not about - // missing records (which `updateData refuses an id …` covers). + // [#4435] A PATCH of an id that names no row answers 404, never + // `200 { record: null }` — answered after the write is judged, as a + // delete's is (`updateData refuses an id …` below covers it). The + // default probe result is a real row: these cases are about OCC. mockEngine.findOne.mockResolvedValue({ id: 'r1', updated_at: '2026-05-22T07:14:00.000Z' }); mockEngine.update = vi.fn().mockResolvedValue({ id: 'r1', updated_at: '2026-05-22T07:14:33.000Z' }); mockEngine.delete = vi.fn().mockResolvedValue(true); @@ -420,11 +419,13 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { it('updateData proceeds when no expectedVersion is supplied (legacy callers)', async () => { await protocol.updateData({ object: 'task', id: 'r1', data: { name: 'New' } }); - // [#4435] One probe — the EXISTENCE probe, which every PATCH now - // makes. No OCC comparison happens (no token was supplied), which - // is what "legacy callers are unaffected" means: they are not - // subject to 409, only to the 404 that GET already answered. - expect(mockEngine.findOne).toHaveBeenCalledOnce(); + // No token, so no OCC comparison — which is what "legacy callers + // are unaffected" means: they are not subject to 409. And no read + // ahead of the engine either: an update by id is judged before its + // row is looked up, as a delete is, so a plain PATCH whose write + // lands reads nothing beside it (`deleteData without a token issues + // NO probe at all` below is the twin). + expect(mockEngine.findOne).not.toHaveBeenCalled(); expect(mockEngine.update).toHaveBeenCalledOnce(); }); @@ -436,20 +437,23 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { data: { name: 'New' }, expectedVersion: '2026-05-22T07:14:00.000Z', }); - // [#4435] Still exactly ONE read. The existence gate and OCC both - // need this row, so they share the probe rather than issuing one - // each — two round-trips per PATCH would be a performance - // regression no gate reports, and the second read could disagree - // with the first. + // Exactly ONE read: OCC's probe, asked because a token was sent, as + // `deleteData` asks it. The write landed (a non-null readback), so + // nothing else is read. expect(mockEngine.findOne).toHaveBeenCalledOnce(); + expect(mockEngine.findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'r1' }, context: { isSystem: true } }); expect(mockEngine.update).toHaveBeenCalledOnce(); }); - it('updateData refuses an id that names no row, before any OCC verdict', async () => { + it('updateData refuses an id that names no row, before any OCC verdict — the engine\'s not-found gate', async () => { // [#4435] 404 wins over 409 when both could apply: OCC has always // declined to treat a missing record as a concurrency conflict, and - // "this record does not exist" is the more specific answer. + // "this record does not exist" is the more specific answer. OCC's + // probe finds no row and leaves it to the engine, which — as + // ObjectQL does — judges the write and then refuses the row that is + // not there with the shared envelope. mockEngine.findOne.mockResolvedValue(null); + mockEngine.update = vi.fn().mockRejectedValue(recordNotFoundError('task', 'gone')); await expect( protocol.updateData({ object: 'task', @@ -458,7 +462,36 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { expectedVersion: '2026-05-22T07:14:00.000Z', }) ).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); - expect(mockEngine.update).not.toHaveBeenCalled(); + expect(mockEngine.update).toHaveBeenCalledOnce(); + // The one read is OCC's probe, as the system; it found nothing, so + // no 409 was answered and nothing else was read. + expect(mockEngine.findOne).toHaveBeenCalledOnce(); + expect(mockEngine.findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'gone' }, context: { isSystem: true } }); + }); + + it('updateData refuses an id that names no row — an engine that resolves a null readback', async () => { + // `IDataEngine.update` declares `null` as a readback, so an engine + // may RESOLVE for a row that is not there. The protocol then asks + // existence itself, as the system, after the engine has judged the + // write: no row ⇒ the same 404, and still no 409 from the token. + mockEngine.findOne.mockResolvedValue(null); + mockEngine.update = vi.fn().mockResolvedValue(null); + await expect( + protocol.updateData({ + object: 'task', + id: 'gone', + data: { name: 'New' }, + expectedVersion: '2026-05-22T07:14:00.000Z', + }) + ).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(mockEngine.update).toHaveBeenCalledOnce(); + // OCC's probe, then the existence check after the write — both as + // the system, the second strictly after the engine was asked. + expect(mockEngine.findOne).toHaveBeenCalledTimes(2); + for (const call of mockEngine.findOne.mock.calls) { + expect(call[1]).toMatchObject({ where: { id: 'gone' }, context: { isSystem: true } }); + } + expect(mockEngine.update.mock.invocationCallOrder[0]).toBeLessThan(mockEngine.findOne.mock.invocationCallOrder[1]); }); it('updateData strips RFC-7232 quotes from the If-Match token', async () => { @@ -508,15 +541,16 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { }); it('updateData skips the check when expectedVersion is empty string', async () => { - // A blank token opts OUT of OCC — the record still has to exist - // (#4435), so the probe happens; what must NOT happen is a 409. + // A blank token opts OUT of OCC — no 409, and, like a PATCH with no + // token, no read ahead of the engine: the record's existence is the + // engine's to answer, after it has judged the write (#4435). await protocol.updateData({ object: 'task', id: 'r1', data: { name: 'New' }, expectedVersion: ' ', }); - expect(mockEngine.findOne).toHaveBeenCalledOnce(); + expect(mockEngine.findOne).not.toHaveBeenCalled(); expect(mockEngine.update).toHaveBeenCalledOnce(); }); diff --git a/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts b/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts index 288656acaf2..dcf25665db1 100644 --- a/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts @@ -580,8 +580,15 @@ describe('showcase: anonymous posture is uniform across surfaces (#2567)', () => // object, one process, one second — and two surfaces that disagreed // (`/actions` → 400, `/data` → 404). Both are asserted here, so they cannot // drift apart again without this gate saying so. + // + // The data surface that answers this the way the action does is the record + // READ: the action loads its record through the read door, and the member + // reads `showcase_task`. A PATCH is not that surface for this caller: an + // update by id is judged before the record is looked up, as a delete is, and + // the member holds no edit grant on `showcase_task` — so it is refused the + // write whatever the id (`update-by-id-refusal-order.dogfood.test.ts`). const ghostId = ACTION.slice(ACTION.lastIndexOf('/') + 1); - const rest = await stack.apiAs(memberToken, 'PATCH', `/data/showcase_task/${ghostId}`, { done: true }); + const rest = await stack.apiAs(memberToken, 'GET', `/data/showcase_task/${ghostId}`); expect(rest.status).toBe(404); const body = (await rest.json()) as Record; @@ -590,6 +597,10 @@ describe('showcase: anonymous posture is uniform across surfaces (#2567)', () => // in its own shape, with no `??` chain across the two (#5632's rule, which // this file already enforces for the 401 bodies). expect(body.code).toBe('RECORD_NOT_FOUND'); + + const patch = await stack.apiAs(memberToken, 'PATCH', `/data/showcase_task/${ghostId}`, { done: true }); + expect(patch.status).toBe(403); + expect(((await patch.json()) as Record).code).toBe('PERMISSION_DENIED'); }); // ── /automation (dispatcher-mounted; runtime domains/automation.ts) ───── diff --git a/packages/qa/dogfood/test/update-by-id-refusal-order.dogfood.test.ts b/packages/qa/dogfood/test/update-by-id-refusal-order.dogfood.test.ts new file mode 100644 index 00000000000..d0dda6dc79e --- /dev/null +++ b/packages/qa/dogfood/test/update-by-id-refusal-order.dogfood.test.ts @@ -0,0 +1,233 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// An update by id answers the same refusal whether or not the id exists, as +// delete does — measured at the HTTP data door of the stock showcase. +// +// ── what is pinned ───────────────────────────────────────────────────────── +// +// For a plain member (the everyone baseline only) on three object families +// the member may not update — a business junction object +// (`showcase_project_membership`), a platform grant object +// (`sys_user_permission_set`) and the two read-only federated objects +// (`showcase_ext_customer`, `showcase_ext_order`): +// +// 1. ⭐ `PATCH /data/:object/:id` answers the SAME status and body for an id +// that names a row and for one that names none — the refusal the write +// gets, judged before the row is looked up; +// 2. the bulk update doors (`updateMany`, `batch` with `operation: update`) +// answer the same per-row refusal for both ids; +// 3. `DELETE` keeps the one answer it already gave for both; +// 4. with a stale `If-Match`, PATCH and DELETE keep one status and code for +// both ids. +// +// Controls, so a green run is not "everything refuses": +// +// - a member holding the object's edit grant (`showcase_ops`) updates an +// in-scope row (200) and still gets `404 RECORD_NOT_FOUND` for an id that +// names no row; +// - a member holding edit with a row-level restriction +// (`showcase_private_note`, private) gets one not-found answer for a row +// out of its scope and for an id that names no row — the row-scope answer, +// unchanged — and still updates its own row; +// - the administrator updates (200) and gets `404` for an id that names no +// row on an ordinary object; on a read-only federated object it gets the +// write guard's refusal for both ids, as its DELETE does; +// - a refused write leaves the row untouched. +// +// The engine-layer half of the order — the middleware chain's refusal comes +// before the by-id not-found gate — is pinned in +// `packages/objectql/src/engine-write-not-found-gate.test.ts`, and the data +// door's half in `packages/metadata-protocol/src/protocol.record-not-found.test.ts` +// and `protocol.bulk-record-not-found.test.ts`. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { onEnable } from '@objectstack/example-showcase'; +import { type VerifyStack } from '@objectstack/verify'; +import { bootShowcase } from './showcase-boot.js'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; +import { catalogPermissionSet } from './catalog-permission-set.js'; + +const SYS = { isSystem: true } as const; +const MISSING = 'zz_no_such_row_0001'; +const MEMBER = 'upd-order-member@verify.test'; +const EDITOR = 'upd-order-editor@verify.test'; + +interface Answer { status: number; body: unknown } + +/** The `code` of a flat (`{ code }`) or nested (`{ error: { code } }`) envelope. */ +const codeOf = (a: Answer): unknown => (a.body as any)?.code ?? (a.body as any)?.error?.code; + +/** The answer with the caller-supplied id written out, so the answers for two ids compare. */ +const withoutId = (a: Answer, id: string): Answer => + JSON.parse(JSON.stringify(a).split(id).join('ID')) as Answer; + +/** The id a create answered, wherever the envelope carries it. */ +const createdId = (a: Answer): string => { + const b = a.body as any; + return String(b?.id ?? b?.record?.id ?? b?.data?.id ?? ''); +}; + +/** The update payload each family is driven with — a value the object accepts. */ +const PATCHES: Record> = { + showcase_project_membership: { engagement: 'reviewer' }, + sys_user_permission_set: { reason: 'x' }, + showcase_ext_customer: { region: 'EU' }, + showcase_ext_order: { status: 'void' }, +}; + +describe('update by id: one refusal whether or not the id exists, as delete', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string; + let memberTok: string; + let editorTok: string; + const existing = new Map(); + let foreignNoteId = ''; + let ownNoteId = ''; + + const call = async ( + token: string, + method: string, + path: string, + body?: unknown, + headers: Record = {}, + ): Promise => { + const res = await stack.api(path, { + method, + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}`, ...headers }, + ...(body !== undefined ? { body: JSON.stringify(body) } : {}), + }); + let parsed: unknown; + try { parsed = await res.json(); } catch { parsed = undefined; } + return { status: res.status, body: parsed }; + }; + + const stored = (object: string, id: string) => ql.findOne(object, { where: { id }, context: SYS }); + + beforeAll(async () => { + // The federated objects' remote tables — the showcase's own fixture + // provisioner, run the way `showcase-external-autoconnect` runs it. + await onEnable({ logger: { info() {}, warn() {} } } as never); + stack = await bootShowcase({ security: showcaseAppDefaultSecurity() }); + adminTok = await stack.signIn(); + ql = await stack.kernel.getServiceAsync('objectql'); + + memberTok = await stack.signUp(MEMBER); + editorTok = await stack.signUp(EDITOR); + const ops = await catalogPermissionSet(ql, 'showcase_ops'); + expect(ops, 'showcase_ops served by the security catalog').toBeTruthy(); + const editorId = (await ql.findOne('sys_user', { where: { email: EDITOR }, context: SYS }))?.id; + expect(editorId, 'editor provisioned').toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: editorId, permission_set: ops.name }, { context: SYS }); + + for (const object of Object.keys(PATCHES)) { + const id = String((await ql.find(object, { limit: 1, context: SYS }))?.[0]?.id ?? ''); + expect(id, `a seeded ${object} row`).toBeTruthy(); + existing.set(object, id); + } + + const foreign = await call(adminTok, 'POST', '/data/showcase_private_note', { title: 'admin note' }); + expect(foreign.status, `admin note created — ${JSON.stringify(foreign.body)}`).toBeLessThan(300); + foreignNoteId = createdId(foreign); + const own = await call(memberTok, 'POST', '/data/showcase_private_note', { title: 'member note' }); + expect(own.status, `member note created — ${JSON.stringify(own.body)}`).toBeLessThan(300); + ownNoteId = createdId(own); + expect(foreignNoteId && ownNoteId, 'both notes carry an id').toBeTruthy(); + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + for (const [object, patch] of Object.entries(PATCHES)) { + it(`${object}: PATCH by id answers one refusal for a row and for no row`, async () => { + const id = existing.get(object)!; + const before = await stored(object, id); + const hit = await call(memberTok, 'PATCH', `/data/${object}/${id}`, patch); + const miss = await call(memberTok, 'PATCH', `/data/${object}/${MISSING}`, patch); + + expect(hit.status, `the member may not update ${object} — ${JSON.stringify(hit.body)}`).toBe(403); + expect(withoutId(miss, MISSING), 'the answer for no row').toEqual(withoutId(hit, id)); + expect(await stored(object, id), 'a refused write leaves the row as it was').toEqual(before); + }); + + it(`${object}: DELETE by id keeps its one refusal for a row and for no row`, async () => { + const id = existing.get(object)!; + const hit = await call(memberTok, 'DELETE', `/data/${object}/${id}`); + const miss = await call(memberTok, 'DELETE', `/data/${object}/${MISSING}`); + + expect(hit.status, JSON.stringify(hit.body)).toBe(403); + expect(withoutId(miss, MISSING)).toEqual(withoutId(hit, id)); + expect(await stored(object, id), 'a refused delete leaves the row').toBeTruthy(); + }); + + it(`${object}: the bulk update doors answer one per-row refusal for a row and for no row`, async () => { + const id = existing.get(object)!; + const many = (rid: string) => + call(memberTok, 'POST', `/data/${object}/updateMany`, { records: [{ id: rid, data: patch }] }); + const batch = (rid: string) => + call(memberTok, 'POST', `/data/${object}/batch`, { operation: 'update', records: [{ id: rid, data: patch }] }); + + const manyHit = await many(id); + expect((manyHit.body as any)?.results?.[0]?.errors?.[0]?.httpStatus, JSON.stringify(manyHit.body)).toBe(403); + expect(withoutId(await many(MISSING), MISSING)).toEqual(withoutId(manyHit, id)); + const batchHit = await batch(id); + expect((batchHit.body as any)?.results?.[0]?.errors?.[0]?.httpStatus, JSON.stringify(batchHit.body)).toBe(403); + expect(withoutId(await batch(MISSING), MISSING)).toEqual(withoutId(batchHit, id)); + }); + } + + it('with a stale If-Match, PATCH and DELETE keep one status and code for a row and for no row', async () => { + const STALE = { 'If-Match': '2001-01-01T00:00:00.000Z' }; + for (const object of ['showcase_project_membership', 'sys_user_permission_set']) { + const id = existing.get(object)!; + for (const method of ['PATCH', 'DELETE'] as const) { + const body = method === 'PATCH' ? PATCHES[object] : undefined; + const hit = await call(memberTok, method, `/data/${object}/${id}`, body, STALE); + const miss = await call(memberTok, method, `/data/${object}/${MISSING}`, body, STALE); + expect(hit.status, `${method} ${object} — ${JSON.stringify(hit.body)}`).toBe(403); + expect({ status: miss.status, code: codeOf(miss) }, `${method} ${object}`).toEqual({ status: hit.status, code: codeOf(hit) }); + } + } + }); + + it('control: a caller holding the edit grant updates an in-scope row and gets 404 for no row', async () => { + const id = existing.get('showcase_project_membership')!; + const ok = await call(editorTok, 'PATCH', `/data/showcase_project_membership/${id}`, { engagement: 'reviewer' }); + const miss = await call(editorTok, 'PATCH', `/data/showcase_project_membership/${MISSING}`, { engagement: 'reviewer' }); + + expect(ok.status, JSON.stringify(ok.body)).toBe(200); + expect(miss.status, JSON.stringify(miss.body)).toBe(404); + expect(codeOf(miss)).toBe('RECORD_NOT_FOUND'); + }); + + it('control: edit with a row-level restriction keeps one not-found for a foreign row and for no row', async () => { + const hit = await call(memberTok, 'PATCH', `/data/showcase_private_note/${foreignNoteId}`, { title: 'x' }); + const miss = await call(memberTok, 'PATCH', `/data/showcase_private_note/${MISSING}`, { title: 'x' }); + const own = await call(memberTok, 'PATCH', `/data/showcase_private_note/${ownNoteId}`, { title: 'mine, renamed' }); + + expect(hit.status, JSON.stringify(hit.body)).toBe(404); + expect(codeOf(hit)).toBe('RECORD_NOT_FOUND'); + expect(withoutId(miss, MISSING)).toEqual(withoutId(hit, foreignNoteId)); + expect(own.status, JSON.stringify(own.body)).toBe(200); + expect((await stored('showcase_private_note', foreignNoteId))?.title).toBe('admin note'); + }); + + it('control: the administrator updates, gets 404 for no row, and gets the write guard for both ids on a read-only federated object', async () => { + const id = existing.get('showcase_project_membership')!; + const ok = await call(adminTok, 'PATCH', `/data/showcase_project_membership/${id}`, { engagement: 'owner' }); + const miss = await call(adminTok, 'PATCH', `/data/showcase_project_membership/${MISSING}`, { engagement: 'owner' }); + expect(ok.status, JSON.stringify(ok.body)).toBe(200); + expect(miss.status, JSON.stringify(miss.body)).toBe(404); + expect(codeOf(miss)).toBe('RECORD_NOT_FOUND'); + + const extId = existing.get('showcase_ext_customer')!; + const extHit = await call(adminTok, 'PATCH', `/data/showcase_ext_customer/${extId}`, { region: 'EU' }); + const extMiss = await call(adminTok, 'PATCH', `/data/showcase_ext_customer/${MISSING}`, { region: 'EU' }); + const extDeleteMiss = await call(adminTok, 'DELETE', `/data/showcase_ext_customer/${MISSING}`); + expect(extHit.status, JSON.stringify(extHit.body)).toBe(403); + expect(codeOf(extHit)).toBe('EXTERNAL_WRITE_FORBIDDEN'); + expect(withoutId(extMiss, MISSING)).toEqual(withoutId(extHit, extId)); + expect({ status: extDeleteMiss.status, code: codeOf(extDeleteMiss) }).toEqual({ status: 403, code: 'EXTERNAL_WRITE_FORBIDDEN' }); + }); +}); diff --git a/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts b/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts index be143198e4f..ea7cd477a57 100644 --- a/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts +++ b/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts @@ -359,9 +359,10 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read findOne.mockClear(); const matched = await door().send(verb, PLAIN, PLAIN_ID, ifMatch(version)); expect(matched.status, JSON.stringify(matched.body)).toBe(200); - // DELETE probes once more when a token is present (it skips the probe without one); - // PATCH probes in both. Nothing else is read on the match path. - expect(findOne.mock.calls.length).toBe(withoutToken + (verb === 'DELETE' ? 1 : 0)); + // Both verbs probe once more when a token is present and skip the probe without + // one: neither looks the record up ahead of the engine, which judges the write + // first. Nothing else is read on the match path. + expect(findOne.mock.calls.length).toBe(withoutToken + 1); } finally { findOne.mockRestore(); } diff --git a/packages/runtime/src/plugin-driver-external-object.spike.test.ts b/packages/runtime/src/plugin-driver-external-object.spike.test.ts index 7a4b751f023..98374805f02 100644 --- a/packages/runtime/src/plugin-driver-external-object.spike.test.ts +++ b/packages/runtime/src/plugin-driver-external-object.spike.test.ts @@ -620,18 +620,16 @@ describe('(g) object permission sets apply to the external object', () => { expect(calls.slice(mark)).toEqual([]); }); - it('the same caller gets 403 on a PATCH; the remote sees one by-id read and no write', async () => { + it('the same caller gets 403 on a PATCH; the remote sees no read and no write', async () => { const mark = calls.length; const res = await as(outsiderToken, 'PATCH', `/data/${OBJECT}/n_cut`, { name: 'Renamed' }); const body = await readJson(res); expect(res.status, JSON.stringify(body)).toBe(403); expect(body?.code).toBe('PERMISSION_DENIED'); - // The write door loads the record before it refuses — on a plugin driver - // that is a round trip to the remote on behalf of a caller who may not read. - // [#27277] The floor answers that `findOne` from one capped `find`. - expect(calls.slice(mark)).toEqual([ - { method: 'find', object: OBJECT, query: { where: { id: 'n_cut' }, object: OBJECT, limit: EXTERNAL_ROW_CAP + 1 } }, - ]); + // The write door judges the update before it looks the record up, as it + // judges a delete, so a caller who may not update the object costs the + // remote no round trip at all. + expect(calls.slice(mark)).toEqual([]); expect(rows.find((r) => r.id === 'n_cut')?.name).toBe('Cut'); }); });