fix(metadata-protocol): the by-id OCC comparison answers only for a row the caller reads

.changeset/harden-occ-unreadable-row-write-door.md

@@ -0,0 +1,13 @@
+---
+'@objectstack/metadata-protocol': patch
+---
+
+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, 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 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.

content/docs/protocol/kernel/http-protocol.mdx

@@ -753,6 +753,12 @@ 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`) 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
 
 Delete a record by ID.

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 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
  *
@@ -227,8 +230,14 @@ 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 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) throw recordNotFoundError(object, id);
+        if (!visible) return;
         throw new ConcurrentUpdateError({
             currentVersion,
             currentRecord: visible,

packages/metadata-protocol/src/protocol-version-tokens.ts

@@ -5,7 +5,8 @@
 //
 // A MOVE, not a redesign: every declaration below is byte-identical to the text that stood
 // above `ObjectStackProtocolImplementation`; only the imports and the export list at the foot
-// are new. `protocol.ts` re-exports what it exported.
+// are new. `protocol.ts` re-exports what it exported. One docblock has changed since the move:
+// `ConcurrentUpdateError`'s sentence on a row the caller does not read (see `assertVersionOf`).
 
 /**
  * Simple hash function for ETag generation (browser-compatible)
@@ -29,8 +30,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: it
- * answers not-found instead (#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

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<string, any> = {},
-  opts: { refuse?: string; leavesScope?: ReadonlySet<string> } = {},
+  opts: {
+    refuse?: string;
+    leavesScope?: ReadonlySet<string>;
+    hidden?: ReadonlySet<string>;
+    readRefusal?: () => Error;
+  } = {},
 ) {
   const store = new Map<string, any>(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,113 @@ 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 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
+  // 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<unknown>) => 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 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 () => {
+        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, 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);
+          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'));
+      });
+    });
+  }
+});