From 243f4bfddfb395ae34221c73a3d38e4d0b551752 Mon Sep 17 00:00:00 2001 From: Dhruwang Jariwala <67850763+Dhruwang@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:57:17 +0000 Subject: [PATCH 1/5] fix(api-v3): return 422 instead of 500 for post-schema survey validation failures [ENG-2587] (#8991) --- apps/web/app/api/v3/surveys/create.test.ts | 167 +++++++++++++++++- apps/web/app/api/v3/surveys/create.ts | 37 +++- .../app/api/v3/surveys/lib/operations.test.ts | 46 ++++- apps/web/app/api/v3/surveys/lib/operations.ts | 20 ++- apps/web/app/api/v3/surveys/schemas.test.ts | 104 +++++++++++ apps/web/app/api/v3/surveys/schemas.ts | 29 ++- docs/api-v3-reference/openapi.yml | 2 +- .../src/paths/api_v3_surveys.yml | 5 +- 8 files changed, 402 insertions(+), 8 deletions(-) diff --git a/apps/web/app/api/v3/surveys/create.test.ts b/apps/web/app/api/v3/surveys/create.test.ts index aea71a13b30d..b8802c017604 100644 --- a/apps/web/app/api/v3/surveys/create.test.ts +++ b/apps/web/app/api/v3/surveys/create.test.ts @@ -5,7 +5,7 @@ import { getActionClasses } from "@/lib/actionClass/service"; import { getOrganizationByWorkspaceId } from "@/lib/organization/service"; import { createSurvey, getSurvey } from "@/lib/survey/service"; import { getExternalUrlsPermission } from "@/modules/survey/lib/permission"; -import { V3SurveyCreatePermissionError, createV3Survey } from "./create"; +import { V3SurveyCreatePermissionError, V3SurveyInputValidationError, createV3Survey } from "./create"; import { V3SurveyReferenceValidationError } from "./reference-validation"; import { ZV3CreateSurveyBody } from "./schemas"; import { resolveV3ContactsEntitlement } from "./targeting"; @@ -306,6 +306,171 @@ describe("createV3Survey", () => { expect(createSurvey).not.toHaveBeenCalled(); }); + // ENG-2587. Driven through the real `ZSurveyCreateInput`/`surveyRefinement` rather than a + // synthetic schema, so it also fails if the refinement's issue path moves, or if the CTA branch + // stops firing (it is gated on `buttonExternal`). `createSurvey` is mocked, so no DB is involved. + test("rejects a CTA buttonUrl scheme the request schema admits but the write schema does not", async () => { + vi.mocked(getExternalUrlsPermission).mockResolvedValue(true); + const body = ZV3CreateSurveyBody.parse({ + ...rawCreateBody, + blocks: [ + { + ...rawCreateBody.blocks[0], + elements: [ + { + id: "tel_cta", + type: "cta", + headline: { "en-US": "Call us", "de-DE": "Ruf uns an" }, + required: false, + buttonExternal: true, + buttonUrl: "tel:+123456789", + ctaButtonLabel: { "en-US": "Call", "de-DE": "Anrufen" }, + }, + ], + }, + ], + }); + + const error = await createV3Survey(body, null, "req_eng2587").catch((err: unknown) => err); + + expect(error).toBeInstanceOf(V3SurveyInputValidationError); + expect((error as V3SurveyInputValidationError).invalidParams).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "blocks.0.elements.0.buttonUrl" })]) + ); + // The whole point of pre-write validation: nothing was written. + expect(createSurvey).not.toHaveBeenCalled(); + }); + + // Found in review on #8991: a second case the request schema admits and the write schema rejects, so + // it used to be a 500 too. Same path as the CTA case above, different refinement — worth pinning + // because the description claims this case is covered, and a live check alone would not keep it so. + // The other case the request schema admits and the write schema rejects: `isSafeLinkUrl` allows + // `http:`, `safeUrlRefinement` allows only `https://` and `http://localhost`. Per review this is the + // most common of the three in practice, so it gets its own assertion rather than riding on `tel:`. + test("rejects a plain http CTA buttonUrl the request schema admits", async () => { + vi.mocked(getExternalUrlsPermission).mockResolvedValue(true); + const body = ZV3CreateSurveyBody.parse({ + ...rawCreateBody, + blocks: [ + { + ...rawCreateBody.blocks[0], + elements: [ + { + id: "http_cta", + type: "cta", + headline: { "en-US": "Docs", "de-DE": "Doku" }, + required: false, + buttonExternal: true, + buttonUrl: "http://example.com", + ctaButtonLabel: { "en-US": "Open", "de-DE": "Oeffnen" }, + }, + ], + }, + ], + }); + + const error = await createV3Survey(body, null, "req_http_cta").catch((err: unknown) => err); + + expect(error).toBeInstanceOf(V3SurveyInputValidationError); + expect((error as V3SurveyInputValidationError).invalidParams).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "blocks.0.elements.0.buttonUrl" })]) + ); + expect(createSurvey).not.toHaveBeenCalled(); + }); + + // A blank headline is rejected either side of the new parse, and which side depends on the language + // set rather than on the blankness. What `prepareV3SurveyCreate` catches here is the *absent* `de-DE` + // key, since `rawCreateBody` declares `de-DE` while this headline carries only `en-US`. + test("routes a blank headline with a missing declared locale to the earlier preparation guard", async () => { + const body = ZV3CreateSurveyBody.parse({ + ...rawCreateBody, + blocks: [ + { + ...rawCreateBody.blocks[0], + elements: [ + { + id: "blank_headline_missing_locale", + type: "openText", + headline: { "en-US": " " }, + required: true, + }, + ], + }, + ], + }); + + const error = await createV3Survey(body, null, "req_blank_missing_locale").catch((err: unknown) => err); + + expect(error).toBeInstanceOf(V3SurveyReferenceValidationError); + expect(error).not.toBeInstanceOf(V3SurveyInputValidationError); + expect(createSurvey).not.toHaveBeenCalled(); + }); + + // ...and once every declared locale key is present, the blank headline reaches the new pre-write parse + // instead. These two shapes were 500s on `main`, so they are part of what this PR fixes. Found in + // review after an earlier version of this suite pinned only the shape above and over-generalised + // from it. + test.each([ + ["no languages declared", undefined, { "en-US": " " }, { "en-US": "Product Feedback" }], + [ + "every declared locale present and blank", + [{ code: "de-DE", enabled: true }], + { "en-US": " ", "de-DE": " " }, + { "en-US": "Product Feedback", "de-DE": "Produktfeedback" }, + ], + ])( + "rejects a blank headline with %s through the new pre-write parse", + async (_label, languages, headline, title) => { + const body = ZV3CreateSurveyBody.parse({ + ...rawCreateBody, + languages, + // The metadata title has to carry the same declared locales, or preparation rejects *that* + // missing key first and the element never reaches the parse under test. + metadata: { cx_operation: "enterprise_onboarding", title }, + blocks: [ + { + ...rawCreateBody.blocks[0], + elements: [{ id: "blank_headline", type: "openText", headline, required: true }], + }, + ], + }); + + const error = await createV3Survey(body, null, "req_blank_headline").catch((err: unknown) => err); + + expect(error).toBeInstanceOf(V3SurveyInputValidationError); + expect((error as V3SurveyInputValidationError).invalidParams).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "blocks.0.elements.0.headline" })]) + ); + expect(createSurvey).not.toHaveBeenCalled(); + } + ); + + test("accepts an https CTA buttonUrl through the same path", async () => { + vi.mocked(getExternalUrlsPermission).mockResolvedValue(true); + const body = ZV3CreateSurveyBody.parse({ + ...rawCreateBody, + blocks: [ + { + ...rawCreateBody.blocks[0], + elements: [ + { + id: "https_cta", + type: "cta", + headline: { "en-US": "Continue", "de-DE": "Weiter" }, + required: false, + buttonExternal: true, + buttonUrl: "https://example.com", + ctaButtonLabel: { "en-US": "Open", "de-DE": "\u00d6ffnen" }, + }, + ], + }, + ], + }); + + await expect(createV3Survey(body, null, "req_eng2587_ok")).resolves.toBeDefined(); + expect(createSurvey).toHaveBeenCalled(); + }); + test("rejects external CTA buttons for API-key creates without external URL permission", async () => { vi.mocked(getExternalUrlsPermission).mockResolvedValue(false); const body = ZV3CreateSurveyBody.parse({ diff --git a/apps/web/app/api/v3/surveys/create.ts b/apps/web/app/api/v3/surveys/create.ts index 2cf4844b8cc7..e0759e6b4316 100644 --- a/apps/web/app/api/v3/surveys/create.ts +++ b/apps/web/app/api/v3/surveys/create.ts @@ -1,5 +1,7 @@ import "server-only"; +import { ZSurveyCreateInput } from "@formbricks/types/surveys/types"; import type { TSurvey, TSurveyCreateInput } from "@formbricks/types/surveys/types"; +import type { InvalidParam } from "@/app/api/v3/lib/response"; import type { TV3Authentication } from "@/app/api/v3/lib/types"; import { getActionClasses } from "@/lib/actionClass/service"; import { getOrganizationByWorkspaceId } from "@/lib/organization/service"; @@ -10,7 +12,7 @@ import { v3DistributionToScalars } from "./distribution"; import { type TV3SurveyLanguageRequest, ensureV3WorkspaceLanguages } from "./languages"; import { prepareV3SurveyCreate } from "./prepare"; import { V3SurveyReferenceValidationError } from "./reference-validation"; -import type { TV3CreateSurveyBody } from "./schemas"; +import { type TV3CreateSurveyBody, formatV3ZodInvalidParams } from "./schemas"; import { V3_CONTACTS_NOT_ENABLED_MESSAGE, assertV3SurveyTargetingFilterReferences, @@ -31,6 +33,29 @@ export class V3SurveyCreatePermissionError extends Error { } } +/** + * The create document passed this route's request schema but failed the stricter schema the survey + * service applies on write (`ZSurveyCreateInput` + `surveyRefinement`) — e.g. a CTA `buttonUrl` + * scheme the element schema allows but the refinement rejects. + * + * This is raised from an explicit pre-write parse rather than by catching `ValidationError` in the + * response mapper, and the distinction matters: `createSurvey` runs `validateInputs` again itself, + * and it also calls out to code *after* its transaction commits (`subscribeOrganizationMembers- + * ToSurveyResponses` → `updateUser`, which re-validates the user's stored `notificationSettings` + * JSON). A `ValidationError` from there means the survey row already exists, so answering it with a + * 4xx would tell the caller nothing was written and invite a duplicate retry. Anything that is not + * this error keeps its 500. + */ +export class V3SurveyInputValidationError extends Error { + invalidParams: InvalidParam[]; + + constructor(invalidParams: InvalidParam[]) { + super("Survey document failed validation"); + this.name = "V3SurveyInputValidationError"; + this.invalidParams = invalidParams; + } +} + function getCreatedBy(authentication: TV3Authentication): string | null { if (authentication && "user" in authentication && authentication.user?.id) { return authentication.user.id; @@ -184,6 +209,16 @@ export async function executeV3SurveyCreate(params: { ...surveyCreateInputOverrides, }; + // Run the survey service's own write schema here, before any DB work, so a document this route's + // request schema admits but `surveyRefinement` rejects (a CTA `buttonUrl` of `tel:…` being the + // known case) fails with a typed error the route can answer 422. `createSurvey` validates this + // same input again; catching its `ValidationError` instead would be wrong, because it also throws + // one from post-commit work where the survey row already exists. See V3SurveyInputValidationError. + const parsedCreateInput = ZSurveyCreateInput.safeParse(surveyCreateInput); + if (!parsedCreateInput.success) { + throw new V3SurveyInputValidationError(formatV3ZodInvalidParams(parsedCreateInput.error, "body")); + } + // App targeting filters are created atomically with the survey's private segment inside // `createSurvey` (a single transaction), so a failed targeting write can't leave a partial survey. const privateSegmentFilters = input.type === "app" ? (input.targeting?.filters ?? []) : []; diff --git a/apps/web/app/api/v3/surveys/lib/operations.test.ts b/apps/web/app/api/v3/surveys/lib/operations.test.ts index 54535e6c72b6..d2993ba43be0 100644 --- a/apps/web/app/api/v3/surveys/lib/operations.test.ts +++ b/apps/web/app/api/v3/surveys/lib/operations.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; -import { DatabaseError, ResourceNotFoundError } from "@formbricks/types/errors"; +import { DatabaseError, ResourceNotFoundError, ValidationError } from "@formbricks/types/errors"; import { requireV3WorkspaceAccess } from "@/app/api/v3/lib/auth"; import { problemForbidden } from "@/app/api/v3/lib/response"; import { capturePostHogEvent } from "@/lib/posthog"; @@ -7,7 +7,7 @@ import { archiveSurvey, deleteSurvey, restoreSurvey } from "@/modules/survey/lib import { getSurveyCount, hasArchivedSurveys } from "@/modules/survey/list/lib/survey"; import { getSurveyListPage } from "@/modules/survey/list/lib/survey-page"; import { getAuthorizedV3Survey } from "../authorization"; -import { V3SurveyCreatePermissionError, createV3Survey } from "../create"; +import { V3SurveyCreatePermissionError, V3SurveyInputValidationError, createV3Survey } from "../create"; import { parseV3SurveysListQuery } from "../parse-v3-surveys-list-query"; import { patchV3Survey } from "../patch"; import { prepareV3SurveyCreateInput, prepareV3SurveyPatchInput } from "../prepare"; @@ -483,6 +483,48 @@ describe("createV3SurveyResponse", () => { ).status ).toBe(500); }); + + // ENG-2587. The document passes `ZV3CreateSurveyBody` but fails the survey service's stricter + // write schema; `executeV3SurveyCreate` catches that with a pre-write parse and throws this typed + // error. Before the fix there was no branch for it and it landed on the generic 500. + test("maps V3SurveyInputValidationError to 422 naming the offending path", async () => { + vi.mocked(createV3Survey).mockRejectedValueOnce( + new V3SurveyInputValidationError([{ name: "blocks.0.elements.0.buttonUrl", reason: "Invalid url" }]) + ); + + const response = await createV3SurveyResponse({ + body: parsedCreateBody, + authentication, + requestId, + instance, + }); + + expect(response.status).toBe(422); + expect(await readJson(response)).toMatchObject({ + invalid_params: [expect.objectContaining({ name: "blocks.0.elements.0.buttonUrl" })], + }); + }); + + // The reason the branch above is keyed on the typed error and not on `ValidationError`: + // `createSurvey` also throws `ValidationError` from work that runs *after* its transaction + // commits (`subscribeOrganizationMembersToSurveyResponses` -> `updateUser` re-validates the + // user's stored `notificationSettings` JSON). The survey row exists by then, so answering 4xx + // would tell the caller nothing was written and invite a duplicate retry — and it would drop a + // genuine server fault out of 5xx alerting. Those must keep the 500. + test("keeps a bare ValidationError on the 500 path, so post-commit faults are not reported as 4xx", async () => { + vi.mocked(createV3Survey).mockRejectedValueOnce( + new ValidationError("Validation failed: notificationSettings.alertExpected boolean") + ); + + const response = await createV3SurveyResponse({ + body: parsedCreateBody, + authentication, + requestId, + instance, + }); + + expect(response.status).toBe(500); + }); }); describe("getV3Survey", () => { diff --git a/apps/web/app/api/v3/surveys/lib/operations.ts b/apps/web/app/api/v3/surveys/lib/operations.ts index d8d5b3fd3d62..acce705af0a2 100644 --- a/apps/web/app/api/v3/surveys/lib/operations.ts +++ b/apps/web/app/api/v3/surveys/lib/operations.ts @@ -20,7 +20,12 @@ import { archiveSurvey, deleteSurvey, restoreSurvey } from "@/modules/survey/lib import { getSurveyCount, hasArchivedSurveys } from "@/modules/survey/list/lib/survey"; import { getSurveyListPage } from "@/modules/survey/list/lib/survey-page"; import { getAuthorizedV3Survey } from "../authorization"; -import { type TV3SurveyCreateOptions, V3SurveyCreatePermissionError, createV3Survey } from "../create"; +import { + type TV3SurveyCreateOptions, + V3SurveyCreatePermissionError, + V3SurveyInputValidationError, + createV3Survey, +} from "../create"; import { parseV3SurveysListQuery } from "../parse-v3-surveys-list-query"; import { patchV3Survey } from "../patch"; import { @@ -259,6 +264,19 @@ function mapV3SurveyCreateError( instance, }); } + if (err instanceof V3SurveyInputValidationError) { + // The document passed `ZV3CreateSurveyBody` but failed the survey service's stricter write + // schema, caught by an explicit pre-write parse in `executeV3SurveyCreate`. Semantic, not + // malformed → 422, in line with the reference-validation branch above. Deliberately keyed on + // this typed error rather than on `ValidationError`: the latter is also thrown from work + // `createSurvey` does *after* its transaction commits, where a 4xx would wrongly tell the + // caller nothing was written. Those keep the 500 below. + log.warn({ statusCode: 422, invalidParams: err.invalidParams }, "Survey input validation failed"); + return problemUnprocessableContent(requestId, "Survey document failed validation", { + invalid_params: err.invalidParams, + instance, + }); + } if (err instanceof DatabaseError) { log.error({ error: err, statusCode: 500 }, "Database error"); return problemInternalError(requestId, "An unexpected error occurred.", instance); diff --git a/apps/web/app/api/v3/surveys/schemas.test.ts b/apps/web/app/api/v3/surveys/schemas.test.ts index 9d65d881c6c9..c0ac3afc6573 100644 --- a/apps/web/app/api/v3/surveys/schemas.test.ts +++ b/apps/web/app/api/v3/surveys/schemas.test.ts @@ -1,4 +1,6 @@ import { describe, expect, test } from "vitest"; +import { z } from "zod"; +import { validateElementLabels } from "@formbricks/types/surveys/elements-validation"; import { ZV3CreateSurveyBody, ZV3PatchSurveyBody, @@ -848,3 +850,105 @@ describe("ZV3PatchSurveyBody", () => { expect(parsed.targeting).toEqual({ filters: [] }); }); }); + +describe("formatV3ZodInvalidParams", () => { + // Built from the real validator rather than a literal message: the marker only reaches clients + // because these shared validators embed it, so a hand-written fixture would assert nothing. + const buildBlankTranslationIssue = (): z.core.$ZodIssue => { + const language = { + id: "clln1234567890123456789012", + code: "de-DE", + alias: null, + workspaceId: "clxx1234567890123456789012", + createdAt: new Date(), + updatedAt: new Date(), + }; + const issue = validateElementLabels( + "headline", + { default: "What should we improve?", "de-DE": " " }, + [ + { + language: { ...language, id: "clle1234567890123456789012", code: "en-US" }, + default: true, + enabled: true, + }, + { language, default: false, enabled: true }, + ], + 0, + 0 + ); + + if (!issue) { + throw new Error("expected the label validator to reject a blank translation"); + } + + return issue as z.core.$ZodIssue; + }; + + test("strips the editor-only -fLang- delimiter from the reason", () => { + const issue = buildBlankTranslationIssue(); + expect(issue.message).toContain("-fLang-"); + + const [invalidParam] = formatV3ZodInvalidParams(new z.ZodError([issue]), "body"); + + expect(invalidParam.reason).not.toContain("-fLang-"); + expect(invalidParam.reason).toBe( + "The question in question 1 of block 1 is missing for the following languages: de-DE" + ); + expect(invalidParam.name).toBe("blocks.0.elements.0.headline"); + }); + + test("leaves a reason without the delimiter untouched", () => { + const result = ZV3CreateSurveyBody.safeParse({ ...validCreateBody, defaultLanguage: "not a locale" }); + + expect(result.success).toBe(false); + if (!result.success) { + expect(formatV3ZodInvalidParams(result.error, "body")).toEqual( + expect.arrayContaining([ + expect.objectContaining({ reason: "Language 'not a locale' is not a valid locale code" }), + ]) + ); + } + }); + + test("normalizes the comma-joined ending-card shape too", () => { + const issue = { + code: "custom", + message: + "The button label on the Ending card 2 is missing for the following languages: -fLang- de, fr", + path: ["endings", 1, "buttonLabel"], + } as z.core.$ZodIssue; + + const [invalidParam] = formatV3ZodInvalidParams(new z.ZodError([issue]), "body"); + + expect(invalidParam.reason).toBe( + "The button label on the Ending card 2 is missing for the following languages: de, fr" + ); + }); + + // A caller-supplied language code reaches `reason` verbatim (createZV3SurveyLanguageTag interpolates + // it and only trims the ends), so a long interior whitespace run is reachable from one request. The + // previous `/\s*-fLang-\s*/` implementation was quadratic on exactly that input — ~5s server-side at + // 100k characters. The timeout is a complexity guard, not a benchmark: the linear implementation + // needs microseconds, so the margin here is several orders of magnitude. + test.each([ + ["no marker present", 100_000, false], + ["marker present", 100_000, true], + ])( + "stays linear on a %s whitespace run", + (_label, runLength, withMarker) => { + const run = " ".repeat(runLength); + const message = withMarker + ? `Language 'a${run}b' is not valid: -fLang- de-DE` + : `Language 'a${run}b' is not a valid locale code`; + const issue = { code: "custom", message, path: ["languages", 0, "code"] } as z.core.$ZodIssue; + + const [invalidParam] = formatV3ZodInvalidParams(new z.ZodError([issue]), "body"); + + // Whole expected string, not fragments: the caller's interior run survives untouched, only the + // marker and the whitespace adjoining it are collapsed, and a marker-free message is identical. + expect(invalidParam.reason).toBe(withMarker ? `Language 'a${run}b' is not valid: de-DE` : message); + }, + 2_000 + ); +}); diff --git a/apps/web/app/api/v3/surveys/schemas.ts b/apps/web/app/api/v3/surveys/schemas.ts index f4ad40ef059a..851c068fa1f1 100644 --- a/apps/web/app/api/v3/surveys/schemas.ts +++ b/apps/web/app/api/v3/surveys/schemas.ts @@ -1309,6 +1309,33 @@ export const ZV3SurveyValidationRequestBody = z.discriminatedUnion("operation", export const ZV3EmptyQuery = z.object({}).strict(); +/** + * `-fLang-` is an editor-only delimiter: the shared label validators embed it in the issue message so + * the editor can split the language list off for its toast (`survey-menu-bar.tsx`). v3 clients receive + * these messages verbatim as `invalid_params[].reason`, so the marker is stripped here rather than in + * the shared validators the editor still depends on. The suffix it precedes already ends in ": ", so + * the segments are trimmed and rejoined with a single space. + * + * Deliberately not a regex. Wrapping the literal in a `\s*` on each side is quadratic on a long + * whitespace run that contains no marker (Sonar S8786): the leading `\s*` matches to the end at every + * start position, then backtracks looking for a literal that is not there. That run is reachable from + * one request: `createZV3SurveyLanguageTag` interpolates the caller's raw language code into its + * message and `.trim()` strips only the ends, so interior whitespace arrives here. Splitting on the + * literal is linear by construction, with no bound to tune and nothing to backtrack. + */ +const FIELD_LANGUAGE_MARKER = "-fLang-"; + +function toV3InvalidParamReason(message: string): string { + if (!message.includes(FIELD_LANGUAGE_MARKER)) { + return message; + } + + return message + .split(FIELD_LANGUAGE_MARKER) + .map((segment) => segment.trim()) + .join(" "); +} + export function formatV3ZodInvalidParams(error: z.ZodError, fallbackName: string): InvalidParam[] { return error.issues.map((issue) => { const params = "params" in issue && isPlainObject(issue.params) ? issue.params : {}; @@ -1316,7 +1343,7 @@ export function formatV3ZodInvalidParams(error: z.ZodError, fallbackName: string return { name: issue.path.length > 0 ? issue.path.join(".") : fallbackName, - reason: issue.message, + reason: toV3InvalidParamReason(issue.message), ...(code ? { code } : {}), }; }); diff --git a/docs/api-v3-reference/openapi.yml b/docs/api-v3-reference/openapi.yml index 3437cee76fa9..05d71dec868a 100644 --- a/docs/api-v3-reference/openapi.yml +++ b/docs/api-v3-reference/openapi.yml @@ -448,7 +448,7 @@ paths: schema: $ref: '#/components/schemas/Problem' '422': - description: 'Unprocessable Content — the document passed schema validation but failed a cross-reference check that requires stored state to detect: duplicate stable ids, dangling logic/jump references, undeclared locale keys used in content, invalid media URLs, a `distribution.triggers[].actionClassId` referencing an action class that does not exist in the workspace (discover valid ids via `GET /api/v3/action-classes`), or a `targeting.filters` entry that cannot resolve in the workspace — an unknown contact-attribute key or segment, an unsupported person identifier, or an unknown device value. The `invalid_params` array pinpoints each issue (e.g. `code: invalid_reference` with the offending `identifier`).' + description: 'Unprocessable Content — the document passed schema validation but failed a cross-reference check that requires stored state to detect: duplicate stable ids, dangling logic/jump references, undeclared locale keys used in content, invalid media URLs, a `distribution.triggers[].actionClassId` referencing an action class that does not exist in the workspace (discover valid ids via `GET /api/v3/action-classes`), or a `targeting.filters` entry that cannot resolve in the workspace — an unknown contact-attribute key or segment, an unsupported person identifier, or an unknown device value. Also returned when the document passes this endpoint''s request schema but is rejected by the stricter validation the survey service applies on write — for example a CTA `buttonUrl` whose scheme the service does not accept. The `invalid_params` array pinpoints each issue (e.g. `code: invalid_reference` with the offending `identifier`).' content: application/problem+json: schema: diff --git a/docs/api-v3-reference/src/paths/api_v3_surveys.yml b/docs/api-v3-reference/src/paths/api_v3_surveys.yml index 744f2b447aa9..fea3116969b1 100644 --- a/docs/api-v3-reference/src/paths/api_v3_surveys.yml +++ b/docs/api-v3-reference/src/paths/api_v3_surveys.yml @@ -458,7 +458,10 @@ post: `distribution.triggers[].actionClassId` referencing an action class that does not exist in the workspace (discover valid ids via `GET /api/v3/action-classes`), or a `targeting.filters` entry that cannot resolve in the workspace — an unknown contact-attribute key or segment, an - unsupported person identifier, or an unknown device value. The `invalid_params` + unsupported person identifier, or an unknown device value. Also returned when the document + passes this endpoint's request schema but is rejected by the stricter validation the survey + service applies on write — for example a CTA `buttonUrl` whose scheme the service does not + accept. The `invalid_params` array pinpoints each issue (e.g. `code: invalid_reference` with the offending `identifier`). content: application/problem+json: From 27bd5b30716ca848366a431be12d5ab76cfabc41 Mon Sep 17 00:00:00 2001 From: Dhruwang Jariwala <67850763+Dhruwang@users.noreply.github.com> Date: Tue, 1 Sep 2026 11:48:41 +0000 Subject: [PATCH 2/5] feat(unify): add "Re-import historic data" to Formbricks survey sources (#8987) Co-authored-by: Claude --- apps/web/i18n.lock | 5 + apps/web/lib/feedback-source/import.test.ts | 28 ++++ apps/web/lib/feedback-source/import.ts | 7 +- apps/web/locales/de-DE.json | 5 + apps/web/locales/en-US.json | 5 + apps/web/locales/es-ES.json | 5 + apps/web/locales/fr-FR.json | 5 + apps/web/locales/hu-HU.json | 5 + apps/web/locales/ja-JP.json | 5 + apps/web/locales/nl-NL.json | 5 + apps/web/locales/pt-BR.json | 5 + apps/web/locales/pt-PT.json | 5 + apps/web/locales/ro-RO.json | 5 + apps/web/locales/ru-RU.json | 5 + apps/web/locales/sv-SE.json | 5 + apps/web/locales/tr-TR.json | 5 + apps/web/locales/zh-Hans-CN.json | 5 + apps/web/locales/zh-Hant-TW.json | 5 + .../create-feedback-source-modal.tsx | 21 ++- .../feedback-source-row-dropdown.tsx | 67 ++++++++ .../feedback-sources-page-client.tsx | 80 ++++++++- .../components/feedback-sources-table.tsx | 6 + .../ee/unify-feedback/sources/utils.test.ts | 146 ++++++++++++++++- .../ee/unify-feedback/sources/utils.ts | 84 ++++++++++ apps/web/modules/hub/hub-client-retry.test.ts | 155 ++++++++++++++++++ apps/web/modules/hub/hub-client.ts | 123 ++++++++++++++ 26 files changed, 781 insertions(+), 16 deletions(-) create mode 100644 apps/web/modules/hub/hub-client-retry.test.ts diff --git a/apps/web/i18n.lock b/apps/web/i18n.lock index 4726c5a91aab..8b166e79e12c 100644 --- a/apps/web/i18n.lock +++ b/apps/web/i18n.lock @@ -3947,6 +3947,11 @@ checksums: workspace/unify/question_type_not_supported: 8d9f7554e3b509dfd5307d8d1fef08d7 workspace/unify/refresh_feedback_records: c111751e02a7dee57390ed7fb79cfcc6 workspace/unify/refreshing_feedback_records: 2a03b44510ebe19eea6473639e9a7222 + workspace/unify/reimport_historic_data: 6ca7d6e7fe5bb36b74c00265571930c5 + workspace/unify/reimport_historic_data_confirmation: e13cd85831ae9a10302ec4b9f5e9679b + workspace/unify/reimport_historic_data_cta: 4d5b3cc074e0a1522683890714a7a583 + workspace/unify/reimport_historic_data_description: 56139cd54372b130dcf78514f8e028d5 + workspace/unify/reimport_no_survey_mapped: 1321ac60065188c5af6fae5aabc2c037 workspace/unify/request_feedback_source: 51045caa2c81dee971d23a1841d19a7e workspace/unify/save_changes: 53dd9f4f0a4accc822fa5c1f2f6d118a workspace/unify/select_a_survey_to_see_questions: 792eba3d2f6d210231a2266401111a20 diff --git a/apps/web/lib/feedback-source/import.test.ts b/apps/web/lib/feedback-source/import.test.ts index 35d6b39f24e1..ef1c535bc0ec 100644 --- a/apps/web/lib/feedback-source/import.test.ts +++ b/apps/web/lib/feedback-source/import.test.ts @@ -187,6 +187,34 @@ describe("importHistoricalResponses", () => { expect(result.skipped).toBe(0); }); + // `transformResponseToFeedbackRecords` filters on `m.surveyId === survey.id`, so counting every + // mapping the source holds would report the *other* survey's mappings as records that were + // expected and never arrived. No behavioural change while a source binds one survey; this pins it + // for the multi-survey source the schema already permits. + test("counts only this survey's mappings towards skipped", async () => { + const twoSurveySource: TFeedbackSourceWithMappings = { + ...mockFeedbackSource, + formbricksMappings: [ + mockFeedbackSource.formbricksMappings[0], + { + ...mockFeedbackSource.formbricksMappings[0], + id: "mapping-2", + surveyId: "clxxxxxxxxxxxxxxxx009", + elementId: "el-2", + }, + ], + }; + + getResponses.mockResolvedValueOnce([{ id: "r1" }] as never); + getResponses.mockResolvedValueOnce([]); + transformResponseToFeedbackRecords.mockReturnValueOnce([{ field: "record1" }] as never); + reconcileFeedbackRecords.mockResolvedValue({ created: 1, reconciled: 0, superseded: 0, failures: [] }); + + const result = await importHistoricalResponses(twoSurveySource, mockSurvey); + + expect(result).toEqual({ successes: 1, failures: 0, skipped: 0 }); + }); + test("paginates through responses in batches", async () => { const batch1 = Array.from({ length: 50 }, (_, i) => ({ id: `r${i}` })); const batch2 = [{ id: "r50" }]; diff --git a/apps/web/lib/feedback-source/import.ts b/apps/web/lib/feedback-source/import.ts index b7183d3a57e9..13f1dd8d7075 100644 --- a/apps/web/lib/feedback-source/import.ts +++ b/apps/web/lib/feedback-source/import.ts @@ -24,7 +24,12 @@ const processBatch = async ( ): Promise => { let successes = 0; let failures = 0; - const expectedRecords = responses.length * mappings.length; + // Only this survey's mappings count towards the expectation. `transformResponseToFeedbackRecords` + // filters on `m.surveyId === survey.id`, so counting every mapping the source holds would report + // the other surveys' mappings as `skipped` records that were never expected in the first place. + // No change while a source binds one survey; it keeps the number honest once one binds several. + const surveyMappings = mappings.filter((mapping) => mapping.surveyId === survey.id); + const expectedRecords = responses.length * surveyMappings.length; const allRecords = responses.flatMap((response) => { try { diff --git a/apps/web/locales/de-DE.json b/apps/web/locales/de-DE.json index 5b14b8bbad3a..921c4945c191 100644 --- a/apps/web/locales/de-DE.json +++ b/apps/web/locales/de-DE.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Dieser Fragetyp wird nicht unterstützt", "refresh_feedback_records": "Feedback-Einträge aktualisieren", "refreshing_feedback_records": "Feedback-Einträge werden aktualisiert...", + "reimport_historic_data": "Historische Daten erneut importieren", + "reimport_historic_data_confirmation": "Dadurch wird jede Antwort, die die verknüpfte Umfrage gesammelt hat, erneut in diese Quelle geschrieben. Geänderte Antworten werden aktualisiert und fehlende Datensätze hinzugefügt – nichts wird dupliziert, und die Importeinstellung der Quelle entscheidet weiterhin, ob unvollständige Antworten einbezogen werden. Fragebeschriftungen und -typen werden bei bereits importierten Datensätzen nicht neu geschrieben. Große Umfragen können eine Weile dauern.", + "reimport_historic_data_cta": "Erneut importieren", + "reimport_historic_data_description": "Du kannst das jederzeit erneut ausführen.", + "reimport_no_survey_mapped": "Diese Quelle hat keine verknüpfte Umfrage, daher gibt es nichts zu importieren.", "request_feedback_source": "Quellen-Integration anfragen", "save_changes": "Änderungen speichern", "select_a_survey_to_see_questions": "Wähle eine Umfrage aus, um ihre Fragen zu sehen", diff --git a/apps/web/locales/en-US.json b/apps/web/locales/en-US.json index 8183cfaaa1b8..ce4f8afa4274 100644 --- a/apps/web/locales/en-US.json +++ b/apps/web/locales/en-US.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "This question type is not supported", "refresh_feedback_records": "Refresh feedback records", "refreshing_feedback_records": "Refreshing feedback records...", + "reimport_historic_data": "Re-import historic data", + "reimport_historic_data_confirmation": "This reads every response the linked survey has collected and writes it into this source again. Answers that changed are updated and missing records are added — nothing is duplicated, and the source's import setting still decides whether partial responses are included. Question labels and types are not rewritten on records that were already imported. Large surveys can take a while.", + "reimport_historic_data_cta": "Re-import", + "reimport_historic_data_description": "This can be run again at any time.", + "reimport_no_survey_mapped": "This source has no survey mapped, so there is nothing to re-import.", "request_feedback_source": "Request source integration", "save_changes": "Save changes", "select_a_survey_to_see_questions": "Select a survey to see its questions", diff --git a/apps/web/locales/es-ES.json b/apps/web/locales/es-ES.json index 038833c53aa3..31f7b6da62b3 100644 --- a/apps/web/locales/es-ES.json +++ b/apps/web/locales/es-ES.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Este tipo de pregunta no es compatible", "refresh_feedback_records": "Actualizar los registros de comentarios", "refreshing_feedback_records": "Actualizando registros de comentarios...", + "reimport_historic_data": "Reimportar datos históricos", + "reimport_historic_data_confirmation": "Esto lee cada respuesta que la encuesta vinculada ha recopilado y la escribe de nuevo en esta fuente. Las respuestas que han cambiado se actualizan y se añaden los registros que faltan; nada se duplica, y la configuración de importación de la fuente sigue decidiendo si se incluyen las respuestas parciales. Las etiquetas y tipos de preguntas no se reescriben en los registros que ya fueron importados. Las encuestas grandes pueden tardar un rato.", + "reimport_historic_data_cta": "Reimportar", + "reimport_historic_data_description": "Esto se puede ejecutar de nuevo en cualquier momento.", + "reimport_no_survey_mapped": "Esta fuente no tiene ninguna encuesta asignada, así que no hay nada que reimportar.", "request_feedback_source": "Solicitar integración de fuente", "save_changes": "Guardar cambios", "select_a_survey_to_see_questions": "Selecciona una encuesta para ver sus preguntas", diff --git a/apps/web/locales/fr-FR.json b/apps/web/locales/fr-FR.json index b7dc57599a8b..06a82a1983d2 100644 --- a/apps/web/locales/fr-FR.json +++ b/apps/web/locales/fr-FR.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Ce type de question n'est pas pris en charge", "refresh_feedback_records": "Actualiser les enregistrements de retours", "refreshing_feedback_records": "Actualisation des enregistrements de feedback...", + "reimport_historic_data": "Réimporter les données historiques", + "reimport_historic_data_confirmation": "Cela lit chaque réponse collectée par l'enquête liée et l'enregistre à nouveau dans cette source. Les réponses modifiées sont mises à jour et les enregistrements manquants sont ajoutés — rien n'est dupliqué, et le paramètre d'importation de la source détermine toujours si les réponses partielles sont incluses. Les libellés et types de questions ne sont pas réécrits sur les enregistrements déjà importés. Les grandes enquêtes peuvent prendre un certain temps.", + "reimport_historic_data_cta": "Réimporter", + "reimport_historic_data_description": "Tu peux relancer cette action à tout moment.", + "reimport_no_survey_mapped": "Cette source n'a aucun questionnaire associé, il n'y a donc rien à réimporter.", "request_feedback_source": "Demander une intégration de source", "save_changes": "Enregistrer les modifications", "select_a_survey_to_see_questions": "Sélectionnez une enquête pour voir ses questions", diff --git a/apps/web/locales/hu-HU.json b/apps/web/locales/hu-HU.json index 2a3a14773e6c..12081d9c11da 100644 --- a/apps/web/locales/hu-HU.json +++ b/apps/web/locales/hu-HU.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Ez a kéréstípus nem támogatott", "refresh_feedback_records": "Visszajelzési rekordok frissítése", "refreshing_feedback_records": "Visszajelzési rekordok frissítése…", + "reimport_historic_data": "Történeti adatok újraimportálása", + "reimport_historic_data_confirmation": "Ez beolvassa a kapcsolt felmérés által összegyűjtött minden választ, és újra beírja azokat ebbe a forrásba. A megváltozott válaszok frissítésre kerülnek, a hiányzó rekordok hozzáadásra kerülnek — semmi sem duplikálódik, és a forrás importálási beállítása továbbra is meghatározza, hogy a részleges válaszok bekerülnek-e. A kérdések címkéi és típusai nem kerülnek újraírásra azokon a rekordokon, amelyek már importálásra kerültek. A nagy felmérések feldolgozása hosszabb időt vehet igénybe.", + "reimport_historic_data_cta": "Újraimportálás", + "reimport_historic_data_description": "Ez bármikor újrafuttatható.", + "reimport_no_survey_mapped": "Ehhez a forráshoz nincs hozzárendelve felmérés, így nincs mit újraimportálni.", "request_feedback_source": "Forrásintegráció kérése", "save_changes": "Változtatások mentése", "select_a_survey_to_see_questions": "Kérdőív kiválasztása a kérdései megtekintéséhez", diff --git a/apps/web/locales/ja-JP.json b/apps/web/locales/ja-JP.json index 9880321c659d..4844bb471ec9 100644 --- a/apps/web/locales/ja-JP.json +++ b/apps/web/locales/ja-JP.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "この質問タイプはサポートされていません", "refresh_feedback_records": "フィードバック記録を更新", "refreshing_feedback_records": "フィードバックレコードを更新中...", + "reimport_historic_data": "過去のデータを再インポート", + "reimport_historic_data_confirmation": "リンクされたアンケートが収集したすべての回答を読み取り、このソースに再度書き込みます。変更された回答は更新され、不足しているレコードは追加されます。重複は発生せず、部分的な回答を含めるかどうかはソースのインポート設定に従います。既にインポート済みのレコードの質問ラベルや種類は上書きされません。大規模なアンケートの場合、処理に時間がかかることがあります。", + "reimport_historic_data_cta": "再インポート", + "reimport_historic_data_description": "これはいつでも再実行できます。", + "reimport_no_survey_mapped": "このソースにはアンケートがマッピングされていないため、再インポートするものがありません。", "request_feedback_source": "ソース統合をリクエスト", "save_changes": "変更を保存", "select_a_survey_to_see_questions": "フォームを選択して質問を表示", diff --git a/apps/web/locales/nl-NL.json b/apps/web/locales/nl-NL.json index c5b102074015..6bb8c782e2ea 100644 --- a/apps/web/locales/nl-NL.json +++ b/apps/web/locales/nl-NL.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Dit vraagtype wordt niet ondersteund", "refresh_feedback_records": "Feedbackrecords verversen", "refreshing_feedback_records": "Feedbackrecords vernieuwen...", + "reimport_historic_data": "Historische gegevens opnieuw importeren", + "reimport_historic_data_confirmation": "Dit leest elke respons die de gekoppelde enquête heeft verzameld en schrijft deze opnieuw naar deze bron. Gewijzigde antwoorden worden bijgewerkt en ontbrekende records worden toegevoegd — er wordt niets gedupliceerd, en de importinstelling van de bron bepaalt nog steeds of gedeeltelijke responsen worden opgenomen. Vraagbenamingen en -typen worden niet herschreven voor records die al zijn geïmporteerd. Grote enquêtes kunnen even duren.", + "reimport_historic_data_cta": "Opnieuw importeren", + "reimport_historic_data_description": "Dit kan op elk moment opnieuw worden uitgevoerd.", + "reimport_no_survey_mapped": "Deze bron heeft geen gekoppelde enquête, dus er is niets om opnieuw te importeren.", "request_feedback_source": "Bronintegratie aanvragen", "save_changes": "Wijzigingen opslaan", "select_a_survey_to_see_questions": "Selecteer een enquête om de vragen te zien", diff --git a/apps/web/locales/pt-BR.json b/apps/web/locales/pt-BR.json index 119d0bfe33e4..10912feea88e 100644 --- a/apps/web/locales/pt-BR.json +++ b/apps/web/locales/pt-BR.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Este tipo de pergunta não é suportado", "refresh_feedback_records": "Atualizar registros de feedback", "refreshing_feedback_records": "Atualizando registros de feedback...", + "reimport_historic_data": "Reimportar dados históricos", + "reimport_historic_data_confirmation": "Isso lê todas as respostas que a pesquisa vinculada coletou e as grava novamente nesta fonte. Respostas que mudaram são atualizadas e registros faltantes são adicionados — nada é duplicado, e a configuração de importação da fonte ainda decide se respostas parciais são incluídas. Rótulos e tipos de perguntas não são regravados em registros que já foram importados. Pesquisas grandes podem demorar um pouco.", + "reimport_historic_data_cta": "Reimportar", + "reimport_historic_data_description": "Isso pode ser executado novamente a qualquer momento.", + "reimport_no_survey_mapped": "Esta fonte não tem uma pesquisa mapeada, então não há nada para reimportar.", "request_feedback_source": "Solicitar integração de fonte", "save_changes": "Salvar alterações", "select_a_survey_to_see_questions": "Selecione uma pesquisa para ver suas perguntas", diff --git a/apps/web/locales/pt-PT.json b/apps/web/locales/pt-PT.json index 4eaaff9f0b84..6b4ff2ee9186 100644 --- a/apps/web/locales/pt-PT.json +++ b/apps/web/locales/pt-PT.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Este tipo de pergunta não é suportado", "refresh_feedback_records": "Atualizar registos de feedback", "refreshing_feedback_records": "A atualizar registos de feedback...", + "reimport_historic_data": "Reimportar dados históricos", + "reimport_historic_data_confirmation": "Isto lê todas as respostas que o inquérito associado recolheu e volta a escrevê-las nesta fonte. As respostas que foram alteradas são atualizadas e os registos em falta são adicionados — nada é duplicado, e a definição de importação da fonte continua a decidir se as respostas parciais são incluídas. Os rótulos e tipos de perguntas não são reescritos nos registos que já foram importados. Inquéritos grandes podem demorar algum tempo.", + "reimport_historic_data_cta": "Reimportar", + "reimport_historic_data_description": "Isto pode ser executado novamente a qualquer momento.", + "reimport_no_survey_mapped": "Esta fonte não tem nenhum questionário associado, portanto não há nada para reimportar.", "request_feedback_source": "Solicitar integração de fonte", "save_changes": "Guardar alterações", "select_a_survey_to_see_questions": "Selecione um inquérito para ver as suas perguntas", diff --git a/apps/web/locales/ro-RO.json b/apps/web/locales/ro-RO.json index 61cbddc10f00..390ada0ae948 100644 --- a/apps/web/locales/ro-RO.json +++ b/apps/web/locales/ro-RO.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Acest tip de întrebare nu este suportat", "refresh_feedback_records": "Reîmprospătează înregistrările de feedback", "refreshing_feedback_records": "Se actualizează înregistrările de feedback...", + "reimport_historic_data": "Reimportă datele istorice", + "reimport_historic_data_confirmation": "Aceasta citește fiecare răspuns colectat de chestionarul conectat și îl scrie din nou în această sursă. Răspunsurile care s-au modificat sunt actualizate, iar înregistrările lipsă sunt adăugate — nimic nu este duplicat, iar setarea de import a sursei decide în continuare dacă răspunsurile parțiale sunt incluse. Etichetele și tipurile de întrebări nu sunt rescrise pe înregistrările deja importate. Chestionarele mari pot dura ceva timp.", + "reimport_historic_data_cta": "Reimportă", + "reimport_historic_data_description": "Acest lucru poate fi executat din nou oricând.", + "reimport_no_survey_mapped": "Această sursă nu are niciun sondaj asociat, așa că nu există nimic de reimportat.", "request_feedback_source": "Solicită integrarea sursei", "save_changes": "Salvează modificările", "select_a_survey_to_see_questions": "Selectează un chestionar pentru a vedea întrebările", diff --git a/apps/web/locales/ru-RU.json b/apps/web/locales/ru-RU.json index 799105c71a78..1a7756837918 100644 --- a/apps/web/locales/ru-RU.json +++ b/apps/web/locales/ru-RU.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Этот тип вопроса не поддерживается", "refresh_feedback_records": "Обновить записи отзывов", "refreshing_feedback_records": "Обновляем записи отзывов...", + "reimport_historic_data": "Повторный импорт исторических данных", + "reimport_historic_data_confirmation": "Эта операция читает каждый ответ, собранный связанным опросом, и записывает его в этот источник заново. Изменённые ответы обновляются, а отсутствующие записи добавляются — ничего не дублируется, и настройка импорта источника по-прежнему определяет, включаются ли частичные ответы. Метки и типы вопросов не перезаписываются в записях, которые уже были импортированы. Обработка больших опросов может занять некоторое время.", + "reimport_historic_data_cta": "Повторно импортировать", + "reimport_historic_data_description": "Это можно запустить снова в любое время.", + "reimport_no_survey_mapped": "К этому источнику не привязан опрос, поэтому нечего импортировать повторно.", "request_feedback_source": "Запросить интеграцию источника", "save_changes": "Сохранить изменения", "select_a_survey_to_see_questions": "Выберите опрос, чтобы увидеть его вопросы", diff --git a/apps/web/locales/sv-SE.json b/apps/web/locales/sv-SE.json index 82534a53f2d5..636ac3b518f6 100644 --- a/apps/web/locales/sv-SE.json +++ b/apps/web/locales/sv-SE.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Den här frågetypen stöds inte", "refresh_feedback_records": "Uppdatera feedbackposter", "refreshing_feedback_records": "Uppdaterar feedbackposter...", + "reimport_historic_data": "Återimportera historisk data", + "reimport_historic_data_confirmation": "Detta läser varje svar som den länkade enkäten har samlat in och skriver in det i den här källan igen. Svar som har ändrats uppdateras och saknade poster läggs till — inget dupliceras, och källans importinställning avgör fortfarande om ofullständiga svar inkluderas. Frågetiketter och typer skrivs inte om på poster som redan har importerats. Stora enkäter kan ta en stund.", + "reimport_historic_data_cta": "Återimportera", + "reimport_historic_data_description": "Detta kan köras igen när som helst.", + "reimport_no_survey_mapped": "Denna källa har ingen enkät kopplad, så det finns inget att återimportera.", "request_feedback_source": "Request source integration", "save_changes": "Spara ändringar", "select_a_survey_to_see_questions": "Välj en enkät för att se dess frågor", diff --git a/apps/web/locales/tr-TR.json b/apps/web/locales/tr-TR.json index f57c2ea42bb9..065f1cdf2432 100644 --- a/apps/web/locales/tr-TR.json +++ b/apps/web/locales/tr-TR.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "Bu soru türü desteklenmiyor", "refresh_feedback_records": "Geri bildirim kayıtlarını yenile", "refreshing_feedback_records": "Geri bildirim kayıtları yenileniyor...", + "reimport_historic_data": "Geçmiş verileri yeniden içe aktar", + "reimport_historic_data_confirmation": "Bu işlem, bağlı anketin topladığı tüm yanıtları okur ve bu kaynağa yeniden yazar. Değişen yanıtlar güncellenir ve eksik kayıtlar eklenir — hiçbir şey kopyalanmaz ve kaynağın içe aktarma ayarı kısmi yanıtların dahil edilip edilmeyeceğine hâlâ karar verir. Soru etiketleri ve türleri, önceden içe aktarılmış kayıtlarda yeniden yazılmaz. Büyük anketler biraz zaman alabilir.", + "reimport_historic_data_cta": "Yeniden içe aktar", + "reimport_historic_data_description": "Bu işlem istediğin zaman tekrar çalıştırılabilir.", + "reimport_no_survey_mapped": "Bu kaynakla eşleştirilmiş bir anket yok, bu yüzden yeniden içe aktarılacak bir şey yok.", "request_feedback_source": "Request source integration", "save_changes": "Değişiklikleri kaydet", "select_a_survey_to_see_questions": "Sorularını görmek için bir anket seç", diff --git a/apps/web/locales/zh-Hans-CN.json b/apps/web/locales/zh-Hans-CN.json index 24a2a655c022..50205a75e0d1 100644 --- a/apps/web/locales/zh-Hans-CN.json +++ b/apps/web/locales/zh-Hans-CN.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "不支持此问题类型", "refresh_feedback_records": "刷新反馈记录", "refreshing_feedback_records": "正在刷新反馈记录…", + "reimport_historic_data": "重新导入历史数据", + "reimport_historic_data_confirmation": "此操作会读取关联问卷收集的所有回复，并将其重新写入此数据源。已更改的答案会被更新，缺失的记录会被添加——不会产生重复数据，且数据源的导入设置仍决定是否包含部分回复。已导入记录的问题标签和类型不会被重写。大型问卷可能需要一些时间。", + "reimport_historic_data_cta": "重新导入", + "reimport_historic_data_description": "你可以随时再次运行。", + "reimport_no_survey_mapped": "此数据源未映射问卷，因此没有可重新导入的内容。", "request_feedback_source": "Request source integration", "save_changes": "保存更改", "select_a_survey_to_see_questions": "请选择一个调查以查看其问题", diff --git a/apps/web/locales/zh-Hant-TW.json b/apps/web/locales/zh-Hant-TW.json index 29bde03f999a..b09e118fb172 100644 --- a/apps/web/locales/zh-Hant-TW.json +++ b/apps/web/locales/zh-Hant-TW.json @@ -4107,6 +4107,11 @@ "question_type_not_supported": "不支援此題型", "refresh_feedback_records": "重新整理回饋紀錄", "refreshing_feedback_records": "正在更新回饋紀錄…", + "reimport_historic_data": "重新匯入歷史資料", + "reimport_historic_data_confirmation": "這會讀取已連結問卷收集到的每一個回應，並再次寫入此資料來源。變更的答案會更新，遺漏的記錄會新增——不會產生重複資料，而且資料來源的匯入設定仍會決定是否包含部分回應。問題標籤和類型不會在已匯入的記錄上重新寫入。大型問卷可能需要一些時間。", + "reimport_historic_data_cta": "重新匯入", + "reimport_historic_data_description": "你可以隨時再次執行此操作。", + "reimport_no_survey_mapped": "此資料來源尚未對應問卷，因此無法重新匯入。", "request_feedback_source": "申請新增來源整合", "save_changes": "儲存變更", "select_a_survey_to_see_questions": "請選擇問卷以查看其問題", diff --git a/apps/web/modules/ee/unify-feedback/sources/components/create-feedback-source-modal.tsx b/apps/web/modules/ee/unify-feedback/sources/components/create-feedback-source-modal.tsx index 78c6448bdbd0..81edb849a816 100644 --- a/apps/web/modules/ee/unify-feedback/sources/components/create-feedback-source-modal.tsx +++ b/apps/web/modules/ee/unify-feedback/sources/components/create-feedback-source-modal.tsx @@ -59,6 +59,7 @@ import { areAllRequiredCsvFieldsMapped, getSelectableQuestionIds, isFeedbackSourceNameValid, + notifyImportResult, toggleQuestionId, validateEnumMappings, } from "../utils"; @@ -344,14 +345,19 @@ export const CreateFeedbackSourceModal = ({ }); if (importResult?.data) { - showFeedbackRecordsSuccessToast( + // Picks the toast by `failures` rather than by the action resolving. The success toast + // carries a link to the feedback records, so a green one on a run that wrote nothing does + // not just misreport — it invites the user to go and look for records that are not there. + const imported = notifyImportResult( + importResult.data, t("workspace.unify.historical_import_complete", { successes: importResult.data.successes, failures: importResult.data.failures, skipped: importResult.data.skipped, - }) + }), + showFeedbackRecordsSuccessToast ); - return "success"; + return imported ? "success" : "error"; } toast.error(getFormattedErrorMessage(importResult)); @@ -376,14 +382,17 @@ export const CreateFeedbackSourceModal = ({ }); if (importResult?.data) { - showFeedbackRecordsSuccessToast( + // Same shape as the historical import above: `failures` decides the toast. + const imported = notifyImportResult( + importResult.data, t("workspace.unify.csv_import_complete", { successes: importResult.data.successes, failures: importResult.data.failures, skipped: importResult.data.skipped, - }) + }), + showFeedbackRecordsSuccessToast ); - return "success"; + return imported ? "success" : "error"; } toast.error( diff --git a/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-row-dropdown.tsx b/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-row-dropdown.tsx index 6fa38de0451e..b06006cd6cc3 100644 --- a/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-row-dropdown.tsx +++ b/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-row-dropdown.tsx @@ -6,6 +6,7 @@ import { MoreVertical, PauseIcon, PlayIcon, + RefreshCwIcon, SquarePenIcon, TrashIcon, } from "lucide-react"; @@ -13,6 +14,7 @@ import { useRouter } from "next/navigation"; import { useState } from "react"; import { useTranslation } from "react-i18next"; import { TFeedbackSourceWithMappings } from "@formbricks/types/feedback-source"; +import { ConfirmationModal } from "@/modules/ui/components/confirmation-modal"; import { DeleteDialog } from "@/modules/ui/components/delete-dialog"; import { DropdownMenu, @@ -22,11 +24,13 @@ import { DropdownMenuSeparator, DropdownMenuTrigger, } from "@/modules/ui/components/dropdown-menu"; +import { canReimportHistoricalData } from "../utils"; interface FeedbackSourceRowDropdownProps { feedbackSource: TFeedbackSourceWithMappings; onEdit: () => void; onCsvImport?: () => void; + onReimport: () => Promise; onToggleStatus: () => Promise; onDelete: () => Promise; } @@ -35,19 +39,39 @@ export function FeedbackSourceRowDropdown({ feedbackSource, onEdit, onCsvImport, + onReimport, onToggleStatus, onDelete, }: Readonly) { const router = useRouter(); const { t } = useTranslation(); const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false); + const [isReimportDialogOpen, setIsReimportDialogOpen] = useState(false); const [isDropDownOpen, setIsDropDownOpen] = useState(false); const [isDeleting, setIsDeleting] = useState(false); + const [isReimporting, setIsReimporting] = useState(false); const isActive = feedbackSource.status === "active"; + const canReimport = canReimportHistoricalData(feedbackSource); const linkedSurveyId = feedbackSource.type === "formbricks_survey" ? feedbackSource.formbricksMappings[0]?.surveyId : undefined; + const handleReimport = async () => { + // The confirm button is already disabled while the import runs (Button applies + // `disabled={loading || disabled}` to `buttonLoading`), so a second run cannot be started from + // the dialog today. Guarding here as well keeps that invariant in this component instead of + // resting on the shared Button's loading behaviour. + if (isReimporting) return; + + setIsReimporting(true); + try { + await onReimport(); + } finally { + setIsReimporting(false); + setIsReimportDialogOpen(false); + } + }; + const handleDelete = async () => { setIsDeleting(true); try { @@ -89,6 +113,29 @@ export function FeedbackSourceRowDropdown({ )} + {canReimport && ( + <> + {/* Disabled while a run is in flight: re-opening the dialog would show a spinning, + disabled Re-import button with nothing explaining why, and the in-flight run's + `finally` would then close the dialog the user had just re-opened. */} + + + + + + )} + {linkedSurveyId && ( <> @@ -155,6 +202,26 @@ export function FeedbackSourceRowDropdown({ + + { + const { successes, failures, skipped } = totals; + notifyImportResult( + totals, + t("workspace.unify.historical_import_complete", { successes, failures, skipped }) + ); + }; + + /** + * Replays the linked survey's historic responses into this source again, so questions added to the + * mapping after the first import no longer need the source deleted and rebuilt (ENG-1889). + * + * Safe to run repeatedly: `importHistoricalResponses` reconciles rather than inserts, and carries + * a `snapshotAt` so a record the live pipeline has since corrected is not reverted to this older + * copy. It also honours the source's saved `importMode`, so a re-import never widens a + * completed-only source to partials. + * + * What it does NOT repair: a question whose mapped `hubFieldType` changed. `field_type` and + * `field_label` identify a record on create and are absent from Hub's update request (see + * `UPDATE_FIELD_KEYS` in `lib/feedback-source/reconcile.ts`), so an already-imported record keeps + * its original type and only gains the new value column. Fixing that belongs in `reconcile.ts`. + */ + const handleReimportHistoricalData = async (feedbackSource: TFeedbackSourceWithMappings): Promise => { + const surveyIds = getMappedSurveyIds(feedbackSource); + // Unreachable from the menu, which renders the item only under `canReimportHistoricalData` — + // itself this same predicate. Kept so a future caller of `onReimport` that does not gate fails + // loudly instead of running an import that reports "0 succeeded". + if (surveyIds.length === 0) { + toast.error(t("workspace.unify.reimport_no_survey_mapped")); + return; + } + + try { + const totals: TFeedbackImportTotals[] = []; + // Sequential rather than concurrent: each import walks every response of a survey in batches + // and writes them into the same feedback directory, so running them in parallel would only + // multiply the load on one directory. Normally there is a single survey anyway. + for (const surveyId of surveyIds) { + const importResult = await importHistoricalResponsesAction({ + feedbackSourceId: feedbackSource.id, + workspaceId, + surveyId, + }); + + if (!importResult?.data) { + toast.error(getTranslatedFeedbackSourceError(getFormattedErrorMessage(importResult), t)); + return; + } + + totals.push(importResult.data); + } + + announceImportResult(sumImportTotals(totals)); + } catch { + toast.error(t("common.something_went_wrong")); + return; + } + + router.refresh(); + }; + const handleToggleStatus = async (feedbackSource: TFeedbackSourceWithMappings): Promise => { const newStatus = feedbackSource.status === "active" ? "paused" : "active"; const result = await updateFeedbackSourceWithMappingsAction({ @@ -272,6 +335,7 @@ export function FeedbackSourcesSection({ workspaceId={workspaceId} onFeedbackSourceClick={setEditingFeedbackSource} onCsvImport={setCsvImportFeedbackSource} + onReimport={handleReimportHistoricalData} onToggleStatus={handleToggleStatus} onDelete={handleDeleteFeedbackSource} onImportResponses={handleImportResponses} diff --git a/apps/web/modules/ee/unify-feedback/sources/components/feedback-sources-table.tsx b/apps/web/modules/ee/unify-feedback/sources/components/feedback-sources-table.tsx index 723582569900..347a09d0e9c8 100644 --- a/apps/web/modules/ee/unify-feedback/sources/components/feedback-sources-table.tsx +++ b/apps/web/modules/ee/unify-feedback/sources/components/feedback-sources-table.tsx @@ -51,6 +51,7 @@ const getFeedbackSourceColumns = ({ locale, surveyNameById, onCsvImport, + onReimport, onToggleStatus, onDelete, onFeedbackSourceClick, @@ -60,6 +61,7 @@ const getFeedbackSourceColumns = ({ locale: string; surveyNameById: Record; onCsvImport: (feedbackSource: TFeedbackSourceWithMappings) => void; + onReimport: (feedbackSource: TFeedbackSourceWithMappings) => Promise; onToggleStatus: (feedbackSource: TFeedbackSourceWithMappings) => Promise; onDelete: (feedbackSourceId: string) => Promise; onFeedbackSourceClick: (feedbackSource: TFeedbackSourceWithMappings) => void; @@ -177,6 +179,7 @@ const getFeedbackSourceColumns = ({ feedbackSource={feedbackSource} onEdit={() => onFeedbackSourceClick(feedbackSource)} onCsvImport={feedbackSource.type === "csv" ? () => onCsvImport(feedbackSource) : undefined} + onReimport={() => onReimport(feedbackSource)} onToggleStatus={() => onToggleStatus(feedbackSource)} onDelete={() => onDelete(feedbackSource.id)} /> @@ -194,6 +197,7 @@ interface FeedbackSourcesTableProps { workspaceId: string; onFeedbackSourceClick: (feedbackSource: TFeedbackSourceWithMappings) => void; onCsvImport: (feedbackSource: TFeedbackSourceWithMappings) => void; + onReimport: (feedbackSource: TFeedbackSourceWithMappings) => Promise; onToggleStatus: (feedbackSource: TFeedbackSourceWithMappings) => Promise; onDelete: (feedbackSourceId: string) => Promise; onImportResponses: (survey: TUnifySurvey) => Promise; @@ -209,6 +213,7 @@ export function FeedbackSourcesTable({ workspaceId, onFeedbackSourceClick, onCsvImport, + onReimport, onToggleStatus, onDelete, onImportResponses, @@ -237,6 +242,7 @@ export function FeedbackSourcesTable({ locale: i18n.language, surveyNameById, onCsvImport, + onReimport, onToggleStatus, onDelete, onFeedbackSourceClick, diff --git a/apps/web/modules/ee/unify-feedback/sources/utils.test.ts b/apps/web/modules/ee/unify-feedback/sources/utils.test.ts index 79a021eff4ac..08bebbb46ff9 100644 --- a/apps/web/modules/ee/unify-feedback/sources/utils.test.ts +++ b/apps/web/modules/ee/unify-feedback/sources/utils.test.ts @@ -1,4 +1,5 @@ -import { describe, expect, test } from "vitest"; +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { TFeedbackSourceWithMappings } from "@formbricks/types/feedback-source"; import { CSV_HIDDEN_STATIC_MAPPINGS, MAX_CSV_VALUES, @@ -9,18 +10,28 @@ import { import { areAllRequiredCsvFieldsMapped, autoMapCsvSourceFields, + canReimportHistoricalData, getCsvIdentityMappingAlert, getFeedbackSourceOptions, + getMappedSurveyIds, getSuggestedSurveys, inferFieldType, isCsvUserDefinedStaticValueMapping, isFeedbackSourceNameValid, + notifyImportResult, parseCSVColumnsToFields, + sumImportTotals, titleizeFromFileName, toggleQuestionId, validateCsvFile, } from "./utils"; +vi.mock("react-hot-toast", () => ({ + toast: { success: vi.fn(), error: vi.fn() }, +})); + +const { toast } = await import("react-hot-toast"); + const mockT = (key: string) => key; const makeUnifySurvey = (overrides: Partial = {}): TUnifySurvey => ({ @@ -617,3 +628,136 @@ describe("toggleQuestionId", () => { expect(toggleQuestionId(["a", "b", "a"], "a")).toEqual(["b"]); }); }); + +describe("re-import historic data", () => { + const buildSource = (overrides: Partial = {}): TFeedbackSourceWithMappings => + ({ + id: "fs1", + type: "formbricks_survey", + status: "active", + formbricksMappings: [], + ...overrides, + }) as TFeedbackSourceWithMappings; + + const mapping = (surveyId: string, elementId: string) => + ({ surveyId, elementId }) as TFeedbackSourceWithMappings["formbricksMappings"][number]; + + describe("getMappedSurveyIds", () => { + test("collapses the one-row-per-question mappings to a single survey", () => { + const source = buildSource({ + formbricksMappings: [mapping("survey1", "q1"), mapping("survey1", "q2"), mapping("survey1", "q3")], + }); + + expect(getMappedSurveyIds(source)).toEqual(["survey1"]); + }); + + test("returns every distinct survey rather than only the first", () => { + const source = buildSource({ + formbricksMappings: [mapping("survey1", "q1"), mapping("survey2", "q1"), mapping("survey1", "q2")], + }); + + expect(getMappedSurveyIds(source)).toEqual(["survey1", "survey2"]); + }); + + test("returns nothing when the source has no mappings", () => { + expect(getMappedSurveyIds(buildSource())).toEqual([]); + }); + }); + + describe("canReimportHistoricalData", () => { + test("applies to a Formbricks source with a mapped survey", () => { + expect(canReimportHistoricalData(buildSource({ formbricksMappings: [mapping("survey1", "q1")] }))).toBe( + true + ); + }); + + test("does not apply to a Formbricks source with nothing mapped", () => { + expect(canReimportHistoricalData(buildSource())).toBe(false); + }); + + test("does not apply to a CSV source, which importHistoricalResponses rejects", () => { + const csvSource = buildSource({ type: "csv", formbricksMappings: [mapping("survey1", "q1")] }); + + expect(canReimportHistoricalData(csvSource)).toBe(false); + }); + + test("does not apply to a paused source, whose owner switched off writes to the directory", () => { + const paused = buildSource({ status: "paused", formbricksMappings: [mapping("survey1", "q1")] }); + + expect(canReimportHistoricalData(paused)).toBe(false); + }); + + test("does not apply to an errored source, which the live pipeline also skips", () => { + const errored = buildSource({ status: "error", formbricksMappings: [mapping("survey1", "q1")] }); + + expect(canReimportHistoricalData(errored)).toBe(false); + }); + }); + + describe("sumImportTotals", () => { + test("adds each count across surveys", () => { + expect( + sumImportTotals([ + { successes: 12, failures: 1, skipped: 3 }, + { successes: 5, failures: 0, skipped: 2 }, + ]) + ).toEqual({ successes: 17, failures: 1, skipped: 5 }); + }); + + test("passes a single result through unchanged", () => { + expect(sumImportTotals([{ successes: 7, failures: 2, skipped: 1 }])).toEqual({ + successes: 7, + failures: 2, + skipped: 1, + }); + }); + + test("is zero for no results", () => { + expect(sumImportTotals([])).toEqual({ successes: 0, failures: 0, skipped: 0 }); + }); + }); +}); + +describe("notifyImportResult", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + // A Hub outage does not throw: per-record errors are folded into `failures` and the action + // resolves, so "resolved" is not evidence anything was written. + test("reports an import that wrote nothing as a failure", () => { + const imported = notifyImportResult({ successes: 0, failures: 412, skipped: 0 }, "message"); + + expect(imported).toBe(false); + expect(toast.error).toHaveBeenCalledWith("message"); + expect(toast.success).not.toHaveBeenCalled(); + }); + + test("reports a partial failure as a failure too", () => { + const imported = notifyImportResult({ successes: 400, failures: 12, skipped: 0 }, "message"); + + expect(imported).toBe(false); + expect(toast.error).toHaveBeenCalledWith("message"); + }); + + test("reports a clean run as a success", () => { + const imported = notifyImportResult({ successes: 6, failures: 0, skipped: 2 }, "message"); + + expect(imported).toBe(true); + expect(toast.success).toHaveBeenCalledWith("message"); + expect(toast.error).not.toHaveBeenCalled(); + }); + + // The create modal's success toast carries a link to the feedback records. Routing a failure + // through it would invite the user to go and look at records that were never written. + test("uses the caller's success toast only when the import succeeded", () => { + const showSuccess = vi.fn(); + + notifyImportResult({ successes: 6, failures: 0, skipped: 0 }, "ok", showSuccess); + notifyImportResult({ successes: 0, failures: 6, skipped: 0 }, "failed", showSuccess); + + expect(showSuccess).toHaveBeenCalledTimes(1); + expect(showSuccess).toHaveBeenCalledWith("ok"); + expect(toast.error).toHaveBeenCalledWith("failed"); + }); +}); diff --git a/apps/web/modules/ee/unify-feedback/sources/utils.ts b/apps/web/modules/ee/unify-feedback/sources/utils.ts index 7b7086e450c6..ad3c77650d45 100644 --- a/apps/web/modules/ee/unify-feedback/sources/utils.ts +++ b/apps/web/modules/ee/unify-feedback/sources/utils.ts @@ -1,6 +1,8 @@ import { TFunction } from "i18next"; +import { toast } from "react-hot-toast"; import { TFeedbackSourceType, + TFeedbackSourceWithMappings, THubFieldType, UNSUPPORTED_FEEDBACK_SOURCE_ELEMENT_TYPES, ZHubFieldType, @@ -38,6 +40,88 @@ export const getSelectableQuestionIds = (survey: TUnifySurvey): string[] => ) .map((element) => element.id); +/** + * Distinct surveys a Formbricks source replays responses from. + * + * `formbricksMappings` holds one row per mapped question, so a source covering five questions has + * five rows all naming the same survey — hence the dedupe. It reads every distinct survey rather + * than assuming the first because the schema does not forbid more than one: + * `@@unique([workspaceId, feedbackSourceId, surveyId, elementId])`. Today's create dialog only + * ever binds one survey, so this normally returns a single id; taking `[0]` instead would turn a + * future multi-survey source into a silent partial re-import. + */ +export const getMappedSurveyIds = (feedbackSource: TFeedbackSourceWithMappings): string[] => [ + ...new Set(feedbackSource.formbricksMappings.map((mapping) => mapping.surveyId)), +]; + +/** + * Whether "Re-import historic data" applies to a source. + * + * Only Formbricks sources replay responses (`importHistoricalResponses` rejects every other type), + * and only one that names a survey has anything to replay from. + * + * `status` is part of the gate because pausing a source is the user's switch for stopping it + * writing into its feedback directory — the live pipeline honours it (`getFeedbackSourcesBySurveyId` + * filters `status: "active"`). Neither the action nor the import re-checks status, so without this + * the menu would offer to replay a whole response history into a directory the user had just + * paused the source to keep out of. Create-time import could never reach that case: a source is + * active the moment it is created. + */ +export const canReimportHistoricalData = (feedbackSource: TFeedbackSourceWithMappings): boolean => + feedbackSource.type === "formbricks_survey" && + feedbackSource.status === "active" && + getMappedSurveyIds(feedbackSource).length > 0; + +/** + * Counts from a historical import. Structurally identical to `TImportResult` in + * `lib/feedback-source/import.ts`, redeclared here because that module is `server-only` and this + * one is reached from client components. + */ +export interface TFeedbackImportTotals { + successes: number; + failures: number; + skipped: number; +} + +/** One set of totals for a source, however many surveys it replayed. */ +export const sumImportTotals = (totals: TFeedbackImportTotals[]): TFeedbackImportTotals => + totals.reduce( + (acc, next) => ({ + successes: acc.successes + next.successes, + failures: acc.failures + next.failures, + skipped: acc.skipped + next.skipped, + }), + { successes: 0, failures: 0, skipped: 0 } + ); + +/** + * Announce an import's totals, picking the toast by `failures` rather than by whether the action + * resolved. + * + * A failed import does not throw: `reconcileFeedbackRecords` folds per-record errors into + * `failures` and the action resolves normally, so a Hub outage returns `{ successes: 0, + * failures: N }`. Reporting that as a green toast made a run that wrote nothing look identical to + * the happy path. + * + * `showSuccess` is how the create modal keeps its "Feedback records" link on the success toast — + * a green toast inviting the user to go and look at records that were never written is the sharper + * half of the same bug. Every import site passes the same `message` it would have shown anyway, + * so the CSV and historical wordings stay distinct. + */ +export const notifyImportResult = ( + totals: TFeedbackImportTotals, + message: string, + showSuccess: (message: string) => void = toast.success +): boolean => { + if (totals.failures > 0) { + toast.error(message); + return false; + } + + showSuccess(message); + return true; +}; + export type TFeedbackSourceOptionId = TFeedbackSourceType | "api_ingestion" | "feedback_record_mcp"; export interface TFeedbackSourceOption { diff --git a/apps/web/modules/hub/hub-client-retry.test.ts b/apps/web/modules/hub/hub-client-retry.test.ts new file mode 100644 index 000000000000..2759da727cec --- /dev/null +++ b/apps/web/modules/hub/hub-client-retry.test.ts @@ -0,0 +1,155 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import type { FeedbackRecordCreateParams } from "@/modules/hub/types"; + +/** + * The retry narrowing, driven through the real SDK. + * + * `hub-client.test.ts` mocks `@formbricks/hub`, which is the right shape for the `getHubClient` + * caching tests but replaces the very thing this behaviour depends on: that the SDK's request loop + * calls `shouldRetry` and honours a `false`. `shouldRetry` is `private` in the SDK's types, so the + * override is installed on the prototype — nothing in the type system says it is still wired up. + * This file therefore leaves the SDK real and stubs only the transport, so the assertion is on the + * number of POSTs actually attempted. + * + * The same reasoning as `assertRepeatedArrayParams`: an SDK release that renames or stops calling + * the hook is legal TypeScript and silently restores the retry storm. + */ +vi.mock("server-only", () => ({})); + +vi.mock("@/lib/env", () => ({ + env: { HUB_API_KEY: "test-key", HUB_API_URL: "https://hub.test" }, +})); + +const globalForHub = globalThis as unknown as { formbricksHubClientRepeatArrays: unknown }; + +const record = { tenant_id: "t1", submission_id: "s1", field_id: "q1" } as FeedbackRecordCreateParams; + +/** + * A stub transport answering with one RFC 9457 problem body. + * + * `code` is the member the narrowing reads, so it is a parameter rather than part of a fixed body: + * on a 409 it is the entire difference between the duplicate row and a draining tenant purge. + * + * `retry-after-ms: 0` is honoured ahead of the SDK's exponential backoff, so a retrying case costs + * no wall clock here. Without it the 429 test would sleep ~1.5s to prove the same thing. + */ +const respondWith = (status: number, code?: string): typeof fetch => + vi.fn(async (input: RequestInfo | URL) => { + const response = new Response(JSON.stringify({ detail: "stubbed", ...(code ? { code } : {}) }), { + status, + headers: { "content-type": "application/problem+json", "retry-after-ms": "0" }, + }); + // A hand-built `Response` has an empty `url`; a real fetch populates it from the request, which + // is what the narrowing reads to tell the create apart from the other operations on this path. + Object.defineProperty(response, "url", { + value: typeof input === "string" ? input : input instanceof URL ? input.href : input.url, + }); + return response; + }) as unknown as typeof fetch; + +const getClient = async () => { + const { getHubClient } = await import("./hub-client"); + const client = getHubClient(); + if (!client) throw new Error("expected a client"); + return client; +}; + +describe("Hub client retries, through the SDK's own request loop", () => { + beforeEach(() => { + vi.resetModules(); + globalForHub.formbricksHubClientRepeatArrays = undefined; + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + test("a create that hits the duplicate row is attempted once, not three times", async () => { + const fetchStub = respondWith(409, "conflict"); + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + await expect(client.feedbackRecords.create(record)).rejects.toThrow(); + + // The behaviour the narrowing exists for: on a re-import every create conflicts, and the SDK's + // default is three POSTs plus ~1.5s of backoff per record for an answer that cannot change. + expect(fetchStub).toHaveBeenCalledTimes(1); + }); + + test("a create refused by a draining tenant purge still gets the SDK's retries", async () => { + const fetchStub = respondWith(409, "tenant_write_conflict"); + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + await expect(client.feedbackRecords.create(record)).rejects.toThrow(); + + // Hub's create answers 409 for two unrelated reasons and documents this one as retryable + // (`openapi.yaml`, and `NewTenantWriteConflictError` in its feedback-records repository). + // Keying the narrowing on the status or the path alone suppressed a retry Hub asked for. + expect(fetchStub).toHaveBeenCalledTimes(3); + }); + + test("a conflicting create whose body carries no code falls back to the SDK's policy", async () => { + const fetchStub = respondWith(409); + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + await expect(client.feedbackRecords.create(record)).rejects.toThrow(); + + // Nothing to read the two 409s apart with, so the degradation is the safe one: the extra + // retries the SDK would have made anyway, never a suppressed one. + expect(fetchStub).toHaveBeenCalledTimes(3); + }); + + test("a rate-limited create still gets the SDK's retries", async () => { + const fetchStub = respondWith(429); + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + await expect(client.feedbackRecords.create(record)).rejects.toThrow(); + + // maxRetries defaults to 2, so one attempt plus two retries. This is what a per-request + // `maxRetries: 0` would have cost us on the highest-volume write path in the app. + expect(fetchStub).toHaveBeenCalledTimes(3); + }); + + test("a 409 outside the feedback-record collection is still retried", async () => { + const fetchStub = respondWith(409, "tenant_write_conflict"); + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + await expect(client.tenants.settings.update("tenant-1", {})).rejects.toThrow(); + + expect(fetchStub).toHaveBeenCalledTimes(3); + }); + + // The narrowing reads `response.url`, which a real fetch always populates. Pinning what happens + // when it is absent so the degradation is a decision rather than a surprise: the SDK's own policy + // stands, which costs the extra retries but never suppresses one that was wanted. + test("a conflict whose response carries no URL falls back to the SDK's policy", async () => { + const fetchStub = vi.fn( + async () => + new Response(JSON.stringify({ code: "conflict" }), { + status: 409, + headers: { "content-type": "application/problem+json", "retry-after-ms": "0" }, + }) + ) as unknown as typeof fetch; + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + await expect(client.feedbackRecords.create(record)).rejects.toThrow(); + + expect(fetchStub).toHaveBeenCalledTimes(3); + }); + + test("a 409 on a single record is still retried", async () => { + const fetchStub = respondWith(409, "conflict"); + vi.stubGlobal("fetch", fetchStub); + const client = await getClient(); + + // `/v1/feedback-records/{id}` is not the collection path the create posts to. + await expect(client.feedbackRecords.update("rec-1", { value_text: "x" })).rejects.toThrow(); + + expect(fetchStub).toHaveBeenCalledTimes(3); + }); +}); diff --git a/apps/web/modules/hub/hub-client.ts b/apps/web/modules/hub/hub-client.ts index 71013161a2e3..2a02c135a884 100644 --- a/apps/web/modules/hub/hub-client.ts +++ b/apps/web/modules/hub/hub-client.ts @@ -17,6 +17,129 @@ class FormbricksHubWithRepeatedArrayParams extends FormbricksHub { } } +/** Hub's status for a conflict. Two unrelated conditions share it — see `isTerminalConflict`. */ +const CONFLICT_STATUS = 409; + +/** + * The collection path `feedbackRecords.create` posts to. + * + * Matched with `endsWith` so a `baseURL` carrying a path prefix still resolves. Deliberately exact: + * `/v1/feedback-records/{id}` and `/v1/feedback-records/count` must not match. + */ +const FEEDBACK_RECORDS_COLLECTION_PATH = "/v1/feedback-records"; + +/** + * Hub's RFC 9457 problem code for a duplicate (tenant_id, submission_id, field_id). + * + * The code, not the status, is what tells the two 409s apart. Hub draws the same line in its own + * error types: `ConflictError` (`code: "conflict"`) is terminal, and `TenantWriteConflictError` + * (`code: "tenant_write_conflict"`) is "deliberately distinct ... so retryable lock conflicts are + * never confused with terminal resource conflicts". + */ +const DUPLICATE_RECORD_CODE = "conflict"; + +type TShouldRetry = (response: Response) => Promise; + +const withShouldRetry = (target: object): { shouldRetry?: TShouldRetry } => + target as { shouldRetry?: TShouldRetry }; + +/** + * Reads the `code` member of Hub's problem body, or undefined when there isn't a usable one. + * + * Reads a **clone**: a body can only be consumed once, and the SDK reads the original right after + * this — `response.text()` to build the APIError, or `CancelReadableStream(response.body)` before + * a retry. + */ +const readProblemCode = async (response: Response): Promise => { + try { + const body: unknown = await response.clone().json(); + + if (typeof body === "object" && body !== null) { + const { code } = body as { code?: unknown }; + + return typeof code === "string" ? code : undefined; + } + } catch { + // A missing, truncated or non-JSON body says nothing about which 409 this is. + } + + return undefined; +}; + +/** + * A 409 from Hub's feedback-record create that is the duplicate row rather than a lock timeout. + * + * The create answers 409 for two unrelated reasons, so the status alone cannot decide: + * + * - `conflict` — the unique index rejected (tenant_id, submission_id, field_id). Terminal, and the + * whole shape of a re-import. + * - `tenant_write_conflict` — the insert's tenant write lock was refused because a tenant data + * purge is draining for that tenant. Hub's `openapi.yaml` documents it on this very operation + * ("retryable – retry after the purge completes"), and its repository raises it beside the + * duplicate (`internal/repository/feedback_records_repository.go`, `Create`). + * + * Only the SDK's *docstrings* confine `tenant_write_conflict` to `tenants/*` and `taxonomy/*`, + * which is what an earlier revision of this file read the enumeration off. Suppressing that retry + * would turn a transient purge into a failed record, so the code is read and only the duplicate is + * treated as terminal. + * + * The path stays as the outer scope: it keeps the narrowing on the one operation whose cost was + * measured, and `conflict` is also raised by taxonomy runs. Three operations share this exact + * path — `create` (POST), `list` (GET) and the delete-by-user bulk delete (DELETE) — and a + * `Response` does not carry the request method, but neither of the other two can reach here with + * this code: a GET cannot conflict, and the bulk delete's only 409 is `tenant_write_conflict` + * (and it is never issued through this client — `feedback-records-proxy.ts` forwards it with raw + * `fetch`, so the SDK's retry policy never applies to it). + */ +const isTerminalConflict = async (response: Response): Promise => { + if (response.status !== CONFLICT_STATUS) return false; + + let pathname: string; + + try { + pathname = new URL(response.url).pathname; + } catch { + // No usable URL is not evidence the conflict is terminal — leave the SDK's own decision alone. + return false; + } + + if (!pathname.endsWith(FEEDBACK_RECORDS_COLLECTION_PATH)) return false; + + return (await readProblemCode(response)) === DUPLICATE_RECORD_CODE; +}; + +/** + * Stop the SDK retrying a create that came back a duplicate 409. + * + * `shouldRetry` returns `true` for 409 ("retry on lock timeouts") and `maxRetries` defaults to 2, + * so every conflicting create costs three POSTs and ~1.5s of backoff before the 409 it was always + * going to get. On a first import nothing conflicts and this is free. On a re-import *everything* + * conflicts — that is the whole shape of the operation (`reconcile.ts`) — so it is 3x the requests + * and ~100x the wall clock, all of it sleeping, inside a synchronous server action. Measured on a + * stub Hub: 60 records went from 60 POSTs/29ms to 180 POSTs/2963ms. + * + * Done here rather than with a per-request `maxRetries: 0` so genuine 429 and 5xx retries survive + * on the highest-volume write path in the app; keyed on the duplicate's problem code so every + * retryable `tenant_write_conflict` keeps its retries — the create's own included. + * + * `shouldRetry` is `private` in the SDK's types — TypeScript forbids redeclaring it in a subclass, + * unlike the `protected` `stringifyQuery` above — so the override is installed on the prototype. + * Only when the base method is actually there: if a future SDK renames it, the SDK stops calling + * ours too and the behaviour degrades to today's extra retries rather than to a broken client. + */ +const baseShouldRetry = withShouldRetry(FormbricksHub.prototype).shouldRetry; + +if (typeof baseShouldRetry === "function") { + withShouldRetry(FormbricksHubWithRepeatedArrayParams.prototype).shouldRetry = async function ( + this: FormbricksHub, + response: Response + ): Promise { + if (await isTerminalConflict(response)) return false; + + return baseShouldRetry.call(this, response); + }; +} + let repeatedArrayParamsVerified = false; /** From e0bb5183d6137e3c8b90a6f6bffa74e57fe69a09 Mon Sep 17 00:00:00 2001 From: Javi Aguilar <122741+itsjavi@users.noreply.github.com> Date: Tue, 1 Sep 2026 12:00:43 +0000 Subject: [PATCH 3/5] fix(workflows): make the rich-text editor look disabled when read-only (#9097) Co-authored-by: Claude Opus 5 --- .../rich-text-translation-input.tsx | 2 +- .../components/editor/components/editor.tsx | 14 ++++++---- .../ui/components/editor/styles-editor.css | 26 +++++++++++++++++++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/apps/web/modules/survey/multi-language-surveys/components/rich-text-translation-input.tsx b/apps/web/modules/survey/multi-language-surveys/components/rich-text-translation-input.tsx index 479289c70dc5..b98493785494 100644 --- a/apps/web/modules/survey/multi-language-surveys/components/rich-text-translation-input.tsx +++ b/apps/web/modules/survey/multi-language-surveys/components/rich-text-translation-input.tsx @@ -45,7 +45,7 @@ export const RichTextTranslationInput = ({ }, [value]); return ( -
+
{ return ( <> -
+
+ className={cn( + "editor-container rounded-md p-0", + // Defined in styles-editor.css — see the comment there for why this state can't be + // expressed as Tailwind utilities. + !editable && "editor-container-disabled", + props.isInvalid && "border! border-red-500!" + )}> { isExternalUrlsAllowed={props.isExternalUrlsAllowed} /> {props.onEmptyChange ? : null} -
+
Date: Tue, 1 Sep 2026 12:02:42 +0000 Subject: [PATCH 4/5] fix: map P2002 with unrecoverable fields to 409 on client response create (#9095) --- .../responses/lib/response-error.test.ts | 24 +++++++++++++++++++ .../responses/lib/response-error.ts | 10 +++++++- .../responses/lib/response.test.ts | 2 +- .../responses/lib/response.test.ts | 6 +++-- 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.test.ts b/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.test.ts index eae25d8e94d2..6dd8f7f9ad9f 100644 --- a/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.test.ts +++ b/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.test.ts @@ -24,6 +24,30 @@ describe("handleClientResponseCreateError", () => { ); }); + // ENG-2251: adapter-pg parses the column list out of the Postgres error DETAIL; when that line is + // absent or unparseable it emits `constraint: undefined`, so no field check can match. A P2002 + // must still be a conflict (409), never fall through to DatabaseError (500). + test("maps a P2002 without recoverable fields to UniqueConstraintError", () => { + const error = new Prisma.PrismaClientKnownRequestError("Unique constraint failed", { + code: "P2002", + clientVersion: "test", + meta: { driverAdapterError: { cause: { kind: "UniqueConstraintViolation" } } }, + }); + expect(() => handleClientResponseCreateError(error)).toThrow( + new UniqueConstraintError("Response already exists") + ); + }); + + test("maps a P2002 without any meta to UniqueConstraintError", () => { + const error = new Prisma.PrismaClientKnownRequestError("Unique constraint failed", { + code: "P2002", + clientVersion: "test", + }); + expect(() => handleClientResponseCreateError(error)).toThrow( + new UniqueConstraintError("Response already exists") + ); + }); + test("maps any other known Prisma error to DatabaseError carrying its message", () => { const error = new Prisma.PrismaClientKnownRequestError("boom", { code: "P2025", clientVersion: "test" }); expect(() => handleClientResponseCreateError(error)).toThrow(new DatabaseError("boom")); diff --git a/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.ts b/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.ts index 6e852d2c1e8f..76d6ff1687b2 100644 --- a/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.ts +++ b/apps/web/app/api/client/[workspaceId]/responses/lib/response-error.ts @@ -19,13 +19,21 @@ export const isDisplayIdUniqueConstraintError = (error: PrismaClientKnownRequest */ export const handleClientResponseCreateError = (error: unknown, displayId?: string | null): never => { if (isPrismaKnownRequestError(error)) { + // Captured before the P2002 guards: their negative branches narrow `error` to `never`. + const { message } = error; if (isDisplayIdUniqueConstraintError(error)) { throw new InvalidInputError(`Display ${displayId} is already linked to a response`); } if (isSingleUseIdUniqueConstraintError(error)) { throw new UniqueConstraintError("Response already submitted for this single-use link"); } - throw new DatabaseError(error.message); + if (isUniqueConstraintError(error)) { + // The adapter recovers the column list by parsing the Postgres error DETAIL, which can be + // absent or unparseable (redacted detail, non-English lc_messages). A P2002 is still a + // duplicate either way, so it must stay a conflict — never fall through to a 500. + throw new UniqueConstraintError("Response already exists"); + } + throw new DatabaseError(message); } throw error; }; diff --git a/apps/web/app/api/v1/client/[workspaceId]/responses/lib/response.test.ts b/apps/web/app/api/v1/client/[workspaceId]/responses/lib/response.test.ts index 400e8e312a4f..00c7fc2d49cb 100644 --- a/apps/web/app/api/v1/client/[workspaceId]/responses/lib/response.test.ts +++ b/apps/web/app/api/v1/client/[workspaceId]/responses/lib/response.test.ts @@ -163,7 +163,7 @@ describe("createResponse", () => { test("should throw DatabaseError on Prisma known request error", async () => { const prismaError = new Prisma.PrismaClientKnownRequestError("Test Prisma Error", { - code: "P2002", + code: "P2025", clientVersion: "test", }); vi.mocked(prisma.response.create).mockRejectedValue(prismaError); diff --git a/apps/web/app/api/v2/client/[workspaceId]/responses/lib/response.test.ts b/apps/web/app/api/v2/client/[workspaceId]/responses/lib/response.test.ts index aa597878fbce..6071d1f23467 100644 --- a/apps/web/app/api/v2/client/[workspaceId]/responses/lib/response.test.ts +++ b/apps/web/app/api/v2/client/[workspaceId]/responses/lib/response.test.ts @@ -202,7 +202,9 @@ describe("createResponse V2", () => { ).rejects.toThrow(UniqueConstraintError); }); - test("should throw DatabaseError on P2002 without singleUseId or displayId target", async () => { + // ENG-2251: a P2002 on this create can only be one of the response table's unique constraints, + // so an unmatched (or unrecoverable) target is still a duplicate — a 409, never a 500. + test("should throw UniqueConstraintError on P2002 without singleUseId or displayId target", async () => { const prismaError = new Prisma.PrismaClientKnownRequestError("Unique constraint failed", { code: "P2002", clientVersion: "test", @@ -211,7 +213,7 @@ describe("createResponse V2", () => { vi.mocked(mockTx.response.create).mockRejectedValue(prismaError); await expect( createResponse(mockResponseInput, mockTx as unknown as Prisma.TransactionClient) - ).rejects.toThrow(DatabaseError); + ).rejects.toThrow(UniqueConstraintError); }); test("should throw InvalidInputError on P2002 with displayId target (race condition)", async () => { From 98e140062f4491edb974fe6add66bb199107abb8 Mon Sep 17 00:00:00 2001 From: Tiago <1585571+xernobyl@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:37:12 +0000 Subject: [PATCH 5/5] fix: match webhook URL denylist ranges as CIDRs (ENG-2554) (#8946) --- .../lib/utils/validate-webhook-url.test.ts | 147 +++++++++++++++++ apps/web/lib/utils/validate-webhook-url.ts | 151 +++++++++++------- sonar-project.properties | 10 ++ 3 files changed, 251 insertions(+), 57 deletions(-) diff --git a/apps/web/lib/utils/validate-webhook-url.test.ts b/apps/web/lib/utils/validate-webhook-url.test.ts index 19d367f8ff92..3bb3cdbf539b 100644 --- a/apps/web/lib/utils/validate-webhook-url.test.ts +++ b/apps/web/lib/utils/validate-webhook-url.test.ts @@ -179,6 +179,153 @@ describe("validateWebhookUrl", () => { }); }); + // ENG-2554. Ranges the previous regex/string-prefix classifier let through, each as an IP + // literal so the classifier is exercised without DNS. + describe("reserved ranges the regex classifier missed", () => { + const rejected: [label: string, url: string][] = [ + // Reported in ENG-1326 and never fixed. + ["Azure WireServer (IMDS sibling, outside 169.254/16)", "http://168.63.129.16/machine"], + ["6to4 encoding of 127.0.0.1", "http://[2002:7f00:1::1]/"], + ["NAT64 encoding of 169.254.169.254", "http://[64:ff9b::a9fe:a9fe]/latest/meta-data/"], + ["NAT64 encoding of 127.0.0.1", "http://[64:ff9b::7f00:1]/"], + ["NAT64 local-use (RFC 8215)", "http://[64:ff9b:1::1]/"], + ["IPv6 multicast, all-nodes", "http://[ff02::1]/"], + ["IPv6 site-local (deprecated)", "http://[fec0::1]/"], + // fe80::/10 spans fe80:: through febf::. The old "fe80:" string prefix covered only + // fe80::/16, so these two link-local addresses passed. + ["link-local, middle of fe80::/10", "http://[fe9f::1]/"], + ["link-local, top of fe80::/10", "http://[febf::1]/"], + // /^224\./ and /^240\./ matched a /8 each while their comments claimed a /4. + ["multicast above 224/8", "http://225.0.0.1/"], + ["SSDP multicast", "http://239.255.255.250/"], + ["reserved above 240/8", "http://241.0.0.1/"], + // Multicast scopes no IPv6 prefix covered. + ["IPv6 multicast, site scope", "http://[ff05::1]/"], + ["IPv6 multicast, global scope", "http://[ff0e::1]/"], + // IPv4-compatible IPv6 (::/96): the same wrapper trick as NAT64/6to4, and not covered by + // the IPv4-mapped handling, which only applies to ::ffff:0:0/96. + ["IPv4-compatible ::127.0.0.1", "http://[::7f00:1]/"], + ["IPv4-compatible, dotted form", "http://[::127.0.0.1]/"], + ["IPv4-compatible ::169.254.169.254 (IMDS)", "http://[::a9fe:a9fe]/"], + ["IPv4-compatible ::10.0.0.1", "http://[::a00:1]/"], + // IPv4-translated (::ffff:0:0:0/96, RFC 2765/SIIT) — the one IPv4 wrapper BlockList does + // NOT fold onto the IPv4 rules, since only ::ffff:0:0/96 gets that treatment. + ["IPv4-translated ::127.0.0.1", "http://[::ffff:0:7f00:1]/"], + ["IPv4-translated ::169.254.169.254 (IMDS)", "http://[::ffff:0:a9fe:a9fe]/"], + ["IPv4-translated ::10.0.0.1", "http://[::ffff:0:a00:1]/"], + ["Teredo — tunnels IPv4 like 6to4", "http://[2001:0:1234::1]/"], + ["IPv6 documentation range", "http://[2001:db8::1]/"], + ["IPv6 discard-only prefix", "http://[100::1]/"], + ["IPv6 unspecified", "http://[::]/"], + ["IPv6 loopback", "http://[::1]/"], + // Already blocked before this change; kept so the CIDR rewrite is pinned to the old + // classifier's full coverage and cannot silently drop a range. + ["0.0.0.0/8 beyond 0.0.0.0 itself", "http://0.1.2.3/"], + ["broadcast", "http://255.255.255.255/"], + ]; + + test.each(rejected)("rejects %s", async (_label, url) => { + await expect(validateWebhookUrl(url)).rejects.toThrow( + "Webhook URL must not point to private or internal IP addresses" + ); + }); + + // The CGNAT alternation /^100\.(6[4-9]|[7-9]\d|1[0-2]\d)\./ matched second octets 100-129, + // but CGNAT is 100.64.0.0/10 (64-127) — so these public addresses were wrongly rejected. + // Fails if anyone re-derives the old regex. + test.each([["100.128.0.1"], ["100.129.0.1"], ["100.130.0.1"]])( + "accepts %s (public, just above CGNAT)", + async (ip) => { + await expect(validateWebhookUrl(`https://${ip}/webhook`)).resolves.toBeUndefined(); + } + ); + + // Boundary controls: one address either side of a range we block, so a prefix written one bit + // too wide fails here rather than silently blocking customer endpoints. + test.each([ + ["100.63.255.255", "just below CGNAT"], + ["172.32.0.1", "just above RFC1918 172.16/12"], + ["192.0.3.1", "just above TEST-NET-1"], + ["198.20.0.1", "just above benchmarking"], + ["11.0.0.1", "just above RFC1918 10/8"], + ["223.255.255.255", "just below multicast"], + ])("accepts %s (%s)", async (ip) => { + await expect(validateWebhookUrl(`https://${ip}/webhook`)).resolves.toBeUndefined(); + }); + + test.each([ + ["2003::1", "just above 6to4"], + ["64:ff9c::1", "just above NAT64 well-known"], + ["fe7f::1", "just below link-local"], + ["2606:2800:220:1:248:1893:25c8:1946", "public (example.com)"], + // 2001::/32 is Teredo; the rest of 2001::/16 is ordinary global unicast. + ["2001:4860:4860::8888", "public in 2001::/16 but outside Teredo"], + ["2001:db9::1", "just above the documentation range"], + ["101::1", "just above the discard prefix"], + ])("accepts [%s] (%s)", async (ip) => { + await expect(validateWebhookUrl(`https://[${ip}]/webhook`)).resolves.toBeUndefined(); + }); + }); + + // BlockList.check() reports IPv4-mapped forms against the IPv4 rules, which is the misclassification + // class tracked in ENG-2218 — covered here so a future refactor cannot quietly drop it. + describe("IPv4-mapped IPv6 forms", () => { + test.each([ + ["::ffff:127.0.0.1", "dotted mapped loopback"], + ["::ffff:7f00:1", "hex mapped loopback"], + ["::FFFF:7F00:1", "uppercase hex mapped loopback"], + ["::ffff:169.254.169.254", "mapped IMDS"], + ["::ffff:10.0.0.1", "mapped RFC1918"], + ["::ffff:100.64.0.1", "mapped CGNAT"], + ])("rejects [%s] (%s)", async (ip) => { + await expect(validateWebhookUrl(`http://[${ip}]/`)).rejects.toThrow( + "Webhook URL must not point to private or internal IP addresses" + ); + }); + + // Pins that neither ::/96 (IPv4-compatible) nor ::ffff:0:0:0/96 (IPv4-translated) swallows + // ::ffff:0:0/96 (IPv4-mapped) — BlockList checks a mapped address against the IPv6 rules as + // well as the IPv4 ones, so an overlapping prefix here would silently block public endpoints. + test("accepts a mapped public address", async () => { + await expect(validateWebhookUrl("https://[::ffff:93.184.216.34]/webhook")).resolves.toBeUndefined(); + }); + }); + + // The classifier derives the family from the address itself, not from which resolver returned it. + // A v4 address arriving on the v6 path would otherwise be checked against the IPv6 rules only, + // match nothing, and be treated as public. + describe("family mismatch between resolver and address", () => { + test("rejects a private IPv4 address returned by the IPv6 resolver", async () => { + setupDnsResolution(null, ["10.0.0.1"]); + await expect(validateWebhookUrl("https://sneaky.example/hook")).rejects.toThrow( + "Webhook URL must not point to private or internal IP addresses" + ); + }); + + test("rejects an unresolvable-looking garbage address rather than allowing it", async () => { + setupDnsResolution(["not-an-ip"]); + await expect(validateWebhookUrl("https://garbage.example/hook")).rejects.toThrow( + "Webhook URL must not point to private or internal IP addresses" + ); + }); + }); + + // The URL parser normalizes the alternative IPv4 notations before the classifier sees them. + // Asserted here because it is load-bearing: validateIpLiteral relies on it rather than + // re-implementing octal/hex/short-form parsing. + describe("alternative IPv4 notations", () => { + test.each([ + ["http://0x7f000001/", "hex"], + ["http://2130706433/", "decimal"], + ["http://0177.0.0.1/", "octal"], + ["http://127.1/", "short form"], + ])("rejects %s (%s form of 127.0.0.1)", async (url) => { + await expect(validateWebhookUrl(url)).rejects.toThrow( + "Webhook URL must not point to private or internal IP addresses" + ); + }); + }); + describe("DNS resolution with private IP results", () => { test("rejects hostname resolving to loopback address", async () => { setupDnsResolution(["127.0.0.1"]); diff --git a/apps/web/lib/utils/validate-webhook-url.ts b/apps/web/lib/utils/validate-webhook-url.ts index c9182993d6dc..5c849fe2ef75 100644 --- a/apps/web/lib/utils/validate-webhook-url.ts +++ b/apps/web/lib/utils/validate-webhook-url.ts @@ -1,5 +1,6 @@ import "server-only"; import dns from "node:dns"; +import net from "node:net"; import { Agent } from "undici"; import { InvalidInputError } from "@formbricks/types/errors"; import { DANGEROUSLY_ALLOW_WEBHOOK_INTERNAL_URLS } from "../constants"; @@ -12,64 +13,95 @@ const BLOCKED_HOSTNAMES = new Set([ "metadata.google.internal", ]); -const PRIVATE_IPV4_PATTERNS: RegExp[] = [ - /^127\./, // 127.0.0.0/8 – Loopback - /^10\./, // 10.0.0.0/8 – Class A private - /^172\.(1[6-9]|2\d|3[01])\./, // 172.16.0.0/12 – Class B private - /^192\.168\./, // 192.168.0.0/16 – Class C private - /^169\.254\./, // 169.254.0.0/16 – Link-local (AWS/GCP/Azure metadata) - /^0\./, // 0.0.0.0/8 – "This" network - /^100\.(6[4-9]|[7-9]\d|1[0-2]\d)\./, // 100.64.0.0/10 – Shared address space (RFC 6598) - /^192\.0\.0\./, // 192.0.0.0/24 – IETF protocol assignments - /^192\.0\.2\./, // 192.0.2.0/24 – TEST-NET-1 (documentation) - /^198\.51\.100\./, // 198.51.100.0/24 – TEST-NET-2 (documentation) - /^203\.0\.113\./, // 203.0.113.0/24 – TEST-NET-3 (documentation) - /^198\.1[89]\./, // 198.18.0.0/15 – Benchmarking - /^224\./, // 224.0.0.0/4 – Multicast - /^240\./, // 240.0.0.0/4 – Reserved for future use - /^255\.255\.255\.255$/, // Limited broadcast +/** + * Private, reserved and otherwise non-routable ranges that must never be a webhook target. + * + * Matched as CIDRs via `net.BlockList` rather than by regex or string prefix. The previous + * regex/prefix classifier silently covered less than its own comments claimed — `/^224\./` and + * `/^240\./` matched a /8 each while documenting a /4, the IPv6 prefix `"fe80:"` covered only + * `fe80::/16` of the `fe80::/10` link-local range, and the CGNAT alternation matched second + * octets 100-129, wrongly rejecting the public `100.128.0.0/15`. CIDR matching makes all of + * those exact by construction. + * + * `BlockList` also normalizes IPv4-mapped IPv6 addresses (`::ffff:127.0.0.1`, the hex form + * `::ffff:7f00:1`, uppercase, and uncompressed) against the IPv4 rules below, so mapped forms of + * an internal address are covered without a hand-rolled unwrapping step. + */ +const BLOCKED_IPV4_SUBNETS: [address: string, prefix: number][] = [ + ["0.0.0.0", 8], // "this" network + ["10.0.0.0", 8], // RFC 1918 private + ["100.64.0.0", 10], // shared address space / CGNAT (RFC 6598) — Tailscale tailnets + ["127.0.0.0", 8], // loopback + ["169.254.0.0", 16], // link-local (AWS/GCP/Azure IMDS) + ["172.16.0.0", 12], // RFC 1918 private + ["192.0.0.0", 24], // IETF protocol assignments + ["192.0.2.0", 24], // TEST-NET-1 (documentation) + ["192.168.0.0", 16], // RFC 1918 private + ["198.18.0.0", 15], // benchmarking (RFC 2544) + ["198.51.100.0", 24], // TEST-NET-2 (documentation) + ["203.0.113.0", 24], // TEST-NET-3 (documentation) + ["224.0.0.0", 4], // multicast + ["240.0.0.0", 4], // reserved for future use, incl. 255.255.255.255 broadcast ]; -const PRIVATE_IPV6_PREFIXES = [ - "::1", // Loopback - "fe80:", // Link-local - "fc", // Unique local address (ULA, fc00::/7 — covers fc00:: through fdff::) - "fd", // Unique local address (ULA, fc00::/7 — covers fc00:: through fdff::) +const BLOCKED_IPV4_ADDRESSES: string[] = [ + "168.63.129.16", // Azure WireServer — platform DNS/agent channel, outside 169.254.0.0/16 ]; -const isPrivateIPv4 = (ip: string): boolean => { - return PRIVATE_IPV4_PATTERNS.some((pattern) => pattern.test(ip)); -}; - -const hexMappedToIPv4 = (hexPart: string): string | null => { - const groups = hexPart.split(":"); - if (groups.length !== 2) return null; - const high = Number.parseInt(groups[0], 16); - const low = Number.parseInt(groups[1], 16); - if (Number.isNaN(high) || Number.isNaN(low) || high > 0xffff || low > 0xffff) return null; - return `${(high >> 8) & 0xff}.${high & 0xff}.${(low >> 8) & 0xff}.${low & 0xff}`; -}; +const BLOCKED_IPV6_SUBNETS: [address: string, prefix: number][] = [ + // ::/96 is the deprecated IPv4-compatible format and covers both :: (unspecified) and ::1 + // (loopback). It does NOT collide with the IPv4-mapped range ::ffff:0:0/96, whose 6th group is + // ffff — so mapped public addresses stay reachable while ::7f00:1 and ::a9fe:a9fe do not. + ["::", 96], // IPv4-compatible IPv6, deprecated (::7f00:1 == 127.0.0.1); incl. :: and ::1 + // IPv4-translated (RFC 2765 / SIIT) is the sixth IPv4-wrapper format, alongside IPv4-mapped, + // IPv4-compatible, NAT64, 6to4 and Teredo. Deprecated by RFC 4966 and absent from the IANA + // special-purpose registry, so neither Node nor Go special-cases it. Its 5th group is ffff and + // 6th is 0 — the mirror of IPv4-mapped — so it overlaps neither ::/96 nor ::ffff:0:0/96, and + // mapped public addresses stay reachable. + ["::ffff:0:0:0", 96], // IPv4-translated IPv6, deprecated (::ffff:0:7f00:1 == 127.0.0.1) + ["64:ff9b::", 96], // NAT64 well-known (64:ff9b::a9fe:a9fe == 169.254.169.254) + ["64:ff9b:1::", 48], // NAT64 local-use (RFC 8215) + ["100::", 64], // discard-only (RFC 6666) + ["2001::", 32], // Teredo — tunnels IPv4 the same way 6to4 does + ["2001:db8::", 32], // documentation (RFC 3849) + ["2002::", 16], // 6to4 (2002:7f00:1::1 == 127.0.0.1) + ["fc00::", 7], // unique local addresses (ULA) + ["fe80::", 10], // link-local — the whole /10, not just fe80::/16 + ["fec0::", 10], // site-local (deprecated) + ["ff00::", 8], // multicast, every scope +]; -const isIPv4Mapped = (normalized: string): boolean => { - if (!normalized.startsWith("::ffff:")) return false; - const suffix = normalized.slice(7); // strip "::ffff:" +const buildBlockList = (): net.BlockList => { + const list = new net.BlockList(); - if (suffix.includes(".")) { - return isPrivateIPv4(suffix); + for (const [address, prefix] of BLOCKED_IPV4_SUBNETS) { + list.addSubnet(address, prefix, "ipv4"); + } + for (const address of BLOCKED_IPV4_ADDRESSES) { + list.addAddress(address, "ipv4"); + } + for (const [address, prefix] of BLOCKED_IPV6_SUBNETS) { + list.addSubnet(address, prefix, "ipv6"); } - const dotted = hexMappedToIPv4(suffix); - return dotted !== null && isPrivateIPv4(dotted); -}; -const isPrivateIPv6 = (ip: string): boolean => { - const normalized = ip.toLowerCase(); - if (normalized === "::") return true; - if (isIPv4Mapped(normalized)) return true; - return PRIVATE_IPV6_PREFIXES.some((prefix) => normalized.startsWith(prefix)); + return list; }; -const isPrivateIP = (ip: string, family: 4 | 6): boolean => { - return family === 4 ? isPrivateIPv4(ip) : isPrivateIPv6(ip); +const blockedAddresses = buildBlockList(); + +/** + * Returns true when `ip` must not be used as a webhook target. + * + * The family is taken from parsing `ip` rather than from the caller, so a mismatch between the two + * cannot pick the wrong rule set — checking an IPv4 address against the IPv6 rules would report it + * as allowed. Unparseable input is rejected for the same reason: `BlockList.check()` reports it as + * *not* blocked, so anything we cannot classify has to fail closed here. + */ +const isPrivateIP = (ip: string): boolean => { + const version = net.isIP(ip); + if (version === 0) return true; + + return blockedAddresses.check(ip, version === 4 ? "ipv4" : "ipv6"); }; const DNS_TIMEOUT_MS = 3000; @@ -115,8 +147,6 @@ const stripIPv6Brackets = (hostname: string): string => { return hostname; }; -const IPV4_LITERAL = /^\d{1,3}(?:\.\d{1,3}){3}$/; - const parseWebhookUrl = (url: string): URL => { let parsed: URL; try { @@ -130,14 +160,21 @@ const parseWebhookUrl = (url: string): URL => { return parsed; }; +/** + * Classifies an IP-literal host, or returns null when the host is a name to resolve via DNS. + * + * `net.isIP` decides the family instead of a dotted-quad regex: the URL parser has already + * normalized the alternative IPv4 notations (`0x7f000001`, `2130706433`, `0177.0.0.1`, `127.1` all + * arrive here as `127.0.0.1`) and rejected malformed ones, so this only has to tell a valid + * literal from a hostname. + */ const validateIpLiteral = (hostname: string): ResolvedAddress | null => { - const isIPv4Literal = IPV4_LITERAL.test(hostname); - const isIPv6Literal = hostname.startsWith("["); - if (!isIPv4Literal && !isIPv6Literal) return null; + const ip = stripIPv6Brackets(hostname); + const version = net.isIP(ip); + if (version === 0) return null; - const ip = isIPv6Literal ? stripIPv6Brackets(hostname) : hostname; - const family: 4 | 6 = isIPv4Literal ? 4 : 6; - if (!DANGEROUSLY_ALLOW_WEBHOOK_INTERNAL_URLS && isPrivateIP(ip, family)) { + const family: 4 | 6 = version === 4 ? 4 : 6; + if (!DANGEROUSLY_ALLOW_WEBHOOK_INTERNAL_URLS && isPrivateIP(ip)) { throw new InvalidInputError("Webhook URL must not point to private or internal IP addresses"); } return { ip, family }; @@ -189,7 +226,7 @@ export const validateAndResolveWebhookUrl = async (url: string): Promise