Skip to content

Commit e99c87c

Browse files
committed
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.
1 parent 4a3b54b commit e99c87c

9 files changed

Lines changed: 637 additions & 118 deletions

File tree

‎src/app/api/auth/backup-pin/route.js‎

Lines changed: 80 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,16 @@
22
// Handles setting and checking the user's backup PIN hash
33

44
import { NextResponse } from 'next/server';
5+
import { randomBytes, scrypt as scryptCallback } from 'node:crypto';
6+
import { promisify } from 'node:util';
57
import { createClient } from '@supabase/supabase-js';
68
import { createServiceRoleClient } from '@/lib/supabase/service-role.js';
79

10+
// node:crypto is required for scrypt, so pin this route to the Node runtime.
11+
export const runtime = 'nodejs';
12+
13+
const scrypt = promisify(scryptCallback);
14+
815
let supabaseServiceRole = null;
916
function getServiceRoleClient() {
1017
if (!supabaseServiceRole) {
@@ -94,17 +101,53 @@ async function authenticateUser(request) {
94101
}
95102
}
96103

104+
// scrypt work factors. N=16384/r=8/p=1 is the Node default and costs ~16MB and
105+
// tens of milliseconds per derivation -- enough to make an offline sweep of the
106+
// 6-12 digit PIN keyspace impractical per user, and the per-user salt means
107+
// there is no shared work across users.
108+
const SCRYPT_N = 16384;
109+
const SCRYPT_R = 8;
110+
const SCRYPT_P = 1;
111+
const SCRYPT_KEYLEN = 64;
112+
const SCRYPT_SALT_BYTES = 16;
113+
export const PIN_ALGORITHM = `scrypt-n${SCRYPT_N}-r${SCRYPT_R}-p${SCRYPT_P}`;
114+
97115
/**
98-
* Hash a PIN using SHA-256
116+
* Derive a PIN hash using scrypt and a per-user random salt.
117+
*
118+
* Replaces the previous unsalted `crypto.subtle.digest('SHA-256', pin)`, which
119+
* a rainbow table over the numeric PIN keyspace reversed instantly once the
120+
* hash column was readable (GHSA-jpfm-vrpc-p6rr).
121+
*
99122
* @param {string} pin
100-
* @returns {Promise<string>} hex-encoded hash
123+
* @param {string} [saltHex] existing salt, hex-encoded; a new one is generated when omitted
124+
* @returns {Promise<{hash: string, salt: string, algorithm: string}>}
101125
*/
102-
async function hashPin(pin) {
103-
const encoder = new TextEncoder();
104-
const data = encoder.encode(pin);
105-
const hashBuffer = await crypto.subtle.digest('SHA-256', data);
106-
const hashArray = Array.from(new Uint8Array(hashBuffer));
107-
return hashArray.map(b => b.toString(16).padStart(2, '0')).join('');
126+
async function hashPin(pin, saltHex) {
127+
const salt = saltHex ?? randomBytes(SCRYPT_SALT_BYTES).toString('hex');
128+
const derived = /** @type {Buffer} */ (
129+
await scrypt(pin, salt, SCRYPT_KEYLEN, { N: SCRYPT_N, r: SCRYPT_R, p: SCRYPT_P })
130+
);
131+
return { hash: derived.toString('hex'), salt, algorithm: PIN_ALGORITHM };
132+
}
133+
134+
/**
135+
* Resolve the internal users.id for a Supabase Auth user.
136+
* @param {{id: string}} user
137+
* @returns {Promise<{userId?: string, error?: string}>}
138+
*/
139+
async function resolveInternalUserId(user) {
140+
const { data, error } = await getServiceRoleClient()
141+
.from('users')
142+
.select('id')
143+
.eq('auth_user_id', user.id)
144+
.single();
145+
146+
if (error || !data?.id) {
147+
return { error: error?.message ?? 'User record not found' };
148+
}
149+
150+
return { userId: data.id };
108151
}
109152

110153
/**
@@ -118,18 +161,27 @@ export async function GET(request) {
118161
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 });
119162
}
120163

164+
const { userId, error: lookupError } = await resolveInternalUserId(user);
165+
if (lookupError || !userId) {
166+
console.error('Error checking backup PIN:', lookupError);
167+
return NextResponse.json({ error: 'Failed to check backup PIN' }, { status: 500 });
168+
}
169+
170+
// PIN material lives in user_backup_pins, which only the service role can
171+
// reach. A row's existence is what marks a PIN as set -- rows migrated from
172+
// the old unsalted column carry NULL hashes on purpose.
121173
const { data, error } = await getServiceRoleClient()
122-
.from('users')
123-
.select('backup_pin_hash')
124-
.eq('auth_user_id', user.id)
125-
.single();
174+
.from('user_backup_pins')
175+
.select('user_id')
176+
.eq('user_id', userId)
177+
.maybeSingle();
126178

127179
if (error) {
128180
console.error('Error checking backup PIN:', error);
129181
return NextResponse.json({ error: 'Failed to check backup PIN' }, { status: 500 });
130182
}
131183

132-
return NextResponse.json({ hasPin: !!data?.backup_pin_hash });
184+
return NextResponse.json({ hasPin: !!data });
133185
} catch (error) {
134186
console.error('Error in GET /api/auth/backup-pin:', error);
135187
return NextResponse.json({ error: 'Internal server error' }, { status: 500 });
@@ -164,12 +216,23 @@ export async function POST(request) {
164216
return NextResponse.json({ error: 'PIN must contain only digits' }, { status: 400 });
165217
}
166218

167-
const pinHash = await hashPin(pin);
219+
const { userId, error: lookupError } = await resolveInternalUserId(user);
220+
if (lookupError || !userId) {
221+
console.error('Error setting backup PIN:', lookupError);
222+
return NextResponse.json({ error: 'Failed to set backup PIN' }, { status: 500 });
223+
}
224+
225+
const { hash, salt, algorithm } = await hashPin(pin);
168226

169227
const { error } = await getServiceRoleClient()
170-
.from('users')
171-
.update({ backup_pin_hash: pinHash })
172-
.eq('auth_user_id', user.id);
228+
.from('user_backup_pins')
229+
.upsert({
230+
user_id: userId,
231+
pin_hash: hash,
232+
pin_salt: salt,
233+
algorithm,
234+
updated_at: new Date().toISOString()
235+
}, { onConflict: 'user_id' });
173236

174237
if (error) {
175238
console.error('Error setting backup PIN:', error);

‎src/app/api/auth/backup-pin/route.test.js‎

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import { beforeEach, describe, expect, it, vi } from 'vitest';
22

3+
import { createHash } from 'node:crypto';
4+
35
const mocks = vi.hoisted(() => ({
46
authGetUser: vi.fn(),
5-
serviceFrom: vi.fn()
7+
serviceFrom: vi.fn(),
8+
pinUpsert: vi.fn()
69
}));
710

811
vi.mock('@supabase/supabase-js', () => ({
@@ -33,14 +36,29 @@ function createUsersQuery() {
3336
eq: vi.fn(() => query),
3437
single: vi.fn(() =>
3538
Promise.resolve({
36-
data: { backup_pin_hash: 'pin-hash' },
39+
data: { id: 'internal-user-id' },
3740
error: null
3841
})
3942
)
4043
};
4144
return query;
4245
}
4346

47+
function createBackupPinsQuery() {
48+
const query = {
49+
select: vi.fn(() => query),
50+
eq: vi.fn(() => query),
51+
maybeSingle: vi.fn(() =>
52+
Promise.resolve({
53+
data: { user_id: 'internal-user-id' },
54+
error: null
55+
})
56+
),
57+
upsert: mocks.pinUpsert
58+
};
59+
return query;
60+
}
61+
4462
describe('backup PIN cookie authentication', () => {
4563
beforeEach(() => {
4664
vi.resetModules();
@@ -52,8 +70,10 @@ describe('backup PIN cookie authentication', () => {
5270
data: { user: { id: 'auth-user-id' } },
5371
error: null
5472
});
73+
mocks.pinUpsert.mockResolvedValue({ error: null });
5574
mocks.serviceFrom.mockImplementation((table) => {
5675
if (table === 'users') return createUsersQuery();
76+
if (table === 'user_backup_pins') return createBackupPinsQuery();
5777
throw new Error(`Unexpected table: ${table}`);
5878
});
5979
});
@@ -119,6 +139,59 @@ describe('backup PIN cookie authentication', () => {
119139
expect(mocks.authGetUser).toHaveBeenCalledWith('cookie-token');
120140
});
121141

142+
it('reads the has-pin flag from the service-role-only table, not from users', async () => {
143+
const { GET } = await import('./route.js');
144+
const response = await GET(
145+
new Request('https://qrypt.chat/api/auth/backup-pin', {
146+
headers: { authorization: 'Bearer access-token' }
147+
})
148+
);
149+
const body = await response.json();
150+
151+
expect(response.status).toBe(200);
152+
expect(body).toEqual({ hasPin: true });
153+
expect(mocks.serviceFrom).toHaveBeenCalledWith('user_backup_pins');
154+
});
155+
156+
it('stores a salted scrypt hash rather than an unsalted SHA-256 digest', async () => {
157+
const { POST } = await import('./route.js');
158+
const pin = '123456';
159+
const response = await POST({
160+
headers: new Headers({ authorization: 'Bearer access-token' }),
161+
json: vi.fn().mockResolvedValue({ pin })
162+
});
163+
164+
expect(response.status).toBe(200);
165+
expect(mocks.pinUpsert).toHaveBeenCalledTimes(1);
166+
167+
const [record] = mocks.pinUpsert.mock.calls[0];
168+
expect(record.user_id).toBe('internal-user-id');
169+
expect(record.algorithm).toMatch(/^scrypt-/);
170+
171+
// A salt is present and actually mixed in: the stored hash must not be the
172+
// bare SHA-256 of the PIN, which is what GHSA-jpfm-vrpc-p6rr was about.
173+
expect(record.pin_salt).toMatch(/^[0-9a-f]{32}$/);
174+
const unsaltedSha256 = createHash('sha256').update(pin).digest('hex');
175+
expect(record.pin_hash).not.toBe(unsaltedSha256);
176+
expect(record.pin_hash).toMatch(/^[0-9a-f]{128}$/);
177+
});
178+
179+
it('derives a different hash per user for the same PIN', async () => {
180+
const { POST } = await import('./route.js');
181+
const request = () => ({
182+
headers: new Headers({ authorization: 'Bearer access-token' }),
183+
json: vi.fn().mockResolvedValue({ pin: '123456' })
184+
});
185+
186+
await POST(request());
187+
await POST(request());
188+
189+
const [first] = mocks.pinUpsert.mock.calls[0];
190+
const [second] = mocks.pinUpsert.mock.calls[1];
191+
expect(first.pin_salt).not.toBe(second.pin_salt);
192+
expect(first.pin_hash).not.toBe(second.pin_hash);
193+
});
194+
122195
it('returns 400 for malformed JSON instead of a generic 500', async () => {
123196
const { POST } = await import('./route.js');
124197
const response = await POST({

0 commit comments

Comments
 (0)