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`. 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..363527d7ab --- /dev/null +++ b/packages/metadata-protocol/src/protocol.record-not-found.test.ts @@ -0,0 +1,202 @@ +// 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 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' } }); + 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(); + }); +}); + +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..8e74fe77d7 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. @@ -4725,7 +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] ONE probe serves both gates. + // + // 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` @@ -4747,10 +4789,24 @@ 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; - 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, @@ -4759,35 +4815,72 @@ export class ObjectStackProtocolImplementation implements } /** - * Optimistic Concurrency Control gate shared by updateData/deleteData. + * [#4435] Does this row EXIST? A fact about the database — deliberately + * NOT "may this caller see it". * - * 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. + * 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 probeRecord(object: string, id: string): Promise { + return this.engine.findOne(object, { where: { id }, context: { isSystem: true } } as any); + } + + /** + * 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), 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; @@ -4800,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) // ========================================== @@ -5359,7 +5468,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) { 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', 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-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..99052de4e1 --- /dev/null +++ b/packages/plugins/driver-memory/src/memory-filter-refusal-envelope.test.ts @@ -0,0 +1,74 @@ +// 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('deal', { + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, + }); + 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 f00e6d96f1..1e52625c4c 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 { @@ -5080,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 ` + @@ -5136,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)}`, ); @@ -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).`, ); @@ -5450,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; @@ -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.`, ); 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; } /** 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)];