From 9d84bc0c8de315bacbde6f1fa4ac75628e5ac5d6 Mon Sep 17 00:00:00 2001 From: Matti Nannt Date: Mon, 11 Aug 2025 10:37:33 +0200 Subject: [PATCH] fix: Uncontrolled data used in path expression in storage service (#6375) Co-authored-by: pandeymangg --- apps/web/lib/storage/service.test.ts | 114 +++++++++++++++++++++++++++ apps/web/lib/storage/service.ts | 28 +++++-- 2 files changed, 137 insertions(+), 5 deletions(-) diff --git a/apps/web/lib/storage/service.test.ts b/apps/web/lib/storage/service.test.ts index bd880aad7b..ed207abe77 100644 --- a/apps/web/lib/storage/service.test.ts +++ b/apps/web/lib/storage/service.test.ts @@ -1,4 +1,7 @@ import { S3Client } from "@aws-sdk/client-s3"; +import { readFile } from "fs/promises"; +import { lookup } from "mime-types"; +import path from "path"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; // Mock AWS SDK @@ -7,6 +10,19 @@ const mockS3Client = { send: mockSend, }; +vi.mock("fs/promises", () => ({ + readFile: vi.fn(), + access: vi.fn(), + mkdir: vi.fn(), + rmdir: vi.fn(), + unlink: vi.fn(), + writeFile: vi.fn(), +})); + +vi.mock("mime-types", () => ({ + lookup: vi.fn(), +})); + vi.mock("@aws-sdk/client-s3", () => ({ S3Client: vi.fn(() => mockS3Client), HeadBucketCommand: vi.fn(), @@ -194,5 +210,103 @@ describe("Storage Service", () => { expect(result.signedUrl).toBe("https://test-bucket.s3.test-region.amazonaws.com"); expect(result.presignedFields).toEqual({ key: "test-key", policy: "test-policy" }); }); + + test("use local storage for private files when S3 is not configured", async () => { + vi.resetModules(); + + vi.doMock("../constants", () => ({ + S3_ACCESS_KEY: "test-access-key", + S3_SECRET_KEY: "test-secret-key", + S3_REGION: "test-region", + S3_BUCKET_NAME: "test-bucket", + S3_ENDPOINT_URL: "http://test-endpoint", + S3_FORCE_PATH_STYLE: true, + isS3Configured: () => false, + IS_FORMBRICKS_CLOUD: false, + MAX_SIZES: { + standard: 5 * 1024 * 1024, + big: 10 * 1024 * 1024, + }, + WEBAPP_URL: "http://test-webapp", + ENCRYPTION_KEY: "test-encryption-key-32-chars-long!!", + UPLOADS_DIR: "/tmp/uploads", + })); + + vi.mock("../getPublicUrl", () => ({ + getPublicDomain: () => "https://public-domain.com", + })); + + const freshModule = await import("./service"); + const freshGetUploadSignedUrl = freshModule.getUploadSignedUrl as typeof getUploadSignedUrl; + + const result = await freshGetUploadSignedUrl("test.jpg", "env123", "image/jpeg", "private"); + + expect(result.fileUrl).toContain("http://test-webapp"); + expect(result.fileUrl).toMatch( + /http:\/\/test-webapp\/storage\/env123\/private\/test--fid--test-uuid\.jpg/ + ); + expect(result.fileUrl).not.toContain("test-bucket"); + expect(result.fileUrl).not.toContain("test-endpoint"); + }); + }); + + describe("getLocalFile", () => { + let getLocalFile: any; + + beforeEach(async () => { + const serviceModule = await import("./service"); + getLocalFile = serviceModule.getLocalFile; + }); + + test("should return file buffer and metadata", async () => { + vi.mocked(readFile).mockResolvedValue(Buffer.from("test")); + vi.mocked(lookup).mockReturnValue("image/jpeg"); + + const result = await getLocalFile("/tmp/uploads/test/test.jpg"); + expect(result.fileBuffer).toBeInstanceOf(Buffer); + expect(result.metaData).toEqual({ contentType: "image/jpeg" }); + }); + + test("should throw error when file does not exist", async () => { + vi.mocked(readFile).mockRejectedValue(new Error("File not found")); + await expect(getLocalFile("/tmp/uploads/test/test.jpg")).rejects.toThrow("File not found"); + }); + + test("should throw error when file path attempts traversal outside uploads dir", async () => { + const traversalOutside = path.join("/tmp/uploads", "../outside.txt"); + await expect(getLocalFile(traversalOutside)).rejects.toThrow( + "Invalid file path: Path must be within uploads folder" + ); + }); + + test("should reject path traversal using '../secret' with security error", async () => { + await expect(getLocalFile("../secret")).rejects.toThrow( + "Invalid file path: Path must be within uploads folder" + ); + }); + + test("should reject Windows-style traversal '..\\\\secret' with security error", async () => { + await expect(getLocalFile("..\\secret")).rejects.toThrow( + "Invalid file path: Path must be within uploads folder" + ); + }); + + test("should reject nested traversal 'subdir/../../etc/passwd' with security error", async () => { + await expect(getLocalFile("subdir/../../etc/passwd")).rejects.toThrow( + "Invalid file path: Path must be within uploads folder" + ); + }); + + test("should throw EISDIR when provided path is a directory inside uploads", async () => { + // Simulate Node throwing EISDIR when attempting to read a directory + const eisdirError: any = new Error("EISDIR: illegal operation on a directory, read"); + eisdirError.code = "EISDIR"; + vi.mocked(readFile).mockRejectedValueOnce(eisdirError); + + await expect(getLocalFile("/tmp/uploads/some-dir")).rejects.toMatchObject({ + code: "EISDIR", + message: expect.stringContaining("EISDIR"), + }); + }); }); }); diff --git a/apps/web/lib/storage/service.ts b/apps/web/lib/storage/service.ts index a9097b0ad4..784f76ba6f 100644 --- a/apps/web/lib/storage/service.ts +++ b/apps/web/lib/storage/service.ts @@ -72,12 +72,28 @@ export const testS3BucketAccess = async () => { } }; +// Helper function to validate file paths are within the uploads directory +const validateAndResolvePath = (filePath: string): string => { + // Resolve and normalize the path to prevent directory traversal attacks + const resolvedPath = path.resolve(filePath); + const uploadsPath = path.resolve(UPLOADS_DIR); + + // Ensure the resolved path is within the uploads directory + if (!resolvedPath.startsWith(uploadsPath)) { + throw new Error("Invalid file path: Path must be within uploads folder"); + } + + return resolvedPath; +}; + const ensureDirectoryExists = async (dirPath: string) => { + const safePath = validateAndResolvePath(dirPath); + try { - await access(dirPath); + await access(safePath); } catch (error: any) { if (error.code === "ENOENT") { - await mkdir(dirPath, { recursive: true }); + await mkdir(safePath, { recursive: true }); } else { throw error; } @@ -126,7 +142,7 @@ export const getS3File = async (fileKey: string): Promise => { export const getLocalFile = async (filePath: string): Promise => { try { - const safeFilePath = path.resolve(process.cwd(), filePath); + const safeFilePath = validateAndResolvePath(filePath); const file = await readFile(safeFilePath); let contentType = ""; @@ -263,6 +279,7 @@ export const putFileToLocalStorage = async ( await ensureDirectoryExists(`${rootDir}/${environmentId}/${accessType}`); const uploadPath = `${rootDir}/${environmentId}/${accessType}/${fileName}`; + const safeUploadPath = validateAndResolvePath(uploadPath); const buffer = Buffer.from(fileBuffer as unknown as WithImplicitCoercion); const bufferBytes = buffer.byteLength; @@ -280,7 +297,7 @@ export const putFileToLocalStorage = async ( throw err; } - await writeFile(uploadPath, buffer as unknown as any); + await writeFile(safeUploadPath, buffer as unknown as any); } catch (err) { throw err; } @@ -346,7 +363,8 @@ export const deleteFile = async ( export const deleteLocalFile = async (filePath: string) => { try { - await unlink(filePath); + const safeFilePath = validateAndResolvePath(filePath); + await unlink(safeFilePath); } catch (err: any) { throw err; }