|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * #3913 follow-up — an action that CRASHED is not an action that REJECTED. |
| 5 | + * |
| 6 | + * #3937 settled that a failed action reports in the payload at HTTP 200: "an |
| 7 | + * action that fails is a normal outcome, not a transport error". That is a |
| 8 | + * statement about the action **rejecting** — a business rule saying no. It was |
| 9 | + * also covering a third case it never argued for: a `TypeError` in a handler, |
| 10 | + * a driver blowing up, a sandbox timeout. Those are not outcomes the action |
| 11 | + * chose to report; they are the server failing to produce one, and serving |
| 12 | + * them as 200 hid every handler crash from gateway error rates, retry policy, |
| 13 | + * APM auto-capture and alerting — the platform had no signal for "customer |
| 14 | + * action bodies are throwing" short of body-parsing at every hop. |
| 15 | + * |
| 16 | + * The discriminator is the error's NAME, the same signal `@objectstack/rest` |
| 17 | + * already uses on this distinction ("non-default names (`TypeError: …`) […] |
| 18 | + * signal a genuine script bug rather than a deliberately thrown business |
| 19 | + * rule"). This file pins BOTH sides, because the whole risk of the change is |
| 20 | + * over-reach: everything #3937 put in the payload must stay there. |
| 21 | + */ |
| 22 | + |
| 23 | +import { describe, it, expect, vi } from 'vitest'; |
| 24 | + |
| 25 | +import { HttpDispatcher } from '../http-dispatcher.js'; |
| 26 | +import { SandboxError } from '../sandbox/quickjs-runner.js'; |
| 27 | + |
| 28 | +const scriptAction = { |
| 29 | + name: 'submit_signoff', |
| 30 | + objectName: 'crm_invoice', |
| 31 | + type: 'script', |
| 32 | + body: { language: 'js', source: 'return 1;', capabilities: ['api.write'] }, |
| 33 | +}; |
| 34 | + |
| 35 | +/** A dispatcher whose action handler rejects with `thrown`. */ |
| 36 | +function makeDispatcher(thrown: unknown) { |
| 37 | + const objectDef = { name: 'crm_invoice', actions: [scriptAction] }; |
| 38 | + const ql: any = { |
| 39 | + executeAction: vi.fn(async () => { throw thrown; }), |
| 40 | + getSchema: (n: string) => (n === objectDef.name ? objectDef : undefined), |
| 41 | + registry: { getObject: (n: string) => (n === objectDef.name ? objectDef : undefined), getItem: () => undefined }, |
| 42 | + find: vi.fn(async () => [{ id: 'inv_1', status: 'draft' }]), |
| 43 | + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), |
| 44 | + }; |
| 45 | + const metadata: any = { |
| 46 | + load: vi.fn(async () => null), |
| 47 | + listObjects: vi.fn(async () => [objectDef]), |
| 48 | + getObject: vi.fn(async () => objectDef), |
| 49 | + }; |
| 50 | + const kernel: any = { |
| 51 | + context: { |
| 52 | + getService: (n: string) => |
| 53 | + n === 'objectql' || n === 'data' ? ql : n === 'metadata' ? metadata : null, |
| 54 | + }, |
| 55 | + }; |
| 56 | + return new HttpDispatcher(kernel); |
| 57 | +} |
| 58 | + |
| 59 | +async function invoke(thrown: unknown) { |
| 60 | + const res: any = await makeDispatcher(thrown).handleActions( |
| 61 | + '/crm_invoice/submit_signoff/inv_1', |
| 62 | + 'POST', |
| 63 | + {}, |
| 64 | + { request: {}, environmentId: 'platform', executionContext: { userId: 'u1', systemPermissions: [] } } as any, |
| 65 | + ); |
| 66 | + return res.response; |
| 67 | +} |
| 68 | + |
| 69 | +describe('a deliberate REJECTION keeps the payload at HTTP 200', () => { |
| 70 | + it('a plain `throw new Error(msg)` from a registered handler', async () => { |
| 71 | + // The shape user code registered via `engine.registerAction` uses to |
| 72 | + // reject. `name === 'Error'` is what marks it deliberate — this is the |
| 73 | + // case a naive "no sandbox marker ⇒ fault" rule would have broken. |
| 74 | + const response = await invoke(new Error('Lead is already converted')); |
| 75 | + |
| 76 | + expect(response.status).toBe(200); |
| 77 | + expect(response.body.data).toEqual({ success: false, error: 'Lead is already converted' }); |
| 78 | + }); |
| 79 | + |
| 80 | + it('a sandboxed body that threw on purpose (SandboxError.innerMessage)', async () => { |
| 81 | + const response = await invoke( |
| 82 | + new SandboxError("action 'submit_signoff' threw: Contact has no phone", 'Contact has no phone'), |
| 83 | + ); |
| 84 | + |
| 85 | + expect(response.status).toBe(200); |
| 86 | + expect(response.body.data).toEqual({ success: false, error: 'Contact has no phone' }); |
| 87 | + }); |
| 88 | + |
| 89 | + it('a record validation failure, code + fields intact (#3937)', async () => { |
| 90 | + const fields = [{ field: 'issued_on', code: 'required', message: 'issued_on is required' }]; |
| 91 | + const response = await invoke( |
| 92 | + new SandboxError( |
| 93 | + "action 'submit_signoff' threw: ValidationError: issued_on is required", |
| 94 | + 'ValidationError: issued_on is required', |
| 95 | + { code: 'VALIDATION_FAILED', fields }, |
| 96 | + ), |
| 97 | + ); |
| 98 | + |
| 99 | + expect(response.status).toBe(200); |
| 100 | + expect(response.body.data).toMatchObject({ success: false, code: 'VALIDATION_FAILED', fields }); |
| 101 | + }); |
| 102 | + |
| 103 | + it('a bare ValidationError whose fields were stripped in transit', async () => { |
| 104 | + // Recognised by NAME via `validationFailureDetails` — the same predicate |
| 105 | + // the dispatcher's error exits use — so losing `fields` downgrades the |
| 106 | + // detail, never the classification. |
| 107 | + const err: any = new Error('issued_on is required'); |
| 108 | + err.name = 'ValidationError'; |
| 109 | + const response = await invoke(err); |
| 110 | + |
| 111 | + expect(response.status).toBe(200); |
| 112 | + expect(response.body.data.success).toBe(false); |
| 113 | + }); |
| 114 | + |
| 115 | + it('a flow that ran and rejected', async () => { |
| 116 | + // `dispatchFlowAction` throws a plain Error, so it lands on the |
| 117 | + // deliberate side with no special-casing. |
| 118 | + const response = await invoke(new Error("Flow 'convert_wizard' failed: lead already converted")); |
| 119 | + |
| 120 | + expect(response.status).toBe(200); |
| 121 | + expect(response.body.data.success).toBe(false); |
| 122 | + expect(response.body.data.error).toMatch(/lead already converted/); |
| 123 | + }); |
| 124 | + |
| 125 | + it('an unrecognisable throw — no name at all — keeps the status quo', async () => { |
| 126 | + // The change only moves what it is SURE about. A thrown string or a |
| 127 | + // bare object is not confidently a fault, so it keeps its 200. |
| 128 | + const response = await invoke('something odd'); |
| 129 | + |
| 130 | + expect(response.status).toBe(200); |
| 131 | + expect(response.body.data.success).toBe(false); |
| 132 | + }); |
| 133 | +}); |
| 134 | + |
| 135 | +describe('an unexpected FAULT is a 500', () => { |
| 136 | + it('a TypeError from a buggy handler', async () => { |
| 137 | + const response = await invoke(new TypeError("Cannot read properties of undefined (reading 'id')")); |
| 138 | + |
| 139 | + expect(response.status).toBe(500); |
| 140 | + expect(response.body.success).toBe(false); |
| 141 | + // No `{success:true, data:{...}}` wrapper — this is the dispatcher's |
| 142 | + // error exit, so monitoring sees a 5xx. |
| 143 | + expect(response.body.data).toBeUndefined(); |
| 144 | + }); |
| 145 | + |
| 146 | + it('a ReferenceError from a buggy handler', async () => { |
| 147 | + const response = await invoke(new ReferenceError('x is not defined')); |
| 148 | + |
| 149 | + expect(response.status).toBe(500); |
| 150 | + }); |
| 151 | + |
| 152 | + it("a driver's own error class", async () => { |
| 153 | + const err: any = new Error('no such table: crm_invoice'); |
| 154 | + err.name = 'SqliteError'; |
| 155 | + const response = await invoke(err); |
| 156 | + |
| 157 | + expect(response.status).toBe(500); |
| 158 | + }); |
| 159 | + |
| 160 | + it('a driver dump the leak heuristic recognises is sanitised', async () => { |
| 161 | + // Reaching the 5xx exit also puts these messages behind |
| 162 | + // `looksLikeInternalErrorLeak` (#3867) — which the 200 payload never |
| 163 | + // consulted, so a driver dump used to reach the client verbatim. |
| 164 | + const err: any = new Error('UNIQUE constraint failed: crm_invoice.number'); |
| 165 | + err.name = 'SqliteError'; |
| 166 | + const response = await invoke(err); |
| 167 | + |
| 168 | + expect(response.status).toBe(500); |
| 169 | + expect(response.body.error.message).toBe('Internal server error'); |
| 170 | + }); |
| 171 | + |
| 172 | + it("the sandbox's OWN internal errors — a timeout, a capability denial", async () => { |
| 173 | + // A `SandboxError` with no `innerMessage` is the sandbox failing, not |
| 174 | + // user code rejecting: a timeout, a denied capability, a marshalling |
| 175 | + // failure. Precisely the class an operator wants to alert on, and |
| 176 | + // precisely what a 200 made invisible. |
| 177 | + const response = await invoke(new SandboxError('action timed out after 5000ms')); |
| 178 | + |
| 179 | + expect(response.status).toBe(500); |
| 180 | + }); |
| 181 | + |
| 182 | + it('still honours an error that carries its own status', async () => { |
| 183 | + // `errorFromThrown` reads `.status` first, so a hand-thrown 4xx is not |
| 184 | + // flattened into the 500 fallback. |
| 185 | + const err: any = new TypeError('Not allowed'); |
| 186 | + err.status = 403; |
| 187 | + err.code = 'FORBIDDEN'; |
| 188 | + const response = await invoke(err); |
| 189 | + |
| 190 | + expect(response.status).toBe(403); |
| 191 | + }); |
| 192 | + |
| 193 | + it('honours an explicit status even on an otherwise-deliberate error', async () => { |
| 194 | + // A plugin's `FORBIDDEN` is a plain Error carrying `status: 403` — the |
| 195 | + // "deliberate" side by name, but burying an explicit 403 in a 200 |
| 196 | + // payload would discard the one thing the thrower was unambiguous |
| 197 | + // about, so `.status` is checked first. |
| 198 | + const err: any = new Error('Record is outside your sharing scope'); |
| 199 | + err.status = 403; |
| 200 | + err.code = 'FORBIDDEN'; |
| 201 | + const response = await invoke(err); |
| 202 | + |
| 203 | + expect(response.status).toBe(403); |
| 204 | + expect(response.body.error.message).toBe('Record is outside your sharing scope'); |
| 205 | + expect(response.body.error.details?.code).toBe('FORBIDDEN'); |
| 206 | + }); |
| 207 | +}); |
0 commit comments