fix(security): resolve CodeQL high alerts #45 #46 #47

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

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 @@
 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 @@
   buildQrCodeDataUrl,
   generateRecoveryCodes,
   hashRecoveryCode,
+  verifyRecoveryCode,
+  isLegacySha256Hash,
   encryptSecret,
   decryptSecret,
 };

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

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');

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]

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', () => {