diff --git a/.changeset/23586-occ-conflict-caller-visibility.md b/.changeset/23586-occ-conflict-caller-visibility.md new file mode 100644 index 00000000000..24261ebc669 --- /dev/null +++ b/.changeset/23586-occ-conflict-caller-visibility.md @@ -0,0 +1,16 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): an optimistic-concurrency conflict reports only what the caller may read + +Clause-②: no + +`updateData` and `deleteData` judge a caller's version token (`If-Match`, or the body/query `expectedVersion`) on the row their existence probe reads with a system context. On a mismatch they used to answer `409 CONCURRENT_UPDATE` with that system-read row as `currentRecord`, before the caller's row- and field-level security had run. + +Now a mismatch re-reads the row under the caller's own context, and only then answers: + +- **A row the caller cannot read:** `404 RECORD_NOT_FOUND`, the same answer the same request without a version token gets. It carries no `currentVersion` and no `currentRecord`. +- **A row the caller can read:** `409 CONCURRENT_UPDATE` as before, with `currentRecord` read as the caller reads it, so a field hidden from the caller is absent from it. `currentVersion` is unchanged. + +A request whose token matches, or that sends none, is unchanged and issues no extra read. Every door that renders the protocol's answer (`RestServer`, the runtime dispatcher's `/data` domain and the Hono hosts) gets the narrowed answer. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 389ca76a105..95d2168c821 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -2798,7 +2798,9 @@ function simpleHash(str: string): string { * The HTTP layer maps this to `409 Conflict` with code `CONCURRENT_UPDATE`, * and includes both the current server-side version and the current record * payload so the client can render an informed conflict-resolution UI - * ("Reload latest" vs. "Overwrite anyway"). + * ("Reload latest" vs. "Overwrite anyway"). The record is the row as the + * caller reads it, and a row the caller cannot read is never a conflict: it + * answers not-found instead (#23586, see `assertVersionOf`). * * NOTE: This is an *application-level* compare-and-set — not an atomic * storage-layer CAS. There is a small TOCTOU window between the version @@ -14633,7 +14635,7 @@ export class ObjectStackProtocolImplementation implements // 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.assertVersionOf(request.object, request.id, current, request.expectedVersion); + await this.assertVersionOf(request.object, request.id, current, 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` @@ -14729,7 +14731,7 @@ export class ObjectStackProtocolImplementation implements async deleteData(request: { object: string, id: string, expectedVersion?: string, context?: any }) { this.assertObjectRegistered(request.object); // [#3770] - await this.assertVersionMatch(request.object, request.id, request.expectedVersion); + 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; const deleted = await this.engine.delete(request.object, opts); @@ -14870,10 +14872,11 @@ export class ObjectStackProtocolImplementation implements } /** - * Optimistic Concurrency Control — the COMPARISON half, over a row the - * caller has already read. Pure: it 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). + * 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. * * When the caller passes a non-empty `expectedVersion` token (typically the * `updated_at` value they read), a mismatch throws `ConcurrentUpdateError`, @@ -14903,13 +14906,35 @@ export class ObjectStackProtocolImplementation implements * SQLite family — must not decide whether a save is a conflict. See * `normaliseVersionToken` for the measured input domain and for why the * change cannot refuse a token that is accepted today. + * + * ## [#23586] The conflict reports only what the caller may read + * + * `current` is the SYSTEM-context probe ({@link probeRecord}: existence, + * not visibility), and this gate runs before `engine.update` / + * `engine.delete` apply the caller's policy. So a mismatch does not answer + * from it. It re-reads the row under the caller's own `context`, the read + * door's question (every RLS and FLS rule applies): + * + * - no row → `recordNotFoundError`, the producer the security layer's + * by-id write gate answers a row the caller cannot read with. The stale + * token then gets exactly the answer the same request without a token + * gets: no `currentVersion`, no `currentRecord`, so the `409` no longer + * tells a caller that a hidden row exists or when it was last written; + * - a row → the `409`, with THAT row as `currentRecord`, field-masked as + * the caller's own read is. `currentVersion` stays the token this + * comparison ran on. + * + * The read is issued only on a mismatch: a request whose token agrees, or + * that sends none, reads exactly what it read before. A read that throws + * (the caller's read grant withheld, a store fault) propagates as raised. */ - private assertVersionOf( + private async assertVersionOf( object: string, id: string, current: any, expectedVersion: string | undefined, - ): void { + context: EngineQueryOptionsParsed['context'], + ): Promise { assertVersionTokenNotMalformed(expectedVersion); const expected = normaliseVersionToken(expectedVersion); if (!expected) return; @@ -14927,9 +14952,11 @@ export class ObjectStackProtocolImplementation implements // it names, turning a resolvable conflict into an unresolvable one. const currentVersion = currentToken.instant ?? currentToken.token; const expectedVersionText = expected.instant ?? expected.token; + const visible = await this.readAsCaller(object, id, context); + if (!visible) throw recordNotFoundError(object, id); throw new ConcurrentUpdateError({ currentVersion, - currentRecord: current, + currentRecord: visible, message: `Record ${object}/${id} was modified by another user (current version ${currentVersion}, expected ${expectedVersionText})`, }); } @@ -14956,11 +14983,24 @@ export class ObjectStackProtocolImplementation implements object: string, id: string, expectedVersion: string | undefined, + context: EngineQueryOptionsParsed['context'], ): Promise { assertVersionTokenNotMalformed(expectedVersion); if (!normaliseVersionToken(expectedVersion)) return; const current = await this.probeRecord(object, id); - this.assertVersionOf(object, id, current, expectedVersion); + await this.assertVersionOf(object, id, current, expectedVersion, context); + } + + /** + * [#23586] The row as the CALLER reads it by id — the twin of + * {@link probeRecord}, which asks existence as the system. Same `context` + * spelling as the write it precedes: passed only when the request carried + * one, so a caller with no context reads as its write would run. + */ + private async readAsCaller(object: string, id: string, context: EngineQueryOptionsParsed['context']): Promise { + const opts: EngineQueryOptionsParsed = { where: { id } }; + if (context !== undefined) opts.context = context; + return this.engine.findOne(object, opts); } // ========================================== diff --git a/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts b/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts new file mode 100644 index 00000000000..d86cd39c03e --- /dev/null +++ b/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts @@ -0,0 +1,368 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23586] A `409 CONCURRENT_UPDATE` reports only what the caller may read — + * with the REAL security layer (`SecurityPlugin` on a real `ObjectQL` over a + * real `SqlDriver`), at both doors of `PATCH` and `DELETE /data/:object/:id`: + * `RestServer`'s routes and the runtime dispatcher's `/data` domain. + * + * ## What was wrong + * + * `updateData` / `deleteData` judge the caller's version token on the row + * `probeRecord` reads with a SYSTEM context (an existence question, on + * purpose), and they judge it before `engine.update` / `engine.delete` runs + * the caller's write policy. The conflict carried that system-read row as + * `currentRecord`. So a stale token got a member a row their RLS hides, or a + * field their FLS hides, and the `409` itself (with `currentVersion`) told + * them a hidden row exists where the same request without a token answers + * `404`. + * + * ## What is held here, per door and per verb + * + * - **A row the caller's RLS hides:** a stale token answers exactly what the + * same request without a token answers (the control): `404 + * RECORD_NOT_FOUND`, the same status and the same body, with no + * `currentVersion` and no `currentRecord`. The row is untouched. + * - **A field the caller's FLS hides:** the `409`'s `currentRecord` does not + * carry it, as the caller's own read of the row does not. + * - **A row the caller may read whole (the positive control):** a stale token + * still answers `409` with the row and its version, so conflict detection + * is not weakened. + * - **A token that matches:** the request reads the store exactly as often as + * one with no token. Only the mismatch branch reads again. + * + * ## How each door is read + * + * `RestServer`: its registered handler, the auth boundary (`resolveExecCtx`) + * stubbed, and the body it answered. The dispatcher: `/data` has no local + * catch, so the protocol's throw reaches the host's exit; it is rendered here + * through `declaredEnvelopeForThrow` (`plugin-hono-server`), the exit both + * Hono hosts answer a throw with, and the thrown error itself is read too. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SecurityPlugin } from '@objectstack/plugin-security'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from '@objectstack/rest'; +import { declaredEnvelopeForThrow } from '@objectstack/plugin-hono-server'; +import { resolveThrownHttpError } from '@objectstack/types'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +const PREFIX = '/api/v1'; +const DATA = `${PREFIX}/data`; + +/** The object with a row scope and a hidden field. */ +const SCOPED = 'occ_vis_ledger'; +/** An object no row or field rule names: the positive control. */ +const PLAIN = 'occ_vis_plain'; + +const SYS_CTX = { isSystem: true, userId: 'usr_system' }; + +const MEMBER_SET = PermissionSetSchema.parse({ + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + fields: { [`${SCOPED}.secret`]: { readable: false, editable: false } }, + rowLevelSecurity: [{ name: 'region_scope', object: SCOPED, operation: 'all', using: "record.region != 'HIDDEN'" }], +}); + +const MEMBER_CTX = { userId: 'usr_member', positions: [] as string[], permissions: [MEMBER_SET.name], posture: 'MEMBER' as const }; + +/** A well-formed instant no row here carries: a token read before a write that moved it. */ +const STALE = '2000-01-01T00:00:00.000Z'; + +/** The row RLS hides from the member. */ +const HIDDEN_ID = 'h1'; +/** The row the member reads, without its `secret`. */ +const MASKED_ID = 'v1'; +/** The positive control's row. */ +const PLAIN_ID = 'p1'; + +/** A by-id read's options, typed to the engine's contract. */ +const byId = (id: string, context: EngineQueryOptions['context']): EngineQueryOptions => ({ where: { id }, context }); + +const clone = (v: T): T => (v === undefined ? v : JSON.parse(JSON.stringify(v))); + +interface Answer { + status: number; + body: any; + /** The dispatcher only: the error the protocol threw, before the exit rendered it. */ + thrown?: any; +} + +type Verb = 'PATCH' | 'DELETE'; + +interface Door { + send(verb: Verb, object: string, id: string, headers?: Record): Promise; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined, headers: {} as Record }; + res.status = (code: number) => { res.statusCode = code; return res; }; + res.json = (body: any) => { res.body = body; return res; }; + res.send = () => res; + res.end = () => res; + res.header = (k: string, v: string) => { res.headers[k] = v; return res; }; + res.setHeader = () => {}; + res.write = () => true; + return res; +} + +/** The PATCH body every request below sends; DELETE carries none. */ +const PATCH_BODY = { title: 'written' }; + +function restDoor(protocol: ObjectStackProtocolImplementation): Door { + const noop = () => {}; + const server = { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; + const rest: any = new RestServer(server as any, protocol as any, { api: { requireAuth: false } } as any); + rest.resolveExecCtx = async () => clone(MEMBER_CTX); + rest.registerRoutes(); + const routeOf = (verb: Verb) => { + const route = rest.getRoutes().find((r: any) => r.method === verb && r.path === `${DATA}/:object/:id`); + if (!route) throw new Error(`${verb} ${DATA}/:object/:id is not registered`); + return route; + }; + return { + send: async (verb, object, id, headers = {}) => { + const res = makeRes(); + await routeOf(verb).handler({ + method: verb, path: `${DATA}/${object}/${id}`, params: { object, id }, + headers: Object.fromEntries(Object.entries(headers).map(([k, v]) => [k.toLowerCase(), v])), + query: {}, + body: verb === 'PATCH' ? clone(PATCH_BODY) : undefined, + }, res); + return { status: res.statusCode, body: res.body }; + }, + }; +} + +/** + * What `createHonoApp`'s `${prefix}/*` catch-all hands `dispatch()`, over a + * real Fetch `Request` (the sibling `data-update-expected-version.test.ts` + * spells the same statements), and the exit's rendering of a throw. + */ +function dispatcherDoor(protocol: ObjectStackProtocolImplementation): Door { + const services: Record = { protocol }; + const get = (n: string) => services[n] ?? null; + const kernel: any = { context: { getService: get }, getService: get, getServiceAsync: async (n: string) => get(n) }; + const dispatcher = new HttpDispatcher(kernel); + (dispatcher as any).timedResolveExecutionContext = async () => clone(MEMBER_CTX); + return { + send: async (verb, object, id, headers = {}) => { + const raw = new Request(`http://localhost${DATA}/${object}/${id}`, { + method: verb, + headers: { 'content-type': 'application/json', ...headers }, + ...(verb === 'PATCH' ? { body: JSON.stringify(PATCH_BODY) } : {}), + }); + const url = new URL(raw.url); + let body: unknown; + try { body = JSON.parse(await raw.clone().text()); } catch { body = {}; } + try { + const res: any = await dispatcher.dispatch(verb, url.pathname.substring(PREFIX.length), body, {}, { request: raw } as any, PREFIX); + return { status: res.response?.status ?? 0, body: res.response?.body }; + } catch (thrown) { + const envelope = declaredEnvelopeForThrow(thrown); + if (envelope) return { status: envelope.status, body: envelope.body, thrown }; + const resolved = resolveThrownHttpError(thrown); + return { status: resolved.status, body: { code: resolved.code, message: resolved.message }, thrown }; + } + }, + }; +} + +const ifMatch = (token: string) => ({ 'If-Match': `"${token}"` }); + +describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read — the real security layer', () => { + let engine: ObjectQL; + let protocol: ObjectStackProtocolImplementation; + const doors: Array<[string, Door]> = []; + + /** The row as the system reads it now (`null` once deleted). */ + const systemRow = async (object: string, id: string) => + (await engine.findOne(object, byId(id, SYS_CTX))) as any; + /** The row's current version token, as the system reads it. */ + const versionOf = async (object: string, id: string): Promise => { + const row = await systemRow(object, id); + const v = row?.updated_at instanceof Date ? row.updated_at.toISOString() : String(row?.updated_at ?? ''); + if (!v) throw new Error(`${object}/${id} carries no updated_at, so no version token exists to pin`); + return v; + }; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as any), + true, + ); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.occ-conflict-caller-visibility', + name: 'OCC conflict caller visibility', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [ + { + name: SCOPED, + label: 'Ledger', + sharingModel: 'public_read_write', + fields: { + title: { name: 'title', type: 'text' }, + region: { name: 'region', type: 'text' }, + secret: { name: 'secret', type: 'text' }, + }, + }, + { + name: PLAIN, + label: 'Plain', + sharingModel: 'public_read_write', + fields: { + title: { name: 'title', type: 'text' }, + note: { name: 'note', type: 'text' }, + }, + }, + ], + } as never); + await engine.syncSchemas(); + + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_SET], + }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + vi.spyOn((engine as unknown as { logger: { warn: () => void } }).logger, 'warn').mockImplementation(() => undefined); + + protocol = new ObjectStackProtocolImplementation(engine as any); + doors.push(['RestServer', restDoor(protocol)], ['the dispatcher', dispatcherDoor(protocol)]); + }, 60_000); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + /** A fresh copy of the three rows, so each case starts from a known store. */ + async function seed(): Promise { + for (const [object, id] of [[SCOPED, HIDDEN_ID], [SCOPED, MASKED_ID], [PLAIN, PLAIN_ID]] as const) { + if (await systemRow(object, id)) await engine.delete(object, { where: { id }, context: SYS_CTX } as any); + } + await engine.insert(SCOPED, [ + { id: HIDDEN_ID, title: 'hidden', region: 'HIDDEN', secret: 'hidden-secret' }, + { id: MASKED_ID, title: 'visible', region: 'NA', secret: 'masked-secret' }, + ], { context: SYS_CTX } as never); + await engine.insert(PLAIN, [{ id: PLAIN_ID, title: 'plain', note: 'readable' }], { context: SYS_CTX } as never); + } + + const codeOf = (body: any): string | undefined => + body?.error && typeof body.error === 'object' ? body.error.code : body?.code; + /** Where either door puts a conflict's state: flat beside `code`, or in the envelope's `details`. */ + const conflictState = (body: any): Record => + (body?.error && typeof body.error === 'object' ? body.error.details : body) ?? {}; + + for (const verb of ['PATCH', 'DELETE'] as const) { + for (const doorIndex of [0, 1]) { + const doorName = doorIndex === 0 ? 'RestServer' : 'the dispatcher'; + const door = () => doors[doorIndex][1]; + + describe(`${doorName}: ${verb} /data/:object/:id`, () => { + it('a row RLS hides: a stale token answers exactly what the control (no token) answers — 404, no version, no record', async () => { + await seed(); + const control = await door().send(verb, SCOPED, HIDDEN_ID); + expect({ status: control.status, code: codeOf(control.body) }, JSON.stringify(control.body)) + .toEqual({ status: 404, code: 'RECORD_NOT_FOUND' }); + + const stale = await door().send(verb, SCOPED, HIDDEN_ID, ifMatch(STALE)); + // Indistinguishable: status, and the whole body (code, message, keys). + expect(stale.status, JSON.stringify(stale.body)).toBe(control.status); + expect(stale.body).toEqual(control.body); + const wire = JSON.stringify(stale.body); + expect(wire).not.toContain('currentVersion'); + expect(wire).not.toContain('currentRecord'); + expect(wire).not.toContain('hidden-secret'); + if (stale.thrown !== undefined) { + // The dispatcher's throw itself carries no row and no version. + expect(stale.thrown.currentRecord).toBeUndefined(); + expect(stale.thrown.currentVersion).toBeUndefined(); + expect(stale.thrown.code).toBe(control.thrown?.code); + } + // Nothing was written. + expect(await systemRow(SCOPED, HIDDEN_ID)).toMatchObject({ title: 'hidden', secret: 'hidden-secret' }); + }, 60_000); + + it('a field FLS hides: the 409 carries the row the caller reads, without that field', async () => { + await seed(); + const version = await versionOf(SCOPED, MASKED_ID); + const stale = await door().send(verb, SCOPED, MASKED_ID, ifMatch(STALE)); + expect({ status: stale.status, code: codeOf(stale.body) }, JSON.stringify(stale.body)) + .toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect(conflictState(stale.body).currentVersion).toBe(version); + expect(JSON.stringify(stale.body)).not.toContain('masked-secret'); + + // The record the conflict holds is the caller's own read of the row. + const thrownRecord = stale.thrown?.currentRecord; + const record: any = doorIndex === 0 ? conflictState(stale.body).currentRecord : thrownRecord; + expect(record).toMatchObject({ id: MASKED_ID, title: 'visible', region: 'NA' }); + expect(record).not.toHaveProperty('secret'); + const callerRead: any = await engine.findOne(SCOPED, byId(MASKED_ID, clone(MEMBER_CTX))); + expect(callerRead).not.toHaveProperty('secret'); + expect(record).toEqual(callerRead); + + expect(await systemRow(SCOPED, MASKED_ID)).toMatchObject({ title: 'visible', secret: 'masked-secret' }); + }, 60_000); + + it('positive control: a row the caller reads whole still conflicts, with the whole row and its version', async () => { + await seed(); + const version = await versionOf(PLAIN, PLAIN_ID); + const stale = await door().send(verb, PLAIN, PLAIN_ID, ifMatch(STALE)); + expect({ status: stale.status, code: codeOf(stale.body) }, JSON.stringify(stale.body)) + .toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect(conflictState(stale.body).currentVersion).toBe(version); + const record: any = doorIndex === 0 ? conflictState(stale.body).currentRecord : stale.thrown?.currentRecord; + expect(record).toEqual(await systemRow(PLAIN, PLAIN_ID)); + expect(record).toMatchObject({ id: PLAIN_ID, title: 'plain', note: 'readable' }); + // Not written. + expect(await systemRow(PLAIN, PLAIN_ID)).toMatchObject({ title: 'plain' }); + }, 60_000); + + it('a matching token reads the store exactly as often as no token, and is served', async () => { + const findOne = vi.spyOn(engine, 'findOne'); + try { + await seed(); + findOne.mockClear(); + const plain = await door().send(verb, PLAIN, PLAIN_ID); + expect(plain.status, JSON.stringify(plain.body)).toBe(200); + const withoutToken = findOne.mock.calls.length; + + await seed(); + const version = await versionOf(PLAIN, PLAIN_ID); + 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)); + } finally { + findOne.mockRestore(); + } + }, 60_000); + }); + } + } +});