diff --git a/.changeset/occ-empty-etag-rejected-at-ingress.md b/.changeset/occ-empty-etag-rejected-at-ingress.md new file mode 100644 index 0000000000..4485ade2a1 --- /dev/null +++ b/.changeset/occ-empty-etag-rejected-at-ingress.md @@ -0,0 +1,62 @@ +--- +"@objectstack/metadata-protocol": minor +--- + +fix(metadata-protocol): refuse the quoted-empty `If-Match` entity-tag instead of silently disabling optimistic concurrency (#13576) + +**BREAKING** accept-set narrowing at the guarded-write door, shipped as +`minor` under the repo's launch-window convention for breaking changes. + +`If-Match: ""` — a syntactically legal RFC-7232 entity-tag with an EMPTY +opaque value — was silently accepted as "no version token supplied", which +**skipped the optimistic-concurrency guard entirely** on both `PATCH +/data/:object/:id` (via `If-Match` or the body's `expectedVersion` field) and +`DELETE /data/:object/:id` (via `If-Match` or the query's `expectedVersion`). +`normaliseVersionToken` strips the RFC-7232 quotes off the token and only +*then* checks emptiness, so `'""'` (2 chars, non-empty) passed every upstream +truthiness gate only to normalise to `''` one layer down — the exact falsy +value every caller's own `if (!token) return` reads as "the client sent +nothing". It was the one token shape that opted OUT of the guard instead of +failing it: a garbage-but-nonempty token (`v2`) has always failed *toward* +`409 CONCURRENT_UPDATE`, the safe direction for a concurrency primitive — +`""` failed toward silent, unguarded acceptance instead. + +**What changes.** Both doors now refuse `expectedVersion`/`If-Match: ""` at +ingress with `400 VALIDATION_FAILED`: + +> expectedVersion (If-Match) is the empty entity-tag `""`. An empty version +> token can never match any stored version, so this is almost certainly a +> client defect rather than a real concurrency check — send the real version +> token you read (e.g. the record's `updated_at`), or omit If-Match / +> expectedVersion entirely to perform an unguarded write. + +**What does NOT change** (both explicitly pinned as regression controls): +omitting `If-Match`/`expectedVersion` entirely is still a legal **unguarded** +write (opt-in semantics, unaffected) — including a bare unquoted empty string +or whitespace-only value, which is not the malformed shape and stays +opted-out; and a garbage-but-nonempty token (`v2`) still fails toward `409 +CONCURRENT_UPDATE`, unchanged. + +**Why 400 rather than 409** (a fail-closed alternative was considered and +rejected — maintainer ruling, 決裁批 #20 ①, 2026-08-31): a 409 would still +have collapsed two different facts into one answer — "you lost a race" +(retry-actionable) and "you sent a token that can never carry a version" +(a client-side bug, not a race). 400 keeps the two legible, which is the +entire point of refusing the *shape* rather than failing the comparison. +`""` is syntactically legal per RFC 7232 §2.3 (`*etagc` — zero or more — +permits an empty opaque-tag); this refusal is a deliberate platform CONTRACT +choice ("an empty tag can never match ⇒ it is necessarily a client defect"), +not a syntax verdict. + +**Who this affects.** Measured: the first-party Console never sends this +shape — `occVersionOf` (`plugin-form/src/occSave.tsx`) and its +`InlineEditSaveBar` counterpart in `objectui` only forward a **truthy** +`updated_at` string as `ifMatch`, and the `@object-ui/data-objectstack` +adapter only sets the `If-Match` header when `options.ifMatch` is itself +truthy — an empty value never reaches the wire on any first-party path. The +exposure was to third-party and hand-rolled clients sending the RFC-7232 +empty-tag shape, which previously got an unguarded write where they asked for +a guarded one. + + + diff --git a/content/docs/api/wire-format.mdx b/content/docs/api/wire-format.mdx index a031992f51..462877c9d6 100644 --- a/content/docs/api/wire-format.mdx +++ b/content/docs/api/wire-format.mdx @@ -198,7 +198,7 @@ Updates specific fields on an existing record. Only include fields you want to c ``` -**Optimistic concurrency:** Pass the `updated_at` value you last read as an `If-Match` request header (or an `expectedVersion` field in the body) and the server returns `409 CONCURRENT_UPDATE` if the record changed in the meantime. +**Optimistic concurrency:** Pass the `updated_at` value you last read as an `If-Match` request header (or an `expectedVersion` field in the body) and the server returns `409 CONCURRENT_UPDATE` if the record changed in the meantime. Omitting `If-Match`/`expectedVersion` entirely performs an unguarded write. Sending the empty entity-tag `If-Match: ""` (or `expectedVersion: '""'`) is refused `400 VALIDATION_FAILED` — an empty token can never match any stored version, so it is treated as a client defect rather than either "no guard requested" or a real conflict. ### Response — `200 OK` @@ -378,6 +378,24 @@ Returned when an `If-Match` / `expectedVersion` token no longer matches the stor } ``` +### Malformed Concurrency Token — `400 Bad Request` + +Returned when `If-Match` / `expectedVersion` is the empty entity-tag `""` — a +syntactically legal [RFC 7232](https://www.rfc-editor.org/rfc/rfc7232#section-2.3) +token, but one that can never match a stored version. Distinct from both the +409 above (a real token that lost a race) and an omitted `If-Match` (a +deliberate unguarded write): sending `""` is treated as a client defect, since +no stored version can ever equal "nothing". + +```json +{ + "error": "expectedVersion (If-Match) is the empty entity-tag \"\". An empty version token can never match any stored version, so this is almost certainly a client defect rather than a real concurrency check — send the real version token you read (e.g. the record's `updated_at`), or omit If-Match / expectedVersion entirely to perform an unguarded write.", + "code": "VALIDATION_FAILED", + "fields": [], + "object": "task" +} +``` + ### Datasource Unavailable — `503 Service Unavailable` Returned when the object's declared `datasource` has no live driver: the host's @@ -543,7 +561,7 @@ When the batch is not atomic and some records fail, each failing entry carries a | `Content-Type` | Yes | `application/json` | | `X-Request-Id` | No | Client-generated request ID for tracing (honored by the observability dispatcher) | | `X-Environment-Id` | No | Targets a specific environment/project on unscoped routes | -| `If-Match` | No | Optimistic-concurrency token for `PATCH` / `DELETE` (the `updated_at` you last read) | +| `If-Match` | No | Optimistic-concurrency token for `PATCH` / `DELETE` (the `updated_at` you last read). Omit for an unguarded write; the empty entity-tag `""` is refused `400`, not treated as omitted. | | `Accept-Language` | No | Locale for translated labels (e.g., `en-US`) | ### Response Headers diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 2242da20b7..2c92d20506 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -112,7 +112,7 @@ that silently does not happen. | 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10712` | | 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10874` | | 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9605` | -| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1576` | +| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1664` | | 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9642`, `readonly-strict-errors.ts:66` | | 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5639` | | 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3574`, `:3584`, `:3611` | diff --git a/content/docs/references/api/protocol.mdx b/content/docs/references/api/protocol.mdx index 80db78c89a..b2ba553f9e 100644 --- a/content/docs/references/api/protocol.mdx +++ b/content/docs/references/api/protocol.mdx @@ -581,7 +581,7 @@ A write-path strip event: caller-supplied fields legally dropped from the payloa | :--- | :--- | :--- | :--- | | **object** | `string` | ✅ | Object name | | **id** | `string` | ✅ | Record ID to delete | -| **expectedVersion** | `string` | optional | Optimistic concurrency token (typically the `updated_at` value the client read). When provided, the server compares it against the current record version and returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check. | +| **expectedVersion** | `string` | optional | Optimistic concurrency token (typically the `updated_at` value the client read). When provided, the server compares it against the current record version and returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check. The quoted-empty entity-tag (`""`) is refused 400 VALIDATION_FAILED, not treated as omitted. | --- @@ -2712,7 +2712,7 @@ Uninstall package response | **object** | `string` | ✅ | The object name. | | **id** | `string` | ✅ | The ID of the record to update. | | **data** | `Record` | ✅ | The fields to update (partial update). | -| **expectedVersion** | `string` | optional | Optimistic concurrency token (typically the `updated_at` value the client read). When provided, the server compares it against the current record version and returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check. | +| **expectedVersion** | `string` | optional | Optimistic concurrency token (typically the `updated_at` value the client read). When provided, the server compares it against the current record version and returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check. The quoted-empty entity-tag (`""`) is refused 400 VALIDATION_FAILED, not treated as omitted. | --- diff --git a/packages/metadata-protocol/src/protocol.occ-empty-etag-rejected.test.ts b/packages/metadata-protocol/src/protocol.occ-empty-etag-rejected.test.ts new file mode 100644 index 0000000000..83e6f2e189 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.occ-empty-etag-rejected.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13576] `If-Match: ""` — a valid RFC-7232 entity-tag with an EMPTY opaque + * value — is refused `400 VALIDATION_FAILED` at ingress, instead of being + * read as "no token supplied" and silently skipping the OCC guard. + * + * ## The defect this closes + * + * `normaliseVersionToken` strips RFC-7232 quotes off an `If-Match` value and + * THEN checks emptiness — so `'""'` (non-empty, 2 chars) passes every + * upstream truthiness check (the REST layer's `expectedVersion ? … : …`) + * only to normalise to the empty string `''` one layer down, which every + * caller's OWN falsiness test (`if (!expected) return`) reads as "the client + * sent no version" — the OPPOSITE of what `If-Match` requests. It is the one + * token shape that opts OUT of the guard rather than failing it: a garbage + * token (`v2`) still normalises to a real, comparable token and fails toward + * `409 CONCURRENT_UPDATE` — the safe direction for a concurrency primitive. + * + * ## The ruling (决裁批 #20 ①, maintainer, 2026-08-31) — option 3, verbatim + * + * `If-Match: ""` (header) and `expectedVersion: '""'` (body) are judged a + * MALFORMED concurrency token AT INGRESS and refused (400-family); the + * message must name the mechanism ("an empty token can never match any + * stored version — this is a client defect, not a lost race"), because that + * diagnostic distinction is the entire reason option 3 (refuse the shape) was + * chosen over option 2 (fail closed to 409, which would have collapsed "you + * sent something meaningless" into "you lost a race"). `""` is syntactically + * LEGAL per RFC-7232 §2.3 (`*etagc` — zero or more — permits an empty opaque + * tag); this refusal is an explicit platform CONTRACT choice ("an empty tag + * can never match ⇒ it is necessarily a client defect"), not a syntax + * verdict. Two things stay explicitly UNCHANGED: no `If-Match` at all is + * still a legal unguarded write, and a garbage-but-nonempty token (`v2`) + * still fails toward 409. + * + * ## The four-way pin set (Zone 3 of the dispatch order) — one describe each + * + * Each catches a DIFFERENT way a naive patch could overreach or underreach: + * 1. `""` → 400. (the fix itself) + * 2. no `If-Match` at all → unguarded write still succeeds. + * (catches a change that rejects EVERY falsy token, not just `""`) + * 3. `v2` (garbage, nonempty) → 409 still. + * (catches a change that also swallows the legitimate-conflict path) + * 4. a real, matching token → guarded write still succeeds. + * (catches a change that rejects even a well-formed token) + * + * ## A2.2 — TWO ingress doors, not one (falsified my own assumption) + * + * `assertVersionOf` (the PATCH door, called from `updateData` after the + * existence probe) and `assertVersionMatch` (the DELETE door, called from + * `deleteData` BEFORE any probe — and which short-circuits BEFORE ever + * calling `assertVersionOf`) each read `normaliseVersionToken`'s falsy return + * independently. A fix at one site alone leaves the other exhibiting the + * exact original defect — the DELETE-door describe block below regresses + * independently of the PATCH-door one for exactly this reason. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + assertEngineDeleteDispatch, + assertEngineFindOnePredicate, + assertEngineUpdateDispatch, +} from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation, MalformedVersionTokenError } from './protocol.js'; + +const SCHEMA = { + name: 'task', + fields: { + title: { name: 'title', type: 'text' }, + updated_at: { name: 'updated_at', type: 'datetime' }, + }, +}; + +/** A fake engine holding ONE row — mirrors `protocol.occ-version-token-instant.test.ts`'s fixture. */ +function makeProtocol(updatedAt: unknown) { + const row: Record = { id: 'rec_1', title: 'one', updated_at: updatedAt }; + const findOne = vi.fn(async (_object: string, opts: any) => { + assertEngineFindOnePredicate(_object, opts); + return String(opts?.where?.id) === 'rec_1' ? { ...row } : null; + }); + const update = vi.fn(async (_object: string, data: any, opts?: any) => { + const dispatch = assertEngineUpdateDispatch(data, opts); + if (dispatch.kind !== 'by-id') { + throw new Error(`fixture drives by-id updates only, got '${dispatch.kind}'`); + } + const fields = { ...(data as Record) }; + delete fields.id; + Object.assign(row, fields); + return { ...row }; + }); + const del = vi.fn(async (_object: string, opts?: any) => { + assertEngineDeleteDispatch(opts); + return String(opts?.where?.id) === 'rec_1'; + }); + const engine = { + registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, + findOne, + update, + delete: del, + }; + return { p: new ObjectStackProtocolImplementation(engine as any) as any, findOne, update, del }; +} + +const NOW = new Date('2026-08-30T10:19:25.947Z'); +const NOW_ISO = NOW.toISOString(); + +// ───────────────────────────────────────────────────────────────────────────── +// Pin 1 — `""` ⇒ 400, on BOTH doors, and the exact shipped message +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13576] pin 1 — the quoted-empty entity-tag is refused 400', () => { + it('PATCH: `expectedVersion: \'""\'` throws MalformedVersionTokenError (400 VALIDATION_FAILED), and writes nothing', async () => { + const { p, update } = makeProtocol(NOW); + await expect( + p.updateData({ object: 'task', id: 'rec_1', data: { title: 'edited' }, expectedVersion: '""' }), + ).rejects.toMatchObject({ code: 'VALIDATION_FAILED', status: 400, name: 'MalformedVersionTokenError' }); + expect(update).not.toHaveBeenCalled(); + }); + + it('DELETE: the same shape throws before the probe, and deletes nothing', async () => { + const { p, del, findOne } = makeProtocol(NOW); + await expect( + p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: '""' }), + ).rejects.toMatchObject({ code: 'VALIDATION_FAILED', status: 400 }); + expect(del).not.toHaveBeenCalled(); + expect(findOne).not.toHaveBeenCalled(); + }); + + it('the shipped error text names the MECHANISM — "cannot match" / "client defect" — not just a generic validation failure', async () => { + // Ruling clause ①: 文案不达意即白改 — the diagnostic value (this is a + // meaningless token, not a lost race) IS the reason option 3 beat + // option 2, so the message is asserted verbatim-in-substance, not just + // the code/status envelope. + const { p } = makeProtocol(NOW); + try { + await p.updateData({ object: 'task', id: 'rec_1', data: {}, expectedVersion: '""' }); + expect.unreachable('expected a MalformedVersionTokenError throw'); + } catch (e: any) { + expect(e).toBeInstanceOf(MalformedVersionTokenError); + expect(e.message).toMatch(/empty/i); + expect(e.message).toMatch(/never match/i); + expect(e.message).toMatch(/client defect/i); + // Tells the caller what to do instead — both remedies the ruling names. + expect(e.message).toMatch(/updated_at/); + expect(e.message).toMatch(/If-Match/); + } + }); + + it('surrounding whitespace around the quoted-empty tag is still caught (REST forwards the trimmed header verbatim)', async () => { + const { p } = makeProtocol(NOW); + await expect( + p.updateData({ object: 'task', id: 'rec_1', data: {}, expectedVersion: ' "" ' }), + ).rejects.toMatchObject({ code: 'VALIDATION_FAILED', status: 400 }); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Pin 2 — no `If-Match` at all ⇒ unguarded write still succeeds (Zone 1.2) +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13576] pin 2 — the legitimate no-token path is UNCHANGED', () => { + it('PATCH with no `expectedVersion` field at all still writes unguarded', async () => { + const { p, update } = makeProtocol(NOW); + const result = await p.updateData({ object: 'task', id: 'rec_1', data: { title: 'edited' } }); + expect(result.record.title).toBe('edited'); + expect(update).toHaveBeenCalledOnce(); + }); + + it('DELETE with no `expectedVersion` field at all still deletes unguarded', async () => { + const { p, del } = makeProtocol(NOW); + await p.deleteData({ object: 'task', id: 'rec_1' }); + expect(del).toHaveBeenCalledOnce(); + }); + + it('an unquoted empty string / whitespace-only token is NOT the malformed shape — still opts out (distinct from `\'""\'`)', async () => { + // These are what the REST layer's own `expectedVersion ? {...} : {}` + // truthiness gate already filters before a bare '' ever reaches this + // layer for the external HTTP doors — pinned here anyway because + // `updateData`/`deleteData` are also reachable directly (import-runner, + // action-execution), where no such gate runs. + const { p: p1, update } = makeProtocol(NOW); + await p1.updateData({ object: 'task', id: 'rec_1', data: { title: 'a' }, expectedVersion: '' }); + expect(update).toHaveBeenCalledOnce(); + const { p: p2, update: update2 } = makeProtocol(NOW); + await p2.updateData({ object: 'task', id: 'rec_1', data: { title: 'b' }, expectedVersion: ' ' }); + expect(update2).toHaveBeenCalledOnce(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Pin 3 — a garbage-but-nonempty token still fails toward 409 (Zone 1.2) +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13576] pin 3 — an opaque non-matching token is UNCHANGED — still 409, never 400', () => { + it('PATCH: `expectedVersion: \'v2\'` against a real stored version still throws ConcurrentUpdateError (409)', async () => { + const { p, update } = makeProtocol(NOW); + await expect( + p.updateData({ object: 'task', id: 'rec_1', data: { title: 'edited' }, expectedVersion: 'v2' }), + ).rejects.toMatchObject({ code: 'CONCURRENT_UPDATE', status: 409 }); + expect(update).not.toHaveBeenCalled(); + }); + + it('DELETE: the same garbage token still throws ConcurrentUpdateError (409), not the new 400', async () => { + const { p, del } = makeProtocol(NOW); + await expect( + p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: 'v2' }), + ).rejects.toMatchObject({ code: 'CONCURRENT_UPDATE', status: 409 }); + expect(del).not.toHaveBeenCalled(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Pin 4 — a real, matching token still guards the write through, successfully +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#13576] pin 4 — a real matching token still performs the guarded write', () => { + it('PATCH succeeds when `expectedVersion` matches the stored `updated_at`', async () => { + const { p, update } = makeProtocol(NOW); + const result = await p.updateData({ + object: 'task', id: 'rec_1', data: { title: 'edited' }, expectedVersion: NOW_ISO, + }); + expect(result.record.title).toBe('edited'); + expect(update).toHaveBeenCalledOnce(); + }); + + it('DELETE succeeds when `expectedVersion` matches the stored `updated_at`', async () => { + const { p, del } = makeProtocol(NOW); + await p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: NOW_ISO }); + expect(del).toHaveBeenCalledOnce(); + }); + + it('the RFC-7232-quoted form of the SAME real token still matches (quotes stripped, not the emptiness path)', async () => { + const { p, update } = makeProtocol(NOW); + const result = await p.updateData({ + object: 'task', id: 'rec_1', data: { title: 'edited' }, expectedVersion: `"${NOW_ISO}"`, + }); + expect(result.record.title).toBe('edited'); + expect(update).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts b/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts index 26aa7d9528..ff6ea1df0a 100644 --- a/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts +++ b/packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts @@ -49,6 +49,23 @@ * prose version of this claim was believed by three readers while * `If-Match: ""` had already flipped from accept to 409. * + * ## [#13576 amendment, 决裁批 #20 ①, 2026-08-31] ONE deliberate exception + * + * Invariant 5 now has exactly one carved-out exception, by maintainer ruling + * rather than by drift: the quoted-empty RFC-7232 entity-tag `""` — which this + * file's own tests below USED to pin as "still opts out" — is refused + * `400 VALIDATION_FAILED` at ingress instead. It was always the one token + * shape that opted OUT of the guard rather than failing it (unlike a + * garbage-but-nonempty token, which still fails toward 409); the ruling's + * reasoning is on {@link MalformedVersionTokenError}'s class doc in + * `protocol.ts`. Every OTHER pair this file pins is UNCHANGED — including + * `''`, whitespace-only, and the whitespace-*inside*-quotes shape `'" "'`, + * none of which are the empty-tag shape and none of which this ruling + * touches. Full pin coverage for the new behaviour (the four-way pin set, + * exact message text, ablation) lives in + * `protocol.occ-empty-etag-rejected.test.ts`; this file keeps only the narrow + * regression pins for the invariant it is about, updated to match. + * * ## What is deliberately NOT claimed here * * That the driver hands this seam a `Date` on Postgres. That is a fact about @@ -371,25 +388,36 @@ describe('[#13382] the change cannot refuse a token that was accepted before', ( expect(await guardedPatch(new Date(INSTANT), ' ')).toMatchObject({ accepted: true }); }); - it('an EMPTY If-Match entity-tag — a bare pair of quotes — still opts out', async () => { + it('[#13576] an EMPTY If-Match entity-tag — a bare pair of quotes — is now REFUSED, not opted out', async () => { // `If-Match: ""` is empty only AFTER the RFC-7232 quotes come off, so the // emptiness test has to run on the stripped token and not just the raw - // one. This is the shipped behaviour, not a new one: the pre-fix seam - // returned the empty STRING here and every caller short-circuited on its - // falsiness. Wrapping the result in an object made it truthy and turned - // this token from "skip the check" into a 409 — an accept-to-refuse flip - // on a live API, which is exactly what the widening guarantee forbids. - expect(await guardedPatch(new Date(INSTANT), '""')).toMatchObject({ accepted: true }); - expect(await guardedPatch(ISO, '""')).toMatchObject({ accepted: true }); + // one — `normaliseVersionToken` itself still reduces it to null, + // unchanged since #13382 (see that function's own doc). What changed is + // what the CALLER does with that null: `assertVersionTokenNotMalformed` + // now distinguishes "caller sent nothing" (`''`, unquoted — still opts + // out, pinned two tests up) from "caller sent a token that names + // nothing" (this — refused). Pre-#13576 this asserted `accepted: true`; + // the maintainer ruling (决裁批 #20 ①, 2026-08-31) is why it does not + // any more. + const verdict1 = await guardedPatch(new Date(INSTANT), '""'); + expect(verdict1).toMatchObject({ accepted: false, code: 'VALIDATION_FAILED', status: 400 }); + expect(verdict1.wrote).toBe(0); + const verdict2 = await guardedPatch(ISO, '""'); + expect(verdict2).toMatchObject({ accepted: false, code: 'VALIDATION_FAILED', status: 400 }); + expect(verdict2.wrote).toBe(0); }); - it('the DELETE door opts out on the same empty tag — it has its own call site', async () => { - // `assertVersionMatch` tests the token's truthiness itself, before it - // probes, so it can regress independently of the PATCH door. + it('[#13576] the DELETE door refuses the same empty tag BEFORE it ever probes', async () => { + // `assertVersionMatch` checks the token's malformed-ness itself, before + // it probes, so it can regress independently of the PATCH door — same + // reason the old "opts out" pin here had its own call site. const { p, del, findOne } = makeProtocol(new Date(INSTANT)); - await p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: '""' }); - expect(del).toHaveBeenCalledOnce(); - // Opting out also means NOT paying for the probe the OCC check needs. + await expect( + p.deleteData({ object: 'task', id: 'rec_1', expectedVersion: '""' }), + ).rejects.toMatchObject({ code: 'VALIDATION_FAILED', status: 400 }); + expect(del).not.toHaveBeenCalled(); + // The malformed check runs BEFORE the probe — a client defect is + // refused without paying for a read the request was never going to earn. expect(findOne).not.toHaveBeenCalled(); }); @@ -446,11 +474,21 @@ describe('[#13382] every pair the OLD comparison accepted is still accepted', () ['empty string', ''], ]; - /** What a client can put in `If-Match` / `expectedVersion`. */ + /** + * What a client can put in `If-Match` / `expectedVersion`. + * + * [#13576] Deliberately EXCLUDES `'""'` (the quoted-empty entity-tag): the + * maintainer ruling carved it out as the one exception to "the accept set + * only grows" (see this file's header amendment), so feeding it through + * this generic sweep would report a "regression" that is in fact the + * intended, ruling-authorized behaviour change — not a defect this sweep + * exists to catch. Its own pin lives two describe-blocks up and in + * `protocol.occ-empty-etag-rejected.test.ts`. `'" "'` (whitespace INSIDE + * the quotes, not empty) stays — it was never part of either defect. + */ const TOKENS: readonly string[] = [ ISO, `"${ISO}"`, - '""', '', ' ', '" "', diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 03be73cf57..4dd5ad2d31 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1370,6 +1370,84 @@ export class ConcurrentUpdateError extends Error { } } +/** + * [#13576] Thrown when a caller-supplied `expectedVersion` — the `If-Match` + * header or the body/query `expectedVersion` field — is a syntactically legal + * but semantically empty RFC-7232 entity-tag: the quoted-empty token `""`. + * + * ## Why this is refused rather than silently treated as "no token" (the + * pre-#13576 behaviour) or silently treated as a conflict (409) + * + * `""` is valid `entity-tag` grammar (RFC 7232 §2.3: `weak? DQUOTE *etagc + * DQUOTE`, and `*etagc` — zero or more — permits an EMPTY opaque-tag). But an + * empty tag can never equal any stored `updated_at`, quoted or not: it is not + * "the version I read", it is nothing. A client that sends it is asking for a + * guarded write and could not possibly have a real version in hand — RFC + * legality does not make it a meaningful concurrency token, and the platform + * makes an explicit contract choice to treat the shape itself as a client + * defect. Before this it fell through {@link normaliseVersionToken}'s + * quote-strip into the same falsy `''` that "caller sent nothing" produces, + * so the OPPOSITE of what `If-Match` requests happened: the guard was + * SKIPPED rather than evaluated, silently, on the one token shape that opts + * OUT of the check instead of failing it (unlike a garbage-but-nonempty token + * such as `v2`, which still normalises to a real, comparable token and fails + * toward `409 CONCURRENT_UPDATE` — the safe direction for a concurrency + * primitive, and unchanged by this fix). + * + * A `409` here (the alternative the maintainer ruling rejected, decision #20 + * ①, 2026-08-31) would have been safer than the pre-#13576 behaviour but + * would still have collapsed two different facts into one answer: "you lost a + * race" (409, actionable by reloading and retrying) and "you sent something + * that can never carry a version" (this — a client-side bug, not a race). A + * `400` keeps that distinction legible, which is the entire reason option 3 + * was chosen over option 2. + * + * `VALIDATION_FAILED`/400 is this package's own house code for a + * caller-request defect the engine never has to touch (mirrors + * {@link rowRequiredIdError}) — already registered in the ADR-0112 + * error-code ledger under `@objectstack/metadata-protocol`, so this needs no + * new ledger entry. + */ +export class MalformedVersionTokenError extends Error { + readonly code = 'VALIDATION_FAILED'; + readonly status = 400; + readonly fields: never[] = []; + constructor() { + super( + 'expectedVersion (If-Match) is the empty entity-tag `""`. An empty version ' + + 'token can never match any stored version, so this is almost certainly a ' + + 'client defect rather than a real concurrency check — send the real version ' + + 'token you read (e.g. the record\'s `updated_at`), or omit If-Match / ' + + 'expectedVersion entirely to perform an unguarded write.', + ); + this.name = 'MalformedVersionTokenError'; + } +} + +/** + * [#13576] Guard for the CLIENT-SUPPLIED half of an OCC token — never call + * this on `current.updated_at`, which is server-computed and must keep its + * existing "no check" fallback on any falsy normalisation. + * + * Detects exactly the RFC-7232 quoted-empty shape (`""`, any surrounding + * whitespace) and nothing wider: the raw value denotes SOMETHING (it is + * non-empty once trimmed — the "no token at all" cases, `undefined`/`null`/ + * `''`/whitespace-only, are unaffected and keep opting out of the guard, per + * the ruling's clause ②), yet {@link normaliseVersionToken} reduces it to + * nothing. That combination — "the caller supplied a token" AND "it + * normalises to no token" — has exactly one source in + * {@link normaliseVersionToken}'s body: the post-quote-strip emptiness check. + * A garbage-but-nonempty token (`v2`) normalises to a REAL (if opaque) token, + * so it never matches this predicate and keeps failing toward 409 unchanged. + */ +function assertVersionTokenNotMalformed(expectedVersion: string | undefined): void { + if (expectedVersion === null || expectedVersion === undefined) return; + const raw = String(expectedVersion).trim(); + if (!raw) return; // no token at all — unaffected, stays "opt out of the guard" + if (normaliseVersionToken(expectedVersion) !== null) return; // a real, comparable token + throw new MalformedVersionTokenError(); +} + /** * An ISO-8601 date-time that denotes an ABSOLUTE instant — it carries an * explicit `Z` or a numeric `±HH:mm` offset, so reading it never consults the @@ -1453,6 +1531,16 @@ function canonicalVersionInstant(value: unknown, token: string): string | null { * whitespace, and returns null for empty / nullish input — a null is the * caller's "no token supplied", which opts out of the check. * + * [#13576] This function's OWN contract is deliberately unchanged: `""` still + * normalises to null here, exactly as #13382 left it (see the load-bearing + * comment on the post-strip check below) — that is what keeps this a pure, + * side-effect-free reader that the SERVER-COMPUTED `current.updated_at` call + * can keep using with its "no check" fallback. The client-facing call sites + * (`assertVersionOf`, `assertVersionMatch`) now call + * {@link assertVersionTokenNotMalformed} FIRST, which distinguishes "caller + * sent nothing" (stays opted out) from "caller sent the quoted-empty tag" + * (throws 400) before either ever reaches this function's uniform null. + * * ## Why this returns two forms rather than one string (#13382) * * It used to return `String(v).trim()` alone, and the two sides of the OCC @@ -10126,6 +10214,13 @@ export class ObjectStackProtocolImplementation implements * Behaviour: * - Empty/missing token → no check (opt-in semantics; existing callers * that haven't yet adopted OCC are unaffected). + * - [#13576] The one exception to "empty → no check": a token that is + * PRESENT but is the quoted-empty RFC-7232 entity-tag (`""`) is a + * malformed concurrency token, not an absent one — see + * {@link assertVersionTokenNotMalformed} — and throws + * `MalformedVersionTokenError` (400) instead of silently skipping the + * guard. Every other falsy shape (no header/field at all, `''`, + * whitespace-only) is untouched by this and still opts out. * - Record not found → no check. We intentionally do not treat "missing * record" as a concurrency conflict; `updateData` has already answered * 404 by this point, and `deleteData` lets the driver report it. @@ -10147,6 +10242,7 @@ export class ObjectStackProtocolImplementation implements current: any, expectedVersion: string | undefined, ): void { + assertVersionTokenNotMalformed(expectedVersion); const expected = normaliseVersionToken(expectedVersion); if (!expected) return; if (!current) return; @@ -10176,12 +10272,24 @@ export class ObjectStackProtocolImplementation implements * existence probe of its own: the driver's own `delete` return reports * whether a row matched (#4435). So this still probes ONLY when the caller * actually opted into OCC, keeping a plain DELETE at zero extra reads. + * + * [#13576] The malformed-token check runs FIRST, before the probe — a + * malformed client token is a request defect independent of whether the + * target record exists, and rejecting it costs nothing extra (no probe + * needed either way). This is a SEPARATE call from the one inside + * {@link assertVersionOf}, not a delegation to it: this function returns + * early on a well-formed-but-absent token (`if (!normaliseVersionToken(…)) + * return`) BEFORE ever reaching `assertVersionOf`, so without its own + * check here a malformed token would take that same early return and the + * guard would stay silently skipped — exactly the two-doors shape the + * original defect had (measured, #13576 A2.2). */ private async assertVersionMatch( object: string, id: string, expectedVersion: string | undefined, ): Promise { + assertVersionTokenNotMalformed(expectedVersion); if (!normaliseVersionToken(expectedVersion)) return; const current = await this.probeRecord(object, id); this.assertVersionOf(object, id, current, expectedVersion); diff --git a/packages/spec/src/api/protocol.zod.ts b/packages/spec/src/api/protocol.zod.ts index a49e0ec997..5b80d67d49 100644 --- a/packages/spec/src/api/protocol.zod.ts +++ b/packages/spec/src/api/protocol.zod.ts @@ -2097,7 +2097,8 @@ export const UpdateDataRequestSchema = lazySchema(() => z.object({ expectedVersion: z.string().optional().describe( 'Optimistic concurrency token (typically the `updated_at` value the client read). ' + 'When provided, the server compares it against the current record version and ' + - 'returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check.' + 'returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check. ' + + 'The quoted-empty entity-tag (`""`) is refused 400 VALIDATION_FAILED, not treated as omitted.' ), })); @@ -2128,7 +2129,8 @@ export const DeleteDataRequestSchema = lazySchema(() => z.object({ expectedVersion: z.string().optional().describe( 'Optimistic concurrency token (typically the `updated_at` value the client read). ' + 'When provided, the server compares it against the current record version and ' + - 'returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check.' + 'returns 409 CONCURRENT_UPDATE if they differ. Optional — omit to skip the check. ' + + 'The quoted-empty entity-tag (`""`) is refused 400 VALIDATION_FAILED, not treated as omitted.' ), })); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 7a6004b4d0..5a1f78b3cb 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -771,6 +771,21 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/metadata-protocol/src/protocol.occ-empty-etag-rejected.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/metadata-protocol/src/protocol.occ-empty-etag-rejected.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/metadata-protocol/src/protocol.occ-empty-etag-rejected.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts", "verb": "delete",