diff --git a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.test.tsx b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.test.tsx index eb2ccd83f8..5fe058471c 100644 --- a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.test.tsx +++ b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.test.tsx @@ -12,6 +12,8 @@ import { TSurvey } from "@formbricks/types/surveys/types"; import { TTag } from "@formbricks/types/tags"; import { TUserLocale } from "@formbricks/types/user"; +vi.mock("@sentry/nextjs", () => ({ captureException: vi.fn() })); + // Mock react-hot-toast vi.mock("react-hot-toast", () => ({ default: { @@ -273,46 +275,58 @@ beforeEach(() => { vi.spyOn(document.body, "removeChild").mockReturnValue(null as any); // Mock File constructor to avoid arrayBuffer issues - global.File = class MockFile { - name: string; - type: string; - size: number; + vi.stubGlobal( + "File", + class MockFile { + name: string; + type: string; + size: number; - constructor(chunks: any[], name: string, options: any = {}) { - this.name = name; - this.type = options.type || ""; - this.size = options.size || 0; - } + constructor(_chunks: any[], name: string, options: any = {}) { + this.name = name; + this.type = options.type || ""; + this.size = options.size || 0; + } - arrayBuffer() { - return Promise.resolve(new ArrayBuffer(0)); - } - } as any; + arrayBuffer() { + return Promise.resolve(new ArrayBuffer(0)); + } + } as any + ); // Mock atob for base64 decoding - global.atob = vi.fn((str: string) => "decoded binary string"); + vi.stubGlobal( + "atob", + vi.fn((_str: string) => "decoded binary string") + ); // Mock Uint8Array and Blob - global.Uint8Array = class MockUint8Array extends Array { - constructor(data: any) { - super(); - this.length = typeof data === "number" ? data : 0; - } + vi.stubGlobal( + "Uint8Array", + class MockUint8Array extends Array { + constructor(data: any) { + super(); + this.length = typeof data === "number" ? data : 0; + } - static from(source: any) { - return new MockUint8Array(source.length || 0); - } - } as any; + static from(source: any) { + return new MockUint8Array(source.length || 0); + } + } as any + ); - global.Blob = class MockBlob { - size: number; - type: string; + vi.stubGlobal( + "Blob", + class MockBlob { + size: number; + type: string; - constructor(parts: any[], options: any = {}) { - this.size = 0; - this.type = options.type || ""; - } - } as any; + constructor(_parts: any[], options: any = {}) { + this.size = 0; + this.type = options.type || ""; + } + } as any + ); }); // Cleanup after each test @@ -323,6 +337,7 @@ afterEach(() => { } cleanup(); vi.restoreAllMocks(); // Restore mocks after each test + vi.unstubAllGlobals(); // Restore global stubs after each test }); describe("ResponseTable", () => { diff --git a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx index 3245a9564a..f21d14aca1 100644 --- a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx +++ b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTable.tsx @@ -212,7 +212,9 @@ export const ResponseTable = ({ }); } - const { url, error } = await handleFileUpload(file, environment.id); + const { url, error } = await handleFileUpload(file, environment.id, [ + format === "xlsx" ? ".xlsx" : ".csv", + ]); if (error) { toast.error(t("environments.surveys.responses.error_downloading_responses")); @@ -220,7 +222,7 @@ export const ResponseTable = ({ const link = document.createElement("a"); link.href = url; - link.download = downloadResponse.data.fileName || `${survey.name}-${format}.csv`; + link.download = downloadResponse.data.fileName || `${survey.name}-${format}.${format}`; document.body.appendChild(link); link.click(); diff --git a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/personal-links-tab.tsx b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/personal-links-tab.tsx index 741f3bba93..5e45f62963 100644 --- a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/personal-links-tab.tsx +++ b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/shareEmbedModal/personal-links-tab.tsx @@ -131,6 +131,7 @@ export const PersonalLinksTab = ({ if (error) { toast.error(t("environments.surveys.share.personal_links.error_generating_links")); + setIsGenerating(false); return; } diff --git a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/components/CustomFilter.tsx b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/components/CustomFilter.tsx index 7bfc309f7f..e15a0f7336 100755 --- a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/components/CustomFilter.tsx +++ b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/components/CustomFilter.tsx @@ -277,11 +277,13 @@ export const CustomFilter = ({ survey }: CustomFilterProps) => { if (error) { toast.error(t("environments.surveys.responses.error_downloading_responses")); + setIsDownloading(false); + return; } const link = document.createElement("a"); link.href = url; - link.download = responsesDownloadUrlResponse.data.fileName || `${survey.name}-${filetype}.csv`; + link.download = responsesDownloadUrlResponse.data.fileName || `${survey.name}-${filetype}.${filetype}`; document.body.appendChild(link); link.click(); diff --git a/apps/web/app/api/v1/client/[environmentId]/storage/route.ts b/apps/web/app/api/v1/client/[environmentId]/storage/route.ts index 0470726eab..3da37ce1f4 100644 --- a/apps/web/app/api/v1/client/[environmentId]/storage/route.ts +++ b/apps/web/app/api/v1/client/[environmentId]/storage/route.ts @@ -6,6 +6,7 @@ import { getOrganizationByEnvironmentId } from "@/lib/organization/service"; import { getSurvey } from "@/lib/survey/service"; import { getBiggerUploadFileSizePermission } from "@/modules/ee/license-check/lib/utils"; import { getSignedUrlForUpload } from "@/modules/storage/service"; +import { getErrorResponseFromStorageError } from "@/modules/storage/utils"; import { NextRequest } from "next/server"; import { logger } from "@formbricks/logger"; import { TUploadPrivateFileRequest, ZUploadPrivateFileRequest } from "@formbricks/types/storage"; @@ -103,9 +104,9 @@ export const POST = withV1ApiWrapper({ if (!signedUrlResponse.ok) { logger.error({ error: signedUrlResponse.error }, "Error getting signed url for upload"); - + const errorResponse = getErrorResponseFromStorageError(signedUrlResponse.error); return { - response: responses.internalServerErrorResponse("Internal server error"), + response: errorResponse, }; } diff --git a/apps/web/app/api/v1/management/storage/route.ts b/apps/web/app/api/v1/management/storage/route.ts index 23a6dc105e..88f9af545c 100644 --- a/apps/web/app/api/v1/management/storage/route.ts +++ b/apps/web/app/api/v1/management/storage/route.ts @@ -3,6 +3,7 @@ import { responses } from "@/app/lib/api/response"; import { transformErrorToDetails } from "@/app/lib/api/validator"; import { TApiV1Authentication, withV1ApiWrapper } from "@/app/lib/api/with-api-logging"; import { getSignedUrlForUpload } from "@/modules/storage/service"; +import { getErrorResponseFromStorageError } from "@/modules/storage/utils"; import { NextRequest } from "next/server"; import { logger } from "@formbricks/logger"; import { TUploadPublicFileRequest, ZUploadPublicFileRequest } from "@formbricks/types/storage"; @@ -53,8 +54,10 @@ export const POST = withV1ApiWrapper({ const signedUrlResponse = await getSignedUrlForUpload(fileName, environmentId, fileType, "public"); if (!signedUrlResponse.ok) { + logger.error({ error: signedUrlResponse.error }, "Error getting signed url for upload"); + const errorResponse = getErrorResponseFromStorageError(signedUrlResponse.error); return { - response: responses.internalServerErrorResponse("Internal server error"), + response: errorResponse, }; } diff --git a/apps/web/app/storage/[environmentId]/[accessType]/[fileName]/route.ts b/apps/web/app/storage/[environmentId]/[accessType]/[fileName]/route.ts index fd38d0d040..922b5fcf04 100644 --- a/apps/web/app/storage/[environmentId]/[accessType]/[fileName]/route.ts +++ b/apps/web/app/storage/[environmentId]/[accessType]/[fileName]/route.ts @@ -7,6 +7,7 @@ import { authOptions } from "@/modules/auth/lib/authOptions"; import { queueAuditEvent } from "@/modules/ee/audit-logs/lib/handler"; import { TAuditStatus, UNKNOWN_DATA } from "@/modules/ee/audit-logs/types/audit-log"; import { deleteFile, getSignedUrlForDownload } from "@/modules/storage/service"; +import { getErrorResponseFromStorageError } from "@/modules/storage/utils"; import { getServerSession } from "next-auth"; import { type NextRequest } from "next/server"; import { logger } from "@formbricks/logger"; @@ -102,13 +103,8 @@ export const GET = async ( const signedUrlResult = await getSignedUrlForDownload(fileName, environmentId, accessType); if (!signedUrlResult.ok) { - if (signedUrlResult.error.code === "file_not_found_error") { - logger.info({ error: signedUrlResult.error }, "File not found"); - return responses.notFoundResponse("File", fileName); - } - - logger.error({ error: signedUrlResult.error }, "Error getting signed url for download"); - return responses.internalServerErrorResponse("Internal server error"); + const errorResponse = getErrorResponseFromStorageError(signedUrlResult.error, { fileName }); + return errorResponse; } return new Response(null, { diff --git a/apps/web/modules/ee/contacts/components/upload-contacts-button.tsx b/apps/web/modules/ee/contacts/components/upload-contacts-button.tsx index ea73c5d291..5d6042b52f 100644 --- a/apps/web/modules/ee/contacts/components/upload-contacts-button.tsx +++ b/apps/web/modules/ee/contacts/components/upload-contacts-button.tsx @@ -256,7 +256,7 @@ export const UploadContactsCSVButton = ({ const headers = Object.keys(exampleData[0]); const csvRows = [headers.join(","), ...exampleData.map((row) => headers.map((h) => row[h]).join(","))]; const csvContent = "data:text/csv;charset=utf-8," + csvRows.join("\n"); - const encodedUri = encodeURI(csvContent); + const encodedUri = encodeURIComponent(csvContent); const link = document.createElement("a"); link.setAttribute("href", encodedUri); diff --git a/apps/web/modules/projects/settings/lib/project.ts b/apps/web/modules/projects/settings/lib/project.ts index cfb735dec0..887ff19c54 100644 --- a/apps/web/modules/projects/settings/lib/project.ts +++ b/apps/web/modules/projects/settings/lib/project.ts @@ -144,11 +144,13 @@ export const deleteProject = async (projectId: string): Promise => { return deleteFilesByEnvironmentId(environment.id); }); - try { - await Promise.all(s3FilesPromises); - } catch (err) { - // fail silently because we don't want to throw an error if the files are not deleted - logger.error(err, "Error deleting S3 files"); + const s3FilesResult = await Promise.all(s3FilesPromises); + + for (const result of s3FilesResult) { + if (!result.ok) { + // fail silently because we don't want to throw an error if the files are not deleted + logger.error(result.error, "Error deleting S3 files"); + } } } diff --git a/apps/web/modules/storage/service.test.ts b/apps/web/modules/storage/service.test.ts index 1c1b416d82..dd82318123 100644 --- a/apps/web/modules/storage/service.test.ts +++ b/apps/web/modules/storage/service.test.ts @@ -30,9 +30,11 @@ vi.mock("@formbricks/logger", () => ({ vi.mock("@formbricks/storage", () => ({ StorageErrorCode: { - Unknown: "UNKNOWN", - FileNotFound: "FILE_NOT_FOUND", - FileSizeExceeded: "FILE_SIZE_EXCEEDED", + Unknown: "unknown", + S3ClientError: "s3_client_error", + S3CredentialsError: "s3_credentials_error", + FileNotFoundError: "file_not_found_error", + InvalidInput: "invalid_input", }, deleteFile: vi.fn(), deleteFilesByPrefix: vi.fn(), @@ -126,6 +128,40 @@ describe("storage service", () => { } }); + test("should properly encode filenames with special characters like # in URL", async () => { + const mockSignedUrlResponse = { + ok: true, + data: { + signedUrl: "https://s3.example.com/upload", + presignedFields: { key: "value" }, + }, + } as MockedSignedUploadReturn; + + vi.mocked(getSignedUploadUrl).mockResolvedValue(mockSignedUrlResponse); + + const result = await getSignedUrlForUpload( + "test#file.txt", + "env-123", + "text/plain", + "public" as TAccessType + ); + + expect(result.ok).toBe(true); + if (result.ok) { + // The filename should be URL-encoded to prevent # from being treated as a URL fragment + expect(result.data.fileUrl).toBe( + `https://public.example.com/storage/env-123/public/test%23file--fid--${mockUUID}.txt` + ); + } + + expect(getSignedUploadUrl).toHaveBeenCalledWith( + `test#file--fid--${mockUUID}.txt`, + "text/plain", + "env-123/public", + 1024 * 1024 * 10 // 10MB default + ); + }); + test("should handle files with multiple dots in filename", async () => { const mockSignedUrlResponse = { ok: true, diff --git a/apps/web/modules/storage/service.ts b/apps/web/modules/storage/service.ts index f0c7ebae15..5b54240e22 100644 --- a/apps/web/modules/storage/service.ts +++ b/apps/web/modules/storage/service.ts @@ -52,7 +52,9 @@ export const getSignedUrlForUpload = async ( return ok({ signedUrl: signedUrlResult.data.signedUrl, presignedFields: signedUrlResult.data.presignedFields, - fileUrl: new URL(`${baseUrl}/storage/${environmentId}/${accessType}/${updatedFileName}`).href, + fileUrl: new URL( + `${baseUrl}/storage/${environmentId}/${accessType}/${encodeURIComponent(updatedFileName)}` + ).href, }); } catch (error) { logger.error({ error }, "Error getting signed url for upload"); diff --git a/apps/web/modules/storage/utils.test.ts b/apps/web/modules/storage/utils.test.ts index 88b496f903..03aa515415 100644 --- a/apps/web/modules/storage/utils.test.ts +++ b/apps/web/modules/storage/utils.test.ts @@ -23,7 +23,16 @@ vi.mock("@/modules/storage/utils", async () => { describe("storage utils", () => { beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); + + // Default: derive filename from URL path for positive-path tests + mockGetOriginalFileNameFromUrl.mockImplementation((url: string) => { + try { + return new URL(url).pathname.split("/").filter(Boolean).pop(); + } catch { + return undefined; + } + }); }); describe("isAllowedFileExtension", () => { @@ -167,7 +176,7 @@ describe("storage utils", () => { test("should return false when file name cannot be extracted", () => { // Mock implementation to return null for this specific URL - mockGetOriginalFileNameFromUrl.mockImplementation(() => undefined); + mockGetOriginalFileNameFromUrl.mockImplementationOnce(() => undefined); const responseData = { question1: ["https://example.com/invalid-url"], @@ -185,7 +194,7 @@ describe("storage utils", () => { }); test("should return false when file has no extension", () => { - mockGetOriginalFileNameFromUrl.mockImplementation(() => "file-without-extension"); + mockGetOriginalFileNameFromUrl.mockImplementationOnce(() => "file-without-extension"); const responseData = { question1: ["https://example.com/storage/file-without-extension"], @@ -248,22 +257,22 @@ describe("storage utils", () => { }); test("should return false when file name cannot be extracted", () => { - mockGetOriginalFileNameFromUrl.mockImplementation(() => undefined); + mockGetOriginalFileNameFromUrl.mockImplementationOnce(() => undefined); expect(isValidImageFile("https://example.com/invalid-url")).toBe(false); }); test("should return false when file has no extension", () => { - mockGetOriginalFileNameFromUrl.mockImplementation(() => "image-without-extension"); + mockGetOriginalFileNameFromUrl.mockImplementationOnce(() => "image-without-extension"); expect(isValidImageFile("https://example.com/image-without-extension")).toBe(false); }); test("should return false when file name ends with a dot", () => { - mockGetOriginalFileNameFromUrl.mockImplementation(() => "image."); + mockGetOriginalFileNameFromUrl.mockImplementationOnce(() => "image."); expect(isValidImageFile("https://example.com/image.")).toBe(false); }); test("should handle case insensitivity correctly", () => { - mockGetOriginalFileNameFromUrl.mockImplementation(() => "image.JPG"); + mockGetOriginalFileNameFromUrl.mockImplementationOnce(() => "image.JPG"); expect(isValidImageFile("https://example.com/image.JPG")).toBe(true); }); }); diff --git a/apps/web/modules/storage/utils.ts b/apps/web/modules/storage/utils.ts index 81e72438d3..cbc7034465 100644 --- a/apps/web/modules/storage/utils.ts +++ b/apps/web/modules/storage/utils.ts @@ -1,39 +1,28 @@ +import { responses } from "@/app/lib/api/response"; import { logger } from "@formbricks/logger"; +import { StorageError, StorageErrorCode } from "@formbricks/storage"; import { TResponseData } from "@formbricks/types/responses"; import { TAllowedFileExtension, ZAllowedFileExtension, mimeTypes } from "@formbricks/types/storage"; import { TSurveyQuestion, TSurveyQuestionTypeEnum } from "@formbricks/types/surveys/types"; export const getOriginalFileNameFromUrl = (fileURL: string) => { try { - const fileNameFromURL = fileURL.startsWith("/storage/") - ? fileURL.split("/").pop() - : new URL(fileURL).pathname.split("/").pop(); + const lastSegment = fileURL.startsWith("/storage/") + ? fileURL + : (new URL(fileURL).pathname.split("/").pop() ?? ""); + const fileNameFromURL = lastSegment.split(/[?#]/)[0]; - const fileExt = fileNameFromURL?.split(".").pop() ?? ""; - const originalFileName = fileNameFromURL?.split("--fid--")[0] ?? ""; - const fileId = fileNameFromURL?.split("--fid--")[1] ?? ""; + const [namePart, fidPart] = fileNameFromURL.split("--fid--"); + if (!fidPart) return namePart ? decodeURIComponent(namePart) : ""; - if (!fileId) { - const fileName = originalFileName ? decodeURIComponent(originalFileName || "") : ""; - return fileName; - } + const dotIdx = fileNameFromURL.lastIndexOf("."); + const hasExt = dotIdx > fileNameFromURL.indexOf("--fid--"); + const ext = hasExt ? fileNameFromURL.slice(dotIdx + 1) : ""; - const fileName = originalFileName ? decodeURIComponent(`${originalFileName}.${fileExt}` || "") : ""; - return fileName; + return decodeURIComponent(ext ? `${namePart}.${ext}` : namePart); } catch (error) { - logger.error(error, "Error parsing file URL"); - } -}; - -export const getFileNameWithIdFromUrl = (fileURL: string) => { - try { - const fileNameFromURL = fileURL.startsWith("/storage/") - ? fileURL.split("/").pop() - : new URL(fileURL).pathname.split("/").pop(); - - return fileNameFromURL ? decodeURIComponent(fileNameFromURL || "") : ""; - } catch (error) { - logger.error(error, "Error parsing file URL"); + logger.error({ error, fileURL }, "Error parsing file URL"); + return ""; } }; @@ -72,15 +61,10 @@ export const validateSingleFile = ( fileUrl: string, allowedFileExtensions?: TAllowedFileExtension[] ): boolean => { - console.log("validateSingleFile", fileUrl); const fileName = getOriginalFileNameFromUrl(fileUrl); - console.log("fileName", fileName); if (!fileName) return false; const extension = fileName.split(".").pop(); - console.log("extension", extension); if (!extension) return false; - console.log("allowedFileExtensions", allowedFileExtensions); - console.log("includes", allowedFileExtensions?.includes(extension as TAllowedFileExtension)); return !allowedFileExtensions || allowedFileExtensions.includes(extension as TAllowedFileExtension); }; @@ -112,3 +96,24 @@ export const isValidImageFile = (fileUrl: string): boolean => { const imageExtensions = ["png", "jpeg", "jpg", "webp", "heic"]; return imageExtensions.includes(extension); }; + +export const getErrorResponseFromStorageError = ( + error: StorageError, + details?: Record +): Response => { + switch (error.code) { + case StorageErrorCode.FileNotFoundError: + return responses.notFoundResponse("file", details?.fileName ?? null, true); + case StorageErrorCode.InvalidInput: + return responses.badRequestResponse("Invalid input", details, true); + case StorageErrorCode.S3ClientError: + return responses.internalServerErrorResponse("Internal server error", true); + case StorageErrorCode.S3CredentialsError: + return responses.internalServerErrorResponse("Internal server error", true); + case StorageErrorCode.Unknown: + return responses.internalServerErrorResponse("Internal server error", true); + default: { + return responses.internalServerErrorResponse("Internal server error", true); + } + } +}; diff --git a/packages/storage/src/service.ts b/packages/storage/src/service.ts index 4219ed0d6a..3491c9c982 100644 --- a/packages/storage/src/service.ts +++ b/packages/storage/src/service.ts @@ -175,6 +175,11 @@ export const deleteFile = async (fileKey: string): Promise> => { try { if (!s3Client) { diff --git a/packages/surveys/src/lib/api-client.ts b/packages/surveys/src/lib/api-client.ts index ff614e8e62..70a3d2db1f 100644 --- a/packages/surveys/src/lib/api-client.ts +++ b/packages/surveys/src/lib/api-client.ts @@ -104,6 +104,10 @@ export class ApiClient { fileUrl: string; }; + if (!signedUrl || !presignedFields || !fileUrl) { + throw new Error("Invalid response"); + } + const formData = new FormData(); Object.entries(presignedFields).forEach(([key, value]) => { @@ -130,6 +134,7 @@ export class ApiClient { }); } catch (err) { console.error("Error uploading file", err); + throw new Error("Network error while uploading file"); } if (!uploadResponse.ok) { diff --git a/packages/types/storage.ts b/packages/types/storage.ts index 3109ada5d9..b1f121077d 100644 --- a/packages/types/storage.ts +++ b/packages/types/storage.ts @@ -160,7 +160,7 @@ const refineFileUploadInput = ({ }; ctx: z.RefinementCtx; }): void => { - const fileExtension = data.fileName.split(".").pop() as TAllowedFileExtension | undefined; + const fileExtension = data.fileName.split(".").pop()?.toLowerCase() as TAllowedFileExtension | undefined; if (!fileExtension || fileExtension.toLowerCase() === data.fileName.toLowerCase()) { ctx.addIssue({ @@ -184,7 +184,8 @@ const refineFileUploadInput = ({ return; } - if (data.fileType.toLowerCase() !== mimeTypes[fileExtension]) { + const normalizedFileType = data.fileType.toLowerCase().split(";")[0]; // removes parameters from fileType like "image/jpeg; charset=binary" + if (normalizedFileType !== mimeTypes[fileExtension]) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: "File type doesn't match the file extension",