Skip to content

Commit cbedd62

Browse files
os-zhuangclaude
andauthored
fix(runtime,hono): close the remaining raw-driver-message exits on the HTTP boundary (#3867 follow-up) (#3884)
#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` and never reaches that catch, and those bodies are built by `HttpDispatcher.error()`, which passed the message through verbatim. Sweeping the boundary for the same defect class 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 driving `PUT /meta/:type/:name` with a throwing `protocol.saveMetaItem`: without the guard the response body is the driver's insert into `sys_team` (`id`) values (?) - 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 and must reach the caller intact. Structured `details` — the semantic `code` and per-field `issues` the Studio maps back to inputs — is never touched. 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) 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 647ec8b commit cbedd62

4 files changed

Lines changed: 186 additions & 4 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@objectstack/runtime": patch
3+
"@objectstack/hono": patch
4+
---
5+
6+
fix(runtime,hono): close the remaining raw-driver-message exits on the HTTP boundary (#3867 follow-up)
7+
8+
#3867 sanitised `dispatcher-plugin`'s `errorResponseBase`. That covers errors
9+
**thrown** out of `dispatch()` — but not the ones it **returns**. A
10+
`{handled: true, response}` result goes to `sendResult`, never through that
11+
catch, and those bodies are built by `HttpDispatcher.error()`, which passed the
12+
message through verbatim. Sweeping the boundary for the same defect class (the
13+
follow-up #3867 called for) turned up two more live exits:
14+
15+
**`HttpDispatcher.error()`** — the single construction point for every returned
16+
error response. Reachable with a raw driver message today through
17+
`errorFromThrown` (`/meta` save, `/packages` install) and the MCP transport's
18+
`deps.error(err?.message, 500)`. Pinned by a test that drives
19+
`PUT /meta/:type/:name` with a throwing `protocol.saveMetaItem`: without the
20+
guard the response body is the driver's `insert into \`sys_team\` … UNIQUE
21+
constraint failed: sys_team.id`, naming a physical table and column.
22+
23+
**`@objectstack/hono`'s auth-config route** — a 500 built from a caught
24+
error with `message: err.message`. The auth service reads from the database, so
25+
that message can carry a driver dump.
26+
27+
Both apply the same `looksLikeInternalErrorLeak` predicate #3867 put in
28+
`@objectstack/types`, and both are scoped to **5xx** for the same reason: a 4xx
29+
message is a deliberate business/validation answer (`Path must be
30+
/actions/:object/:action`, a hook's own `throw`, a `saveMetaItem` field error)
31+
and must reach the caller intact. Structured `details` — the semantic `code` and
32+
per-field `issues` the Studio maps back to inputs — is never touched, so a
33+
sanitised 500 still carries everything a client can act on.
34+
35+
Diagnostics are unaffected: callers that threw still hand the original error to
36+
`errorReporter` via `__obsRecordedError`, and every 5xx is logged server-side.
37+
38+
Audited in the same pass and deliberately left alone: the inline error bodies in
39+
the `ai` / `mcp` domains (static literal strings, no interpolated error text) and
40+
`plugin-hono-server`'s 403s (4xx, deliberate messages). With this change every
41+
dynamic message on both dispatcher exits and the REST data routes goes through
42+
one predicate.

packages/adapters/hono/src/index.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ import {
77
HttpDispatcher,
88
HttpDispatcherResult,
99
} from '@objectstack/runtime';
10-
import { readEnvWithDeprecation } from '@objectstack/types';
10+
import {
11+
readEnvWithDeprecation,
12+
looksLikeInternalErrorLeak,
13+
INTERNAL_ERROR_MESSAGE,
14+
} from '@objectstack/types';
1115

1216
/**
1317
* Re-export the `Hono` type from the copy of `hono` this adapter owns.
@@ -308,11 +312,17 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono {
308312
}
309313
} catch (error) {
310314
const err = error instanceof Error ? error : new Error(String(error));
315+
// [#3867 follow-up] Same guard the two dispatcher error exits apply.
316+
// This is a 500 built from a caught error, and the auth service reads
317+
// from the database, so its message can carry a driver dump. Only the
318+
// leak case is replaced — an ordinary config error still names itself.
311319
return c.json({
312320
success: false,
313321
error: {
314322
code: 'auth_config_error',
315-
message: err.message,
323+
message: looksLikeInternalErrorLeak(err.message)
324+
? INTERNAL_ERROR_MESSAGE
325+
: err.message,
316326
},
317327
}, 500);
318328
}
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #3867 follow-up — the OTHER dispatcher error exit.
5+
*
6+
* #3867 sanitised `dispatcher-plugin`'s `errorResponseBase`, which handles
7+
* errors THROWN out of `dispatch()`. It does not cover the errors `dispatch()`
8+
* RETURNS: a `{handled: true, response}` result goes to `sendResult`, never
9+
* through that catch. Those bodies are built by `HttpDispatcher.error()` — the
10+
* single construction point for every returned error — and it passed the
11+
* message through verbatim.
12+
*
13+
* That path is reachable with a raw driver message today via `errorFromThrown`
14+
* (`/meta` save, `/packages` install) and the MCP transport's
15+
* `deps.error(err?.message, 500)`.
16+
*
17+
* `error()` is private, so these drive it the way real traffic does: through
18+
* `dispatch()` on routes whose service throws.
19+
*/
20+
21+
import { describe, it, expect } from 'vitest';
22+
23+
import { HttpDispatcher } from './http-dispatcher.js';
24+
25+
const SQL_DUMP = 'insert into `sys_team` (`id`) values (?) - UNIQUE constraint failed: sys_team.id';
26+
27+
/**
28+
* A kernel whose `protocol` service throws on `saveMetaItem` — the `/meta` PUT
29+
* route catches it and RETURNS `deps.errorFromThrown(e, 400)`, which is the
30+
* returned-error path this guard covers (it never reaches the plugin's
31+
* throw-side `errorResponseBase`).
32+
*/
33+
function makeDispatcher(saveError: unknown) {
34+
const protocol = {
35+
saveMetaItem: async () => { throw saveError; },
36+
};
37+
const kernel: any = {
38+
getService: (name: string) => (name === 'protocol' ? protocol : undefined),
39+
getServiceAsync: async (name: string) => (name === 'protocol' ? protocol : undefined),
40+
};
41+
return new HttpDispatcher(kernel);
42+
}
43+
44+
async function putMeta(saveError: unknown) {
45+
const dispatcher = makeDispatcher(saveError);
46+
return dispatcher.dispatch(
47+
'PUT',
48+
'/meta/object/widget',
49+
{ name: 'widget' },
50+
{},
51+
{} as any,
52+
);
53+
}
54+
55+
describe('#3867 follow-up — HttpDispatcher.error() does not return raw driver messages', () => {
56+
it('replaces a SQL dump on a returned 5xx', async () => {
57+
const err = Object.assign(new Error(SQL_DUMP), { status: 500 });
58+
const result: any = await putMeta(err);
59+
60+
expect(result.response.status).toBe(500);
61+
expect(result.response.body.error.message).toBe('Internal server error');
62+
expect(String(result.response.body.error.message)).not.toContain('sys_team');
63+
});
64+
65+
it('leaves a deliberate 4xx message intact even when it resembles SQL', async () => {
66+
// The tier that matters: `errorFromThrown` defaults `/meta` saves to
67+
// 400, and a validation message is the caller's answer — swallowing it
68+
// would be a worse bug than the leak.
69+
const err = Object.assign(new Error('unique constraint on name — pick another'), {
70+
status: 422,
71+
});
72+
const result: any = await putMeta(err);
73+
74+
expect(result.response.status).toBe(422);
75+
expect(result.response.body.error.message).toBe('unique constraint on name — pick another');
76+
});
77+
78+
it('leaves an ordinary 5xx message intact — only leaks are replaced', async () => {
79+
const err = Object.assign(new Error('metadata store is unavailable'), { status: 503 });
80+
const result: any = await putMeta(err);
81+
82+
expect(result.response.status).toBe(503);
83+
expect(result.response.body.error.message).toBe('metadata store is unavailable');
84+
});
85+
86+
it('preserves structured `details` (code / issues) while sanitising the message', async () => {
87+
// `details` carries the semantic code and per-field `issues` the UI maps
88+
// back to inputs; it is never free-form driver prose, so the guard must
89+
// not touch it.
90+
const err = Object.assign(new Error(SQL_DUMP), {
91+
status: 500,
92+
code: 'STORAGE_FAILURE',
93+
issues: [{ path: 'name', message: 'taken', code: 'duplicate' }],
94+
});
95+
const result: any = await putMeta(err);
96+
97+
expect(result.response.body.error.message).toBe('Internal server error');
98+
expect(result.response.body.error.details).toMatchObject({
99+
code: 'STORAGE_FAILURE',
100+
issues: [{ path: 'name', message: 'taken', code: 'duplicate' }],
101+
});
102+
});
103+
});

packages/runtime/src/http-dispatcher.ts

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import {
44
ObjectKernel, getEnv, evaluateAuthGate, isAuthGateAllowlisted,
55
} from '@objectstack/core';
6-
import { isMcpServerEnabled } from '@objectstack/types';
6+
import { isMcpServerEnabled, looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types';
77
import { measureServerTiming, allowPerfDisclosure, isPerfDisclosurePrincipal } from '@objectstack/observability';
88
import { CoreServiceName } from '@objectstack/spec/system';
99
import { readServiceSelfInfo } from '@objectstack/spec/api';
@@ -448,10 +448,37 @@ export class HttpDispatcher {
448448
};
449449
}
450450

451+
/**
452+
* The single construction point for every error response this dispatcher
453+
* RETURNS. Distinct from `dispatcher-plugin`'s `errorResponseBase`, which
454+
* covers the errors that are THROWN out of `dispatch()` — a returned
455+
* `{handled: true, response}` goes to `sendResult`, never through that
456+
* catch. #3867 sanitised the thrown path; this is the returned one, and it
457+
* needs the same guard for the same reason.
458+
*
459+
* Reachable with a raw driver/engine message today via
460+
* {@link errorFromThrown} (`/meta` save, `/packages` install) and the MCP
461+
* transport's `deps.error(err?.message, 500)` — any of which can be
462+
* carrying a SQL dump naming physical tables and columns.
463+
*
464+
* Scoped to 5xx: a 4xx message is a deliberate business/validation answer
465+
* (`Path must be /actions/:object/:action`, a hook's own `throw`, a
466+
* `saveMetaItem` field error) and must reach the caller intact. `details`
467+
* is left alone — it carries structured `code`/`issues` the UI maps to
468+
* fields, never free-form driver prose.
469+
*
470+
* The unsanitised error is not lost: callers that THREW still hand the
471+
* original to `errorReporter` via `__obsRecordedError`, and every 5xx is
472+
* logged server-side.
473+
*/
451474
private error(message: string, code: number = 500, details?: any) {
475+
const safe =
476+
code >= 500 && looksLikeInternalErrorLeak(message)
477+
? INTERNAL_ERROR_MESSAGE
478+
: message;
452479
return {
453480
status: code,
454-
body: { success: false, error: { message, code, details } }
481+
body: { success: false, error: { message: safe, code, details } }
455482
};
456483
}
457484

0 commit comments

Comments
 (0)