diff --git a/src/app/api/auth/upload-avatar/route.js b/src/app/api/auth/upload-avatar/route.js index 1cae484..8ee1645 100644 --- a/src/app/api/auth/upload-avatar/route.js +++ b/src/app/api/auth/upload-avatar/route.js @@ -1,5 +1,6 @@ import { NextResponse } from 'next/server'; import { createClient } from '@supabase/supabase-js'; +import { detectImageType } from '@/lib/server/detect-image-type.js'; const supabaseAuth = createClient(process.env.NEXT_PUBLIC_SUPABASE_URL, process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY); @@ -46,39 +47,34 @@ export async function POST(request) { return NextResponse.json({ error: 'No file provided' }, { status: 400 }); } - // Validate file type - const allowedTypes = ['image/jpeg', 'image/png', 'image/webp', 'image/gif']; - if (!allowedTypes.includes(file.type)) { - return NextResponse.json({ - error: 'Invalid file type. Please upload JPEG, PNG, WebP, or GIF images only.' - }, { status: 400 }); - } - // Validate file size (5MB limit) if (file.size > 5 * 1024 * 1024) { - return NextResponse.json({ - error: 'File size too large. Please upload files smaller than 5MB.' + return NextResponse.json({ + error: 'File size too large. Please upload files smaller than 5MB.' }, { status: 400 }); } - // Generate unique filename - const fileExtByType = { - 'image/jpeg': 'jpg', - 'image/png': 'png', - 'image/webp': 'webp', - 'image/gif': 'gif' - }; - const fileExt = fileExtByType[file.type]; - const fileName = `${user.id}/${Date.now()}.${fileExt}`; - - // Convert file to buffer for upload + // Convert file to buffer so the contents -- not the caller's claim about them -- + // decide what this is. const fileBuffer = await file.arrayBuffer(); + // `file.type` is just a header the uploader chose, so an attacker could store + // arbitrary content under an image content-type and have it served back from the + // avatars bucket. Sniff the magic bytes and use only what they say. + const detected = detectImageType(new Uint8Array(fileBuffer)); + if (!detected) { + return NextResponse.json({ + error: 'Invalid file type. Please upload JPEG, PNG, WebP, or GIF images only.' + }, { status: 400 }); + } + + const fileName = `${user.id}/${Date.now()}.${detected.ext}`; + // Upload to Supabase Storage const { error: uploadError } = await supabase.storage .from('avatars') .upload(fileName, fileBuffer, { - contentType: file.type, + contentType: detected.mime, cacheControl: '3600', upsert: false }); diff --git a/src/lib/server/detect-image-type.js b/src/lib/server/detect-image-type.js new file mode 100644 index 0000000..e1ea297 --- /dev/null +++ b/src/lib/server/detect-image-type.js @@ -0,0 +1,35 @@ +/** + * @fileoverview Identify an image by its magic bytes. + * + * An uploader controls both the filename and the Content-Type header, so neither can be + * allowed to decide what gets written into a public bucket and served back with an image + * content-type. Only the bytes decide. + */ + +/** + * @param {Uint8Array} bytes + * @returns {{mime: string, ext: string} | null} null when it is not an allowed image + */ +export function detectImageType(bytes) { + if (!bytes || bytes.length < 12) return null; + + const startsWith = (...sig) => sig.every((b, i) => bytes[i] === b); + const ascii = (offset, text) => + [...text].every((ch, i) => bytes[offset + i] === ch.charCodeAt(0)); + + // JPEG: FF D8 FF + if (startsWith(0xff, 0xd8, 0xff)) return { mime: 'image/jpeg', ext: 'jpg' }; + + // PNG: 89 50 4E 47 0D 0A 1A 0A + if (startsWith(0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a)) { + return { mime: 'image/png', ext: 'png' }; + } + + // GIF: "GIF87a" or "GIF89a" + if (ascii(0, 'GIF8')) return { mime: 'image/gif', ext: 'gif' }; + + // WebP: "RIFF" .... "WEBP" + if (ascii(0, 'RIFF') && ascii(8, 'WEBP')) return { mime: 'image/webp', ext: 'webp' }; + + return null; +} diff --git a/src/lib/server/detect-image-type.test.js b/src/lib/server/detect-image-type.test.js new file mode 100644 index 0000000..e30a6fa --- /dev/null +++ b/src/lib/server/detect-image-type.test.js @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest'; +import { detectImageType } from './detect-image-type.js'; + +/** Build a 16-byte buffer starting with `head`, so length checks always pass. */ +function bytes(head) { + const out = new Uint8Array(16); + head.forEach((b, i) => (out[i] = b)); + return out; +} + +const ascii = (text) => [...text].map((c) => c.charCodeAt(0)); + +describe('detectImageType', () => { + it('identifies the four allowed formats by magic bytes', () => { + expect(detectImageType(bytes([0xff, 0xd8, 0xff]))).toEqual({ mime: 'image/jpeg', ext: 'jpg' }); + expect(detectImageType(bytes([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]))) + .toEqual({ mime: 'image/png', ext: 'png' }); + expect(detectImageType(bytes(ascii('GIF89a')))).toEqual({ mime: 'image/gif', ext: 'gif' }); + + const webp = new Uint8Array(16); + ascii('RIFF').forEach((b, i) => (webp[i] = b)); + ascii('WEBP').forEach((b, i) => (webp[8 + i] = b)); + expect(detectImageType(webp)).toEqual({ mime: 'image/webp', ext: 'webp' }); + }); + + // The point of the change: a caller claiming image/png proves nothing. + it('rejects non-image content regardless of what the caller claims', () => { + expect(detectImageType(bytes(ascii('')))).toBeNull(); + expect(detectImageType(bytes([0x4d, 0x5a]))).toBeNull(); // PE executable + }); + + it('rejects input too short to carry a signature', () => { + expect(detectImageType(new Uint8Array([0xff, 0xd8, 0xff]))).toBeNull(); + expect(detectImageType(new Uint8Array())).toBeNull(); + expect(detectImageType(null)).toBeNull(); + }); +}); diff --git a/supabase/migrations/20260816140000_close_conversation_hijack_and_profile_mass_assignment.sql b/supabase/migrations/20260816140000_close_conversation_hijack_and_profile_mass_assignment.sql new file mode 100644 index 0000000..4f626c9 --- /dev/null +++ b/supabase/migrations/20260816140000_close_conversation_hijack_and_profile_mass_assignment.sql @@ -0,0 +1,104 @@ +-- Wave 3 of the 2026-08 security remediation. +-- +-- Two findings from the consolidated assessment (GHSA-3hqc-9v44-j37g) that were +-- still live in production: +-- +-- V-007 Orphaned conversation hijacking. The UPDATE policy on `conversations` +-- began `(created_by IS NULL) AND (auth.uid() IS NOT NULL)`, so any +-- authenticated account could take over any conversation whose creator +-- column was NULL -- rename it, or claim it by setting created_by. +-- V-009 Mass assignment on `users`. `Users can update own profile` had a USING +-- clause and no WITH CHECK and no column restriction, so a user could +-- rewrite any column of their own row, including the identity-bearing +-- `phone_number` and `unique_identifier`. +-- +-- Also drops four UPDATE policies that compare `auth.uid() = id`. That is the +-- identity domain drift recorded as IA-040: `auth.uid()` is the Supabase Auth +-- UUID while `users.id` is the internal key. Verified against production before +-- dropping -- 0 of 86 rows have `id = auth_user_id`, so these policies have never +-- matched a single row and grant nothing. + +-- -------------------------------------------------------------------------- +-- V-007: a conversation may only be updated by its creator. +-- +-- `conversations_update_policy` compared `created_by::text = auth.uid()::text`, +-- which is the same domain drift as above and never matched either. Both are +-- replaced by one policy that resolves the caller properly, and which carries a +-- WITH CHECK so the new row cannot hand the conversation to someone else. +-- -------------------------------------------------------------------------- +DROP POLICY IF EXISTS "Users can update own conversations" ON public.conversations; +DROP POLICY IF EXISTS conversations_update_policy ON public.conversations; +DROP POLICY IF EXISTS conversations_update_creator ON public.conversations; + +CREATE POLICY conversations_update_creator ON public.conversations + FOR UPDATE TO authenticated + USING (created_by = public.current_app_user_id()) + WITH CHECK (created_by = public.current_app_user_id()); + +-- -------------------------------------------------------------------------- +-- V-009: pin the identity-bearing columns of `users`. +-- +-- A column-level REVOKE was considered and rejected: it would have to enumerate +-- every writable column correctly, and any column added later defaults back to +-- writable. A trigger states the invariant directly and fails closed for columns +-- nobody thought about. +-- +-- service_role is exempt because the legitimate writers of these columns are all +-- server-side and run as service_role: /api/auth/salt (salt), +-- /api/auth/verify-sms and the CoinPay callback (phone_number), +-- /api/auth/register-anon (unique_identifier). +-- +-- The authenticated role legitimately writes only bio, website, updated_at and +-- sms_notifications_enabled, none of which are pinned here. +-- -------------------------------------------------------------------------- +CREATE OR REPLACE FUNCTION public.enforce_users_immutable_columns() +RETURNS trigger +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path TO '' +AS $function$ +BEGIN + IF current_setting('role', true) = 'service_role' + OR auth.uid() IS NULL THEN + RETURN NEW; + END IF; + + IF NEW.id IS DISTINCT FROM OLD.id + OR NEW.auth_user_id IS DISTINCT FROM OLD.auth_user_id + OR NEW.phone_number IS DISTINCT FROM OLD.phone_number + OR NEW.unique_identifier IS DISTINCT FROM OLD.unique_identifier + OR NEW.created_at IS DISTINCT FROM OLD.created_at + OR NEW.salt IS DISTINCT FROM OLD.salt + THEN + RAISE EXCEPTION + 'Identity columns of users are not client-writable (id, auth_user_id, phone_number, unique_identifier, created_at, salt)'; + END IF; + + RETURN NEW; +END; +$function$; + +DROP TRIGGER IF EXISTS users_immutable_columns ON public.users; +CREATE TRIGGER users_immutable_columns + BEFORE UPDATE ON public.users + FOR EACH ROW + EXECUTE FUNCTION public.enforce_users_immutable_columns(); + +-- Give the surviving profile policy an explicit WITH CHECK so a row can never be +-- updated out from under its owner, independent of the trigger above. +DROP POLICY IF EXISTS "Users can update own profile" ON public.users; +CREATE POLICY "Users can update own profile" ON public.users + FOR UPDATE TO authenticated + USING (auth_user_id = auth.uid()) + WITH CHECK (auth_user_id = auth.uid()); + +-- -------------------------------------------------------------------------- +-- IA-040: drop the policies that can never match (auth.uid() = users.id). +-- -------------------------------------------------------------------------- +DROP POLICY IF EXISTS "Users can update their own avatar_url" ON public.users; +DROP POLICY IF EXISTS "Users can update their own disappearing messages settings" ON public.users; +DROP POLICY IF EXISTS "Users can update their own profile fields" ON public.users; +DROP POLICY IF EXISTS "Users can read their own disappearing messages settings" ON public.users; + +COMMENT ON FUNCTION public.enforce_users_immutable_columns() IS + 'Blocks client-side rewrites of identity columns on users (V-009). service_role is exempt.';