From f540db571079326889a163562dab6e9037bf4305 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 09:13:44 +0000 Subject: [PATCH] fix(runtime): serve VALIDATION_FAILED as 400 with fields[] from both dispatcher exits (#3918) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ValidationError` carries `.code = 'VALIDATION_FAILED'` and `.fields[]`, and deliberately carries no `.status`, no `.statusCode` and no `.issues` — deciding it means "400" is the HTTP boundary's job. `@objectstack/rest` has always done that (`mapDataError`). The runtime dispatcher's two error exits did not, because each read exactly the properties this error lacks: - `HttpDispatcher.errorFromThrown` fell back to the caller's `fallbackStatus` for want of a `.status`, and built `details` from `.issues` alone — so `fields[]` was dropped. - `dispatcher-plugin`'s `errorResponseBase` took the same 500 fallback, and its body was only `{message, code}`. Landing on 5xx then dragged the message through the #3867 leak sanitiser, so a bad email address came back as a 500 "Internal server error" with nothing to attach to the offending input. Both exits now answer the way rest-server does: status 400, with `fields[]` passed through verbatim in `details` alongside `code: 'VALIDATION_FAILED'`, so every surface the dispatcher serves can do per-field error display. The predicate lives in the new `validation-failure.ts` — one definition shared by both exits, duck-typed on `code`/`name` exactly like `mapDataError`, so the runtime takes no dependency on objectql and hand-thrown errors of the same shape are served identically. An explicit `.status`/`.statusCode` still wins; 400 is supplied only as the fallback that used to be 500. Non-validation errors keep their status, message sanitising and `details` untouched, and `errorResponseBase` still emits its exact two-key body for them. Covered by `dispatcher-validation-error.test.ts`, which drives both exits through real routes (`/packages/:id/publish-drafts` for the returned path, `POST /analytics/query` for the thrown one); 11 of its 15 cases fail on the pre-fix source. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeZ7DmSKjiKmQzYh8L5CJS --- .../dispatcher-validation-error-fields.md | 40 +++ packages/runtime/src/dispatcher-plugin.ts | 18 +- .../src/dispatcher-validation-error.test.ts | 305 ++++++++++++++++++ packages/runtime/src/http-dispatcher.ts | 22 +- packages/runtime/src/validation-failure.ts | 47 +++ 5 files changed, 428 insertions(+), 4 deletions(-) create mode 100644 .changeset/dispatcher-validation-error-fields.md create mode 100644 packages/runtime/src/dispatcher-validation-error.test.ts create mode 100644 packages/runtime/src/validation-failure.ts diff --git a/.changeset/dispatcher-validation-error-fields.md b/.changeset/dispatcher-validation-error-fields.md new file mode 100644 index 0000000000..02d19967f5 --- /dev/null +++ b/.changeset/dispatcher-validation-error-fields.md @@ -0,0 +1,40 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): dispatcher error exits serve VALIDATION_FAILED as 400 with `fields[]` (#3918) + +`ValidationError` — what objectql's record and rule validators throw — carries +`.code = 'VALIDATION_FAILED'` and `.fields[]`, one entry per offending field. It +deliberately carries no `.status`, no `.statusCode`, and no `.issues`: it is a +plain domain error, and deciding it means "400" is the HTTP boundary's job. +`@objectstack/rest` has always done that (`mapDataError` → 400 with `fields[]`). +The runtime dispatcher's two error exits did not, because each read exactly the +properties this error lacks: + +- **`HttpDispatcher.errorFromThrown`** (the RETURNED-error path — `/meta` save, + `/packages` publish, …) fell back to the caller's `fallbackStatus` for want of + a `.status`, and built its structured `details` from `.issues` alone, so + `fields[]` was dropped. +- **`dispatcher-plugin`'s `errorResponseBase`** (the THROWN-error path — every + route the plugin mounts: `/analytics`, `/packages`, `/i18n`, `/storage`, + `/automation`, `/auth`, `/notifications`, `/mcp`, …) took the same 500 + fallback, and its body was only `{message, code}`. Landing on 5xx then dragged + the message through the #3867 leak sanitiser, so a user typing a bad email + address got back a **500 "Internal server error"** — no status a client could + act on, no message worth showing, and nothing to attach to the input. + +Both exits now recognise the shape and answer the way rest-server does: **status +400**, with the error's `fields[]` passed through verbatim in `details` +alongside `code: 'VALIDATION_FAILED'`. Any surface the dispatcher serves can +therefore highlight the specific field the user got wrong, the way a form served +by `/data` already could. + +Matched by duck-typing on `code === 'VALIDATION_FAILED' || name === +'ValidationError'` — the same both-ways predicate `mapDataError` uses — so a +hook or service that throws `{ code: 'VALIDATION_FAILED', fields }` by hand is +served identically, and the runtime takes no dependency on objectql. An explicit +`.status` / `.statusCode` on the error still wins: 400 is supplied only as the +fallback that was previously 500. Non-validation errors are untouched — same +status, same message sanitising, same `details`, and `errorResponseBase` still +emits the exact two-key body it always did. diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index f3d899c019..c77562dd4c 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -3,6 +3,7 @@ import { Plugin, PluginContext, IHttpServer } from '@objectstack/core'; import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; import { HttpDispatcher, HttpDispatcherResult } from './http-dispatcher.js'; +import { validationFailureDetails, VALIDATION_FAILED_STATUS } from './validation-failure.js'; import { buildSecurityHeaders, type SecurityHeadersOptions, @@ -374,12 +375,25 @@ function sendResultBase( * * Sanitising costs no diagnostics: the untouched error is still handed to * `errorReporter` through the `__obsRecordedError` side-channel below. + * + * [#3918] A third defect, of the same family as (1): a record-level + * `ValidationError` carries neither `status` nor `statusCode`, so it landed on + * the 500 fallback — and because the body was only `{message, code}`, its + * `fields[]` was dropped AND the 5xx sanitiser above replaced the human message + * with the generic INTERNAL_ERROR_MESSAGE. A user typing a bad email got back a + * 500 "internal error" with nothing to attach to the offending input. + * `@objectstack/rest` has mapped this shape to a 400 with `fields[]` since + * forever (`mapDataError`); this exit now does the same, so a form served by + * the dispatcher can highlight the field the way a form served by /data can. + * `details` is only emitted for that shape — everything else keeps the exact + * two-key body it had. */ function errorResponseBase(err: any, res: any, securityHeaders?: Record): void { + const validation = validationFailureDetails(err); const code = (typeof err?.status === 'number' ? err.status : undefined) ?? (typeof err?.statusCode === 'number' ? err.statusCode : undefined) ?? - 500; + (validation ? VALIDATION_FAILED_STATUS : 500); res.status(code); if (securityHeaders) { for (const [k, v] of Object.entries(securityHeaders)) { @@ -404,7 +418,7 @@ function errorResponseBase(err: any, res: any, securityHeaders?: Record { throw publishError; }, + }; + // The route's 503 pre-check: it needs an objectql service with a registry + // before it will reach the protocol call at all. + const objectql = { registry: {} }; + const resolve = (name: string) => + name === 'protocol' ? protocol : name === 'objectql' ? objectql : undefined; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + }; + return new HttpDispatcher(kernel); +} + +async function publishPackage(publishError: unknown) { + const dispatcher = makeDispatcher(publishError); + const result: any = await dispatcher.dispatch( + 'POST', + '/packages/demo/publish-drafts', + {}, + {}, + {} as any, + ); + return result.response; +} + +describe('#3918 — HttpDispatcher.errorFromThrown maps VALIDATION_FAILED', () => { + it('answers 400, not the caller\'s 500 fallback', async () => { + const res = await publishPackage(makeValidationError()); + + expect(res.status).toBe(400); + expect(res.body.error.code).toBe(400); + }); + + it('passes `fields[]` through in `details` so the UI can anchor each error', async () => { + const res = await publishPackage(makeValidationError()); + + expect(res.body.error.details).toEqual({ + code: 'VALIDATION_FAILED', + fields: FIELDS, + }); + }); + + it('keeps the human message intact (400 never reaches the 5xx sanitiser)', async () => { + const res = await publishPackage(makeValidationError()); + + expect(res.body.error.message).toBe('email must be a valid email address; name is required'); + }); + + it('recognises the shape by `code` alone, for hand-thrown validation errors', async () => { + // A hook or service that throws `{ code: 'VALIDATION_FAILED', fields }` + // without extending ValidationError must be served identically — the + // same both-ways predicate `mapDataError` uses. + const err = Object.assign(new Error('quantity must be at least 1'), { + code: 'VALIDATION_FAILED', + fields: [{ field: 'quantity', code: 'min_value', message: 'quantity must be at least 1' }], + }); + const res = await publishPackage(err); + + expect(res.status).toBe(400); + expect(res.body.error.details.fields).toEqual(err.fields); + }); + + it('pins `details.code` to VALIDATION_FAILED when matched by `name` alone', async () => { + const err = new Error('name is required'); + err.name = 'ValidationError'; + (err as any).fields = [{ field: 'name', code: 'required', message: 'name is required' }]; + const res = await publishPackage(err); + + expect(res.status).toBe(400); + expect(res.body.error.details.code).toBe('VALIDATION_FAILED'); + }); + + it('defaults `fields` to [] when the error carries none', async () => { + // `details.fields` is the contract; a caller mapping it must never have + // to guard for `undefined`. + const err = Object.assign(new Error('invalid'), { code: 'VALIDATION_FAILED' }); + const res = await publishPackage(err); + + expect(res.status).toBe(400); + expect(res.body.error.details.fields).toEqual([]); + }); + + it('still lets an explicit `status` win — 400 is only the fallback', async () => { + const err = Object.assign(makeValidationError(), { status: 422 }); + const res = await publishPackage(err); + + expect(res.status).toBe(422); + expect(res.body.error.details.fields).toEqual(FIELDS); + }); + + it('leaves a non-validation error on its old path', async () => { + // Anti-regression for the #3867 tier this sits next to: an ordinary + // throw still takes the caller's 500 fallback and its `issues`/`code` + // details shape. + const err = Object.assign(new Error('publish backend unavailable'), { + code: 'STORAGE_FAILURE', + issues: [{ path: 'a', message: 'b', code: 'c' }], + }); + const res = await publishPackage(err); + + expect(res.status).toBe(500); + expect(res.body.error.details).toEqual({ + code: 'STORAGE_FAILURE', + issues: [{ path: 'a', message: 'b', code: 'c' }], + }); + expect(res.body.error.details.fields).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// Exit 2 — dispatcher-plugin's errorResponseBase (the THROWN error path) +// --------------------------------------------------------------------------- + +function makeFakeServer() { + const handlers: Record any> = {}; + const rec = (verb: string) => (path: string, handler: any) => { + handlers[`${verb} ${path}`] = handler; + }; + return { + handlers, + server: { + get: rec('GET'), + post: rec('POST'), + put: rec('PUT'), + delete: rec('DELETE'), + patch: rec('PATCH'), + }, + }; +} + +function makeCtx(fakeServer: any, analyticsError: unknown) { + const analytics = { + query: async () => { throw analyticsError; }, + getMeta: async () => ({ cubes: [] }), + generateSql: async () => ({ sql: null }), + }; + const kernel = { + getService: (name: string) => (name === 'analytics' ? analytics : undefined), + getServiceAsync: async (name: string) => (name === 'analytics' ? analytics : undefined), + }; + return { + getKernel: () => kernel, + getService: (name: string) => (name === 'http.server' ? fakeServer : undefined), + environmentId: undefined, + logger: { info() {}, warn() {}, error() {}, debug() {} }, + hook: () => {}, + on: () => {}, + } as any; +} + +function makeRes() { + const res: any = { + statusCode: undefined as number | undefined, + body: undefined as any, + status(c: number) { res.statusCode = c; return res; }, + header() { return res; }, + json(b: any) { res.body = b; return res; }, + }; + return res; +} + +/** Drive `POST /analytics/query` with a service that throws `err`. */ +async function postAnalyticsQuery(err: unknown) { + const { server, handlers } = makeFakeServer(); + const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false }); + await plugin.start?.(makeCtx(server, err)); + + const handler = handlers['POST /api/v1/analytics/query']; + expect(handler, 'POST /api/v1/analytics/query must be mounted').toBeTypeOf('function'); + + const res = makeRes(); + await handler({ body: { cube: 'x', query: {} }, query: {} }, res); + return res; +} + +describe('#3918 — dispatcher-plugin errorResponseBase maps VALIDATION_FAILED', () => { + it('answers 400 instead of 500', async () => { + const res = await postAnalyticsQuery(makeValidationError()); + + expect(res.statusCode).toBe(400); + expect(res.body.success).toBe(false); + expect(res.body.error.code).toBe(400); + }); + + it('carries `fields[]` — the body used to be `{message, code}` only', async () => { + const res = await postAnalyticsQuery(makeValidationError()); + + expect(res.body.error.details).toEqual({ + code: 'VALIDATION_FAILED', + fields: FIELDS, + }); + }); + + it('keeps the human message — the 500 fallback used to replace it', async () => { + // The regression that motivated the issue: on 5xx the #3867 sanitiser + // swaps the message for INTERNAL_ERROR_MESSAGE, so a user-input mistake + // was served as a generic "internal error". + const res = await postAnalyticsQuery(makeValidationError()); + + expect(res.body.error.message).toBe('email must be a valid email address; name is required'); + expect(res.body.error.message).not.toBe('Internal server error'); + }); + + it('does not record a validation failure as a server fault', async () => { + // `__obsRecordedError` feeds errorReporter and is set on 5xx only. A bad + // payload is not an incident — at 400 the side-channel stays clear. + const res = await postAnalyticsQuery(makeValidationError()); + + expect((res as any).__obsRecordedError).toBeUndefined(); + }); + + it('still lets an explicit `status` win', async () => { + const res = await postAnalyticsQuery(Object.assign(makeValidationError(), { status: 422 })); + + expect(res.statusCode).toBe(422); + expect(res.body.error.details.fields).toEqual(FIELDS); + }); + + it('leaves non-validation errors with the exact two-key body they had', async () => { + const res = await postAnalyticsQuery(new Error('analytics engine unavailable')); + + expect(res.statusCode).toBe(500); + expect(res.body.error).toEqual({ message: 'analytics engine unavailable', code: 500 }); + }); +}); + +// --------------------------------------------------------------------------- +// Parity with the surface that already got this right +// --------------------------------------------------------------------------- + +describe('#3918 — both dispatcher exits agree with @objectstack/rest', () => { + it('serves the same status and the same `fields[]` from either exit', async () => { + const returned = await publishPackage(makeValidationError()); + const thrown = await postAnalyticsQuery(makeValidationError()); + + // `mapDataError` answers 400 with `fields` = the error's `.fields`. + expect(returned.status).toBe(400); + expect(thrown.statusCode).toBe(400); + expect(returned.body.error.details.fields).toEqual(FIELDS); + expect(thrown.body.error.details.fields).toEqual(FIELDS); + }); +}); diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 5e927cc53f..0d99145aa3 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -35,6 +35,7 @@ import { resolveExecutionContext, isPermissionDeniedError, } from './security/resolve-execution-context.js'; +import { validationFailureDetails, VALIDATION_FAILED_STATUS } from './validation-failure.js'; // randomUUID moved to ./domains/auth.ts with its only consumer (D11③ PR-7). @@ -491,16 +492,33 @@ export class HttpDispatcher { * banner; carrying `issues` (and the semantic `code`) in `details` lets it * map each error back to the offending field. Falls back to `fallbackStatus` * and behaves exactly like `error()` for errors that carry neither. + * + * [#3918] A record-level `ValidationError` is the third structured shape, + * and it used to fall through BOTH branches: it carries no `.status` (so a + * `/meta`-style 400 fallback saved it, but a 500-fallback caller did not) + * and no `.issues` (so its `.fields[]` — the whole point — was dropped and + * the UI could only show a banner). It now maps the way + * `@objectstack/rest`'s `mapDataError` has always mapped it: status 400, + * `fields[]` passed through in `details`. An explicit `.status` / + * `.statusCode` still wins, so this only supplies the fallback. */ private errorFromThrown(e: any, fallbackStatus = 500) { + const validation = validationFailureDetails(e); const status = typeof e?.status === 'number' ? e.status : typeof e?.statusCode === 'number' ? e.statusCode + : validation ? VALIDATION_FAILED_STATUS : fallbackStatus; const issues = Array.isArray(e?.issues) ? e.issues : undefined; const details = - issues || e?.code - ? { ...(e?.code ? { code: e.code } : {}), ...(issues ? { issues } : {}) } + issues || e?.code || validation + ? { + ...(e?.code ? { code: e.code } : {}), + ...(issues ? { issues } : {}), + // Last so `code` is pinned to VALIDATION_FAILED even when the + // error was matched by `name` alone and carries no `.code`. + ...(validation ?? {}), + } : undefined; return this.error(e?.message ?? String(e), status, details); } diff --git a/packages/runtime/src/validation-failure.ts b/packages/runtime/src/validation-failure.ts new file mode 100644 index 0000000000..af7c7263aa --- /dev/null +++ b/packages/runtime/src/validation-failure.ts @@ -0,0 +1,47 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Recognising a record-validation failure at an HTTP boundary. + * + * `ValidationError` (`@objectstack/objectql`'s record/rule validators) carries + * `.code = 'VALIDATION_FAILED'` and `.fields[]` — one entry per offending + * field — but deliberately carries NO `.status` / `.statusCode` and no + * `.issues`. It is a plain domain error; deciding it means "400" is the job of + * whichever boundary serves it. + * + * `@objectstack/rest` has always done that (`mapDataError` → 400 with + * `fields[]`). The runtime dispatcher's two error exits did not (#3918): with + * no `.status` to read they fell back to **500**, and both read only `.issues` + * for structured detail — which a `ValidationError` never has — so `fields[]` + * was dropped and the caller got a generic "internal error" for what was + * really a user-input mistake. That forecloses per-field error display on every + * surface the dispatcher serves. + * + * Matched by duck-typing on `code` / `name` — exactly the predicate + * `mapDataError` uses — so this module stays free of a runtime dependency on + * `objectql`, and so hand-rolled errors of the same shape (e.g. a hook that + * throws `{ code: 'VALIDATION_FAILED', fields }`) are served identically. + */ + +/** The HTTP status a validation failure maps to when the error names none. */ +export const VALIDATION_FAILED_STATUS = 400; + +export interface ValidationFailureDetails { + code: 'VALIDATION_FAILED'; + /** Per-field envelopes, passed through verbatim. `[]` when absent/malformed. */ + fields: unknown[]; +} + +/** + * Structured `details` for a thrown validation failure, or `undefined` when + * `err` is not one. Callers use the `undefined` result as the predicate and the + * returned object as the `details` payload, so the two can never disagree. + */ +export function validationFailureDetails(err: any): ValidationFailureDetails | undefined { + if (!err) return undefined; + if (err.code !== 'VALIDATION_FAILED' && err.name !== 'ValidationError') return undefined; + return { + code: 'VALIDATION_FAILED', + fields: Array.isArray(err.fields) ? err.fields : [], + }; +}