From 49fb34fee4ed48435f97d3840737c636653ff044 Mon Sep 17 00:00:00 2001 From: Johan Lindengard Date: Mon, 18 May 2026 18:32:47 +0200 Subject: [PATCH] fix: prevent upload path traversal --- src/app/api/upload/route.test.ts | 143 +++++++++++++++++++++++++++---- src/app/api/upload/route.ts | 26 +++++- src/lib/supabase-storage.test.ts | 35 +++++++- src/lib/supabase-storage.ts | 26 +++++- 4 files changed, 208 insertions(+), 22 deletions(-) diff --git a/src/app/api/upload/route.test.ts b/src/app/api/upload/route.test.ts index c2c6386f..36276c02 100644 --- a/src/app/api/upload/route.test.ts +++ b/src/app/api/upload/route.test.ts @@ -36,7 +36,7 @@ const mockProviderSession = { id: "provider-user-1", email: "magnus@test.se", userType: "provider", - providerId: "provider-1", + providerId: "a0000000-0000-4000-a000-000000000002", }, } as never @@ -77,7 +77,7 @@ describe("POST /api/upload", () => { it("should upload a horse photo", async () => { vi.mocked(auth).mockResolvedValue(mockSession) vi.mocked(prisma.horse.findFirst).mockResolvedValue({ - id: "horse-1", + id: "a0000000-0000-4000-a000-000000000001", ownerId: "customer-1", } as never) vi.mocked(prisma.horse.update).mockResolvedValue({} as never) @@ -88,7 +88,7 @@ describe("POST /api/upload", () => { } as never) const request = createMockUploadRequest( - { bucket: "horses", entityId: "horse-1" }, + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }, { name: "photo.jpg", type: "image/jpeg", size: 1024 } ) @@ -102,7 +102,7 @@ describe("POST /api/upload", () => { it("should upload a provider avatar", async () => { vi.mocked(auth).mockResolvedValue(mockProviderSession) vi.mocked(prisma.provider.findUnique).mockResolvedValue({ - id: "provider-1", + id: "a0000000-0000-4000-a000-000000000002", userId: "provider-user-1", } as never) vi.mocked(prisma.provider.update).mockResolvedValue({} as never) @@ -113,7 +113,7 @@ describe("POST /api/upload", () => { } as never) const request = createMockUploadRequest( - { bucket: "avatars", entityId: "provider-1" }, + { bucket: "avatars", entityId: "a0000000-0000-4000-a000-000000000002" }, { name: "avatar.jpg", type: "image/jpeg", size: 2048 } ) @@ -125,7 +125,7 @@ describe("POST /api/upload", () => { it("should return 400 when no file is uploaded", async () => { vi.mocked(auth).mockResolvedValue(mockSession) - const request = createMockUploadRequest({ bucket: "horses", entityId: "horse-1" }) + const request = createMockUploadRequest({ bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }) const response = await POST(request) const data = await response.json() @@ -137,7 +137,7 @@ describe("POST /api/upload", () => { vi.mocked(auth).mockResolvedValue(mockSession) const request = createMockUploadRequest( - { bucket: "invalid", entityId: "horse-1" }, + { bucket: "invalid", entityId: "a0000000-0000-4000-a000-000000000001" }, { name: "photo.jpg", type: "image/jpeg", size: 1024 } ) const response = await POST(request) @@ -150,7 +150,7 @@ describe("POST /api/upload", () => { vi.mocked(prisma.horse.findFirst).mockResolvedValue(null) const request = createMockUploadRequest( - { bucket: "horses", entityId: "other-horse" }, + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000004" }, { name: "photo.jpg", type: "image/jpeg", size: 1024 } ) const response = await POST(request) @@ -161,8 +161,8 @@ describe("POST /api/upload", () => { it("should upload a verification image", async () => { vi.mocked(auth).mockResolvedValue(mockProviderSession) vi.mocked(prisma.providerVerification.findFirst).mockResolvedValue({ - id: "ver-1", - providerId: "provider-1", + id: "a0000000-0000-4000-a000-000000000003", + providerId: "a0000000-0000-4000-a000-000000000002", status: "pending", } as never) vi.mocked(prisma.upload.count).mockResolvedValue(0) @@ -173,7 +173,7 @@ describe("POST /api/upload", () => { } as never) const request = createMockUploadRequest( - { bucket: "verifications", entityId: "ver-1" }, + { bucket: "verifications", entityId: "a0000000-0000-4000-a000-000000000003" }, { name: "cert.jpg", type: "image/jpeg", size: 2048 } ) @@ -187,7 +187,7 @@ describe("POST /api/upload", () => { vi.mocked(prisma.providerVerification.findFirst).mockResolvedValue(null) const request = createMockUploadRequest( - { bucket: "verifications", entityId: "other-ver" }, + { bucket: "verifications", entityId: "a0000000-0000-4000-a000-000000000005" }, { name: "cert.jpg", type: "image/jpeg", size: 1024 } ) @@ -202,7 +202,7 @@ describe("POST /api/upload", () => { vi.mocked(prisma.providerVerification.findFirst).mockResolvedValue(null) const request = createMockUploadRequest( - { bucket: "verifications", entityId: "ver-approved" }, + { bucket: "verifications", entityId: "a0000000-0000-4000-a000-000000000006" }, { name: "cert.jpg", type: "image/jpeg", size: 1024 } ) @@ -214,14 +214,14 @@ describe("POST /api/upload", () => { it("should reject when max 5 images per verification reached", async () => { vi.mocked(auth).mockResolvedValue(mockProviderSession) vi.mocked(prisma.providerVerification.findFirst).mockResolvedValue({ - id: "ver-1", - providerId: "provider-1", + id: "a0000000-0000-4000-a000-000000000003", + providerId: "a0000000-0000-4000-a000-000000000002", status: "pending", } as never) vi.mocked(prisma.upload.count).mockResolvedValue(5) const request = createMockUploadRequest( - { bucket: "verifications", entityId: "ver-1" }, + { bucket: "verifications", entityId: "a0000000-0000-4000-a000-000000000003" }, { name: "cert6.jpg", type: "image/jpeg", size: 1024 } ) @@ -241,7 +241,7 @@ describe("POST /api/upload", () => { ) const request = createMockUploadRequest( - { bucket: "horses", entityId: "horse-1" }, + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }, { name: "photo.jpg", type: "image/jpeg", size: 1024 } ) const response = await POST(request) @@ -252,10 +252,117 @@ describe("POST /api/upload", () => { it("returns 401 when session is null", async () => { vi.mocked(auth).mockResolvedValue(null as never) const request = createMockUploadRequest( - { bucket: "horses", entityId: "horse-1" }, + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }, { name: "photo.jpg", type: "image/jpeg", size: 1024 } ) const response = await POST(request) expect(response.status).toBe(401) }) + + // C3: path traversal regression tests + describe("C3 path traversal hardening", () => { + it("T1: traversal in file.name does not leak into storage path; ext derived from MIME", async () => { + vi.mocked(auth).mockResolvedValue(mockSession) + vi.mocked(prisma.horse.findFirst).mockResolvedValue({ + id: "a0000000-0000-4000-a000-000000000001", + ownerId: "customer-1", + } as never) + vi.mocked(prisma.horse.update).mockResolvedValue({} as never) + vi.mocked(prisma.upload.create).mockResolvedValue({ + id: "upload-1", + url: "https://storage.example.com/horses/test.jpg", + path: "horses/test.jpg", + } as never) + + const { uploadFile } = await import("@/lib/supabase-storage") + + const request = createMockUploadRequest( + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }, + { name: "evil.png/../../../etc/passwd", type: "image/png", size: 1024 } + ) + + const response = await POST(request) + expect(response.status).toBe(201) + + // The uploaded fileName must never include traversal sequences + const call = vi.mocked(uploadFile).mock.calls.at(-1) + expect(call).toBeDefined() + const fileNameArg = call![2] + expect(fileNameArg).not.toContain("/") + expect(fileNameArg).not.toContain("..") + expect(fileNameArg).not.toContain("\\") + expect(fileNameArg.endsWith(".png")).toBe(true) + }) + + it("T2: null byte in file.name does not propagate to storage path", async () => { + vi.mocked(auth).mockResolvedValue(mockSession) + vi.mocked(prisma.horse.findFirst).mockResolvedValue({ + id: "a0000000-0000-4000-a000-000000000001", + ownerId: "customer-1", + } as never) + vi.mocked(prisma.horse.update).mockResolvedValue({} as never) + vi.mocked(prisma.upload.create).mockResolvedValue({ id: "upload-1" } as never) + + const { uploadFile } = await import("@/lib/supabase-storage") + + const request = createMockUploadRequest( + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }, + { name: "image\x00.png", type: "image/png", size: 1024 } + ) + + const response = await POST(request) + expect(response.status).toBe(201) + + const fileNameArg = vi.mocked(uploadFile).mock.calls.at(-1)![2] + expect(fileNameArg).not.toContain("\x00") + }) + + it("T3: ext is derived from MIME, not from file extension", async () => { + vi.mocked(auth).mockResolvedValue(mockSession) + vi.mocked(prisma.horse.findFirst).mockResolvedValue({ + id: "a0000000-0000-4000-a000-000000000001", + ownerId: "customer-1", + } as never) + vi.mocked(prisma.horse.update).mockResolvedValue({} as never) + vi.mocked(prisma.upload.create).mockResolvedValue({ id: "upload-1" } as never) + + const { uploadFile } = await import("@/lib/supabase-storage") + + const request = createMockUploadRequest( + { bucket: "horses", entityId: "a0000000-0000-4000-a000-000000000001" }, + { name: "weird.shouldNotMatter", type: "image/jpeg", size: 1024 } + ) + + const response = await POST(request) + expect(response.status).toBe(201) + + const fileNameArg = vi.mocked(uploadFile).mock.calls.at(-1)![2] + expect(fileNameArg.endsWith(".jpg")).toBe(true) + expect(fileNameArg).not.toContain("shouldNotMatter") + }) + + it("T4: traversal in entityId is rejected with 400", async () => { + vi.mocked(auth).mockResolvedValue(mockSession) + + const request = createMockUploadRequest( + { bucket: "horses", entityId: "../../something" }, + { name: "photo.jpg", type: "image/jpeg", size: 1024 } + ) + + const response = await POST(request) + expect(response.status).toBe(400) + }) + + it("T5: non-UUID entityId is rejected with 400", async () => { + vi.mocked(auth).mockResolvedValue(mockSession) + + const request = createMockUploadRequest( + { bucket: "horses", entityId: "not-a-uuid" }, + { name: "photo.jpg", type: "image/jpeg", size: 1024 } + ) + + const response = await POST(request) + expect(response.status).toBe(400) + }) + }) }) diff --git a/src/app/api/upload/route.ts b/src/app/api/upload/route.ts index ee5ae075..eec09b81 100644 --- a/src/app/api/upload/route.ts +++ b/src/app/api/upload/route.ts @@ -1,4 +1,5 @@ import { NextRequest, NextResponse } from "next/server" +import { z } from "zod" import { auth } from "@/lib/auth-server" import { prisma } from "@/lib/prisma" import { rateLimiters, getClientIP } from "@/lib/rate-limit" @@ -10,6 +11,17 @@ type UploadBucket = (typeof VALID_BUCKETS)[number] const MAX_IMAGES_PER_VERIFICATION = 5 +const entityIdSchema = z.string().uuid() + +// C3: derive extension from MIME type, never from user-controlled file.name +// to prevent path traversal via crafted filenames. +const MIME_TO_EXT: Record = { + "image/jpeg": "jpg", + "image/png": "png", + "image/webp": "webp", + "application/pdf": "pdf", +} + // POST /api/upload - Upload a file export async function POST(request: NextRequest) { const clientIp = getClientIP(request) @@ -64,6 +76,15 @@ export async function POST(request: NextRequest) { ) } + // C3: reject non-UUID entityIds early — defense-in-depth before + // entityId is interpolated into the storage path. + if (!entityIdSchema.safeParse(entityId).success) { + return NextResponse.json( + { error: "Ogiltigt entityId" }, + { status: 400 } + ) + } + // Validate file type and size const validationError = validateFile(file.type, file.size) if (validationError) { @@ -127,8 +148,9 @@ export async function POST(request: NextRequest) { } // "services" bucket - provider ownership checked via providerId - // Generate unique filename - const ext = file.name.split(".").pop() || "jpg" + // Generate unique filename — extension is derived from validated MIME + // type, NOT from file.name, to prevent path traversal. + const ext = MIME_TO_EXT[file.type] ?? "bin" const fileName = `${entityId}-${Date.now()}.${ext}` // Upload to Supabase Storage (pass File object directly) diff --git a/src/lib/supabase-storage.test.ts b/src/lib/supabase-storage.test.ts index 9c113e8f..e6b5b80a 100644 --- a/src/lib/supabase-storage.test.ts +++ b/src/lib/supabase-storage.test.ts @@ -7,7 +7,7 @@ vi.mock('file-type', () => ({ fileTypeFromBuffer: vi.fn(), })) -import { validateMessageAttachment } from './supabase-storage' +import { validateMessageAttachment, assertSafeStorageFileName } from './supabase-storage' import { fileTypeFromBuffer } from 'file-type' function makeHeicBuffer(): Buffer { @@ -83,3 +83,36 @@ describe('validateMessageAttachment', () => { expect(result?.code).toBe('MAGIC_BYTES_MISMATCH') }) }) + +describe('assertSafeStorageFileName', () => { + it('H1: rejects fileName containing forward slash', () => { + expect(() => assertSafeStorageFileName('foo/bar.jpg')).toThrow('INVALID_FILENAME') + }) + + it('H2: rejects fileName containing backslash', () => { + expect(() => assertSafeStorageFileName('foo\\bar.jpg')).toThrow('INVALID_FILENAME') + }) + + it('H3: rejects fileName containing parent directory traversal', () => { + expect(() => assertSafeStorageFileName('..jpg')).toThrow('INVALID_FILENAME') + expect(() => assertSafeStorageFileName('uuid..jpg')).toThrow('INVALID_FILENAME') + expect(() => assertSafeStorageFileName('uuid-123./../etc/passwd')).toThrow('INVALID_FILENAME') + }) + + it('H4: rejects fileName containing null byte', () => { + expect(() => assertSafeStorageFileName('evil\x00.jpg')).toThrow('INVALID_FILENAME') + }) + + it('H5: rejects empty, leading-dot, and too-long fileName', () => { + expect(() => assertSafeStorageFileName('')).toThrow('INVALID_FILENAME') + expect(() => assertSafeStorageFileName('.hidden.jpg')).toThrow('INVALID_FILENAME') + expect(() => assertSafeStorageFileName('a'.repeat(256) + '.jpg')).toThrow('INVALID_FILENAME') + }) + + it('H6: accepts a safe fileName', () => { + expect(() => assertSafeStorageFileName('uuid-1234.jpg')).not.toThrow() + expect(() => + assertSafeStorageFileName('a0000000-0000-4000-a000-000000000001-1700000000000.png') + ).not.toThrow() + }) +}) diff --git a/src/lib/supabase-storage.ts b/src/lib/supabase-storage.ts index 990998fa..cdfc6bc9 100644 --- a/src/lib/supabase-storage.ts +++ b/src/lib/supabase-storage.ts @@ -29,6 +29,27 @@ interface UploadError { let supabaseClient: SupabaseClient | null = null +/** + * Defense-in-depth guard against path traversal in storage paths. + * Callers should always derive `fileName` from server-controlled data + * (entity IDs, timestamps, MIME-mapped extensions), but this helper + * provides a fail-loud check in case a user-controlled value slips through. + */ +export function assertSafeStorageFileName(fileName: string): void { + if ( + typeof fileName !== "string" || + fileName.length === 0 || + fileName.length > 255 || + fileName.startsWith(".") || + fileName.includes("/") || + fileName.includes("\\") || + fileName.includes("..") || + fileName.includes("\x00") + ) { + throw new Error("INVALID_FILENAME") + } +} + function getSupabase(): SupabaseClient | null { if (supabaseClient) return supabaseClient @@ -77,6 +98,7 @@ export async function uploadFile( fileName: string, mimeType: string ): Promise<{ data?: UploadResult; error?: UploadError }> { + assertSafeStorageFileName(fileName) const supabase = getSupabase() if (!supabase) { @@ -260,7 +282,9 @@ export async function uploadMessageAttachment( mimeType: string ): Promise { const ext = MIME_TO_EXT[mimeType] ?? 'jpg' - const path = `${bookingId}/${messageId}.${ext}` + const leaf = `${messageId}.${ext}` + assertSafeStorageFileName(leaf) + const path = `${bookingId}/${leaf}` const supabase = getSupabase() if (!supabase) {