diff --git a/.changeset/api-v1-strict-validation.md b/.changeset/api-v1-strict-validation.md new file mode 100644 index 00000000..831ae621 --- /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` (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 5ffa11a7..4c09f0b8 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, @@ -88,7 +89,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 +98,8 @@ import { EmailEffects } from './email-effects'; 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'; @@ -105,16 +107,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.' }); @@ -585,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, 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 // exempt — see `hasExtraHubspotDestination`). Same one-HubSpot rule as @@ -602,7 +598,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(keyNormalizedConfig(p, input.config)); if (kit) { const base = config && typeof config === 'object' && !Array.isArray(config) @@ -657,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, storesDestinations: false }), + ); + const config = keyNormalizedConfig(p, sent); if (slug !== undefined) { unwrapCrud(await setFormSlug(this.db, p.accountId, id, slugify(slug))); } 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..514e1741 100644 --- a/apps/api/src/integrations.controller.ts +++ b/apps/api/src/integrations.controller.ts @@ -46,6 +46,8 @@ 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 { keyDestinationsBodyIssues } from './strict-config'; import { RateLimitGuard } from './rate-limit'; import { captchaActive, type CaptchaVerifier } from './captcha'; import { fileLinksFor } from './file-link'; @@ -752,14 +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); - 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, 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..672717b1 --- /dev/null +++ b/apps/api/src/strict-config.ts @@ -0,0 +1,227 @@ +/** + * 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 { isDeepStrictEqual } from 'node:util'; +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; + +/** + * 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. + * + * 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; + 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) => v === 'Continue' }, + { path: ['sliderUnitLabel'] }, + { path: ['reveal', 'subtitleTemplate'] }, + { 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), + }, + // 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; + /** + * 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. */ +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']); + // 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'])); + } + + for (const field of TOP_LEVEL_FIELDS) { + if (isSet(at(config, field))) issues.push(unsupported([...prefix, ...field])); + } + + if (opts.destinations) { + 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. + * 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; storesDestinations: 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, destinations: opts.storesDestinations }), + ); + 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 new file mode 100644 index 00000000..d2c7b11a --- /dev/null +++ b/apps/api/src/strict-validation.spec.ts @@ -0,0 +1,474 @@ +/** + * 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']); + }); +}); + +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, + }, + ), + }), + ); + 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', + ].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(), { + 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 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 }); + 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(); + + // 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']); + }); + + 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: [ + { + 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/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; +} 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;