security: address silent-failure review findings on top of batch 1

Second-pass fixes caught by a self-review after the initial commit — each
one would have undermined a fix from the previous commit.

- mfaPolicy now goes through `verifyJwtAndLoadUser` too. Without this,
  a JWT stolen before a password reset still satisfied `require_mfa`
  until its natural 24h expiry, defeating the whole point of the
  password_version bump.
- Drop the `?? keys[0]` fallback in OIDC JWKS key selection. When the
  token carries a `kid` that is not in the current JWKS, refuse
  outright instead of picking an arbitrary key and letting the
  signature check produce a generic failure — the real failure mode
  deserves a specific error code.
- Tighten OAuth DCR custom-scheme rule so `javascript:`, `data:`,
  `vbscript:`, `file:`, `blob:`, `about:`, `chrome:` are all rejected.
  Previously the catch-all "not http/https" check admitted them; the
  authorize flow later 302s the browser to whatever is registered,
  which with a `javascript:` URI would execute attacker script on
  redirect. Also require the private-use scheme body to be reverse-DNS
  (contain a dot), matching RFC 8252 §7.1.
- permanentDeleteFile / emptyTrash only delete the trip_files row when
  the on-disk unlink actually succeeded. Previously Promise.all
  swallowed individual unlink failures and DELETE ran unconditionally,
  so a permission / ENOSPC failure would orphan bytes on disk.
- restoreFromZip also invalidates the permissions cache in the outer
  catch. If extraction threw before the DB swap even started, the
  cache wasn't stale, but belt-and-braces is cheap and guarantees no
  failed-restore path leaves stale cache behind.

server/src/middleware/mfaPolicy.ts

@@ -1,8 +1,6 @@
 import { Request, Response, NextFunction } from 'express';
-import jwt from 'jsonwebtoken';
 import { db } from '../db/database';
-import { JWT_SECRET } from '../config';
+import { extractToken, verifyJwtAndLoadUser } from './auth';
-import { extractToken } from './auth';
 import { DEMO_EMAILS } from '../services/demo';
 
 /** Paths that never require MFA (public or pre-auth). */
@@ -54,14 +52,15 @@ export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFu
     return;
   }
 
