From 57b661d2a4d6aabbfa543e8c79135c6054e4d569 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 12:18:19 +0000 Subject: [PATCH 1/5] fix(metadata-protocol): the by-id OCC comparison defers to the write door when the caller's read returns no row On a version-token mismatch, updateData/deleteData re-read the row as the caller. When that read returns nothing, or refuses with a declared client-band status, the comparison is now dropped and the request goes on to the write, whose door answers it - the answer the same request without a token gets. A row the caller reads still answers 409 CONCURRENT_UPDATE with that row; a read that faults still propagates as raised. Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../docs/protocol/kernel/http-protocol.mdx | 5 ++ .../src/protocol-record-reads.ts | 29 ++++++++- packages/metadata-protocol/src/protocol.ts | 60 +++++++++++++------ 3 files changed, 73 insertions(+), 21 deletions(-) diff --git a/content/docs/protocol/kernel/http-protocol.mdx b/content/docs/protocol/kernel/http-protocol.mdx index 08674db433d..65aee670930 100644 --- a/content/docs/protocol/kernel/http-protocol.mdx +++ b/content/docs/protocol/kernel/http-protocol.mdx @@ -721,6 +721,11 @@ each row of `updateMany` and of `batch` with `operation: "update"`. A caller who update the object gets `404 RECORD_NOT_FOUND` for an id that names no record, and for a record its row-level access hides. +A version token (`If-Match`, or `expectedVersion`) changes none of these answers. An +update or delete carrying a stale token is answered `409 CONCURRENT_UPDATE` only for a +record the caller can read; for any other id it gets exactly what the same request +without a token gets. + ### Delete Record Delete a record by ID. diff --git a/packages/metadata-protocol/src/protocol-record-reads.ts b/packages/metadata-protocol/src/protocol-record-reads.ts index f59cd262fb6..d41e4636b0b 100644 --- a/packages/metadata-protocol/src/protocol-record-reads.ts +++ b/packages/metadata-protocol/src/protocol-record-reads.ts @@ -50,6 +50,7 @@ import type { ObjectStackProtocolImplementation } from './protocol.js'; import { ConcurrentUpdateError, MalformedVersionTokenError, canonicalVersionInstant } from './protocol.js'; import { recordNotFoundError } from '@objectstack/core'; +import { declaredHttpStatus } from '@objectstack/types'; import { collectInternalWriteResponseFields, omitInternalFieldsFromWriteResponse } from './write-response-internal-fields.js'; import { isMissingTableError } from '@objectstack/metadata/errors'; import { canServeApiOperation, resolveSearchFieldResolution } from '@objectstack/spec/data'; @@ -169,6 +170,19 @@ function versionTokensAgree(current: NormalisedVersion, expected: NormalisedVers return current.token === expected.token; } +/** + * Did the caller's own read of a row REFUSE, rather than fault? A refusal + * declares a client-band status (`4xx`, in either spelling + * `declaredHttpStatus` reads) — the read door's verdict on this caller, which + * says nothing about the row. A server-band status, or none at all (a bare + * driver `Error`), is a fault, and the OCC comparison lets it propagate as + * raised rather than read it as "the caller reads no row". + */ +function isCallerReadRefusal(e: unknown): boolean { + const status = declaredHttpStatus(e); + return status !== undefined && status < 500; +} + /** * The `ObjectStackProtocolImplementation` members the functions below read — * every one of them, and no other; each function's `this` is the `Pick` of the @@ -227,8 +241,19 @@ export async function assertVersionOf( // 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); + // A row the caller's own read does not return — absent, or the read + // refused — is no conflict this gate may report, and no answer of its + // own either: the comparison is dropped and the write door answers, + // exactly as it answers the same request without a token. The + // method's docblock in `protocol.ts` carries the reasoning. + let visible: unknown; + try { + visible = await this.readAsCaller(object, id, context); + } catch (e) { + if (!isCallerReadRefusal(e)) throw e; + return; + } + if (!visible) return; throw new ConcurrentUpdateError({ currentVersion, currentRecord: visible, diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index b2b42b025ec..8b7eb2b64a2 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3199,8 +3199,9 @@ function simpleHash(str: string): string { * 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"). 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`). + * caller reads it, and a row the caller cannot read is never a conflict: the + * write door answers it, as it answers the same request without a token + * (#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 @@ -14444,8 +14445,10 @@ export class ObjectStackProtocolImplementation implements // // 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. + // probe does not find, or that the caller's own read does not return, + // is left to the engine's answer: OCC has never treated a missing + // record as a concurrency conflict, and a row the caller cannot read + // answers what a missing one does ({@link assertVersionOf}). 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; @@ -14663,13 +14666,14 @@ 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. * - * ⛔ 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. + * ⛔ And what it finds never answers 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 answering + * only for a row the caller's own read returns ({@link assertVersionOf}), + * 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, token or none. */ private async probeRecord(object: string, id: string): Promise { return this.engine.findOne(object, { where: { id }, context: { isSystem: true } } as any); @@ -14772,18 +14776,36 @@ export class ObjectStackProtocolImplementation implements * 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. + * comparison ran on. A caller who reads the row but may not write it + * gets this `409` too: the row it is shown is one it can GET anyway; + * - no row, or a read that REFUSES (a declared `4xx`) → no answer from + * this gate at all. The comparison is dropped and the request goes on + * to the write, whose door answers it: the stale token then gets + * exactly the answer the same request without a token gets — status, + * code and sentence — and so does a token on an id that names no row + * ("Record not found → no check" above), so the two cannot answer + * apart. That is the governing text for the write doors (ruling of + * record 5985885287): a row the caller cannot read answers what a + * nonexistent id answers, on update and delete alike. + * + * Why the write door's answer and not one of this gate's own: this gate + * runs BEFORE the write is authorized, so any answer it gives for a row the + * caller cannot read — a `404`, or the read door's refusal rethrown — + * differs from the write door's for every caller the write door refuses + * first (no update or delete grant, the delegated-administration gate on + * the RBAC tables), and from what an id naming no row gets. Handing over + * loses no guard: the write door refuses a by-id write of a row the + * caller cannot read (the security layer asks the read question ahead of + * the write and answers `recordNotFoundError`), and a token that could not + * be compared never lets a write land that the same request without a + * token would not. * * 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. + * that sends none, reads exactly what it read before. A read that FAULTS — + * a server-band status, or none declared at all — propagates as raised: a + * store fault is no verdict about the caller. */ private assertVersionOf( object: string, From 612f0d3b7aebcbfec7a54959c4b81d32a11f552f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 12:24:23 +0000 Subject: [PATCH 2/5] test: pin the by-id OCC handover to the write door at the protocol, runtime and HTTP doors The protocol pins cover both arms (no row, read refused), the fault that still propagates and the readable-row conflict. The runtime pin adds an object the caller reads but may not write under the real security layer. The showcase pins compare status, code and sentence for a row and for no row, token or none, on a single install and behind the organization wall. Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../harden-occ-unreadable-row-write-door.md | 13 + .../src/protocol.record-not-found.test.ts | 126 +++++++++- ...le-token-organization-wall.dogfood.test.ts | 223 ++++++++++++++++++ ...update-by-id-refusal-order.dogfood.test.ts | 119 +++++++++- .../data-conflict-caller-visibility.test.ts | 61 ++++- 5 files changed, 525 insertions(+), 17 deletions(-) create mode 100644 .changeset/harden-occ-unreadable-row-write-door.md create mode 100644 packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts diff --git a/.changeset/harden-occ-unreadable-row-write-door.md b/.changeset/harden-occ-unreadable-row-write-door.md new file mode 100644 index 00000000000..0ef04c36db4 --- /dev/null +++ b/.changeset/harden-occ-unreadable-row-write-door.md @@ -0,0 +1,13 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): the by-id OCC comparison answers only for a row the caller reads; every other row gets the write door's answer + +Clause-②: no +Metadata-protocol: none + +`updateData` and `deleteData` compare a caller's version token (`If-Match`, or `expectedVersion`) before the write is authorized. On a mismatch they re-read the row as the caller. When that read returns no row, or refuses with a declared client-band status, the comparison is now dropped and the request goes on to the write, whose door answers it: the same status, code and sentence the request gets without a token, and the same a token on an id naming no row gets. This restores the governing text for the write doors (ruling of record 5985885287). + +- **Unchanged:** a row the caller reads still answers `409 CONCURRENT_UPDATE` with that row as `currentRecord`, including a row the caller reads but may not write. A matching token, or none, reads nothing more than before. A read that faults (a server-band status, or none declared) still propagates as raised. +- **The write door's answers:** a caller who may not update or delete the object gets that refusal (`403`), token or none; a caller who may write the object gets `404 RECORD_NOT_FOUND` for a row its read does not return, as for an id naming no row. 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 43822f850a9..47d1001d205 100644 --- a/packages/metadata-protocol/src/protocol.record-not-found.test.ts +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -22,6 +22,7 @@ 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 = { name: 'task', fields: { title: { name: 'title', type: 'text' } } }; @@ -43,18 +44,40 @@ function writeRefusal(): Error { * 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. + * + * `hidden` names rows a CALLER's read does not return (the system's read + * still does), and a by-id write of one is answered the way the security + * layer answers it: the read question is asked ahead of the write, and a row + * the caller cannot read gets what an id naming no row gets. `readRefusal` + * makes every caller read throw instead — a refusal, or a fault. */ function makeProtocol( rows: Record = {}, - opts: { refuse?: string; leavesScope?: ReadonlySet } = {}, + opts: { + refuse?: string; + leavesScope?: ReadonlySet; + hidden?: ReadonlySet; + readRefusal?: () => Error; + } = {}, ) { const store = new Map(Object.entries(rows)); 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; }); + /** A read or write made as a caller rather than as the system. */ + const asCaller = (o: any) => o?.context?.isSystem !== true; + const findOne = vi.fn(async (_object: string, o: any) => { + assertEngineFindOnePredicate(_object, o); + const id = String(o?.where?.id); + if (asCaller(o)) { + if (opts.readRefusal) throw opts.readRefusal(); + if (opts.hidden?.has(id)) return null; + } + return store.get(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 (asCaller(o) && opts.hidden?.has(id)) throw recordNotFoundError('task', id); if (!store.has(id)) return null; const next = { ...store.get(id), ...data }; store.set(id, next); @@ -63,7 +86,9 @@ function makeProtocol( const del = vi.fn(async (_object: string, o: any) => { assertEngineDeleteDispatch(o); if (refused(o)) throw writeRefusal(); - return store.delete(String(o?.where?.id)); + const id = String(o?.where?.id); + if (asCaller(o) && opts.hidden?.has(id)) throw recordNotFoundError('task', id); + return store.delete(id); }); const engine = { registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, @@ -358,3 +383,98 @@ describe('[#4435] deleteManyData reports per id, not per request', () => { expect(res.succeeded + res.failed).toBe(res.total); }); }); + +describe('a stale version token on a row the caller does not read gets the write door\'s answer, as no token does', () => { + // The by-id OCC comparison runs before the write is authorized. When the + // tokens disagree it re-reads the row as the caller, and a row that read + // does not return — absent, or the read refused — leaves the answer to the + // write door: what the same request without a token gets, status, code and + // sentence, which is also what a token on an id naming no row gets (ruling + // of record 5985885287: a row the caller cannot read answers what a + // nonexistent id answers, on update and delete alike). + const CURRENT = '2026-10-30T08:00:00.000Z'; + const STALE = '2001-01-01T00:00:00.000Z'; + const CALLER = { userId: 'u1' }; + const MISSING = 'definitely_not_a_row'; + const row = (id: string) => ({ id, title: 'x', updated_at: CURRENT }); + + /** A refused call as the doors render it: status, code and sentence. */ + const answerOf = (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 }), + ); + /** The answer with the id its sentence names written out, so the answers for two ids compare. */ + const withoutId = (a: { message: unknown }, id: string) => ({ ...a, message: String(a.message).split(id).join('ID') }); + + for (const verb of ['updateData', 'deleteData'] as const) { + const write = (p: any, id: string, expectedVersion?: string) => verb === 'updateData' + ? p.updateData({ object: 'task', id, data: { title: 'y' }, context: CALLER, ...(expectedVersion ? { expectedVersion } : {}) }) + : p.deleteData({ object: 'task', id, context: CALLER, ...(expectedVersion ? { expectedVersion } : {}) }); + + describe(verb, () => { + it('a hidden row, the write refused: the write refusal answers, for the row and for no row, token or none', async () => { + const { p, store } = makeProtocol({ real: row('real') }, { refuse: 'u1', hidden: new Set(['real']) }); + const stale = await answerOf(() => write(p, 'real', STALE)); + const noToken = await answerOf(() => write(p, 'real')); + const missing = await answerOf(() => write(p, MISSING, STALE)); + expect(stale).toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(stale).toEqual(noToken); + expect(withoutId(missing, MISSING)).toEqual(withoutId(stale, 'real')); + expect(store.get('real')).toEqual(row('real')); + }); + + it('the caller\'s read REFUSES: the write door\'s refusal answers, never the read\'s', async () => { + // A declared client-band status in either spelling is a refusal. + for (const readRefusal of [ + () => Object.assign(new Error('Reading this object is not permitted.'), { code: 'PERMISSION_DENIED', status: 403 }), + () => Object.assign(new Error('Reading this object is not permitted.'), { code: 'PERMISSION_DENIED', statusCode: 403 }), + ]) { + const { p, store } = makeProtocol({ real: row('real') }, { refuse: 'u1', readRefusal }); + const stale = await answerOf(() => write(p, 'real', STALE)); + const noToken = await answerOf(() => write(p, 'real')); + const missing = await answerOf(() => write(p, MISSING, STALE)); + expect(stale).toEqual(noToken); + expect(stale.message).toBe(writeRefusal().message); + expect(withoutId(missing, MISSING)).toEqual(withoutId(stale, 'real')); + expect(store.get('real')).toEqual(row('real')); + } + }); + + it('a hidden row, the write admitted up to its read question: the write door\'s 404, for the row and for no row alike', async () => { + const { p, store, update, del } = makeProtocol({ real: row('real') }, { hidden: new Set(['real']) }); + const stale = await answerOf(() => write(p, 'real', STALE)); + expect(stale).toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + // The write was asked: the 404 is the write door's, not one of the comparison's own. + expect((verb === 'updateData' ? update : del)).toHaveBeenCalledTimes(1); + expect(await answerOf(() => write(p, 'real'))).toEqual(stale); + expect(withoutId(await answerOf(() => write(p, MISSING, STALE)), MISSING)).toEqual(withoutId(stale, 'real')); + expect(store.get('real')).toEqual(row('real')); + }); + + it('a read that FAULTS propagates as raised, and the write is never asked', async () => { + for (const fault of [ + new Error('connection reset'), + Object.assign(new Error('store unavailable'), { code: 'SERVICE_UNAVAILABLE', status: 503 }), + ]) { + const { p, update, del } = makeProtocol({ real: row('real') }, { readRefusal: () => fault }); + await expect(write(p, 'real', STALE)).rejects.toBe(fault); + expect(update).not.toHaveBeenCalled(); + expect(del).not.toHaveBeenCalled(); + } + }); + + it('control: a row the caller reads still conflicts — 409 with that row, and the write is never asked', async () => { + const { p, store, update, del } = makeProtocol({ real: row('real') }); + await expect(write(p, 'real', STALE)).rejects.toMatchObject({ + code: 'CONCURRENT_UPDATE', + status: 409, + currentVersion: CURRENT, + currentRecord: row('real'), + }); + expect(update).not.toHaveBeenCalled(); + expect(del).not.toHaveBeenCalled(); + expect(store.get('real')).toEqual(row('real')); + }); + }); + } +}); diff --git a/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts b/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts new file mode 100644 index 00000000000..4894844fead --- /dev/null +++ b/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts @@ -0,0 +1,223 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// A by-id PATCH or DELETE carrying a stale version token, aimed at a row in +// another organization, answers exactly what the same request answers without +// a token — measured at the HTTP data door of the showcase behind a real +// organization wall (`multiTenant: 'posture-only'`, the `isolated` posture). +// +// ── what is pinned ───────────────────────────────────────────────────────── +// +// A plain member of organization B (the showcase's everyone baseline only) +// aims at two rows of organization A, which the wall hides from its reads: +// +// - an account (the member reads accounts and may not write them): PATCH +// and DELETE answer the write refusal; +// - a private note (the member may update notes and not delete them): PATCH +// answers not-found, DELETE the write refusal. +// +// For each, with the token in `If-Match`, in the PATCH body's +// `expectedVersion` and in the DELETE's `?expectedVersion`, the answer — +// status, code and sentence — equals the answer without a token, and equals +// the answer for an id that names no row. The version comparison answers only +// for a row the caller reads. +// +// Controls, so a green run is not "every token is ignored": +// +// - the wall is real (`isolated`, active, not degraded), the member is no +// platform administrator, and it reads its own organization's rows and +// neither of the other's; +// - on rows of its own organization the member's stale token answers +// `409 CONCURRENT_UPDATE` carrying the row — on the account it may not +// write too — and the current token updates its own note (200); +// - every refused write leaves the row as it was. +// +// The single-organization half (rows the row scope hides) is pinned in +// `update-by-id-refusal-order.dogfood.test.ts`, the data door's in +// `packages/metadata-protocol/src/protocol.record-not-found.test.ts`. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { type VerifyStack } from '@objectstack/verify'; +import { bootShowcase } from './showcase-boot.js'; + +const SYS = { isSystem: true } as const; +const OWNER_A = 'occ-wall-owner-a@verify.test'; +const OWNER_B = 'occ-wall-owner-b@verify.test'; +const MEMBER_B = 'occ-wall-member-b@verify.test'; +const MISSING = 'zz_no_such_row_0001'; +/** A version token no row carries: one read before a write that moved the row on. */ +const STALE = '2001-01-01T00:00:00.000Z'; + +interface Answer { status: number; body: unknown } +type Carrier = 'none' | 'If-Match' | 'expectedVersion'; + +/** 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; + +const idOf = (b: any): string => String(b?.id ?? b?.record?.id ?? b?.data?.id ?? ''); + +describe('behind the organization wall, a stale version token on another organization\'s row answers what no token answers', () => { + let stack: VerifyStack; + let ql: any; + let memberTok: string; + let orgA = ''; + let orgB = ''; + let accountA = ''; + let noteA = ''; + let accountB = ''; + let noteB = ''; + + 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 }; + }; + + /** + * A by-id PATCH or DELETE carrying `version` the way `carrier` names: the + * PATCH body's `expectedVersion`, the DELETE's `?expectedVersion`, or the + * `If-Match` header on either. + */ + const versioned = ( + method: 'PATCH' | 'DELETE', + object: string, + id: string, + patch: Record, + carrier: Carrier, + version = STALE, + ): Promise => { + const headers = carrier === 'If-Match' ? { 'If-Match': version } : {}; + if (method === 'PATCH') { + const body = carrier === 'expectedVersion' ? { ...patch, expectedVersion: version } : patch; + return call(memberTok, 'PATCH', `/data/${object}/${id}`, body, headers); + } + const query = carrier === 'expectedVersion' ? `?expectedVersion=${encodeURIComponent(version)}` : ''; + return call(memberTok, 'DELETE', `/data/${object}/${id}${query}`, undefined, headers); + }; + + const stored = (object: string, id: string) => ql.findOne(object, { where: { id }, context: SYS }); + const userId = async (email: string): Promise => + String((await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id ?? ''); + + /** A new organization, created and made active by its owner. */ + const organization = async (ownerEmail: string, name: string, slug: string): Promise => { + const owner = await stack.signUp(ownerEmail); + const created = await stack.apiAs(owner, 'POST', '/auth/organization/create', { name, slug }); + expect(created.status, `org create ${slug}: ${await created.clone().text()}`).toBe(200); + const id = String(((await created.json()) as { id: string }).id); + const active = await stack.apiAs(owner, 'POST', '/auth/organization/set-active', { organizationId: id }); + expect(active.status, `set-active ${slug}: ${await active.clone().text()}`).toBe(200); + return id; + }; + + beforeAll(async () => { + stack = await bootShowcase({ multiTenant: 'posture-only' }); + ql = await stack.kernel.getServiceAsync('objectql'); + + orgA = await organization(OWNER_A, 'OCC Wall A', 'occ-wall-a'); + orgB = await organization(OWNER_B, 'OCC Wall B', 'occ-wall-b'); + + // A plain member of organization B, active in it. + memberTok = await stack.signUp(MEMBER_B); + const memberId = await userId(MEMBER_B); + expect(memberId, 'member provisioned').toBeTruthy(); + await ql.insert('sys_member', { user_id: memberId, organization_id: orgB, role: 'member' }, { context: SYS }); + const active = await stack.apiAs(memberTok, 'POST', '/auth/organization/set-active', { organizationId: orgB }); + expect(active.status, `member set-active: ${await active.clone().text()}`).toBe(200); + + // Organization A's rows, which the wall hides from the member. + const ownerA = await userId(OWNER_A); + accountA = idOf(await ql.insert('showcase_account', { name: 'Wall A account', status: 'active', organization_id: orgA }, { context: SYS })); + noteA = idOf(await ql.insert('showcase_private_note', { title: 'Wall A note', owner_id: ownerA, organization_id: orgA }, { context: SYS })); + + // Organization B's rows, which the member reads. + accountB = idOf(await ql.insert('showcase_account', { name: 'Wall B account', status: 'active', organization_id: orgB }, { context: SYS })); + const own = await call(memberTok, 'POST', '/data/showcase_private_note', { title: 'Wall B note' }); + expect(own.status, `member note created — ${JSON.stringify(own.body)}`).toBeLessThan(300); + noteB = idOf(own.body); + expect(accountA && noteA && accountB && noteB, 'every row carries an id').toBeTruthy(); + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + it('guard the guard: a real wall, a member of B who is no platform administrator, reading B\'s rows and neither of A\'s', async () => { + const tenancy = await stack.kernel.getServiceAsync<{ posture: string; isolationActive: boolean; degraded: boolean }>('tenancy'); + expect([tenancy.posture, tenancy.isolationActive, tenancy.degraded]).toEqual(['isolated', true, false]); + expect(orgA).not.toBe(orgB); + const session = (await (await stack.apiAs(memberTok, 'GET', '/auth/get-session')).json()) as any; + expect(session.user?.isPlatformAdmin).not.toBe(true); + expect(session.session?.activeOrganizationId).toBe(orgB); + + for (const [object, id] of [['showcase_account', accountA], ['showcase_private_note', noteA]]) { + const got = await call(memberTok, 'GET', `/data/${object}/${id}`); + expect(got.status, `${object} of A — ${JSON.stringify(got.body)}`).toBe(404); + expect((await stored(object, id))?.organization_id, `${object}: the row lives in A`).toBe(orgA); + } + for (const [object, id] of [['showcase_account', accountB], ['showcase_private_note', noteB]]) { + const got = await call(memberTok, 'GET', `/data/${object}/${id}`); + expect(got.status, `${object} of B — ${JSON.stringify(got.body)}`).toBe(200); + } + }); + + it('a row of A answers a stale token exactly as it answers no token, and as an id naming no row does', async () => { + const families = [ + { object: 'showcase_account', id: accountA, patch: { name: 'x' }, status: { PATCH: 403, DELETE: 403 } }, + { object: 'showcase_private_note', id: noteA, patch: { title: 'x' }, status: { PATCH: 404, DELETE: 403 } }, + ]; + for (const { object, id, patch, status } of families) { + const before = await stored(object, id); + for (const method of ['PATCH', 'DELETE'] as const) { + const noToken = await versioned(method, object, id, patch, 'none'); + expect(noToken.status, `${method} ${object} without a token — ${JSON.stringify(noToken.body)}`).toBe(status[method]); + for (const carrier of ['If-Match', 'expectedVersion'] as const) { + const where = `${method} ${object} (${carrier})`; + const hit = await versioned(method, object, id, patch, carrier); + const miss = await versioned(method, object, MISSING, patch, carrier); + expect(withoutId(hit, id), `${where}: the answer without a token`).toEqual(withoutId(noToken, id)); + expect(withoutId(miss, MISSING), `${where}: the answer for no row`).toEqual(withoutId(hit, id)); + } + } + expect(await stored(object, id), `${object}: refused writes leave the row as it was`).toEqual(before); + } + }); + + it('control: on rows of B the stale token still answers 409 carrying the row, and the current token updates', async () => { + const accountBefore = await stored('showcase_account', accountB); + for (const method of ['PATCH', 'DELETE'] as const) { + const stale = await versioned(method, 'showcase_account', accountB, { name: 'x' }, 'If-Match'); + expect({ status: stale.status, code: codeOf(stale) }, `${method} account — ${JSON.stringify(stale.body)}`) + .toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect((stale.body as any)?.currentRecord?.id).toBe(accountB); + } + expect(await stored('showcase_account', accountB)).toEqual(accountBefore); + + const read = await call(memberTok, 'GET', `/data/showcase_private_note/${noteB}`); + const current = String((read.body as any)?.record?.updated_at ?? ''); + expect(current, `the note serves a version token — ${JSON.stringify(read.body)}`).toBeTruthy(); + for (const carrier of ['If-Match', 'expectedVersion'] as const) { + const stale = await versioned('PATCH', 'showcase_private_note', noteB, { title: 'x' }, carrier); + expect({ status: stale.status, code: codeOf(stale) }, `${carrier} — ${JSON.stringify(stale.body)}`) + .toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect((stale.body as any)?.currentRecord?.id).toBe(noteB); + } + const fresh = await versioned('PATCH', 'showcase_private_note', noteB, { title: 'Wall B note, renamed' }, 'If-Match', current); + expect(fresh.status, JSON.stringify(fresh.body)).toBe(200); + expect((await stored('showcase_private_note', noteB))?.title).toBe('Wall B note, renamed'); + }); +}); 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 index d0dda6dc79e..571bc61678e 100644 --- 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 @@ -17,8 +17,13 @@ // 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. +// 4. ⭐ with a stale version token — `If-Match`, the PATCH body's +// `expectedVersion`, the DELETE's `?expectedVersion` — PATCH and DELETE +// answer the SAME status, code and sentence for both ids, and the answer +// the same request gets without a token. So do two rows the member's row +// scope hides on objects it reads (private OWD): an inquiry, which it may +// not update, and a note, which it may update but not delete. The version +// comparison answers only for a row the caller reads. // // Controls, so a green run is not "everything refuses": // @@ -32,8 +37,14 @@ // - 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; +// - with a stale token the edit-grant holder gets `409 CONCURRENT_UPDATE` +// carrying the row, and with the current token it updates (200); the plain +// member gets the same `409` on a row it reads but may not write; // - a refused write leaves the row untouched. // +// The organization wall's half of item 4 — a row in another organization — is +// pinned in `occ-stale-token-organization-wall.dogfood.test.ts`. +// // 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 @@ -61,6 +72,12 @@ const codeOf = (a: Answer): unknown => (a.body as any)?.code ?? (a.body as any)? const withoutId = (a: Answer, id: string): Answer => JSON.parse(JSON.stringify(a).split(id).join('ID')) as Answer; +/** A version token no row carries: one read before a write that moved the row on. */ +const STALE = '2001-01-01T00:00:00.000Z'; + +/** Where a request carries its version token: nowhere, the `If-Match` header, or `expectedVersion`. */ +type Carrier = 'none' | 'If-Match' | 'expectedVersion'; + /** The id a create answered, wherever the envelope carries it. */ const createdId = (a: Answer): string => { const b = a.body as any; @@ -84,6 +101,8 @@ describe('update by id: one refusal whether or not the id exists, as delete', () const existing = new Map(); let foreignNoteId = ''; let ownNoteId = ''; + let foreignInquiryId = ''; + let accountId = ''; const call = async ( token: string, @@ -104,6 +123,29 @@ describe('update by id: one refusal whether or not the id exists, as delete', () const stored = (object: string, id: string) => ql.findOne(object, { where: { id }, context: SYS }); + /** + * A by-id PATCH or DELETE carrying `version` the way `carrier` names: the + * PATCH body's `expectedVersion`, the DELETE's `?expectedVersion`, or the + * `If-Match` header on either. + */ + const versioned = ( + token: string, + method: 'PATCH' | 'DELETE', + object: string, + id: string, + patch: Record, + carrier: Carrier, + version = STALE, + ): Promise => { + const headers = carrier === 'If-Match' ? { 'If-Match': version } : {}; + if (method === 'PATCH') { + const body = carrier === 'expectedVersion' ? { ...patch, expectedVersion: version } : patch; + return call(token, 'PATCH', `/data/${object}/${id}`, body, headers); + } + const query = carrier === 'expectedVersion' ? `?expectedVersion=${encodeURIComponent(version)}` : ''; + return call(token, 'DELETE', `/data/${object}/${id}${query}`, undefined, headers); + }; + beforeAll(async () => { // The federated objects' remote tables — the showcase's own fixture // provisioner, run the way `showcase-external-autoconnect` runs it. @@ -133,6 +175,14 @@ describe('update by id: one refusal whether or not the id exists, as delete', () expect(own.status, `member note created — ${JSON.stringify(own.body)}`).toBeLessThan(300); ownNoteId = createdId(own); expect(foreignNoteId && ownNoteId, 'both notes carry an id').toBeTruthy(); + + const inquiry = await call(adminTok, 'POST', '/data/showcase_inquiry', { + name: 'Admin inquiry', email: 'admin-inquiry@verify.test', message: 'owned by the administrator', + }); + expect(inquiry.status, `admin inquiry created — ${JSON.stringify(inquiry.body)}`).toBeLessThan(300); + foreignInquiryId = createdId(inquiry); + accountId = String((await ql.find('showcase_account', { limit: 1, context: SYS }))?.[0]?.id ?? ''); + expect(foreignInquiryId && accountId, 'an inquiry and an account to aim at').toBeTruthy(); }, 300_000); afterAll(async () => { @@ -177,18 +227,65 @@ describe('update by id: one refusal whether or not the id exists, as delete', () }); } - 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)!; + it('guard the guard: the member reads neither hidden row, and reads the account it may not write', async () => { + for (const [object, id] of [['showcase_inquiry', foreignInquiryId], ['showcase_private_note', foreignNoteId]]) { + const got = await call(memberTok, 'GET', `/data/${object}/${id}`); + expect(got.status, `${object} — ${JSON.stringify(got.body)}`).toBe(404); + expect(await stored(object, id), `${object}: the row is there`).toBeTruthy(); + } + const account = await call(memberTok, 'GET', `/data/showcase_account/${accountId}`); + expect(account.status, JSON.stringify(account.body)).toBe(200); + }); + + it('with a stale version token, PATCH and DELETE answer one status, code and sentence for a row and for no row — the answer without a token', async () => { + const families: Array<{ object: string; id: string; patch: Record; status: { PATCH: number; DELETE: number } }> = [ + { object: 'showcase_project_membership', id: existing.get('showcase_project_membership')!, patch: PATCHES.showcase_project_membership, status: { PATCH: 403, DELETE: 403 } }, + { object: 'sys_user_permission_set', id: existing.get('sys_user_permission_set')!, patch: PATCHES.sys_user_permission_set, status: { PATCH: 403, DELETE: 403 } }, + // Rows the member's row scope hides, on objects it reads: no update grant on + // the inquiry, and update without delete on the note. + { object: 'showcase_inquiry', id: foreignInquiryId, patch: { message: 'x' }, status: { PATCH: 403, DELETE: 403 } }, + { object: 'showcase_private_note', id: foreignNoteId, patch: { title: 'x' }, status: { PATCH: 404, DELETE: 403 } }, + ]; + for (const { object, id, patch, status } of families) { + const before = await stored(object, id); 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) }); + const noToken = await versioned(memberTok, method, object, id, patch, 'none'); + expect(noToken.status, `${method} ${object} without a token — ${JSON.stringify(noToken.body)}`).toBe(status[method]); + for (const carrier of ['If-Match', 'expectedVersion'] as const) { + const where = `${method} ${object} (${carrier})`; + const hit = await versioned(memberTok, method, object, id, patch, carrier); + const miss = await versioned(memberTok, method, object, MISSING, patch, carrier); + expect(withoutId(hit, id), `${where}: the answer without a token`).toEqual(withoutId(noToken, id)); + expect(withoutId(miss, MISSING), `${where}: the answer for no row`).toEqual(withoutId(hit, id)); + } } + expect(await stored(object, id), `${object}: refused writes leave the row as it was`).toEqual(before); + } + }); + + it('control: a stale token on a row the caller reads still answers 409 carrying the row; the current token updates', async () => { + const id = existing.get('showcase_project_membership')!; + const read = await call(editorTok, 'GET', `/data/showcase_project_membership/${id}`); + expect(read.status, JSON.stringify(read.body)).toBe(200); + const current = String((read.body as any)?.record?.updated_at ?? ''); + expect(current, 'the row serves a version token').toBeTruthy(); + for (const carrier of ['If-Match', 'expectedVersion'] as const) { + const stale = await versioned(editorTok, 'PATCH', 'showcase_project_membership', id, { engagement: 'reviewer' }, carrier); + expect({ status: stale.status, code: codeOf(stale) }, `${carrier} — ${JSON.stringify(stale.body)}`).toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect((stale.body as any)?.currentRecord?.id).toBe(id); + } + const fresh = await versioned(editorTok, 'PATCH', 'showcase_project_membership', id, { engagement: 'reviewer' }, 'If-Match', current); + expect(fresh.status, JSON.stringify(fresh.body)).toBe(200); + + // The plain member reads the account and may not write it: the 409 stays, and + // the row it carries is one the member reads anyway. + const before = await stored('showcase_account', accountId); + for (const method of ['PATCH', 'DELETE'] as const) { + const stale = await versioned(memberTok, method, 'showcase_account', accountId, { name: 'x' }, 'If-Match'); + expect({ status: stale.status, code: codeOf(stale) }, `${method} — ${JSON.stringify(stale.body)}`).toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect((stale.body as any)?.currentRecord?.id).toBe(accountId); } + expect(await stored('showcase_account', accountId)).toEqual(before); }); it('control: a caller holding the edit grant updates an in-scope row and gets 404 for no row', async () => { 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 ea7cd477a57..c4173fa2ab2 100644 --- a/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts +++ b/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts @@ -23,6 +23,11 @@ * 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. + * - **The same, on an object the caller reads but may not write:** the write + * door answers first, so the control is its `403` — and a stale token, on + * the hidden row and on an id naming no row, gets that same `403` body. The + * comparison gives no answer of its own for a row the caller does not read. + * A row the caller reads there still conflicts `409`. * - **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 @@ -60,15 +65,23 @@ const DATA = `${PREFIX}/data`; const SCOPED = 'occ_vis_ledger'; /** An object no row or field rule names: the positive control. */ const PLAIN = 'occ_vis_plain'; +/** An object the member reads but may not write, under the same row scope as {@link SCOPED}. */ +const READONLY = 'occ_vis_readonly'; 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 } }, + objects: { + '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + [READONLY]: { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }, + }, fields: { [`${SCOPED}.secret`]: { readable: false, editable: false } }, - rowLevelSecurity: [{ name: 'region_scope', object: SCOPED, operation: 'all', using: "record.region != 'HIDDEN'" }], + rowLevelSecurity: [ + { name: 'region_scope', object: SCOPED, operation: 'all', using: "record.region != 'HIDDEN'" }, + { name: 'region_scope_readonly', object: READONLY, operation: 'all', using: "record.region != 'HIDDEN'" }, + ], }); const MEMBER_CTX = { userId: 'usr_member', positions: [] as string[], permissions: [MEMBER_SET.name], posture: 'MEMBER' as const }; @@ -82,6 +95,10 @@ const HIDDEN_ID = 'h1'; const MASKED_ID = 'v1'; /** The positive control's row. */ const PLAIN_ID = 'p1'; +/** The row the member reads on {@link READONLY}. */ +const READABLE_ID = 'r1'; +/** An id that names no row anywhere. */ +const MISSING_ID = 'occ_vis_missing'; /** A by-id read's options, typed to the engine's contract. */ const byId = (id: string, context: EngineQueryOptions['context']): EngineQueryOptions => ({ where: { id }, context }); @@ -217,6 +234,15 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read secret: { name: 'secret', type: 'text' }, }, }, + { + name: READONLY, + label: 'Read-only ledger', + sharingModel: 'public_read_write', + fields: { + title: { name: 'title', type: 'text' }, + region: { name: 'region', type: 'text' }, + }, + }, { name: PLAIN, label: 'Plain', @@ -264,7 +290,7 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read /** 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) { + for (const [object, id] of [[SCOPED, HIDDEN_ID], [SCOPED, MASKED_ID], [PLAIN, PLAIN_ID], [READONLY, HIDDEN_ID], [READONLY, READABLE_ID]] as const) { if (await systemRow(object, id)) await engine.delete(object, { where: { id }, context: SYS_CTX } as any); } await engine.insert(SCOPED, [ @@ -272,8 +298,15 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read { 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); + await engine.insert(READONLY, [ + { id: HIDDEN_ID, title: 'hidden', region: 'HIDDEN' }, + { id: READABLE_ID, title: 'readable', region: 'NA' }, + ], { context: SYS_CTX } as never); } + /** The body with the id it names written out, so the answers for two ids compare. */ + const withoutId = (body: unknown, id: string) => JSON.parse(JSON.stringify(body ?? null).split(id).join('ID')); + 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`. */ @@ -310,6 +343,28 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read expect(await systemRow(SCOPED, HIDDEN_ID)).toMatchObject({ title: 'hidden', secret: 'hidden-secret' }); }, 60_000); + it('a row RLS hides on an object the caller reads but may not write: a stale token answers the write door\'s 403, as the control does, and so does an id naming no row', async () => { + await seed(); + const control = await door().send(verb, READONLY, HIDDEN_ID); + expect({ status: control.status, code: codeOf(control.body) }, JSON.stringify(control.body)) + .toEqual({ status: 403, code: 'PERMISSION_DENIED' }); + + const stale = await door().send(verb, READONLY, HIDDEN_ID, ifMatch(STALE)); + expect(stale.status, JSON.stringify(stale.body)).toBe(control.status); + expect(stale.body).toEqual(control.body); + const missing = await door().send(verb, READONLY, MISSING_ID, ifMatch(STALE)); + expect(missing.status, JSON.stringify(missing.body)).toBe(control.status); + expect(withoutId(missing.body, MISSING_ID)).toEqual(withoutId(control.body, HIDDEN_ID)); + expect(await systemRow(READONLY, HIDDEN_ID)).toMatchObject({ title: 'hidden' }); + + // A row the caller reads on the same object still conflicts: the record it is + // shown is one it can GET anyway. + const readable = await door().send(verb, READONLY, READABLE_ID, ifMatch(STALE)); + expect({ status: readable.status, code: codeOf(readable.body) }, JSON.stringify(readable.body)) + .toEqual({ status: 409, code: 'CONCURRENT_UPDATE' }); + expect(await systemRow(READONLY, READABLE_ID)).toMatchObject({ title: 'readable' }); + }, 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); From a3bd49c9a7cb94adbdd88c8b95376886b1bc9ef5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 14:15:43 +0000 Subject: [PATCH 3/5] test(dogfood): type the version-token header record the request helper takes Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../test/occ-stale-token-organization-wall.dogfood.test.ts | 2 +- .../qa/dogfood/test/update-by-id-refusal-order.dogfood.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts b/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts index 4894844fead..3aa969d3020 100644 --- a/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts +++ b/packages/qa/dogfood/test/occ-stale-token-organization-wall.dogfood.test.ts @@ -100,7 +100,7 @@ describe('behind the organization wall, a stale version token on another organiz carrier: Carrier, version = STALE, ): Promise => { - const headers = carrier === 'If-Match' ? { 'If-Match': version } : {}; + const headers: Record = carrier === 'If-Match' ? { 'If-Match': version } : {}; if (method === 'PATCH') { const body = carrier === 'expectedVersion' ? { ...patch, expectedVersion: version } : patch; return call(memberTok, 'PATCH', `/data/${object}/${id}`, body, headers); 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 index 571bc61678e..e0c97523736 100644 --- 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 @@ -137,7 +137,7 @@ describe('update by id: one refusal whether or not the id exists, as delete', () carrier: Carrier, version = STALE, ): Promise => { - const headers = carrier === 'If-Match' ? { 'If-Match': version } : {}; + const headers: Record = carrier === 'If-Match' ? { 'If-Match': version } : {}; if (method === 'PATCH') { const body = carrier === 'expectedVersion' ? { ...patch, expectedVersion: version } : patch; return call(token, 'PATCH', `/data/${object}/${id}`, body, headers); From ae97f57ba42fb3b3af41f3bb877d5a2cc06808a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 14:26:37 +0000 Subject: [PATCH 4/5] docs(metadata-protocol): the record-reads module header names the one body changed since the move Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- packages/metadata-protocol/src/protocol-record-reads.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/metadata-protocol/src/protocol-record-reads.ts b/packages/metadata-protocol/src/protocol-record-reads.ts index d41e4636b0b..13a2ceb0f8e 100644 --- a/packages/metadata-protocol/src/protocol-record-reads.ts +++ b/packages/metadata-protocol/src/protocol-record-reads.ts @@ -21,7 +21,10 @@ * re-indented one level and otherwise unedited: the same reads, made as the * same principal, refusing exactly where they did with the same codes and * messages. The module-level declarations only these bodies read (the OCC - * version-token reading) moved with them, also unedited. + * version-token reading) moved with them, also unedited. One body has changed + * since: `assertVersionOf`'s mismatch branch now hands a row the caller's own + * read does not return to the write door instead of answering it (its + * docblock in `protocol.ts` says why). * * ## How the moved bodies reach the class * From 8372e0a15d5871e41ad25c8361f7168e4639fb12 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 11:34:45 +0000 Subject: [PATCH 5/5] fix(metadata-protocol): the by-id OCC comparison hands over only a row absent from the caller's read A caller re-read that throws now propagates as raised again, refusals included; only a read that returns no row leaves the answer to the write door. The docblocks, the changeset and the http-protocol paragraph describe that absent-row case only. Pins: a write grant without a read grant keeps its stale-token write from running at the protocol double and at the runtime doors; the dogfood families whose read is refused compare status and code. Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../harden-occ-unreadable-row-write-door.md | 8 +-- .../docs/protocol/kernel/http-protocol.mdx | 9 +-- .../src/protocol-record-reads.ts | 37 +++-------- .../src/protocol.record-not-found.test.ts | 59 ++++++++++------- packages/metadata-protocol/src/protocol.ts | 63 +++++++++---------- ...update-by-id-refusal-order.dogfood.test.ts | 47 +++++++++----- .../data-conflict-caller-visibility.test.ts | 44 ++++++++++++- 7 files changed, 158 insertions(+), 109 deletions(-) diff --git a/.changeset/harden-occ-unreadable-row-write-door.md b/.changeset/harden-occ-unreadable-row-write-door.md index 0ef04c36db4..c730dfbc03b 100644 --- a/.changeset/harden-occ-unreadable-row-write-door.md +++ b/.changeset/harden-occ-unreadable-row-write-door.md @@ -2,12 +2,12 @@ '@objectstack/metadata-protocol': patch --- -fix(metadata-protocol): the by-id OCC comparison answers only for a row the caller reads; every other row gets the write door's answer +fix(metadata-protocol): the by-id OCC comparison hands a row absent from the caller's read to the write door Clause-②: no Metadata-protocol: none -`updateData` and `deleteData` compare a caller's version token (`If-Match`, or `expectedVersion`) before the write is authorized. On a mismatch they re-read the row as the caller. When that read returns no row, or refuses with a declared client-band status, the comparison is now dropped and the request goes on to the write, whose door answers it: the same status, code and sentence the request gets without a token, and the same a token on an id naming no row gets. This restores the governing text for the write doors (ruling of record 5985885287). +`updateData` and `deleteData` compare a caller's version token (`If-Match`, or `expectedVersion`) before the write is authorized. On a mismatch they re-read the row as the caller. When that read returns no row, the comparison is now dropped and the request goes on to the write, whose door answers it: the answer the request gets without a token, and the answer a token on an id naming no row gets. This restores the governing text for the write doors (ruling of record 5985885287). -- **Unchanged:** a row the caller reads still answers `409 CONCURRENT_UPDATE` with that row as `currentRecord`, including a row the caller reads but may not write. A matching token, or none, reads nothing more than before. A read that faults (a server-band status, or none declared) still propagates as raised. -- **The write door's answers:** a caller who may not update or delete the object gets that refusal (`403`), token or none; a caller who may write the object gets `404 RECORD_NOT_FOUND` for a row its read does not return, as for an id naming no row. +- **Unchanged:** a row the caller reads still answers `409 CONCURRENT_UPDATE` with that row as `currentRecord`, including a row the caller reads but may not write. A caller read that throws — a refusal or a fault — propagates as raised, as before. A matching token, or none, reads nothing more than before. +- **The write door's answers for a row absent from the caller's read:** a caller who may not update or delete the object gets that refusal (`403`); a caller who may write the object gets `404 RECORD_NOT_FOUND`, as for an id naming no row. diff --git a/content/docs/protocol/kernel/http-protocol.mdx b/content/docs/protocol/kernel/http-protocol.mdx index 63e0c988453..246e03db861 100644 --- a/content/docs/protocol/kernel/http-protocol.mdx +++ b/content/docs/protocol/kernel/http-protocol.mdx @@ -753,10 +753,11 @@ each row of `updateMany` and of `batch` with `operation: "update"`. A caller who update the object gets `404 RECORD_NOT_FOUND` for an id that names no record, and for a record its row-level access hides. -A version token (`If-Match`, or `expectedVersion`) changes none of these answers. An -update or delete carrying a stale token is answered `409 CONCURRENT_UPDATE` only for a -record the caller can read; for any other id it gets exactly what the same request -without a token gets. +A version token (`If-Match`, or `expectedVersion`) is compared only against a record the +caller can read: a stale token on such a record is answered `409 CONCURRENT_UPDATE`, +whether or not the caller may write it. For a record the caller's read does not return, +and for an id that names no record, a stale token changes none of the answers above. A +caller refused reading the object gets that read refusal. ### Delete Record diff --git a/packages/metadata-protocol/src/protocol-record-reads.ts b/packages/metadata-protocol/src/protocol-record-reads.ts index 13a2ceb0f8e..78150561999 100644 --- a/packages/metadata-protocol/src/protocol-record-reads.ts +++ b/packages/metadata-protocol/src/protocol-record-reads.ts @@ -23,8 +23,8 @@ * messages. The module-level declarations only these bodies read (the OCC * version-token reading) moved with them, also unedited. One body has changed * since: `assertVersionOf`'s mismatch branch now hands a row the caller's own - * read does not return to the write door instead of answering it (its - * docblock in `protocol.ts` says why). + * read returns no row for to the write door instead of answering it (its + * docblock in `protocol.ts` says why); a read that throws still propagates. * * ## How the moved bodies reach the class * @@ -53,7 +53,6 @@ import type { ObjectStackProtocolImplementation } from './protocol.js'; import { ConcurrentUpdateError, MalformedVersionTokenError, canonicalVersionInstant } from './protocol.js'; import { recordNotFoundError } from '@objectstack/core'; -import { declaredHttpStatus } from '@objectstack/types'; import { collectInternalWriteResponseFields, omitInternalFieldsFromWriteResponse } from './write-response-internal-fields.js'; import { isMissingTableError } from '@objectstack/metadata/errors'; import { canServeApiOperation, resolveSearchFieldResolution } from '@objectstack/spec/data'; @@ -173,19 +172,6 @@ function versionTokensAgree(current: NormalisedVersion, expected: NormalisedVers return current.token === expected.token; } -/** - * Did the caller's own read of a row REFUSE, rather than fault? A refusal - * declares a client-band status (`4xx`, in either spelling - * `declaredHttpStatus` reads) — the read door's verdict on this caller, which - * says nothing about the row. A server-band status, or none at all (a bare - * driver `Error`), is a fault, and the OCC comparison lets it propagate as - * raised rather than read it as "the caller reads no row". - */ -function isCallerReadRefusal(e: unknown): boolean { - const status = declaredHttpStatus(e); - return status !== undefined && status < 500; -} - /** * The `ObjectStackProtocolImplementation` members the functions below read — * every one of them, and no other; each function's `this` is the `Pick` of the @@ -244,18 +230,13 @@ export async function assertVersionOf( // it names, turning a resolvable conflict into an unresolvable one. const currentVersion = currentToken.instant ?? currentToken.token; const expectedVersionText = expected.instant ?? expected.token; - // A row the caller's own read does not return — absent, or the read - // refused — is no conflict this gate may report, and no answer of its - // own either: the comparison is dropped and the write door answers, - // exactly as it answers the same request without a token. The - // method's docblock in `protocol.ts` carries the reasoning. - let visible: unknown; - try { - visible = await this.readAsCaller(object, id, context); - } catch (e) { - if (!isCallerReadRefusal(e)) throw e; - return; - } + // A row the caller's own read RETURNS NO ROW for is no conflict this + // gate may report, and no answer of its own either: the comparison is + // dropped and the write door answers, as it answers the same request + // without a token. ONLY an absent row: a read that throws — a refusal + // or a fault — propagates as raised. The method's docblock in + // `protocol.ts` carries the reasoning. + const visible = await this.readAsCaller(object, id, context); if (!visible) return; throw new ConcurrentUpdateError({ currentVersion, 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 47d1001d205..d621af0aca7 100644 --- a/packages/metadata-protocol/src/protocol.record-not-found.test.ts +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -384,19 +384,23 @@ describe('[#4435] deleteManyData reports per id, not per request', () => { }); }); -describe('a stale version token on a row the caller does not read gets the write door\'s answer, as no token does', () => { +describe('a stale version token on a row absent from the caller\'s read gets the write door\'s answer, as no token does', () => { // The by-id OCC comparison runs before the write is authorized. When the // tokens disagree it re-reads the row as the caller, and a row that read - // does not return — absent, or the read refused — leaves the answer to the - // write door: what the same request without a token gets, status, code and - // sentence, which is also what a token on an id naming no row gets (ruling - // of record 5985885287: a row the caller cannot read answers what a - // nonexistent id answers, on update and delete alike). + // RETURNS NO ROW for leaves the answer to the write door: what the same + // request without a token gets, status, code and sentence, which is also + // what a token on an id naming no row gets (ruling of record 5985885287: a + // row the caller cannot read answers what a nonexistent id answers, on + // update and delete alike). Only an absent row is handed over: a caller + // read that THROWS — a refusal or a fault — propagates as raised. const CURRENT = '2026-10-30T08:00:00.000Z'; const STALE = '2001-01-01T00:00:00.000Z'; const CALLER = { userId: 'u1' }; const MISSING = 'definitely_not_a_row'; const row = (id: string) => ({ id, title: 'x', updated_at: CURRENT }); + /** The read door's refusal of a caller without the object's read grant. */ + const readRefusalError = () => + Object.assign(new Error('Reading this object is not permitted.'), { code: 'PERMISSION_DENIED', status: 403 }); /** A refused call as the doors render it: status, code and sentence. */ const answerOf = (run: () => Promise) => run().then( @@ -423,21 +427,31 @@ describe('a stale version token on a row the caller does not read gets the write expect(store.get('real')).toEqual(row('real')); }); - it('the caller\'s read REFUSES: the write door\'s refusal answers, never the read\'s', async () => { - // A declared client-band status in either spelling is a refusal. - for (const readRefusal of [ - () => Object.assign(new Error('Reading this object is not permitted.'), { code: 'PERMISSION_DENIED', status: 403 }), - () => Object.assign(new Error('Reading this object is not permitted.'), { code: 'PERMISSION_DENIED', statusCode: 403 }), - ]) { - const { p, store } = makeProtocol({ real: row('real') }, { refuse: 'u1', readRefusal }); - const stale = await answerOf(() => write(p, 'real', STALE)); - const noToken = await answerOf(() => write(p, 'real')); - const missing = await answerOf(() => write(p, MISSING, STALE)); - expect(stale).toEqual(noToken); - expect(stale.message).toBe(writeRefusal().message); - expect(withoutId(missing, MISSING)).toEqual(withoutId(stale, 'real')); - expect(store.get('real')).toEqual(row('real')); - } + it('the caller\'s read REFUSES and the write is refused too: the read refusal propagates, with the write\'s status and code', async () => { + // A caller holding neither grant. The refused read is the answer, so its + // sentence is the read door's: status and code are compared here, the + // sentence deliberately is not. + const { p, store } = makeProtocol({ real: row('real') }, { refuse: 'u1', readRefusal: readRefusalError }); + const stale = await answerOf(() => write(p, 'real', STALE)); + const noToken = await answerOf(() => write(p, 'real')); + const missing = await answerOf(() => write(p, MISSING, STALE)); + expect(stale).toEqual({ code: 'PERMISSION_DENIED', status: 403, message: readRefusalError().message }); + expect({ code: noToken.code, status: noToken.status }).toEqual({ code: stale.code, status: stale.status }); + expect({ code: missing.code, status: missing.status }).toEqual({ code: stale.code, status: stale.status }); + expect(store.get('real')).toEqual(row('real')); + }); + + it('the caller\'s read REFUSES and the write door admits the write (a write grant without a read grant): the read refusal propagates and nothing is written', async () => { + const { p, store, update, del } = makeProtocol({ real: row('real') }, { readRefusal: readRefusalError }); + await expect(write(p, 'real', STALE)).rejects.toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); + expect(update).not.toHaveBeenCalled(); + expect(del).not.toHaveBeenCalled(); + expect(store.get('real')).toEqual(row('real')); + // Control: without a token the same double admits the same write, so the + // pin above is not vacuous. + await write(p, 'real'); + if (verb === 'updateData') expect(store.get('real')).toMatchObject({ title: 'y' }); + else expect(store.has('real')).toBe(false); }); it('a hidden row, the write admitted up to its read question: the write door\'s 404, for the row and for no row alike', async () => { @@ -451,10 +465,11 @@ describe('a stale version token on a row the caller does not read gets the write expect(store.get('real')).toEqual(row('real')); }); - it('a read that FAULTS propagates as raised, and the write is never asked', async () => { + it('a read that FAULTS, or throws any other 4xx, propagates as raised, and the write is never asked', async () => { for (const fault of [ new Error('connection reset'), Object.assign(new Error('store unavailable'), { code: 'SERVICE_UNAVAILABLE', status: 503 }), + Object.assign(new Error('bad read'), { code: 'VALIDATION_FAILED', statusCode: 400 }), ]) { const { p, update, del } = makeProtocol({ real: row('real') }, { readRefusal: () => fault }); await expect(write(p, 'real', STALE)).rejects.toBe(fault); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 8d2f5122639..3be0239dc1c 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3327,9 +3327,10 @@ function simpleHash(str: string): string { * 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"). The record is the row as the - * caller reads it, and a row the caller cannot read is never a conflict: the - * write door answers it, as it answers the same request without a token - * (#23586, see `assertVersionOf`). + * caller reads it, and a row the caller's read returns no row for is never a + * conflict: the write door answers it, as it answers the same request without + * a token; a caller read that is refused propagates as that refusal (#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 @@ -14748,10 +14749,10 @@ export class ObjectStackProtocolImplementation implements // // 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, or that the caller's own read does not return, - // is left to the engine's answer: OCC has never treated a missing - // record as a concurrency conflict, and a row the caller cannot read - // answers what a missing one does ({@link assertVersionOf}). + // probe does not find, or that the caller's own read returns no row + // for, is left to the engine's answer: OCC has never treated a missing + // record as a concurrency conflict, and a row absent from the caller's + // read answers what a missing one does ({@link assertVersionOf}). 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; @@ -14989,7 +14990,8 @@ export class ObjectStackProtocolImplementation implements * 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, token or none. + * refusal whether or not the id exists, as delete does. With a mismatched + * token, a caller whose read of the row is refused gets that read refusal. */ private async probeRecord(object: string, id: string): Promise { return this.engine.findOne(object, { where: { id }, context: { isSystem: true } } as any); @@ -15096,32 +15098,29 @@ export class ObjectStackProtocolImplementation implements * the caller's own read is. `currentVersion` stays the token this * comparison ran on. A caller who reads the row but may not write it * gets this `409` too: the row it is shown is one it can GET anyway; - * - no row, or a read that REFUSES (a declared `4xx`) → no answer from - * this gate at all. The comparison is dropped and the request goes on - * to the write, whose door answers it: the stale token then gets - * exactly the answer the same request without a token gets — status, - * code and sentence — and so does a token on an id that names no row - * ("Record not found → no check" above), so the two cannot answer - * apart. That is the governing text for the write doors (ruling of - * record 5985885287): a row the caller cannot read answers what a - * nonexistent id answers, on update and delete alike. - * - * Why the write door's answer and not one of this gate's own: this gate - * runs BEFORE the write is authorized, so any answer it gives for a row the - * caller cannot read — a `404`, or the read door's refusal rethrown — - * differs from the write door's for every caller the write door refuses - * first (no update or delete grant, the delegated-administration gate on - * the RBAC tables), and from what an id naming no row gets. Handing over - * loses no guard: the write door refuses a by-id write of a row the - * caller cannot read (the security layer asks the read question ahead of - * the write and answers `recordNotFoundError`), and a token that could not - * be compared never lets a write land that the same request without a - * token would not. + * - no row — the read RETURNS none → no answer from this gate at all. + * The comparison is dropped and the request goes on to the write, + * whose door answers it: the stale token then gets exactly the answer + * the same request without a token gets, and so does a token on an id + * that names no row ("Record not found → no check" above). That is the + * governing text for the write doors (ruling of record 5985885287): a + * row the caller cannot read answers what a nonexistent id answers, on + * update and delete alike; + * - a read that THROWS — a refusal (the caller's read grant withheld) or + * a fault — propagates as raised, exactly as it did before the + * absent-row handover existed. A refused read is not an absent row, + * and only an absent row is handed over. + * + * Why the write door's answer for an absent row, and not one of this + * gate's own: this gate runs BEFORE the write is authorized, so a `404` of + * its own differs from the write door's answer for every caller the write + * door refuses first (no update or delete grant), and from what an id + * naming no row gets. For an absent row the write door holds the guard + * itself: it asks the caller's read question ahead of a by-id write, and a + * row that read does not return is answered `recordNotFoundError`. * * 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 FAULTS — - * a server-band status, or none declared at all — propagates as raised: a - * store fault is no verdict about the caller. + * that sends none, reads exactly what it read before. */ private assertVersionOf( object: string, 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 index e0c97523736..2a821c1d452 100644 --- 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 @@ -18,12 +18,17 @@ // answer the same per-row refusal for both ids; // 3. `DELETE` keeps the one answer it already gave for both; // 4. ⭐ with a stale version token — `If-Match`, the PATCH body's -// `expectedVersion`, the DELETE's `?expectedVersion` — PATCH and DELETE -// answer the SAME status, code and sentence for both ids, and the answer -// the same request gets without a token. So do two rows the member's row -// scope hides on objects it reads (private OWD): an inquiry, which it may -// not update, and a note, which it may update but not delete. The version -// comparison answers only for a row the caller reads. +// `expectedVersion`, the DELETE's `?expectedVersion` — two rows the +// member's row scope hides on objects it reads (private OWD: an inquiry, +// which it may not update, and a note, which it may update but not +// delete) answer the SAME status, code and sentence for the row, for no +// row and without a token: the version comparison hands a row absent +// from the caller's read to the write door. On the two objects the member +// may neither read nor write, its read is REFUSED, and a refused read is +// not handed over: the row, no row and no token answer the same status +// and code. Their sentences are not compared — the read door's refusal +// and the write door's may word it differently, a residue aligned by its +// own item. // // Controls, so a green run is not "everything refuses": // @@ -237,26 +242,36 @@ describe('update by id: one refusal whether or not the id exists, as delete', () expect(account.status, JSON.stringify(account.body)).toBe(200); }); - it('with a stale version token, PATCH and DELETE answer one status, code and sentence for a row and for no row — the answer without a token', async () => { - const families: Array<{ object: string; id: string; patch: Record; status: { PATCH: number; DELETE: number } }> = [ - { object: 'showcase_project_membership', id: existing.get('showcase_project_membership')!, patch: PATCHES.showcase_project_membership, status: { PATCH: 403, DELETE: 403 } }, - { object: 'sys_user_permission_set', id: existing.get('sys_user_permission_set')!, patch: PATCHES.sys_user_permission_set, status: { PATCH: 403, DELETE: 403 } }, + it('with a stale version token, PATCH and DELETE answer one answer for a row and for no row — the answer without a token', async () => { + /** + * `whole`: status, code and sentence (the row is absent from the member's + * read, so the write door answers). `status+code`: the member may not read + * the object, its refused read is the answer, and the sentence is + * deliberately not compared. + */ + type Compare = 'whole' | 'status+code'; + const families: Array<{ object: string; id: string; patch: Record; status: { PATCH: number; DELETE: number }; compare: Compare }> = [ + // Objects the member may neither read nor write. + { object: 'showcase_project_membership', id: existing.get('showcase_project_membership')!, patch: PATCHES.showcase_project_membership, status: { PATCH: 403, DELETE: 403 }, compare: 'status+code' }, + { object: 'sys_user_permission_set', id: existing.get('sys_user_permission_set')!, patch: PATCHES.sys_user_permission_set, status: { PATCH: 403, DELETE: 403 }, compare: 'status+code' }, // Rows the member's row scope hides, on objects it reads: no update grant on // the inquiry, and update without delete on the note. - { object: 'showcase_inquiry', id: foreignInquiryId, patch: { message: 'x' }, status: { PATCH: 403, DELETE: 403 } }, - { object: 'showcase_private_note', id: foreignNoteId, patch: { title: 'x' }, status: { PATCH: 404, DELETE: 403 } }, + { object: 'showcase_inquiry', id: foreignInquiryId, patch: { message: 'x' }, status: { PATCH: 403, DELETE: 403 }, compare: 'whole' }, + { object: 'showcase_private_note', id: foreignNoteId, patch: { title: 'x' }, status: { PATCH: 404, DELETE: 403 }, compare: 'whole' }, ]; - for (const { object, id, patch, status } of families) { + const shown = (a: Answer, id: string, compare: Compare): unknown => + compare === 'whole' ? withoutId(a, id) : { status: a.status, code: codeOf(a) }; + for (const { object, id, patch, status, compare } of families) { const before = await stored(object, id); for (const method of ['PATCH', 'DELETE'] as const) { const noToken = await versioned(memberTok, method, object, id, patch, 'none'); expect(noToken.status, `${method} ${object} without a token — ${JSON.stringify(noToken.body)}`).toBe(status[method]); for (const carrier of ['If-Match', 'expectedVersion'] as const) { - const where = `${method} ${object} (${carrier})`; + const where = `${method} ${object} (${carrier}, ${compare})`; const hit = await versioned(memberTok, method, object, id, patch, carrier); const miss = await versioned(memberTok, method, object, MISSING, patch, carrier); - expect(withoutId(hit, id), `${where}: the answer without a token`).toEqual(withoutId(noToken, id)); - expect(withoutId(miss, MISSING), `${where}: the answer for no row`).toEqual(withoutId(hit, id)); + expect(shown(hit, id, compare), `${where}: the answer without a token`).toEqual(shown(noToken, id, compare)); + expect(shown(miss, MISSING, compare), `${where}: the answer for no row`).toEqual(shown(hit, id, compare)); } } expect(await stored(object, id), `${object}: refused writes leave the row as it was`).toEqual(before); 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 c4173fa2ab2..a402188bcec 100644 --- a/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts +++ b/packages/runtime/src/domains/data-conflict-caller-visibility.test.ts @@ -26,8 +26,13 @@ * - **The same, on an object the caller reads but may not write:** the write * door answers first, so the control is its `403` — and a stale token, on * the hidden row and on an id naming no row, gets that same `403` body. The - * comparison gives no answer of its own for a row the caller does not read. - * A row the caller reads there still conflicts `409`. + * comparison gives no answer of its own for a row absent from the + * caller's read. A row the caller reads there still conflicts `409`. + * - **An object the caller may write but not read, with no row filter on + * the write:** the caller's re-read is REFUSED, not empty, and a refusal is + * never handed to the write door: a stale token answers the read door's + * `403` and the row is unchanged, on update and on delete. The control + * without a token lands, so the pin is not vacuous. * - **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 @@ -67,6 +72,8 @@ const SCOPED = 'occ_vis_ledger'; const PLAIN = 'occ_vis_plain'; /** An object the member reads but may not write, under the same row scope as {@link SCOPED}. */ const READONLY = 'occ_vis_readonly'; +/** An object the member may update and delete but not read, with no row rule at all. */ +const WRITEONLY = 'occ_vis_writeonly'; const SYS_CTX = { isSystem: true, userId: 'usr_system' }; @@ -76,6 +83,7 @@ const MEMBER_SET = PermissionSetSchema.parse({ objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, [READONLY]: { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }, + [WRITEONLY]: { allowRead: false, allowCreate: false, allowEdit: true, allowDelete: true }, }, fields: { [`${SCOPED}.secret`]: { readable: false, editable: false } }, rowLevelSecurity: [ @@ -99,6 +107,8 @@ const PLAIN_ID = 'p1'; const READABLE_ID = 'r1'; /** An id that names no row anywhere. */ const MISSING_ID = 'occ_vis_missing'; +/** The row on {@link WRITEONLY}. */ +const WRITEONLY_ID = 'w1'; /** A by-id read's options, typed to the engine's contract. */ const byId = (id: string, context: EngineQueryOptions['context']): EngineQueryOptions => ({ where: { id }, context }); @@ -243,6 +253,14 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read region: { name: 'region', type: 'text' }, }, }, + { + name: WRITEONLY, + label: 'Write-only ledger', + sharingModel: 'public_read_write', + fields: { + title: { name: 'title', type: 'text' }, + }, + }, { name: PLAIN, label: 'Plain', @@ -290,7 +308,7 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read /** 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], [READONLY, HIDDEN_ID], [READONLY, READABLE_ID]] as const) { + for (const [object, id] of [[SCOPED, HIDDEN_ID], [SCOPED, MASKED_ID], [PLAIN, PLAIN_ID], [READONLY, HIDDEN_ID], [READONLY, READABLE_ID], [WRITEONLY, WRITEONLY_ID]] as const) { if (await systemRow(object, id)) await engine.delete(object, { where: { id }, context: SYS_CTX } as any); } await engine.insert(SCOPED, [ @@ -302,6 +320,7 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read { id: HIDDEN_ID, title: 'hidden', region: 'HIDDEN' }, { id: READABLE_ID, title: 'readable', region: 'NA' }, ], { context: SYS_CTX } as never); + await engine.insert(WRITEONLY, [{ id: WRITEONLY_ID, title: 'write-only' }], { context: SYS_CTX } as never); } /** The body with the id it names written out, so the answers for two ids compare. */ @@ -365,6 +384,25 @@ describe('[#23586] a 409 CONCURRENT_UPDATE reports only what the caller may read expect(await systemRow(READONLY, READABLE_ID)).toMatchObject({ title: 'readable' }); }, 60_000); + it('an object the caller may write but not read: a stale token answers the read refusal and the row is unchanged; without a token the write lands', async () => { + await seed(); + const before = await systemRow(WRITEONLY, WRITEONLY_ID); + expect(before).toMatchObject({ title: 'write-only' }); + + const stale = await door().send(verb, WRITEONLY, WRITEONLY_ID, ifMatch(STALE)); + expect({ status: stale.status, code: codeOf(stale.body) }, JSON.stringify(stale.body)) + .toEqual({ status: 403, code: 'PERMISSION_DENIED' }); + expect(JSON.stringify(stale.body)).not.toContain('currentRecord'); + expect(await systemRow(WRITEONLY, WRITEONLY_ID), 'the stale token wrote nothing').toEqual(before); + + // Control: the same request without a token is admitted and lands — + // the caller really holds the write grant. + const plain = await door().send(verb, WRITEONLY, WRITEONLY_ID); + expect(plain.status, JSON.stringify(plain.body)).toBe(200); + if (verb === 'PATCH') expect(await systemRow(WRITEONLY, WRITEONLY_ID)).toMatchObject({ title: 'written' }); + else expect(await systemRow(WRITEONLY, WRITEONLY_ID)).toBeFalsy(); + }, 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);