From e99c87c13172bf99c7ef3fd34bbecd87ad5d451f Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sat, 15 Aug 2026 20:43:58 +0000 Subject: [PATCH 1/2] fix(security): scope cleanup endpoints and harden backup PIN storage Closes the two GitHub security advisories still open against this repo. The other four were remediated in #244-#249. GHSA-2wqx-qppx-4rgf (High) -- cleanup endpoints: /api/cleanup/legacy-messages and /api/cleanup/empty-conversations authenticated with getSession(), which reads the cookie without re-validating the JWT against the auth server, and then operated platform-wide: legacy-messages selected and deleted up to 1000 matching messages with no user filter at all, and empty-conversations enumerated every conversation in the table. Both now use getUser(), resolve the internal user id, and scope every query to the caller -- matching the pattern already used by cleanup/legacy-keys. GHSA-jpfm-vrpc-p6rr (Medium) -- backup PIN: PINs were hashed with unsalted SHA-256, while users_select_authenticated lets any logged-in account read every row of `users`, so one throwaway signup could dump every backup_pin_hash and reverse the 6-12 digit keyspace with a precomputed table. Narrowing the RLS policy alone does not fix this -- the app legitimately reads other users' rows and several callers do select('*'), which a column-level REVOKE would break. So the credential column moves out of `users` into user_backup_pins, a table anon and authenticated hold no privileges on. New PINs are derived with scrypt (N=16384, r=8, p=1) over a per-user random salt. The old digests are discarded rather than relocated: nothing verified them server-side, they only ever backed the hasPin boolean, and being reversible is the entire problem. Row existence preserves hasPin for the 5 affected users. The column drop is a separate migration (20260815120100) because it must not land until this code is deployed. 20260815120000 is additive and is already applied to prod. --- src/app/api/auth/backup-pin/route.js | 97 +++++++++--- src/app/api/auth/backup-pin/route.test.js | 77 +++++++++- .../api/cleanup/empty-conversations/route.js | 141 ++++++++++-------- .../cleanup/empty-conversations/route.test.js | 129 ++++++++++++++++ src/app/api/cleanup/legacy-messages/route.js | 90 ++++++----- .../api/cleanup/legacy-messages/route.test.js | 112 ++++++++++++++ .../api/users/by-username/[username]/route.js | 3 +- ...move_backup_pins_to_service_role_table.sql | 89 +++++++++++ ...60815120100_drop_users_backup_pin_hash.sql | 17 +++ 9 files changed, 637 insertions(+), 118 deletions(-) create mode 100644 src/app/api/cleanup/empty-conversations/route.test.js create mode 100644 src/app/api/cleanup/legacy-messages/route.test.js create mode 100644 supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql create mode 100644 supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql diff --git a/src/app/api/auth/backup-pin/route.js b/src/app/api/auth/backup-pin/route.js index 4f2e9019..3df0af24 100644 --- a/src/app/api/auth/backup-pin/route.js +++ b/src/app/api/auth/backup-pin/route.js @@ -2,9 +2,16 @@ // Handles setting and checking the user's backup PIN hash import { NextResponse } from 'next/server'; +import { randomBytes, scrypt as scryptCallback } from 'node:crypto'; +import { promisify } from 'node:util'; import { createClient } from '@supabase/supabase-js'; import { createServiceRoleClient } from '@/lib/supabase/service-role.js'; +// node:crypto is required for scrypt, so pin this route to the Node runtime. +export const runtime = 'nodejs'; + +const scrypt = promisify(scryptCallback); + let supabaseServiceRole = null; function getServiceRoleClient() { if (!supabaseServiceRole) { @@ -94,17 +101,53 @@ async function authenticateUser(request) { } } +// scrypt work factors. N=16384/r=8/p=1 is the Node default and costs ~16MB and +// tens of milliseconds per derivation -- enough to make an offline sweep of the +// 6-12 digit PIN keyspace impractical per user, and the per-user salt means +// there is no shared work across users. +const SCRYPT_N = 16384; +const SCRYPT_R = 8; +const SCRYPT_P = 1; +const SCRYPT_KEYLEN = 64; +const SCRYPT_SALT_BYTES = 16; +export const PIN_ALGORITHM = `scrypt-n${SCRYPT_N}-r${SCRYPT_R}-p${SCRYPT_P}`; + /** - * Hash a PIN using SHA-256 + * Derive a PIN hash using scrypt and a per-user random salt. + * + * Replaces the previous unsalted `crypto.subtle.digest('SHA-256', pin)`, which + * a rainbow table over the numeric PIN keyspace reversed instantly once the + * hash column was readable (GHSA-jpfm-vrpc-p6rr). + * * @param {string} pin - * @returns {Promise} hex-encoded hash + * @param {string} [saltHex] existing salt, hex-encoded; a new one is generated when omitted + * @returns {Promise<{hash: string, salt: string, algorithm: string}>} */ -async function hashPin(pin) { - const encoder = new TextEncoder(); - const data = encoder.encode(pin); - const hashBuffer = await crypto.subtle.digest('SHA-256', data); - const hashArray = Array.from(new Uint8Array(hashBuffer)); - return hashArray.map(b => b.toString(16).padStart(2, '0')).join(''); +async function hashPin(pin, saltHex) { + const salt = saltHex ?? randomBytes(SCRYPT_SALT_BYTES).toString('hex'); + const derived = /** @type {Buffer} */ ( + await scrypt(pin, salt, SCRYPT_KEYLEN, { N: SCRYPT_N, r: SCRYPT_R, p: SCRYPT_P }) + ); + return { hash: derived.toString('hex'), salt, algorithm: PIN_ALGORITHM }; +} + +/** + * Resolve the internal users.id for a Supabase Auth user. + * @param {{id: string}} user + * @returns {Promise<{userId?: string, error?: string}>} + */ +async function resolveInternalUserId(user) { + const { data, error } = await getServiceRoleClient() + .from('users') + .select('id') + .eq('auth_user_id', user.id) + .single(); + + if (error || !data?.id) { + return { error: error?.message ?? 'User record not found' }; + } + + return { userId: data.id }; } /** @@ -118,18 +161,27 @@ export async function GET(request) { return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); } + const { userId, error: lookupError } = await resolveInternalUserId(user); + if (lookupError || !userId) { + console.error('Error checking backup PIN:', lookupError); + return NextResponse.json({ error: 'Failed to check backup PIN' }, { status: 500 }); + } + + // PIN material lives in user_backup_pins, which only the service role can + // reach. A row's existence is what marks a PIN as set -- rows migrated from + // the old unsalted column carry NULL hashes on purpose. const { data, error } = await getServiceRoleClient() - .from('users') - .select('backup_pin_hash') - .eq('auth_user_id', user.id) - .single(); + .from('user_backup_pins') + .select('user_id') + .eq('user_id', userId) + .maybeSingle(); if (error) { console.error('Error checking backup PIN:', error); return NextResponse.json({ error: 'Failed to check backup PIN' }, { status: 500 }); } - return NextResponse.json({ hasPin: !!data?.backup_pin_hash }); + return NextResponse.json({ hasPin: !!data }); } catch (error) { console.error('Error in GET /api/auth/backup-pin:', error); return NextResponse.json({ error: 'Internal server error' }, { status: 500 }); @@ -164,12 +216,23 @@ export async function POST(request) { return NextResponse.json({ error: 'PIN must contain only digits' }, { status: 400 }); } - const pinHash = await hashPin(pin); + const { userId, error: lookupError } = await resolveInternalUserId(user); + if (lookupError || !userId) { + console.error('Error setting backup PIN:', lookupError); + return NextResponse.json({ error: 'Failed to set backup PIN' }, { status: 500 }); + } + + const { hash, salt, algorithm } = await hashPin(pin); const { error } = await getServiceRoleClient() - .from('users') - .update({ backup_pin_hash: pinHash }) - .eq('auth_user_id', user.id); + .from('user_backup_pins') + .upsert({ + user_id: userId, + pin_hash: hash, + pin_salt: salt, + algorithm, + updated_at: new Date().toISOString() + }, { onConflict: 'user_id' }); if (error) { console.error('Error setting backup PIN:', error); diff --git a/src/app/api/auth/backup-pin/route.test.js b/src/app/api/auth/backup-pin/route.test.js index 6407f06b..626e701e 100644 --- a/src/app/api/auth/backup-pin/route.test.js +++ b/src/app/api/auth/backup-pin/route.test.js @@ -1,8 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createHash } from 'node:crypto'; + const mocks = vi.hoisted(() => ({ authGetUser: vi.fn(), - serviceFrom: vi.fn() + serviceFrom: vi.fn(), + pinUpsert: vi.fn() })); vi.mock('@supabase/supabase-js', () => ({ @@ -33,7 +36,7 @@ function createUsersQuery() { eq: vi.fn(() => query), single: vi.fn(() => Promise.resolve({ - data: { backup_pin_hash: 'pin-hash' }, + data: { id: 'internal-user-id' }, error: null }) ) @@ -41,6 +44,21 @@ function createUsersQuery() { return query; } +function createBackupPinsQuery() { + const query = { + select: vi.fn(() => query), + eq: vi.fn(() => query), + maybeSingle: vi.fn(() => + Promise.resolve({ + data: { user_id: 'internal-user-id' }, + error: null + }) + ), + upsert: mocks.pinUpsert + }; + return query; +} + describe('backup PIN cookie authentication', () => { beforeEach(() => { vi.resetModules(); @@ -52,8 +70,10 @@ describe('backup PIN cookie authentication', () => { data: { user: { id: 'auth-user-id' } }, error: null }); + mocks.pinUpsert.mockResolvedValue({ error: null }); mocks.serviceFrom.mockImplementation((table) => { if (table === 'users') return createUsersQuery(); + if (table === 'user_backup_pins') return createBackupPinsQuery(); throw new Error(`Unexpected table: ${table}`); }); }); @@ -119,6 +139,59 @@ describe('backup PIN cookie authentication', () => { expect(mocks.authGetUser).toHaveBeenCalledWith('cookie-token'); }); + it('reads the has-pin flag from the service-role-only table, not from users', async () => { + const { GET } = await import('./route.js'); + const response = await GET( + new Request('https://qrypt.chat/api/auth/backup-pin', { + headers: { authorization: 'Bearer access-token' } + }) + ); + const body = await response.json(); + + expect(response.status).toBe(200); + expect(body).toEqual({ hasPin: true }); + expect(mocks.serviceFrom).toHaveBeenCalledWith('user_backup_pins'); + }); + + it('stores a salted scrypt hash rather than an unsalted SHA-256 digest', async () => { + const { POST } = await import('./route.js'); + const pin = '123456'; + const response = await POST({ + headers: new Headers({ authorization: 'Bearer access-token' }), + json: vi.fn().mockResolvedValue({ pin }) + }); + + expect(response.status).toBe(200); + expect(mocks.pinUpsert).toHaveBeenCalledTimes(1); + + const [record] = mocks.pinUpsert.mock.calls[0]; + expect(record.user_id).toBe('internal-user-id'); + expect(record.algorithm).toMatch(/^scrypt-/); + + // A salt is present and actually mixed in: the stored hash must not be the + // bare SHA-256 of the PIN, which is what GHSA-jpfm-vrpc-p6rr was about. + expect(record.pin_salt).toMatch(/^[0-9a-f]{32}$/); + const unsaltedSha256 = createHash('sha256').update(pin).digest('hex'); + expect(record.pin_hash).not.toBe(unsaltedSha256); + expect(record.pin_hash).toMatch(/^[0-9a-f]{128}$/); + }); + + it('derives a different hash per user for the same PIN', async () => { + const { POST } = await import('./route.js'); + const request = () => ({ + headers: new Headers({ authorization: 'Bearer access-token' }), + json: vi.fn().mockResolvedValue({ pin: '123456' }) + }); + + await POST(request()); + await POST(request()); + + const [first] = mocks.pinUpsert.mock.calls[0]; + const [second] = mocks.pinUpsert.mock.calls[1]; + expect(first.pin_salt).not.toBe(second.pin_salt); + expect(first.pin_hash).not.toBe(second.pin_hash); + }); + it('returns 400 for malformed JSON instead of a generic 500', async () => { const { POST } = await import('./route.js'); const response = await POST({ diff --git a/src/app/api/cleanup/empty-conversations/route.js b/src/app/api/cleanup/empty-conversations/route.js index 962f5e6e..3681400c 100644 --- a/src/app/api/cleanup/empty-conversations/route.js +++ b/src/app/api/cleanup/empty-conversations/route.js @@ -1,83 +1,98 @@ import { NextResponse } from 'next/server'; -import { createSupabaseClient } from '@/lib/supabase.js'; - +import { createSupabaseServerClient } from '@/lib/supabase.js'; +import { createServiceRoleClient } from '@/lib/supabase/service-role.js'; export async function DELETE(request) { try { - // Get supabase client (will use cookies automatically) - const supabase = createSupabaseClient(); - - // Verify user is authenticated with better error handling - const { data: { session } } = await supabase.auth.getSession(); - - if (!session || !session.user) { - console.error('Authentication failed - no valid session found'); + // Use createSupabaseServerClient (reads cookies server-side) and validate + // with getUser() rather than getSession() to ensure the JWT is re-verified + // against the Supabase Auth server and cannot be spoofed via cookie tampering. + const supabase = await createSupabaseServerClient(); + const { data: { user }, error: authError } = await supabase.auth.getUser(); + + if (!user || authError) { + console.error('Authentication failed - no valid user found'); return NextResponse.json({ error: 'Unauthorized - No valid session', details: 'Please login again' }, { status: 401 }); } - - const userId = session.user.id; - console.log(`Authenticated user ${userId} requesting empty conversation cleanup`); - - // First, check if the stored procedure exists - // If not, we'll use a more direct approach - const { data: functionExists, error: functionCheckError } = await supabase - .from('pg_catalog.pg_proc') - .select('proname') - .eq('proname', 'get_empty_conversations') - .maybeSingle(); - - let emptyConversationIds = []; - - if (functionCheckError || !functionExists) { - // Fallback: Get conversations with no messages directly - const { data: conversations, error: findError } = await supabase - .from('conversations') - .select('id, (SELECT count(*) FROM messages WHERE messages.conversation_id = conversations.id) as message_count'); - - if (findError) { - console.error('Error finding empty conversations:', findError); - return NextResponse.json({ error: findError.message }, { status: 500 }); - } - - // Filter for empty conversations - emptyConversationIds = conversations - .filter((/** @type {any} */ conv) => conv.message_count === 0) - .map((/** @type {any} */ conv) => conv.id); - } else { - // Use the stored procedure if it exists - const { data: emptyConversations, error: findError } = await supabase.rpc( - 'get_empty_conversations' - ); - - if (findError) { - console.error('Error finding empty conversations:', findError); - return NextResponse.json({ error: findError.message }, { status: 500 }); - } - - emptyConversationIds = emptyConversations?.map((/** @type {any} */ conv) => conv.id) || []; + + // Resolve the internal user ID from the Supabase Auth UUID. + const serviceRoleClient = createServiceRoleClient(); + const { data: internalUser, error: internalUserError } = await serviceRoleClient + .from('users') + .select('id') + .eq('auth_user_id', user.id) + .single(); + + if (internalUserError || !internalUser) { + console.error('User record not found for auth_user_id:', user.id); + return NextResponse.json({ error: 'User not found' }, { status: 404 }); + } + + const internalUserId = internalUser.id; + console.log(`Authenticated user ${internalUserId} requesting empty conversation cleanup (own conversations only)`); + + // SECURITY FIX: Only ever consider conversations the caller participates in. + // Previously this route enumerated every conversation in the table (via a + // bogus pg_catalog probe and an unscoped fallback) and deleted all empty + // ones platform-wide. + const { data: participantRows, error: participantError } = await serviceRoleClient + .from('conversation_participants') + .select('conversation_id') + .eq('user_id', internalUserId); + + if (participantError) { + console.error('Error finding user conversations:', participantError); + return NextResponse.json({ error: participantError.message }, { status: 500 }); + } + + const conversationIds = [...new Set( + (participantRows || []).map((/** @type {any} */ row) => row.conversation_id) + )]; + + if (conversationIds.length === 0) { + return NextResponse.json({ + message: 'No empty conversations found', + deletedCount: 0 + }); + } + + // Find which of those conversations still hold at least one message. + const { data: messageRows, error: messageError } = await serviceRoleClient + .from('messages') + .select('conversation_id') + .in('conversation_id', conversationIds); + + if (messageError) { + console.error('Error finding empty conversations:', messageError); + return NextResponse.json({ error: messageError.message }, { status: 500 }); } - - console.log(`Found ${emptyConversationIds.length} empty conversations`); - + + const nonEmpty = new Set( + (messageRows || []).map((/** @type {any} */ row) => row.conversation_id) + ); + const emptyConversationIds = conversationIds.filter((id) => !nonEmpty.has(id)); + + console.log(`Found ${emptyConversationIds.length} empty conversations for user ${internalUserId}`); + // No empty conversations found if (emptyConversationIds.length === 0) { - return NextResponse.json({ - message: 'No empty conversations found', - deletedCount: 0 + return NextResponse.json({ + message: 'No empty conversations found', + deletedCount: 0 }); } - - // Delete the empty conversations - const { data: deleteData, error: deleteError } = await supabase + + // Delete only the empty conversations the caller belongs to. + const { error: deleteError } = await serviceRoleClient .from('conversations') .delete() .in('id', emptyConversationIds); - + if (deleteError) { console.error('Error deleting empty conversations:', deleteError); return NextResponse.json({ error: deleteError.message }, { status: 500 }); } - + return NextResponse.json({ message: 'Successfully deleted empty conversations', deletedCount: emptyConversationIds.length @@ -89,4 +104,4 @@ export async function DELETE(request) { { status: 500 } ); } -} \ No newline at end of file +} diff --git a/src/app/api/cleanup/empty-conversations/route.test.js b/src/app/api/cleanup/empty-conversations/route.test.js new file mode 100644 index 00000000..79440467 --- /dev/null +++ b/src/app/api/cleanup/empty-conversations/route.test.js @@ -0,0 +1,129 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(), + getSession: vi.fn(), + serviceFrom: vi.fn(), + conversationDelete: vi.fn() +})); + +vi.mock('@/lib/supabase.js', () => ({ + createSupabaseServerClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser, getSession: mocks.getSession } + })) +})); + +vi.mock('@/lib/supabase/service-role.js', () => ({ + createServiceRoleClient: vi.fn(() => ({ from: mocks.serviceFrom })) +})); + +function createUsersQuery() { + const query = { + select: vi.fn(() => query), + eq: vi.fn(() => query), + single: vi.fn(() => Promise.resolve({ data: { id: 'internal-user-id' }, error: null })) + }; + return query; +} + +// The caller participates in conv-empty and conv-busy; conv-other belongs to +// someone else entirely and must never be considered. +function createParticipantsQuery(state) { + const query = { + select: vi.fn(() => query), + eq: vi.fn((column, value) => { + state.participantFilters.push([column, value]); + return Promise.resolve({ + data: [{ conversation_id: 'conv-empty' }, { conversation_id: 'conv-busy' }], + error: null + }); + }) + }; + return query; +} + +function createMessagesQuery(state) { + const query = { + select: vi.fn(() => query), + in: vi.fn((column, value) => { + state.messageScope.push([column, value]); + return Promise.resolve({ data: [{ conversation_id: 'conv-busy' }], error: null }); + }) + }; + return query; +} + +function createConversationsQuery(state) { + return { + delete: vi.fn(() => ({ + in: vi.fn((column, value) => { + state.deleted.push([column, value]); + return mocks.conversationDelete(); + }) + })) + }; +} + +describe('DELETE /api/cleanup/empty-conversations', () => { + let state; + + beforeEach(() => { + vi.resetModules(); + vi.clearAllMocks(); + state = { participantFilters: [], messageScope: [], deleted: [] }; + + mocks.getUser.mockResolvedValue({ data: { user: { id: 'auth-user-id' } }, error: null }); + mocks.getSession.mockResolvedValue({ data: { session: { user: { id: 'auth-user-id' } } } }); + mocks.conversationDelete.mockResolvedValue({ error: null }); + mocks.serviceFrom.mockImplementation((table) => { + if (table === 'users') return createUsersQuery(); + if (table === 'conversation_participants') return createParticipantsQuery(state); + if (table === 'messages') return createMessagesQuery(state); + if (table === 'conversations') return createConversationsQuery(state); + throw new Error(`Unexpected table: ${table}`); + }); + }); + + it('re-validates the JWT with getUser() and never trusts getSession()', async () => { + const { DELETE } = await import('./route.js'); + await DELETE( + new Request('https://qrypt.chat/api/cleanup/empty-conversations', { method: 'DELETE' }) + ); + + expect(mocks.getUser).toHaveBeenCalled(); + expect(mocks.getSession).not.toHaveBeenCalled(); + }); + + it('rejects an unauthenticated caller before touching any data', async () => { + mocks.getUser.mockResolvedValue({ data: { user: null }, error: { message: 'no session' } }); + + const { DELETE } = await import('./route.js'); + const response = await DELETE( + new Request('https://qrypt.chat/api/cleanup/empty-conversations', { method: 'DELETE' }) + ); + + expect(response.status).toBe(401); + expect(mocks.serviceFrom).not.toHaveBeenCalled(); + }); + + it('only deletes empty conversations the caller participates in', async () => { + const { DELETE } = await import('./route.js'); + const response = await DELETE( + new Request('https://qrypt.chat/api/cleanup/empty-conversations', { method: 'DELETE' }) + ); + const body = await response.json(); + + expect(response.status).toBe(200); + expect(body.deletedCount).toBe(1); + + // Candidates come from the caller's own participant rows... + expect(state.participantFilters).toContainEqual(['user_id', 'internal-user-id']); + // ...emptiness is only ever evaluated within that set... + expect(state.messageScope).toContainEqual([ + 'conversation_id', + ['conv-empty', 'conv-busy'] + ]); + // ...and only the empty one is deleted. + expect(state.deleted).toEqual([['id', ['conv-empty']]]); + }); +}); diff --git a/src/app/api/cleanup/legacy-messages/route.js b/src/app/api/cleanup/legacy-messages/route.js index a819304b..cf0e2fb5 100644 --- a/src/app/api/cleanup/legacy-messages/route.js +++ b/src/app/api/cleanup/legacy-messages/route.js @@ -1,61 +1,81 @@ import { NextResponse } from 'next/server'; -import { createSupabaseClient } from '@/lib/supabase.js'; +import { createSupabaseServerClient } from '@/lib/supabase.js'; +import { createServiceRoleClient } from '@/lib/supabase/service-role.js'; +const LEGACY_BATCH_LIMIT = 1000; export async function DELETE(request) { try { - // Get supabase client (will use cookies automatically) - const supabase = createSupabaseClient(); - - // Verify user is authenticated with better error handling - const { data: { session } } = await supabase.auth.getSession(); - - if (!session || !session.user) { - console.error('Authentication failed - no valid session found'); + // Use createSupabaseServerClient (reads cookies server-side) and validate + // with getUser() rather than getSession() to ensure the JWT is re-verified + // against the Supabase Auth server and cannot be spoofed via cookie tampering. + const supabase = await createSupabaseServerClient(); + const { data: { user }, error: authError } = await supabase.auth.getUser(); + + if (!user || authError) { + console.error('Authentication failed - no valid user found'); return NextResponse.json({ error: 'Unauthorized - No valid session', details: 'Please login again' }, { status: 401 }); } - - const userId = session.user.id; - console.log(`Authenticated user ${userId} requesting legacy message cleanup`); - - // Find messages with legacy encryption (not ML-KEM-1024) - // This includes any messages with "FALLBACK" in the encryption algorithm - // or any message with "ML-KEM-768" algorithm - const { data: messages, error: findError } = await supabase + + // Resolve the internal user ID from the Supabase Auth UUID. + const serviceRoleClient = createServiceRoleClient(); + const { data: internalUser, error: internalUserError } = await serviceRoleClient + .from('users') + .select('id') + .eq('auth_user_id', user.id) + .single(); + + if (internalUserError || !internalUser) { + console.error('User record not found for auth_user_id:', user.id); + return NextResponse.json({ error: 'User not found' }, { status: 404 }); + } + + const internalUserId = internalUser.id; + console.log(`Authenticated user ${internalUserId} requesting legacy message cleanup (own messages only)`); + + // SECURITY FIX: Scope the query to messages the authenticated user sent. + // Previously the query selected and deleted every matching message in the + // table with no user filter at all, so any caller could wipe up to + // LEGACY_BATCH_LIMIT legacy messages across every conversation on the + // platform. + const { data: messages, error: findError } = await serviceRoleClient .from('messages') - .select('id, encrypted_content') + .select('id') + .eq('sender_id', internalUserId) .or('encrypted_content.ilike.%FALLBACK%,encrypted_content.ilike.%ML-KEM-768%') - .limit(1000); // Process in batches for safety - + .limit(LEGACY_BATCH_LIMIT); // Process in batches for safety + if (findError) { console.error('Error finding legacy messages:', findError); return NextResponse.json({ error: findError.message }, { status: 500 }); } - - console.log(`Found ${messages?.length || 0} legacy encrypted messages`); - + + console.log(`Found ${messages?.length || 0} legacy encrypted messages for user ${internalUserId}`); + // No legacy messages found if (!messages || messages.length === 0) { - return NextResponse.json({ - message: 'No legacy messages found', - deletedCount: 0 + return NextResponse.json({ + message: 'No legacy messages found', + deletedCount: 0 }); } - - // Get message IDs to delete + const messageIds = messages.map((/** @type {any} */ msg) => msg.id); - - // Delete the legacy messages - const { data: deleteData, error: deleteError } = await supabase + + // Delete only the authenticated user's legacy messages. + // The extra .eq('sender_id', internalUserId) is a defence-in-depth guard so + // that an id list is never enough on its own to delete another user's row. + const { error: deleteError } = await serviceRoleClient .from('messages') .delete() - .in('id', messageIds); - + .in('id', messageIds) + .eq('sender_id', internalUserId); // defence-in-depth ownership check + if (deleteError) { console.error('Error deleting legacy messages:', deleteError); return NextResponse.json({ error: deleteError.message }, { status: 500 }); } - + return NextResponse.json({ message: 'Successfully deleted legacy messages', deletedCount: messageIds.length @@ -67,4 +87,4 @@ export async function DELETE(request) { { status: 500 } ); } -} \ No newline at end of file +} diff --git a/src/app/api/cleanup/legacy-messages/route.test.js b/src/app/api/cleanup/legacy-messages/route.test.js new file mode 100644 index 00000000..6bdfa620 --- /dev/null +++ b/src/app/api/cleanup/legacy-messages/route.test.js @@ -0,0 +1,112 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getUser: vi.fn(), + getSession: vi.fn(), + serviceFrom: vi.fn(), + messageDelete: vi.fn() +})); + +vi.mock('@/lib/supabase.js', () => ({ + createSupabaseServerClient: vi.fn(async () => ({ + auth: { getUser: mocks.getUser, getSession: mocks.getSession } + })) +})); + +vi.mock('@/lib/supabase/service-role.js', () => ({ + createServiceRoleClient: vi.fn(() => ({ from: mocks.serviceFrom })) +})); + +function createUsersQuery() { + const query = { + select: vi.fn(() => query), + eq: vi.fn(() => query), + single: vi.fn(() => Promise.resolve({ data: { id: 'internal-user-id' }, error: null })) + }; + return query; +} + +/** Records every filter applied on the way to select() or delete(). */ +function createMessagesQuery(state) { + const select = { + eq: vi.fn((column, value) => { + state.selectFilters.push([column, value]); + return select; + }), + or: vi.fn(() => select), + limit: vi.fn(() => + Promise.resolve({ data: [{ id: 'msg-1' }, { id: 'msg-2' }], error: null }) + ) + }; + + const del = { + in: vi.fn((column, value) => { + state.deleteFilters.push([column, value]); + return del; + }), + eq: vi.fn((column, value) => { + state.deleteFilters.push([column, value]); + return mocks.messageDelete(); + }) + }; + + return { + select: vi.fn(() => select), + delete: vi.fn(() => del) + }; +} + +describe('DELETE /api/cleanup/legacy-messages', () => { + let state; + + beforeEach(() => { + vi.resetModules(); + vi.clearAllMocks(); + state = { selectFilters: [], deleteFilters: [] }; + + mocks.getUser.mockResolvedValue({ data: { user: { id: 'auth-user-id' } }, error: null }); + mocks.getSession.mockResolvedValue({ data: { session: { user: { id: 'auth-user-id' } } } }); + mocks.messageDelete.mockResolvedValue({ error: null }); + mocks.serviceFrom.mockImplementation((table) => { + if (table === 'users') return createUsersQuery(); + if (table === 'messages') return createMessagesQuery(state); + throw new Error(`Unexpected table: ${table}`); + }); + }); + + it('re-validates the JWT with getUser() and never trusts getSession()', async () => { + const { DELETE } = await import('./route.js'); + await DELETE(new Request('https://qrypt.chat/api/cleanup/legacy-messages', { method: 'DELETE' })); + + expect(mocks.getUser).toHaveBeenCalled(); + expect(mocks.getSession).not.toHaveBeenCalled(); + }); + + it('rejects an unauthenticated caller before touching any data', async () => { + mocks.getUser.mockResolvedValue({ data: { user: null }, error: { message: 'no session' } }); + + const { DELETE } = await import('./route.js'); + const response = await DELETE( + new Request('https://qrypt.chat/api/cleanup/legacy-messages', { method: 'DELETE' }) + ); + + expect(response.status).toBe(401); + expect(mocks.serviceFrom).not.toHaveBeenCalled(); + }); + + it('scopes both the search and the delete to the caller as sender', async () => { + const { DELETE } = await import('./route.js'); + const response = await DELETE( + new Request('https://qrypt.chat/api/cleanup/legacy-messages', { method: 'DELETE' }) + ); + const body = await response.json(); + + expect(response.status).toBe(200); + expect(body.deletedCount).toBe(2); + + // The platform-wide variant of this route had no user filter at all. + expect(state.selectFilters).toContainEqual(['sender_id', 'internal-user-id']); + expect(state.deleteFilters).toContainEqual(['sender_id', 'internal-user-id']); + expect(state.deleteFilters).toContainEqual(['id', ['msg-1', 'msg-2']]); + }); +}); diff --git a/src/app/api/users/by-username/[username]/route.js b/src/app/api/users/by-username/[username]/route.js index b94640b5..c19e8e36 100644 --- a/src/app/api/users/by-username/[username]/route.js +++ b/src/app/api/users/by-username/[username]/route.js @@ -13,7 +13,8 @@ export async function GET(request, { params } = {}) { // read cannot rely on the caller's session. RLS on `users` is restricted to // the `authenticated` role, so use the service role and keep the explicit // column list below as the boundary — never widen it to `*`, and never add - // phone_number / backup_pin_hash / salt. + // phone_number / salt. (backup_pin_hash no longer lives on `users`; PIN + // material moved to the service-role-only user_backup_pins table.) const supabase = getServiceRoleClient(); const { data, error } = await supabase diff --git a/supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql b/supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql new file mode 100644 index 00000000..6bb8b528 --- /dev/null +++ b/supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql @@ -0,0 +1,89 @@ +-- Step 4 of the 2026-08 security remediation. +-- Context: docs/audits/2026-08-14-noir0x63-verification.md (§2, NEW-04) +-- Advisory: GHSA-jpfm-vrpc-p6rr +-- +-- Two problems, one chain: +-- +-- 1. `users_select_authenticated` is `USING (true)` for the `authenticated` +-- role, so any single logged-in account can read every row of +-- `public.users` -- including `backup_pin_hash`. +-- 2. Those hashes were plain unsalted SHA-256 of a 6-12 digit PIN, which a +-- precomputed table recovers instantly. +-- +-- Chained, one throwaway signup recovered every user's backup PIN. +-- +-- Narrowing the RLS policy alone does not fix this: the app legitimately reads +-- other users' rows (search, participant lookup) and several callers do +-- `select('*')`, which a column-level REVOKE would break outright. So the +-- credential column is moved out of `users` entirely, into a table that the +-- `anon` and `authenticated` roles have no privileges on at all. Only the +-- service role -- i.e. server-side route handlers -- can reach it. +-- +-- The PIN hash itself is never verified server-side; it only ever backed the +-- `hasPin` boolean returned by GET /api/auth/backup-pin. The PIN's real job is +-- client-side key-backup derivation. So the existing unsalted digests are NOT +-- carried across: row existence preserves `hasPin`, and the weak hashes are +-- discarded rather than relocated. Setting a new PIN writes a salted scrypt +-- hash (see src/app/api/auth/backup-pin/route.js). + +-- -------------------------------------------------------------------------- +-- Service-role-only home for backup PIN material. +-- -------------------------------------------------------------------------- +CREATE TABLE IF NOT EXISTS public.user_backup_pins ( + user_id uuid PRIMARY KEY REFERENCES public.users(id) ON DELETE CASCADE, + pin_hash text, + pin_salt text, + algorithm text NOT NULL DEFAULT 'legacy-sha256-discarded', + created_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now() +); + +COMMENT ON TABLE public.user_backup_pins IS + 'Backup PIN material. Service role only -- anon and authenticated hold no ' + 'privileges and there are deliberately no RLS policies. See GHSA-jpfm-vrpc-p6rr.'; +COMMENT ON COLUMN public.user_backup_pins.algorithm IS + 'scrypt-n16384-r8-p1 for PINs set after 2026-08-15. ' + 'legacy-sha256-discarded marks a pre-migration PIN whose weak unsalted ' + 'digest was destroyed; pin_hash/pin_salt are NULL and the row exists only ' + 'so that hasPin stays true until the user sets a new PIN.'; + +-- RLS on with zero policies: PostgREST requests from anon/authenticated match +-- nothing. The REVOKE below is the actual lock; this is defence in depth. +ALTER TABLE public.user_backup_pins ENABLE ROW LEVEL SECURITY; + +REVOKE ALL ON public.user_backup_pins FROM PUBLIC; +REVOKE ALL ON public.user_backup_pins FROM anon; +REVOKE ALL ON public.user_backup_pins FROM authenticated; +GRANT ALL ON public.user_backup_pins TO service_role; + +-- -------------------------------------------------------------------------- +-- Carry across the fact that a PIN is set, without carrying the weak digest. +-- -------------------------------------------------------------------------- +INSERT INTO public.user_backup_pins (user_id, pin_hash, pin_salt, algorithm) +SELECT id, NULL, NULL, 'legacy-sha256-discarded' +FROM public.users +WHERE backup_pin_hash IS NOT NULL +ON CONFLICT (user_id) DO NOTHING; + +-- -------------------------------------------------------------------------- +-- The `users.backup_pin_hash` column itself is dropped by the NEXT migration, +-- 20260815120100_drop_users_backup_pin_hash.sql. It is deliberately a separate +-- step: this migration is additive and safe to apply to a running deployment, +-- whereas the drop must not land until the application code that stopped +-- reading that column has shipped. +-- -------------------------------------------------------------------------- + +-- -------------------------------------------------------------------------- +-- Note on the columns that remain readable across accounts. +-- +-- `salt` stays in `public.users`. It is a per-user KDF salt, which is not a +-- secret on its own -- its exposure only mattered because the PIN hash sat +-- beside it. With `backup_pin_hash` gone there is nothing left to precompute +-- against, and /api/auth/salt already restricts callers to their own row via +-- the service role. +-- +-- `phone_number` also stays readable to `authenticated`, because +-- /api/users/search deliberately matches on it (results are masked to +-- ***-***-1234 before they leave the server). That is tracked separately as a +-- Medium-severity enumeration issue, not as part of this advisory. +-- -------------------------------------------------------------------------- diff --git a/supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql b/supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql new file mode 100644 index 00000000..fd5a64ad --- /dev/null +++ b/supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql @@ -0,0 +1,17 @@ +-- Step 4b of the 2026-08 security remediation. +-- Advisory: GHSA-jpfm-vrpc-p6rr +-- +-- Second half of 20260815120000_move_backup_pins_to_service_role_table.sql. +-- That migration created the service-role-only `user_backup_pins` table and +-- carried across which users have a PIN set. This one removes the column that +-- every logged-in account could read via `users_select_authenticated`. +-- +-- ORDERING: apply this only AFTER the code that stopped reading +-- `users.backup_pin_hash` is deployed (src/app/api/auth/backup-pin/route.js). +-- Applying it against the previous release breaks GET/POST /api/auth/backup-pin. +-- +-- The unsalted SHA-256 digests in this column are discarded rather than +-- migrated: nothing ever verified them server-side, they only backed a boolean, +-- and their whole problem is that they are trivially reversible. + +ALTER TABLE public.users DROP COLUMN IF EXISTS backup_pin_hash; From ee655ee72434e9d102f852de8bbf29054b814926 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sat, 15 Aug 2026 20:47:32 +0000 Subject: [PATCH 2/2] chore(db): align migration filenames with the applied prod versions MCP apply_migration restamps the version it records; prod logged 20260815204334, not the 20260815120000 in the filename. Rename so supabase db push does not rerun it, and renumber the follow-up drop migration to stay after it. --- ...260815204334_move_backup_pins_to_service_role_table.sql} | 2 +- ...sh.sql => 20260815204400_drop_users_backup_pin_hash.sql} | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) rename supabase/migrations/{20260815120000_move_backup_pins_to_service_role_table.sql => 20260815204334_move_backup_pins_to_service_role_table.sql} (98%) rename supabase/migrations/{20260815120100_drop_users_backup_pin_hash.sql => 20260815204400_drop_users_backup_pin_hash.sql} (77%) diff --git a/supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql b/supabase/migrations/20260815204334_move_backup_pins_to_service_role_table.sql similarity index 98% rename from supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql rename to supabase/migrations/20260815204334_move_backup_pins_to_service_role_table.sql index 6bb8b528..97bb5def 100644 --- a/supabase/migrations/20260815120000_move_backup_pins_to_service_role_table.sql +++ b/supabase/migrations/20260815204334_move_backup_pins_to_service_role_table.sql @@ -67,7 +67,7 @@ ON CONFLICT (user_id) DO NOTHING; -- -------------------------------------------------------------------------- -- The `users.backup_pin_hash` column itself is dropped by the NEXT migration, --- 20260815120100_drop_users_backup_pin_hash.sql. It is deliberately a separate +-- 20260815204400_drop_users_backup_pin_hash.sql. It is deliberately a separate -- step: this migration is additive and safe to apply to a running deployment, -- whereas the drop must not land until the application code that stopped -- reading that column has shipped. diff --git a/supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql b/supabase/migrations/20260815204400_drop_users_backup_pin_hash.sql similarity index 77% rename from supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql rename to supabase/migrations/20260815204400_drop_users_backup_pin_hash.sql index fd5a64ad..6ca6d5aa 100644 --- a/supabase/migrations/20260815120100_drop_users_backup_pin_hash.sql +++ b/supabase/migrations/20260815204400_drop_users_backup_pin_hash.sql @@ -1,7 +1,11 @@ -- Step 4b of the 2026-08 security remediation. -- Advisory: GHSA-jpfm-vrpc-p6rr -- --- Second half of 20260815120000_move_backup_pins_to_service_role_table.sql. +-- Second half of 20260815204334_move_backup_pins_to_service_role_table.sql. +-- +-- NOTE: when this is applied via the Supabase MCP, the ledger will restamp it +-- with its own timestamp. Rename this file to match afterwards, or +-- `supabase db push` will rerun it. -- That migration created the service-role-only `user_backup_pins` table and -- carried across which users have a PIN set. This one removes the column that -- every logged-in account could read via `users_select_authenticated`.