-  let userId: number;
+  // Use the shared verify helper so the `password_version` gate applies
-  try {
+  // here too — a JWT stolen before a password reset would otherwise
-    const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number };
+  // continue to satisfy this middleware until its natural 24h expiry.
-    userId = decoded.id;
+  const verified = verifyJwtAndLoadUser(token);
-  } catch {
+  if (!verified) {
     next();
     return;
   }
+  const userId = verified.id;
 
   const requireRow = db.prepare("SELECT value FROM app_settings WHERE key = 'require_mfa'").get() as { value: string } | undefined;
   if (requireRow?.value !== 'true') {
@@ -69,16 +68,13 @@ export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFu
     return;
   }
 
-  if (process.env.DEMO_MODE === 'true') {
+  if (process.env.DEMO_MODE === 'true' && verified.email && DEMO_EMAILS.has(verified.email)) {
-    const demo = db.prepare('SELECT email FROM users WHERE id = ?').get(userId) as { email: string } | undefined;
+    next();
-    if (demo?.email && DEMO_EMAILS.has(demo.email)) {
+    return;
-      next();
-      return;
-    }
   }
 
-  const row = db.prepare('SELECT mfa_enabled, role FROM users WHERE id = ?').get(userId) as
+  const row = db.prepare('SELECT mfa_enabled FROM users WHERE id = ?').get(userId) as
-    | { mfa_enabled: number | boolean; role: string }
+    | { mfa_enabled: number | boolean }
     | undefined;
   if (!row) {
     next();

server/src/routes/oauth.ts

@@ -206,16 +206,28 @@ oauthPublicRouter.post('/oauth/register', dcrLimiter, (req: Request, res: Respon
     return res.status(400).json({ error: 'invalid_redirect_uri', error_description: 'redirect_uris is required and must be a non-empty array' });
   }
   // OAuth 2.1 + RFC 8252: confidential web apps need HTTPS; public
-  // clients (MCP, native) are limited to loopback or custom schemes.
+  // clients (MCP, native) are limited to loopback or a reverse-DNS
-  // This rejects `http://evil.example` DCR payloads that today would
+  // private-use scheme. This rejects `http://evil.example` DCR payloads
-  // otherwise be accepted since we previously only checked shape.
+  // that today would otherwise be accepted since we previously only
+  // checked shape. Dangerous URL schemes (`javascript:`, `data:` etc.)
+  // are explicitly rejected — the authorize flow later 302s the
+  // browser to this URI, which with `javascript:` would execute
+  // attacker-controlled script under our redirect origin's context.
+  const DANGEROUS_SCHEMES = new Set([
+    'javascript:', 'data:', 'vbscript:', 'file:', 'blob:', 'about:', 'chrome:', 'chrome-extension:',
+  ]);
   const allowed = redirectUris.every((u) => {
     try {
       const url = new URL(u);
+      if (DANGEROUS_SCHEMES.has(url.protocol)) return false;
       if (url.protocol === 'https:') return true;
       if (url.protocol === 'http:' && (url.hostname === 'localhost' || url.hostname === '127.0.0.1' || url.hostname === '[::1]')) return true;
-      // RFC 8252 custom scheme for native/MCP clients (e.g. "myapp://cb")
+      // RFC 8252 §7.1 private-use scheme: must be a reverse-DNS name
-      if (!/^https?:$/.test(url.protocol) && url.protocol.endsWith(':') && !url.protocol.includes(' ')) return true;
+      // (e.g. `com.example.myapp:/callback`). Requiring a dot in the
+      // scheme is a cheap heuristic that rules out bare `myapp:` and
+      // `x:` one-off schemes the spec explicitly discourages.
+      const schemeBody = url.protocol.slice(0, -1);
+      if (/^[a-z][a-z0-9+.-]*$/i.test(schemeBody) && schemeBody.includes('.')) return true;
       return false;
     } catch {
       return false;

server/src/services/backupService.ts

@@ -260,6 +260,13 @@ export async function restoreFromZip(zipPath: string): Promise<RestoreResult> {
   } catch (err: unknown) {
     console.error('Restore error:', err);
     if (fs.existsSync(extractDir)) fs.rmSync(extractDir, { recursive: true, force: true });
+    // Belt-and-braces: the inner `finally` already drops the permissions
+    // cache after a successful swap, but if the extraction/copy step
+    // itself threw before the DB swap even started, the cache wasn't
+    // stale anyway. Invalidating here too costs nothing and guarantees
+    // we never serve cached permissions that don't match the DB state
+    // we leave the process in after a failed restore.
+    try { invalidatePermissionsCache(); } catch { /* best-effort */ }
     throw err;
   }
 }

server/src/services/fileService.ts

@@ -205,20 +205,40 @@ export function restoreFile(id: string | number) {
 
 export async function permanentDeleteFile(file: TripFile): Promise<void> {
   const { resolved } = resolveFilePath(file.filename);
-  // `force: true` swallows ENOENT, removing the prior existsSync+unlink
+  // `force: true` swallows ENOENT, replacing the prior existsSync+unlink
-  // double-call that blocked the event loop twice per deletion.
+  // double-call that blocked the event loop twice per deletion. Only
-  await fs.promises.rm(resolved, { force: true }).catch((e) => console.error('Error deleting file:', e));
+  // drop the DB row when the on-disk unlink either succeeded or the
+  // file was already gone — otherwise a permission / ENOSPC failure
+  // would orphan the bytes on disk with no DB pointer left to clean it.
+  try {
+    await fs.promises.rm(resolved, { force: true });
+  } catch (e) {
+    console.error(`[files] unlink failed for ${file.filename}, keeping DB row:`, e);
+    throw e;
+  }
   db.prepare('DELETE FROM trip_files WHERE id = ?').run(file.id);
 }
 
 export async function emptyTrash(tripId: string | number): Promise<number> {
   const trashed = db.prepare('SELECT * FROM trip_files WHERE trip_id = ? AND deleted_at IS NOT NULL').all(tripId) as TripFile[];
+  // Collect successful IDs separately so we only DELETE rows whose disk
+  // content was actually removed — failing unlinks keep their DB row
+  // and a retry via the single-file delete path can try again.
+  const successfullyUnlinked: number[] = [];
   await Promise.all(trashed.map(async (file) => {
     const { resolved } = resolveFilePath(file.filename);
-    await fs.promises.rm(resolved, { force: true }).catch((e) => console.error('Error deleting file:', e));
+    try {
+      await fs.promises.rm(resolved, { force: true });
+      successfullyUnlinked.push(Number(file.id));
+    } catch (e) {
+      console.error(`[files] unlink failed for ${file.filename}, keeping DB row:`, e);
+    }
   }));
-  db.prepare('DELETE FROM trip_files WHERE trip_id = ? AND deleted_at IS NOT NULL').run(tripId);
+  if (successfullyUnlinked.length > 0) {
-  return trashed.length;
+    const placeholders = successfullyUnlinked.map(() => '?').join(',');
+    db.prepare(`DELETE FROM trip_files WHERE id IN (${placeholders})`).run(...successfullyUnlinked);
+  }
+  return successfullyUnlinked.length;
 }
 
 // ---------------------------------------------------------------------------

server/src/services/oidcService.ts

@@ -284,7 +284,13 @@ export async function verifyIdToken(
   try { keys = await fetchJwks(doc.jwks_uri); }
   catch (e) { return { ok: false, error: 'jwks_fetch_failed' }; }
 
-  const jwk = keys.find(k => !header.kid || k['kid'] === header.kid) ?? keys[0];
+  // When the token carries a `kid`, refuse to fall back to any other
+  // key in the JWKS — a mismatch means the token was signed with a key
+  // the provider no longer publishes, and we should reject rather than
+  // mask the failure by trying another key.
+  const jwk = header.kid
+    ? keys.find((k) => k['kid'] === header.kid)
+    : keys[0];
   if (!jwk) return { ok: false, error: 'no_matching_key' };
 
   let publicKey;