Skip to content

Commit f30370c

Browse files
committed
fix(runtime): carry code/fields across the sandbox boundary so form actions can anchor validation errors (#3918 follow-up)
Found by dogfooding the merged #3918 chain against a running app. Submitting a record that fails validation through a form ACTION came back as HTTP 200 { success: true, data: { success: false, error: "ValidationError: issued_on is required" } } — no status to branch on, no code, no fields[]. The chain's dispatcher fixes could not help: the field list was already gone before any dispatcher exit ran. It was lost at the QuickJS boundary, twice: 1. host → VM: `vm.newError({name, message})` dropped every other property, so a body reaching a record ValidationError through `ctx.api.object(x).update(…)` saw bare prose. 2. VM → host: the wrapper's reject handler flattened the error to the string `<name>: <message>` before the host ever saw it. Both hops now carry an explicit ALLOWLIST — `code` and `fields` — alongside the message, and SandboxError exposes them as `.code` / `.fields`. The allowlist is a security boundary, not a style choice: host errors routinely hang driver state, connection details or whole record payloads off themselves, and anything crossing INTO the VM is readable by untrusted sandboxed code. `/actions` then surfaces them so a form can highlight the offending input. Its wire contract is deliberately UNCHANGED: status stays 200 and `success: false` remains the failure signal — that route has always reported business failure in the payload and every caller branches on `data.success`, so `code`/`fields` are purely additive and omitted when absent. Message channels are byte-identical: `.message` keeps the debug wrapper for server logs, `.innerMessage` stays the toast text. Also adds dispatcher-validation-error.real.test.ts, pinning both dispatcher exits against the REAL objectql ValidationError rather than a fixture — including its deliberate absence of `.status`, the assumption the whole #3918 fix rests on. Covered by 19 new cases across sandbox, /actions and the dispatcher exits; 7 of them fail on the pre-fix source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LeZ7DmSKjiKmQzYh8L5CJS
1 parent cbc08eb commit f30370c

6 files changed

Lines changed: 634 additions & 14 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
"@objectstack/runtime": patch
3+
---
4+
5+
fix(runtime): carry `code` / `fields[]` across the sandbox boundary so form actions can anchor validation errors (#3918 follow-up)
6+
7+
Found by dogfooding the merged #3918 chain against a running app. Submitting a
8+
record that fails validation through a form **action** came back as:
9+
10+
```
11+
HTTP 200
12+
{ "success": true, "data": { "success": false,
13+
"error": "ValidationError: issued_on is required" } }
14+
```
15+
16+
No status a client could branch on, no code, no `fields[]`. The chain's
17+
dispatcher fixes could not help: the field list was already gone before any
18+
dispatcher exit ran. It was lost at the QuickJS boundary, twice —
19+
20+
1. **host → VM.** `vm.newError({ name, message })` dropped every other property,
21+
so a body reaching a record `ValidationError` through
22+
`ctx.api.object(x).update(...)` saw bare prose.
23+
2. **VM → host.** The wrapper's reject handler flattened the error to the string
24+
`<name>: <message>` before the host ever saw it.
25+
26+
Both hops now carry an explicit **allowlist**`code` and `fields` — alongside
27+
the message, and `SandboxError` exposes them as `.code` / `.fields`. The
28+
allowlist is a security boundary, not a style choice: host errors routinely hang
29+
driver state, connection details or whole record payloads off themselves, and
30+
anything crossing INTO the VM is readable by untrusted sandboxed code. Copying
31+
the error's own enumerable keys would leak all of it.
32+
33+
`/actions` then surfaces them, so a form can highlight the offending input:
34+
35+
```
36+
HTTP 200
37+
{ "success": true, "data": { "success": false,
38+
"error": "ValidationError: issued_on is required",
39+
"code": "VALIDATION_FAILED",
40+
"fields": [ { "field": "issued_on", "code": "required", … } ] } }
41+
```
42+
43+
**The `/actions` wire contract is deliberately unchanged.** The status stays
44+
200 and `success: false` remains the failure signal: that route has always
45+
reported business failure in the payload (an action that "fails" is a normal
46+
outcome, not a transport error) and every caller branches on `data.success`.
47+
Making it a 4xx would be a break in exchange for a strictly additive fix, so the
48+
fix is additive — `code` and `fields` are simply omitted when absent, and a
49+
caller that ignores them sees exactly what it saw before.
50+
51+
Message channels are byte-identical: `SandboxError.message` keeps the
52+
`<kind> '<name>' threw:` debug wrapper for server logs and `.innerMessage` stays
53+
the plain business text a toast shows. The structured payload rides alongside
54+
them, never instead of them.
55+
56+
Also adds `dispatcher-validation-error.real.test.ts`, which pins both dispatcher
57+
exits against the **real** objectql `ValidationError` rather than a hand-built
58+
fixture — including its deliberate absence of `.status`, the assumption the
59+
whole #3918 fix rests on. The existing fixture-based tests restate that contract;
60+
these check it, so a future change to the class fails a test instead of quietly
61+
regressing production.
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #3918 follow-up — pin both dispatcher exits against the REAL `ValidationError`.
5+
*
6+
* `dispatcher-validation-error.test.ts` drives the exits with a hand-built
7+
* fixture: an `Error` with `name = 'ValidationError'`, `code =
8+
* 'VALIDATION_FAILED'` and a `fields[]`. That is deliberate — it proves the
9+
* duck-typing predicate accepts the SHAPE, including for hand-thrown errors
10+
* that never touched objectql.
11+
*
12+
* What it cannot prove is that the shape it asserts is still the shape objectql
13+
* actually throws. The whole fix rests on three facts about that class —
14+
* `.code`, `.fields[]`, and the deliberate ABSENCE of `.status` — and a fixture
15+
* restates those facts rather than checking them. Give `ValidationError` a
16+
* `.status` one day, or rename `.fields`, and every fixture-based test here
17+
* keeps passing while production quietly regresses.
18+
*
19+
* So these construct the genuine article and send it through both exits. They
20+
* are the tests that fail if the contract moves.
21+
*/
22+
23+
import { describe, it, expect } from 'vitest';
24+
import { ValidationError } from '@objectstack/objectql';
25+
26+
import { HttpDispatcher } from './http-dispatcher.js';
27+
import { createDispatcherPlugin } from './dispatcher-plugin.js';
28+
29+
const FIELDS = [
30+
{ field: 'email', code: 'invalid_email' as const, message: 'email must be a valid email address' },
31+
{ field: 'name', code: 'required' as const, message: 'name is required' },
32+
];
33+
34+
describe('#3918 — the real objectql ValidationError still has the shape the fix relies on', () => {
35+
const err = new ValidationError(FIELDS);
36+
37+
it('carries `code` and `fields[]`', () => {
38+
expect(err.code).toBe('VALIDATION_FAILED');
39+
expect(err.fields).toEqual(FIELDS);
40+
expect(err.name).toBe('ValidationError');
41+
});
42+
43+
it('carries NO `status` / `statusCode` — which is why the boundary must supply 400', () => {
44+
// If this ever fails, the dispatcher's `.status`-first precedence will
45+
// start winning over the 400 default and these exits change behaviour
46+
// silently. That is the assumption the whole fix is built on.
47+
expect((err as any).status).toBeUndefined();
48+
expect((err as any).statusCode).toBeUndefined();
49+
});
50+
51+
it('carries no `issues` — the property the old `details` builder read', () => {
52+
expect((err as any).issues).toBeUndefined();
53+
});
54+
});
55+
56+
// ---------------------------------------------------------------------------
57+
58+
/** Exit 1 — the RETURNED path, through a route whose fallback is 500. */
59+
async function publishDrafts(thrown: unknown) {
60+
const protocol = { publishPackageDrafts: async () => { throw thrown; } };
61+
const objectql = { registry: {} };
62+
const resolve = (name: string) =>
63+
name === 'protocol' ? protocol : name === 'objectql' ? objectql : undefined;
64+
const kernel: any = { getService: resolve, getServiceAsync: async (n: string) => resolve(n) };
65+
const result: any = await new HttpDispatcher(kernel).dispatch(
66+
'POST', '/packages/demo/publish-drafts', {}, {}, {} as any,
67+
);
68+
return result.response;
69+
}
70+
71+
/** Exit 2 — the THROWN path, through the plugin's real route handler. */
72+
async function analyticsQuery(thrown: unknown) {
73+
const handlers: Record<string, (req: any, res: any) => any> = {};
74+
const rec = (verb: string) => (path: string, h: any) => { handlers[`${verb} ${path}`] = h; };
75+
const server = {
76+
get: rec('GET'), post: rec('POST'), put: rec('PUT'),
77+
delete: rec('DELETE'), patch: rec('PATCH'),
78+
};
79+
const analytics = {
80+
query: async () => { throw thrown; },
81+
getMeta: async () => ({ cubes: [] }),
82+
generateSql: async () => ({ sql: null }),
83+
};
84+
const kernel = {
85+
getService: (n: string) => (n === 'analytics' ? analytics : undefined),
86+
getServiceAsync: async (n: string) => (n === 'analytics' ? analytics : undefined),
87+
};
88+
const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false });
89+
await plugin.start?.({
90+
getKernel: () => kernel,
91+
getService: (n: string) => (n === 'http.server' ? server : undefined),
92+
environmentId: undefined,
93+
logger: { info() {}, warn() {}, error() {}, debug() {} },
94+
hook: () => {}, on: () => {},
95+
} as any);
96+
97+
const res: any = {
98+
statusCode: undefined, body: undefined,
99+
status(c: number) { res.statusCode = c; return res; },
100+
header() { return res; },
101+
json(b: any) { res.body = b; return res; },
102+
};
103+
await handlers['POST /api/v1/analytics/query']({ body: { cube: 'x', query: {} }, query: {} }, res);
104+
return res;
105+
}
106+
107+
describe('#3918 — both exits serve the real ValidationError as 400 + fields[]', () => {
108+
it('errorFromThrown (returned path)', async () => {
109+
const res = await publishDrafts(new ValidationError(FIELDS));
110+
111+
expect(res.status).toBe(400);
112+
expect(res.body.error.details).toEqual({ code: 'VALIDATION_FAILED', fields: FIELDS });
113+
// The class builds its message from the human field messages — that is
114+
// what a toast shows, so it must survive intact rather than being
115+
// replaced by the 5xx sanitiser.
116+
expect(res.body.error.message).toContain('email must be a valid email address');
117+
});
118+
119+
it('errorResponseBase (thrown path)', async () => {
120+
const res = await analyticsQuery(new ValidationError(FIELDS));
121+
122+
expect(res.statusCode).toBe(400);
123+
expect(res.body.error.details).toEqual({ code: 'VALIDATION_FAILED', fields: FIELDS });
124+
expect(res.body.error.message).toContain('name is required');
125+
// Not a server fault: the errorReporter side-channel stays clear.
126+
expect(res.__obsRecordedError).toBeUndefined();
127+
});
128+
129+
it('an empty-fields ValidationError still answers 400, not 500', async () => {
130+
// The class defaults its message to 'Validation failed' for an empty
131+
// list; the status must not depend on the list being non-empty.
132+
const res = await publishDrafts(new ValidationError([]));
133+
134+
expect(res.status).toBe(400);
135+
expect(res.body.error.details.fields).toEqual([]);
136+
});
137+
});
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #3918 follow-up — the LAST hop: what `/actions` puts on the wire.
5+
*
6+
* The sandbox now carries `code` / `fields` back out of the VM on a
7+
* `SandboxError` (see `sandbox/error-passthrough.test.ts`). This pins the other
8+
* half: the actions route surfacing them, which is what a form actually reads.
9+
*
10+
* Before, this envelope was the message string and nothing else, so a form
11+
* action could raise a toast but never highlight the offending input — the
12+
* symptom in the original report.
13+
*
14+
* **The status stays 200.** `/actions` has always reported business failure in
15+
* the payload (`{ success: false }`), not as a transport error, and every
16+
* caller branches on `data.success`. Making this a 4xx would be a wire-contract
17+
* break in exchange for a strictly additive fix, so the fix is additive: the
18+
* same envelope, now carrying what the client needs. That choice is asserted
19+
* below so it cannot drift silently.
20+
*/
21+
22+
import { describe, it, expect, vi } from 'vitest';
23+
24+
import { HttpDispatcher } from '../http-dispatcher.js';
25+
import { SandboxError } from '../sandbox/quickjs-runner.js';
26+
27+
const FIELDS = [
28+
{ field: 'issued_on', code: 'required', message: 'issued_on is required' },
29+
];
30+
31+
const scriptAction = {
32+
name: 'submit_signoff',
33+
objectName: 'crm_invoice',
34+
type: 'script',
35+
body: { language: 'js', source: 'return 1;', capabilities: ['api.write'] },
36+
};
37+
38+
/** A dispatcher whose action handler rejects with `thrown`. */
39+
function makeDispatcher(thrown: unknown) {
40+
const objectDef = { name: 'crm_invoice', actions: [scriptAction] };
41+
const ql: any = {
42+
executeAction: vi.fn(async () => { throw thrown; }),
43+
getSchema: (n: string) => (n === objectDef.name ? objectDef : undefined),
44+
registry: { getObject: (n: string) => (n === objectDef.name ? objectDef : undefined), getItem: () => undefined },
45+
find: vi.fn(async () => [{ id: 'inv_1', status: 'draft' }]),
46+
insert: vi.fn(), update: vi.fn(), delete: vi.fn(),
47+
};
48+
const metadata: any = {
49+
load: vi.fn(async () => null),
50+
listObjects: vi.fn(async () => [objectDef]),
51+
getObject: vi.fn(async () => objectDef),
52+
};
53+
const kernel: any = {
54+
context: {
55+
getService: (n: string) =>
56+
n === 'objectql' || n === 'data' ? ql : n === 'metadata' ? metadata : null,
57+
},
58+
};
59+
return new HttpDispatcher(kernel);
60+
}
61+
62+
async function invoke(thrown: unknown) {
63+
const res: any = await makeDispatcher(thrown).handleActions(
64+
'/crm_invoice/submit_signoff/inv_1',
65+
'POST',
66+
{},
67+
{ request: {}, environmentId: 'platform', executionContext: { userId: 'u1', systemPermissions: [] } } as any,
68+
);
69+
return res.response;
70+
}
71+
72+
/** What the sandbox now throws for a record validation failure. */
73+
const validationSandboxError = () =>
74+
new SandboxError(
75+
"action 'submit_signoff' threw: ValidationError: issued_on is required",
76+
'ValidationError: issued_on is required',
77+
{ code: 'VALIDATION_FAILED', fields: FIELDS },
78+
);
79+
80+
describe('#3918 follow-up — /actions surfaces code + fields on a validation failure', () => {
81+
it('carries `fields[]` so a form can anchor the error to the input', async () => {
82+
const response = await invoke(validationSandboxError());
83+
84+
expect(response.body.data).toMatchObject({
85+
success: false,
86+
code: 'VALIDATION_FAILED',
87+
fields: FIELDS,
88+
});
89+
});
90+
91+
it('keeps the human message as the toast text', async () => {
92+
const response = await invoke(validationSandboxError());
93+
94+
expect(response.body.data.error).toBe('ValidationError: issued_on is required');
95+
});
96+
97+
it('KEEPS HTTP 200 and `success: false` — the wire contract is unchanged', async () => {
98+
// Deliberate: see the file header. If this flips to 4xx it must be a
99+
// conscious, documented break, not a side effect.
100+
const response = await invoke(validationSandboxError());
101+
102+
expect(response.status).toBe(200);
103+
expect(response.body.success).toBe(true); // dispatcher envelope
104+
expect(response.body.data.success).toBe(false); // business outcome
105+
});
106+
107+
it('omits `code` / `fields` entirely for an ordinary failure', async () => {
108+
// Callers branch on presence; emitting empty values would claim a
109+
// field-anchored failure that never happened.
110+
const response = await invoke(
111+
new SandboxError("action 'x' threw: boom", 'boom'),
112+
);
113+
114+
expect(response.body.data).toEqual({ success: false, error: 'boom' });
115+
expect('code' in response.body.data).toBe(false);
116+
expect('fields' in response.body.data).toBe(false);
117+
});
118+
119+
it('carries a code without fields when that is all the error had', async () => {
120+
const response = await invoke(
121+
new SandboxError("action 'x' threw: pick another", 'pick another', { code: 'DUPLICATE' }),
122+
);
123+
124+
expect(response.body.data).toEqual({
125+
success: false, error: 'pick another', code: 'DUPLICATE',
126+
});
127+
});
128+
});

