From 825a36fcb7d03703ddf756941bd56604befc3260 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 16 Aug 2026 02:52:28 +0000 Subject: [PATCH] fix(security): close conversation hijacking, profile mass assignment, avatar MIME trust Three findings from the consolidated assessment (GHSA-3hqc-9v44-j37g) that were still live, verified against production before writing the fix. 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 with a NULL creator. Replaced with a creator-only policy that also carries a WITH CHECK, so the row cannot be handed to someone else on the way out. V-009 -- mass assignment on `users`. `Users can update own profile` had no WITH CHECK and no column restriction, letting a user rewrite any column of their own row including `phone_number` and `unique_identifier`. Enforced with a BEFORE UPDATE trigger rather than column-level REVOKE: a REVOKE has to enumerate every writable column correctly and any column added later defaults back to writable, whereas the trigger states the invariant once and fails closed. service_role is exempt, since every legitimate writer of those columns is server-side (salt, verify-sms, CoinPay callback, register-anon). NEW-08 -- avatar upload trusted the client's Content-Type, so arbitrary content could be stored in the public avatars bucket and served back as an image. Now sniffed from magic bytes, and the detected type drives both the extension and the stored content-type. Also drops four UPDATE policies and one SELECT policy comparing `auth.uid() = id` (IA-040 identity domain drift: auth.uid() is the Auth UUID, users.id is the internal key). Confirmed against production that 0 of 86 rows have id = auth_user_id, so these have never matched a row and grant nothing. Not fixed here and still open from the umbrella: V-011/V-012/V-013/V-022 (invitation quota and recipient validation), V-014, V-023, V-025, NEW-05 (webhook SSRF), and the remaining lows. 500 tests pass; build clean. Co-Authored-By: Claude Opus 5 --- src/app/api/auth/upload-avatar/route.js | 40 +++---- src/lib/server/detect-image-type.js | 35 ++++++ src/lib/server/detect-image-type.test.js | 38 +++++++ ...ion_hijack_and_profile_mass_assignment.sql | 104 ++++++++++++++++++ 4 files changed, 195 insertions(+), 22 deletions(-) create mode 100644 src/lib/server/detect-image-type.js create mode 100644 src/lib/server/detect-image-type.test.js create mode 100644 supabase/migrations/20260816140000_close_conversation_hijack_and_profile_mass_assignment.sql 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.';