Skip to content

Commit 18d60ac

Browse files
fix(flow): validate and coerce Execute JavaScript step values against custom field types (#997)
* fix(flow): validate and coerce Execute JavaScript step values against custom field types Execute JavaScript's output is now validated against the target output custom field's declared type before being persisted, instead of always writing the raw returned value as text. Input custom fields passed into the sandbox are coerced to match their declared type (number/boolean) so JS arithmetic and comparisons behave numerically instead of as string concatenation. Shared normalizer logic is extracted into @chatbotx.io/business/javascript-execution and re-exported for the CSV import validator that used the same rules. * chore(worker): remove redundant custom-field-value re-export shim The import handler now imports validateCustomFieldValue directly from @chatbotx.io/business/javascript-execution instead of hopping through a one-line re-export left behind by the prior Execute JavaScript refactor.
1 parent 9c598ad commit 18d60ac

12 files changed

Lines changed: 901 additions & 51 deletions

File tree

apps/worker/__tests__/custom-field-value.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1+
import { validateCustomFieldValue } from "@chatbotx.io/business/javascript-execution"
12
import { describe, expect, it } from "vitest"
2-
import { validateCustomFieldValue } from "../src/default/handlers/imports/validations/custom-field-value"
33

44
describe("validateCustomFieldValue", () => {
55
describe("shortText / longText", () => {

apps/worker/__tests__/execute-javascript-handler.test.ts

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,38 @@ const mocks = vi.hoisted(() => ({
99
workspace: null,
1010
})),
1111
getSystemFieldValue: vi.fn(async () => null as string | null),
12-
resolveJavascriptInput: vi.fn(async () => new Map<string, string | null>()),
12+
resolveJavascriptInput: vi.fn(
13+
async () => new Map<string, string | number | boolean | null>(),
14+
),
1315
interpolateIntoJavascript: vi.fn((code: string) => code),
1416
executeAndMap: vi.fn(async () => ({ value: null })),
1517
}))
1618

19+
// Mirrors packages/variables/src/javascript-interpolation.ts's real
20+
// implementation exactly (a pure number/boolean coercion, no DB
21+
// dependency), since `@chatbotx.io/variables`'s barrel can't be imported
22+
// actual here without resurrecting the Snowflake-collision problem
23+
// documented at the bottom of this file.
24+
const coerceCustomFieldValueForJavascript = (
25+
value: string,
26+
type: string,
27+
): string | number | boolean | null => {
28+
if (type === "number") {
29+
const numberValue = Number(value)
30+
return Number.isFinite(numberValue) ? numberValue : null
31+
}
32+
if (type === "boolean") {
33+
return value === "true"
34+
}
35+
return value
36+
}
37+
1738
vi.mock("@chatbotx.io/variables", () => ({
1839
contactVariableService: { getAll: mocks.getAll },
1940
getSystemFieldValue: mocks.getSystemFieldValue,
2041
resolveJavascriptInput: mocks.resolveJavascriptInput,
2142
interpolateIntoJavascript: mocks.interpolateIntoJavascript,
43+
coerceCustomFieldValueForJavascript,
2244
}))
2345

2446
vi.mock("@chatbotx.io/business/javascript-execution", () => ({
@@ -138,6 +160,54 @@ describe("handleExecuteJavascript", () => {
138160
result: null,
139161
})
140162
})
163+
164+
test("routes an output type mismatch to the error result with its actionable message", async () => {
165+
// executeAndMap throws a ChatbotXException (which extends Error) when
166+
// the sandbox's result doesn't fit the output field's declared type —
167+
// this proves that failure reaches the step's error branch instead of
168+
// being swallowed or reported as a generic success.
169+
mocks.executeAndMap.mockRejectedValue(
170+
new Error(
171+
'JavaScript returned "Abcd 123", which is not a valid number value for the output field "Age".',
172+
),
173+
)
174+
175+
await expect(handleExecuteJavascript(createProps())).resolves.toEqual({
176+
status: "error",
177+
errorMessage:
178+
'JavaScript returned "Abcd 123", which is not a valid number value for the output field "Age".',
179+
result: null,
180+
})
181+
})
182+
183+
test("coerces every custom field seeded into input, not only ones referenced via {{...}}", async () => {
184+
// A field read directly as input.age (never as {{age}}) must still be
185+
// typed consistently with one read via {{age}} — both come from the
186+
// same customFieldsMap, so both must go through the same coercion.
187+
mocks.getAll.mockResolvedValue({
188+
contact: { id: "contact-1", email: "a@example.com" },
189+
contactInbox: null,
190+
conversation: null,
191+
customFieldsMap: new Map([
192+
["age", { key: "age", type: "number", value: "30", description: "" }],
193+
[
194+
"is_vip",
195+
{ key: "is_vip", type: "boolean", value: "true", description: "" },
196+
],
197+
]),
198+
workspace: null,
199+
})
200+
201+
const props = createProps()
202+
props.step.code = "return input.age + 1"
203+
await handleExecuteJavascript(props)
204+
205+
const call = mocks.executeAndMap.mock.calls[0]?.[0] as {
206+
input: Record<string, unknown>
207+
}
208+
expect(call.input.age).toBe(30)
209+
expect(call.input.is_vip).toBe(true)
210+
})
141211
})
142212

143213
// The suite above mocks @chatbotx.io/variables entirely, so it only pins the

apps/worker/src/default/handlers/imports/handler/contacts/handler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
workspaceService,
77
workspaceUsageService,
88
} from "@chatbotx.io/business"
9+
import { validateCustomFieldValue } from "@chatbotx.io/business/javascript-execution"
910
import { db, inArray } from "@chatbotx.io/database/client"
1011
import {
1112
type ContactImportMeta,
@@ -28,7 +29,6 @@ import type {
2829
ImportRow,
2930
ImportTypeHandler,
3031
} from "../../base-import"
31-
import { validateCustomFieldValue } from "../../validations/custom-field-value"
3232
import { type ContactRow, extractRowData } from "./extractor"
3333

3434
type ContactDeps = {

apps/worker/src/integration/handlers/tool-handler.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
SourceTimezoneStrategy,
2727
} from "@chatbotx.io/utils/datetime"
2828
import {
29+
coerceCustomFieldValueForJavascript,
2930
contactVariableService,
3031
extractVariables,
3132
getSystemFieldValue,
@@ -344,10 +345,14 @@ export async function handleExecuteJavascript({
344345
contactInbox,
345346
conversation,
346347
})
348+
// Coerced the same way resolveJavascriptInput coerces `{{name}}`
349+
// lookups below, so a custom field is typed consistently in `input`
350+
// regardless of whether the code reaches it via `input["name"]` or via
351+
// a `{{name}}` placeholder rewritten to that same property access.
347352
const input: Record<string, unknown> = Object.fromEntries(
348353
[...variables.customFieldsMap.entries()].map(([name, field]) => [
349354
name,
350-
field.value,
355+
coerceCustomFieldValueForJavascript(field.value, field.type),
351356
]),
352357
)
353358

Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
import { describe, expect, test } from "vitest"
2+
import type { ChatbotXException } from "../src/errors"
3+
import { toValidatedCustomFieldValue } from "../src/javascript-execution/output-value"
4+
5+
const validate = (props: Parameters<typeof toValidatedCustomFieldValue>[0]) =>
6+
toValidatedCustomFieldValue(props)
7+
8+
describe("toValidatedCustomFieldValue", () => {
9+
test("skips the write for null or undefined", () => {
10+
expect(
11+
validate({ value: null, type: "number", fieldName: "Age" }),
12+
).toBeNull()
13+
expect(
14+
validate({ value: undefined, type: "number", fieldName: "Age" }),
15+
).toBeNull()
16+
})
17+
18+
describe("number", () => {
19+
test("accepts a finite JS number", () => {
20+
expect(validate({ value: 42, type: "number", fieldName: "Age" })).toBe(
21+
"42",
22+
)
23+
})
24+
25+
test("accepts a numeric string", () => {
26+
expect(validate({ value: "42", type: "number", fieldName: "Age" })).toBe(
27+
"42",
28+
)
29+
})
30+
31+
test("rejects a non-numeric string", () => {
32+
expect(() =>
33+
validate({ value: "Abcd 123", type: "number", fieldName: "Age" }),
34+
).toThrowError(
35+
expect.objectContaining<Partial<ChatbotXException>>({
36+
code: "javascriptOutputTypeMismatch",
37+
}),
38+
)
39+
})
40+
41+
test("rejects NaN and Infinity instead of persisting the JSON.stringify null", () => {
42+
expect(() =>
43+
validate({
44+
value: Number.NaN,
45+
type: "number",
46+
fieldName: "Age",
47+
}),
48+
).toThrowError(
49+
expect.objectContaining<Partial<ChatbotXException>>({
50+
code: "javascriptOutputTypeMismatch",
51+
}),
52+
)
53+
expect(() =>
54+
validate({
55+
value: Number.POSITIVE_INFINITY,
56+
type: "number",
57+
fieldName: "Age",
58+
}),
59+
).toThrowError(
60+
expect.objectContaining<Partial<ChatbotXException>>({
61+
code: "javascriptOutputTypeMismatch",
62+
}),
63+
)
64+
})
65+
66+
test("rejects an object", () => {
67+
expect(() =>
68+
validate({ value: { a: 1 }, type: "number", fieldName: "Age" }),
69+
).toThrowError(
70+
expect.objectContaining<Partial<ChatbotXException>>({
71+
code: "javascriptOutputTypeMismatch",
72+
}),
73+
)
74+
})
75+
})
76+
77+
describe("boolean", () => {
78+
test("accepts a JS boolean", () => {
79+
expect(
80+
validate({ value: true, type: "boolean", fieldName: "Active" }),
81+
).toBe("true")
82+
})
83+
84+
test("accepts canonical string forms", () => {
85+
expect(
86+
validate({ value: "1", type: "boolean", fieldName: "Active" }),
87+
).toBe("true")
88+
expect(
89+
validate({ value: "0", type: "boolean", fieldName: "Active" }),
90+
).toBe("false")
91+
})
92+
93+
test("rejects a non-boolean-like string", () => {
94+
expect(() =>
95+
validate({ value: "garbage", type: "boolean", fieldName: "Active" }),
96+
).toThrowError(
97+
expect.objectContaining<Partial<ChatbotXException>>({
98+
code: "javascriptOutputTypeMismatch",
99+
}),
100+
)
101+
})
102+
})
103+
104+
describe("email", () => {
105+
test("lowercases a valid email", () => {
106+
expect(
107+
validate({
108+
value: "Foo@Bar.COM",
109+
type: "email",
110+
fieldName: "Email",
111+
}),
112+
).toBe("foo@bar.com")
113+
})
114+
115+
test("rejects a non-string value", () => {
116+
expect(() =>
117+
validate({ value: 42, type: "email", fieldName: "Email" }),
118+
).toThrowError(
119+
expect.objectContaining<Partial<ChatbotXException>>({
120+
code: "javascriptOutputTypeMismatch",
121+
}),
122+
)
123+
})
124+
})
125+
126+
describe("phoneNumber", () => {
127+
test("strips formatting, preserves +", () => {
128+
expect(
129+
validate({
130+
value: "+1 (555) 123-4567",
131+
type: "phoneNumber",
132+
fieldName: "Phone",
133+
}),
134+
).toBe("+15551234567")
135+
})
136+
})
137+
138+
describe("date / datetime", () => {
139+
test("hands a parseable value through raw", () => {
140+
expect(
141+
validate({
142+
value: "2026-07-22T10:00:00Z",
143+
type: "datetime",
144+
fieldName: "Signed up",
145+
}),
146+
).toBe("2026-07-22T10:00:00Z")
147+
})
148+
149+
test("rejects an unparseable value", () => {
150+
expect(() =>
151+
validate({
152+
value: "not a date",
153+
type: "datetime",
154+
fieldName: "Signed up",
155+
}),
156+
).toThrowError(
157+
expect.objectContaining<Partial<ChatbotXException>>({
158+
code: "javascriptOutputTypeMismatch",
159+
}),
160+
)
161+
})
162+
})
163+
164+
describe("shortText / longText", () => {
165+
test("stringifies objects and arrays unchanged", () => {
166+
expect(
167+
validate({
168+
value: { a: 1 },
169+
type: "shortText",
170+
fieldName: "Note",
171+
}),
172+
).toBe(JSON.stringify({ a: 1 }))
173+
})
174+
175+
test("accepts an empty string", () => {
176+
expect(
177+
validate({ value: "", type: "shortText", fieldName: "Note" }),
178+
).toBe("")
179+
})
180+
})
181+
182+
test("throws for an empty string into a non-text field", () => {
183+
expect(() =>
184+
validate({ value: "", type: "number", fieldName: "Age" }),
185+
).toThrowError(
186+
expect.objectContaining<Partial<ChatbotXException>>({
187+
code: "javascriptOutputTypeMismatch",
188+
}),
189+
)
190+
})
191+
192+
test("throws javascriptOutputValueTooLarge before the type check", () => {
193+
expect(() =>
194+
validate({
195+
value: "a".repeat(64 * 1024 + 1),
196+
type: "number",
197+
fieldName: "Age",
198+
}),
199+
).toThrowError(
200+
expect.objectContaining<Partial<ChatbotXException>>({
201+
code: "javascriptOutputValueTooLarge",
202+
}),
203+
)
204+
})
205+
})

0 commit comments

Comments
 (0)