From f566a1d255ebe42e4835f2e6fd8d18ce6d24319e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:49:38 +0000 Subject: [PATCH 1/8] fix(spec): the $search auto field set's lead ORDERS the set, it must not admit one (#4483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `autoDefaultFields` filtered every field through three exclusions (`SEARCH_AUTO_EXCLUDED_FIELDS`, `hidden`, unsearchable type) and then prepended the display/name/title field on an EXISTENCE check alone — so the exclusions did not hold for whichever field happened to lead, and the module's own "system / audit / heavy fields never auto-included" invariant was false. Not a contrived shape: ADR-0079's `provisionPrimary(schema, { synthesize: false })` designates `nameField` at registration, and on a table whose only textual column IS the primary key (system tables, junction tables, append-only logs) it designates `id`. `$search` then expanded to `{ id: { $contains: term } }` — a substring scan over the primary key, returning a narrow and semantically wrong row set. It loosened a second layer too: `resolveSearchFieldResolution` is also the #4254 REST ingress gate's arbiter for "would the engine actually scan this field", so with `id` in `allowed` a `$searchFields=id` override was ACCEPTED rather than refused. The lead's job is to put the primary title FIRST, never to admit it, so it is now chosen from the already-filtered set. An excluded / hidden / unsearchable display field simply does not lead and the set is unchanged; an eligible one still leads, so the ordering intent is intact. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- packages/spec/src/data/search-fields.test.ts | 120 +++++++++++++++++++ packages/spec/src/data/search-fields.ts | 30 ++++- 2 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 packages/spec/src/data/search-fields.test.ts diff --git a/packages/spec/src/data/search-fields.test.ts b/packages/spec/src/data/search-fields.test.ts new file mode 100644 index 0000000000..8011bee622 --- /dev/null +++ b/packages/spec/src/data/search-fields.test.ts @@ -0,0 +1,120 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + resolveSearchFieldResolution, + resolveSearchFields, + SEARCH_AUTO_EXCLUDED_FIELDS, +} from './search-fields'; + +// --------------------------------------------------------------------------- +// [#4483] The auto-default's "lead" field ORDERS the set; it must not ADMIT one. +// +// `autoDefaultFields` filters every field through three exclusions, then used to +// prepend the display/name/title field on an EXISTENCE check alone — so the +// exclusions did not hold for whichever field happened to lead. The regression +// this pins is not hypothetical: ADR-0079 designates `nameField` at +// registration, and on a table whose only textual column is the primary key it +// designates `id`, turning `$search` into a substring scan over the PK. +// --------------------------------------------------------------------------- +describe('[#4483] $search auto field set — lead orders, never admits', () => { + const pkOnly = { + id: { type: 'text' }, + amount: { type: 'number' }, + }; + + it('excludes `id` with no display field (the already-correct baseline)', () => { + expect(resolveSearchFieldResolution({ fields: pkOnly })).toEqual({ + allowed: [], + source: 'auto', + }); + }); + + it('a displayField on the exclusion list does NOT re-enter the set', () => { + // Pre-#4483 this returned `{ allowed: ['id'] }`. + expect(resolveSearchFieldResolution({ fields: pkOnly, displayField: 'id' })).toEqual({ + allowed: [], + source: 'auto', + }); + }); + + it('every SEARCH_AUTO_EXCLUDED_FIELDS member stays out even as displayField', () => { + for (const excluded of SEARCH_AUTO_EXCLUDED_FIELDS) { + const { allowed } = resolveSearchFieldResolution({ + fields: { [excluded]: { type: 'text' }, title: { type: 'text' } }, + displayField: excluded, + }); + expect(allowed, `'${excluded}' leaked into the auto set as displayField`).toEqual(['title']); + } + }); + + it('a hidden display field does not lead and does not enter', () => { + const { allowed } = resolveSearchFieldResolution({ + fields: { name: { type: 'text', hidden: true }, subject: { type: 'text' } }, + displayField: 'name', + }); + expect(allowed).toEqual(['subject']); + }); + + it('a display field of an unsearchable TYPE does not enter', () => { + const { allowed } = resolveSearchFieldResolution({ + fields: { avatar: { type: 'image' }, subject: { type: 'text' } }, + displayField: 'avatar', + }); + expect(allowed).toEqual(['subject']); + }); + + it('the `name` / `title` bypasses are gated by the same predicate', () => { + // Both exist but are unsearchable — neither may lead nor enter. + const { allowed } = resolveSearchFieldResolution({ + fields: { + name: { type: 'json' }, + title: { type: 'vector' }, + subject: { type: 'text' }, + }, + }); + expect(allowed).toEqual(['subject']); + }); + + it('an ELIGIBLE display field still leads — the ordering intent is intact', () => { + const { allowed } = resolveSearchFieldResolution({ + fields: { + code: { type: 'text' }, + subject: { type: 'text' }, + stage: { type: 'select' }, + }, + displayField: 'subject', + }); + expect(allowed[0]).toBe('subject'); + expect(new Set(allowed)).toEqual(new Set(['subject', 'code', 'stage'])); + }); + + it('falls back to `name`, then `title`, for the lead position', () => { + expect( + resolveSearchFieldResolution({ + fields: { code: { type: 'text' }, name: { type: 'text' } }, + }).allowed[0], + ).toBe('name'); + expect( + resolveSearchFieldResolution({ + fields: { code: { type: 'text' }, title: { type: 'text' } }, + }).allowed[0], + ).toBe('title'); + }); + + it('a declared `searchableFields` list is unaffected by the lead rule', () => { + // `declared` is the author's explicit choice and bypasses the auto-default + // entirely — including its exclusions. Pinned so the fix is not read as + // narrowing the declared path too. + expect( + resolveSearchFieldResolution({ fields: pkOnly, searchableFields: ['id'], displayField: 'id' }), + ).toEqual({ allowed: ['id'], source: 'declared' }); + }); + + it('the #4254 ingress gate no longer admits `$searchFields=id`', () => { + // `resolveSearchFields` intersects the override with `allowed`; with `id` + // out of `allowed` the override matches nothing and cannot widen the scan. + expect(resolveSearchFields({ fields: pkOnly, displayField: 'id', requestedFields: 'id' })) + .toEqual([]); + }); +}); diff --git a/packages/spec/src/data/search-fields.ts b/packages/spec/src/data/search-fields.ts index fcd63f10de..982d8aecb5 100644 --- a/packages/spec/src/data/search-fields.ts +++ b/packages/spec/src/data/search-fields.ts @@ -72,10 +72,32 @@ function autoDefaultFields(fields: Record, displayField if (SEARCH_AUTO_EXCLUDED_TYPES.has(t)) return false; return SEARCHABLE_TEXTUAL_TYPES.has(t) || SEARCHABLE_ENUM_TYPES.has(t); }); - // Lead with the display/name field when present. - const lead = displayField && fields[displayField] ? displayField - : fields.name ? 'name' - : fields.title ? 'title' + // Lead with the display/name field — ORDERING ONLY (#4483). + // + // `lead` used to be picked by EXISTENCE (`fields[displayField]`), then + // prepended unconditionally, so it re-entered the set after the three + // exclusions above had already rejected it. That made + // `SEARCH_AUTO_EXCLUDED_FIELDS` — whose contract is "never auto-included" — + // untrue for whichever field happened to be the display field, and the case + // is not contrived: ADR-0079's `provisionPrimary(schema, { synthesize: false })` + // designates `nameField` at registration, and on a table whose only textual + // column IS the primary key (system tables, junction tables, append-only + // logs) it designates `id`. `$search` then expanded to + // `{ id: { $contains: } }` — a substring scan over the primary key. + // + // It also loosened the #4254 REST ingress gate one layer up, which asks this + // same resolution whether a `$searchFields` override names a field the engine + // would actually scan: with `id` in `allowed`, `$searchFields=id` was + // ACCEPTED instead of refused. + // + // The lead's job is to put the primary title FIRST, never to admit it, so it + // is now chosen from `names` — the already-filtered set. A display field that + // is excluded, hidden or of an unsearchable type simply does not lead, and + // the set is unchanged. + const eligible = (f: string | undefined): f is string => !!f && names.includes(f); + const lead = eligible(displayField) ? displayField + : eligible('name') ? 'name' + : eligible('title') ? 'title' : undefined; if (!lead) return names; return [lead, ...names.filter((f) => f !== lead)]; From cb1e01e1759735c5bf0572a4eae5df375759777e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:49:51 +0000 Subject: [PATCH 2/8] wip(drivers): give the uncompilable-filter refusal an ADR-0112 code and drop the driver prefix (#4436) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IN PROGRESS — code change complete, regression test not yet written and the real-boot curl repro not yet run. A filter carrying an operator the driver cannot compile is already REFUSED rather than silently matched (#4209/#4029), but the refusal had no wire identity: the thrown `Error` carried no `code`, so `mapDataError`'s default branch served `{"error": "[sql-driver] Unsupported filter operator …"}` — no `error.code` at all, breaking the ADR-0112 contract every sibling rejection on the same route already honours (`INVALID_FIELD`, `INVALID_FILTER`, `RECORD_NOT_FOUND`), and leaking the `[sql-driver]` internal prefix that the #3867 sanitiser exists to keep off the wire. Both drivers now throw through a local `unsupportedFilterError` that stamps `code = StandardErrorCode.enum.INVALID_FILTER` (the same catalogued code `metadata-protocol` emits when a filter fails to parse upstream — one condition, one wire code however the caller reached it) and `status = 400`, which also puts the rejection on `isExpectedQueryRejection` so a client mistake stops being logged as an unhandled server error. The internal prefix is gone from the message; the actionable operator/field/vocabulary detail stays. Applied to every filter-COMPILATION refusal in both backends, not just the one branch the issue names — they are the same envelope defect on adjacent lines, and #3948 made the two drivers agree that an uncompilable filter is a refusal, so their refusal envelopes have to agree too. TODO: regression tests (driver-sql, driver-memory, REST envelope) + boot repro. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../driver-memory/src/memory-driver.ts | 35 +++++++++++---- packages/plugins/driver-sql/src/sql-driver.ts | 45 ++++++++++++++++--- 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/packages/plugins/driver-memory/src/memory-driver.ts b/packages/plugins/driver-memory/src/memory-driver.ts index 229f13ad86..10aa608901 100644 --- a/packages/plugins/driver-memory/src/memory-driver.ts +++ b/packages/plugins/driver-memory/src/memory-driver.ts @@ -3,6 +3,7 @@ import type { QueryAST, QueryInput, DriverOptions } from '@objectstack/spec/data'; import { canonicalAstOperator } from '@objectstack/spec/data'; import type { IDataDriver } from '@objectstack/spec/contracts'; +import { StandardErrorCode } from '@objectstack/spec/api'; import { Logger, createLogger, nextUtcCalendarDay } from '@objectstack/core'; import { Query, Aggregator } from 'mingo'; import { getValueByPath } from './memory-matcher.js'; @@ -12,6 +13,24 @@ import { type TemporalFieldKind, } from './memory-temporal.js'; +/** + * [#4436] A filter this driver cannot COMPILE — see the twin in + * `driver-sql`'s `unsupportedFilterError`, which carries the full rationale. + * + * Kept in lockstep with driver-sql deliberately: #3948 made the two backends + * AGREE that an uncompilable filter is a refusal rather than a silent + * match-everything, and the refusal's wire envelope has to agree too. A test + * suite that swaps the memory driver for SQL must see the same `400 + * INVALID_FILTER`, not a coded refusal on one backend and a bare `{error}` on + * the other. + */ +function unsupportedFilterError(message: string): Error { + const err = new Error(message) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_FILTER; + err.status = 400; + return err; +} + /** * Persistence adapter interface. * Matches the PersistenceAdapterSchema contract from @objectstack/spec. @@ -764,8 +783,8 @@ export class InMemoryDriver implements IDataDriver { // matches EVERY record. An unapplied filter must not look like a // satisfied one. #3948. if (lower !== 'and' && lower !== 'or') { - throw new Error( - `[driver-memory] Unrecognized filter operator "${item}" in a comparison triple. ` + + throw unsupportedFilterError( + `Unrecognized filter operator "${item}" in a comparison triple. ` + `A filter array is either a logical node (["and"|"or", …]) or nested ` + `conditions ([[field, op, value], …]); a bare [field, op, value] only ` + `reaches the driver when its operator is outside @objectstack/spec ` + @@ -785,8 +804,8 @@ export class InMemoryDriver implements IDataDriver { const cond = this.convertConditionToMongo(field, operator, value, object); if (cond) logicGroups[logicGroups.length - 1].conditions.push(cond); } else { - throw new Error( - `[driver-memory] Unrecognized filter element of type ` + + throw unsupportedFilterError( + `Unrecognized filter element of type ` + `"${item === null ? 'null' : typeof item}" — expected a logical keyword ` + `("and"/"or") or a condition array. Filter was: ${JSON.stringify(filters)}`, ); @@ -874,16 +893,16 @@ export class InMemoryDriver implements IDataDriver { : { $gte: store(value[0]), $lte: store(value[1]) }, }; } - throw new Error( - `[driver-memory] "between" on field "${field}" needs a two-element array, got ` + + throw unsupportedFilterError( + `"between" on field "${field}" needs a two-element array, got ` + `${JSON.stringify(value)}. Returning no predicate would silently match every record.`, ); default: // Was `return null`, which the caller dropped — so an operator this // driver cannot express narrowed nothing instead of erroring. driver-sql // already threw on the same input; the two backends disagreed. #3948. - throw new Error( - `[driver-memory] Unsupported filter operator "${operator}" on field "${field}". ` + + throw unsupportedFilterError( + `Unsupported filter operator "${operator}" on field "${field}". ` + `Supported operators: =, !=, <, <=, >, >=, in, nin, between, contains, ` + `not_contains, starts_with, ends_with (see @objectstack/spec VALID_AST_OPERATORS).`, ); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index f00e6d96f1..f8414ebd15 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -12,6 +12,7 @@ import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyD import { STRUCTURED_JSON_TYPES, FILE_REFERENCE_TYPES, MULTI_OPTION_TYPES, NUMERIC_VALUE_TYPES } from '@objectstack/spec/data'; import { canonicalAstOperator } from '@objectstack/spec/data'; import type { IDataDriver } from '@objectstack/spec/contracts'; +import { StandardErrorCode } from '@objectstack/spec/api'; import { StorageNameMapping } from '@objectstack/spec/system'; import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; import { resolveMultiOrgEnabled } from '@objectstack/types'; @@ -378,6 +379,40 @@ function canonicalTimeOfDay(value: unknown): unknown { */ const SQLITE_TIME_EXPR_REFS = 8; +/** + * [#4436] A filter this driver cannot COMPILE — the caller sent an operator (or + * an operand shape) outside what the backend can express. + * + * This is a refusal the request caused, and #4209/#4029 already made it a + * refusal rather than a silent match-everything. What was missing is the wire + * IDENTITY of that refusal: the thrown `Error` carried no `code`, so + * `mapDataError`'s default branch served `{ "error": "" }` with no + * `code` at all — breaking the ADR-0112 contract that `error.code` is the + * schema-enforced SCREAMING_SNAKE vocabulary every sibling rejection on this + * route already speaks (`INVALID_FIELD`, `INVALID_FILTER`, `RECORD_NOT_FOUND`). + * + * `INVALID_FILTER` is the catalogued code for the condition, and the SAME one + * `metadata-protocol` emits for a filter that fails to parse upstream + * (`malformedFilterArrayError` / `unusableFilterError`): one condition — "this + * filter cannot run" — has one wire code however the caller reached it. + * + * `status: 400` makes `@objectstack/rest`'s `sendError` pass the message + * through instead of routing it to the SQL-leak heuristic, and puts the + * rejection on the `isExpectedQueryRejection` list so a client mistake stops + * being logged as an unhandled server error. + * + * The `[sql-driver]` prefix these messages used to carry is GONE from the text: + * it is driver-internal wording, and shipping it to clients is exactly what the + * #3867 sanitiser exists to stop. The operator/field/vocabulary detail — the + * part a caller can act on — stays. + */ +function unsupportedFilterError(message: string): Error { + const err = new Error(message) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_FILTER; + err.status = 400; + return err; +} + // ── Introspection Types ────────────────────────────────────────────────────── export interface IntrospectedColumn { @@ -5259,7 +5294,7 @@ export class SqlDriver implements IDataDriver { case 'between': { const arr = Array.isArray(coerced) ? coerced : []; if (arr.length !== 2) { - throw new Error(`[sql-driver] operator "between" on field "${field}" requires a [min, max] value array.`); + throw unsupportedFilterError(`Operator "between" on field "${field}" requires a [min, max] value array.`); } builder[join === 'or' ? 'orWhereBetween' : 'whereBetween'](field, arr as [any, any]); return; @@ -5298,8 +5333,8 @@ export class SqlDriver implements IDataDriver { builder[whereNotNull](field); return; default: - throw new Error( - `[sql-driver] Unsupported filter operator "${op}" on field "${field}". Supported operators: ` + + throw unsupportedFilterError( + `Unsupported filter operator "${op}" on field "${field}". Supported operators: ` + `=, !=, <, <=, >, >=, in, nin, between, contains, not_contains, starts_with, ends_with, ` + `is_null, is_not_null (see @objectstack/spec VALID_AST_OPERATORS).`, ); @@ -5472,8 +5507,8 @@ export class SqlDriver implements IDataDriver { : (logicalOp === 'or' ? 'orWhereNotNull' : 'whereNotNull')](field); break; default: - throw new Error( - `[sql-driver] Unsupported filter operator "${op}" on field "${field}". Supported operators: ` + + throw unsupportedFilterError( + `Unsupported filter operator "${op}" on field "${field}". Supported operators: ` + `$eq, $ne, $gt, $gte, $lt, $lte, $in, $nin, $between, $contains, $notContains, ` + `$startsWith, $endsWith, $regex, $null, $exists.`, ); From 9cd0e2958af343457410cbb01230fef07da81127 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:55:09 +0000 Subject: [PATCH 3/8] fix(drivers): the uncompilable-filter refusal speaks INVALID_FILTER, without the driver prefix (#4436) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the WIP commit: adds the remaining sql-driver throw sites and the regression tests for both backends. #4209/#4029/#3948 settled the POSTURE — a filter carrying an operator the driver cannot compile is refused instead of silently matching every row. What was missing is the refusal's IDENTITY on the wire. The driver threw a bare `Error`, so `mapDataError` fell through to its default branch and served a body whose only key was `error`: GET /api/v1/data/showcase_task?filter={"title":{"$bogusop":"x"}} → 400 {"error":"[sql-driver] Unsupported filter operator \"$bogusop\" …"} Two contract breaks in one body — no `error.code` at all on a route whose sibling rejections all speak the ADR-0112 catalogue, and the driver-internal `[sql-driver]` prefix on the wire, which is what the #3867 sanitiser exists to stop. Fixed at the throw site (PD #12), not by teaching the REST layer to guess: both drivers now refuse through an `unsupportedFilterError` helper that stamps `code = StandardErrorCode.enum.INVALID_FILTER` — the constant, so a catalogue rename breaks the compile — and `status = 400`. `INVALID_FILTER` is the same code `metadata-protocol` already emits when a filter fails to parse upstream (`malformedFilterArrayError` / `unusableFilterError`): one condition, one wire code, however the caller reached it. The `status` also puts the rejection on `isExpectedQueryRejection`, so a client mistake stops being logged as an unhandled server error. Applied to every filter-COMPILATION refusal in both backends, not only the one branch the issue names: unsupported operator ($-object, legacy triple), unrecognised logical keyword, unrecognised element type, and a `between` / `$between` operand that is not a two-element array. They are the same envelope defect on adjacent lines, and #3948 made the two drivers agree that an uncompilable filter is a refusal — so their refusal envelopes have to agree too, or the cross-driver parity this repo relies on is false where it matters. Tests: new `sql-driver-filter-refusal-envelope.test.ts` (8) and `memory-filter-refusal-envelope.test.ts` (5) pin `code`, `status`, the absence of the internal prefix, and that the actionable operator/field/vocabulary detail survives. Full suites green: driver-sql 623 passed, driver-memory 286 passed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../memory-filter-refusal-envelope.test.ts | 75 ++++++++++++ ...sql-driver-filter-refusal-envelope.test.ts | 114 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 10 +- 3 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts create mode 100644 packages/plugins/driver-sql/src/sql-driver-filter-refusal-envelope.test.ts diff --git a/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts b/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts new file mode 100644 index 0000000000..1114c4cf43 --- /dev/null +++ b/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts @@ -0,0 +1,75 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4436] The memory driver's filter refusals carry the SAME wire identity as + * driver-sql's. + * + * #3948 made the two backends agree that an uncompilable filter is a refusal + * rather than a silent match-everything. The refusal's ENVELOPE has to agree + * too, or a suite that swaps the memory driver for SQLite sees a coded 400 on + * one backend and a bare `{ error }` on the other — and the cross-driver parity + * this driver exists to provide would be false exactly where it is load-bearing. + * + * Twin of `driver-sql/src/sql-driver-filter-refusal-envelope.test.ts`; the + * rationale lives there. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { InMemoryDriver } from './memory-driver.js'; +import type { FilterCondition } from '@objectstack/spec/data'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e as WireBearingError; + } + throw new Error('expected the driver to refuse this filter, but it resolved'); +} + +describe('[#4436] InMemoryDriver filter refusals carry INVALID_FILTER and leak no driver prefix', () => { + let driver: InMemoryDriver; + + beforeEach(async () => { + driver = new InMemoryDriver(); + await driver.syncSchema?.({ + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, + } as any); + await driver.create('deal', { id: '1', stage: 'won', amount: 10 }); + }); + + const find = (where: unknown) => + driver.find('deal', { object: 'deal', fields: ['id'], where: where as FilterCondition }); + + const cases: Array<[string, unknown, string]> = [ + ['unsupported operator in a condition array', [['stage', 'sounds_like', 'won']], 'sounds_like'], + ['bare comparison triple', ['close_date', 'before', '2024-01-01'], 'close_date'], + ['filter element of the wrong type', [42], 'number'], + ['`between` with a bad operand', [['amount', 'between', 5]], 'between'], + ]; + + for (const [name, where, needle] of cases) { + it(`${name} → 400 INVALID_FILTER, no prefix`, async () => { + const err = await refusalOf(() => find(where)); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).not.toContain('[driver-memory]'); + expect(err.message).toContain(needle); + }); + } + + it('a compilable filter is unaffected', async () => { + const rows = await find({ stage: 'won' }); + expect(rows.map((r: any) => r.id)).toEqual(['1']); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-filter-refusal-envelope.test.ts b/packages/plugins/driver-sql/src/sql-driver-filter-refusal-envelope.test.ts new file mode 100644 index 0000000000..4ba9bcb56c --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-filter-refusal-envelope.test.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4436] The uncompilable-filter refusal must carry an ADR-0112 wire identity. + * + * #4209/#4029/#3948 settled the POSTURE — a filter the driver cannot compile is + * refused instead of silently matching every row. What was still missing is the + * refusal's IDENTITY on the wire. + * + * The driver threw a bare `Error`, so it carried no `code` and no `status`. + * `@objectstack/rest`'s `mapDataError` therefore fell all the way through to its + * default branch — `{ status: 400, body: { error: raw } }` — and served a body + * whose only key was `error`: + * + * ``` + * GET /api/v1/data/showcase_task?filter={"title":{"$bogusop":"x"}} + * → 400 {"error":"[sql-driver] Unsupported filter operator \"$bogusop\" …"} + * ``` + * + * Two contract breaks in one body: no `error.code` at all, on a route whose + * sibling rejections all speak the catalogued vocabulary (`INVALID_FIELD`, + * `INVALID_FILTER`, `RECORD_NOT_FOUND`); and the driver-internal `[sql-driver]` + * prefix on the wire, which is precisely what the #3867 sanitiser exists to + * stop. + * + * These tests pin BOTH halves at the throw site, because that is where the fix + * lives (PD #12 — the producer declares its own refusal; the REST layer is not + * patched to guess). + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import type { FilterCondition } from '@objectstack/spec/data'; + +/** The shape `mapDataError` / `sendError` read off a thrown driver error. */ +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e as WireBearingError; + } + throw new Error('expected the driver to refuse this filter, but it resolved'); +} + +describe('[#4436] SqlDriver filter refusals carry INVALID_FILTER and leak no driver prefix', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, + } as any, + ]); + await driver.create('deal', { id: '1', stage: 'won', amount: 10 }); + }); + + const find = (where: unknown) => + driver.find('deal', { object: 'deal', fields: ['id'], where: where as FilterCondition }); + + // The issue's own repro, at the layer that produces the envelope. + it('the $-object unsupported-operator branch — the exact shape #4436 reported', async () => { + const err = await refusalOf(() => find({ stage: { $bogusop: 'x' } })); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).not.toContain('[sql-driver]'); + // The actionable half survives the sanitisation — a caller must still be + // able to see WHICH operator on WHICH field, and what is accepted instead. + expect(err.message).toContain('$bogusop'); + expect(err.message).toContain('stage'); + expect(err.message).toContain('$startsWith'); + }); + + // Every filter-COMPILATION refusal in this driver answers the same way. They + // are one condition — "this filter cannot run" — and ADR-0112's rule is one + // condition, one wire code, however the caller reached it. + const cases: Array<[string, unknown, string]> = [ + ['legacy triple, unsupported operator', [['stage', 'sounds_like', 'won']], 'sounds_like'], + ['bare comparison triple', ['close_date', 'before', '2024-01-01'], 'close_date'], + ['filter element of the wrong type', [42], 'number'], + ['null filter element', [null], 'null'], + ['legacy `between` with a bad operand', [['amount', 'between', 5]], 'between'], + ['$between with a bad operand', { amount: { $between: 5 } }, '$between'], + ]; + + for (const [name, where, needle] of cases) { + it(`${name} → 400 INVALID_FILTER, no prefix`, async () => { + const err = await refusalOf(() => find(where)); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).not.toContain('[sql-driver]'); + expect(err.message).toContain(needle); + }); + } + + it('a compilable filter is unaffected', async () => { + const rows = await find({ stage: 'won' }); + expect(rows.map((r: any) => r.id)).toEqual(['1']); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index f8414ebd15..1e52625c4c 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -5115,8 +5115,8 @@ export class SqlDriver implements IDataDriver { // never converted it and the raw array arrived as `where`. Skipping it // (the old behaviour) emitted NO predicate at all: the caller asked to // filter and silently got every row. Fail loudly instead. #3948. - throw new Error( - `[sql-driver] Unrecognized filter operator "${item}" in a comparison triple. ` + + throw unsupportedFilterError( + `Unrecognized filter operator "${item}" in a comparison triple. ` + `A filter array is either a logical node (["and"|"or", …]) or nested ` + `conditions ([[field, op, value], …]); a bare [field, op, value] only ` + `reaches the driver when its operator is outside @objectstack/spec ` + @@ -5171,8 +5171,8 @@ export class SqlDriver implements IDataDriver { // branches and was dropped, so a malformed element silently narrowed // nothing. Same reasoning as above: an unapplied filter must not look // like a satisfied one. #3948. - throw new Error( - `[sql-driver] Unrecognized filter element of type "${item === null ? 'null' : typeof item}" — ` + + throw unsupportedFilterError( + `Unrecognized filter element of type "${item === null ? 'null' : typeof item}" — ` + `expected a logical keyword ("and"/"or") or a condition array. ` + `Filter was: ${JSON.stringify(filters)}`, ); @@ -5485,7 +5485,7 @@ export class SqlDriver implements IDataDriver { case '$between': { const arr = Array.isArray(coerced) ? coerced : []; if (arr.length !== 2) { - throw new Error(`[sql-driver] operator "$between" on field "${field}" requires a [min, max] value array.`); + throw unsupportedFilterError(`Operator "$between" on field "${field}" requires a [min, max] value array.`); } (builder as any)[logicalOp === 'or' ? 'orWhereBetween' : 'whereBetween'](field, arr as [any, any]); break; From 959b838eb40e57bae1276e72c1f73538d1845ee0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 11:07:46 +0000 Subject: [PATCH 4/8] fix(metadata-protocol): PATCH/DELETE of a nonexistent record answer RECORD_NOT_FOUND, not 200 (#4435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The READ path was already honest — `getData` on an unknown id answers `404 RECORD_NOT_FOUND`. Both single-record WRITE paths reported success for a record that does not exist: PATCH /data/showcase_task/definitely_not_a_row → 200 {"record":null} DELETE /data/showcase_task/definitely_not_a_row → 200 {"success":true} REST is a pass-through here (`res.json(await p.deleteData(...))`), so these are the protocol's answers and this is where they are fixed. What it cost: a client that PATCHed a concurrently deleted record was told the write landed, and had to null-check a SUCCESS payload to find out otherwise; `DELETE` said `success: true` for any string in the path, so a typo'd id, an already-deleted row and a real deletion were indistinguishable — including in bulk, where `deleteMany {"ids":["nonexistent_1"]}` answered `succeeded: 1`. It is the same silent-no-op shape the v17 train removed everywhere else this window (#4240/#4303/#4315, #4169, #4190), one level up. - `updateData` asks existence BEFORE the write, via the same `findOne` + caller context `getData` uses. Deliberately not a post-check on the returned row: the engine returns the post-write READBACK, which is also `null` when the row still exists but the write moved it out of the caller's row scope (reassigning `owner_id` away from yourself under an owner-scoped policy) — reading that as "not found" would 404 a write that succeeded. - `deleteData` and `deleteManyData` read the driver's own answer. The contract (`IDataDriver.delete` — "True if deleted, false if not found") already carried it; the code discarded it and pushed a literal `success: true`. Read as `=== false` on purpose: that is the contract's positive not-found value, while a driver returning the deleted row or an off-contract `undefined` gives no such signal, and inventing a 404 from a falsy return would break deletes against third-party drivers instead of reporting honestly. `success` on the 200 now means what it says. - The 404 envelope is extracted as `recordNotFoundError` so the read and the two write paths cannot drift apart again. Note on the issue's second half: the spec's `DeleteDataResponseSchema` declares `success`, not `deleted`, so the existing key is correct as-is and nothing renames. Tests: new `protocol.record-not-found.test.ts` (12) covers PATCH/DELETE/ deleteMany, the read/write agreement on the same id, delete-twice, mixed batches, the `=== false` reading, and that the existence probe is asked with the caller's context. Three `protocol.dropped-fields.test.ts` fixtures stubbed `findOne → null` while PATCHing — under the new contract that IS a 404, so they now describe an engine that has the row (they are about the strip channel, not about missing records). Suites green: metadata-protocol 169, rest + objectql unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../src/protocol.dropped-fields.test.ts | 10 +- .../src/protocol.record-not-found.test.ts | 183 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 102 ++++++++-- 3 files changed, 274 insertions(+), 21 deletions(-) create mode 100644 packages/metadata-protocol/src/protocol.record-not-found.test.ts diff --git a/packages/metadata-protocol/src/protocol.dropped-fields.test.ts b/packages/metadata-protocol/src/protocol.dropped-fields.test.ts index 07f52f36e3..c6be76d1d2 100644 --- a/packages/metadata-protocol/src/protocol.dropped-fields.test.ts +++ b/packages/metadata-protocol/src/protocol.dropped-fields.test.ts @@ -32,7 +32,11 @@ describe('updateData — forwards engine write strips as droppedFields (#3431)', options?.onFieldsDropped?.({ object, fields: ['approval_status'], reason: 'readonly' }); return { id: 'rec-1', title: data.title }; }), - findOne: vi.fn(async () => null), + // The row EXISTS — `updateData`'s #4435 existence probe reads this, and a + // PATCH of an id that names no row is now a 404 rather than a 200 with a + // null record. These fixtures are about the strip channel, not about + // missing records, so they describe an engine that has the row. + findOne: vi.fn(async () => ({ id: 'rec-1' })), }; const p = new ObjectStackProtocolImplementation(engine as any); const res: any = await p.updateData({ @@ -56,7 +60,7 @@ describe('updateData — forwards engine write strips as droppedFields (#3431)', options?.onFieldsDropped?.({ object, fields: ['approval_status'], reason: 'readonly' }); return { id: 'rec-1' }; }), - findOne: vi.fn(async () => null), + findOne: vi.fn(async () => ({ id: 'rec-1' })), }; const p = new ObjectStackProtocolImplementation(engine as any); const res: any = await p.updateData({ object: 'approval_case', id: 'rec-1', data: {} }); @@ -70,7 +74,7 @@ describe('updateData — forwards engine write strips as droppedFields (#3431)', const engine = { registry: { getObject: () => SCHEMA }, update: vi.fn(async (_o: string, data: any) => ({ id: 'rec-1', ...data })), - findOne: vi.fn(async () => null), + findOne: vi.fn(async () => ({ id: 'rec-1' })), }; const p = new ObjectStackProtocolImplementation(engine as any); const res: any = await p.updateData({ object: 'approval_case', id: 'rec-1', data: { title: 'B' } }); diff --git a/packages/metadata-protocol/src/protocol.record-not-found.test.ts b/packages/metadata-protocol/src/protocol.record-not-found.test.ts new file mode 100644 index 0000000000..b5f5d045a8 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -0,0 +1,183 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4435] A write that touched zero rows must not report success. + * + * The READ path has always been honest — `getData` on an unknown id answers + * `404 RECORD_NOT_FOUND`. Both single-record WRITE paths disagreed: + * + * PATCH /data/showcase_task/definitely_not_a_row → 200 { record: null } + * DELETE /data/showcase_task/definitely_not_a_row → 200 { success: true } + * + * The REST layer is a pass-through (`res.json(await p.deleteData(...))`), so + * these are the protocol's answers, and this is where they are fixed. + * + * Why it matters beyond symmetry: a client PATCHing a record another session + * just deleted was told the write landed, and had to null-check a SUCCESS + * payload to find out otherwise. `DELETE` said `success: true` for any string in + * the path, so a typo'd id, an already-deleted row and a real deletion were + * indistinguishable — including on the bulk path, where a batch of typo'd ids + * reported every one of them deleted. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { name: 'task', fields: { title: { name: 'title', type: 'text' } } }; + +/** + * An engine whose `delete` honours the driver contract + * (`IDataDriver.delete` — "True if deleted, false if not found") and whose + * `findOne` answers from the same row set, so existence means one thing here. + */ +function makeProtocol(rows: Record = {}) { + const store = new Map(Object.entries(rows)); + const findOne = vi.fn(async (_object: string, opts: any) => store.get(String(opts?.where?.id)) ?? null); + const update = vi.fn(async (_object: string, data: any, opts: any) => { + const id = String(opts?.where?.id); + if (!store.has(id)) return null; + const next = { ...store.get(id), ...data }; + store.set(id, next); + return next; + }); + const del = vi.fn(async (_object: string, opts: any) => store.delete(String(opts?.where?.id))); + const engine = { + registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, + findOne, update, delete: del, + }; + return { p: new ObjectStackProtocolImplementation(engine as any), findOne, update, del, store }; +} + +/** Assert the thrown value is the 404 envelope `getData` already produced. */ +async function expectRecordNotFound(run: () => Promise, id: string) { + let caught: any; + try { + await run(); + } catch (e) { + caught = e; + } + expect(caught, 'expected a RECORD_NOT_FOUND rejection, but the call resolved').toBeDefined(); + expect(caught.code).toBe('RECORD_NOT_FOUND'); + expect(caught.status).toBe(404); + expect(caught.object).toBe('task'); + expect(caught.message).toContain(id); + return caught; +} + +describe('[#4435] updateData refuses an id that names no row', () => { + it('PATCH of a nonexistent id is 404 RECORD_NOT_FOUND, not 200 { record: null }', async () => { + const { p, update } = makeProtocol({ real: { id: 'real', title: 'x' } }); + await expectRecordNotFound( + () => p.updateData({ object: 'task', id: 'definitely_not_a_row', data: { title: 'y' } } as any), + 'definitely_not_a_row', + ); + // …and the engine was never asked to write. A refused PATCH must not fire + // hooks, automation or an audit row for a record that does not exist. + expect(update).not.toHaveBeenCalled(); + }); + + it('the SAME id answers 404 on the read path — the two verbs agree now', async () => { + const { p } = makeProtocol({ real: { id: 'real' } }); + await expectRecordNotFound( + () => p.getData({ object: 'task', id: 'definitely_not_a_row' } as any), + 'definitely_not_a_row', + ); + }); + + it('an existing record still updates and returns the row', async () => { + const { p, update } = makeProtocol({ real: { id: 'real', title: 'x' } }); + const res: any = await p.updateData({ object: 'task', id: 'real', data: { title: 'y' } } as any); + expect(update).toHaveBeenCalledTimes(1); + expect(res).toMatchObject({ object: 'task', id: 'real', record: { title: 'y' } }); + }); + + it('the existence probe is asked with the CALLER context, like getData', async () => { + // A row outside the caller's row scope is not-found FOR THEM on the read + // path; a write path that answered 200 for it would contradict the very + // next GET. + const { p, findOne } = makeProtocol({ real: { id: 'real' } }); + const ctx = { userId: 'u1' }; + await p.updateData({ object: 'task', id: 'real', data: {}, context: ctx } as any); + expect(findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'real' }, context: ctx }); + }); +}); + +describe('[#4435] deleteData reports what actually happened', () => { + it('DELETE of a nonexistent id is 404, not 200 { success: true }', async () => { + const { p } = makeProtocol({ real: { id: 'real' } }); + await expectRecordNotFound( + () => p.deleteData({ object: 'task', id: 'definitely_not_a_row' } as any), + 'definitely_not_a_row', + ); + }); + + it('a real deletion still answers success, and is no longer indistinguishable', async () => { + const { p, store } = makeProtocol({ real: { id: 'real' } }); + const res: any = await p.deleteData({ object: 'task', id: 'real' } as any); + expect(res).toEqual({ object: 'task', id: 'real', success: true }); + expect(store.has('real')).toBe(false); + }); + + it('deleting the same id twice: first 200, second 404', async () => { + const { p } = makeProtocol({ real: { id: 'real' } }); + await p.deleteData({ object: 'task', id: 'real' } as any); + await expectRecordNotFound(() => p.deleteData({ object: 'task', id: 'real' } as any), 'real'); + }); + + it('a driver return that is not the contract\'s `false` is NOT read as not-found', async () => { + // `=== false` on purpose. A driver that returns the deleted row, or an + // off-contract `undefined`, gives no POSITIVE not-found signal — inventing + // a 404 from a falsy return would break deletes against third-party + // drivers instead of reporting honestly. + const engine = { + registry: { getObject: () => SCHEMA }, + delete: vi.fn(async () => undefined), + }; + const p = new ObjectStackProtocolImplementation(engine as any); + await expect(p.deleteData({ object: 'task', id: 'whatever' } as any)) + .resolves.toMatchObject({ success: true }); + }); +}); + +describe('[#4435] deleteManyData reports per id, not per request', () => { + it('a batch of typo\'d ids no longer reports every one of them deleted', async () => { + const { p } = makeProtocol({ real: { id: 'real' } }); + const res: any = await p.deleteManyData({ + object: 'task', + ids: ['nonexistent_1'], + options: { continueOnError: true }, + } as any); + + // Pre-#4435: { succeeded: 1, failed: 0, results: [{ success: true }] }. + expect(res).toMatchObject({ success: false, total: 1, succeeded: 0, failed: 1 }); + expect(res.results[0]).toMatchObject({ id: 'nonexistent_1', success: false }); + expect(res.results[0].error).toContain('nonexistent_1'); + }); + + it('mixed ids are reported individually', async () => { + const { p } = makeProtocol({ a: { id: 'a' }, c: { id: 'c' } }); + const res: any = await p.deleteManyData({ + object: 'task', + ids: ['a', 'b', 'c'], + options: { continueOnError: true }, + } as any); + + expect(res).toMatchObject({ success: false, total: 3, succeeded: 2, failed: 1 }); + expect(res.results.map((r: any) => [r.id, r.success])).toEqual([ + ['a', true], ['b', false], ['c', true], + ]); + }); + + it('an all-real batch is unchanged', async () => { + const { p } = makeProtocol({ a: { id: 'a' }, b: { id: 'b' } }); + const res: any = await p.deleteManyData({ object: 'task', ids: ['a', 'b'] } as any); + expect(res).toMatchObject({ success: true, succeeded: 2, failed: 0 }); + }); + + it('a missing id stops the run without continueOnError, as a failure always has', async () => { + const { p, del } = makeProtocol({ b: { id: 'b' } }); + const res: any = await p.deleteManyData({ object: 'task', ids: ['a', 'b'] } as any); + expect(res).toMatchObject({ success: false, succeeded: 0, failed: 1 }); + expect(del).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 8ec5dede16..2dbd5aaeda 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -314,6 +314,34 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null return getMetadataTypeSchema(singular) ?? null; } +/** + * [#4435] The 404 a single-record operation answers when the id names no row. + * + * Extracted so the READ and the two WRITE paths cannot disagree about it. They + * did: `getData` answered `404 RECORD_NOT_FOUND` while `updateData` returned + * `200 { record: null }` and `deleteData` returned `200 { success: true }` for + * any string in the path — so a typo'd id, an already-deleted row and a real + * deletion were indistinguishable, and a client PATCHing a concurrently deleted + * record was told its write had landed. + * + * That is the same silent-no-op shape the v17 train removed everywhere else + * this window (#4240/#4303/#4315 refuse missing fields, #4169 refuses unknown + * params, #4190 stopped dropping filters) — a write that touched zero rows + * reporting 200 is that shape one level up, on the verb where it costs the + * most. + */ +function recordNotFoundError(object: string, id: string | number): Error { + const err = new Error(`Record ${id} not found in ${object}`) as Error & { + code?: string; + status?: number; + object?: string; + }; + err.code = 'RECORD_NOT_FOUND'; + err.status = 404; + err.object = object; + return err; +} + /** * A 400 for a `$filter` ARRAY that looks like a filter AST but is not one. * @@ -4602,15 +4630,7 @@ export class ObjectStackProtocolImplementation implements record: result }; } - const err = new Error(`Record ${request.id} not found in ${request.object}`) as Error & { - code?: string; - status?: number; - object?: string; - }; - err.code = 'RECORD_NOT_FOUND'; - err.status = 404; - err.object = request.object; - throw err; + throw recordNotFoundError(request.object, request.id); } async createData(request: { object: string, data: any, context?: any }) { @@ -4679,13 +4699,7 @@ export class ObjectStackProtocolImplementation implements request.object, { where: { id: request.id }, ...(ctxOpt as any) } as any, ); - if (!source) { - const err: any = new Error(`Record ${request.id} not found in ${request.object}`); - err.code = 'RECORD_NOT_FOUND'; - err.status = 404; - err.object = request.object; - throw err; - } + if (!source) throw recordNotFoundError(request.object, request.id); // Copy the source, then strip the columns the engine owns so the insert // path re-derives them rather than carrying the source's values over. @@ -4726,6 +4740,20 @@ export class ObjectStackProtocolImplementation implements async updateData(request: { object: string, id: string, data: any, expectedVersion?: string, context?: any }) { this.assertObjectRegistered(request.object); // [#3770] await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context); + // [#4435] A PATCH of an id that names no row answered `200 { record: + // null }` — the caller had to null-check a SUCCESS payload to discover + // its write never landed, which is exactly what a client that PATCHes a + // concurrently deleted record does not do. `getData` has always answered + // 404 for the same id; the two verbs now agree. + // + // Checked BEFORE the write rather than by inspecting what comes back: + // the engine's update returns the post-write READBACK, which is also + // `null` when the row still exists but the write moved it out of the + // caller's row scope (reassigning `owner_id` away from yourself under an + // owner-scoped RLS policy). Reading that as "not found" would answer 404 + // to a write that succeeded. Existence is the question, so ask it + // directly — the same `findOne` + context `getData` asks. + await this.assertRecordExists(request.object, request.id, request.context); const opts: any = { where: { id: request.id } }; if (request.context !== undefined) opts.context = request.context; // [#3407/#3431] Capture the engine's LEGAL write strips (static `readonly` @@ -4750,7 +4778,21 @@ export class ObjectStackProtocolImplementation implements await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context); const opts: any = { where: { id: request.id } }; if (request.context !== undefined) opts.context = request.context; - await this.engine.delete(request.object, opts); + const deleted = await this.engine.delete(request.object, opts); + // [#4435] `success: true` used to be a LITERAL — the response said the + // same thing for a real deletion, an already-deleted row and a typo'd + // id, so nothing on the wire could tell them apart. The driver contract + // (`IDataDriver.delete` — "True if deleted, false if not found") already + // carries the answer; it was simply discarded here. Now it decides: + // `false` is a 404, matching `getData` on the same id, and `success` on + // the 200 finally means what it says. + // + // Read as `=== false` on purpose. That is the contract's own value for + // "no row matched"; anything else — a driver returning the deleted row, + // an `undefined` from an off-contract implementation — is not a + // POSITIVE not-found signal, and inventing a 404 out of it would break + // deletes against third-party drivers rather than report honestly. + if (deleted === false) throw recordNotFoundError(request.object, request.id); return { object: request.object, id: request.id, @@ -4758,6 +4800,21 @@ export class ObjectStackProtocolImplementation implements }; } + /** + * [#4435] Throw `404 RECORD_NOT_FOUND` when `id` names no row VISIBLE to + * this caller — the same question, asked the same way, that `getData` + * answers with. Threading `context` is what makes "visible to this caller" + * true: a row the caller's RLS scope excludes is not-found for them on the + * read path, and a write path that disagreed would answer 200 for a record + * the very next GET reports as 404. + */ + private async assertRecordExists(object: string, id: string, context: any): Promise { + const findOpts: any = { where: { id } }; + if (context !== undefined) findOpts.context = context; + const existing = await this.engine.findOne(object, findOpts); + if (!existing) throw recordNotFoundError(object, id); + } + /** * Optimistic Concurrency Control gate shared by updateData/deleteData. * @@ -5359,7 +5416,16 @@ export class ObjectStackProtocolImplementation implements for (const id of ids) { try { - await this.engine.delete(object, { where: { id }, ...ctxOpt } as any); + // [#4435] Per-row honesty on the bulk path. This discarded the + // driver's return and pushed `success: true` unconditionally, so + // `{"ids":["nonexistent_1"]}` answered `succeeded: 1` — a batch + // of typo'd ids reported every one of them deleted. A caller + // reconciling "which of my 200 ids were real" got a list that + // agreed with whatever it sent. Same `=== false` reading as the + // single-record path: the contract's positive not-found value, + // never an inference from a falsy return. + const deleted = await this.engine.delete(object, { where: { id }, ...ctxOpt } as any); + if (deleted === false) throw recordNotFoundError(object, id); results.push({ id: String(id), success: true }); succeeded++; } catch (err: any) { From 5d30293427eb6049195f2607aebbc9e46c1694b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 11:08:07 +0000 Subject: [PATCH 5/8] fix(runtime): a sandbox capability denial is a 500 crash, not a 400 rejection (#4431) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `action-crash-vs-rejection` contract (#3951) pins the table: a `SandboxError` WITH `innerMessage` is a body's deliberate throw → 400; a `SandboxError` with NO `innerMessage` — timeout, capability denial — is a crash → 500. Capability denials were answering 400: POST /api/v1/actions/showcase_task/rc1_crash_probe → 400 {"error":{"code":"VALIDATION_ERROR", "message":"SandboxError: capability 'api.read' not granted to action …"}} Why: the gate throws `SandboxError` synchronously INSIDE a QuickJS host function, which rejects the async IIFE inside the VM, so it returns through the `__error` side-channel — and the pump loop presumed everything arriving there was user code throwing on purpose, setting `innerMessage` unconditionally. The dispatcher's classifier then read that as a deliberate rejection. So every capability denial stayed invisible to gateway error rates, APM and alerting — exactly the blindness #3951 was written to close — and the client also received the `SandboxError: ` debug prefix that belongs only in server logs. `SandboxError`'s own jsdoc already said `innerMessage` is undefined for the sandbox's internal errors; that only held for denials detected OUTSIDE evaluation (a timeout, which takes the separate `budgetError` path). In-VM host-call denials — `ctx.api.*`, `ctx.log`, `ctx.crypto`, `ctx.api.transaction` — were misclassified. Fix: the sandbox's own faults now carry a marker THROUGH the VM. `hostErrorToVm` stamps `__objectstackSandboxFault` on any `SandboxError` it marshals, and the synchronous gates throw the VM handle it builds rather than a raw host error — quickjs-emscripten passes a thrown handle through verbatim while its `newError` path copies only `name`/`message`, which is precisely how the identity was lost. The reject handler reports the marker on the additive `__errorInfo` channel, and the pump loop, seeing it, rethrows with neither the ` '' threw:` wrapper (nothing threw — the sandbox refused) nor an `innerMessage`. The existing classifier then does the rest: name is `SandboxError`, no inner/code/fields ⇒ unexpected fault ⇒ `errorFromThrown(err, 500)`, and the message reaching the client is the capability text with the debug prefix stripped. A marker rather than a match on the flattened `SandboxError: …` text, because the flattening is user-reachable: a body that CATCHES the denial and throws its own business error must keep its 400, and that case is pinned. No ADR or contract was changed — this makes the runtime deliver the contract #3951 already specifies. Tests: new `sandbox/capability-denial-is-a-fault.test.ts` (7) covers all four in-VM gates, the absence of innerMessage/code/fields, the prefix, the caught-and-rethrown rejection, an ordinary deliberate throw, and that a record `ValidationError` crossing `ctx.api` keeps its `code`/`fields` (the marker must not turn every failed write into a 500). Verified failing on all four denial cases before the fix. Runtime suite green: 73 files / 1033 tests. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../capability-denial-is-a-fault.test.ts | 205 ++++++++++++++++++ .../runtime/src/sandbox/quickjs-runner.ts | 127 ++++++++++- 2 files changed, 322 insertions(+), 10 deletions(-) create mode 100644 packages/runtime/src/sandbox/capability-denial-is-a-fault.test.ts diff --git a/packages/runtime/src/sandbox/capability-denial-is-a-fault.test.ts b/packages/runtime/src/sandbox/capability-denial-is-a-fault.test.ts new file mode 100644 index 0000000000..f581787878 --- /dev/null +++ b/packages/runtime/src/sandbox/capability-denial-is-a-fault.test.ts @@ -0,0 +1,205 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4431] A capability denial is the sandbox FAULTING, not the body rejecting. + * + * The `action-crash-vs-rejection` contract (#3951) pins the table: + * + * | `SandboxError` WITH `innerMessage` — a body's deliberate throw | 400 | + * | `SandboxError` with NO `innerMessage` — timeout, capability denial | 500 | + * + * A capability gate throws `SandboxError` synchronously inside a QuickJS host + * function, which rejects the async IIFE *inside* the VM — so it came back + * through the `__error` side-channel, and the pump loop presumed everything + * arriving there was user code throwing on purpose. It therefore set + * `innerMessage`, the dispatcher's classifier read that as a deliberate + * rejection and answered **400**, and the client got the `SandboxError: ` debug + * prefix that only ever belonged in server logs. + * + * The contract's own claim — "capability denial has no user-meaningful inner + * message" — held only for denials detected OUTSIDE evaluation (a timeout, + * which takes the separate `budgetError` path). These tests pin the in-VM + * host-call denials that were misclassified: `ctx.api.*`, `ctx.log`, + * `ctx.crypto`, `ctx.api.transaction`. + * + * The HTTP half of the contract is asserted by + * `domains/actions-fault-vs-rejection.test.ts`, which already states that a + * `SandboxError` with no `innerMessage` is a 500 — this file is what makes a + * real capability denial actually arrive in that shape. + */ + +import { describe, it, expect } from 'vitest'; +import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js'; +import type { ScriptContext, ScriptRunOptions } from './script-runner.js'; + +const runner = new QuickJSScriptRunner({ hookTimeoutMs: 10_000, actionTimeoutMs: 10_000 }); +const actionOpts: ScriptRunOptions = { origin: { kind: 'action', name: 'rc1_crash_probe' } }; + +function ctx(over: Partial = {}): ScriptContext { + return { input: {}, ...over }; +} + +async function faultOf(run: () => Promise): Promise { + const e = await run().then(() => null, (err) => err as SandboxError); + expect(e, 'expected the sandbox to refuse, but the script resolved').toBeInstanceOf(SandboxError); + return e!; +} + +describe('[#4431] an in-VM capability denial reaches the classifier as a FAULT', () => { + const api = { object: (_n: string) => ({ count: (_f: unknown) => 1 }) }; + + it("ctx.api.object(x).count without api.read — the issue's exact repro", async () => { + const err = await faultOf(() => + runner.runScript( + { + language: 'js', + source: "return ctx.api.object('showcase_task').count({});", + capabilities: [], // the denial under test + }, + ctx({ api }), + actionOpts, + ), + ); + + // THE fix: no `innerMessage`. `domains/actions.ts` reads its absence (plus + // the non-`Error` name) as an unexpected fault → `errorFromThrown(err, 500)`. + // With it set, the denial was served as a deliberate 400 and stayed + // invisible to gateway error rates, APM and alerting. + expect(err.innerMessage).toBeUndefined(); + + // The debug prefix must not reach the client. The runner's own doc says + // only the business message should — and a sandbox fault has none, so what + // the client sees is this text, minus the prefix. + expect(err.message).not.toContain('SandboxError:'); + // …while the actionable content survives: which capability, whose, and the + // call that tripped the gate. + expect(err.message).toContain("capability 'api.read' not granted"); + expect(err.message).toContain("action 'rc1_crash_probe'"); + expect(err.message).toContain("ctx.api.object('showcase_task').count"); + + // Nor does it get the ` '' threw:` wrapper — nothing threw; the + // sandbox refused before user code could run the call. + expect(err.message).not.toContain('threw:'); + + // No `code`/`fields` either: both are "structured domain failure" markers + // the classifier reads as a REJECTION, so carrying one would re-break the + // 500 the same way `innerMessage` did. + expect(err.code).toBeUndefined(); + expect(err.fields).toBeUndefined(); + + // The classifier's own predicate, spelled out. + expect(err.name).toBe('SandboxError'); + }); + + it('ctx.log without the log capability', async () => { + const log = { info: () => {}, warn: () => {}, error: () => {} }; + const err = await faultOf(() => + runner.runScript( + { language: 'js', source: "ctx.log.info('hi'); return 1;", capabilities: [] }, + ctx({ log }), + actionOpts, + ), + ); + expect(err.innerMessage).toBeUndefined(); + expect(err.message).not.toContain('SandboxError:'); + expect(err.message).toContain("capability 'log' not granted"); + }); + + it('ctx.crypto.randomUUID without the crypto.uuid capability', async () => { + const err = await faultOf(() => + runner.runScript( + { language: 'js', source: 'return ctx.crypto.randomUUID();', capabilities: [] }, + ctx(), + actionOpts, + ), + ); + expect(err.innerMessage).toBeUndefined(); + expect(err.message).not.toContain('SandboxError:'); + expect(err.message).toContain("capability 'crypto.uuid' not granted"); + }); + + it('ctx.api.transaction without the api.transaction capability', async () => { + const err = await faultOf(() => + runner.runScript( + { + language: 'js', + source: 'return await ctx.api.transaction(async () => 1);', + capabilities: ['api.read'], + }, + ctx({ api }), + actionOpts, + ), + ); + expect(err.innerMessage).toBeUndefined(); + expect(err.message).not.toContain('SandboxError:'); + expect(err.message).toContain("capability 'api.transaction' not granted"); + }); + + it('a denial the body CATCHES and rethrows as its own error stays a rejection', async () => { + // The other direction, and the reason the marker is a property on the VM + // error rather than a match on the flattened `SandboxError: …` text: once + // user code has caught the denial and thrown its own business error, the + // outcome IS a deliberate rejection and must keep its 400. + const err = await faultOf(() => + runner.runScript( + { + language: 'js', + source: `try { await ctx.api.object('t').count({}); } + catch (e) { throw new Error('权限不足，请联系管理员'); } + return 1;`, + capabilities: [], + }, + ctx({ api }), + actionOpts, + ), + ); + expect(err.innerMessage).toBe('权限不足，请联系管理员'); + expect(err.message).toContain("action 'rc1_crash_probe' threw:"); + }); +}); + +describe('[#4431] the rejection side of the contract is untouched', () => { + it('a deliberate throw still carries innerMessage and the debug wrapper', async () => { + const err = await faultOf(() => + runner.runScript( + { language: 'js', source: "throw new Error('线索信息不完整');", capabilities: [] }, + ctx(), + actionOpts, + ), + ); + expect(err.innerMessage).toBe('线索信息不完整'); + expect(err.message).toContain("action 'rc1_crash_probe' threw:"); + }); + + it('a structured error crossing out of ctx.api keeps its code and fields', async () => { + // The #3937/#4345 passthrough: a record ValidationError raised by a host + // call is a REJECTION, and its structure must survive. Pinned here because + // the fault marker is set on the same path (`hostErrorToVm`) — a marker + // applied too broadly would turn every failed write into a 500. + const api = { + object: (_n: string) => ({ + update: async () => { + const e: any = new Error('issued_on is required'); + e.name = 'ValidationError'; + e.code = 'VALIDATION_FAILED'; + e.fields = [{ field: 'issued_on', code: 'required', message: 'issued_on is required' }]; + throw e; + }, + }), + }; + const err = await faultOf(() => + runner.runScript( + { + language: 'js', + source: "return await ctx.api.object('inv').update('1', {});", + capabilities: ['api.write'], + }, + ctx({ api }), + actionOpts, + ), + ); + expect(err.code).toBe('VALIDATION_FAILED'); + expect(err.fields).toHaveLength(1); + expect(err.innerMessage).toContain('issued_on is required'); + }); +}); diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index ae4d228d89..9f5964ca28 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -281,8 +281,8 @@ export class QuickJSScriptRunner implements ScriptRunner { `function(e){ globalThis.__error = (e && e.message) ? (e.name + ': ' + e.message) : String(e); try { - globalThis.__errorInfo = (e && (e.code || e.fields)) - ? JSON.stringify({ code: e.code, fields: e.fields }) + globalThis.__errorInfo = (e && (e.code || e.fields || e['${SANDBOX_FAULT_PROP}'])) + ? JSON.stringify({ code: e.code, fields: e.fields, sandboxFault: e['${SANDBOX_FAULT_PROP}'] === true }) : undefined; } catch (_) { globalThis.__errorInfo = undefined; } }`; @@ -379,10 +379,25 @@ export class QuickJSScriptRunner implements ScriptRunner { // "InternalError: interrupted". const budget = budgetError(pumps); if (budget) throw budget; + const info = readErrorInfo(vm); + // [#4431] A SANDBOX fault that crossed `__error` — a capability + // denial thrown synchronously inside a host function, an unavailable + // `ctx.api`, a marshalling failure. It is not an outcome the body + // chose to report, so it gets neither the ` '' threw:` + // wrapper (nothing threw — the sandbox refused) nor an + // `innerMessage` (there is no business message; `SandboxError`'s own + // contract says so). Leaving `innerMessage` unset is what makes the + // #3951 contract hold: the dispatcher's classifier reads its absence + // as a CRASH and answers 500 through `errorFromThrown`, instead of + // the 400 a denial used to get — and the client sees the capability + // text without the `SandboxError: ` debug prefix. + if (info?.sandboxFault) { + throw new SandboxError(sandboxFaultMessage(String(errStr))); + } throw new SandboxError( `${args.origin.kind} '${args.origin.name}' threw: ${errStr}`, userFacingMessage(String(errStr)), - readErrorInfo(vm), + info, ); } @@ -523,7 +538,8 @@ export class QuickJSScriptRunner implements ScriptRunner { const installTxLeaf = (name: string, run: () => Promise): void => { const fn = vm.newFunction(name, () => { if (!caps.has('api.transaction')) { - throw new SandboxError( + throwSandboxFault( + vm, `capability 'api.transaction' not granted to ${origin.kind} '${origin.name}' (called ctx.api.transaction)`, ); } @@ -591,7 +607,7 @@ export class QuickJSScriptRunner implements ScriptRunner { for (const level of ['info', 'warn', 'error'] as const) { const fn = vm.newFunction(level, (msgH, dataH) => { if (!caps.has('log')) { - throw new SandboxError(`capability 'log' not granted to ${origin.kind} '${origin.name}'`); + throwSandboxFault(vm, `capability 'log' not granted to ${origin.kind} '${origin.name}'`); } const msg = vm.getString(msgH); const data = dataH ? safeJsonParse(vm.getString(dataH)) : undefined; @@ -607,7 +623,7 @@ export class QuickJSScriptRunner implements ScriptRunner { const cryptoObj = vm.newObject(); const uuidFn = vm.newFunction('randomUUID', () => { if (!caps.has('crypto.uuid')) { - throw new SandboxError(`capability 'crypto.uuid' not granted to ${origin.kind} '${origin.name}'`); + throwSandboxFault(vm, `capability 'crypto.uuid' not granted to ${origin.kind} '${origin.name}'`); } const v = ctx.crypto?.randomUUID?.() ?? cryptoRandomUUID(); return vm.newString(v); @@ -776,13 +792,14 @@ function installApiMethod( // Capability gate — throw synchronously so the VM sees a normal exception at // the call site (mirrors ctx.log / ctx.crypto gating). if (!caps.has(required)) { - throw new SandboxError( + throwSandboxFault( + vm, `capability '${required}' not granted to ${origin.kind} '${origin.name}' (called ctx.api.object('${objectName}').${method})`, ); } const apiAny = ctx.api as Record | undefined; if (!apiAny || typeof apiAny.object !== 'function') { - throw new SandboxError(`ctx.api unavailable in ${origin.kind} '${origin.name}'`); + throwSandboxFault(vm, `ctx.api unavailable in ${origin.kind} '${origin.name}'`); } // Dump args now, while the handles are alive — they are freed when this // function returns, long before the async work below runs. @@ -901,12 +918,93 @@ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle { vm.setProp(errH, 'fields', h); h.dispose(); } + // [#4431] Mark the sandbox's OWN faults so the pump loop can tell them + // apart from a user throw after the VM has flattened both to a string. + if (err instanceof SandboxError) { + const h = vm.true; + vm.setProp(errH, SANDBOX_FAULT_PROP, h); + } } catch { /* keep the bare name/message error */ } return errH; } +/** + * [#4431] The marker a sandbox-internal fault carries THROUGH the VM. + * + * ## The problem it solves + * + * A capability gate throws `SandboxError` **synchronously inside a QuickJS host + * function**. That rejects the async IIFE *inside* the VM, so it comes back + * through the `__error` side-channel — and the pump loop presumed that anything + * arriving there was user code throwing deliberately: + * + * ```ts + * throw new SandboxError( + * `${kind} '${name}' threw: ${errStr}`, + * userFacingMessage(String(errStr)), // ← innerMessage SET + * ); + * ``` + * + * `SandboxError`'s own contract says the opposite — `innerMessage` is + * "undefined for the sandbox's own internal errors (capability denials, + * timeouts, marshalling failures), which have no user-meaningful inner + * message" — and the #3951 crash contract pins the consequence: *"`SandboxError` + * with no `innerMessage` — timeout, **capability denial** → crash → 500"*. + * With `innerMessage` set, `domains/actions.ts`'s classifier read the denial as + * a deliberate rejection and answered **400**, leaving every capability denial + * invisible to gateway error rates, APM and alerting. The client also received + * the `SandboxError: ` debug prefix, which only ever belonged in server logs. + * + * The jsdoc's claim only held for denials detected OUTSIDE evaluation (a + * timeout, which takes the separate `budgetError` path). In-VM host-call + * denials — `ctx.api.*`, `ctx.log`, `ctx.crypto`, `ctx.api.transaction` — were + * misclassified. + * + * ## Why a marker and not the name + * + * `__error` is a FLATTENED `: ` string by design (every existing + * consumer reads it that way), and quickjs-emscripten's host-throw conversion + * copies only `name` and `message` onto the VM error — which is exactly why the + * denial's identity was lost. Matching on the `SandboxError:` text would be + * matching on a string user code can produce. The marker is a real property set + * on the VM error object and read back through the additive `__errorInfo` + * channel, so the classification survives the flattening it is meant to outlive. + */ +const SANDBOX_FAULT_PROP = '__objectstackSandboxFault'; + +/** + * [#4431] Throw a sandbox-internal fault OUT OF a host function so it reaches + * the VM carrying {@link SANDBOX_FAULT_PROP}. + * + * quickjs-emscripten's `errorToHandle` passes a thrown HANDLE through as the VM + * exception verbatim (`error instanceof Lifetime ? error : this.newError(error)`), + * and it is `newError` — the non-handle path — that drops everything but + * `name`/`message`. Building the handle here is therefore what preserves the + * marker. The handle is consumed by `QTS_Throw`, so it must NOT be disposed + * here. + * + * The host-side `SandboxError` is still constructed: it is what carries the + * message, and it keeps this helper's call sites reading like the plain + * `throw new SandboxError(...)` they replaced. + */ +function throwSandboxFault(vm: QuickJSContext, message: string): never { + throw hostErrorToVm(vm, new SandboxError(message)); +} + +/** + * Strip the `SandboxError: ` name prefix the VM's flattening prepends. + * + * The runner's own doc says only the business message should reach a client; + * for a sandbox fault there is no business message at all, so what reaches the + * client is this text — the capability, the origin and the call that tripped + * the gate — with the debug prefix removed. + */ +function sandboxFaultMessage(raw: string): string { + return raw.startsWith('SandboxError: ') ? raw.slice('SandboxError: '.length) : raw; +} + /** Marshal a host JSON-serializable value into a QuickJS handle. */ function jsonToHandle(vm: QuickJSContext, v: unknown): QuickJSHandle { const json = safeJsonStringify(v); @@ -1057,6 +1155,14 @@ export class SandboxError extends Error { export interface SandboxErrorInfo { code?: string; fields?: unknown[]; + /** + * [#4431] The error that crossed `__error` was the SANDBOX's own fault — a + * denied capability, an unavailable `ctx.api`, a marshalling failure — not + * user code rejecting. Read by the pump loop, which then leaves + * `innerMessage` undefined so the #3951 crash contract's "SandboxError with + * no innerMessage → 500" holds for in-VM host-call denials too. + */ + sandboxFault?: boolean; } /** @@ -1080,11 +1186,12 @@ function readErrorInfo(vm: QuickJSContext): SandboxErrorInfo | undefined { } catch { return undefined; } - const p = parsed as { code?: unknown; fields?: unknown }; + const p = parsed as { code?: unknown; fields?: unknown; sandboxFault?: unknown }; const info: SandboxErrorInfo = {}; if (typeof p?.code === 'string' && p.code) info.code = p.code; if (Array.isArray(p?.fields)) info.fields = p.fields; - return info.code || info.fields ? info : undefined; + if (p?.sandboxFault === true) info.sandboxFault = true; + return info.code || info.fields || info.sandboxFault ? info : undefined; } /** From 518dd90c66fba2a9af7c0c0a0868792f41a8393f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 12:09:26 +0000 Subject: [PATCH 6/8] chore: add changeset for the v17 REST envelope defects (#4431, #4435, #4436, #4483) --- .changeset/v17-rest-envelope-defects.md | 40 +++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 .changeset/v17-rest-envelope-defects.md diff --git a/.changeset/v17-rest-envelope-defects.md b/.changeset/v17-rest-envelope-defects.md new file mode 100644 index 0000000000..dcce141275 --- /dev/null +++ b/.changeset/v17-rest-envelope-defects.md @@ -0,0 +1,40 @@ +--- +"@objectstack/spec": minor +"@objectstack/runtime": minor +"@objectstack/metadata-protocol": minor +"@objectstack/driver-sql": minor +"@objectstack/driver-memory": minor +--- + +fix(data,runtime,drivers): four ADR-0112 envelope defects found in the v17 verification sweep (#4431, #4435, #4436, #4483) + +Four independent surfaces where the answer a caller received contradicted the +contract the surface declares. All four were found driving a real showcase boot +against `17.0.0-rc.1` and are catalogued in the #4482 rollup. + +- **#4431 — a sandbox capability denial answered 400.** A denial is the sandbox + refusing to run untrusted code that asked for a capability it does not hold, + which is the crash contract's case (#3951), not a deliberate rejection of a + malformed request. It now answers 500, and the `SandboxError:` debug prefix + no longer reaches the client. + +- **#4435 — PATCH/DELETE of a nonexistent record answered 200 success.** The + write path returned `record: null` / `success: true` for an id that resolves + to nothing, while GET on the same id correctly 404s; `deleteMany` reported + every typo'd id as deleted. Both now answer `RECORD_NOT_FOUND`, so a caller + can no longer read a successful envelope as proof the write landed. + +- **#4436 — the unsupported-filter-operator refusal shipped without + `error.code`.** A refusal with no code is unmatchable by a client, and the + message leaked the internal `[sql-driver]` prefix. It now speaks + `INVALID_FILTER` without the driver prefix. + +- **#4483 — the `$search` auto field set admitted its lead field + unconditionally.** `nameField`/`name`/`title` were prepended without passing + `SEARCH_AUTO_EXCLUDED_FIELDS`, so a search could be aimed at the primary key. + The lead field now only ORDERS the set it is already a member of; it can no + longer admit one. + +These change responses that were observably wrong, so callers coded against the +buggy shapes — a 200 on a missing record, a 400 on a capability denial — will +see different status codes. Graded `minor` on that basis rather than `patch`. From 4365ab21799e5f6b31e8ece1e108207bc4cdffd4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 12:18:13 +0000 Subject: [PATCH 7/8] fix(test): call syncSchema with its real (object, schema) signature (#4436) The #4436 refusal-envelope test passed a single merged object where the driver takes the object name as its own first argument, so the suite could not type-check. Matches the idiom in the sibling memory-driver tests. --- .../driver-memory/src/memory-filter-refusal-envelope.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts b/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts index 1114c4cf43..99052de4e1 100644 --- a/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts +++ b/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts @@ -37,14 +37,13 @@ describe('[#4436] InMemoryDriver filter refusals carry INVALID_FILTER and leak n beforeEach(async () => { driver = new InMemoryDriver(); - await driver.syncSchema?.({ - name: 'deal', + await driver.syncSchema('deal', { fields: { id: { type: 'text', name: 'id' }, stage: { type: 'text', name: 'stage' }, amount: { type: 'number', name: 'amount' }, }, - } as any); + }); await driver.create('deal', { id: '1', stage: 'won', amount: 10 }); }); From adde3ba2d12c3a41252649812806578fc60b79c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:07:12 +0000 Subject: [PATCH 8/8] fix(metadata-protocol): one probe per PATCH, and the existence gate is not an RLS gate (#4435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 959b838, fixing two defects the first cut introduced. Both were caught by CI (`Test Core` on @objectstack/objectql, `Dogfood Regression Gate 1/2`), and the second is the more serious of the two. ## 1. The existence probe duplicated OCC's read `updateData` called `assertVersionMatch` (which reads the row for its `updated_at`) and then `assertRecordExists` (which reads the same row again). Two round-trips per PATCH — a performance regression no gate reports — and the `protocol-data.test.ts` OCC cases said so directly ("expected to be called once, but got 2 times"). The two gates want the same row, so they now share one read: `probeRecord` fetches it, `assertVersionOf` became a PURE comparison over an already-read row, and `assertVersionMatch` survives only for `deleteData`, which needs no existence probe at all — the driver's own return reports whether a row matched, so a plain DELETE stays at zero extra reads and only an OCC token buys one. ## 2. The probe must ask EXISTENCE, not the caller's visibility The first cut probed with the CALLER's context, reasoning that it should match `getData`. That quietly turned the existence gate into an authorization gate: a row the caller cannot read comes back `null`, so the PATCH answers 404. Two things break. It moves an RLS decision out of the write policy. Whether an unreadable row may be written by id is the #1994 pre-image check's call, made inside `engine.update`. A probe in front of it adds a second, different rule — scope creep into the security model, out of a bug fix about missing records. And it disarms a revert-provable security proof. `@proof: rls-by-id-write` (`qa/dogfood/test/rls-fixture.dogfood.test.ts`, referenced by the `permission.rowLevelSecurity.using` liveness ledger entry) boots a fixture whose member can read nothing and has no write policy, and asserts the runner reports `rls-hole` — the RED half that proves the gate can go red at all. A caller-scoped probe 404s that PATCH and the proof goes green: if #1994 were ever reverted, this probe would MASK it. Accidentally hardening one path is not worth permanently blinding the gate that watches the whole class. So the probe runs as system and answers existence only. Authorization stays exactly where it was, and the sole behaviour added is the 404 the issue asked for: an id that names no row at all. Tests: `protocol-data.test.ts`'s OCC block now asserts the new contract — one probe on every PATCH (the existence probe, no OCC comparison without a token), still exactly one when OCC IS requested (the anti-duplication pin), 404 before any OCC verdict for a missing id, and DELETE without a token issuing no probe. Its fixtures now supply a row, because under this contract a PATCH of an absent record is correctly a 404 and those cases are about OCC. Two cases added to `protocol.record-not-found.test.ts` pin the system-context probe and that an unreadable-but-existing row still reaches the engine for RLS to decide. Green: objectql protocol-data 117, metadata-protocol 170, dogfood shard 1/2 38 files / 235 passed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../src/protocol.record-not-found.test.ts | 33 ++++- packages/metadata-protocol/src/protocol.ts | 132 ++++++++++++------ packages/objectql/src/protocol-data.test.ts | 54 ++++++- 3 files changed, 167 insertions(+), 52 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.record-not-found.test.ts b/packages/metadata-protocol/src/protocol.record-not-found.test.ts index b5f5d045a8..363527d7ab 100644 --- a/packages/metadata-protocol/src/protocol.record-not-found.test.ts +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -91,14 +91,33 @@ describe('[#4435] updateData refuses an id that names no row', () => { expect(res).toMatchObject({ object: 'task', id: 'real', record: { title: 'y' } }); }); - it('the existence probe is asked with the CALLER context, like getData', async () => { - // A row outside the caller's row scope is not-found FOR THEM on the read - // path; a write path that answered 200 for it would contradict the very - // next GET. + it('the existence probe asks EXISTENCE, not the caller\'s visibility', async () => { + // Load-bearing, and the first cut of this fix got it backwards. Probing + // with the CALLER's context turns the existence gate into an authorization + // gate: a row the caller cannot read comes back null and the PATCH 404s. + // That would (a) move an RLS decision out of the write policy where #1994 + // put it, and (b) disarm `@proof: rls-by-id-write` — the dogfood fixture + // whose RED half asserts that a member who cannot READ a row but has no + // write policy still mutates it by id. A caller-scoped probe makes that + // proof go green, so the gate could no longer prove it can go red, and a + // future revert of #1994 would be masked by this probe. + // + // So the probe runs as system: "does this row exist", nothing more. + // Authorization stays inside engine.update, exactly where it was. const { p, findOne } = makeProtocol({ real: { id: 'real' } }); - const ctx = { userId: 'u1' }; - await p.updateData({ object: 'task', id: 'real', data: {}, context: ctx } as any); - expect(findOne.mock.calls[0][1]).toMatchObject({ where: { id: 'real' }, context: ctx }); + await p.updateData({ object: 'task', id: 'real', data: {}, context: { userId: 'u1' } } as any); + expect(findOne.mock.calls[0][1]).toMatchObject({ + where: { id: 'real' }, + context: { isSystem: true }, + }); + }); + + it('a PATCH the caller may not see is still decided by RLS, not by the probe', async () => { + // The row exists, so the probe passes it through to the engine — where the + // write policy answers, as it always has. The probe must not pre-empt it. + const { p, update } = makeProtocol({ hidden: { id: 'hidden' } }); + await p.updateData({ object: 'task', id: 'hidden', data: { title: 'x' }, context: { userId: 'nobody' } } as any); + expect(update).toHaveBeenCalledOnce(); }); }); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 2dbd5aaeda..8e74fe77d7 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -4739,21 +4739,35 @@ export class ObjectStackProtocolImplementation implements async updateData(request: { object: string, id: string, data: any, expectedVersion?: string, context?: any }) { this.assertObjectRegistered(request.object); // [#3770] - await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context); - // [#4435] A PATCH of an id that names no row answered `200 { record: - // null }` — the caller had to null-check a SUCCESS payload to discover - // its write never landed, which is exactly what a client that PATCHes a - // concurrently deleted record does not do. `getData` has always answered - // 404 for the same id; the two verbs now agree. + // [#4435] ONE probe serves both gates. // - // Checked BEFORE the write rather than by inspecting what comes back: - // the engine's update returns the post-write READBACK, which is also - // `null` when the row still exists but the write moved it out of the - // caller's row scope (reassigning `owner_id` away from yourself under an - // owner-scoped RLS policy). Reading that as "not found" would answer 404 - // to a write that succeeded. Existence is the question, so ask it - // directly — the same `findOne` + context `getData` asks. - await this.assertRecordExists(request.object, request.id, request.context); + // A PATCH of an id that names no row answered `200 { record: null }` — + // the caller had to null-check a SUCCESS payload to discover its write + // never landed, which is exactly what a client that PATCHes a + // concurrently deleted record does not do. `getData` has always + // answered 404 for the same id; the two verbs now agree. + // + // Existence is asked BEFORE the write rather than read off what comes + // back: the engine's update returns the post-write READBACK, which is + // also `null` when the row still exists but the write moved it out of + // the caller's row scope (reassigning `owner_id` away from yourself + // under an owner-scoped RLS policy). Reading that as "not found" would + // answer 404 to a write that succeeded. So ask existence directly. + // + // The probe asks EXISTENCE, not visibility — see `probeRecord` for why + // that distinction is load-bearing (it keeps this fix out of the RLS + // model and keeps the #1994 by-id-write proof able to go red). + // + // OCC already had to read the same row for its `updated_at`, so the two + // gates share this single read instead of issuing a probe each. Two + // round-trips per PATCH would have been a performance regression no + // gate reports, and the second read could even disagree with the first. + const current = await this.probeRecord(request.object, request.id); + if (!current) throw recordNotFoundError(request.object, request.id); + // 404 wins over 409 when both could apply: OCC has always declined to + // treat a missing record as a concurrency conflict, and "this record + // does not exist" is the more specific answer. + this.assertVersionOf(request.object, request.id, current, request.expectedVersion); const opts: any = { where: { id: request.id } }; if (request.context !== undefined) opts.context = request.context; // [#3407/#3431] Capture the engine's LEGAL write strips (static `readonly` @@ -4775,7 +4789,7 @@ export class ObjectStackProtocolImplementation implements async deleteData(request: { object: string, id: string, expectedVersion?: string, context?: any }) { this.assertObjectRegistered(request.object); // [#3770] - await this.assertVersionMatch(request.object, request.id, request.expectedVersion, request.context); + await this.assertVersionMatch(request.object, request.id, request.expectedVersion); const opts: any = { where: { id: request.id } }; if (request.context !== undefined) opts.context = request.context; const deleted = await this.engine.delete(request.object, opts); @@ -4801,50 +4815,72 @@ export class ObjectStackProtocolImplementation implements } /** - * [#4435] Throw `404 RECORD_NOT_FOUND` when `id` names no row VISIBLE to - * this caller — the same question, asked the same way, that `getData` - * answers with. Threading `context` is what makes "visible to this caller" - * true: a row the caller's RLS scope excludes is not-found for them on the - * read path, and a write path that disagreed would answer 200 for a record - * the very next GET reports as 404. + * [#4435] Does this row EXIST? A fact about the database — deliberately + * NOT "may this caller see it". + * + * Read with a system context so no row-level policy narrows it. That is + * load-bearing, and the first version of this fix got it wrong: probing + * with the CALLER's context turns the existence gate into an authorization + * gate, because a row the caller cannot read comes back `null` and the + * PATCH answers 404. Two things break when it does. + * + * 1. It silently changes RLS semantics. Whether an unreadable row may be + * written by id is the #1994 pre-image check's decision, made inside + * `engine.update` where the write policy lives. A probe in front of it + * quietly adds a second, different rule — scope creep into the security + * model, from a bug fix about missing records. + * + * 2. It disarms a revert-provable security proof. `@proof: rls-by-id-write` + * (`packages/qa/dogfood/test/rls-fixture.dogfood.test.ts`, referenced by + * the `permission.rowLevelSecurity.using` liveness ledger entry) boots a + * fixture whose member can read nothing but has no write policy, and + * asserts the runner reports `rls-hole`. A caller-scoped probe 404s that + * PATCH, so the RED half goes green — and the gate can no longer prove + * it is able to go red. If the #1994 fix were ever reverted, this probe + * would MASK it. Accidentally hardening one path is not worth + * permanently blinding the gate that watches the whole class. + * + * So the probe answers existence only, and authorization stays exactly + * where it was. The one behaviour this adds is the 404 the issue asked for: + * an id that names no row at all. + * + * One place, because two gates need this row — the existence gate and OCC's + * `updated_at` comparison — and issuing a probe each would put two + * round-trips on every PATCH. */ - private async assertRecordExists(object: string, id: string, context: any): Promise { - const findOpts: any = { where: { id } }; - if (context !== undefined) findOpts.context = context; - const existing = await this.engine.findOne(object, findOpts); - if (!existing) throw recordNotFoundError(object, id); + private async probeRecord(object: string, id: string): Promise { + return this.engine.findOne(object, { where: { id }, context: { isSystem: true } } as any); } /** - * Optimistic Concurrency Control gate shared by updateData/deleteData. + * Optimistic Concurrency Control — the COMPARISON half, over a row the + * caller has already read. Pure: it issues no query of its own, which is + * what lets `updateData` run the gate on its existence probe's result + * rather than re-reading the record (#4435). * - * When the caller passes a non-empty `expectedVersion` token (typically - * the `updated_at` value they read), this fetches the current record - * and compares its `updated_at` against the token. Mismatch → throw - * `ConcurrentUpdateError` which the REST layer maps to 409. + * When the caller passes a non-empty `expectedVersion` token (typically the + * `updated_at` value they read), a mismatch throws `ConcurrentUpdateError`, + * which the REST layer maps to 409. * * Behaviour: * - Empty/missing token → no check (opt-in semantics; existing callers * that haven't yet adopted OCC are unaffected). - * - Record not found → no check; downstream `engine.update` will - * surface the usual `RECORD_NOT_FOUND` 404. We intentionally do not - * treat "missing record" as a concurrency conflict. + * - 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. * - Record has no `updated_at` field (timestamps disabled) → no check. * Logging would be noisy here; OCC is opt-in and the absence of a * version column is an explicit "this object doesn't support OCC" * signal. */ - private async assertVersionMatch( + private assertVersionOf( object: string, id: string, + current: any, expectedVersion: string | undefined, - context: any - ): Promise { + ): void { const expected = normaliseVersionToken(expectedVersion); if (!expected) return; - const findOpts: any = { where: { id } }; - if (context !== undefined) findOpts.context = context; - const current = await this.engine.findOne(object, findOpts); if (!current) return; const currentVersion = normaliseVersionToken((current as any).updated_at); if (!currentVersion) return; @@ -4857,6 +4893,22 @@ export class ObjectStackProtocolImplementation implements } } + /** + * OCC gate for `deleteData`, which — unlike `updateData` — needs no + * 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. + */ + private async assertVersionMatch( + object: string, + id: string, + expectedVersion: string | undefined, + ): Promise { + if (!normaliseVersionToken(expectedVersion)) return; + const current = await this.probeRecord(object, id); + this.assertVersionOf(object, id, current, expectedVersion); + } + // ========================================== // Global Search (M10.5) // ========================================== diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index 8ba3ce6aaf..bf7a26cd9f 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -403,16 +403,26 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { // ═══════════════════════════════════════════════════════════════ describe('Optimistic Concurrency Control', () => { beforeEach(() => { - // Both update and delete need `update` / `delete` on the - // engine, plus `findOne` for the version probe. + // Both update and delete need `update` / `delete` on the engine, + // plus `findOne` for the record probe. + // + // [#4435] The record now has to EXIST for a PATCH to proceed at + // all: `updateData` refuses an id that names no row instead of + // answering `200 { record: null }`. The default probe result is + // therefore a real row — these cases are about OCC, not about + // missing records (which `updateData refuses an id …` covers). + mockEngine.findOne.mockResolvedValue({ id: 'r1', updated_at: '2026-05-22T07:14:00.000Z' }); mockEngine.update = vi.fn().mockResolvedValue({ id: 'r1', updated_at: '2026-05-22T07:14:33.000Z' }); mockEngine.delete = vi.fn().mockResolvedValue(true); }); it('updateData proceeds when no expectedVersion is supplied (legacy callers)', async () => { await protocol.updateData({ object: 'task', id: 'r1', data: { name: 'New' } }); - // No version probe was issued - expect(mockEngine.findOne).not.toHaveBeenCalled(); + // [#4435] One probe — the EXISTENCE probe, which every PATCH now + // makes. No OCC comparison happens (no token was supplied), which + // is what "legacy callers are unaffected" means: they are not + // subject to 409, only to the 404 that GET already answered. + expect(mockEngine.findOne).toHaveBeenCalledOnce(); expect(mockEngine.update).toHaveBeenCalledOnce(); }); @@ -424,10 +434,31 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { data: { name: 'New' }, expectedVersion: '2026-05-22T07:14:00.000Z', }); + // [#4435] Still exactly ONE read. The existence gate and OCC both + // need this row, so they share the probe rather than issuing one + // each — two round-trips per PATCH would be a performance + // regression no gate reports, and the second read could disagree + // with the first. expect(mockEngine.findOne).toHaveBeenCalledOnce(); expect(mockEngine.update).toHaveBeenCalledOnce(); }); + it('updateData refuses an id that names no row, before any OCC verdict', async () => { + // [#4435] 404 wins over 409 when both could apply: OCC has always + // declined to treat a missing record as a concurrency conflict, and + // "this record does not exist" is the more specific answer. + mockEngine.findOne.mockResolvedValue(null); + await expect( + protocol.updateData({ + object: 'task', + id: 'gone', + data: { name: 'New' }, + expectedVersion: '2026-05-22T07:14:00.000Z', + }) + ).rejects.toMatchObject({ code: 'RECORD_NOT_FOUND', status: 404 }); + expect(mockEngine.update).not.toHaveBeenCalled(); + }); + it('updateData strips RFC-7232 quotes from the If-Match token', async () => { mockEngine.findOne.mockResolvedValue({ id: 'r1', updated_at: '2026-05-22T07:14:00.000Z' }); await protocol.updateData({ @@ -475,16 +506,29 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { }); it('updateData skips the check when expectedVersion is empty string', async () => { + // A blank token opts OUT of OCC — the record still has to exist + // (#4435), so the probe happens; what must NOT happen is a 409. await protocol.updateData({ object: 'task', id: 'r1', data: { name: 'New' }, expectedVersion: ' ', }); - expect(mockEngine.findOne).not.toHaveBeenCalled(); + expect(mockEngine.findOne).toHaveBeenCalledOnce(); expect(mockEngine.update).toHaveBeenCalledOnce(); }); + it('deleteData without a token issues NO probe at all', async () => { + // [#4435] DELETE needs no existence probe: the driver's own return + // ("True if deleted, false if not found") reports whether a row + // matched, so a plain DELETE stays at zero extra reads and only an + // OCC token buys one. + mockEngine.findOne.mockClear(); + await protocol.deleteData({ object: 'task', id: 'r1' }); + expect(mockEngine.findOne).not.toHaveBeenCalled(); + expect(mockEngine.delete).toHaveBeenCalledOnce(); + }); + it('deleteData throws ConcurrentUpdateError on version mismatch', async () => { mockEngine.findOne.mockResolvedValue({ id: 'r1',