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 () => { 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/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/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 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; /** 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} -
+