From 485ab982a74cea559681cb8cf8ed7b82cef0cb9e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 12:18:49 +0000 Subject: [PATCH 1/6] fix(metadata-protocol): batchData's by-id upsert chooses its arm by what the caller can read Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../src/protocol-batch-data.ts | 136 ++++++++++++++---- packages/metadata-protocol/src/protocol.ts | 10 +- 2 files changed, 117 insertions(+), 29 deletions(-) diff --git a/packages/metadata-protocol/src/protocol-batch-data.ts b/packages/metadata-protocol/src/protocol-batch-data.ts index c8edfa78d25..68d52fa28f4 100644 --- a/packages/metadata-protocol/src/protocol-batch-data.ts +++ b/packages/metadata-protocol/src/protocol-batch-data.ts @@ -32,8 +32,9 @@ * path → `runAtomicBatch` → the loop → the builders) do so through the * protocol, exactly as before. * - * The one field the bodies read, `engine`, goes as a GETTER, so each read - * happens when the body makes it: tests replace it on a constructed protocol. + * The two fields the bodies read, `engine` and `getServicesRegistry`, go as + * GETTERS, so each read happens when the body makes it: tests replace them on a + * constructed protocol. * * The other module-level declarations the bodies read stay in `protocol.ts`, * where code that did not move reads them too, and are imported from there: @@ -48,6 +49,7 @@ import type { ObjectStackProtocolImplementation, BatchDataLoopOutcome, BatchData import { byIdUpdateRecord, rowRequiredIdError, toRowApiError } from './protocol.js'; import type { BatchUpdateRequest, BatchUpdateResponse, DeleteManyDataRequest, UpdateManyDataRequest } from '@objectstack/spec/api'; import type { DroppedFieldsEvent } from '@objectstack/spec/data'; +import type { ISecurityService } from '@objectstack/spec/contracts'; import { engineCanRollBack, recordNotFoundError } from '@objectstack/core'; import { omitInternalFieldsFromWriteResponse } from './write-response-internal-fields.js'; @@ -107,6 +109,40 @@ function rowOperationFailureFallback(operation: string): string { return `The ${operation} of this record failed. The reason is in the server log.`; } +/** + * The OBJECT-level half of the question `batchData`'s by-id upsert forks on — + * may the CALLER read `object` at all? — asked of the `security` service's + * `canReadObject`, the engine middleware's own read gate arm for arm + * (`ISecurityService.canReadObject`), with the context the loop's writes run + * under. The ROW-level half is the caller's own `findOne` by id, at the fork. + * + * Asked separately because the middleware answers a caller with no read grant + * by REFUSING the `findOne` (`403`), not with `null`: without this half, a + * caller who may create but not read would have every by-id upsert row refused + * outright, where a create naming the same id is accepted. A `false` here is + * "this caller can read no row of the object", so every row takes the insert + * arm, which `engine.insert` still judges in full. + * + * `true` means only "not refused at this layer" and never widens anything: the + * `findOne` that follows is still the middleware's, row scope included. So a + * context-less in-process call (the read `findOne` makes without a context is + * the one its writes make) is not asked about, and neither is a host whose + * security service is absent or predates the method — `findOne` still + * enforces there, so the absence can only make the answer stricter (a refusal + * fails the row), as `searchAll` reasons for the same method. A + * `canReadObject` that throws fails the row rather than choosing an arm. + */ +async function callerMayReadObject( + this: Pick, + object: string, + context: unknown, +): Promise { + if (context === undefined) return true; + const security = this.getServicesRegistry?.().get('security') as Partial | undefined; + if (typeof security?.canReadObject !== 'function') return true; + return security.canReadObject(object, context as Parameters>[1]); +} + /** * The `ObjectStackProtocolImplementation` members the functions below read — * every one of them, and no other; each function's `this` is the `Pick` of the @@ -123,8 +159,8 @@ export interface BatchDataHost { buildRolledBackBatchResponse: ObjectStackProtocolImplementation['buildRolledBackBatchResponse']; buildUpdateManyResponse: ObjectStackProtocolImplementation['buildUpdateManyResponse']; readonly engine: ObjectStackProtocolImplementation['engine']; + readonly getServicesRegistry: ObjectStackProtocolImplementation['getServicesRegistry']; locateBatchCause: ObjectStackProtocolImplementation['locateBatchCause']; - probeRecord: ObjectStackProtocolImplementation['probeRecord']; reconcileStoppedBatch: ObjectStackProtocolImplementation['reconcileStoppedBatch']; runAtomicBatch: ObjectStackProtocolImplementation['runAtomicBatch']; runAtomicBatchData: ObjectStackProtocolImplementation['runAtomicBatchData']; @@ -262,7 +298,7 @@ export async function runAtomicBatch( /** The body of `ObjectStackProtocolImplementation#runBatchDataLoop`; the method's docblock there is its contract. */ export async function runBatchDataLoop( - this: Pick, + this: Pick, args: { object: string; operation: BatchUpdateRequest['operation']; @@ -287,6 +323,11 @@ export async function runBatchDataLoop( // drop — the upsert arms did exactly that until the patch round. const ctxOpt = context !== undefined ? { context } : {}; + // The object-level half of the upsert fork's question (see the fork and + // `callerMayReadObject`), asked at most once per loop: the object and the + // context are the loop's, so every row gets the same answer. + let objectReadable: Promise | undefined; + // [#4793] `index` is the row's position in the REQUEST array — the // correlation a caller needs for failure rows that carry no id. for (const [index, record] of records.entries()) { @@ -328,28 +369,73 @@ export async function runBatchDataLoop( } case 'upsert': { if (record.id) { - // [#5099] The update-or-insert fork asks EXISTENCE, - // not visibility. `findOne` under the CALLER's - // context is the read RLS/sharing narrows (#3455), - // so an existing row outside the caller's scope - // answered null, took the insert arm, and either - // duplicate-keyed — an authorization/update - // scenario reported as a key collision — or, on a - // store without a unique id constraint, wrote a - // second row. The same `probeRecord` the update branch - // above asks after its write (#4620): it asks the - // database a fact, and whether the caller may WRITE - // the row it proves stays #1994's decision inside - // `engine.update`. + // The update-or-insert fork asks what the CALLER can + // read. On the write doors a row the caller cannot + // read is a row that does not exist (ruling + // 5985885287 on #21771, the governing text for the + // write doors), so such a row takes the INSERT arm — + // what a create naming the same id does — and that + // insert meets the deployment-global primary key and + // answers the `409 UNIQUE_VIOLATION` a plain create + // with that id answers. A row the caller CAN read + // keeps the UPDATE arm, where `engine.update` still + // judges whether the caller may write it. + // + // "Can read" is the read door's question in both its + // halves, under the context the arm's write runs + // with (`ctxOpt`): the object-level admission + // (`callerMayReadObject`, once per loop) and then the + // caller's own `findOne` by id, where RLS, sharing + // and the organization wall all apply. Inside the + // atomic arm that context is the transaction's, so + // the read sees the batch's own uncommitted rows. + // + // ⚠️ This REVERSES #5099 / #5187, by ruling. #5187 + // moved this fork FROM the caller's read TO the + // system-context `probeRecord`, because forking on + // visibility reported an authorization/update + // scenario as a key collision. That trade is now + // ruled the other way, for two reasons: + // + // - forking on existence answered a row the caller + // cannot read differently from an id that names + // nothing — the update arm's refusal (`403`, or the + // by-id pre-image check's `404`) beside a CREATED + // row — so a caller holding create learned that a + // row it cannot read exists, which ruling + // 5985885287 closed on the by-id update and delete + // doors; + // - the key collision #5187 called a misdirection is + // the answer the maintainer ratified on 2026-11-01 + // as the accepted cost of a deployment-global key, + // extending ruling 5934879010 on #21185 (rule 2), + // which `SqlDriver.upsert` already follows for a + // conflict in another organization: it tells the + // caller exactly what a create with that id tells + // it, and nothing more. + // + // ⛔ So a hidden collision gets NO answer of its own: + // no new code, field, detail or sentence, and nothing + // that names another organization. The collision is + // `engine.insert`'s own refusal, reported through the + // same row builder `case 'create'` uses. + // + // #5099's second observation stays true, as a fact + // about the store rather than this fork: on a store + // that does not enforce the primary key + // (`driver-memory` declares it out of scope) the + // insert lands a second row with that id, exactly as + // a create naming that id already does there. // - // The old fallback (update threw → blind insert) - // is gone with the fork's visibility read: with - // existence decided BEFORE the write, a fallback - // insert could only bury a real update failure - // under the duplicate-key error of inserting a row - // just proven to exist — the same masking ADR-0119 - // D4 forbade inside the atomic arm. - const existing = await this.probeRecord(object, record.id); + // #5099's other half is kept unchanged: there is NO + // fallback. The arm is chosen before any write, so an + // update failure surfaces itself and is never buried + // under an insert's error — the masking ADR-0119 D4 + // forbade inside the atomic arm. + objectReadable ??= callerMayReadObject.call(this, object, context); + const existing = (await objectReadable) + ? await this.engine.findOne(object, { where: { id: record.id }, ...ctxOpt } as any) + : null; if (existing) { const dropped: DroppedFieldsEvent[] = []; const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any)); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index b2b42b025ec..8242ac144ed 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -15222,9 +15222,10 @@ export class ObjectStackProtocolImplementation implements * `atomic=false`). It used to change a second — forbidding the upsert * fallback insert, whose failure inside an aborted transaction could only * mask the real cause — until #5099 removed that fallback from BOTH arms: - * the upsert fork is decided by an existence probe before any write, so a - * fallback insert could only bury a real update failure under the - * duplicate-key error of inserting a row just proven to exist. + * the upsert fork is decided before any write (by what the caller can + * read, since the fork's own note says why), so a fallback insert could + * only bury a real update failure under the duplicate-key error of + * inserting a row the caller was just shown to read. */ private runBatchDataLoop(args: { object: string; @@ -15237,10 +15238,11 @@ export class ObjectStackProtocolImplementation implements }): Promise { // [#25467 cut 1] The body moved to `protocol-batch-data.ts` (see `BatchDataHost`). const readEngine = () => this.engine; + const readGetServicesRegistry = () => this.getServicesRegistry; const host: ThisParameterType = { assertRecordExists: (...a) => this.assertRecordExists(...a), get engine() { return readEngine(); }, - probeRecord: (...a) => this.probeRecord(...a), + get getServicesRegistry() { return readGetServicesRegistry(); }, }; return runBatchDataLoop.call(host, args); } From e4ac41c74ef222554f31539c07ee24cdb04895e6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 12:25:01 +0000 Subject: [PATCH 2/6] test: pin the by-id upsert's caller-read fork at the protocol seam and the HTTP door Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .changeset/batch-upsert-by-id-caller-read.md | 14 + .../src/protocol.upsert-existence.test.ts | 221 +++++++---- .../upsert-by-id-caller-read.dogfood.test.ts | 373 ++++++++++++++++++ 3 files changed, 541 insertions(+), 67 deletions(-) create mode 100644 .changeset/batch-upsert-by-id-caller-read.md create mode 100644 packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts diff --git a/.changeset/batch-upsert-by-id-caller-read.md b/.changeset/batch-upsert-by-id-caller-read.md new file mode 100644 index 00000000000..f9108a41c39 --- /dev/null +++ b/.changeset/batch-upsert-by-id-caller-read.md @@ -0,0 +1,14 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): the by-id `upsert` of `batchData` chooses its arm by what the caller can read + +Clause-②: no +Metadata-protocol: none + +Security hardening of `POST /data/:object/batch` with `operation: 'upsert'` (the protocol's `batchData`). A row that carries an `id` now chooses between its update and create arms by what the calling principal can read, the rule the other by-id write doors already follow: a record the caller cannot read is treated as one that does not exist. + +- **Unchanged:** a record the caller can read is updated, under the caller's own write permissions, exactly as before; a row without an `id` is created; atomic batches roll back on the first failing row; system and context-less calls fork as their reads run. +- **Changed:** any other `id` takes the create arm, so the row answers exactly as a `create` naming that `id` answers on the same door. +- **Your fix:** none. A client reconciling batch results reads each row's `errors[0].code` as before. diff --git a/packages/metadata-protocol/src/protocol.upsert-existence.test.ts b/packages/metadata-protocol/src/protocol.upsert-existence.test.ts index 6dd37b77ad3..f34da5689f6 100644 --- a/packages/metadata-protocol/src/protocol.upsert-existence.test.ts +++ b/packages/metadata-protocol/src/protocol.upsert-existence.test.ts @@ -1,28 +1,28 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * [#5099] `batchData`'s upsert fork asks EXISTENCE, not visibility. + * `batchData`'s by-id upsert forks on what the CALLER can read. * - * The fork used to ask `findOne` under the CALLER's context — the read that - * RLS/sharing narrows (#3455). An existing row outside the caller's read scope - * therefore answered `null` and took the INSERT arm: on a store with a unique - * id constraint the insert duplicate-keyed (an authorization/update scenario - * reported as a key collision — the same misdirection class as #5088), and on - * a store without one it wrote a second row. `probeRecord`'s own comment - * (#4435) calls the distinction load-bearing; #5088 put it on the update and - * delete faces of this file; the upsert fork was the one branch still reading - * it backwards (#4620: one reading per file, not three that happen to agree). + * On the write doors a row the caller cannot read is a row that does not exist + * (ruling 5985885287 on #21771). So the fork asks the caller's own read — the + * object-level admission, then `findOne` by id under the caller's context — and + * a row that read does not return takes the INSERT arm, exactly as a create + * naming the same id does. That insert meets the deployment-global primary key + * and answers what a plain create answers: the 2026-11-01 maintainer ruling + * ratified that collision as the accepted cost of a global key (extending + * ruling 5934879010 on #21185, rule 2). A row the caller CAN read keeps the + * UPDATE arm, where the engine still judges the write. * - * So the fork now uses the same existence probe as every other by-id face, and - * whether the caller may WRITE the row it proves stays #1994's decision inside - * `engine.update` — the fake engine below models that pre-image check masking - * an out-of-scope by-id write as the same 404 the read path answers. + * ⚠️ The file name is historical: it pinned #5099's EXISTENCE fork, which this + * ruling reverses (the fork's own note in `protocol-batch-data.ts` records + * why). What #5099 established and is kept — no fallback insert after an + * update failure, on either arm — is still pinned here. * - * The old fallback (update threw → blind insert) goes with it, on BOTH arms: - * existence is now decided before the fork, so a fallback insert could only - * bury a real update failure under the duplicate-key error of inserting a row - * just proven to exist — the exact masking ADR-0119 D4 already forbade inside - * the atomic arm. + * The double: a caller-scoped read path, a #1994-style by-id write policy (an + * out-of-scope by-id write masks as the read path's 404), and a primary key + * that refuses a colliding insert the way the engine does (`UNIQUE_VIOLATION`, + * 409), so the upsert's insert arm and a plain create meet the SAME refusal and + * their rows can be compared whole. */ import { describe, it, expect, vi } from 'vitest'; @@ -37,12 +37,16 @@ const SCHEMA = { }, }; +const CALLER = { userId: 'u1' }; + /** * In-memory store with a caller-scoped read path and a #1994-style by-id * write policy: reads under a non-system context only see the caller's own * rows, and a by-id write to a row outside the caller's scope is masked as - * the same 404 the read path answers. Snapshot/rollback transaction semantics - * as in the #4620 / #5088 harnesses. + * the same 404 the read path answers. Inserting an id that is already stored + * is refused as the engine refuses it — a classified `UNIQUE_VIOLATION` (409) + * that names the column and nothing about the stored row. Snapshot/rollback + * transaction semantics as in the #4620 / #5088 harnesses. */ function makeRlsEngine() { const rows = new Map([ @@ -66,7 +70,7 @@ function makeRlsEngine() { const row = rows.get(id); if (!row || !visible(row, options?.context)) { // #1994's pre-image check: an unwritable/unknown row masks as the - // read path's 404 — THIS answer must reach the response row. + // read path's 404. throw Object.assign(new Error(`Record ${id} not found in showcase_task`), { code: 'RECORD_NOT_FOUND', status: 404, }); @@ -77,9 +81,12 @@ function makeRlsEngine() { }); const insert = vi.fn(async (_object: string, data: any) => { if (data.id && rows.has(data.id)) { - // Unclassified, as a driver throws it → `toRowApiError` renders - // INTERNAL_ERROR: the misdirection the probe exists to prevent. - throw new Error('duplicate key value violates unique constraint "showcase_task_pkey"'); + // The primary key's refusal, as the engine classifies a driver's + // duplicate: the registered code and status, the column, no row. + throw Object.assign( + new Error("Duplicate record refused on 'Task': a unique constraint on 'id' already holds this value. No record was written."), + { code: 'UNIQUE_VIOLATION', status: 409 }, + ); } const rec = { id: data.id ?? `new-${rows.size + 1}`, ...data }; rows.set(rec.id, rec); @@ -108,41 +115,51 @@ function makeRlsEngine() { return { engine, rows, findOne, update, insert }; } -const upsert = (p: any, records: any[], options?: any) => +/** A services registry carrying a `security` service whose object-level read admission answers `readable`. */ +function securityRegistry(readable: boolean | (() => Promise)) { + const canReadObject = vi.fn(async (_object: string, _context?: unknown) => + typeof readable === 'function' ? readable() : readable); + const services = new Map([['security', { canReadObject }]]); + return { getServicesRegistry: () => services, canReadObject }; +} + +const batch = (p: any, operation: string, records: any[], options?: any) => p.batchData({ object: 'showcase_task', - request: { operation: 'upsert', records, ...(options ? { options } : {}) }, - context: { userId: 'u1' }, + request: { operation, records, ...(options ? { options } : {}) }, + context: CALLER, } as any); -describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', () => { - it('an existing row outside the caller`s read scope takes the UPDATE arm — never a duplicate insert', async () => { +const upsert = (p: any, records: any[], options?: any) => batch(p, 'upsert', records, options); + +describe('batchData upsert — the fork asks what the CALLER can read', () => { + it('a row outside the caller`s read scope takes the INSERT arm and answers what a create naming that id answers', async () => { const t = makeRlsEngine(); const p = new ObjectStackProtocolImplementation(t.engine); + const before = JSON.stringify(t.rows.get('theirs_1')); - const res: any = await upsert(p, [{ id: 'theirs_1', data: { progress: 5 } }], { continueOnError: true }); + const ups: any = await upsert(p, [{ id: 'theirs_1', data: { progress: 5 } }], { continueOnError: true }); + const create: any = await batch(p, 'create', [{ data: { id: 'theirs_1', progress: 5 } }], { continueOnError: true }); - // THE CRUX. The row EXISTS, so the fork must not try to create it — - // under the old visibility read this row inserted, duplicate-keyed, - // and the non-atomic fallback inserted AGAIN. - expect(t.insert).not.toHaveBeenCalled(); - expect(t.update).toHaveBeenCalledTimes(1); - expect(t.update.mock.calls[0][2].where.id).toBe('theirs_1'); + // THE CRUX. The update arm is never reached for a row the caller + // cannot read, and the insert the fork chose meets the primary key. + expect(t.update).not.toHaveBeenCalled(); + expect(t.insert).toHaveBeenCalledTimes(2); + expect(t.insert.mock.calls[0][1]).toMatchObject({ id: 'theirs_1' }); - // What the caller sees is the WRITE POLICY's answer (#1994), not a - // primary-key collision. - expect(res.succeeded).toBe(0); - expect(res.failed).toBe(1); - expect(res.results[0].errors?.[0]?.code).toBe('RECORD_NOT_FOUND'); - expect(res.results[0].errors?.[0]?.httpStatus).toBe(404); - expect(res.results[0].errors?.[0]?.message).not.toContain('duplicate key'); + expect(ups.succeeded).toBe(0); + expect(ups.failed).toBe(1); + expect(ups.results[0].errors?.[0]).toMatchObject({ code: 'UNIQUE_VIOLATION', httpStatus: 409 }); + // ⭐ The row's refusal IS the create's refusal — byte for byte, no + // field, code or sentence of its own. + expect(ups.results[0].errors).toEqual(create.results[0].errors); - // And the store is untouched — no second row, no clobbered value. - expect(t.rows.get('theirs_1')).toMatchObject({ progress: 0, owner_id: 'u2' }); + // And the stored row is untouched. + expect(JSON.stringify(t.rows.get('theirs_1'))).toBe(before); expect(t.rows.size).toBe(2); }); - it('the probe asks with SYSTEM context — visibility stays the write policy`s decision', async () => { + it('the fork reads with the CALLER`s context — never the system`s', async () => { const t = makeRlsEngine(); const p = new ObjectStackProtocolImplementation(t.engine); @@ -153,7 +170,80 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', ], { continueOnError: true }); expect(t.findOne).toHaveBeenCalledTimes(3); - expect(t.findOne.mock.calls.every((c: any[]) => c[1]?.context?.isSystem === true)).toBe(true); + for (const [, options] of t.findOne.mock.calls as any[]) { + expect(options?.context?.userId).toBe('u1'); + expect(options?.context?.isSystem).not.toBe(true); + } + }); + + it('a caller the security service refuses the OBJECT to takes the insert arm for every row — the create`s answer each time', async () => { + const t = makeRlsEngine(); + const sec = securityRegistry(false); + const p = new ObjectStackProtocolImplementation(t.engine, sec.getServicesRegistry); + + const ups: any = await upsert(p, [ + { id: 'mine_1', data: { progress: 1 } }, + { id: 'brand_new', data: { progress: 7 } }, + ], { continueOnError: true }); + const create: any = await batch(p, 'create', [{ data: { id: 'mine_1', progress: 1 } }], { continueOnError: true }); + + // No row read is attempted: the caller can read no row of the object, + // and the middleware would refuse that read outright. + expect(t.findOne).not.toHaveBeenCalled(); + expect(t.update).not.toHaveBeenCalled(); + // Asked once for the whole loop, with the caller's context. + expect(sec.canReadObject).toHaveBeenCalledTimes(1); + expect(sec.canReadObject.mock.calls[0]).toEqual(['showcase_task', CALLER]); + + expect(ups.results[0].errors).toEqual(create.results[0].errors); + expect(ups.results[1]).toMatchObject({ id: 'brand_new', success: true }); + expect(t.rows.get('mine_1')).toMatchObject({ progress: 0 }); + expect(t.rows.get('brand_new')).toMatchObject({ progress: 7 }); + }); + + it('an object the security service admits still forks on the caller`s row read', async () => { + const t = makeRlsEngine(); + const sec = securityRegistry(true); + const p = new ObjectStackProtocolImplementation(t.engine, sec.getServicesRegistry); + + const ups: any = await upsert(p, [ + { id: 'mine_1', data: { progress: 3 } }, + { id: 'theirs_1', data: { progress: 5 } }, + ], { continueOnError: true }); + + expect(ups.results[0]).toMatchObject({ id: 'mine_1', success: true }); + expect(ups.results[1].errors?.[0]).toMatchObject({ code: 'UNIQUE_VIOLATION', httpStatus: 409 }); + expect(t.rows.get('mine_1')).toMatchObject({ progress: 3 }); + expect(t.rows.get('theirs_1')).toMatchObject({ progress: 0 }); + expect(sec.canReadObject).toHaveBeenCalledTimes(1); + }); + + it('an object-level admission that throws fails the row — no arm is chosen, nothing is written', async () => { + const t = makeRlsEngine(); + const sec = securityRegistry(async () => { throw new Error('permission resolution exploded'); }); + const p = new ObjectStackProtocolImplementation(t.engine, sec.getServicesRegistry); + + const ups: any = await upsert(p, [{ id: 'brand_new', data: { progress: 7 } }], { continueOnError: true }); + + expect(ups.results[0].success).toBe(false); + expect(t.insert).not.toHaveBeenCalled(); + expect(t.update).not.toHaveBeenCalled(); + expect(t.rows.has('brand_new')).toBe(false); + }); + + it('a context-less call is not asked about — its read runs as its write does, without a context', async () => { + const t = makeRlsEngine(); + const sec = securityRegistry(false); + const p = new ObjectStackProtocolImplementation(t.engine, sec.getServicesRegistry); + + await p.batchData({ + object: 'showcase_task', + request: { operation: 'upsert', records: [{ id: 'brand_new', data: { progress: 7 } }] }, + } as any); + + expect(sec.canReadObject).not.toHaveBeenCalled(); + expect(t.findOne).toHaveBeenCalledTimes(1); + expect(t.findOne.mock.calls[0][1]).not.toHaveProperty('context'); }); it('a real update failure surfaces ITSELF — no fallback insert to mask it (non-atomic)', async () => { @@ -164,23 +254,17 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', const res: any = await upsert(p, [{ id: 'mine_1', data: { progress: 1 } }], { continueOnError: true }); - // The old non-atomic fallback inserted here, buried 'update exploded' - // under a duplicate-key error, and reported THAT to the caller. - // [#8502] The message no longer discriminates: `update exploded` is a - // bare `Error`, so its sentence is withheld, and the fallback insert's - // duplicate-key text would be withheld too. `.not.toContain('duplicate - // key')` alone would therefore pass for the WRONG reason — it would - // pass even if the fallback had run. So the claim is carried by the - // structural pair, which is strictly stronger than the string ever - // was: the update WAS attempted, and no insert followed it. + // [#8502] `update exploded` is a bare `Error`, so its sentence is + // withheld, and a fallback insert's duplicate refusal would be + // classified rather than withheld. The claim is carried by the + // structural pair: the update WAS attempted, and no insert followed. expect(explodingUpdate).toHaveBeenCalledTimes(1); expect(t.insert).not.toHaveBeenCalled(); expect(res.results[0].success).toBe(false); expect(res.results[0].errors?.[0]?.message).toBe('The upsert of this record failed. The reason is in the server log.'); - expect(res.results[0].errors?.[0]?.message).not.toContain('duplicate key'); }); - it('a missing id still INSERTS — the #5088 pin is unchanged', async () => { + it('a missing id still INSERTS', async () => { const t = makeRlsEngine(); const p = new ObjectStackProtocolImplementation(t.engine); @@ -192,7 +276,7 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', expect(t.update).not.toHaveBeenCalled(); }); - it('a visible existing id still UPDATES, with the CALLER`s context on the write (#3455)', async () => { + it('a readable existing id still UPDATES, with the CALLER`s context on the write (#3455)', async () => { const t = makeRlsEngine(); const p = new ObjectStackProtocolImplementation(t.engine); @@ -202,12 +286,11 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', expect(res.results[0]).toMatchObject({ id: 'mine_1', success: true }); expect(t.rows.get('mine_1')).toMatchObject({ progress: 9 }); expect(t.insert).not.toHaveBeenCalled(); - // Only the PROBE is system — the write still runs as the caller. expect(t.update.mock.calls[0][2].context?.userId).toBe('u1'); expect(t.update.mock.calls[0][2].context?.isSystem).not.toBe(true); }); - it('atomic: the out-of-scope row is CAUSAL, no fallback fires, the earlier write rolls back', async () => { + it('atomic: the unreadable row is CAUSAL with the create`s refusal, the read rides the transaction, the earlier write rolls back', async () => { const t = makeRlsEngine(); const p = new ObjectStackProtocolImplementation(t.engine); @@ -217,13 +300,17 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', ], { atomic: true }); expect(res.results.map((r: any) => r.errors?.[0]?.code)).toEqual([ - 'ROLLED_BACK', 'RECORD_NOT_FOUND', + 'ROLLED_BACK', 'UNIQUE_VIOLATION', ]); - expect(t.insert).not.toHaveBeenCalled(); + expect(t.update).toHaveBeenCalledTimes(1); + expect(t.update.mock.calls[0][2].where.id).toBe('mine_1'); + // Both reads carry the transaction handle beside the caller. + expect(t.findOne.mock.calls.map((c: any[]) => c[1]?.context?.transaction?.id)).toEqual(['trx-1', 'trx-1']); expect(t.rows.get('mine_1')).toMatchObject({ progress: 0 }); + expect(t.rows.get('theirs_1')).toMatchObject({ progress: 0 }); }); - it('mixed batch, continueOnError: update / policy-denied / insert each answer for themselves', async () => { + it('mixed batch, continueOnError: update / unreadable / insert each answer for themselves', async () => { const t = makeRlsEngine(); const p = new ObjectStackProtocolImplementation(t.engine); @@ -236,11 +323,11 @@ describe('[#5099] batchData upsert — the fork asks EXISTENCE, not visibility', expect(res.succeeded).toBe(2); expect(res.failed).toBe(1); expect(res.results[0]).toMatchObject({ id: 'mine_1', success: true }); - expect(res.results[1].errors?.[0]?.code).toBe('RECORD_NOT_FOUND'); + expect(res.results[1].errors?.[0]?.code).toBe('UNIQUE_VIOLATION'); expect(res.results[2]).toMatchObject({ id: 'brand_new', success: true }); expect(t.rows.get('mine_1')).toMatchObject({ progress: 1 }); expect(t.rows.get('theirs_1')).toMatchObject({ progress: 0 }); expect(t.rows.get('brand_new')).toMatchObject({ progress: 7 }); - expect(t.insert).toHaveBeenCalledTimes(1); + expect(t.update).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts b/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts new file mode 100644 index 00000000000..8d64198e225 --- /dev/null +++ b/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts @@ -0,0 +1,373 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The by-id upsert of `POST /data/:object/batch` chooses its arm by what the +// CALLER can read — measured at the HTTP data door of the stock showcase and of +// a walled (`isolated`) two-organization boot. +// +// ── what is pinned ───────────────────────────────────────────────────────── +// +// On the write doors a row the caller cannot read is a row that does not exist +// (ruling 5985885287 on #21771). So an upsert row naming such a row takes the +// INSERT arm, and answers exactly what a create naming that id answers — the +// collision on the deployment-global primary key the maintainer ratified on +// 2026-11-01 as the accepted cost of a global key. For every persona below, +// atomic and not: +// +// 1. ⭐ an upsert naming a row the caller cannot read answers the SAME body as +// a batch create naming that id (the row's `id` echo aside), whose code and +// status are the plain `POST /data/:object` create's `409 +// UNIQUE_VIOLATION`; the stored row is byte-identical afterwards and the +// answer names no other organization; +// 2. an upsert naming an id that exists nowhere is created, as the create is; +// 3. a row the caller CAN read keeps the update arm: written with the edit +// grant, the engine's refusal without it. +// +// Personas — stock showcase: +// +// - member, `showcase_inquiry` (read+create, private OWD, no edit) — a row +// another user owns; +// - member, `showcase_private_note` (read+create+edit, private OWD) — another +// user's note (hidden) and the member's own (written); +// - member, `showcase_task` (read+create, no edit, public) — a row the member +// reads: the update arm's refusal, unchanged; +// - a create-only principal on `showcase_contact` (no read at all) — every +// row is one it cannot read; +// - control: member, `showcase_account` (read only) — one refusal for a row +// that exists and for one that does not, unchanged. +// +// Personas — walled boot, `showcase_category`, two organizations: +// +// - organization B's editor (read+create+edit) and creator (read+create) on +// organization A's row: the wall hides it, so each gets the create's +// answer; the editor writes its own organization's row, the creator gets +// the update arm's refusal on it. +// +// The fork itself is pinned at the protocol seam in +// `packages/metadata-protocol/src/protocol.upsert-existence.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 PASS = 'Upsert-Read-Pass-0001'; + +interface Answer { status: number; body: any } + +let seq = 0; +/** A fresh id no row carries — 16+ characters, the driver's id floor. */ +const unknownId = (): string => `qaupsread${(seq++).toString(36)}${Date.now().toString(36)}`.padEnd(18, '0'); + +/** A batch body with each row's caller-supplied `id` echo dropped and the verb written out, so an upsert and a create compare. */ +const comparable = (a: Answer): Answer => ({ + status: a.status, + body: { + ...a.body, + operation: 'VERB', + results: (a.body?.results ?? []).map((r: any) => { + const { id: _id, ...rest } = r ?? {}; + return rest; + }), + }, +}); + +/** The four doors one persona drives one id through. */ +function doors(stack: () => VerifyStack) { + const call = async (token: string, method: string, path: string, body?: unknown): Promise => { + const res = await stack().apiAs(token, method, path, body); + let parsed: unknown; + try { parsed = await res.json(); } catch { parsed = undefined; } + return { status: res.status, body: parsed }; + }; + const options = (atomic: boolean) => (atomic ? { atomic: true } : { continueOnError: true }); + return { + call, + upsert: (token: string, object: string, id: string, data: Record, atomic: boolean) => + call(token, 'POST', `/data/${object}/batch`, { operation: 'upsert', records: [{ id, data }], options: options(atomic) }), + batchCreate: (token: string, object: string, id: string, data: Record, atomic: boolean) => + call(token, 'POST', `/data/${object}/batch`, { operation: 'create', records: [{ data: { id, ...data } }], options: options(atomic) }), + create: (token: string, object: string, id: string, data: Record) => + call(token, 'POST', `/data/${object}`, { id, ...data }), + }; +} + +/** The id a single create answered, wherever the envelope carries it. */ +const createdId = (a: Answer): string => String(a.body?.id ?? a.body?.record?.id ?? a.body?.data?.id ?? ''); + +describe('upsert by id: a row the caller cannot read answers as a create naming that id (stock showcase)', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string; + let memberTok: string; + let creatorTok: string; + const d = doors(() => stack); + const rows: Record = {}; + let projectId = ''; + + const CREATE_ONLY = 'qa_upsert_read_contact_create_only'; + const CREATOR = 'upsert-read-creator@verify.test'; + const MEMBER = 'upsert-read-member@verify.test'; + + const stored = async (object: string, id: string) => + JSON.stringify(await ql.findOne(object, { where: { id }, context: SYS })); + + const PAYLOAD: Record Record> = { + showcase_inquiry: () => ({ name: 'Upsert Probe', email: 'upsert-probe@verify.test', message: 'probe' }), + showcase_private_note: () => ({ title: 'upsert probe' }), + showcase_task: () => ({ title: 'upsert probe', project: projectId, status: 'todo' }), + showcase_contact: () => ({ name: 'Upsert Probe', email: 'upsert-probe@verify.test' }), + showcase_account: () => ({ name: 'Upsert Probe', status: 'prospect' }), + }; + + beforeAll(async () => { + stack = await bootShowcase(); + adminTok = await stack.signIn(); + ql = await stack.kernel.getServiceAsync('objectql'); + memberTok = await stack.signUp(MEMBER, PASS); + + // A principal who may CREATE contacts and do nothing else with them: no + // read, no edit. The member baseline grants nothing on the object. + const saved = await stack.apiAs(adminTok, 'PUT', `/meta/permission/${CREATE_ONLY}`, { + name: CREATE_ONLY, label: 'QA create-only contacts', systemPermissions: [], + objects: { showcase_contact: { allowRead: false, allowCreate: true, allowEdit: false, allowDelete: false } }, + }); + expect(saved.status, await saved.clone().text()).toBe(200); + creatorTok = await stack.signUp(CREATOR, PASS); + const creatorId = (await ql.findOne('sys_user', { where: { email: CREATOR }, context: SYS }))?.id; + expect(creatorId, 'creator provisioned').toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: creatorId, permission_set: CREATE_ONLY }, { context: SYS }); + + projectId = String((await ql.find('showcase_project', { limit: 1, context: SYS }))?.[0]?.id ?? ''); + expect(projectId, 'the showcase seed provides a project').toBeTruthy(); + + // Rows the admin owns: hidden from the member by the private OWD, and from + // the creator by having no read at all. + for (const object of ['showcase_inquiry', 'showcase_private_note', 'showcase_contact']) { + const made = await d.create(adminTok, object, unknownId(), PAYLOAD[object]()); + expect(made.status, `admin ${object} created — ${JSON.stringify(made.body)}`).toBeLessThan(300); + rows[`hidden:${object}`] = createdId(made); + } + const own = await d.create(memberTok, 'showcase_private_note', unknownId(), { title: 'member note' }); + expect(own.status, `member note created — ${JSON.stringify(own.body)}`).toBeLessThan(300); + rows.ownNote = createdId(own); + rows.task = String((await ql.find('showcase_task', { limit: 1, context: SYS }))?.[0]?.id ?? ''); + rows.account = String((await ql.find('showcase_account', { limit: 1, context: SYS }))?.[0]?.id ?? ''); + for (const [k, v] of Object.entries(rows)) expect(v, `fixture row ${k}`).toBeTruthy(); + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + it('guard the guard: who can read which row, and which grants each persona holds', async () => { + for (const object of ['showcase_inquiry', 'showcase_private_note']) { + expect((await d.call(memberTok, 'GET', `/data/${object}/${rows[`hidden:${object}`]}`)).status, `member cannot read the admin's ${object}`).toBe(404); + expect((await d.call(adminTok, 'GET', `/data/${object}/${rows[`hidden:${object}`]}`)).status).toBe(200); + } + expect((await d.call(memberTok, 'GET', `/data/showcase_private_note/${rows.ownNote}`)).status).toBe(200); + expect((await d.call(memberTok, 'GET', `/data/showcase_task/${rows.task}`)).status, 'the member reads the task').toBe(200); + expect((await d.call(memberTok, 'PATCH', `/data/showcase_task/${rows.task}`, { title: 'x' })).status, 'the member may not edit tasks').toBe(403); + expect((await d.call(memberTok, 'GET', `/data/showcase_account/${rows.account}`)).status, 'the member reads the account').toBe(200); + expect((await d.create(memberTok, 'showcase_account', unknownId(), PAYLOAD.showcase_account())).status, 'the member may not create accounts').toBe(403); + expect((await d.call(creatorTok, 'GET', `/data/showcase_contact/${rows['hidden:showcase_contact']}`)).status, 'the creator reads no contact').toBe(403); + expect((await d.create(creatorTok, 'showcase_contact', unknownId(), PAYLOAD.showcase_contact())).status, 'the creator creates contacts').toBe(201); + }); + + const HIDDEN: Array<[string, string, () => string]> = [ + ['member, read+create without edit, private row', 'showcase_inquiry', () => memberTok], + ['member, read+create+edit, private row', 'showcase_private_note', () => memberTok], + ['create-only principal, no read at all', 'showcase_contact', () => creatorTok], + ]; + + for (const [label, object, tok] of HIDDEN) { + for (const atomic of [false, true]) { + it(`${label} (${object}, ${atomic ? 'atomic' : 'non-atomic'}): the unreadable row answers as a create naming it`, async () => { + const id = rows[`hidden:${object}`]; + const before = await stored(object, id); + + const ups = await d.upsert(tok(), object, id, PAYLOAD[object](), atomic); + const batchCreate = await d.batchCreate(tok(), object, id, PAYLOAD[object](), atomic); + const create = await d.create(tok(), object, id, PAYLOAD[object]()); + + // ⭐ The create's answer, whole: same status, same envelope, same row + // refusal — nothing of the upsert's own. + expect(comparable(ups), JSON.stringify(ups.body)).toEqual(comparable(batchCreate)); + expect(create.status, JSON.stringify(create.body)).toBe(409); + expect(create.body?.code).toBe('UNIQUE_VIOLATION'); + expect(ups.body?.results?.[0]?.errors?.[0]).toMatchObject({ code: create.body.code, httpStatus: create.status }); + expect(ups.body?.results?.[0]?.id).toBe(id); + // Nothing moved. + expect(await stored(object, id)).toBe(before); + }); + + it(`${label} (${object}, ${atomic ? 'atomic' : 'non-atomic'}): an id that names no row is created, as the create is`, async () => { + const id = unknownId(); + const ups = await d.upsert(tok(), object, id, PAYLOAD[object](), atomic); + expect(ups.status, JSON.stringify(ups.body)).toBe(200); + expect(ups.body?.success, JSON.stringify(ups.body)).toBe(true); + expect(ups.body?.results?.[0]).toMatchObject({ id, success: true }); + expect(JSON.parse(await stored(object, id))?.id).toBe(id); + + const other = unknownId(); + const batchCreate = await d.batchCreate(tok(), object, other, PAYLOAD[object](), atomic); + expect(batchCreate.body?.success, JSON.stringify(batchCreate.body)).toBe(true); + }); + } + } + + for (const atomic of [false, true]) { + it(`member, read+create+edit, own row (${atomic ? 'atomic' : 'non-atomic'}): the readable row is UPDATED`, async () => { + const title = `renamed ${atomic ? 'atomic' : 'plain'}`; + const ups = await d.upsert(memberTok, 'showcase_private_note', rows.ownNote, { title }, atomic); + expect(ups.body?.success, JSON.stringify(ups.body)).toBe(true); + expect(ups.body?.results?.[0]).toMatchObject({ id: rows.ownNote, success: true }); + expect(JSON.parse(await stored('showcase_private_note', rows.ownNote))?.title).toBe(title); + }); + + it(`member, read+create without edit, readable row (${atomic ? 'atomic' : 'non-atomic'}): the update arm's refusal, unchanged`, async () => { + const before = await stored('showcase_task', rows.task); + const ups = await d.upsert(memberTok, 'showcase_task', rows.task, { title: 'should not land' }, atomic); + expect(ups.body?.success).toBe(false); + expect(ups.body?.results?.[0]?.errors?.[0], JSON.stringify(ups.body)).toMatchObject({ code: 'PERMISSION_DENIED', httpStatus: 403 }); + expect(await stored('showcase_task', rows.task)).toBe(before); + + const id = unknownId(); + const created = await d.upsert(memberTok, 'showcase_task', id, PAYLOAD.showcase_task(), atomic); + expect(created.body?.results?.[0], JSON.stringify(created.body)).toMatchObject({ id, success: true }); + }); + + it(`control — member refused create and edit (showcase_account, ${atomic ? 'atomic' : 'non-atomic'}): one refusal for a row that exists and one that does not`, async () => { + const before = await stored('showcase_account', rows.account); + const miss = unknownId(); + const hit = await d.upsert(memberTok, 'showcase_account', rows.account, PAYLOAD.showcase_account(), atomic); + const none = await d.upsert(memberTok, 'showcase_account', miss, PAYLOAD.showcase_account(), atomic); + expect(hit.body?.results?.[0]?.errors?.[0], JSON.stringify(hit.body)).toMatchObject({ code: 'PERMISSION_DENIED', httpStatus: 403 }); + expect(comparable(hit)).toEqual(comparable(none)); + expect(await stored('showcase_account', rows.account)).toBe(before); + expect(await ql.findOne('showcase_account', { where: { id: miss }, context: SYS })).toBeFalsy(); + }); + } +}); + +describe('upsert by id: a row behind the organization wall answers as a create naming that id (walled two-organization boot)', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string; + const tok: Record = {}; + const org: Record = {}; + const d = doors(() => stack); + const rows: Record = {}; + + const OBJECT = 'showcase_category'; + const RW = 'qa_upsert_read_category_rw'; + const CREATE = 'qa_upsert_read_category_create'; + + const stored = async (id: string) => JSON.stringify(await ql.findOne(OBJECT, { where: { id }, context: SYS })); + const userId = async (email: string) => String((await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id ?? ''); + + beforeAll(async () => { + stack = await bootShowcase({ multiTenant: true }); + adminTok = await stack.signIn(); + ql = await stack.kernel.getServiceAsync('objectql'); + + for (const [set, grant] of [ + [RW, { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: false }], + [CREATE, { allowRead: true, allowCreate: true, allowEdit: false, allowDelete: false }], + ] as const) { + const saved = await stack.apiAs(adminTok, 'PUT', `/meta/permission/${set}`, { + name: set, label: set, systemPermissions: [], objects: { [OBJECT]: grant }, + }); + expect(saved.status, await saved.clone().text()).toBe(200); + } + + const people: Array<[string, string, string, string]> = [ + // [key, email, organization key, permission set] + ['aOwner', 'upsert-read-a-owner@verify.test', 'A', RW], + ['bEditor', 'upsert-read-b-owner@verify.test', 'B', RW], + ['bCreator', 'upsert-read-b-member@verify.test', 'B', CREATE], + ]; + for (const [key, email] of people) tok[key] = await stack.signUp(email, PASS); + + for (const [key, owner] of [['A', 'aOwner'], ['B', 'bEditor']] as const) { + const slug = `upsert-read-${key.toLowerCase()}-org`; + const name = `Upsert Read Org ${key}`; + const made = await stack.apiAs(tok[owner], 'POST', '/auth/organization/create', { name, slug }); + expect(made.status, `org ${key}: ${await made.clone().text()}`).toBe(200); + org[key] = { id: String(((await made.json()) as any).id), name, slug }; + } + await ql.insert('sys_member', { user_id: await userId('upsert-read-b-member@verify.test'), organization_id: org.B.id, role: 'member' }, { context: SYS }); + for (const [key, email, orgKey, set] of people) { + const active = await stack.apiAs(tok[key], 'POST', '/auth/organization/set-active', { organizationSlug: org[orgKey].slug }); + expect(active.status, `set-active ${key}: ${await active.clone().text()}`).toBe(200); + await ql.insert('sys_user_permission_set', { user_id: await userId(email), permission_set: set, organization_id: org[orgKey].id }, { context: SYS }); + } + + const a = await d.create(tok.aOwner, OBJECT, unknownId(), { name: 'org A category' }); + expect(a.status, `org A row — ${JSON.stringify(a.body)}`).toBe(201); + rows.foreign = createdId(a); + const b = await d.create(tok.bEditor, OBJECT, unknownId(), { name: 'org B category' }); + expect(b.status, `org B row — ${JSON.stringify(b.body)}`).toBe(201); + rows.own = createdId(b); + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + it('guard the guard: a real walled posture, two organizations, and the wall hides A`s row from B', async () => { + const tenancy = await stack.kernel.getServiceAsync('tenancy'); + expect(tenancy.posture).toBe('isolated'); + expect(tenancy.isolationActive).toBe(true); + expect(tenancy.degraded).toBe(false); + expect(org.A.id).not.toBe(org.B.id); + + expect(JSON.parse(await stored(rows.foreign))?.organization_id).toBe(org.A.id); + expect(JSON.parse(await stored(rows.own))?.organization_id).toBe(org.B.id); + expect((await d.call(tok.aOwner, 'GET', `/data/${OBJECT}/${rows.foreign}`)).status).toBe(200); + for (const who of ['bEditor', 'bCreator']) { + expect((await d.call(tok[who], 'GET', `/data/${OBJECT}/${rows.foreign}`)).status, `${who} cannot read A's row`).toBe(404); + expect((await d.call(tok[who], 'GET', `/data/${OBJECT}/${rows.own}`)).status, `${who} reads B's row`).toBe(200); + } + expect((await d.call(tok.bCreator, 'PATCH', `/data/${OBJECT}/${rows.own}`, { name: 'x' })).status, 'the B creator may not edit').toBe(403); + }); + + for (const who of ['bEditor', 'bCreator']) { + for (const atomic of [false, true]) { + it(`${who} (${atomic ? 'atomic' : 'non-atomic'}): organization A's row answers as a create naming it, and names no organization`, async () => { + const before = await stored(rows.foreign); + const ups = await d.upsert(tok[who], OBJECT, rows.foreign, { name: 'cross-org probe' }, atomic); + const batchCreate = await d.batchCreate(tok[who], OBJECT, rows.foreign, { name: 'cross-org probe' }, atomic); + const create = await d.create(tok[who], OBJECT, rows.foreign, { name: 'cross-org probe' }); + + expect(comparable(ups), JSON.stringify(ups.body)).toEqual(comparable(batchCreate)); + expect(create.status, JSON.stringify(create.body)).toBe(409); + expect(create.body?.code).toBe('UNIQUE_VIOLATION'); + expect(ups.body?.results?.[0]?.errors?.[0]).toMatchObject({ code: create.body.code, httpStatus: create.status }); + const text = JSON.stringify(ups.body); + for (const name of [org.A.id, org.A.name, org.A.slug]) expect(text).not.toContain(name); + expect(await stored(rows.foreign)).toBe(before); + + const id = unknownId(); + const fresh = await d.upsert(tok[who], OBJECT, id, { name: 'fresh' }, atomic); + expect(fresh.body?.results?.[0], JSON.stringify(fresh.body)).toMatchObject({ id, success: true }); + expect(JSON.parse(await stored(id))?.organization_id).toBe(org.B.id); + }); + } + } + + for (const atomic of [false, true]) { + it(`B editor, own organization's row (${atomic ? 'atomic' : 'non-atomic'}): UPDATED`, async () => { + const name = `B renamed ${atomic ? 'atomic' : 'plain'}`; + const ups = await d.upsert(tok.bEditor, OBJECT, rows.own, { name }, atomic); + expect(ups.body?.results?.[0], JSON.stringify(ups.body)).toMatchObject({ id: rows.own, success: true }); + expect(JSON.parse(await stored(rows.own))?.name).toBe(name); + }); + + it(`B creator, own organization's row (${atomic ? 'atomic' : 'non-atomic'}): the update arm's refusal`, async () => { + const before = await stored(rows.own); + const ups = await d.upsert(tok.bCreator, OBJECT, rows.own, { name: 'should not land' }, atomic); + expect(ups.body?.results?.[0]?.errors?.[0], JSON.stringify(ups.body)).toMatchObject({ code: 'PERMISSION_DENIED', httpStatus: 403 }); + expect(await stored(rows.own)).toBe(before); + }); + } +}); From 57c70faba39d5188af03a6f4ffbf8ff43190366e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 12:28:19 +0000 Subject: [PATCH 3/6] test(metadata-protocol): name the upsert fork's read in the readonly-insert double's note Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- packages/metadata-protocol/src/protocol.readonly-insert.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/metadata-protocol/src/protocol.readonly-insert.test.ts b/packages/metadata-protocol/src/protocol.readonly-insert.test.ts index 8c46ba50d7c..a0d6ccbcba4 100644 --- a/packages/metadata-protocol/src/protocol.readonly-insert.test.ts +++ b/packages/metadata-protocol/src/protocol.readonly-insert.test.ts @@ -96,7 +96,7 @@ function makeProtocol(schema: any = SCHEMA) { findOne: vi.fn(async (object: string, query?: EngineFindOneQueryInput) => { assertEngineFindOnePredicate(object, query); // The clone source exists; any other id names no row, which is what sends - // `batchData`'s upsert fork (`probeRecord`, #5099) down its CREATE arm. + // `batchData`'s upsert fork (the caller's `findOne`) down its CREATE arm. const id = (query as any)?.where?.id; return id === 'src-1' ? { id: 'src-1', title: 'Source', approval_status: 'approved' } : null; }), From bf59c1e42fd197fe0f698ccb4230981c5211a5f8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 13:34:23 +0000 Subject: [PATCH 4/6] test(dogfood): the walled upsert boot declares an invitation-only membership policy Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../test/upsert-by-id-caller-read.dogfood.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts b/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts index 8d64198e225..8c8bfff1d62 100644 --- a/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts +++ b/packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts @@ -265,7 +265,14 @@ describe('upsert by id: a row behind the organization wall answers as a create n const stored = async (id: string) => JSON.stringify(await ql.findOne(OBJECT, { where: { id }, context: SYS })); const userId = async (email: string) => String((await ql.findOne('sys_user', { where: { email }, context: SYS }))?.id ?? ''); + // A walled deployment must DECLARE its membership policy or it refuses to + // boot; invitation-only is the declaration a walled deployment makes, and + // every membership below comes from an explicit act (creating an + // organization, an admin-added member row). + const priorPolicy = process.env.OS_AUTH_MEMBERSHIP_POLICY; + beforeAll(async () => { + process.env.OS_AUTH_MEMBERSHIP_POLICY = 'invite-only'; stack = await bootShowcase({ multiTenant: true }); adminTok = await stack.signIn(); ql = await stack.kernel.getServiceAsync('objectql'); @@ -312,6 +319,8 @@ describe('upsert by id: a row behind the organization wall answers as a create n afterAll(async () => { await stack?.stop?.(); + if (priorPolicy === undefined) delete process.env.OS_AUTH_MEMBERSHIP_POLICY; + else process.env.OS_AUTH_MEMBERSHIP_POLICY = priorPolicy; }); it('guard the guard: a real walled posture, two organizations, and the wall hides A`s row from B', async () => { From adf4e0cb45ea2b6f621f538f97d2978c3d23194f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 13:55:19 +0000 Subject: [PATCH 5/6] fix(metadata-protocol): type the upsert fork's caller read instead of erasing its options Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- packages/metadata-protocol/src/protocol-batch-data.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/metadata-protocol/src/protocol-batch-data.ts b/packages/metadata-protocol/src/protocol-batch-data.ts index 68d52fa28f4..3d08cbd65d2 100644 --- a/packages/metadata-protocol/src/protocol-batch-data.ts +++ b/packages/metadata-protocol/src/protocol-batch-data.ts @@ -48,7 +48,7 @@ import type { ObjectStackProtocolImplementation, BatchDataLoopOutcome, BatchDataRowResult } from './protocol.js'; import { byIdUpdateRecord, rowRequiredIdError, toRowApiError } from './protocol.js'; import type { BatchUpdateRequest, BatchUpdateResponse, DeleteManyDataRequest, UpdateManyDataRequest } from '@objectstack/spec/api'; -import type { DroppedFieldsEvent } from '@objectstack/spec/data'; +import type { DroppedFieldsEvent, EngineQueryOptionsParsed } from '@objectstack/spec/data'; import type { ISecurityService } from '@objectstack/spec/contracts'; import { engineCanRollBack, recordNotFoundError } from '@objectstack/core'; import { omitInternalFieldsFromWriteResponse } from './write-response-internal-fields.js'; @@ -433,8 +433,10 @@ export async function runBatchDataLoop( // under an insert's error — the masking ADR-0119 D4 // forbade inside the atomic arm. objectReadable ??= callerMayReadObject.call(this, object, context); + const callerRead: EngineQueryOptionsParsed = { where: { id: record.id } }; + if (context !== undefined) callerRead.context = context; const existing = (await objectReadable) - ? await this.engine.findOne(object, { where: { id: record.id }, ...ctxOpt } as any) + ? await this.engine.findOne(object, callerRead) : null; if (existing) { const dropped: DroppedFieldsEvent[] = []; From 9fb84ac128d303c0e23da06e8376802ff02520cc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 16:50:03 +0000 Subject: [PATCH 6/6] test(core): classify the batch upsert fork's read-admission ask as not a row-serving door Claude-Session: https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk Co-authored-by: Claude --- .../src/security/row-serving-door-exposure.pin.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/core/src/security/row-serving-door-exposure.pin.test.ts b/packages/core/src/security/row-serving-door-exposure.pin.test.ts index 7d1a48b304c..c45cfa70e9b 100644 --- a/packages/core/src/security/row-serving-door-exposure.pin.test.ts +++ b/packages/core/src/security/row-serving-door-exposure.pin.test.ts @@ -109,6 +109,14 @@ const CALLERS: Record = { + 'that only narrows the read the caller addressed, whose own door already admitted it; no row and no field of the ' + 'parent is served, and a refused parent answers `$in: []`', }, + 'packages/metadata-protocol/src/protocol-batch-data.ts': { + kind: 'not-a-door', + reason: + "the by-id upsert fork of `batchData` (`callerMayReadObject`): it asks the admission only to choose between the " + + 'update and create arms of a row the caller addressed on the batch data route, whose own door already judged the ' + + "object's exposure; `true` still reads the row as the caller, through the engine middleware, and it serves no row " + + 'of its own', + }, // [#25522 cut 11] File-field hydration's `sys_file` admission, moved out of engine.ts. 'packages/objectql/src/engine-file-reference-hydration.ts': { kind: 'not-a-door',