From e9a00dab03d77109f0c4a66f81a3c13ec1d37fb5 Mon Sep 17 00:00:00 2001 From: Josue Hernandez Date: Mon, 12 Oct 2026 17:49:26 -0700 Subject: [PATCH 1/5] feat(api): list every validation issue with its dot path A 400 from the form, branding and destinations routes now carries `issues: [{ path, message, code }]` with every zod issue, `path` being the dot path inside the request body (`config.steps.2.question`). `message` stays the first issue's, so a client that reads only `message` sees no change. One shared `parse` replaces the three copies that kept only the first issue. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Josue Hernandez --- apps/api/src/admin-crud.controller.ts | 12 +- apps/api/src/branding.controller.ts | 14 +-- apps/api/src/integrations.controller.ts | 10 +- apps/api/src/strict-validation.spec.ts | 161 ++++++++++++++++++++++++ apps/api/src/validation.ts | 75 +++++++++++ 5 files changed, 241 insertions(+), 31 deletions(-) create mode 100644 apps/api/src/strict-validation.spec.ts create mode 100644 apps/api/src/validation.ts diff --git a/apps/api/src/admin-crud.controller.ts b/apps/api/src/admin-crud.controller.ts index 5ffa11a7..c64059b1 100644 --- a/apps/api/src/admin-crud.controller.ts +++ b/apps/api/src/admin-crud.controller.ts @@ -88,7 +88,6 @@ import { workspaceTimezoneSchema, type FormConfig, } from '@quill/types'; -import { ZodError } from 'zod'; import { AdminService } from './admin.service'; import { SubmissionService } from './submission.service'; import { AnalyticsService } from './analytics.service'; @@ -98,6 +97,7 @@ import { EmailEffects } from './email-effects'; import { AnalyticsEffects } from './analytics-effects'; import { UploadService } from './upload.service'; import { unwrap } from './http'; +import { parse } from './validation'; import type { ServerEnv } from '@quill/config/env'; import { assertAdmin, assertCanManageTarget, assertNotSelf, assertOwner } from './permissions'; import { parseIntParam, parseKinds, parseOutboxStatuses } from './query-params'; @@ -105,16 +105,6 @@ import { parseSort, parseSubmissionFilter, workspaceZone, type FilterQuery } fro import type { CaptchaVerifier } from './captcha'; import { CAPTCHA, DB, ENV } from './tokens'; -function parse(schema: { parse: (v: unknown) => T }, body: unknown): T { - try { - return schema.parse(body); - } catch (err) { - if (err instanceof ZodError) - throw new BadRequestException({ error: 'BAD_REQUEST', message: err.issues[0]?.message }); - throw err; - } -} - function unwrapCrud(r: CrudResult): T { if (r.ok) return r.value; if (r.reason === 'NOT_FOUND') throw new NotFoundException({ error: 'NOT_FOUND', message: 'Not found.' }); diff --git a/apps/api/src/branding.controller.ts b/apps/api/src/branding.controller.ts index 2d53eae0..aab3e491 100644 --- a/apps/api/src/branding.controller.ts +++ b/apps/api/src/branding.controller.ts @@ -1,5 +1,4 @@ import { - BadRequestException, Body, Controller, Get, @@ -16,21 +15,12 @@ import { upsertAccountBranding, } from '@quill/db'; import { brandKitSchema } from '@quill/types'; -import { z, ZodError } from 'zod'; +import { z } from 'zod'; import { AuthService, type ReqLike } from './auth.service'; import { assertAdmin } from './permissions'; +import { parse } from './validation'; import { DB } from './tokens'; -function parse(schema: { parse: (v: unknown) => T }, body: unknown): T { - try { - return schema.parse(body); - } catch (err) { - if (err instanceof ZodError) - throw new BadRequestException({ error: 'BAD_REQUEST', message: err.issues[0]?.message }); - throw err; - } -} - const formIdsSchema = z.object({ formIds: z.array(z.string().min(1)).min(1).max(500) }); /** diff --git a/apps/api/src/integrations.controller.ts b/apps/api/src/integrations.controller.ts index e2ad3f6a..1756744a 100644 --- a/apps/api/src/integrations.controller.ts +++ b/apps/api/src/integrations.controller.ts @@ -46,6 +46,7 @@ import { syncMirrorForm, type MirrorSyncResult } from './hubspot-mirror'; import type { ServerEnv } from '@quill/config/env'; import { AuthService, type ReqLike } from './auth.service'; import { assertAdmin } from './permissions'; +import { parse } from './validation'; import { RateLimitGuard } from './rate-limit'; import { captchaActive, type CaptchaVerifier } from './captcha'; import { fileLinksFor } from './file-link'; @@ -752,14 +753,7 @@ export class FormDestinationsController { @Put('forms/:id/destinations') async putDestinations(@Req() req: ReqLike, @Param('id') id: string, @Body() body: unknown) { const p = await this.auth.resolveHost(req); - let destinations: unknown[]; - try { - destinations = destinationsBodySchema.parse(body).destinations; - } catch (err) { - if (err instanceof ZodError) - throw new BadRequestException({ error: 'BAD_REQUEST', message: err.issues[0]?.message }); - throw err; - } + const { destinations } = parse(destinationsBodySchema, body); // At most one HubSpot destination — see `hasExtraHubspotDestination`. Not in // the schema above, so a form that already stores two still READS fine. // Compared against what is STORED, not against 1: the builder's per-question diff --git a/apps/api/src/strict-validation.spec.ts b/apps/api/src/strict-validation.spec.ts new file mode 100644 index 00000000..d2ddd8f0 --- /dev/null +++ b/apps/api/src/strict-validation.spec.ts @@ -0,0 +1,161 @@ +/** + * API v1 validation, end to end through the real controllers on in-memory + * SQLite (same harness as draft-publish.spec.ts): + * + * 1. every validation 400 of the form, branding and destinations routes + * carries `issues[]`: every problem, each with its dot path inside the + * request body, its message and a stable code. `message` stays the first + * issue's, as it always was; + * 2. with an API key (`via: 'api_key'`), the config may not carry the fields + * the editor has no control for (UNSUPPORTED_FIELD), steps need a question + * (EMPTY_QUESTION), a PUT config needs `steps` (STEPS_REQUIRED), and the + * saved config is the engine's normalized one; + * 3. with a session, nothing of (2) applies: the editor saves through the + * same routes and legacy forms carry those fields, so they keep saving + * exactly as before; + * 4. POST /v1/forms drops the HubSpot mirror's `formGuid` and + * `formSignature`, for every caller, like duplicating already does. + * + * Key principals are built by hand (`via: 'api_key'` on the seeded owner): the + * rules read the principal, not how a key is resolved. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { createDb, migrate, seed, getFormById, sql, type Db } from '@quill/db'; +import { formConfigSchema, type FormConfig } from '@quill/types'; +import { AdminCrudController } from './admin-crud.controller'; +import { AdminService } from './admin.service'; +import { AuthService } from './auth.service'; +import { BrandingController } from './branding.controller'; +import { FormDestinationsController } from './integrations.controller'; +import { LocalAuthProvider, type HostPrincipal, type ReqLike } from './auth.provider'; + +let db: Db; +let accountId: string; +let formId: string; +/** The seeded owner through a browser session. */ +let session: { forms: AdminCrudController; branding: BrandingController; destinations: FormDestinationsController }; +/** The same owner through an API key. */ +let key: typeof session; + +const req = (): ReqLike => ({ headers: {} }); + +function controllers(auth: AuthService) { + return { + // Submission/Analytics services are unused by the routes under test. + forms: new AdminCrudController(db, auth, new AdminService(db), {} as never, {} as never), + branding: new BrandingController(db, auth), + destinations: new FormDestinationsController(db, auth), + }; +} + +beforeEach(async () => { + db = await createDb('file::memory:'); + await migrate(db); + await seed(db); // account "acme" + demo form "lead-qualifier" + const provider = new LocalAuthProvider(db, { + NODE_ENV: 'test', + DEV_LOGIN_EMAIL: undefined, + AUTH_LOCAL_STRICT: undefined, + SEED_DEMO_FORM: false, + ONBOARDING_WIZARD: false, + }); + const auth = new AuthService(db, provider); + const keyAuth = Object.create(auth) as AuthService; + keyAuth.resolveHost = async (r: ReqLike): Promise => ({ + ...(await auth.resolveHost(r)), + via: 'api_key', + apiKeyId: 'key_test', + }); + session = controllers(auth); + key = controllers(keyAuth); + + const row = await db.get<{ id: string; account_id: string }>( + sql`SELECT id, account_id FROM form WHERE slug = 'lead-qualifier' LIMIT 1`, + ); + formId = row!.id; + accountId = row!.account_id; +}); + +afterEach(async () => { + await db.close(); +}); + +interface Issue { + path: string; + message: string; + code: string; +} +interface Refusal { + error: string; + message: string; + issues: Issue[]; +} + +/** The body of the 400 a call must answer with. */ +async function refusal(call: Promise): Promise { + const err = await call.then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(BadRequestException); + return (err as BadRequestException).getResponse() as Refusal; +} + +const paths = (r: Refusal) => r.issues.map((i) => i.path); +/** A stored config, typed (the controller returns rows with `config: unknown`). */ +const cfg = (c: unknown) => c as FormConfig; + +const step = (over: Record = {}) => ({ + key: 'company', + type: 'text', + question: 'Company?', + ...over, +}); +const config = (steps: unknown[], extra: Record = {}) => ({ + version: 1, + steps, + ...extra, +}); + +describe('issues[] on validation errors (every caller)', () => { + it('returns all three problems of a config, each with its dot path', async () => { + const r = await refusal( + session.forms.updateForm(req(), formId, { + config: { + version: 2, + steps: [step({ type: 'bogus' }), step({ key: 'b', question: 'x'.repeat(501) })], + }, + }), + ); + expect(r.error).toBe('BAD_REQUEST'); + expect(paths(r).sort()).toEqual(['config.steps.0.type', 'config.steps.1.question', 'config.version']); + for (const issue of r.issues) { + expect(issue.message).toEqual(expect.any(String)); + expect(issue.code).toEqual(expect.any(String)); + } + // `message` is still the first issue's, as before `issues[]` existed. + expect(r.message).toBe(r.issues[0]!.message); + }); + + it('a key caller gets the same zod issues', async () => { + const r = await refusal(key.forms.createForm(req(), { name: '', config: config([step({ type: 'bogus' })]) })); + expect(paths(r)).toEqual(expect.arrayContaining(['name', 'config.steps.0.type'])); + }); + + it('branding writes list every problem too', async () => { + const r = await refusal(session.branding.put(req(), { primaryColor: 'not a color!', radius: 'huge' })); + expect(r.error).toBe('BAD_REQUEST'); + expect(paths(r).sort()).toEqual(['primaryColor', 'radius']); + expect(r.message).toBe(r.issues[0]!.message); + }); + + it('destination writes list every problem too', async () => { + const r = await refusal( + session.destinations.putDestinations(req(), formId, { + destinations: [{ type: 'webhook', enabled: true, settings: { url: 'ftp://example.com' } }], + }), + ); + expect(paths(r)).toEqual(['destinations.0.settings.url']); + }); +}); diff --git a/apps/api/src/validation.ts b/apps/api/src/validation.ts new file mode 100644 index 00000000..34a1041c --- /dev/null +++ b/apps/api/src/validation.ts @@ -0,0 +1,75 @@ +import { BadRequestException } from '@nestjs/common'; +import { ZodError } from 'zod'; + +/** + * One problem with a request body, shaped so a caller (an agent, most of all) + * can fix every problem of a request in one attempt instead of one per retry. + */ +export interface ValidationIssue { + /** + * Dot path inside the request BODY, numbers being array positions: + * `config.steps.2.question` is the question of the body's third step. Empty + * for a problem with the body as a whole. + */ + path: string; + message: string; + /** zod's own issue code (`invalid_type`, `too_big`, …) or one of ours (`UNSUPPORTED_FIELD`, …). */ + code: string; +} + +/** + * Issue codes that name the whole refusal: a 400 carrying one answers with it + * as `error` (and leads with it) instead of the generic BAD_REQUEST, because + * it is the one a caller must fix before the rest means anything. + */ +const HEADLINE_CODES: ReadonlySet = new Set(['STEPS_REQUIRED']); + +export function dotPath(path: ReadonlyArray): string { + return path.map(String).join('.'); +} + +export function zodIssues(err: ZodError): ValidationIssue[] { + return err.issues.map((issue) => ({ + path: dotPath(issue.path), + message: issue.message, + code: issue.code, + })); +} + +/** + * The 400 for a list of issues: `{ error, message, issues }`. `message` is the + * first issue's, which is what every validation 400 answered before `issues` + * existed, so a client reading only `message` sees no change. + */ +export function validationError(issues: ValidationIssue[]): BadRequestException { + const ordered = [ + ...issues.filter((i) => HEADLINE_CODES.has(i.code)), + ...issues.filter((i) => !HEADLINE_CODES.has(i.code)), + ]; + const headline = ordered[0] && HEADLINE_CODES.has(ordered[0].code) ? ordered[0].code : 'BAD_REQUEST'; + return new BadRequestException({ error: headline, message: ordered[0]?.message, issues: ordered }); +} + +/** + * Parse a request body, or answer 400 with EVERY problem: the schema's own, + * followed by `extra` (rules the schema cannot express, such as the stricter + * ones an API key is held to). A body the schema accepts but `extra` refuses is + * refused too. + */ +export function parse( + schema: { parse: (v: unknown) => T }, + body: unknown, + extra: ValidationIssue[] = [], +): T { + let value: T | undefined; + let issues: ValidationIssue[] = []; + try { + value = schema.parse(body); + } catch (err) { + if (!(err instanceof ZodError)) throw err; + issues = zodIssues(err); + } + issues = [...issues, ...extra]; + if (issues.length > 0) throw validationError(issues); + return value as T; +} From 12b5bd81fa7b59f9d8585bba1560c4a7728a59f6 Mon Sep 17 00:00:00 2001 From: Josue Hernandez Date: Mon, 12 Oct 2026 17:49:46 -0700 Subject: [PATCH 2/5] feat(api): drop the HubSpot mirror guid from configs sent to POST /v1/forms A config copied from another form carries that form's `formGuid` and `formSignature`, and keeping them aims the new form at the original's mirror form in the portal. Creating now strips them for every caller, with the same helper duplicate uses, now exported from @quill/db. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Josue Hernandez --- apps/api/src/admin-crud.controller.ts | 5 ++++- apps/api/src/strict-validation.spec.ts | 29 ++++++++++++++++++++++++++ packages/db/src/forms.ts | 5 ++++- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/apps/api/src/admin-crud.controller.ts b/apps/api/src/admin-crud.controller.ts index c64059b1..a09ed874 100644 --- a/apps/api/src/admin-crud.controller.ts +++ b/apps/api/src/admin-crud.controller.ts @@ -45,6 +45,7 @@ import { setMemberStatus, updateForm, upsertNotificationSetting, + withoutOwnedMirrorState, type CrudResult, getMemberProfileRaw, setMemberLocale, @@ -592,7 +593,9 @@ export class AdminCrudController { // kit exactly like dashboard-created ones. Caller-supplied branding wins // over the kit (explicit input beats a default). const kit = await getAccountBranding(this.db, p.accountId); - let config: unknown = input.config; + // The HubSpot mirror's `formGuid`/`formSignature` belong to the form that + // made them (a copied config would aim this one at it), as on duplicate. + let config: unknown = withoutOwnedMirrorState(input.config); if (kit) { const base = config && typeof config === 'object' && !Array.isArray(config) diff --git a/apps/api/src/strict-validation.spec.ts b/apps/api/src/strict-validation.spec.ts index d2ddd8f0..a7f863d3 100644 --- a/apps/api/src/strict-validation.spec.ts +++ b/apps/api/src/strict-validation.spec.ts @@ -159,3 +159,32 @@ describe('issues[] on validation errors (every caller)', () => { expect(paths(r)).toEqual(['destinations.0.settings.url']); }); }); + +describe('POST /v1/forms drops the HubSpot mirror state', () => { + const withMirror = config([step()], { + destinations: [ + { + type: 'hubspot', + enabled: true, + settings: { formGuid: 'guid-of-another-form', formSignature: 'sig', formActivity: true }, + }, + ], + }); + + for (const [who, get] of [ + ['a session', () => session], + ['an API key', () => key], + ] as const) { + it(`for ${who}`, async () => { + const created = await get().forms.createForm(req(), { name: 'Copied', config: withMirror }); + const hubspot = (cfg(created.config).destinations ?? [])[0] as { settings: Record }; + expect(hubspot.settings).not.toHaveProperty('formGuid'); + expect(hubspot.settings).not.toHaveProperty('formSignature'); + expect(hubspot.settings.formActivity).toBe(true); + const stored = cfg((await getFormById(db, accountId, created.id))!.config); + expect((stored.destinations![0] as { settings: Record }).settings).not.toHaveProperty( + 'formGuid', + ); + }); + } +}); diff --git a/packages/db/src/forms.ts b/packages/db/src/forms.ts index 952c74fc..bc431bb0 100644 --- a/packages/db/src/forms.ts +++ b/packages/db/src/forms.ts @@ -864,8 +864,11 @@ export async function publishForm( * Duck-typed (no schema parse): a legacy config that fails validation must * still come out with its guid stripped, for the same reason * `storedMirrorState` in the API reads it duck-typed. + * + * Exported for `POST /v1/forms`, the other way a config that names another + * form's mirror can arrive: a config copied from an existing form. */ -function withoutOwnedMirrorState(config: unknown): unknown { +export function withoutOwnedMirrorState(config: unknown): unknown { if (!config || typeof config !== 'object' || Array.isArray(config)) return config; const destinations = (config as { destinations?: unknown }).destinations; if (!Array.isArray(destinations)) return config; From 908f08849ec9dee4460aa1687bdb01d8d470dc5e Mon Sep 17 00:00:00 2001 From: Josue Hernandez Date: Mon, 12 Oct 2026 17:50:19 -0700 Subject: [PATCH 3/5] feat(api): hold API key writes to what the editor can show With `via: 'api_key'` only, a form config is refused with UNSUPPORTED_FIELD and the exact path when it carries a field the editor has no control for (the spec's research list), a step without a question is refused with EMPTY_QUESTION, and a PUT config without `steps` answers 400 STEPS_REQUIRED before anything is written. The webhook fields are also refused on PUT /v1/forms/:id/destinations. The issues join the schema's in one 400. The value the editor itself writes passes (the derived flowGroup, "Continue" on a message, reveal.enabled and scheduler.prefill true, the brand kit's own client logos): every builder step carries those, and so does the normalized config this API answers with, so refusing them would refuse a form read and sent straight back. A key's config is saved as `normalizeConfig` returns it, and the response carries it. A session saves exactly as before, legacy fields included, with an explicit regression test. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Josue Hernandez --- .changeset/api-v1-strict-validation.md | 11 ++ apps/api/src/admin-crud.controller.ts | 16 +- apps/api/src/integrations.controller.ts | 3 +- apps/api/src/strict-config.ts | 208 ++++++++++++++++++++ apps/api/src/strict-validation.spec.ts | 240 ++++++++++++++++++++++++ 5 files changed, 474 insertions(+), 4 deletions(-) create mode 100644 .changeset/api-v1-strict-validation.md create mode 100644 apps/api/src/strict-config.ts diff --git a/.changeset/api-v1-strict-validation.md b/.changeset/api-v1-strict-validation.md new file mode 100644 index 00000000..640e4166 --- /dev/null +++ b/.changeset/api-v1-strict-validation.md @@ -0,0 +1,11 @@ +--- +'@quill/db': minor +--- + +The admin API explains its validation errors and holds API keys to what the editor can show. + +- Every validation 400 of the form, branding and destinations routes now carries `issues: [{ path, message, code }]` with every problem at once. `path` is the dot path inside the request body (`config.steps.2.question`); `message` is still the first issue's, so nothing that reads only `message` changes. +- `POST /v1/forms` drops `formGuid` and `formSignature` from HubSpot destinations, for every caller, as duplicating already did. A config copied from another form no longer points the new form at the original's HubSpot mirror form. `@quill/db` exports the helper both paths use, `withoutOwnedMirrorState`. +- With an API key only (a browser session saves exactly as before, legacy fields included), a form config is refused with `UNSUPPORTED_FIELD` and the field's exact path when it carries a field the editor has no control for: `outcomes[].booking`, `steps[].buttonText`, `steps[].sliderUnitLabel`, `steps[].reveal.subtitleTemplate`, `steps[].reveal.enabled`, `steps[].scheduler.prefill`, `steps[].flowGroup`, `branding.clientLogos`, the webhook `signatureHeader` and `timeoutMs` (also on `PUT /v1/forms/:id/destinations`), and the legacy `config.reveal`, `config.revealAfterStep` and `steps[].triggersReveal`. The value the editor itself writes passes, so a form read and sent straight back saves: the derived `flowGroup`, `buttonText` "Continue" on a message, `reveal.enabled` true on a reveal step, `scheduler.prefill` true, and the brand kit's own client logos. +- With an API key, a step without a question is refused with `EMPTY_QUESTION` (a reveal step asks nothing and is exempt), and `PUT /v1/forms/:id` with a config that has no `steps` answers 400 `STEPS_REQUIRED` and writes nothing. +- With an API key, the saved config is the engine's normalized one (`normalizeConfig`: unique step keys, deduplicated option values, derived flow groups and scoring), and the response returns it. diff --git a/apps/api/src/admin-crud.controller.ts b/apps/api/src/admin-crud.controller.ts index a09ed874..d7fe4f32 100644 --- a/apps/api/src/admin-crud.controller.ts +++ b/apps/api/src/admin-crud.controller.ts @@ -99,6 +99,7 @@ import { AnalyticsEffects } from './analytics-effects'; import { UploadService } from './upload.service'; import { unwrap } from './http'; import { parse } from './validation'; +import { keyFormBodyIssues, keyNormalizedConfig } from './strict-config'; import type { ServerEnv } from '@quill/config/env'; import { assertAdmin, assertCanManageTarget, assertNotSelf, assertOwner } from './permissions'; import { parseIntParam, parseKinds, parseOutboxStatuses } from './query-params'; @@ -576,7 +577,11 @@ export class AdminCrudController { const p = await this.auth.resolveHost(req); // The create schema (not the shared input one): only creation may name a // folder; PUT /v1/forms/:id never moves a form, PATCH /forms/:id/folder does. - const input = parse(formCreateInputSchema, body); + const input = parse( + formCreateInputSchema, + body, + await keyFormBodyIssues(this.db, p, body, { requireSteps: false }), + ); // The other path that AUTHORS a `destinations` array (PUT /forms/:id stages // a draft, and drafts strip the key; duplicate copies stored state and is // exempt — see `hasExtraHubspotDestination`). Same one-HubSpot rule as @@ -595,7 +600,7 @@ export class AdminCrudController { const kit = await getAccountBranding(this.db, p.accountId); // The HubSpot mirror's `formGuid`/`formSignature` belong to the form that // made them (a copied config would aim this one at it), as on duplicate. - let config: unknown = withoutOwnedMirrorState(input.config); + let config: unknown = withoutOwnedMirrorState(keyNormalizedConfig(p, input.config)); if (kit) { const base = config && typeof config === 'object' && !Array.isArray(config) @@ -650,7 +655,12 @@ export class AdminCrudController { @Put('forms/:id') async updateForm(@Req() req: ReqLike, @Param('id') id: string, @Body() body: unknown) { const p = await this.auth.resolveHost(req); - const { config, slug, expectedUpdatedAt, ...meta } = parse(formInputSchema.partial(), body); + const { config: sent, slug, expectedUpdatedAt, ...meta } = parse( + formInputSchema.partial(), + body, + await keyFormBodyIssues(this.db, p, body, { requireSteps: true }), + ); + const config = keyNormalizedConfig(p, sent); if (slug !== undefined) { unwrapCrud(await setFormSlug(this.db, p.accountId, id, slugify(slug))); } diff --git a/apps/api/src/integrations.controller.ts b/apps/api/src/integrations.controller.ts index 1756744a..514e1741 100644 --- a/apps/api/src/integrations.controller.ts +++ b/apps/api/src/integrations.controller.ts @@ -47,6 +47,7 @@ import type { ServerEnv } from '@quill/config/env'; import { AuthService, type ReqLike } from './auth.service'; import { assertAdmin } from './permissions'; import { parse } from './validation'; +import { keyDestinationsBodyIssues } from './strict-config'; import { RateLimitGuard } from './rate-limit'; import { captchaActive, type CaptchaVerifier } from './captcha'; import { fileLinksFor } from './file-link'; @@ -753,7 +754,7 @@ export class FormDestinationsController { @Put('forms/:id/destinations') async putDestinations(@Req() req: ReqLike, @Param('id') id: string, @Body() body: unknown) { const p = await this.auth.resolveHost(req); - const { destinations } = parse(destinationsBodySchema, body); + const { destinations } = parse(destinationsBodySchema, body, keyDestinationsBodyIssues(p, body)); // At most one HubSpot destination — see `hasExtraHubspotDestination`. Not in // the schema above, so a form that already stores two still READS fine. // Compared against what is STORED, not against 1: the builder's per-question diff --git a/apps/api/src/strict-config.ts b/apps/api/src/strict-config.ts new file mode 100644 index 00000000..c48e451f --- /dev/null +++ b/apps/api/src/strict-config.ts @@ -0,0 +1,208 @@ +/** + * The stricter rules a form config is held to when an API KEY writes it. + * + * An agent will write any field the schema accepts, so with a key the API + * refuses what a person could not see or edit afterwards in the builder: the + * fields that have no control in the editor (UNSUPPORTED_FIELD), steps with no + * question (EMPTY_QUESTION), and a PUT config without `steps`, which would + * replace every step with none (STEPS_REQUIRED). What a key saves is the + * engine's normalized config, the same canonical shape the editor saves. + * + * None of this applies to a browser session. The editor saves through the very + * same routes, and forms built before these fields lost their control still + * carry them; refusing them there would stop people saving their own forms. + * + * The rules read the raw body (duck-typed), not the parsed one, so their + * issues join the schema's in a single 400 even when the schema refuses too. + */ +import type { Db } from '@quill/db'; +import { getAccountBranding } from '@quill/db'; +import { + defaultFlowGroup, + normalizeConfig, + type FormConfig as EngineConfig, + type FormFieldType, +} from '@quill/engine'; +import { isApiKeyPrincipal, type HostPrincipal } from './auth.provider'; +import { dotPath, type ValidationIssue } from './validation'; + +export const UNSUPPORTED_FIELD = 'UNSUPPORTED_FIELD'; +export const EMPTY_QUESTION = 'EMPTY_QUESTION'; +export const STEPS_REQUIRED = 'STEPS_REQUIRED'; + +const UNSUPPORTED_MESSAGE = 'This field has no control in the editor.'; +const EMPTY_QUESTION_MESSAGE = 'Every step needs a question.'; +const STEPS_REQUIRED_MESSAGE = + 'Send the full steps array: a config without steps would replace every step with none.'; + +type Path = Array; +type Obj = Record; + +const isObj = (v: unknown): v is Obj => !!v && typeof v === 'object' && !Array.isArray(v); +/** `null` stores nothing a person would need a control for, so it reads as absent. */ +const isSet = (v: unknown): boolean => v !== undefined && v !== null; +const sameJson = (a: unknown, b: unknown): boolean => JSON.stringify(a) === JSON.stringify(b); + +/** + * A step field with no control in the editor, and the one value the editor + * itself writes there (`createEmptyStep` and `normalizeConfig`). That value is + * accepted: every step the builder makes carries it, and so does every config + * this API answers with, so refusing it would refuse a form read and sent + * straight back. Any other value is one the builder cannot show. + */ +interface StepField { + path: Path; + editorWrites?: (value: unknown, type: unknown) => boolean; +} + +/** The research list of config fields without an editor control (see the API spec). */ +const STEP_FIELDS: StepField[] = [ + { path: ['buttonText'], editorWrites: (v, type) => type === 'message' && v === 'Continue' }, + { path: ['sliderUnitLabel'] }, + { path: ['reveal', 'subtitleTemplate'] }, + { path: ['reveal', 'enabled'], editorWrites: (v, type) => type === 'reveal' && v === true }, + { path: ['scheduler', 'prefill'], editorWrites: (v, type) => type === 'scheduler' && v === true }, + { + path: ['flowGroup'], + editorWrites: (v, type) => typeof type === 'string' && v === defaultFlowGroup(type as FormFieldType), + }, + // Legacy: the single form-level reveal's position, before reveal steps. + { path: ['triggersReveal'] }, +]; +const OUTCOME_FIELDS: Path[] = [['booking']]; +/** Legacy: the single form-level reveal, before reveal steps. */ +const TOP_LEVEL_FIELDS: Path[] = [['reveal'], ['revealAfterStep']]; +const WEBHOOK_FIELDS: Path[] = [ + ['settings', 'signatureHeader'], + ['settings', 'timeoutMs'], +]; + +function at(value: unknown, path: Path): unknown { + let node = value; + for (const segment of path) { + if (!isObj(node)) return undefined; + node = node[segment as string]; + } + return node; +} + +const unsupported = (path: Path): ValidationIssue => ({ + path: dotPath(path), + message: UNSUPPORTED_MESSAGE, + code: UNSUPPORTED_FIELD, +}); + +/** Webhook entries of a `destinations` array carrying fields Connect cannot show. */ +export function strictDestinationIssues(destinations: unknown, prefix: Path): ValidationIssue[] { + if (!Array.isArray(destinations)) return []; + const issues: ValidationIssue[] = []; + destinations.forEach((entry, i) => { + if (!isObj(entry) || entry.type !== 'webhook') return; + for (const field of WEBHOOK_FIELDS) { + if (isSet(at(entry, field))) issues.push(unsupported([...prefix, i, ...field])); + } + }); + return issues; +} + +interface StrictConfigOptions { + /** Where the config sits in the request body (`['config']`). */ + prefix: Path; + /** + * The workspace brand kit's client logos. Creating a form (and applying the + * kit) copies them into `branding.clientLogos`, so that exact value is the + * server's own and passes; any other is refused like the rest. + */ + kitClientLogos?: unknown; +} + +/** Every strict-rule issue of one config, steps in order. */ +export function strictConfigIssues(config: unknown, opts: StrictConfigOptions): ValidationIssue[] { + if (!isObj(config)) return []; + const { prefix } = opts; + const issues: ValidationIssue[] = []; + + if (Array.isArray(config.steps)) { + config.steps.forEach((step, i) => { + if (!isObj(step)) return; + // A reveal asks nothing: it has no title and plays its own copy. + // A question of the wrong type is the schema's to report, not ours. + const question = step.question; + const blank = !isSet(question) || (typeof question === 'string' && question.trim() === ''); + if (step.type !== 'reveal' && blank) { + issues.push({ + path: dotPath([...prefix, 'steps', i, 'question']), + message: EMPTY_QUESTION_MESSAGE, + code: EMPTY_QUESTION, + }); + } + for (const field of STEP_FIELDS) { + const value = at(step, field.path); + if (!isSet(value) || field.editorWrites?.(value, step.type)) continue; + issues.push(unsupported([...prefix, 'steps', i, ...field.path])); + } + }); + } + + if (Array.isArray(config.outcomes)) { + config.outcomes.forEach((outcome, i) => { + for (const field of OUTCOME_FIELDS) { + if (isSet(at(outcome, field))) issues.push(unsupported([...prefix, 'outcomes', i, ...field])); + } + }); + } + + const logos = at(config, ['branding', 'clientLogos']); + if (isSet(logos) && !sameJson(logos, opts.kitClientLogos)) { + issues.push(unsupported([...prefix, 'branding', 'clientLogos'])); + } + + for (const field of TOP_LEVEL_FIELDS) { + if (isSet(at(config, field))) issues.push(unsupported([...prefix, ...field])); + } + + issues.push(...strictDestinationIssues(config.destinations, [...prefix, 'destinations'])); + return issues; +} + +/** + * The strict-rule issues of a form write's body (`POST /v1/forms`, + * `PUT /v1/forms/:id`) for this principal: none for a session. `requireSteps` + * is the PUT's: a `config` without `steps` there replaces every step with none. + */ +export async function keyFormBodyIssues( + db: Db, + p: HostPrincipal, + body: unknown, + opts: { requireSteps: boolean }, +): Promise { + if (!isApiKeyPrincipal(p) || !isObj(body) || !isObj(body.config)) return []; + const config = body.config; + const issues: ValidationIssue[] = []; + if (opts.requireSteps && config.steps === undefined) { + issues.push({ path: 'config.steps', message: STEPS_REQUIRED_MESSAGE, code: STEPS_REQUIRED }); + } + // Only read the kit when there is something to compare it with. + const kitClientLogos = isSet(at(config, ['branding', 'clientLogos'])) + ? (await getAccountBranding(db, p.accountId))?.config.clientLogos + : undefined; + issues.push(...strictConfigIssues(config, { prefix: ['config'], kitClientLogos })); + return issues; +} + +/** The strict-rule issues of a `PUT /v1/forms/:id/destinations` body: none for a session. */ +export function keyDestinationsBodyIssues(p: HostPrincipal, body: unknown): ValidationIssue[] { + if (!isApiKeyPrincipal(p) || !isObj(body)) return []; + return strictDestinationIssues(body.destinations, ['destinations']); +} + +/** + * The config an API key saves: the engine's normalized one (unique step keys + * with references following them, deduped option values, derived flow groups + * and scoring, sorted outcomes), the same canonical shape the editor saves. A + * session's config is stored as sent, exactly as before. + */ +export function keyNormalizedConfig(p: HostPrincipal, config: T): T { + if (!isApiKeyPrincipal(p) || config == null) return config; + return normalizeConfig(config as unknown as EngineConfig) as unknown as T; +} diff --git a/apps/api/src/strict-validation.spec.ts b/apps/api/src/strict-validation.spec.ts index a7f863d3..4f678bb1 100644 --- a/apps/api/src/strict-validation.spec.ts +++ b/apps/api/src/strict-validation.spec.ts @@ -160,6 +160,246 @@ describe('issues[] on validation errors (every caller)', () => { }); }); +describe('strict rules with an API key', () => { + it('refuses flowGroup on a step with UNSUPPORTED_FIELD and its exact path', async () => { + const r = await refusal( + key.forms.createForm(req(), { + name: 'Agent form', + config: config([step(), step({ key: 'size', flowGroup: 'lead_capture' })]), + }), + ); + expect(r.error).toBe('BAD_REQUEST'); + expect(r.issues).toEqual([ + { path: 'config.steps.1.flowGroup', code: 'UNSUPPORTED_FIELD', message: expect.any(String) }, + ]); + }); + + it('refuses every field without an editor control, all in one answer', async () => { + const r = await refusal( + key.forms.updateForm(req(), formId, { + config: config( + [ + step({ + buttonText: 'Go', + sliderUnitLabel: 'people', + flowGroup: 'lead_capture', + triggersReveal: true, + }), + { key: 'wait', type: 'reveal', reveal: { enabled: false, subtitleTemplate: 'Hi [company]' } }, + { key: 'book', type: 'scheduler', question: 'Pick a time', scheduler: { prefill: false } }, + ], + { + outcomes: [{ id: 'hot', label: 'Hot', minScore: 0, booking: { provider: 'calendly', url: 'https://example.com/b' } }], + branding: { clientLogos: [{ name: 'Example' }] }, + reveal: { enabled: true, headline: 'Wait' }, + revealAfterStep: 1, + destinations: [ + { + type: 'webhook', + enabled: true, + settings: { url: 'https://example.com/hook', signatureHeader: 'X-Sig', timeoutMs: 5000 }, + }, + ], + }, + ), + }), + ); + expect(r.issues.every((i) => i.code === 'UNSUPPORTED_FIELD')).toBe(true); + expect(paths(r).sort()).toEqual( + [ + 'config.steps.0.buttonText', + 'config.steps.0.sliderUnitLabel', + 'config.steps.0.flowGroup', + 'config.steps.0.triggersReveal', + 'config.steps.1.reveal.enabled', + 'config.steps.1.reveal.subtitleTemplate', + 'config.steps.2.scheduler.prefill', + 'config.outcomes.0.booking', + 'config.branding.clientLogos', + 'config.reveal', + 'config.revealAfterStep', + 'config.destinations.0.settings.signatureHeader', + 'config.destinations.0.settings.timeoutMs', + ].sort(), + ); + }); + + it("accepts the values the editor itself writes, so a form round-trips", async () => { + // What `createEmptyStep` and `normalizeConfig` put on new steps. + const created = await key.forms.createForm(req(), { + name: 'Defaults', + config: config([ + step({ flowGroup: 'qualification' }), + { key: 'email', type: 'email', question: 'Email?', flowGroup: 'lead_capture' }, + { key: 'note', type: 'message', question: 'Thanks', buttonText: 'Continue' }, + { key: 'wait', type: 'reveal', reveal: { enabled: true, headline: '' } }, + { key: 'book', type: 'scheduler', question: 'Pick a time', scheduler: { provider: 'calendly', prefill: true } }, + ]), + }); + // The normalized answer, sent straight back, saves. + const back = await key.forms.updateForm(req(), created.id, { + config: created.config, + expectedUpdatedAt: created.updatedAt, + }); + expect(back.draftConfig).toEqual(created.config); + }); + + it('accepts the brand kit client logos the server put on the form', async () => { + const logos = [{ name: 'Example' }]; + await session.branding.put(req(), { clientLogos: logos }); + const created = await key.forms.createForm(req(), { name: 'Kit', config: config([step()]) }); + expect(cfg(created.config).branding?.clientLogos).toEqual(logos); + await expect(key.forms.updateForm(req(), created.id, { config: created.config })).resolves.toBeTruthy(); + + const other = { ...cfg(created.config), branding: { ...cfg(created.config).branding, clientLogos: [{ name: 'Other' }] } }; + const r = await refusal(key.forms.updateForm(req(), created.id, { config: other })); + expect(paths(r)).toEqual(['config.branding.clientLogos']); + }); + + it('refuses a step without a question with EMPTY_QUESTION, but not a reveal', async () => { + const r = await refusal( + key.forms.updateForm(req(), formId, { + config: config([ + step({ question: ' ' }), + { key: 'email', type: 'email' }, + { key: 'wait', type: 'reveal', reveal: { enabled: true, headline: 'One moment' } }, + ]), + }), + ); + expect(r.issues).toEqual([ + { path: 'config.steps.0.question', code: 'EMPTY_QUESTION', message: expect.any(String) }, + { path: 'config.steps.1.question', code: 'EMPTY_QUESTION', message: expect.any(String) }, + ]); + }); + + it('lists schema issues and strict issues together', async () => { + const r = await refusal( + key.forms.updateForm(req(), formId, { + config: config([step({ type: 'bogus' }), step({ key: 'b', question: '', flowGroup: 'lead_capture' })]), + }), + ); + expect(paths(r)).toEqual(['config.steps.0.type', 'config.steps.1.question', 'config.steps.1.flowGroup']); + expect(r.message).toBe(r.issues[0]!.message); + }); + + it('refuses a PUT config without steps with STEPS_REQUIRED and writes nothing', async () => { + const before = await getFormById(db, accountId, formId); + const r = await refusal( + key.forms.updateForm(req(), formId, { name: 'Renamed', slug: 'renamed', config: { version: 1 } }), + ); + expect(r.error).toBe('STEPS_REQUIRED'); + expect(r.issues[0]).toEqual({ path: 'config.steps', code: 'STEPS_REQUIRED', message: r.message }); + const after = await getFormById(db, accountId, formId); + expect(after).toEqual(before); + }); + + it('a PUT without a config at all still renames', async () => { + const out = await key.forms.updateForm(req(), formId, { name: 'Renamed' }); + expect(out.name).toBe('Renamed'); + }); + + it('saves and returns the normalized config', async () => { + const dupes = config([ + step({ + key: 'size', + type: 'multiple_choice', + question: 'Team size?', + options: [ + { label: 'Small', value: 'a' }, + { label: 'Large', value: 'a' }, + ], + }), + ]); + const updated = await key.forms.updateForm(req(), formId, { config: dupes }); + const options = cfg(updated.draftConfig).steps[0]!.options!; + expect(new Set(options.map((o) => o.value)).size).toBe(2); + expect(cfg(updated.draftConfig).steps[0]!.selectionMode).toBe('single'); + // What came back is what is stored. + expect((await getFormById(db, accountId, formId))!.draftConfig).toEqual(updated.draftConfig); + + const created = await key.forms.createForm(req(), { name: 'Normalized', config: dupes }); + expect(new Set(cfg(created.config).steps[0]!.options!.map((o) => o.value)).size).toBe(2); + }); + + it('refuses webhook signatureHeader and timeoutMs on PUT .../destinations', async () => { + const body = { + destinations: [ + { + type: 'webhook', + enabled: true, + settings: { url: 'https://example.com/hook', signatureHeader: 'X-Sig', timeoutMs: 5000 }, + }, + ], + }; + const r = await refusal(key.destinations.putDestinations(req(), formId, body)); + expect(r.issues).toEqual([ + { path: 'destinations.0.settings.signatureHeader', code: 'UNSUPPORTED_FIELD', message: expect.any(String) }, + { path: 'destinations.0.settings.timeoutMs', code: 'UNSUPPORTED_FIELD', message: expect.any(String) }, + ]); + // A plain webhook is fine with a key. + const plain = { destinations: [{ type: 'webhook', enabled: true, settings: { url: 'https://example.com/hook' } }] }; + await expect(key.destinations.putDestinations(req(), formId, plain)).resolves.toBeTruthy(); + }); +}); + +describe('a session keeps saving legacy forms exactly as before', () => { + const LEGACY_REVEAL = { enabled: true, headline: 'Wait', subtitleTemplate: 'Hi [company]' }; + /** A form from before the editor dropped these fields. */ + const LEGACY = config( + [ + step({ flowGroup: 'lead_capture', buttonText: 'Next one', sliderUnitLabel: 'people', triggersReveal: true }), + step({ key: 'empty', question: '' }), + { + key: 'size', + type: 'multiple_choice', + question: 'Team size?', + options: [ + { label: 'Small', value: 'a' }, + { label: 'Large', value: 'a' }, + ], + }, + ], + { + reveal: LEGACY_REVEAL, + revealAfterStep: 1, + branding: { clientLogos: [{ name: 'Example' }] }, + }, + ); + + it('PUT stores the legacy config untouched: no refusal, no normalization', async () => { + const updated = await session.forms.updateForm(req(), formId, { config: LEGACY }); + expect(updated.draftConfig).toEqual(formConfigSchema.parse(LEGACY)); + expect(cfg(updated.draftConfig).steps[0]!.flowGroup).toBe('lead_capture'); + expect(cfg(updated.draftConfig).reveal).toEqual(LEGACY_REVEAL); + }); + + it('PUT without steps still saves through a session', async () => { + const updated = await session.forms.updateForm(req(), formId, { config: { version: 1 } }); + expect(updated.draftConfig).toEqual({ version: 1, steps: [] }); + }); + + it('POST stores the legacy config as given', async () => { + const created = await session.forms.createForm(req(), { name: 'Legacy', config: LEGACY }); + expect(cfg(created.config).steps).toEqual(formConfigSchema.parse(LEGACY).steps); + expect(cfg(created.config).reveal).toEqual(LEGACY_REVEAL); + expect(cfg(created.config).revealAfterStep).toBe(1); + }); + + it('PUT .../destinations keeps webhook signatureHeader and timeoutMs', async () => { + const out = await session.destinations.putDestinations(req(), formId, { + destinations: [ + { + type: 'webhook', + enabled: true, + settings: { url: 'https://example.com/hook', signatureHeader: 'X-Sig', timeoutMs: 5000 }, + }, + ], + }); + const hook = (out.config as { destinations: Array<{ settings: Record }> }).destinations[0]!; + expect(hook.settings).toMatchObject({ signatureHeader: 'X-Sig', timeoutMs: 5000 }); + }); +}); + describe('POST /v1/forms drops the HubSpot mirror state', () => { const withMirror = config([step()], { destinations: [ From f81411d309e2988861dbde4f6921ee62e0d48fd6 Mon Sep 17 00:00:00 2001 From: Josue Hernandez Date: Mon, 12 Oct 2026 17:55:26 -0700 Subject: [PATCH 4/5] fix(api): accept the builder's step defaults on a retyped step Changing a step's type in the builder keeps what createEmptyStep seeded (buttonText "Continue", reveal.enabled true, scheduler.prefill true) and only resets flowGroup. A builder form with a retyped step was refused with UNSUPPORTED_FIELD when an API key sent it straight back. Those seeded values now pass on any step type; flowGroup stays tied to the step's type. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Josue Hernandez --- .changeset/api-v1-strict-validation.md | 2 +- apps/api/src/strict-config.ts | 11 ++++++++--- apps/api/src/strict-validation.spec.ts | 20 ++++++++++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/.changeset/api-v1-strict-validation.md b/.changeset/api-v1-strict-validation.md index 640e4166..69d189e8 100644 --- a/.changeset/api-v1-strict-validation.md +++ b/.changeset/api-v1-strict-validation.md @@ -6,6 +6,6 @@ The admin API explains its validation errors and holds API keys to what the edit - Every validation 400 of the form, branding and destinations routes now carries `issues: [{ path, message, code }]` with every problem at once. `path` is the dot path inside the request body (`config.steps.2.question`); `message` is still the first issue's, so nothing that reads only `message` changes. - `POST /v1/forms` drops `formGuid` and `formSignature` from HubSpot destinations, for every caller, as duplicating already did. A config copied from another form no longer points the new form at the original's HubSpot mirror form. `@quill/db` exports the helper both paths use, `withoutOwnedMirrorState`. -- With an API key only (a browser session saves exactly as before, legacy fields included), a form config is refused with `UNSUPPORTED_FIELD` and the field's exact path when it carries a field the editor has no control for: `outcomes[].booking`, `steps[].buttonText`, `steps[].sliderUnitLabel`, `steps[].reveal.subtitleTemplate`, `steps[].reveal.enabled`, `steps[].scheduler.prefill`, `steps[].flowGroup`, `branding.clientLogos`, the webhook `signatureHeader` and `timeoutMs` (also on `PUT /v1/forms/:id/destinations`), and the legacy `config.reveal`, `config.revealAfterStep` and `steps[].triggersReveal`. The value the editor itself writes passes, so a form read and sent straight back saves: the derived `flowGroup`, `buttonText` "Continue" on a message, `reveal.enabled` true on a reveal step, `scheduler.prefill` true, and the brand kit's own client logos. +- With an API key only (a browser session saves exactly as before, legacy fields included), a form config is refused with `UNSUPPORTED_FIELD` and the field's exact path when it carries a field the editor has no control for: `outcomes[].booking`, `steps[].buttonText`, `steps[].sliderUnitLabel`, `steps[].reveal.subtitleTemplate`, `steps[].reveal.enabled`, `steps[].scheduler.prefill`, `steps[].flowGroup`, `branding.clientLogos`, the webhook `signatureHeader` and `timeoutMs` (also on `PUT /v1/forms/:id/destinations`), and the legacy `config.reveal`, `config.revealAfterStep` and `steps[].triggersReveal`. The value the editor itself writes passes, so a form read and sent straight back saves: the derived `flowGroup`, the step defaults `buttonText` "Continue", `reveal.enabled` true and `scheduler.prefill` true (on any step, since changing a step's type in the builder keeps them), and the brand kit's own client logos. - With an API key, a step without a question is refused with `EMPTY_QUESTION` (a reveal step asks nothing and is exempt), and `PUT /v1/forms/:id` with a config that has no `steps` answers 400 `STEPS_REQUIRED` and writes nothing. - With an API key, the saved config is the engine's normalized one (`normalizeConfig`: unique step keys, deduplicated option values, derived flow groups and scoring), and the response returns it. diff --git a/apps/api/src/strict-config.ts b/apps/api/src/strict-config.ts index c48e451f..0e91eb5f 100644 --- a/apps/api/src/strict-config.ts +++ b/apps/api/src/strict-config.ts @@ -49,6 +49,11 @@ const sameJson = (a: unknown, b: unknown): boolean => JSON.stringify(a) === JSON * accepted: every step the builder makes carries it, and so does every config * this API answers with, so refusing it would refuse a form read and sent * straight back. Any other value is one the builder cannot show. + * + * The seeded values pass on ANY step type, not only the type that seeds them: + * changing a step's type in the builder keeps them (a message turned into a + * text question still carries `buttonText` "Continue"). `flowGroup` is the + * exception, because the type change rewrites it to the new type's default. */ interface StepField { path: Path; @@ -57,11 +62,11 @@ interface StepField { /** The research list of config fields without an editor control (see the API spec). */ const STEP_FIELDS: StepField[] = [ - { path: ['buttonText'], editorWrites: (v, type) => type === 'message' && v === 'Continue' }, + { path: ['buttonText'], editorWrites: (v) => v === 'Continue' }, { path: ['sliderUnitLabel'] }, { path: ['reveal', 'subtitleTemplate'] }, - { path: ['reveal', 'enabled'], editorWrites: (v, type) => type === 'reveal' && v === true }, - { path: ['scheduler', 'prefill'], editorWrites: (v, type) => type === 'scheduler' && v === true }, + { path: ['reveal', 'enabled'], editorWrites: (v) => v === true }, + { path: ['scheduler', 'prefill'], editorWrites: (v) => v === true }, { path: ['flowGroup'], editorWrites: (v, type) => typeof type === 'string' && v === defaultFlowGroup(type as FormFieldType), diff --git a/apps/api/src/strict-validation.spec.ts b/apps/api/src/strict-validation.spec.ts index 4f678bb1..89b1ecea 100644 --- a/apps/api/src/strict-validation.spec.ts +++ b/apps/api/src/strict-validation.spec.ts @@ -244,6 +244,26 @@ describe('strict rules with an API key', () => { expect(back.draftConfig).toEqual(created.config); }); + it('accepts the seeded values on a step whose type the builder changed', async () => { + // The builder's type change keeps what `createEmptyStep` seeded and only + // resets `flowGroup`: a message, reveal and scheduler turned into text. + const created = await session.forms.createForm(req(), { + name: 'Retyped', + config: config([ + step({ key: 'was_message', flowGroup: 'qualification', buttonText: 'Continue' }), + step({ key: 'was_reveal', flowGroup: 'qualification', reveal: { enabled: true, headline: '' } }), + step({ + key: 'was_scheduler', + flowGroup: 'qualification', + scheduler: { provider: 'calendly', prefill: true }, + }), + ]), + }); + await expect( + key.forms.updateForm(req(), created.id, { config: created.config, expectedUpdatedAt: created.updatedAt }), + ).resolves.toBeTruthy(); + }); + it('accepts the brand kit client logos the server put on the form', async () => { const logos = [{ name: 'Example' }]; await session.branding.put(req(), { clientLogos: logos }); From 7e2c000c1c01749ee1d53d2accb4ca68190eee92 Mon Sep 17 00:00:00 2001 From: Josue Hernandez Date: Mon, 12 Oct 2026 17:57:24 -0700 Subject: [PATCH 5/5] fix(api): skip draft destinations and compare kit logos by value PUT /v1/forms/:id drops destinations from the draft, so refusing webhook signatureHeader/timeoutMs there refused every edit of a form whose live webhook already carried one, and removing it from the body never helped. The webhook rule now runs where destinations are written: POST /v1/forms and PUT /v1/forms/:id/destinations. The brand kit client logos comparison used JSON.stringify, so the same logos with their keys in another order were refused. It now uses isDeepStrictEqual. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Josue Hernandez --- .changeset/api-v1-strict-validation.md | 2 +- apps/api/src/admin-crud.controller.ts | 4 +-- apps/api/src/strict-config.ts | 24 ++++++++++++--- apps/api/src/strict-validation.spec.ts | 42 ++++++++++++++++++++------ 4 files changed, 55 insertions(+), 17 deletions(-) diff --git a/.changeset/api-v1-strict-validation.md b/.changeset/api-v1-strict-validation.md index 69d189e8..831ae621 100644 --- a/.changeset/api-v1-strict-validation.md +++ b/.changeset/api-v1-strict-validation.md @@ -6,6 +6,6 @@ The admin API explains its validation errors and holds API keys to what the edit - Every validation 400 of the form, branding and destinations routes now carries `issues: [{ path, message, code }]` with every problem at once. `path` is the dot path inside the request body (`config.steps.2.question`); `message` is still the first issue's, so nothing that reads only `message` changes. - `POST /v1/forms` drops `formGuid` and `formSignature` from HubSpot destinations, for every caller, as duplicating already did. A config copied from another form no longer points the new form at the original's HubSpot mirror form. `@quill/db` exports the helper both paths use, `withoutOwnedMirrorState`. -- With an API key only (a browser session saves exactly as before, legacy fields included), a form config is refused with `UNSUPPORTED_FIELD` and the field's exact path when it carries a field the editor has no control for: `outcomes[].booking`, `steps[].buttonText`, `steps[].sliderUnitLabel`, `steps[].reveal.subtitleTemplate`, `steps[].reveal.enabled`, `steps[].scheduler.prefill`, `steps[].flowGroup`, `branding.clientLogos`, the webhook `signatureHeader` and `timeoutMs` (also on `PUT /v1/forms/:id/destinations`), and the legacy `config.reveal`, `config.revealAfterStep` and `steps[].triggersReveal`. The value the editor itself writes passes, so a form read and sent straight back saves: the derived `flowGroup`, the step defaults `buttonText` "Continue", `reveal.enabled` true and `scheduler.prefill` true (on any step, since changing a step's type in the builder keeps them), and the brand kit's own client logos. +- With an API key only (a browser session saves exactly as before, legacy fields included), a form config is refused with `UNSUPPORTED_FIELD` and the field's exact path when it carries a field the editor has no control for: `outcomes[].booking`, `steps[].buttonText`, `steps[].sliderUnitLabel`, `steps[].reveal.subtitleTemplate`, `steps[].reveal.enabled`, `steps[].scheduler.prefill`, `steps[].flowGroup`, `branding.clientLogos`, the webhook `signatureHeader` and `timeoutMs` (checked where destinations are written: `POST /v1/forms` and `PUT /v1/forms/:id/destinations`; `PUT /v1/forms/:id` drops destinations from the draft, so it does not refuse them), and the legacy `config.reveal`, `config.revealAfterStep` and `steps[].triggersReveal`. The value the editor itself writes passes, so a form read and sent straight back saves: the derived `flowGroup`, the step defaults `buttonText` "Continue", `reveal.enabled` true and `scheduler.prefill` true (on any step, since changing a step's type in the builder keeps them), and the brand kit's own client logos. - With an API key, a step without a question is refused with `EMPTY_QUESTION` (a reveal step asks nothing and is exempt), and `PUT /v1/forms/:id` with a config that has no `steps` answers 400 `STEPS_REQUIRED` and writes nothing. - With an API key, the saved config is the engine's normalized one (`normalizeConfig`: unique step keys, deduplicated option values, derived flow groups and scoring), and the response returns it. diff --git a/apps/api/src/admin-crud.controller.ts b/apps/api/src/admin-crud.controller.ts index d7fe4f32..4c09f0b8 100644 --- a/apps/api/src/admin-crud.controller.ts +++ b/apps/api/src/admin-crud.controller.ts @@ -580,7 +580,7 @@ export class AdminCrudController { const input = parse( formCreateInputSchema, body, - await keyFormBodyIssues(this.db, p, body, { requireSteps: false }), + await keyFormBodyIssues(this.db, p, body, { requireSteps: false, storesDestinations: true }), ); // The other path that AUTHORS a `destinations` array (PUT /forms/:id stages // a draft, and drafts strip the key; duplicate copies stored state and is @@ -658,7 +658,7 @@ export class AdminCrudController { const { config: sent, slug, expectedUpdatedAt, ...meta } = parse( formInputSchema.partial(), body, - await keyFormBodyIssues(this.db, p, body, { requireSteps: true }), + await keyFormBodyIssues(this.db, p, body, { requireSteps: true, storesDestinations: false }), ); const config = keyNormalizedConfig(p, sent); if (slug !== undefined) { diff --git a/apps/api/src/strict-config.ts b/apps/api/src/strict-config.ts index 0e91eb5f..672717b1 100644 --- a/apps/api/src/strict-config.ts +++ b/apps/api/src/strict-config.ts @@ -15,6 +15,7 @@ * The rules read the raw body (duck-typed), not the parsed one, so their * issues join the schema's in a single 400 even when the schema refuses too. */ +import { isDeepStrictEqual } from 'node:util'; import type { Db } from '@quill/db'; import { getAccountBranding } from '@quill/db'; import { @@ -41,7 +42,6 @@ type Obj = Record; const isObj = (v: unknown): v is Obj => !!v && typeof v === 'object' && !Array.isArray(v); /** `null` stores nothing a person would need a control for, so it reads as absent. */ const isSet = (v: unknown): boolean => v !== undefined && v !== null; -const sameJson = (a: unknown, b: unknown): boolean => JSON.stringify(a) === JSON.stringify(b); /** * A step field with no control in the editor, and the one value the editor @@ -119,6 +119,13 @@ interface StrictConfigOptions { * server's own and passes; any other is refused like the rest. */ kitClientLogos?: unknown; + /** + * Whether the route stores `config.destinations`. `POST /v1/forms` does; + * `PUT /v1/forms/:id` drops them from the draft (the integrations screen owns + * them live), so a webhook field there is never saved and refusing it would + * refuse every edit of a form whose live webhook already carries one. + */ + destinations: boolean; } /** Every strict-rule issue of one config, steps in order. */ @@ -158,7 +165,8 @@ export function strictConfigIssues(config: unknown, opts: StrictConfigOptions): } const logos = at(config, ['branding', 'clientLogos']); - if (isSet(logos) && !sameJson(logos, opts.kitClientLogos)) { + // Key order is not part of the value (Postgres jsonb reorders keys). + if (isSet(logos) && !isDeepStrictEqual(logos, opts.kitClientLogos)) { issues.push(unsupported([...prefix, 'branding', 'clientLogos'])); } @@ -166,7 +174,9 @@ export function strictConfigIssues(config: unknown, opts: StrictConfigOptions): if (isSet(at(config, field))) issues.push(unsupported([...prefix, ...field])); } - issues.push(...strictDestinationIssues(config.destinations, [...prefix, 'destinations'])); + if (opts.destinations) { + issues.push(...strictDestinationIssues(config.destinations, [...prefix, 'destinations'])); + } return issues; } @@ -174,12 +184,14 @@ export function strictConfigIssues(config: unknown, opts: StrictConfigOptions): * The strict-rule issues of a form write's body (`POST /v1/forms`, * `PUT /v1/forms/:id`) for this principal: none for a session. `requireSteps` * is the PUT's: a `config` without `steps` there replaces every step with none. + * The PUT's draft also drops `destinations`, so their webhook fields are only + * checked on create (and on `PUT .../destinations`). */ export async function keyFormBodyIssues( db: Db, p: HostPrincipal, body: unknown, - opts: { requireSteps: boolean }, + opts: { requireSteps: boolean; storesDestinations: boolean }, ): Promise { if (!isApiKeyPrincipal(p) || !isObj(body) || !isObj(body.config)) return []; const config = body.config; @@ -191,7 +203,9 @@ export async function keyFormBodyIssues( const kitClientLogos = isSet(at(config, ['branding', 'clientLogos'])) ? (await getAccountBranding(db, p.accountId))?.config.clientLogos : undefined; - issues.push(...strictConfigIssues(config, { prefix: ['config'], kitClientLogos })); + issues.push( + ...strictConfigIssues(config, { prefix: ['config'], kitClientLogos, destinations: opts.storesDestinations }), + ); return issues; } diff --git a/apps/api/src/strict-validation.spec.ts b/apps/api/src/strict-validation.spec.ts index 89b1ecea..d2c7b11a 100644 --- a/apps/api/src/strict-validation.spec.ts +++ b/apps/api/src/strict-validation.spec.ts @@ -193,13 +193,6 @@ describe('strict rules with an API key', () => { branding: { clientLogos: [{ name: 'Example' }] }, reveal: { enabled: true, headline: 'Wait' }, revealAfterStep: 1, - destinations: [ - { - type: 'webhook', - enabled: true, - settings: { url: 'https://example.com/hook', signatureHeader: 'X-Sig', timeoutMs: 5000 }, - }, - ], }, ), }), @@ -218,12 +211,34 @@ describe('strict rules with an API key', () => { 'config.branding.clientLogos', 'config.reveal', 'config.revealAfterStep', - 'config.destinations.0.settings.signatureHeader', - 'config.destinations.0.settings.timeoutMs', ].sort(), ); }); + const hookWithExtras = { + type: 'webhook', + enabled: true, + settings: { url: 'https://example.com/hook', signatureHeader: 'X-Sig', timeoutMs: 5000 }, + }; + + it('refuses webhook signatureHeader and timeoutMs in a created config', async () => { + const r = await refusal( + key.forms.createForm(req(), { name: 'Hooked', config: config([step()], { destinations: [hookWithExtras] }) }), + ); + expect(paths(r)).toEqual([ + 'config.destinations.0.settings.signatureHeader', + 'config.destinations.0.settings.timeoutMs', + ]); + }); + + it('lets a PUT through when the live webhook already carries them (the draft drops destinations)', async () => { + await session.destinations.putDestinations(req(), formId, { destinations: [hookWithExtras] }); + const live = (await getFormById(db, accountId, formId))!; + const sent = { ...cfg(live.config), steps: [step()] }; + const updated = await key.forms.updateForm(req(), formId, { config: sent }); + expect(cfg(updated.draftConfig).destinations).toBeUndefined(); + }); + it("accepts the values the editor itself writes, so a form round-trips", async () => { // What `createEmptyStep` and `normalizeConfig` put on new steps. const created = await key.forms.createForm(req(), { @@ -271,6 +286,15 @@ describe('strict rules with an API key', () => { expect(cfg(created.config).branding?.clientLogos).toEqual(logos); await expect(key.forms.updateForm(req(), created.id, { config: created.config })).resolves.toBeTruthy(); + // The same logos with their keys in another order are the same value. + const kitLogos = [{ name: 'Example', src: 'https://example.com/logo.png' }]; + await session.branding.put(req(), { clientLogos: kitLogos }); + const reordered = { + ...cfg(created.config), + branding: { ...cfg(created.config).branding, clientLogos: [{ src: 'https://example.com/logo.png', name: 'Example' }] }, + }; + await expect(key.forms.updateForm(req(), created.id, { config: reordered })).resolves.toBeTruthy(); + const other = { ...cfg(created.config), branding: { ...cfg(created.config).branding, clientLogos: [{ name: 'Other' }] } }; const r = await refusal(key.forms.updateForm(req(), created.id, { config: other })); expect(paths(r)).toEqual(['config.branding.clientLogos']);