Skip to content

Commit 7dc7d22

Browse files
fix(metadata-protocol): an optimistic-concurrency conflict reports only what the caller may read (#23628)
Fixes #23586 Clause-②: no ## What changes (function level) `metadata-protocol`'s `updateData` and `deleteData` judge the caller's version token on the row `probeRecord` reads with a system context. That probe asks existence, not visibility, and it stays exactly as it is. What changes is the mismatch branch of `assertVersionOf`: - On a mismatch, the row is read again under the caller's own `context` (`readAsCaller`, a by-id `engine.findOne` with the same context spelling as the write it precedes), so every RLS and FLS rule of the read door applies. - **No row for the caller:** `assertVersionOf` throws `recordNotFoundError(object, id)`. That is the same producer the security layer's by-id write gate uses for a row the caller cannot read, so the stale-token request gets exactly what the same request without a token gets: `404 RECORD_NOT_FOUND`, with no `currentVersion` and no `currentRecord`. - **A row for the caller:** `ConcurrentUpdateError` as before, with `currentRecord` set to the caller's read, so it is field-masked like the caller's own read. `currentVersion` is still the token the comparison ran on. - **A token that matches, or no token:** unchanged. The extra read happens only on a mismatch. `assertVersionOf` became async and takes the request context. Both callers (`updateData`, and `assertVersionMatch` for `deleteData`) pass `request.context` and await it. ### The four `probeRecord` callers - `updateData` and `assertVersionMatch` (`deleteData`): these are the conflict path. Both are fixed by the one change in `assertVersionOf`. - `assertRecordExists`, used by `updateManyData` and `batchData`'s `update` branch: existence only. It never takes a version token, never hands the probed row to the caller, and leaves the write to `engine.update` under the caller's policy. Not this path. - `batchData`'s `upsert` fork: existence only, deciding update versus insert. It has no version token, and the response row is `engine.update`'s readback under the caller's context, never the probed row. Not this path. ## Pins New file `packages/runtime/src/domains/data-conflict-caller-visibility.test.ts`. It uses the measured rig shape: a real `ObjectQL` over a real `SqlDriver` (better-sqlite3), with the real `SecurityPlugin` started (an RLS policy and an FLS rule on one object, nothing on a second). It runs `PATCH` and `DELETE` through `RestServer`'s registered routes and through the runtime `HttpDispatcher`'s `/data` domain. A dispatcher throw is rendered by `declaredEnvelopeForThrow`, the exit the Hono hosts use, and the thrown error is read too. Per door and verb: 1. **A row RLS hides:** the stale-token answer equals the control (the same request without a token): same status, same body (`toEqual`), with no `currentVersion` or `currentRecord` on the wire or on the thrown error. The row is unchanged. 2. **A field FLS hides:** `409 CONCURRENT_UPDATE`, `currentVersion` equal to the stored token, and `currentRecord` equal to the caller's own read of the row, without the hidden field. 3. **Positive control, a row the caller reads whole:** `409` with the whole row and its version, so conflict detection still works. 4. **A matching token:** `200`, and the engine's `findOne` is called the same number of times as without a token (plus the one probe `DELETE` already issues when a token is present). The pins live in `runtime`, beside the existing both-door OCC pins (`data-update-expected-version.test.ts`, `data-delete-expected-version.test.ts`). It is the one package whose test graph holds `RestServer`, the dispatcher, `plugin-security`, a SQL driver and `declaredEnvelopeForThrow` together. They are in a new file. ## Verification The readings below were taken at `7a4652fdb4`, before `main` was merged in at `2a20fe2d80`. The re-runs on the merged head are in the `os-dev-report` comment on #23586. - **Red, before the fix** (pins on the unfixed `metadata-protocol` dist): `8 failed | 8 passed (16)`. The eight failures are the RLS and FLS rows on both doors and both verbs (a `409` carrying the hidden row or field where the control answered `404`). The positive-control and matching-token rows passed. - **Green, after the fix** (`metadata-protocol` rebuilt): `16 passed (16)`. The final verification runs are listed in the report comment on #23586. - `pnpm --filter @objectstack/metadata-protocol test`: 237 files, 28429 passed. `typecheck`: exit 0. - `pnpm --filter @objectstack/runtime test`: 385 files, 5798 passed. `typecheck` (with `check:test-typecheck`): exit 0, and the new file adds no ledger entry. - `pnpm --filter @objectstack/rest test`: 285 files, 5858 passed. - Gates: the `dispatch-gates --commands` list for this diff, run one by one. `check:query-options-erasure` caught an untyped options bag on the first pass. It is now typed to `EngineQueryOptionsParsed` / `EngineQueryOptions`. Per-gate readings are in the report. ## Acceptance notes - **The Hono doors (`plugin-hono-server`) are not touched.** After this change the producer's conflict answer holds only what the caller reads, so if `declaredConflictContext` forwarded `currentRecord`, that record would be safe to send. The hold #23587 placed there can be lifted on that card. A row the caller cannot read now answers `404 RECORD_NOT_FOUND` through those doors as well. - **`currentVersion` is the token the comparison ran on** (the system probe's `updated_at`), so its meaning is unchanged. If an FLS rule ever hid `updated_at` itself, the version token would still be published on a conflict. A `git grep` over `examples/` and `packages/` finds no field-permission entry keyed on `.updated_at` (0 hits), and this PR does not change that case. - **A caller-context read that throws** (the object's read grant withheld, a store fault) propagates as raised. It is never relabelled as a conflict or a not-found. --- _Generated by [Claude Code](https://claude.ai/code/session_01YNquHRZKDpiKsgmHXSd9Jx)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent edac218 commit 7dc7d22

3 files changed

Lines changed: 435 additions & 11 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): an optimistic-concurrency conflict reports only what the caller may read
6+
7+
Clause-②: no
8+
9+
`updateData` and `deleteData` judge a caller's version token (`If-Match`, or the body/query `expectedVersion`) on the row their existence probe reads with a system context. On a mismatch they used to answer `409 CONCURRENT_UPDATE` with that system-read row as `currentRecord`, before the caller's row- and field-level security had run.
10+
11+
Now a mismatch re-reads the row under the caller's own context, and only then answers:
12+
13+
- **A row the caller cannot read:** `404 RECORD_NOT_FOUND`, the same answer the same request without a version token gets. It carries no `currentVersion` and no `currentRecord`.
14+
- **A row the caller can read:** `409 CONCURRENT_UPDATE` as before, with `currentRecord` read as the caller reads it, so a field hidden from the caller is absent from it. `currentVersion` is unchanged.
15+
16+
A request whose token matches, or that sends none, is unchanged and issues no extra read. Every door that renders the protocol's answer (`RestServer`, the runtime dispatcher's `/data` domain and the Hono hosts) gets the narrowed answer.

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2798,7 +2798,9 @@ function simpleHash(str: string): string {
27982798
* The HTTP layer maps this to `409 Conflict` with code `CONCURRENT_UPDATE`,
27992799
* and includes both the current server-side version and the current record
28002800
* payload so the client can render an informed conflict-resolution UI
2801-
* ("Reload latest" vs. "Overwrite anyway").
2801+
* ("Reload latest" vs. "Overwrite anyway"). The record is the row as the
2802+
* caller reads it, and a row the caller cannot read is never a conflict: it
2803+
* answers not-found instead (#23586, see `assertVersionOf`).
28022804
*
28032805
* NOTE: This is an *application-level* compare-and-set — not an atomic
28042806
* storage-layer CAS. There is a small TOCTOU window between the version
@@ -14633,7 +14635,7 @@ export class ObjectStackProtocolImplementation implements
1463314635
// 404 wins over 409 when both could apply: OCC has always declined to
1463414636
// treat a missing record as a concurrency conflict, and "this record
1463514637
// does not exist" is the more specific answer.
14636-
this.assertVersionOf(request.object, request.id, current, request.expectedVersion);
14638+
await this.assertVersionOf(request.object, request.id, current, request.expectedVersion, request.context);
1463714639
const opts: any = { where: { id: request.id } };
1463814640
if (request.context !== undefined) opts.context = request.context;
1463914641
// [#3407/#3431] Capture the engine's LEGAL write strips (static `readonly`
@@ -14729,7 +14731,7 @@ export class ObjectStackProtocolImplementation implements
1472914731

1473014732
async deleteData(request: { object: string, id: string, expectedVersion?: string, context?: any }) {
1473114733
this.assertObjectRegistered(request.object); // [#3770]
14732-
await this.assertVersionMatch(request.object, request.id, request.expectedVersion);
14734+
await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context);
1473314735
const opts: any = { where: { id: request.id } };
1473414736
if (request.context !== undefined) opts.context = request.context;
1473514737
const deleted = await this.engine.delete(request.object, opts);
@@ -14870,10 +14872,11 @@ export class ObjectStackProtocolImplementation implements
1487014872
}
1487114873

1487214874
/**
14873-
* Optimistic Concurrency Control — the COMPARISON half, over a row the
14874-
* caller has already read. Pure: it issues no query of its own, which is
14875-
* what lets `updateData` run the gate on its existence probe's result
14876-
* rather than re-reading the record (#4435).
14875+
* Optimistic Concurrency Control — the COMPARISON half, over the row the
14876+
* existence probe has already read. A token that agrees (or no token)
14877+
* issues no query of its own, which is what lets `updateData` run the gate
14878+
* on its existence probe's result rather than re-reading the record
14879+
* (#4435). Only a MISMATCH reads again — see the conflict paragraph below.
1487714880
*
1487814881
* When the caller passes a non-empty `expectedVersion` token (typically the
1487914882
* `updated_at` value they read), a mismatch throws `ConcurrentUpdateError`,
@@ -14903,13 +14906,35 @@ export class ObjectStackProtocolImplementation implements
1490314906
* SQLite family — must not decide whether a save is a conflict. See
1490414907
* `normaliseVersionToken` for the measured input domain and for why the
1490514908
* change cannot refuse a token that is accepted today.
14909+
*
14910+
* ## [#23586] The conflict reports only what the caller may read
14911+
*
14912+
* `current` is the SYSTEM-context probe ({@link probeRecord}: existence,
14913+
* not visibility), and this gate runs before `engine.update` /
14914+
* `engine.delete` apply the caller's policy. So a mismatch does not answer
14915+
* from it. It re-reads the row under the caller's own `context`, the read
14916+
* door's question (every RLS and FLS rule applies):
14917+
*
14918+
* - no row → `recordNotFoundError`, the producer the security layer's
14919+
* by-id write gate answers a row the caller cannot read with. The stale
14920+
* token then gets exactly the answer the same request without a token
14921+
* gets: no `currentVersion`, no `currentRecord`, so the `409` no longer
14922+
* tells a caller that a hidden row exists or when it was last written;
14923+
* - a row → the `409`, with THAT row as `currentRecord`, field-masked as
14924+
* the caller's own read is. `currentVersion` stays the token this
14925+
* comparison ran on.
14926+
*
14927+
* The read is issued only on a mismatch: a request whose token agrees, or
14928+
* that sends none, reads exactly what it read before. A read that throws
14929+
* (the caller's read grant withheld, a store fault) propagates as raised.
1490614930
*/
14907-
private assertVersionOf(
14931+
private async assertVersionOf(
1490814932
object: string,
1490914933
id: string,
1491014934
current: any,
1491114935
expectedVersion: string | undefined,
14912-
): void {
14936+
context: EngineQueryOptionsParsed['context'],
14937+
): Promise<void> {
1491314938
assertVersionTokenNotMalformed(expectedVersion);
1491414939
const expected = normaliseVersionToken(expectedVersion);
1491514940
if (!expected) return;
@@ -14927,9 +14952,11 @@ export class ObjectStackProtocolImplementation implements
1492714952
// it names, turning a resolvable conflict into an unresolvable one.
1492814953
const currentVersion = currentToken.instant ?? currentToken.token;
1492914954
const expectedVersionText = expected.instant ?? expected.token;
14955+
const visible = await this.readAsCaller(object, id, context);
14956+
if (!visible) throw recordNotFoundError(object, id);
1493014957
throw new ConcurrentUpdateError({
1493114958
currentVersion,
14932-
currentRecord: current,
14959+
currentRecord: visible,
1493314960
message: `Record ${object}/${id} was modified by another user (current version ${currentVersion}, expected ${expectedVersionText})`,
1493414961
});
1493514962
}
@@ -14956,11 +14983,24 @@ export class ObjectStackProtocolImplementation implements
1495614983
object: string,
1495714984
id: string,
1495814985
expectedVersion: string | undefined,
14986+
context: EngineQueryOptionsParsed['context'],
1495914987
): Promise<void> {
1496014988
assertVersionTokenNotMalformed(expectedVersion);
1496114989
if (!normaliseVersionToken(expectedVersion)) return;
1496214990
const current = await this.probeRecord(object, id);
14963-
this.assertVersionOf(object, id, current, expectedVersion);
14991+
await this.assertVersionOf(object, id, current, expectedVersion, context);
14992+
}
14993+
14994+
/**
14995+
* [#23586] The row as the CALLER reads it by id — the twin of
14996+
* {@link probeRecord}, which asks existence as the system. Same `context`
14997+
* spelling as the write it precedes: passed only when the request carried
14998+
* one, so a caller with no context reads as its write would run.
14999+
*/
15000+
private async readAsCaller(object: string, id: string, context: EngineQueryOptionsParsed['context']): Promise<any> {
15001+
const opts: EngineQueryOptionsParsed = { where: { id } };
15002+
if (context !== undefined) opts.context = context;
15003+
return this.engine.findOne(object, opts);
1496415004
}
1496515005

1496615006
// ==========================================

0 commit comments

Comments
 (0)