packages/runtime/src/domains/actions.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,29 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string
272272
const inner: unknown = err?.innerMessage;
273273
const clientMsg = (typeof inner === 'string' && inner) ? inner : full;
274274
if (clientMsg !== full) console.error(`[action ${objectName}/${actionName}] ${full}`);
275-
return { handled: true, response: deps.success({ success: false, error: clientMsg }) };
275+
// [#3918 follow-up] Carry the structured payload when the failure was a
276+
// record validation error. Until the sandbox learned to pass `code` /
277+
// `fields` back OUT of the VM, this envelope was the message string and
278+
// nothing else — so a form action could only ever raise a toast, never
279+
// highlight the field the user actually got wrong, which is the symptom
280+
// the original report was about.
281+
//
282+
// The HTTP status stays 200 and `success: false` remains the failure
283+
// signal. `/actions` has always reported business failure in the payload
284+
// (an action that "fails" is a normal outcome, not a transport error) and
285+
// every existing caller branches on `data.success`; turning this into a
286+
// 4xx would be a wire-contract break in exchange for a strictly additive
287+
// fix.
288+
const code: unknown = err?.code;
289+
const fields: unknown = err?.fields;
290+
return {
291+
handled: true,
292+
response: deps.success({
293+
success: false,
294+
error: clientMsg,
295+
...(typeof code === 'string' && code ? { code } : {}),
296+
...(Array.isArray(fields) ? { fields } : {}),
297+
}),
298+
};
276299
}
277300
}

0 commit comments

Comments
 (0)