From 7e4e40307f86bab87a0bcfd33378855ca879f6a5 Mon Sep 17 00:00:00 2001 From: NeuroKoder3 Date: Thu, 30 Jul 2026 15:36:26 -0500 Subject: [PATCH] fix(security): resolve CodeQL high alerts #45 #46 #47 Co-authored-by: Cursor --- electron/database/init.cjs | 29 ++++++++++++-------- server/src/auth/mfa.js | 44 ++++++++++++++++++++++++++++-- server/src/routes/auth.js | 4 ++- server/src/routes/smart.js | 21 ++++++++------ server/src/services/authService.js | 34 +++++++++++++++++------ server/test/unit/mfa.test.mjs | 19 +++++++++++++ 6 files changed, 118 insertions(+), 33 deletions(-) diff --git a/electron/database/init.cjs b/electron/database/init.cjs index e03efb3..46fc66a 100644 --- a/electron/database/init.cjs +++ b/electron/database/init.cjs @@ -291,20 +291,25 @@ async function migrateToEncrypted(unencryptedPath, encryptedPath, encryptionKey) // Move new encrypted database to final location fs.renameSync(encryptedPath + '.new', encryptedPath); - // Securely overwrite the plaintext database: write zeros over the file, - // then unlink. Never leave plaintext PHI on disk. + // Securely overwrite the plaintext database: open by fd, fstat that fd, + // zero the contents, then unlink. Avoids TOCTOU between exists/stat and write. try { - const stat = fs.statSync(unencryptedPath); - const fd = fs.openSync(unencryptedPath, 'w'); - const zeroChunk = Buffer.alloc(Math.min(stat.size, 64 * 1024), 0); - let remaining = stat.size; - while (remaining > 0) { - const toWrite = Math.min(remaining, zeroChunk.length); - fs.writeSync(fd, zeroChunk, 0, toWrite); - remaining -= toWrite; + const fd = fs.openSync(unencryptedPath, 'r+'); + try { + const stat = fs.fstatSync(fd); + const zeroChunk = Buffer.alloc(Math.min(stat.size, 64 * 1024), 0); + let remaining = stat.size; + let offset = 0; + while (remaining > 0) { + const toWrite = Math.min(remaining, zeroChunk.length); + fs.writeSync(fd, zeroChunk, 0, toWrite, offset); + offset += toWrite; + remaining -= toWrite; + } + fs.fdatasyncSync(fd); + } finally { + fs.closeSync(fd); } - fs.fdatasyncSync(fd); - fs.closeSync(fd); fs.unlinkSync(unencryptedPath); } catch (wipeErr) { // Best-effort: at minimum, unlink it diff --git a/server/src/auth/mfa.js b/server/src/auth/mfa.js index 815f106..6857cd3 100644 --- a/server/src/auth/mfa.js +++ b/server/src/auth/mfa.js @@ -3,6 +3,7 @@ const crypto = require('crypto'); const otplib = require('otplib'); const QRCode = require('qrcode'); +const password = require('./password'); /** * TOTP (RFC 6238) helpers, plus AES-256-GCM encryption of the shared secret @@ -99,13 +100,48 @@ async function buildQrCodeDataUrl(otpauthUrl) { function generateRecoveryCodes(n = 10) { const codes = []; for (let i = 0; i < n; i++) { - codes.push(crypto.randomBytes(5).toString('hex').toUpperCase()); + // 80 bits of entropy (10 random bytes → 20 hex chars) + codes.push(crypto.randomBytes(10).toString('hex').toUpperCase()); } return codes; } -function hashRecoveryCode(code) { - return crypto.createHash('sha256').update(code.toUpperCase().trim()).digest('hex'); +function normalizeRecoveryCode(code) { + return String(code || '').toUpperCase().trim(); +} + +function isLegacySha256Hash(stored) { + return typeof stored === 'string' && /^[a-f0-9]{64}$/i.test(stored); +} + +/** + * Hash a recovery code with Argon2id (same parameters as passwords). + * Legacy SHA-256 hashes are still accepted by verifyRecoveryCode. + */ +async function hashRecoveryCode(code) { + return password.hash(normalizeRecoveryCode(code)); +} + +/** + * Verify a recovery code against a stored hash. + * Supports Argon2id and legacy SHA-256 hex digests. + */ +async function verifyRecoveryCode(code, storedHash) { + if (!storedHash) return false; + const normalized = normalizeRecoveryCode(code); + if (!normalized) return false; + if (isLegacySha256Hash(storedHash)) { + const legacy = crypto.createHash('sha256').update(normalized).digest('hex'); + try { + return crypto.timingSafeEqual( + Buffer.from(legacy, 'utf8'), + Buffer.from(storedHash.toLowerCase(), 'utf8') + ); + } catch { + return false; + } + } + return password.verify(storedHash, normalized); } module.exports = { @@ -116,6 +152,8 @@ module.exports = { buildQrCodeDataUrl, generateRecoveryCodes, hashRecoveryCode, + verifyRecoveryCode, + isLegacySha256Hash, encryptSecret, decryptSecret, }; diff --git a/server/src/routes/auth.js b/server/src/routes/auth.js index 3b7d72b..ac17355 100644 --- a/server/src/routes/auth.js +++ b/server/src/routes/auth.js @@ -179,7 +179,9 @@ module.exports = async function authRoutes(app, opts) { const secret = mfa.decryptSecret(r.rows[0].secret_encrypted, config.JWT_SECRET); if (!mfa.verifyCode(secret, body.code)) throw errors.badRequest('Invalid code'); const codes = mfa.generateRecoveryCodes(10); - const stored = codes.map(c => ({ hash: mfa.hashRecoveryCode(c), used_at: null })); + const stored = await Promise.all( + codes.map(async (c) => ({ hash: await mfa.hashRecoveryCode(c), used_at: null })) + ); await client.query( `UPDATE mfa_enrollments SET confirmed_at = now(), recovery_codes = $1 diff --git a/server/src/routes/smart.js b/server/src/routes/smart.js index 4a1aeb2..8501940 100644 --- a/server/src/routes/smart.js +++ b/server/src/routes/smart.js @@ -342,18 +342,23 @@ module.exports = async function smartRoutes(app, opts) { if (grantType === 'refresh_token') { const data = z.object({ refresh_token: z.string().min(1) }).parse(body); - // Authenticate confidential client if credentials are provided - if (clientId) { - const smartClient = await clients.getUnscoped(clientId); - if (smartClient && smartClient.client_type === 'confidential') { - const ok = await clients.verifySecret(smartClient, clientSecret); - if (!ok) throw errors.unauthorized('invalid_client'); - } + // Resolve the client from the refresh token (server-controlled), not from + // caller-supplied client_id. Confidential clients MUST authenticate. + const tokenClientId = await tokens.lookupTokenClientId(data.refresh_token); + if (!tokenClientId) throw errors.unauthorized('invalid_grant'); + if (clientId && clientId !== tokenClientId) { + throw errors.unauthorized('invalid_grant'); + } + const smartClient = await clients.getUnscoped(tokenClientId); + if (!smartClient) throw errors.unauthorized('invalid_client'); + if (smartClient.client_type === 'confidential') { + const ok = await clients.verifySecret(smartClient, clientSecret); + if (!ok) throw errors.unauthorized('invalid_client'); } try { return await tokens.refresh(data.refresh_token, { ttlSeconds: config.JWT_ACCESS_TTL_SECONDS, - clientId: clientId || undefined, + clientId: tokenClientId, }); } catch (_e) { throw errors.unauthorized('invalid_grant'); diff --git a/server/src/services/authService.js b/server/src/services/authService.js index 8514947..f978bd2 100644 --- a/server/src/services/authService.js +++ b/server/src/services/authService.js @@ -204,12 +204,20 @@ async function consumeMfaChallenge(client, config, { challengeId, code, ip, user const secret = mfa.decryptSecret(ch.secret_encrypted, config.JWT_SECRET); const ok = mfa.verifyCode(secret, code); if (!ok) { - // Try recovery codes (one-time) - const codeHash = mfa.hashRecoveryCode(String(code || '')); + // Try recovery codes (one-time). Argon2id (with legacy SHA-256 upgrade). const list = ch.recovery_codes || []; - const idx = list.findIndex(c => c.hash === codeHash && !c.used_at); - if (idx < 0) throw errors.unauthorized('Invalid code'); - list[idx].used_at = new Date().toISOString(); + let matched = false; + for (let i = 0; i < list.length; i++) { + if (list[i].used_at) continue; + if (!(await mfa.verifyRecoveryCode(code, list[i].hash))) continue; + list[i].used_at = new Date().toISOString(); + if (mfa.isLegacySha256Hash(list[i].hash)) { + list[i].hash = await mfa.hashRecoveryCode(code); + } + matched = true; + break; + } + if (!matched) throw errors.unauthorized('Invalid code'); await client.query( `UPDATE mfa_enrollments SET recovery_codes = $1 WHERE user_id = $2`, [JSON.stringify(list), ch.user_id] @@ -444,11 +452,19 @@ async function verifySmartMfa({ challengeId, code, userId }) { const secret = mfa.decryptSecret(ch.secret_encrypted, config.JWT_SECRET); const valid = mfa.verifyCode(secret, code); if (!valid) { - const codeHash = mfa.hashRecoveryCode(String(code || '')); const list = ch.recovery_codes || []; - const idx = list.findIndex(c => c.hash === codeHash && !c.used_at); - if (idx < 0) throw errors.unauthorized('Invalid code'); - list[idx].used_at = new Date().toISOString(); + let matched = false; + for (let i = 0; i < list.length; i++) { + if (list[i].used_at) continue; + if (!(await mfa.verifyRecoveryCode(code, list[i].hash))) continue; + list[i].used_at = new Date().toISOString(); + if (mfa.isLegacySha256Hash(list[i].hash)) { + list[i].hash = await mfa.hashRecoveryCode(code); + } + matched = true; + break; + } + if (!matched) throw errors.unauthorized('Invalid code'); await client.query( `UPDATE mfa_enrollments SET recovery_codes = $1 WHERE user_id = $2`, [JSON.stringify(list), ch.user_id] diff --git a/server/test/unit/mfa.test.mjs b/server/test/unit/mfa.test.mjs index 391a1f8..ec707fe 100644 --- a/server/test/unit/mfa.test.mjs +++ b/server/test/unit/mfa.test.mjs @@ -28,6 +28,25 @@ describe('mfa', () => { const codes = mfa.generateRecoveryCodes(10); expect(codes).toHaveLength(10); expect(new Set(codes).size).toBe(10); + for (const c of codes) { + expect(c).toMatch(/^[0-9A-F]{20}$/); + } + }); + + it('hashes recovery codes with Argon2id and verifies them', async () => { + const codes = mfa.generateRecoveryCodes(1); + const hash = await mfa.hashRecoveryCode(codes[0]); + expect(hash.startsWith('$argon2')).toBe(true); + expect(await mfa.verifyRecoveryCode(codes[0], hash)).toBe(true); + expect(await mfa.verifyRecoveryCode('DEADBEEFDEADBEEFDEAD', hash)).toBe(false); + }); + + it('still accepts legacy SHA-256 recovery hashes', async () => { + const code = 'ABCDEF1234567890ABCD'; + const crypto = require('crypto'); + const legacy = crypto.createHash('sha256').update(code).digest('hex'); + expect(mfa.isLegacySha256Hash(legacy)).toBe(true); + expect(await mfa.verifyRecoveryCode(code, legacy)).toBe(true); }); it('builds an otpauth URI', () => {