From ac7831fa3d04ba4ab3c160c8405285bf003ea511 Mon Sep 17 00:00:00 2001 From: Dhruwang Jariwala <67850763+Dhruwang@users.noreply.github.com> Date: Wed, 11 Jun 2025 10:26:20 +0530 Subject: [PATCH] fix: auth checks in storage management api (#5931) --- .../v1/management/storage/lib/utils.test.ts | 204 ++++++++++++++++++ .../api/v1/management/storage/lib/utils.ts | 38 ++++ .../api/v1/management/storage/local/route.ts | 36 +--- .../app/api/v1/management/storage/route.ts | 37 +--- 4 files changed, 259 insertions(+), 56 deletions(-) create mode 100644 apps/web/app/api/v1/management/storage/lib/utils.test.ts create mode 100644 apps/web/app/api/v1/management/storage/lib/utils.ts diff --git a/apps/web/app/api/v1/management/storage/lib/utils.test.ts b/apps/web/app/api/v1/management/storage/lib/utils.test.ts new file mode 100644 index 0000000000..33cff35de6 --- /dev/null +++ b/apps/web/app/api/v1/management/storage/lib/utils.test.ts @@ -0,0 +1,204 @@ +import { checkForRequiredFields } from "./utils"; +import { describe, test, expect } from "vitest"; +import { responses } from "@/app/lib/api/response"; +import { hasUserEnvironmentAccess } from "@/lib/environment/auth"; +import { hasPermission } from "@/modules/organization/settings/api-keys/lib/utils"; +import { NextRequest } from "next/server"; +import { Session } from "next-auth"; +import { vi } from "vitest"; +import { TAuthenticationApiKey } from "@formbricks/types/auth"; +import { authenticateRequest } from "@/app/api/v1/auth"; +import { checkAuth } from "./utils"; + +// Create mock response objects +const mockBadRequestResponse = new Response("Bad Request", { status: 400 }); +const mockNotAuthenticatedResponse = new Response("Not authenticated", { status: 401 }); +const mockUnauthorizedResponse = new Response("Unauthorized", { status: 401 }); + +vi.mock("@/app/api/v1/auth", () => ({ + authenticateRequest: vi.fn(), +})); + +vi.mock("@/lib/environment/auth", () => ({ + hasUserEnvironmentAccess: vi.fn(), +})); + +vi.mock("@/modules/organization/settings/api-keys/lib/utils", () => ({ + hasPermission: vi.fn(), +})); + +vi.mock("@/app/lib/api/response", () => ({ + responses: { + badRequestResponse: vi.fn(() => mockBadRequestResponse), + notAuthenticatedResponse: vi.fn(() => mockNotAuthenticatedResponse), + unauthorizedResponse: vi.fn(() => mockUnauthorizedResponse), + }, +})); + +describe("checkForRequiredFields", () => { + test("should return undefined when all required fields are present", () => { + const result = checkForRequiredFields("env-123", "image/png", "test-file.png"); + expect(result).toBeUndefined(); + }); + + test("should return bad request response when environmentId is missing", () => { + const result = checkForRequiredFields("", "image/png", "test-file.png"); + expect(responses.badRequestResponse).toHaveBeenCalledWith("environmentId is required"); + expect(result).toBe(mockBadRequestResponse); + }); + + test("should return bad request response when fileType is missing", () => { + const result = checkForRequiredFields("env-123", "", "test-file.png"); + expect(responses.badRequestResponse).toHaveBeenCalledWith("contentType is required"); + expect(result).toBe(mockBadRequestResponse); + }); + + test("should return bad request response when encodedFileName is missing", () => { + const result = checkForRequiredFields("env-123", "image/png", ""); + expect(responses.badRequestResponse).toHaveBeenCalledWith("fileName is required"); + expect(result).toBe(mockBadRequestResponse); + }); + + test("should return bad request response when environmentId is undefined", () => { + const result = checkForRequiredFields(undefined as any, "image/png", "test-file.png"); + expect(responses.badRequestResponse).toHaveBeenCalledWith("environmentId is required"); + expect(result).toBe(mockBadRequestResponse); + }); + + test("should return bad request response when fileType is undefined", () => { + const result = checkForRequiredFields("env-123", undefined as any, "test-file.png"); + expect(responses.badRequestResponse).toHaveBeenCalledWith("contentType is required"); + expect(result).toBe(mockBadRequestResponse); + }); + + test("should return bad request response when encodedFileName is undefined", () => { + const result = checkForRequiredFields("env-123", "image/png", undefined as any); + expect(responses.badRequestResponse).toHaveBeenCalledWith("fileName is required"); + expect(result).toBe(mockBadRequestResponse); + }); +}); + +describe("checkAuth", () => { + const environmentId = "env-123"; + const mockRequest = new NextRequest("http://localhost:3000/api/test"); + + test("returns notAuthenticatedResponse when no session and no authentication", async () => { + vi.mocked(authenticateRequest).mockResolvedValue(null); + + const result = await checkAuth(null, environmentId, mockRequest); + + expect(authenticateRequest).toHaveBeenCalledWith(mockRequest); + expect(responses.notAuthenticatedResponse).toHaveBeenCalled(); + expect(result).toBe(mockNotAuthenticatedResponse); + }); + + test("returns unauthorizedResponse when no session and authentication lacks POST permission", async () => { + const mockAuthentication: TAuthenticationApiKey = { + type: "apiKey", + environmentPermissions: [ + { + environmentId: "env-123", + permission: "read", + environmentType: "development", + projectId: "project-1", + projectName: "Project 1", + }, + ], + hashedApiKey: "hashed-key", + apiKeyId: "api-key-id", + organizationId: "org-id", + organizationAccess: { + accessControl: {}, + }, + }; + + vi.mocked(authenticateRequest).mockResolvedValue(mockAuthentication); + vi.mocked(hasPermission).mockReturnValue(false); + + const result = await checkAuth(null, environmentId, mockRequest); + + expect(authenticateRequest).toHaveBeenCalledWith(mockRequest); + expect(hasPermission).toHaveBeenCalledWith(mockAuthentication.environmentPermissions, environmentId, "POST"); + expect(responses.unauthorizedResponse).toHaveBeenCalled(); + expect(result).toBe(mockUnauthorizedResponse); + }); + + test("returns undefined when no session and authentication has POST permission", async () => { + const mockAuthentication: TAuthenticationApiKey = { + type: "apiKey", + environmentPermissions: [ + { + environmentId: "env-123", + permission: "write", + environmentType: "development", + projectId: "project-1", + projectName: "Project 1", + }, + ], + hashedApiKey: "hashed-key", + apiKeyId: "api-key-id", + organizationId: "org-id", + organizationAccess: { + accessControl: {}, + }, + }; + + vi.mocked(authenticateRequest).mockResolvedValue(mockAuthentication); + vi.mocked(hasPermission).mockReturnValue(true); + + const result = await checkAuth(null, environmentId, mockRequest); + + expect(authenticateRequest).toHaveBeenCalledWith(mockRequest); + expect(hasPermission).toHaveBeenCalledWith(mockAuthentication.environmentPermissions, environmentId, "POST"); + expect(result).toBeUndefined(); + }); + + test("returns unauthorizedResponse when session exists but user lacks environment access", async () => { + const mockSession: Session = { + user: { + id: "user-123", + }, + expires: "2024-12-31T23:59:59.999Z", + }; + + vi.mocked(hasUserEnvironmentAccess).mockResolvedValue(false); + + const result = await checkAuth(mockSession, environmentId, mockRequest); + + expect(hasUserEnvironmentAccess).toHaveBeenCalledWith("user-123", environmentId); + expect(responses.unauthorizedResponse).toHaveBeenCalled(); + expect(result).toBe(mockUnauthorizedResponse); + }); + + test("returns undefined when session exists and user has environment access", async () => { + const mockSession: Session = { + user: { + id: "user-123", + }, + expires: "2024-12-31T23:59:59.999Z", + }; + + vi.mocked(hasUserEnvironmentAccess).mockResolvedValue(true); + + const result = await checkAuth(mockSession, environmentId, mockRequest); + + expect(hasUserEnvironmentAccess).toHaveBeenCalledWith("user-123", environmentId); + expect(result).toBeUndefined(); + }); + + test("does not call authenticateRequest when session exists", async () => { + const mockSession: Session = { + user: { + id: "user-123", + }, + expires: "2024-12-31T23:59:59.999Z", + }; + + vi.mocked(hasUserEnvironmentAccess).mockResolvedValue(true); + + await checkAuth(mockSession, environmentId, mockRequest); + + expect(authenticateRequest).not.toHaveBeenCalled(); + expect(hasUserEnvironmentAccess).toHaveBeenCalledWith("user-123", environmentId); + }); +}); \ No newline at end of file diff --git a/apps/web/app/api/v1/management/storage/lib/utils.ts b/apps/web/app/api/v1/management/storage/lib/utils.ts new file mode 100644 index 0000000000..36ea9ad20b --- /dev/null +++ b/apps/web/app/api/v1/management/storage/lib/utils.ts @@ -0,0 +1,38 @@ +import { authenticateRequest } from "@/app/api/v1/auth"; +import { responses } from "@/app/lib/api/response"; +import { hasUserEnvironmentAccess } from "@/lib/environment/auth"; +import { NextRequest } from "next/server"; +import { Session } from "next-auth"; +import { hasPermission } from "@/modules/organization/settings/api-keys/lib/utils"; + + +export const checkForRequiredFields = (environmentId: string, fileType: string, encodedFileName: string): Response | undefined => { + if (!environmentId) { + return responses.badRequestResponse("environmentId is required"); + } + + if (!fileType) { + return responses.badRequestResponse("contentType is required"); + } + + if (!encodedFileName) { + return responses.badRequestResponse("fileName is required"); + } +}; + +export const checkAuth = async (session: Session | null, environmentId: string, request: NextRequest) => { + if (!session) { + //check whether its using API key + const authentication = await authenticateRequest(request); + if (!authentication) return responses.notAuthenticatedResponse(); + + if (!hasPermission(authentication.environmentPermissions, environmentId, "POST")) { + return responses.unauthorizedResponse(); + } + } else { + const isUserAuthorized = await hasUserEnvironmentAccess(session.user.id, environmentId); + if (!isUserAuthorized) { + return responses.unauthorizedResponse(); + } + } +}; \ No newline at end of file diff --git a/apps/web/app/api/v1/management/storage/local/route.ts b/apps/web/app/api/v1/management/storage/local/route.ts index 49f17be735..2279614b7f 100644 --- a/apps/web/app/api/v1/management/storage/local/route.ts +++ b/apps/web/app/api/v1/management/storage/local/route.ts @@ -4,13 +4,13 @@ import { responses } from "@/app/lib/api/response"; import { ENCRYPTION_KEY, UPLOADS_DIR } from "@/lib/constants"; import { validateLocalSignedUrl } from "@/lib/crypto"; -import { hasUserEnvironmentAccess } from "@/lib/environment/auth"; import { validateFile } from "@/lib/fileValidation"; import { putFileToLocalStorage } from "@/lib/storage/service"; import { authOptions } from "@/modules/auth/lib/authOptions"; import { getServerSession } from "next-auth"; import { NextRequest } from "next/server"; import { logger } from "@formbricks/logger"; +import { checkAuth, checkForRequiredFields } from "@/app/api/v1/management/storage/lib/utils"; export const POST = async (req: NextRequest): Promise => { if (!ENCRYPTION_KEY) { @@ -27,41 +27,17 @@ export const POST = async (req: NextRequest): Promise => { const signedTimestamp = jsonInput.timestamp as string; const environmentId = jsonInput.environmentId as string; - if (!environmentId) { - return responses.badRequestResponse("environmentId is required"); - } + const requiredFieldResponse = checkForRequiredFields(environmentId, fileType, encodedFileName); + if (requiredFieldResponse) return requiredFieldResponse; - if (!fileType) { - return responses.badRequestResponse("contentType is required"); - } - - if (!encodedFileName) { - return responses.badRequestResponse("fileName is required"); - } - - if (!signedSignature) { - return responses.unauthorizedResponse(); - } - - if (!signedUuid) { - return responses.unauthorizedResponse(); - } - - if (!signedTimestamp) { + if (!signedSignature || !signedUuid || !signedTimestamp) { return responses.unauthorizedResponse(); } const session = await getServerSession(authOptions); - if (!session || !session.user) { - return responses.notAuthenticatedResponse(); - } - - const isUserAuthorized = await hasUserEnvironmentAccess(session.user.id, environmentId); - - if (!isUserAuthorized) { - return responses.unauthorizedResponse(); - } + const authResponse = await checkAuth(session, environmentId, req); + if (authResponse) return authResponse; const fileName = decodeURIComponent(encodedFileName); diff --git a/apps/web/app/api/v1/management/storage/route.ts b/apps/web/app/api/v1/management/storage/route.ts index 3f1ffe9774..b8321eff9d 100644 --- a/apps/web/app/api/v1/management/storage/route.ts +++ b/apps/web/app/api/v1/management/storage/route.ts @@ -1,11 +1,12 @@ import { responses } from "@/app/lib/api/response"; -import { hasUserEnvironmentAccess } from "@/lib/environment/auth"; import { validateFile } from "@/lib/fileValidation"; import { authOptions } from "@/modules/auth/lib/authOptions"; import { getServerSession } from "next-auth"; import { NextRequest } from "next/server"; import { logger } from "@formbricks/logger"; import { getSignedUrlForPublicFile } from "./lib/getSignedUrl"; +import { checkAuth, checkForRequiredFields } from "@/app/api/v1/management/storage/lib/utils"; + // api endpoint for uploading public files // uploaded files will be public, anyone can access the file @@ -13,29 +14,26 @@ import { getSignedUrlForPublicFile } from "./lib/getSignedUrl"; // use this to upload files for a specific resource, e.g. a user profile picture or a survey // this api endpoint will return a signed url for uploading the file to s3 and another url for uploading file to the local storage -export const POST = async (req: NextRequest): Promise => { + +export const POST = async (request: NextRequest): Promise => { let storageInput; try { - storageInput = await req.json(); + storageInput = await request.json(); } catch (error) { - logger.error({ error, url: req.url }, "Error parsing JSON input"); + logger.error({ error, url: request.url }, "Error parsing JSON input"); return responses.badRequestResponse("Malformed JSON input, please check your request body"); } const { fileName, fileType, environmentId, allowedFileExtensions } = storageInput; - if (!fileName) { - return responses.badRequestResponse("fileName is required"); - } + const requiredFieldResponse = checkForRequiredFields(environmentId, fileType, fileName); + if (requiredFieldResponse) return requiredFieldResponse; + const session = await getServerSession(authOptions); - if (!fileType) { - return responses.badRequestResponse("fileType is required"); - } + const authResponse = await checkAuth(session, environmentId, request); + if (authResponse) return authResponse; - if (!environmentId) { - return responses.badRequestResponse("environmentId is required"); - } // Perform server-side file validation first to block dangerous file types const fileValidation = validateFile(fileName, fileType); @@ -53,18 +51,5 @@ export const POST = async (req: NextRequest): Promise => { } } - // auth and upload private file - const session = await getServerSession(authOptions); - - if (!session || !session.user) { - return responses.notAuthenticatedResponse(); - } - - const isUserAuthorized = await hasUserEnvironmentAccess(session.user.id, environmentId); - - if (!isUserAuthorized) { - return responses.unauthorizedResponse(); - } - return await getSignedUrlForPublicFile(fileName, environmentId, fileType); };