From b2a13e3b1fbe227c54efec5d0e0eaec25ca2f46a Mon Sep 17 00:00:00 2001 From: Anshuman Pandey <54475686+pandeymangg@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:46:48 +0000 Subject: [PATCH 1/9] fix(security): scope response file deletion to the owning workspace [ENG-2291] (#8861) --- apps/web/lib/response/service.ts | 41 ++--- .../responses/[responseId]/lib/response.ts | 2 +- .../[responseId]/lib/tests/response.test.ts | 2 +- .../[responseId]/lib/tests/utils.test.ts | 97 ++++++++++-- .../responses/[responseId]/lib/utils.ts | 41 ++--- .../storage/lib/delete-response-files.test.ts | 140 ++++++++++++++++++ .../storage/lib/delete-response-files.ts | 89 +++++++++++ apps/web/modules/storage/utils.ts | 2 +- 8 files changed, 340 insertions(+), 74 deletions(-) create mode 100644 apps/web/modules/storage/lib/delete-response-files.test.ts create mode 100644 apps/web/modules/storage/lib/delete-response-files.ts diff --git a/apps/web/lib/response/service.ts b/apps/web/lib/response/service.ts index b5b5dcc84e27..d4aa98796cd0 100644 --- a/apps/web/lib/response/service.ts +++ b/apps/web/lib/response/service.ts @@ -4,7 +4,6 @@ import { z } from "zod"; import { prisma } from "@formbricks/database"; import { Prisma } from "@formbricks/database/prisma"; import { PrismaErrorType } from "@formbricks/database/types/error"; -import { logger } from "@formbricks/logger"; import { ZId, ZOptionalNumber, ZString } from "@formbricks/types/common"; import { DatabaseError, ResourceNotFoundError } from "@formbricks/types/errors"; import { @@ -16,14 +15,12 @@ import { ZResponseFilterCriteria, ZResponseUpdateInput, } from "@formbricks/types/responses"; -import { TSurveyElementTypeEnum } from "@formbricks/types/surveys/elements"; import { TSurvey } from "@formbricks/types/surveys/types"; import { TTag } from "@formbricks/types/tags"; -import { getElementsFromBlocks } from "@/lib/survey/utils"; import { getIsQuotasEnabled } from "@/modules/ee/license-check/lib/utils"; import { reduceQuotaLimits } from "@/modules/ee/quotas/lib/quotas"; -import { deleteFile } from "@/modules/storage/service"; -import { parseStorageFileUrl, resolveStorageUrlsInObject } from "@/modules/storage/utils"; +import { deleteResponseFileUrls } from "@/modules/storage/lib/delete-response-files"; +import { getSurveyFileUploadConfigs, resolveStorageUrlsInObject } from "@/modules/storage/utils"; import { getOrganizationIdFromWorkspaceId } from "@/modules/survey/lib/organization"; import { getOrganizationBilling } from "@/modules/survey/lib/survey"; import { ITEMS_PER_PAGE } from "../constants"; @@ -633,36 +630,20 @@ export const updateResponse = async ( }; const findAndDeleteUploadedFilesInResponse = async (response: TResponse, survey: TSurvey): Promise => { - const elements = getElementsFromBlocks(survey.blocks); - - const fileUploadElements = new Set( - elements.filter((element) => element.type === TSurveyElementTypeEnum.FileUpload).map((q) => q.id) + // Match write-time validation: a survey holds file uploads in either blocks or questions, so build + // the id set from the union of both rather than one shape (getSurveyFileUploadConfigs is exactly what + // validateClientFileUploads uses). Keying off a single shape silently skips deletes for the other. + const fileUploadElementIds = new Set( + getSurveyFileUploadConfigs({ blocks: survey.blocks, questions: survey.questions }).map( + (config) => config.id + ) ); const fileUrls = Object.entries(response.data) - .filter(([elementId]) => fileUploadElements.has(elementId)) + .filter(([elementId]) => fileUploadElementIds.has(elementId)) .flatMap(([, elementResponse]) => elementResponse as string[]); - const deletionPromises = fileUrls.map(async (fileUrl) => { - try { - const storageFile = parseStorageFileUrl(fileUrl); - - if (!storageFile) { - throw new Error(`Invalid storage file URL: ${fileUrl}`); - } - - return deleteFile( - storageFile.storageId, - storageFile.accessType, - storageFile.fileName, - survey.workspaceId - ); - } catch (error) { - logger.error(error, `Failed to delete file ${fileUrl}`); - } - }); - - await Promise.all(deletionPromises); + await deleteResponseFileUrls(fileUrls, survey.workspaceId); }; export const deleteResponse = async ( diff --git a/apps/web/modules/api/v2/management/responses/[responseId]/lib/response.ts b/apps/web/modules/api/v2/management/responses/[responseId]/lib/response.ts index e2381e2eedcc..d066b5a8ac1c 100644 --- a/apps/web/modules/api/v2/management/responses/[responseId]/lib/response.ts +++ b/apps/web/modules/api/v2/management/responses/[responseId]/lib/response.ts @@ -114,7 +114,7 @@ export const deleteResponse = async (responseId: string): Promise { expect(getSurveyQuestions).toHaveBeenCalledWith(response.surveyId); expect(findAndDeleteUploadedFilesInResponse).toHaveBeenCalledWith( response.data, - survey.questions, + survey, survey.workspaceId ); expect(result.ok).toBe(true); diff --git a/apps/web/modules/api/v2/management/responses/[responseId]/lib/tests/utils.test.ts b/apps/web/modules/api/v2/management/responses/[responseId]/lib/tests/utils.test.ts index 35a1cbf11158..d1d69d7d3ac7 100644 --- a/apps/web/modules/api/v2/management/responses/[responseId]/lib/tests/utils.test.ts +++ b/apps/web/modules/api/v2/management/responses/[responseId]/lib/tests/utils.test.ts @@ -2,6 +2,8 @@ import { fileUploadQuestion, openTextQuestion, responseData, workspaceId } from import { beforeEach, describe, expect, test, vi } from "vitest"; import { logger } from "@formbricks/logger"; import { okVoid } from "@formbricks/types/error-handlers"; +import { TSurveyElementTypeEnum } from "@formbricks/types/surveys/elements"; +import { findWorkspaceByIdOrLegacyEnvId } from "@/lib/utils/resolve-client-id"; import { deleteFile } from "@/modules/storage/service"; import { findAndDeleteUploadedFilesInResponse } from "../utils"; @@ -11,28 +13,89 @@ vi.mock("@formbricks/logger", () => ({ }, })); +vi.mock("@/lib/utils/resolve-client-id", () => ({ + findWorkspaceByIdOrLegacyEnvId: vi.fn(), +})); + vi.mock("@/modules/storage/service", () => ({ deleteFile: vi.fn(), })); +// The delete helper takes a survey shape ({ blocks, questions }); most cases here only use questions. +const questionsSurvey = (questions: unknown[]) => ({ questions, blocks: [] }) as any; + describe("findAndDeleteUploadedFilesInResponse", () => { beforeEach(() => { vi.clearAllMocks(); + // Default: every storage id resolves back to the survey's own workspace, so deletion is authorized. + vi.mocked(findWorkspaceByIdOrLegacyEnvId).mockResolvedValue({ + id: workspaceId, + organizationId: "org-1", + }); + vi.mocked(deleteFile).mockResolvedValue({ ok: true, data: undefined }); }); test("delete files for file upload questions and return okVoid", async () => { - vi.mocked(deleteFile).mockResolvedValue({ ok: true, data: undefined }); - - const result = await findAndDeleteUploadedFilesInResponse(responseData, [fileUploadQuestion]); + const result = await findAndDeleteUploadedFilesInResponse( + responseData, + questionsSurvey([fileUploadQuestion]), + workspaceId + ); expect(deleteFile).toHaveBeenCalledTimes(2); - expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "file1.png", undefined); - expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "file2.pdf", undefined); + expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "file1.png", workspaceId); + expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "file2.pdf", workspaceId); + expect(result).toEqual(okVoid()); + }); + + // File uploads can live in blocks instead of questions; this path used to key off questions only, so + // it silently deleted nothing for block-based surveys and leaked their uploads. + test("delete files for block-based file-upload elements", async () => { + const elementId = "block-file-upload-element"; + const blockSurvey = { + questions: [], + blocks: [{ id: "block-1", elements: [{ id: elementId, type: TSurveyElementTypeEnum.FileUpload }] }], + } as any; + const blockResponseData = { + [elementId]: [`https://example.com/storage/${workspaceId}/private/block-file.png`], + } as any; + + const result = await findAndDeleteUploadedFilesInResponse(blockResponseData, blockSurvey, workspaceId); + + expect(deleteFile).toHaveBeenCalledTimes(1); + expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "block-file.png", workspaceId); expect(result).toEqual(okVoid()); }); test("not call deleteFile if no file upload questions match response data", async () => { - const result = await findAndDeleteUploadedFilesInResponse(responseData, [openTextQuestion]); + const result = await findAndDeleteUploadedFilesInResponse( + responseData, + questionsSurvey([openTextQuestion]), + workspaceId + ); + + expect(deleteFile).not.toHaveBeenCalled(); + expect(result).toEqual(okVoid()); + }); + + // A planted URL pointing into another tenant's storage prefix must not be deleted, even though the + // survey now lists a file-upload question whose id matches the planted response key (the TOCTOU). + test("refuse to delete a file whose storage id belongs to a different workspace", async () => { + const foreignWorkspaceId = "foreign-workspace-id"; + vi.mocked(findWorkspaceByIdOrLegacyEnvId).mockResolvedValue({ + id: foreignWorkspaceId, + organizationId: "org-2", + }); + + const plantedData = { + [fileUploadQuestion.id]: [`https://example.com/storage/${foreignWorkspaceId}/public/victim.png`], + } as any; + + const result = await findAndDeleteUploadedFilesInResponse( + plantedData, + questionsSurvey([fileUploadQuestion]), + workspaceId + ); expect(deleteFile).not.toHaveBeenCalled(); expect(result).toEqual(okVoid()); @@ -40,13 +103,17 @@ describe("findAndDeleteUploadedFilesInResponse", () => { test("handle invalid file URLs and log errors", async () => { const invalidFileUrl = "https://example.com/invalid-url"; - const responseData = { + const invalidResponseData = { [fileUploadQuestion.id]: [invalidFileUrl], - }; + } as any; const loggerSpy = vi.spyOn(logger, "error"); - const result = await findAndDeleteUploadedFilesInResponse(responseData, [fileUploadQuestion]); + const result = await findAndDeleteUploadedFilesInResponse( + invalidResponseData, + questionsSurvey([fileUploadQuestion]), + workspaceId + ); expect(deleteFile).not.toHaveBeenCalled(); expect(loggerSpy).toHaveBeenCalled(); @@ -56,13 +123,15 @@ describe("findAndDeleteUploadedFilesInResponse", () => { }); test("process multiple file URLs", async () => { - vi.mocked(deleteFile).mockResolvedValue({ ok: true, data: undefined }); - - const result = await findAndDeleteUploadedFilesInResponse(responseData, [fileUploadQuestion]); + const result = await findAndDeleteUploadedFilesInResponse( + responseData, + questionsSurvey([fileUploadQuestion]), + workspaceId + ); expect(deleteFile).toHaveBeenCalledTimes(2); - expect(deleteFile).toHaveBeenNthCalledWith(1, workspaceId, "private", "file1.png", undefined); - expect(deleteFile).toHaveBeenNthCalledWith(2, workspaceId, "private", "file2.pdf", undefined); + expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "file1.png", workspaceId); + expect(deleteFile).toHaveBeenCalledWith(workspaceId, "private", "file2.pdf", workspaceId); expect(result).toEqual(okVoid()); }); }); diff --git a/apps/web/modules/api/v2/management/responses/[responseId]/lib/utils.ts b/apps/web/modules/api/v2/management/responses/[responseId]/lib/utils.ts index 435d6d4d273b..50c36299f337 100644 --- a/apps/web/modules/api/v2/management/responses/[responseId]/lib/utils.ts +++ b/apps/web/modules/api/v2/management/responses/[responseId]/lib/utils.ts @@ -1,42 +1,29 @@ import { Response, Survey } from "@formbricks/database/prisma"; -import { logger } from "@formbricks/logger"; import { Result, okVoid } from "@formbricks/types/error-handlers"; -import { TSurveyQuestionTypeEnum } from "@formbricks/types/surveys/types"; import { ApiErrorResponseV2 } from "@/modules/api/v2/types/api-error"; -import { deleteFile } from "@/modules/storage/service"; -import { parseStorageFileUrl } from "@/modules/storage/utils"; +import { deleteResponseFileUrls } from "@/modules/storage/lib/delete-response-files"; +import { getSurveyFileUploadConfigs } from "@/modules/storage/utils"; export const findAndDeleteUploadedFilesInResponse = async ( responseData: Response["data"], - questions: Survey["questions"], + survey: Pick, workspaceId?: string ): Promise> => { - const fileUploadQuestions = new Set( - questions - .filter( - (question: { type: string; id: string }) => question.type === TSurveyQuestionTypeEnum.FileUpload - ) - .map((q: { type: string; id: string }) => q.id) + // A survey holds file uploads in either blocks or questions, so build the id set from the union of + // both — the same source write-time validation uses. A questions-only set silently skipped deletes + // for block-based surveys (the common shape), leaking their uploads. + const fileUploadElementIds = new Set( + getSurveyFileUploadConfigs({ + blocks: survey.blocks, + questions: survey.questions, + }).map((config) => config.id) ); const fileUrls = Object.entries(responseData) - .filter(([questionId]) => fileUploadQuestions.has(questionId)) - .flatMap(([, questionResponse]) => questionResponse as string[]); + .filter(([elementId]) => fileUploadElementIds.has(elementId)) + .flatMap(([, elementResponse]) => elementResponse as string[]); - const deletionPromises = fileUrls.map(async (fileUrl) => { - try { - const storageFile = parseStorageFileUrl(fileUrl); - - if (!storageFile) { - throw new Error(`Invalid storage file URL: ${fileUrl}`); - } - return deleteFile(storageFile.storageId, storageFile.accessType, storageFile.fileName, workspaceId); - } catch (error) { - logger.error({ error, fileUrl }, "Failed to delete file"); - } - }); - - await Promise.all(deletionPromises); + await deleteResponseFileUrls(fileUrls, workspaceId); return okVoid(); }; diff --git a/apps/web/modules/storage/lib/delete-response-files.test.ts b/apps/web/modules/storage/lib/delete-response-files.test.ts new file mode 100644 index 000000000000..86423ba889ab --- /dev/null +++ b/apps/web/modules/storage/lib/delete-response-files.test.ts @@ -0,0 +1,140 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { logger } from "@formbricks/logger"; +import { findWorkspaceByIdOrLegacyEnvId } from "@/lib/utils/resolve-client-id"; +import { deleteFile } from "@/modules/storage/service"; +import { deleteResponseFileUrls } from "./delete-response-files"; + +vi.mock("server-only", () => ({})); + +vi.mock("@formbricks/logger", () => ({ + logger: { + error: vi.fn(), + }, +})); + +vi.mock("@/lib/utils/resolve-client-id", () => ({ + findWorkspaceByIdOrLegacyEnvId: vi.fn(), +})); + +vi.mock("@/modules/storage/service", () => ({ + deleteFile: vi.fn(), +})); + +const mockedResolve = vi.mocked(findWorkspaceByIdOrLegacyEnvId); +const mockedDeleteFile = vi.mocked(deleteFile); + +const OWN_WORKSPACE = "ws-own"; +const FOREIGN_WORKSPACE = "ws-victim"; + +describe("deleteResponseFileUrls", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockedDeleteFile.mockResolvedValue({ ok: true, data: undefined } as any); + }); + + test("deletes a file whose URL storage id belongs to the survey's workspace", async () => { + mockedResolve.mockResolvedValue({ id: OWN_WORKSPACE, organizationId: "org-1" }); + + await deleteResponseFileUrls([`/storage/${OWN_WORKSPACE}/private/answer.png`], OWN_WORKSPACE); + + expect(mockedDeleteFile).toHaveBeenCalledTimes(1); + expect(mockedDeleteFile).toHaveBeenCalledWith(OWN_WORKSPACE, "private", "answer.png", OWN_WORKSPACE); + }); + + // The core cross-tenant guard: a planted URL pointing into another tenant's storage prefix must not be + // deleted, even though it survived write-time validation via the element-id flip TOCTOU. + test("refuses to delete a planted URL that resolves to a different workspace", async () => { + mockedResolve.mockResolvedValue({ id: FOREIGN_WORKSPACE, organizationId: "org-2" }); + + await deleteResponseFileUrls([`/storage/${FOREIGN_WORKSPACE}/public/bg--fid--uuid.png`], OWN_WORKSPACE); + + expect(mockedDeleteFile).not.toHaveBeenCalled(); + }); + + // Legacy uploads were prefixed with the environment id; the resolver maps that back to the owning + // workspace, so a same-workspace legacy prefix is still deletable — using its real (legacy) prefix. + test("allows a legacy environment-id prefix that maps back to the survey's workspace", async () => { + const legacyEnvId = "env-legacy"; + mockedResolve.mockResolvedValue({ id: OWN_WORKSPACE, organizationId: "org-1" }); + + await deleteResponseFileUrls([`/storage/${legacyEnvId}/private/old.png`], OWN_WORKSPACE); + + expect(mockedResolve).toHaveBeenCalledWith(legacyEnvId); + expect(mockedDeleteFile).toHaveBeenCalledWith(legacyEnvId, "private", "old.png", OWN_WORKSPACE); + }); + + test("refuses when the storage id resolves to no workspace", async () => { + mockedResolve.mockResolvedValue(null); + + await deleteResponseFileUrls(["/storage/ws-unknown/public/x.png"], OWN_WORKSPACE); + + expect(mockedDeleteFile).not.toHaveBeenCalled(); + }); + + test("deletes nothing and never resolves when no survey workspace id is given", async () => { + await deleteResponseFileUrls([`/storage/${OWN_WORKSPACE}/private/answer.png`], undefined); + + expect(mockedResolve).not.toHaveBeenCalled(); + expect(mockedDeleteFile).not.toHaveBeenCalled(); + }); + + test("skips an unparseable URL without throwing", async () => { + await deleteResponseFileUrls(["not-a-storage-url"], OWN_WORKSPACE); + + expect(mockedResolve).not.toHaveBeenCalled(); + expect(mockedDeleteFile).not.toHaveBeenCalled(); + }); + + test("in a mixed batch, deletes the owned file and refuses the foreign one", async () => { + mockedResolve.mockImplementation(async (id: string) => + id === OWN_WORKSPACE + ? { id: OWN_WORKSPACE, organizationId: "org-1" } + : { id: id, organizationId: "org-2" } + ); + + await deleteResponseFileUrls( + [`/storage/${OWN_WORKSPACE}/private/mine.png`, `/storage/${FOREIGN_WORKSPACE}/public/theirs.png`], + OWN_WORKSPACE + ); + + expect(mockedDeleteFile).toHaveBeenCalledTimes(1); + expect(mockedDeleteFile).toHaveBeenCalledWith(OWN_WORKSPACE, "private", "mine.png", OWN_WORKSPACE); + }); + + // deleteFile returns an error result instead of throwing, so a failed storage deletion must still be + // logged — otherwise a leftover object looks like a clean success. + test("logs when deleteFile returns a failure result", async () => { + mockedResolve.mockResolvedValue({ id: OWN_WORKSPACE, organizationId: "org-1" }); + mockedDeleteFile.mockResolvedValue({ ok: false, error: { code: "s3_client_error" } } as any); + + await deleteResponseFileUrls([`/storage/${OWN_WORKSPACE}/private/answer.png`], OWN_WORKSPACE); + + expect(mockedDeleteFile).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + expect.objectContaining({ error: { code: "s3_client_error" } }), + "Failed to delete a response file from storage" + ); + }); + + test("resolves each distinct storage id only once for multiple files under it", async () => { + mockedResolve.mockResolvedValue({ id: OWN_WORKSPACE, organizationId: "org-1" }); + + await deleteResponseFileUrls( + [`/storage/${OWN_WORKSPACE}/private/one.png`, `/storage/${OWN_WORKSPACE}/private/two.png`], + OWN_WORKSPACE + ); + + expect(mockedResolve).toHaveBeenCalledTimes(1); + expect(mockedDeleteFile).toHaveBeenCalledTimes(2); + }); + + // The URL carries a percent-encoded file name but the object is stored under the decoded name, so the + // helper must decode before building the S3 key, or files with spaces/non-ASCII names never delete. + test("decodes the percent-encoded file name before deleting", async () => { + mockedResolve.mockResolvedValue({ id: OWN_WORKSPACE, organizationId: "org-1" }); + + await deleteResponseFileUrls([`/storage/${OWN_WORKSPACE}/private/my%20file%20(1).png`], OWN_WORKSPACE); + + expect(mockedDeleteFile).toHaveBeenCalledWith(OWN_WORKSPACE, "private", "my file (1).png", OWN_WORKSPACE); + }); +}); diff --git a/apps/web/modules/storage/lib/delete-response-files.ts b/apps/web/modules/storage/lib/delete-response-files.ts new file mode 100644 index 000000000000..cf713f998f8e --- /dev/null +++ b/apps/web/modules/storage/lib/delete-response-files.ts @@ -0,0 +1,89 @@ +import "server-only"; +import { logger } from "@formbricks/logger"; +import { findWorkspaceByIdOrLegacyEnvId } from "@/lib/utils/resolve-client-id"; +import { deleteFile } from "@/modules/storage/service"; +import { parseStorageFileUrl } from "@/modules/storage/utils"; + +/** + * Deletes the storage objects a response's file-upload answers point at, restricted to the survey's own + * workspace. + * + * The file URLs come out of `response.data`, i.e. from whoever wrote the response, and the S3 key is + * built from the id in the *URL* rather than the survey's workspace. Write-time validation + * (`validateClientFileUploads` -> `isScopedPrivateUploadUrl`) does pin uploaded URLs to the survey's + * workspace, but it only inspects keys that match a file-upload element that exists *at write time*. A + * caller can therefore plant a foreign URL under a key that is not yet an element, then edit the survey + * to turn that key into a file-upload element — a time-of-check/time-of-use gap that makes the planted, + * unvalidated URL look like a real answer at delete time. + * + * So the delete side cannot trust the URL's id. Re-resolve each URL's storage id here and drop anything + * that does not belong to this survey's workspace, regardless of which write path produced the data. The + * id may be a workspace id or a legacy environment id (older uploads were prefixed with the environment + * id), which is why it goes through `findWorkspaceByIdOrLegacyEnvId` rather than a plain string compare. + */ +export const deleteResponseFileUrls = async ( + fileUrls: string[], + surveyWorkspaceId: string | undefined +): Promise => { + if (!surveyWorkspaceId) { + // Without the owning workspace there is nothing to authorize against, so delete nothing. + logger.error({ fileCount: fileUrls.length }, "Skipping response file deletion: no workspace id given"); + return; + } + + // Several files in one response usually share a storage id (same survey/workspace prefix). Cache the + // resolution promise per id so the batch does one lookup per distinct id instead of one per file. + const workspaceByStorageId = new Map>(); + const resolveStorageWorkspace = (storageId: string) => { + const cached = workspaceByStorageId.get(storageId); + if (cached) return cached; + + const pending = findWorkspaceByIdOrLegacyEnvId(storageId); + workspaceByStorageId.set(storageId, pending); + return pending; + }; + + await Promise.all( + fileUrls.map(async (fileUrl) => { + try { + const storageFile = parseStorageFileUrl(fileUrl); + + if (!storageFile) { + throw new Error(`Invalid storage file URL: ${fileUrl}`); + } + + const storageWorkspace = await resolveStorageWorkspace(storageFile.storageId); + if (storageWorkspace?.id !== surveyWorkspaceId) { + logger.error( + { fileUrl, surveyWorkspaceId, storageId: storageFile.storageId }, + "Refusing to delete a response file stored outside the survey's workspace" + ); + return; + } + + // The URL carries the percent-encoded file name, but the object is stored under the decoded + // name (upload encodes it into the URL; the download path decodes before hitting S3). Decode + // here too, or files with spaces/non-ASCII names miss their key and never get deleted. Decoding + // before deleteFile also lets its hasTraversalSegment check run on the decoded segments. + const fileName = decodeURIComponent(storageFile.fileName); + + // deleteFile returns an error result (it does not throw) on S3 failures, so a discarded result + // would treat a failed deletion as a success and leave the object behind unlogged. + const result = await deleteFile( + storageFile.storageId, + storageFile.accessType, + fileName, + surveyWorkspaceId + ); + if (!result.ok) { + logger.error( + { fileUrl, surveyWorkspaceId, error: result.error }, + "Failed to delete a response file from storage" + ); + } + } catch (error) { + logger.error({ error, fileUrl }, "Failed to delete file"); + } + }) + ); +}; diff --git a/apps/web/modules/storage/utils.ts b/apps/web/modules/storage/utils.ts index 661a294d77b6..10a1e446a8b4 100644 --- a/apps/web/modules/storage/utils.ts +++ b/apps/web/modules/storage/utils.ts @@ -119,7 +119,7 @@ const getAllowedFileExtensionFromFileName = (fileName: string): TAllowedFileExte return extensionValidation.success ? extensionValidation.data : null; }; -const getSurveyFileUploadConfigs = ({ +export const getSurveyFileUploadConfigs = ({ blocks, questions, }: { From 4b6824bc1c52f6dacb423e06a0d6dc633aab16d1 Mon Sep 17 00:00:00 2001 From: Tiago <1585571+xernobyl@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:29:40 +0000 Subject: [PATCH 2/9] fix(unify): retire stale coming-soon on feedback source options (#8908) --- apps/web/i18n.lock | 2 -- apps/web/locales/en-US.json | 2 -- .../create-feedback-source-modal.tsx | 4 +-- .../feedback-source-type-selector.tsx | 30 ++----------------- .../ee/unify-feedback/sources/utils.test.ts | 11 ------- .../ee/unify-feedback/sources/utils.ts | 8 ----- 6 files changed, 4 insertions(+), 53 deletions(-) diff --git a/apps/web/i18n.lock b/apps/web/i18n.lock index d024f5c4c937..f31022014fbf 100644 --- a/apps/web/i18n.lock +++ b/apps/web/i18n.lock @@ -188,7 +188,6 @@ checksums: common/code: 343bc5386149b97cece2b093c39034b2 common/collapse_rows: 24988527f9180f37aa55d2aa183ccb21 common/column_n: b98315f0e504fad7e784d77f153a7d9d - common/coming_soon: ee2b0671e00972773210c5be5a9ccb89 common/completed: 0e4bbce9985f25eb673d9a054c8d5334 common/confirm: 90930b51154032f119fa75c1bd422d8b common/connect: 8778ee245078a8be4a2ce855c8c56edc @@ -3781,7 +3780,6 @@ checksums: workspace/unify/allowed_values: 430e0721aa2c52745ef8f8b6918bb7d2 workspace/unify/api_ingestion: a14642d27bbb6843f9f4903b6555dfbb workspace/unify/api_ingestion_settings_description: b4d3f00729154c01d0dd46b551c1f402 - workspace/unify/api_ingestion_setup_description: d18a267d0e50198682950f5341307fa3 workspace/unify/auto_generated: 6e83e8febd63275692c444cb8074531d workspace/unify/change_file: c5163ac18bf443370228a8ecbb0b07da workspace/unify/clear_mapping: 9bd7c716667838b9f203f5af0ac2d651 diff --git a/apps/web/locales/en-US.json b/apps/web/locales/en-US.json index 2be22469a2cb..e126e1c680f4 100644 --- a/apps/web/locales/en-US.json +++ b/apps/web/locales/en-US.json @@ -217,7 +217,6 @@ "code": "Code", "collapse_rows": "Collapse rows", "column_n": "Column {n}", - "coming_soon": "Coming soon", "completed": "Completed", "confirm": "Confirm", "connect": "Connect", @@ -3941,7 +3940,6 @@ "allowed_values": "Allowed values: {values}", "api_ingestion": "API ingestion", "api_ingestion_settings_description": "Create feedback records using the Management API", - "api_ingestion_setup_description": "Use the REST API to send feedback records directly into Formbricks. The API ingestion docs include the endpoint, payload shape, and authentication details.", "auto_generated": "Auto-generated", "change_file": "Change file", "clear_mapping": "Clear mapping", 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 77df5a1d6d08..78c6448bdbd0 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 @@ -67,8 +67,8 @@ import { FeedbackSourceTypeSelector } from "./feedback-source-type-selector"; import { FormbricksQuestionList } from "./formbricks-question-list"; import { ImportModeField } from "./import-mode-field"; -const API_INGESTION_DOCS_URL = "https://formbricks.com/docs/unify-feedback/api/rest-api"; -const FEEDBACK_RECORD_MCP_DOCS_URL = "https://formbricks.com/docs/unify-feedback/api/mcp"; +const API_INGESTION_DOCS_URL = "https://formbricks.com/docs/unify-feedback/feedback-sources"; +const FEEDBACK_RECORD_MCP_DOCS_URL = "https://formbricks.com/docs/platform/mcp/overview"; interface CreateFeedbackSourceModalProps { open: boolean; diff --git a/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-type-selector.tsx b/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-type-selector.tsx index 3843c2812405..089c4c2a09a8 100644 --- a/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-type-selector.tsx +++ b/apps/web/modules/ee/unify-feedback/sources/components/feedback-source-type-selector.tsx @@ -3,7 +3,6 @@ import Link from "next/link"; import { Trans, useTranslation } from "react-i18next"; import { Alert, AlertButton, AlertDescription, AlertTitle } from "@/modules/ui/components/alert"; -import { Badge } from "@/modules/ui/components/badge"; import { TFeedbackSourceOptionId, getFeedbackSourceOptions } from "../utils"; interface FeedbackSourceTypeSelectorProps { @@ -15,17 +14,12 @@ interface FeedbackSourceTypeSelectorProps { const getOptionClassName = ( selectedType: TFeedbackSourceOptionId | null, - optionId: TFeedbackSourceOptionId, - disabled: boolean + optionId: TFeedbackSourceOptionId ): string => { if (selectedType === optionId) { return "border-brand-dark bg-slate-50"; } - if (disabled) { - return "cursor-not-allowed border-slate-200 bg-slate-50 opacity-60"; - } - return "border-slate-200 hover:border-slate-300 hover:bg-slate-50"; }; @@ -44,23 +38,18 @@ export function FeedbackSourceTypeSelector({ {feedbackSourceOptions.map((option) => { const showNoSurveysAlert = surveyCount === 0 && option.id === "formbricks_survey" && selectedType === "formbricks_survey"; - const showApiIngestionSetupAlert = - option.id === "api_ingestion" && selectedType === "api_ingestion"; return (
{showNoSurveysAlert && } - {showApiIngestionSetupAlert && } ); })} @@ -97,20 +85,6 @@ export function FeedbackSourceTypeSelector({ ); } -const ApiIngestionSetupAlert = () => { - const { t } = useTranslation(); - - return ( - -
- -

{t("workspace.unify.api_ingestion_setup_description")}

- -
- - ); -}; - const NoFormbricksSurveysAlert = ({ workspaceId }: Readonly<{ workspaceId: string }>) => { return ( 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 12e65ead5dcd..79a021eff4ac 100644 --- a/apps/web/modules/ee/unify-feedback/sources/utils.test.ts +++ b/apps/web/modules/ee/unify-feedback/sources/utils.test.ts @@ -79,17 +79,6 @@ describe("getFeedbackSourceOptions", () => { expect(options[3].id).toBe("feedback_record_mcp"); }); - test("formbricks and csv are enabled; api ingestion and mcp are coming soon (disabled)", () => { - const options = getFeedbackSourceOptions(mockT as never); - const byId = Object.fromEntries(options.map((o) => [o.id, o])); - expect(byId.formbricks_survey.disabled).toBe(false); - expect(byId.csv.disabled).toBe(false); - expect(byId.api_ingestion.disabled).toBe(true); - expect(byId.api_ingestion.badge?.text).toBe("common.coming_soon"); - expect(byId.feedback_record_mcp.disabled).toBe(true); - expect(byId.feedback_record_mcp.badge?.text).toBe("common.coming_soon"); - }); - test("uses translation keys for name and description", () => { const options = getFeedbackSourceOptions(mockT as never); expect(options[0].name).toBe("workspace.unify.formbricks_surveys"); diff --git a/apps/web/modules/ee/unify-feedback/sources/utils.ts b/apps/web/modules/ee/unify-feedback/sources/utils.ts index b21dca1ce0e6..7b7086e450c6 100644 --- a/apps/web/modules/ee/unify-feedback/sources/utils.ts +++ b/apps/web/modules/ee/unify-feedback/sources/utils.ts @@ -44,8 +44,6 @@ export interface TFeedbackSourceOption { id: TFeedbackSourceOptionId; name: string; description: string; - disabled: boolean; - badge?: { text: string; type: "success" | "gray" | "warning" }; } export const getFeedbackSourceOptions = (t: TFunction): TFeedbackSourceOption[] => [ @@ -53,27 +51,21 @@ export const getFeedbackSourceOptions = (t: TFunction): TFeedbackSourceOption[] id: "formbricks_survey", name: t("workspace.unify.formbricks_surveys"), description: t("workspace.unify.source_connect_formbricks_description"), - disabled: false, }, { id: "csv", name: t("workspace.unify.csv_import"), description: t("workspace.unify.source_connect_csv_description"), - disabled: false, }, { id: "api_ingestion", name: t("workspace.unify.api_ingestion"), description: t("workspace.unify.api_ingestion_settings_description"), - disabled: true, - badge: { text: t("common.coming_soon"), type: "gray" }, }, { id: "feedback_record_mcp", name: t("workspace.unify.feedback_record_mcp"), description: t("workspace.unify.source_connect_feedback_record_mcp_description"), - disabled: true, - badge: { text: t("common.coming_soon"), type: "gray" }, }, ]; From e114d0a4a994560be542eaa0629f3419817901ad Mon Sep 17 00:00:00 2001 From: Dhruwang Jariwala <67850763+Dhruwang@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:03:53 +0000 Subject: [PATCH 3/9] fix(api): restore legacy environmentId on API v1 surveys and webhooks (#8855) --- .../v1/management/surveys/[surveyId]/route.ts | 28 +++- .../app/api/v1/management/surveys/route.ts | 15 +- .../app/api/v1/webhooks/[webhookId]/route.ts | 18 ++- apps/web/app/api/v1/webhooks/route.ts | 13 +- .../app/lib/api/legacy-environment-id.test.ts | 133 ++++++++++++++++++ apps/web/app/lib/api/legacy-environment-id.ts | 66 +++++++++ .../management/legacy-environment-id.spec.ts | 131 +++++++++++++++++ 7 files changed, 389 insertions(+), 15 deletions(-) create mode 100644 apps/web/app/lib/api/legacy-environment-id.test.ts create mode 100644 apps/web/app/lib/api/legacy-environment-id.ts create mode 100644 apps/web/playwright/api/management/legacy-environment-id.spec.ts diff --git a/apps/web/app/api/v1/management/surveys/[surveyId]/route.ts b/apps/web/app/api/v1/management/surveys/[surveyId]/route.ts index 911c3e8550ca..a43410ebc869 100644 --- a/apps/web/app/api/v1/management/surveys/[surveyId]/route.ts +++ b/apps/web/app/api/v1/management/surveys/[surveyId]/route.ts @@ -9,6 +9,10 @@ import { addLegacyProjectOverwrites, normaliseProjectOverwritesToWorkspace, } from "@/app/lib/api/api-backwards-compat"; +import { + addLegacyEnvironmentId, + addLegacyEnvironmentIdBestEffort, +} from "@/app/lib/api/legacy-environment-id"; import { RequestBodyTooLargeError, parseJsonBodyWithLimit } from "@/app/lib/api/request-body"; import { responses } from "@/app/lib/api/response"; import { @@ -65,7 +69,9 @@ export const GET = withV1ApiWrapper({ // consumers get a consistent shape regardless of how the survey was built. return { response: responses.successResponse( - addLegacyProjectOverwrites(resolveStorageUrlsInObject(withDerivedQuestions(result.survey))) + await addLegacyEnvironmentId( + addLegacyProjectOverwrites(resolveStorageUrlsInObject(withDerivedQuestions(result.survey))) + ) ), }; } catch (error) { @@ -94,6 +100,8 @@ export const DELETE = withV1ApiWrapper({ if (auditLog) { auditLog.targetId = params.surveyId; } + + let deletedSurvey: Awaited>; try { const result = await fetchAndAuthorizeSurvey(params.surveyId, authentication, "DELETE"); if (result.error) { @@ -105,13 +113,16 @@ export const DELETE = withV1ApiWrapper({ auditLog.oldObject = result.survey; } - const deletedSurvey = await deleteSurvey(params.surveyId); - return { - response: responses.successResponse(deletedSurvey), - }; + deletedSurvey = await deleteSurvey(params.surveyId); } catch (error) { return handleErrorResponse(error); } + + // Enrich outside the delete's try/catch: the survey is already gone, so a lookup failure here + // must not mask a successful delete behind a generic error response. + return { + response: responses.successResponse(await addLegacyEnvironmentIdBestEffort(deletedSurvey)), + }; }, action: "deleted", targetType: "survey", @@ -218,8 +229,13 @@ export const PUT = withV1ApiWrapper({ } return { + // Best-effort, not strict: the update has committed by now, so a failed workspace lookup + // would report a failure for an update that succeeded — and mark the audit entry failed + // with it. response: responses.successResponse( - addLegacyProjectOverwrites(resolveStorageUrlsInObject(withDerivedQuestions(updatedSurvey))) + await addLegacyEnvironmentIdBestEffort( + addLegacyProjectOverwrites(resolveStorageUrlsInObject(withDerivedQuestions(updatedSurvey))) + ) ), }; } catch (error) { diff --git a/apps/web/app/api/v1/management/surveys/route.ts b/apps/web/app/api/v1/management/surveys/route.ts index 24f245c391eb..b83ea2dda63a 100644 --- a/apps/web/app/api/v1/management/surveys/route.ts +++ b/apps/web/app/api/v1/management/surveys/route.ts @@ -8,6 +8,10 @@ import { normaliseProjectOverwritesToWorkspace, } from "@/app/lib/api/api-backwards-compat"; import { handleApiError } from "@/app/lib/api/handle-api-error"; +import { + addLegacyEnvironmentIdBestEffort, + addLegacyEnvironmentIdToList, +} from "@/app/lib/api/legacy-environment-id"; import { RequestBodyTooLargeError, parseJsonBodyWithLimit } from "@/app/lib/api/request-body"; import { responses } from "@/app/lib/api/response"; import { @@ -47,7 +51,9 @@ export const GET = withV1ApiWrapper({ return { response: responses.successResponse( - addLegacyProjectOverwritesToList(resolveStorageUrlsInObject(surveysWithQuestions)) + await addLegacyEnvironmentIdToList( + addLegacyProjectOverwritesToList(resolveStorageUrlsInObject(surveysWithQuestions)) + ) ), }; } catch (error) { @@ -146,8 +152,13 @@ export const POST = withV1ApiWrapper({ } return { + // Best-effort, not strict: the insert has committed by now, so a failed workspace lookup here + // would return an error for a survey that exists. `Survey` has no unique constraint to dedup + // on, so a client retrying that false error creates a second survey. response: responses.successResponse( - addLegacyProjectOverwrites(resolveStorageUrlsInObject(withDerivedQuestions(survey))) + await addLegacyEnvironmentIdBestEffort( + addLegacyProjectOverwrites(resolveStorageUrlsInObject(withDerivedQuestions(survey))) + ) ), }; } catch (error) { diff --git a/apps/web/app/api/v1/webhooks/[webhookId]/route.ts b/apps/web/app/api/v1/webhooks/[webhookId]/route.ts index 988a0832ddbd..7325424c26ae 100644 --- a/apps/web/app/api/v1/webhooks/[webhookId]/route.ts +++ b/apps/web/app/api/v1/webhooks/[webhookId]/route.ts @@ -1,5 +1,9 @@ import { logger } from "@formbricks/logger"; import { deleteWebhook, getWebhook } from "@/app/api/v1/webhooks/[webhookId]/lib/webhook"; +import { + addLegacyEnvironmentId, + addLegacyEnvironmentIdBestEffort, +} from "@/app/lib/api/legacy-environment-id"; import { responses } from "@/app/lib/api/response"; import { THandlerParams, withV1ApiWrapper } from "@/app/lib/api/with-api-logging"; import { hasPermission } from "@/modules/organization/settings/api-keys/lib/utils"; @@ -24,7 +28,7 @@ export const GET = withV1ApiWrapper({ }; } return { - response: responses.successResponse(webhook), + response: responses.successResponse(await addLegacyEnvironmentId(webhook)), }; }, }); @@ -63,11 +67,9 @@ export const DELETE = withV1ApiWrapper({ } // delete webhook from database + let deletedWebhook: Awaited>; try { - const deletedWebhook = await deleteWebhook(params.webhookId); - return { - response: responses.successResponse(deletedWebhook), - }; + deletedWebhook = await deleteWebhook(params.webhookId); } catch (e) { if (auditLog) { auditLog.status = "failure"; @@ -77,6 +79,12 @@ export const DELETE = withV1ApiWrapper({ response: responses.notFoundResponse("Webhook", params.webhookId), }; } + + // Enrich outside the delete's try/catch: the webhook is already gone, so a lookup failure here + // must not report a failed delete (a false 404 plus a "failure" audit entry). + return { + response: responses.successResponse(await addLegacyEnvironmentIdBestEffort(deletedWebhook)), + }; }, action: "deleted", targetType: "webhook", diff --git a/apps/web/app/api/v1/webhooks/route.ts b/apps/web/app/api/v1/webhooks/route.ts index 75822c7b3cd6..a033d15f1443 100644 --- a/apps/web/app/api/v1/webhooks/route.ts +++ b/apps/web/app/api/v1/webhooks/route.ts @@ -2,6 +2,10 @@ import { resolveBodyIds } from "@/app/api/v1/management/lib/workspace-resolver"; import { createWebhook, getWebhooks } from "@/app/api/v1/webhooks/lib/webhook"; import { ZWebhookInput } from "@/app/api/v1/webhooks/types/webhooks"; import { handleApiError } from "@/app/lib/api/handle-api-error"; +import { + addLegacyEnvironmentIdBestEffort, + addLegacyEnvironmentIdToList, +} from "@/app/lib/api/legacy-environment-id"; import { RequestBodyTooLargeError, parseJsonBodyWithLimit } from "@/app/lib/api/request-body"; import { responses } from "@/app/lib/api/response"; import { transformErrorToDetails } from "@/app/lib/api/validator"; @@ -20,7 +24,7 @@ export const GET = withV1ApiWrapper({ ]; const webhooks = await getWebhooks(workspaceIds); return { - response: responses.successResponse(webhooks), + response: responses.successResponse(await addLegacyEnvironmentIdToList(webhooks)), }; } catch (error) { return handleApiError(error); @@ -84,8 +88,13 @@ export const POST = withV1ApiWrapper({ auditLog.newObject = webhook; } + // Best-effort, not strict: the insert has committed by now, and a failed workspace lookup here + // (e.g. a P2024 pool timeout on the helper's own connection checkout) would surface as a 500 for + // a webhook that exists. Zapier retries on that, and `Webhook` has no uniqueness on + // `(url, workspaceId)`, so the retry would silently create a second subscription and duplicate + // every delivery. return { - response: responses.successResponse(webhook), + response: responses.successResponse(await addLegacyEnvironmentIdBestEffort(webhook)), }; } catch (error) { return handleApiError(error); diff --git a/apps/web/app/lib/api/legacy-environment-id.test.ts b/apps/web/app/lib/api/legacy-environment-id.test.ts new file mode 100644 index 000000000000..05efc1fef80a --- /dev/null +++ b/apps/web/app/lib/api/legacy-environment-id.test.ts @@ -0,0 +1,133 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { prisma } from "@formbricks/database"; +import { + addLegacyEnvironmentId, + addLegacyEnvironmentIdBestEffort, + addLegacyEnvironmentIdToList, +} from "./legacy-environment-id"; + +vi.mock("@formbricks/database", () => ({ + prisma: { + workspace: { + findMany: vi.fn(), + }, + }, +})); + +vi.mock("@formbricks/logger", () => ({ + logger: { error: vi.fn() }, +})); + +const findManyMock = vi.mocked(prisma.workspace.findMany); + +describe("addLegacyEnvironmentIdToList", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + test("uses the workspace's legacyEnvironmentId when the workspace was migrated", async () => { + findManyMock.mockResolvedValue([{ id: "ws_1", legacyEnvironmentId: "env_1" }] as never); + + const result = await addLegacyEnvironmentIdToList([{ id: "survey_1", workspaceId: "ws_1" }]); + + expect(result).toEqual([{ id: "survey_1", workspaceId: "ws_1", environmentId: "env_1" }]); + }); + + test("falls back to the workspace id when there is no legacy environment id", async () => { + findManyMock.mockResolvedValue([{ id: "ws_1", legacyEnvironmentId: null }] as never); + + const result = await addLegacyEnvironmentIdToList([{ id: "survey_1", workspaceId: "ws_1" }]); + + expect(result[0].environmentId).toBe("ws_1"); + }); + + test("falls back to the workspace id when the workspace row is missing", async () => { + findManyMock.mockResolvedValue([] as never); + + const result = await addLegacyEnvironmentIdToList([{ id: "survey_1", workspaceId: "ws_1" }]); + + expect(result[0].environmentId).toBe("ws_1"); + }); + + test("resolves each entity against its own workspace and queries unique ids once", async () => { + findManyMock.mockResolvedValue([ + { id: "ws_1", legacyEnvironmentId: "env_1" }, + { id: "ws_2", legacyEnvironmentId: "env_2" }, + ] as never); + + const result = await addLegacyEnvironmentIdToList([ + { id: "a", workspaceId: "ws_1" }, + { id: "b", workspaceId: "ws_2" }, + { id: "c", workspaceId: "ws_1" }, + ]); + + expect(result.map((entity) => entity.environmentId)).toEqual(["env_1", "env_2", "env_1"]); + // Three entities across two workspaces must cost one query for two ids. Deduping has no form in + // the return value — the mapping above passes either way — so the query is the only place it is + // observable. Matched loosely so adding a selected column doesn't break this. + expect(findManyMock).toHaveBeenCalledTimes(1); + expect(findManyMock).toHaveBeenCalledWith( + expect.objectContaining({ where: { id: { in: ["ws_1", "ws_2"] } } }) + ); + }); + + test("returns an empty list without querying", async () => { + const result = await addLegacyEnvironmentIdToList([]); + + expect(result).toEqual([]); + expect(findManyMock).not.toHaveBeenCalled(); + }); +}); + +describe("addLegacyEnvironmentId", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + test("adds environmentId to a single entity while preserving its other fields", async () => { + findManyMock.mockResolvedValue([{ id: "ws_1", legacyEnvironmentId: "env_1" }] as never); + + const result = await addLegacyEnvironmentId({ + id: "webhook_1", + workspaceId: "ws_1", + url: "https://x.co", + }); + + expect(result).toEqual({ + id: "webhook_1", + workspaceId: "ws_1", + url: "https://x.co", + environmentId: "env_1", + }); + }); + + test("propagates lookup failures so a wrong id is never handed to a client", async () => { + findManyMock.mockRejectedValue(new Error("connection lost")); + + await expect(addLegacyEnvironmentId({ id: "survey_1", workspaceId: "ws_1" })).rejects.toThrow( + "connection lost" + ); + }); +}); + +describe("addLegacyEnvironmentIdBestEffort", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + test("enriches like addLegacyEnvironmentId when the lookup succeeds", async () => { + findManyMock.mockResolvedValue([{ id: "ws_1", legacyEnvironmentId: "env_1" }] as never); + + const result = await addLegacyEnvironmentIdBestEffort({ id: "webhook_1", workspaceId: "ws_1" }); + + expect(result).toEqual({ id: "webhook_1", workspaceId: "ws_1", environmentId: "env_1" }); + }); + + test("returns the un-enriched entity when the lookup throws, so a committed write still reports success", async () => { + findManyMock.mockRejectedValue(new Error("connection lost")); + + const result = await addLegacyEnvironmentIdBestEffort({ id: "webhook_1", workspaceId: "ws_1" }); + + expect(result).toEqual({ id: "webhook_1", workspaceId: "ws_1" }); + }); +}); diff --git a/apps/web/app/lib/api/legacy-environment-id.ts b/apps/web/app/lib/api/legacy-environment-id.ts new file mode 100644 index 000000000000..e4c831f1c668 --- /dev/null +++ b/apps/web/app/lib/api/legacy-environment-id.ts @@ -0,0 +1,66 @@ +import "server-only"; +import { prisma } from "@formbricks/database"; +import { logger } from "@formbricks/logger"; + +/** + * Backwards compatibility layer for the environment → workspace rename in API v1 responses. + * + * Before Formbricks 5, v1 surveys and webhooks carried an `environmentId`. Long-lived integrations + * built against v1 (Zapier, Make, n8n) still read that field — e.g. the Zapier "Response Finished" + * trigger derives the subscription's environment id from the surveys it lists — so v1 keeps emitting + * it as `legacyEnvironmentId ?? workspaceId`. + * + * That is the same id `GET /api/v1/management/me` returns and the one `resolveBodyIds` accepts back + * on writes, so a client can round-trip it without knowing about workspaces (ENG-2270). + */ +export const addLegacyEnvironmentIdToList = async ( + entities: T[] +): Promise<(T & { environmentId: string })[]> => { + if (entities.length === 0) return []; + + const workspaceIds = [...new Set(entities.map((entity) => entity.workspaceId))]; + + const workspaces = await prisma.workspace.findMany({ + where: { id: { in: workspaceIds } }, + select: { id: true, legacyEnvironmentId: true }, + }); + + const legacyEnvironmentIdByWorkspaceId = new Map( + workspaces.map((workspace) => [workspace.id, workspace.legacyEnvironmentId]) + ); + + return entities.map((entity) => ({ + ...entity, + // Workspaces created after the migration have no legacy id; their own id is the v1 environment id. + environmentId: legacyEnvironmentIdByWorkspaceId.get(entity.workspaceId) ?? entity.workspaceId, + })); +}; + +export const addLegacyEnvironmentId = async ( + entity: T +): Promise => { + const [entityWithLegacyId] = await addLegacyEnvironmentIdToList([entity]); + return entityWithLegacyId; +}; + +/** + * Variant for responses that echo an already-committed write. + * + * The write cannot be undone by the time this runs, so a failed workspace lookup must not turn it + * into an error response — the caller would retry an operation that already happened. On a delete + * that means deleting twice; on a create it means a duplicate row, since `Webhook` has no + * uniqueness on `(url, workspaceId)`. Degrades to the un-enriched entity instead. + */ +export const addLegacyEnvironmentIdBestEffort = async ( + entity: T +): Promise => { + try { + return await addLegacyEnvironmentId(entity); + } catch (error) { + logger.error( + { error, workspaceId: entity.workspaceId }, + "Failed to resolve legacy environmentId for a committed write" + ); + return entity; + } +}; diff --git a/apps/web/playwright/api/management/legacy-environment-id.spec.ts b/apps/web/playwright/api/management/legacy-environment-id.spec.ts new file mode 100644 index 000000000000..479f33eefc89 --- /dev/null +++ b/apps/web/playwright/api/management/legacy-environment-id.spec.ts @@ -0,0 +1,131 @@ +import { expect } from "@playwright/test"; +import { logger } from "@formbricks/logger"; +import { test } from "../../lib/fixtures"; +import { loginAndGetApiKey } from "../../lib/utils"; +import { SURVEYS_API_URL } from "../constants"; + +const V1_WEBHOOKS_API_URL = "/api/v1/webhooks"; + +// ENG-2270: integrations built against API v1 (Zapier, Make, n8n) read `environmentId` off the +// surveys they list and post it back when subscribing a webhook. This walks that exact path so the +// legacy field can't silently disappear from v1 again. +test.describe("API v1 legacy environmentId", () => { + test("Zapier-style webhook subscription round-trips environmentId from the survey list", async ({ + page, + users, + request, + }) => { + let workspaceId: string; + let apiKey: string; + + try { + ({ workspaceId, apiKey } = await loginAndGetApiKey(page, users)); + } catch (error) { + logger.error(error, "Error during login and getting API key"); + throw error; + } + + let surveyId: string; + + await test.step("Create Survey via API", async () => { + const response = await request.post(SURVEYS_API_URL, { + headers: { + "Content-Type": "application/json", + "x-api-key": apiKey, + }, + data: { + workspaceId, + type: "link", + name: "Survey for legacy environmentId", + questions: [ + { + id: "jpvm9b73u06xdrhzi11k2h76", + type: "openText", + headline: { default: "What would you like to know?" }, + required: true, + inputType: "text", + }, + ], + }, + }); + + expect(response.ok()).toBe(true); + const responseBody = await response.json(); + surveyId = responseBody.data.id; + expect(responseBody.data.environmentId).toBeTruthy(); + }); + + let environmentId: string; + + await test.step("Survey list exposes environmentId alongside workspaceId", async () => { + const response = await request.get(SURVEYS_API_URL, { + headers: { "x-api-key": apiKey }, + }); + + expect(response.ok()).toBe(true); + const responseBody = await response.json(); + const survey = responseBody.data.find((item: { id: string }) => item.id === surveyId); + + expect(survey).toBeTruthy(); + expect(survey.workspaceId).toBe(workspaceId); + expect(typeof survey.environmentId).toBe("string"); + expect(survey.environmentId).not.toBe("undefined"); + + environmentId = survey.environmentId; + }); + + let createdWebhookId: string; + + await test.step("Create webhook with environmentId only", async () => { + const response = await request.post(V1_WEBHOOKS_API_URL, { + headers: { + "Content-Type": "application/json", + "x-api-key": apiKey, + }, + data: { + environmentId, + name: "Zapier Response Finished", + url: "https://example.com/zapier-webhook", + source: "zapier", + triggers: ["responseFinished"], + surveyIds: [surveyId], + }, + }); + + expect(response.ok()).toBe(true); + const responseBody = await response.json(); + expect(responseBody.data.workspaceId).toBe(workspaceId); + expect(responseBody.data.environmentId).toBe(environmentId); + createdWebhookId = responseBody.data.id; + }); + + await test.step("Webhook list and detail expose environmentId", async () => { + const listResponse = await request.get(V1_WEBHOOKS_API_URL, { + headers: { "x-api-key": apiKey }, + }); + + expect(listResponse.ok()).toBe(true); + const listBody = await listResponse.json(); + const webhook = listBody.data.find((item: { id: string }) => item.id === createdWebhookId); + expect(webhook.environmentId).toBe(environmentId); + + const detailResponse = await request.get(`${V1_WEBHOOKS_API_URL}/${createdWebhookId}`, { + headers: { "x-api-key": apiKey }, + }); + + expect(detailResponse.ok()).toBe(true); + const detailBody = await detailResponse.json(); + expect(detailBody.data.environmentId).toBe(environmentId); + }); + + await test.step("Delete webhook via API", async () => { + const response = await request.delete(`${V1_WEBHOOKS_API_URL}/${createdWebhookId}`, { + headers: { "x-api-key": apiKey }, + }); + + expect(response.ok()).toBe(true); + const responseBody = await response.json(); + expect(responseBody.data.environmentId).toBe(environmentId); + }); + }); +}); From fe07c9694c3fb9c5941d57bff566e88346c1241e Mon Sep 17 00:00:00 2001 From: Dhruwang Jariwala <67850763+Dhruwang@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:04:05 +0000 Subject: [PATCH 4/9] ci: drive the breaking-change label from a checkbox (ENG-2332) (#8862) --- .coderabbit.yaml | 25 +++++---- .github/pull_request_template.md | 21 +++++--- .github/workflows/pr-label-sync.yml | 82 ++++++++++++++++------------- AGENTS.md | 2 + 4 files changed, 77 insertions(+), 53 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index c4e0871db8ff..cdbe5780b8de 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -320,14 +320,17 @@ reviews: mode: warning issue_assessment: mode: warning - # `pr-label-sync.yml` reads what the Breaking changes section says; these check whether - # what it says is true of the diff, and whether the PR's own QA covers every behaviour it - # changes. Both are advisory — nothing here blocks a merge. + # `pr-label-sync.yml` reads the checkbox in the Breaking changes section; these check whether + # what the author ticked is true of the diff, and whether the PR's own QA covers every behaviour + # it changes. Both are advisory — nothing here blocks a merge. custom_checks: - name: Breaking changes match the diff mode: warning instructions: | - Compare the PR description's "## Breaking changes" section against the actual diff. + Compare the checkbox in the PR description's "## Breaking changes" section against the + actual diff. A ticked box (`- [x] This PR contains breaking changes`) is the author's + declaration that the PR has one; an unticked box declares that it has none. The prose or + table under the checkbox is the explanation and does not change the declaration. Treat a change as breaking if the diff does any of the following: - Renames, removes, or retypes a field in an API request or response shape, or changes @@ -341,15 +344,15 @@ reviews: - Changes an exported signature of the public SDK surface in `packages/js-core` or `packages/surveys`. - FAIL if the diff contains at least one of the above and the "Breaking changes" section - says "None", is empty, or still holds the unedited template comment. Name the specific - file and line that is breaking. + FAIL if the diff contains at least one of the above and the checkbox is unticked, or the + section is empty or still holds the unedited template comment. Name the specific file and + line that is breaking, and ask the author to tick the box. - FAIL if the section claims a breaking change that has no corresponding change in the diff. + FAIL if the checkbox is ticked but no change in the diff matches the list above. - PASS if the section describes, in a table or in prose, each breaking change present in - the diff — `.github/pull_request_template.md` accepts either shape — or if the diff - contains none of the above and the section says "None". + PASS if the checkbox is ticked and the section documents each breaking change present in + the diff, in a table or in prose, or if the checkbox is unticked and the diff contains + none of the above — however the author words the explanation underneath. Return inconclusive rather than failing when the diff alone does not let you determine an API response shape — for example when the shape comes from a serializer or type that diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 3b894a9c0b06..0cb5bb5cfc0b 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -23,15 +23,24 @@ to grasp without opening files. --> ## Breaking changes - + + +- [ ] This PR contains breaking changes None -