diff --git a/.changeset/dispatcher-returned-error-leak.md b/.changeset/dispatcher-returned-error-leak.md new file mode 100644 index 0000000000..cb0ff05aa1 --- /dev/null +++ b/.changeset/dispatcher-returned-error-leak.md @@ -0,0 +1,42 @@ +--- +"@objectstack/runtime": patch +"@objectstack/hono": patch +--- + +fix(runtime,hono): close the remaining raw-driver-message exits on the HTTP boundary (#3867 follow-up) + +#3867 sanitised `dispatcher-plugin`'s `errorResponseBase`. That covers errors +**thrown** out of `dispatch()` — but not the ones it **returns**. A +`{handled: true, response}` result goes to `sendResult`, never through that +catch, and those bodies are built by `HttpDispatcher.error()`, which passed the +message through verbatim. Sweeping the boundary for the same defect class (the +follow-up #3867 called for) turned up two more live exits: + +**`HttpDispatcher.error()`** — the single construction point for every returned +error response. Reachable with a raw driver message today through +`errorFromThrown` (`/meta` save, `/packages` install) and the MCP transport's +`deps.error(err?.message, 500)`. Pinned by a test that drives +`PUT /meta/:type/:name` with a throwing `protocol.saveMetaItem`: without the +guard the response body is the driver's `insert into \`sys_team\` … UNIQUE +constraint failed: sys_team.id`, naming a physical table and column. + +**`@objectstack/hono`'s auth-config route** — a 500 built from a caught +error with `message: err.message`. The auth service reads from the database, so +that message can carry a driver dump. + +Both apply the same `looksLikeInternalErrorLeak` predicate #3867 put in +`@objectstack/types`, and both are scoped to **5xx** for the same reason: a 4xx +message is a deliberate business/validation answer (`Path must be +/actions/:object/:action`, a hook's own `throw`, a `saveMetaItem` field error) +and must reach the caller intact. Structured `details` — the semantic `code` and +per-field `issues` the Studio maps back to inputs — is never touched, so a +sanitised 500 still carries everything a client can act on. + +Diagnostics are unaffected: callers that threw still hand the original error to +`errorReporter` via `__obsRecordedError`, and every 5xx is logged server-side. + +Audited in the same pass and deliberately left alone: the inline error bodies in +the `ai` / `mcp` domains (static literal strings, no interpolated error text) and +`plugin-hono-server`'s 403s (4xx, deliberate messages). With this change every +dynamic message on both dispatcher exits and the REST data routes goes through +one predicate. diff --git a/packages/adapters/hono/src/index.ts b/packages/adapters/hono/src/index.ts index 8aab0c070f..93e3b8cc8d 100644 --- a/packages/adapters/hono/src/index.ts +++ b/packages/adapters/hono/src/index.ts @@ -7,7 +7,11 @@ import { HttpDispatcher, HttpDispatcherResult, } from '@objectstack/runtime'; -import { readEnvWithDeprecation } from '@objectstack/types'; +import { + readEnvWithDeprecation, + looksLikeInternalErrorLeak, + INTERNAL_ERROR_MESSAGE, +} from '@objectstack/types'; /** * Re-export the `Hono` type from the copy of `hono` this adapter owns. @@ -308,11 +312,17 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { } } catch (error) { const err = error instanceof Error ? error : new Error(String(error)); + // [#3867 follow-up] Same guard the two dispatcher error exits apply. + // This is a 500 built from a caught error, and the auth service reads + // from the database, so its message can carry a driver dump. Only the + // leak case is replaced — an ordinary config error still names itself. return c.json({ success: false, error: { code: 'auth_config_error', - message: err.message, + message: looksLikeInternalErrorLeak(err.message) + ? INTERNAL_ERROR_MESSAGE + : err.message, }, }, 500); } diff --git a/packages/runtime/src/http-dispatcher.error-leak.test.ts b/packages/runtime/src/http-dispatcher.error-leak.test.ts new file mode 100644 index 0000000000..7545fe648f --- /dev/null +++ b/packages/runtime/src/http-dispatcher.error-leak.test.ts @@ -0,0 +1,103 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3867 follow-up — the OTHER dispatcher error exit. + * + * #3867 sanitised `dispatcher-plugin`'s `errorResponseBase`, which handles + * errors THROWN out of `dispatch()`. It does not cover the errors `dispatch()` + * RETURNS: a `{handled: true, response}` result goes to `sendResult`, never + * through that catch. Those bodies are built by `HttpDispatcher.error()` — the + * single construction point for every returned error — and it passed the + * message through verbatim. + * + * That path is reachable with a raw driver message today via `errorFromThrown` + * (`/meta` save, `/packages` install) and the MCP transport's + * `deps.error(err?.message, 500)`. + * + * `error()` is private, so these drive it the way real traffic does: through + * `dispatch()` on routes whose service throws. + */ + +import { describe, it, expect } from 'vitest'; + +import { HttpDispatcher } from './http-dispatcher.js'; + +const SQL_DUMP = 'insert into `sys_team` (`id`) values (?) - UNIQUE constraint failed: sys_team.id'; + +/** + * A kernel whose `protocol` service throws on `saveMetaItem` — the `/meta` PUT + * route catches it and RETURNS `deps.errorFromThrown(e, 400)`, which is the + * returned-error path this guard covers (it never reaches the plugin's + * throw-side `errorResponseBase`). + */ +function makeDispatcher(saveError: unknown) { + const protocol = { + saveMetaItem: async () => { throw saveError; }, + }; + const kernel: any = { + getService: (name: string) => (name === 'protocol' ? protocol : undefined), + getServiceAsync: async (name: string) => (name === 'protocol' ? protocol : undefined), + }; + return new HttpDispatcher(kernel); +} + +async function putMeta(saveError: unknown) { + const dispatcher = makeDispatcher(saveError); + return dispatcher.dispatch( + 'PUT', + '/meta/object/widget', + { name: 'widget' }, + {}, + {} as any, + ); +} + +describe('#3867 follow-up — HttpDispatcher.error() does not return raw driver messages', () => { + it('replaces a SQL dump on a returned 5xx', async () => { + const err = Object.assign(new Error(SQL_DUMP), { status: 500 }); + const result: any = await putMeta(err); + + expect(result.response.status).toBe(500); + expect(result.response.body.error.message).toBe('Internal server error'); + expect(String(result.response.body.error.message)).not.toContain('sys_team'); + }); + + it('leaves a deliberate 4xx message intact even when it resembles SQL', async () => { + // The tier that matters: `errorFromThrown` defaults `/meta` saves to + // 400, and a validation message is the caller's answer — swallowing it + // would be a worse bug than the leak. + const err = Object.assign(new Error('unique constraint on name — pick another'), { + status: 422, + }); + const result: any = await putMeta(err); + + expect(result.response.status).toBe(422); + expect(result.response.body.error.message).toBe('unique constraint on name — pick another'); + }); + + it('leaves an ordinary 5xx message intact — only leaks are replaced', async () => { + const err = Object.assign(new Error('metadata store is unavailable'), { status: 503 }); + const result: any = await putMeta(err); + + expect(result.response.status).toBe(503); + expect(result.response.body.error.message).toBe('metadata store is unavailable'); + }); + + it('preserves structured `details` (code / issues) while sanitising the message', async () => { + // `details` carries the semantic code and per-field `issues` the UI maps + // back to inputs; it is never free-form driver prose, so the guard must + // not touch it. + const err = Object.assign(new Error(SQL_DUMP), { + status: 500, + code: 'STORAGE_FAILURE', + issues: [{ path: 'name', message: 'taken', code: 'duplicate' }], + }); + const result: any = await putMeta(err); + + expect(result.response.body.error.message).toBe('Internal server error'); + expect(result.response.body.error.details).toMatchObject({ + code: 'STORAGE_FAILURE', + issues: [{ path: 'name', message: 'taken', code: 'duplicate' }], + }); + }); +}); diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index e0fcef5963..5e927cc53f 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -3,7 +3,7 @@ import { ObjectKernel, getEnv, evaluateAuthGate, isAuthGateAllowlisted, } from '@objectstack/core'; -import { isMcpServerEnabled } from '@objectstack/types'; +import { isMcpServerEnabled, looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; import { measureServerTiming, allowPerfDisclosure, isPerfDisclosurePrincipal } from '@objectstack/observability'; import { CoreServiceName } from '@objectstack/spec/system'; import { readServiceSelfInfo } from '@objectstack/spec/api'; @@ -448,10 +448,37 @@ export class HttpDispatcher { }; } + /** + * The single construction point for every error response this dispatcher + * RETURNS. Distinct from `dispatcher-plugin`'s `errorResponseBase`, which + * covers the errors that are THROWN out of `dispatch()` — a returned + * `{handled: true, response}` goes to `sendResult`, never through that + * catch. #3867 sanitised the thrown path; this is the returned one, and it + * needs the same guard for the same reason. + * + * Reachable with a raw driver/engine message today via + * {@link errorFromThrown} (`/meta` save, `/packages` install) and the MCP + * transport's `deps.error(err?.message, 500)` — any of which can be + * carrying a SQL dump naming physical tables and columns. + * + * Scoped to 5xx: a 4xx message is a deliberate business/validation answer + * (`Path must be /actions/:object/:action`, a hook's own `throw`, a + * `saveMetaItem` field error) and must reach the caller intact. `details` + * is left alone — it carries structured `code`/`issues` the UI maps to + * fields, never free-form driver prose. + * + * The unsanitised error is not lost: callers that THREW still hand the + * original to `errorReporter` via `__obsRecordedError`, and every 5xx is + * logged server-side. + */ private error(message: string, code: number = 500, details?: any) { + const safe = + code >= 500 && looksLikeInternalErrorLeak(message) + ? INTERNAL_ERROR_MESSAGE + : message; return { status: code, - body: { success: false, error: { message, code, details } } + body: { success: false, error: { message: safe, code, details } } }; }