Merge pull request #766 from mauriceboe/security/audit-fixes-batch-1

security: internal audit — batch 1

release-notes.md

@@ -1,7 +0,0 @@
-# Release Notes
-
-## v2.9.11
-
-### Bug Fixes
-
-- **OIDC-only mode: resolved login/logout loop** — When password authentication is disabled, logging out no longer triggers an immediate re-authentication loop. After logout, users land on the login page and must manually click "Sign in with {provider}" to start the OIDC flow. Also fixed a secondary loop that could occur on the OIDC callback page under React 18 StrictMode, where the auth code exchange would be interrupted before completing, causing the app to redirect back to the identity provider instead of landing on the dashboard. (#491)

server/src/app.ts

@@ -5,11 +5,9 @@ import cookieParser from 'cookie-parser';
 import path from 'node:path';
 import fs from 'node:fs';
 
-import jwt from 'jsonwebtoken';
-import { JWT_SECRET } from './config';
 import { logDebug, logWarn, logError } from './services/auditLog';
 import { enforceGlobalMfaPolicy } from './middleware/mfaPolicy';
-import { authenticate } from './middleware/auth';
+import { authenticate, verifyJwtAndLoadUser } from './middleware/auth';
 import { db } from './db/database';
 
 import authRoutes from './routes/auth';
@@ -76,6 +74,19 @@ export function createApp(): express.Application {
   }
 
   const shouldForceHttps = process.env.FORCE_HTTPS === 'true';
+  // HSTS is worth enabling any time we're serving production traffic,
+  // not only when FORCE_HTTPS is set. Self-hosters behind Traefik /
+  // Caddy / Cloudflare Tunnel typically leave FORCE_HTTPS unset (the
+  // proxy handles the redirect for them), and the previous "HSTS off by
+  // default" meant those instances never advertised HSTS at all.
+  //
+  // `includeSubDomains` stays OFF by default on purpose: an instance
+  // running on an apex domain would otherwise force HTTPS on every
+  // sibling subdomain the same operator may still be running over plain
+  // HTTP. Operators who want the stricter policy opt in with
+  // `HSTS_INCLUDE_SUBDOMAINS=true`.
+  const hstsActive = shouldForceHttps || process.env.NODE_ENV === 'production';
+  const hstsIncludeSubdomains = process.env.HSTS_INCLUDE_SUBDOMAINS === 'true';
 
   // RFC 8414 / RFC 9728: discovery docs are world-readable — open CORS regardless of deployment config
   app.use(
@@ -112,7 +123,7 @@ export function createApp(): express.Application {
       }
     },
     crossOriginEmbedderPolicy: false,
-    hsts: shouldForceHttps ? { maxAge: 31536000, includeSubDomains: false } : false,
+    hsts: hstsActive ? { maxAge: 31536000, includeSubDomains: hstsIncludeSubdomains } : false,
   }));
 
   if (shouldForceHttps) {
@@ -161,12 +172,33 @@ export function createApp(): express.Application {
     });
   }
 
-  // Static: avatars, covers, and journey photos
+  // Static: avatars, covers, and journey photos.
+  //
+  // Security model (audit SEC-M9): these paths are unauthenticated by
+  // design. All filenames are server-chosen UUID v4 (see `uuid()` in
+  // the multer storage config for avatars / covers / journey uploads),
+  // which gives each asset >122 bits of namespace entropy — not
+  // guessable via enumeration. An attacker would need to have already
+  // seen the URL (email, shared journey, etc.) to request the file.
+  //
+  // Moving these behind auth would also break:
+  //   - Unauthenticated trip-card rendering on public share links
+  //   - Journey public-share pages (/public/journey/:token)
+  //   - Email-embedded avatars
+  //
+  // The `/uploads/photos/...` route below is DIFFERENT: photo URLs are
+  // not embedded in unauthenticated UI contexts, so that endpoint IS
+  // gated (session JWT with pv, or a share token scoped to the photo's
+  // trip).
   app.use('/uploads/avatars', express.static(path.join(__dirname, '../uploads/avatars')));
   app.use('/uploads/covers', express.static(path.join(__dirname, '../uploads/covers')));
   app.use('/uploads/journey', express.static(path.join(__dirname, '../uploads/journey')));
 
-  // Photos require auth or valid share token
+  // Photos require either a valid logged-in session (via JWT with the
+  // password_version gate) OR a share token that covers the SPECIFIC
+  // photo's trip. Previously any share token for any trip could request
+  // any photo filename by UUID — fine in practice because UUIDs are
+  // unguessable, but the auth model was wrong.
   app.get('/uploads/photos/:filename', (req: Request, res: Response) => {
     const safeName = path.basename(req.params.filename);
     const filePath = path.join(__dirname, '../uploads/photos', safeName);
@@ -174,17 +206,28 @@ export function createApp(): express.Application {
     if (!resolved.startsWith(path.resolve(__dirname, '../uploads/photos'))) {
       return res.status(403).send('Forbidden');
     }
+    // existsSync here is cheap and avoids a sendFile error frame; kept
+    // sync because the handler is already short-lived.
     if (!fs.existsSync(resolved)) return res.status(404).send('Not found');
 
     const authHeader = req.headers.authorization;
-    const token = (req.query.token as string) || (authHeader?.startsWith('Bearer ') ? authHeader.slice(7) : null);
+    const rawToken = (req.query.token as string) || (authHeader?.startsWith('Bearer ') ? authHeader.slice(7) : null);
-    if (!token) return res.status(401).send('Authentication required');
+    if (!rawToken) return res.status(401).send('Authentication required');
 
-    try {
+    // JWT session path (with pv check).
-      jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] });
+    const user = verifyJwtAndLoadUser(rawToken);
-    } catch {
+    if (user) return res.sendFile(resolved);
-      const shareRow = db.prepare('SELECT id FROM share_tokens WHERE token = ?').get(token);
+
-      if (!shareRow) return res.status(401).send('Authentication required');
+    // Share-token path: require the token to cover the exact trip the
+    // photo belongs to. Expired tokens fall through to 401.
+    const photo = db.prepare('SELECT trip_id FROM photos WHERE filename = ?').get(safeName) as { trip_id: number } | undefined;
+    if (!photo) return res.status(401).send('Authentication required');
+
+    const share = db.prepare(
+      "SELECT trip_id FROM share_tokens WHERE token = ? AND (expires_at IS NULL OR expires_at > datetime('now'))"
+    ).get(rawToken) as { trip_id: number } | undefined;
+    if (!share || share.trip_id !== photo.trip_id) {
+      return res.status(401).send('Authentication required');
     }
     res.sendFile(resolved);
   });

server/src/db/migrations.ts

@@ -1799,6 +1799,83 @@ function runMigrations(db: Database.Database): void {
       try { db.exec('ALTER TABLE todo_items ADD COLUMN reminded_at DATETIME'); }
       catch (err: any) { if (!err.message?.includes('duplicate column name')) throw err; }
     },
+    // Migration: security audit batch 1 — columns + indexes required
+    // by several fixes bundled into one PR.
+    // - share_tokens.expires_at: public share links now get a 90-day
+    //   TTL by default; existing rows stay NULL (= no expiry) to avoid
+    //   silently breaking already-published links.
+    // - Missing indexes on high-cardinality query paths (see PERF-H1
+    //   in the audit): every listTrips() used to full-scan trips on
+    //   user_id, and notifications/photos/reservations had similar
+    //   gaps.
+    () => {
+      try { db.exec('ALTER TABLE share_tokens ADD COLUMN expires_at TEXT'); }
+      catch (err: any) { if (!err.message?.includes('duplicate column name')) throw err; }
+      db.exec(`
+        CREATE INDEX IF NOT EXISTS idx_trips_user_id ON trips(user_id);
+        CREATE INDEX IF NOT EXISTS idx_trips_created_at ON trips(created_at DESC);
+        CREATE INDEX IF NOT EXISTS idx_photos_day_id ON photos(day_id);
+        CREATE INDEX IF NOT EXISTS idx_photos_place_id ON photos(place_id);
+        CREATE INDEX IF NOT EXISTS idx_reservations_day_id ON reservations(day_id);
+        CREATE INDEX IF NOT EXISTS idx_share_tokens_token ON share_tokens(token);
+      `);
+      try {
+        // day_accommodations may have either start_day_id/end_day_id or a
+        // single day_id depending on how far the schema has evolved;
+        // build whichever index makes sense for the live columns.
+        const cols = db.prepare("PRAGMA table_info('day_accommodations')").all() as Array<{ name: string }>;
+        const names = new Set(cols.map((c) => c.name));
+        if (names.has('start_day_id')) db.exec('CREATE INDEX IF NOT EXISTS idx_day_accommodations_start_day_id ON day_accommodations(start_day_id)');
+        if (names.has('end_day_id')) db.exec('CREATE INDEX IF NOT EXISTS idx_day_accommodations_end_day_id ON day_accommodations(end_day_id)');
+      } catch { /* table may not exist on very old installs */ }
+      try {
+        // notifications schema has varied; probe before indexing.
+        const cols = db.prepare("PRAGMA table_info('notifications')").all() as Array<{ name: string }>;
+        const names = new Set(cols.map((c) => c.name));
+        if (names.has('target') && names.has('scope')) {
+          db.exec('CREATE INDEX IF NOT EXISTS idx_notifications_target_scope ON notifications(target, scope)');
+        }
+      } catch { /* notifications table may not exist on very old installs */ }
+    },
+    // Migration: widen idempotency_keys primary key to (key, user_id,
+    // method, path). The middleware lookup was widened in the same audit
+    // batch so a reused X-Idempotency-Key against a different endpoint
+    // does not replay the cached body of an unrelated request. The old
+    // PK was only (key, user_id), so the `INSERT OR IGNORE` on the
+    // second endpoint silently skipped — the cache never stored request
+    // B's response and replays re-executed the handler. Rebuild the
+    // table with the widened PK, preserving existing rows (the old PK
+    // guarantees no conflicts in the new, strictly looser unique key).
+    () => {
+      const hasTable = db.prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name = 'idempotency_keys'").get();
+      if (!hasTable) return;
+      db.exec(`
+        CREATE TABLE idempotency_keys_new (
+          key         TEXT NOT NULL,
+          user_id     INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
+          method      TEXT NOT NULL,
+          path        TEXT NOT NULL,
+          status_code INTEGER NOT NULL,
+          response_body TEXT NOT NULL,
+          created_at  INTEGER NOT NULL DEFAULT (strftime('%s','now')),
+          PRIMARY KEY (key, user_id, method, path)
+        );
+        INSERT INTO idempotency_keys_new (key, user_id, method, path, status_code, response_body, created_at)
+          SELECT key, user_id, method, path, status_code, response_body, created_at FROM idempotency_keys;
+        DROP TABLE idempotency_keys;
+        ALTER TABLE idempotency_keys_new RENAME TO idempotency_keys;
+        CREATE INDEX IF NOT EXISTS idx_idempotency_keys_created ON idempotency_keys(created_at);
+      `);
+    },
+    // SEC-H6: revoke all OAuth tokens issued before audience binding was
+    // enforced. mcp/index.ts now unconditionally checks audience; tokens
+    // with audience=null would be permanently rejected by the check, so
+    // removing them here avoids leaving dead rows and makes the intent clear.
+    () => {
+      const hasCol = db.prepare("SELECT name FROM pragma_table_info('oauth_tokens') WHERE name = 'audience'").get();
+      if (!hasCol) return;
+      db.prepare('DELETE FROM oauth_tokens WHERE audience IS NULL').run();
+    },
   ];
 
   if (currentVersion < migrations.length) {

server/src/mcp/index.ts

@@ -180,11 +180,10 @@ function verifyToken(authHeader: string | undefined): VerifyTokenResult | null {
   if (token.startsWith('trekoa_')) {
     const result = getUserByAccessToken(token);
     if (!result) return null;
-    // RFC 8707: if the token carries an audience, it must match this resource endpoint
+    // RFC 8707: audience must always match this resource endpoint.
-    if (result.audience !== null) {
+    // Pre-audit tokens with audience=null are revoked by the SEC-H6 migration.
-      const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`;
+    const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`;
-      if (result.audience !== expected) return null;
+    if (result.audience !== expected) return null;
-    }
     return { user: result.user, scopes: result.scopes, clientId: result.clientId, isStaticToken: false };
   }
 

server/src/middleware/auth.ts

@@ -4,6 +4,7 @@ import { db } from '../db/database';
 import { JWT_SECRET } from '../config';
 import { AuthRequest, OptionalAuthRequest, User } from '../types';
 import { applyIdempotency } from './idempotency';
+import { isDemoEmail } from '../services/demo';
 
 export function extractToken(req: Request): string | null {
   // Prefer httpOnly cookie; fall back to Authorization: Bearer (MCP, API clients)
@@ -13,7 +14,18 @@ export function extractToken(req: Request): string | null {
   return (authHeader && authHeader.split(' ')[1]) || null;
 }
 
-function verifyJwtAndLoadUser(token: string): User | null {
+/**
+ * Verify a JWT and load its user, enforcing the password_version gate.
+ *
+ * Exported so every auth surface in the codebase (MCP bearer tokens,
+ * file download query tokens, the photo-serving route) goes through the
+ * same check. A password reset bumps `users.password_version`, which
+ * invalidates every JWT that embedded the prior value — but only if
+ * every verify path actually compares the claim. Previously several
+ * paths called `jwt.verify` directly and skipped the DB lookup, so a
+ * stolen token kept working after the victim reset.
+ */
+export function verifyJwtAndLoadUser(token: string): User | null {
   try {
     const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number; pv?: number };
     const row = db.prepare(
@@ -93,8 +105,8 @@ const adminOnly = (req: Request, res: Response, next: NextFunction): void => {
 
 const demoUploadBlock = (req: Request, res: Response, next: NextFunction): void => {
   const authReq = req as AuthRequest;
-  if (process.env.DEMO_MODE === 'true' && authReq.user?.email === 'demo@nomad.app') {
+  if (process.env.DEMO_MODE === 'true' && isDemoEmail(authReq.user?.email)) {
-    res.status(403).json({ error: 'Uploads are disabled in demo mode. Self-host NOMAD for full functionality.' });
+    res.status(403).json({ error: 'Uploads are disabled in demo mode. Self-host TREK for full functionality.' });
     return;
   }
   next();

server/src/middleware/idempotency.ts

@@ -2,6 +2,13 @@ import { Request, Response, NextFunction } from 'express';
 import { db } from '../db/database';
 
 const MUTATING_METHODS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']);
+// Reject pathological client-supplied keys outright instead of hashing
+// everything — 128 chars is plenty for any realistic UUID / ULID / nonce.
+const MAX_KEY_LENGTH = 128;
+// Responses larger than this are not worth caching — a backup-restore
+// endpoint could otherwise store a megabyte-sized JSON body per request
+// key and, with mass key creation, blow up idempotency_keys.
+const MAX_CACHED_BODY_BYTES = 256 * 1024;
 
 interface IdempotencyRow {
   status_code: number;
@@ -12,9 +19,14 @@ interface IdempotencyRow {
  * Called from within `authenticate` after req.user is set.
  *
  * For mutating requests carrying X-Idempotency-Key:
- * - If (key, userId) already stored: replays the cached response.
+ * - If (key, userId, method, path) already stored: replays the cached response.
  * - Otherwise: wraps res.json to capture and store a successful response.
  *
+ * The lookup is scoped by method + path as well as user so the same key
+ * replayed against a different endpoint doesn't return the cached body
+ * of an unrelated request. Key length is capped and the cached body is
+ * skipped when it exceeds `MAX_CACHED_BODY_BYTES`.
+ *
  * Storing happens in idempotency_keys (24h TTL, cleaned by scheduler).
  */
 export function applyIdempotency(req: Request, res: Response, next: NextFunction, userId: number): void {
@@ -28,11 +40,17 @@ export function applyIdempotency(req: Request, res: Response, next: NextFunction
     next();
     return;
   }
+  if (key.length > MAX_KEY_LENGTH) {
+    res.status(400).json({ error: 'X-Idempotency-Key exceeds maximum length of 128 characters' });
+    return;
+  }
 
-  // Return cached response if key already processed for this user
+  // Return cached response only if the same key was seen for the same
+  // user AND the same method+path — avoids a POST's cached body leaking
+  // into an unrelated PATCH that reused the idempotency-key string.
   const existing = db.prepare(
-    'SELECT status_code, response_body FROM idempotency_keys WHERE key = ? AND user_id = ?'
+    'SELECT status_code, response_body FROM idempotency_keys WHERE key = ? AND user_id = ? AND method = ? AND path = ?'
-  ).get(key, userId) as IdempotencyRow | undefined;
+  ).get(key, userId, req.method, req.path) as IdempotencyRow | undefined;
 
   if (existing) {
     res.status(existing.status_code).json(JSON.parse(existing.response_body));
@@ -44,10 +62,13 @@ export function applyIdempotency(req: Request, res: Response, next: NextFunction
   res.json = function (body: unknown): Response {
     if (res.statusCode >= 200 && res.statusCode < 300) {
       try {
-        db.prepare(
+        const serialized = JSON.stringify(body);
-          `INSERT OR IGNORE INTO idempotency_keys (key, user_id, method, path, status_code, response_body, created_at)
+        if (serialized.length <= MAX_CACHED_BODY_BYTES) {
-           VALUES (?, ?, ?, ?, ?, ?, ?)`
+          db.prepare(
-        ).run(key, userId, req.method, req.path, res.statusCode, JSON.stringify(body), Math.floor(Date.now() / 1000));
+            `INSERT OR IGNORE INTO idempotency_keys (key, user_id, method, path, status_code, response_body, created_at)
+             VALUES (?, ?, ?, ?, ?, ?, ?)`
+          ).run(key, userId, req.method, req.path, res.statusCode, serialized, Math.floor(Date.now() / 1000));
+        }
       } catch {
         // Non-fatal: if storage fails, the request still succeeds
       }

server/src/middleware/mfaPolicy.ts

@@ -1,7 +1,7 @@
 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 { DEMO_EMAILS } from '../services/demo';
 
 /** Paths that never require MFA (public or pre-auth). */
 export function isPublicApiPath(method: string, pathNoQuery: string): boolean {
@@ -42,21 +42,25 @@ export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFu
     return;
   }
 
-  const authHeader = req.headers.authorization;
+  // Accept both the httpOnly session cookie (regular SPA users) and the
-  const token = authHeader && authHeader.split(' ')[1];
+  // Authorization header (MCP / API clients). Previously this only looked
+  // at the header so every normal cookie-authenticated session sailed
+  // past `require_mfa` unchecked.
+  const token = extractToken(req);
   if (!token) {
     next();
     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') {
@@ -64,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@trek.app' || demo?.email === 'demo@nomad.app') {
+    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/auth.ts

@@ -39,7 +39,7 @@ import {
   requestPasswordReset,
   resetPassword,
 } from '../services/authService';
-import { sendPasswordResetEmail } from '../services/notifications';
+import { sendPasswordResetEmail, getAppUrl } from '../services/notifications';
 
 const router = express.Router();
 
@@ -127,10 +127,10 @@ router.get('/app-config', optionalAuth, (req: Request, res: Response) => {
   res.json(getAppConfig(user));
 });
 
-router.post('/demo-login', (_req: Request, res: Response) => {
+router.post('/demo-login', (req: Request, res: Response) => {
   const result = demoLogin();
   if (result.error) return res.status(result.status!).json({ error: result.error });
-  setAuthCookie(res, result.token!);
+  setAuthCookie(res, result.token!, req);
   res.json({ token: result.token, user: result.user });
 });
 
@@ -144,7 +144,7 @@ router.post('/register', authLimiter, (req: Request, res: Response) => {
   const result = registerUser(req.body);
   if (result.error) return res.status(result.status!).json({ error: result.error });
   writeAudit({ userId: result.auditUserId!, action: 'user.register', ip: getClientIp(req), details: result.auditDetails });
-  setAuthCookie(res, result.token!);
+  setAuthCookie(res, result.token!, req);
   res.status(201).json({ token: result.token, user: result.user });
 });
 
@@ -155,7 +155,7 @@ router.post('/login', authLimiter, (req: Request, res: Response) => {
   }
   if (result.error) return res.status(result.status!).json({ error: result.error });
   if (result.mfa_required) return res.json({ mfa_required: true, mfa_token: result.mfa_token });
-  setAuthCookie(res, result.token!);
+  setAuthCookie(res, result.token!, req);
   res.json({ token: result.token, user: result.user });
 });
 
@@ -178,11 +178,12 @@ router.post('/forgot-password', forgotLimiter, async (req: Request, res: Respons
   const outcome = requestPasswordReset(rawEmail, ip);
 
   if (outcome.reason === 'issued' && outcome.tokenForDelivery && outcome.userEmail) {
-    // Build the reset URL from the incoming request origin so dev /
+    // Build the reset URL from the server-side canonical APP_URL (or
-    // prod both work without extra config.
+    // first ALLOWED_ORIGINS entry) — never from request headers. A
-    const origin = (req.headers['origin'] as string | undefined)
+    // crafted `Origin` / `Host` / `Referer` would otherwise put an
-      || (req.headers['referer'] ? new URL(req.headers['referer'] as string).origin : undefined)
+    // attacker-controlled domain into the emailed reset link while the
-      || `${req.protocol}://${req.get('host')}`;
+    // token itself is still legitimate.
+    const origin = getAppUrl();
     const url = `${origin.replace(/\/$/, '')}/reset-password?token=${encodeURIComponent(outcome.tokenForDelivery)}`;
 
     // Audit the REQUEST always — even for "no user" — so abuse is visible.
@@ -231,8 +232,8 @@ router.get('/me', authenticate, (req: Request, res: Response) => {
   res.json({ user });
 });
 
-router.post('/logout', (_req: Request, res: Response) => {
+router.post('/logout', (req: Request, res: Response) => {
-  clearAuthCookie(res);
+  clearAuthCookie(res, req);
   res.json({ success: true });
 });
 
@@ -276,15 +277,15 @@ router.get('/me/settings', authenticate, (req: Request, res: Response) => {
   res.json({ settings: result.settings });
 });
 
-router.post('/avatar', authenticate, demoUploadBlock, avatarUpload.single('avatar'), (req: Request, res: Response) => {
+router.post('/avatar', authenticate, demoUploadBlock, avatarUpload.single('avatar'), async (req: Request, res: Response) => {
   const authReq = req as AuthRequest;
   if (!req.file) return res.status(400).json({ error: 'No image uploaded' });
-  res.json(saveAvatar(authReq.user.id, req.file.filename));
+  res.json(await saveAvatar(authReq.user.id, req.file.filename));
 });
 
-router.delete('/avatar', authenticate, (req: Request, res: Response) => {
+router.delete('/avatar', authenticate, async (req: Request, res: Response) => {
   const authReq = req as AuthRequest;
-  res.json(deleteAvatar(authReq.user.id));
+  res.json(await deleteAvatar(authReq.user.id));
 });
 
 router.get('/users', authenticate, (req: Request, res: Response) => {
@@ -329,7 +330,7 @@ router.post('/mfa/verify-login', mfaLimiter, (req: Request, res: Response) => {
   const result = verifyMfaLogin(req.body);
   if (result.error) return res.status(result.status!).json({ error: result.error });
   writeAudit({ userId: result.auditUserId!, action: 'user.login', ip: getClientIp(req), details: { mfa: true } });
-  setAuthCookie(res, result.token!);
+  setAuthCookie(res, result.token!, req);
   res.json({ token: result.token, user: result.user });
 });
 

server/src/routes/collab.ts

@@ -9,6 +9,7 @@ import { validateStringLengths } from '../middleware/validate';
 import { checkPermission } from '../services/permissions';
 import { AuthRequest } from '../types';
 import { db } from '../db/database';
+import { BLOCKED_EXTENSIONS } from '../services/fileService';
 import {
   verifyTripAccess,
   listNotes,
@@ -41,8 +42,10 @@ const noteUpload = multer({
   defParamCharset: 'utf8',
   fileFilter: (_req, file, cb) => {
     const ext = path.extname(file.originalname).toLowerCase();
-    const BLOCKED = ['.svg', '.html', '.htm', '.xml', '.xhtml', '.js', '.jsx', '.ts', '.exe', '.bat', '.sh', '.cmd', '.msi', '.dll', '.com', '.vbs', '.ps1', '.php'];
+    // Share the single BLOCKED_EXTENSIONS list from fileService so
-    if (BLOCKED.includes(ext) || file.mimetype.includes('svg') || file.mimetype.includes('html') || file.mimetype.includes('javascript')) {
+    // executable/script attachments can't sneak in via collab when the
+    // main uploader already rejects them.
+    if (BLOCKED_EXTENSIONS.includes(ext) || file.mimetype.includes('svg') || file.mimetype.includes('html') || file.mimetype.includes('javascript')) {
       const err: Error & { statusCode?: number } = new Error('File type not allowed');
       err.statusCode = 400;
       return cb(err);

server/src/routes/files.ts

@@ -210,7 +210,7 @@ router.post('/:id/restore', authenticate, (req: Request, res: Response) => {
 });
 
 // Permanently delete from trash
-router.delete('/:id/permanent', authenticate, (req: Request, res: Response) => {
+router.delete('/:id/permanent', authenticate, async (req: Request, res: Response) => {
   const authReq = req as AuthRequest;
   const { tripId, id } = req.params;
 
@@ -222,13 +222,13 @@ router.delete('/:id/permanent', authenticate, (req: Request, res: Response) => {
   const file = getDeletedFile(id, tripId);
   if (!file) return res.status(404).json({ error: 'File not found in trash' });
 
-  permanentDeleteFile(file);
+  await permanentDeleteFile(file);
   res.json({ success: true });
   broadcast(tripId, 'file:deleted', { fileId: Number(id) }, req.headers['x-socket-id'] as string);
 });
 
 // Empty entire trash
-router.delete('/trash/empty', authenticate, (req: Request, res: Response) => {
+router.delete('/trash/empty', authenticate, async (req: Request, res: Response) => {
   const authReq = req as AuthRequest;
   const { tripId } = req.params;
 
@@ -237,7 +237,7 @@ router.delete('/trash/empty', authenticate, (req: Request, res: Response) => {
   if (!checkPermission('file_delete', authReq.user.role, trip.user_id, authReq.user.id, trip.user_id !== authReq.user.id))
     return res.status(403).json({ error: 'No permission' });
 
-  const deleted = emptyTrash(tripId);
+  const deleted = await emptyTrash(tripId);
   res.json({ success: true, deleted });
 });
 

server/src/routes/oauth.ts

@@ -205,6 +205,37 @@ oauthPublicRouter.post('/oauth/register', dcrLimiter, (req: Request, res: Respon
   if (redirectUris.length === 0) {
     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 a reverse-DNS
+  // private-use scheme. This rejects `http://evil.example` DCR payloads
+  // 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 §7.1 private-use scheme: must be a reverse-DNS name
+      // (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;
+    }
+  });
+  if (!allowed) {
+    return res.status(400).json({ error: 'invalid_redirect_uri', error_description: 'redirect_uris must be HTTPS, loopback HTTP, or a private custom scheme' });
+  }
 
   const rawName = typeof body.client_name === 'string' ? body.client_name.trim().slice(0, 100) : '';
   const clientName = rawName || 'MCP Client';

server/src/routes/oidc.ts

@@ -9,6 +9,7 @@ import {
   consumeAuthCode,
   exchangeCodeForToken,
   getUserInfo,
+  verifyIdToken,
   findOrCreateUser,
   touchLastLogin,
   generateToken,
@@ -97,10 +98,40 @@ router.get('/callback', async (req: Request, res: Response) => {
       return res.redirect(frontendUrl('/login?oidc_error=token_failed'));
     }
 
+    // Strict id_token verification: signature via JWKS + iss + aud.
+    // Previously only the access_token was used to hit userinfo, so a
+    // compromised provider or MITM could supply a crafted userinfo
+    // response the server would blindly trust. When the id_token is
+    // missing from the token response (non-compliant provider) we still
+    // reject — an Authorization Code flow MUST return one per OIDC Core.
+    if (!tokenData.id_token) {
+      console.error('[OIDC] Token response missing id_token — refusing login');
+      return res.redirect(frontendUrl('/login?oidc_error=no_id_token'));
+    }
+    const idVerify = await verifyIdToken(
+      tokenData.id_token,
+      doc,
+      config.clientId,
+      config.issuer,
+    );
+    if (idVerify.ok !== true) {
+      const reason = 'error' in idVerify ? idVerify.error : 'unknown';
+      console.error('[OIDC] id_token verification failed:', reason);
+      return res.redirect(frontendUrl('/login?oidc_error=id_token_invalid'));
+    }
+
     const userInfo = await getUserInfo(doc.userinfo_endpoint, tokenData.access_token);
     if (!userInfo.email) {
       return res.redirect(frontendUrl('/login?oidc_error=no_email'));
     }
+    // Cross-check: the userinfo response must be for the same subject
+    // the id_token signed. Catches a compromised userinfo endpoint that
+    // speaks for a different principal than the id_token's claim.
+    const tokenSub = idVerify.claims.sub;
+    if (typeof tokenSub === 'string' && userInfo.sub && userInfo.sub !== tokenSub) {
+      console.error('[OIDC] userinfo.sub does not match id_token.sub — refusing login');
+      return res.redirect(frontendUrl('/login?oidc_error=subject_mismatch'));
+    }
 
     const result = findOrCreateUser(userInfo, config, pending.inviteToken);
     if ('error' in result) {
@@ -126,7 +157,7 @@ router.get('/exchange', (req: Request, res: Response) => {
   const result = consumeAuthCode(code);
   if ('error' in result) return res.status(400).json({ error: result.error });
 
-  setAuthCookie(res, result.token);
+  setAuthCookie(res, result.token, req);
   res.json({ token: result.token });
 });
 

server/src/services/authService.ts

@@ -15,7 +15,9 @@ import { decrypt_api_key, maybe_encrypt_api_key, encrypt_api_key } from './apiKe
 import { createEphemeralToken } from './ephemeralTokens';
 import { revokeUserSessions } from '../mcp';
 import { startTripReminders } from '../scheduler';
+import { verifyJwtAndLoadUser } from '../middleware/auth';
 import { User } from '../types';
+import { DEMO_EMAIL_PRIMARY, isDemoEmail } from './demo';
 
 // ---------------------------------------------------------------------------
 // Constants
@@ -175,10 +177,46 @@ export function normalizeBackupCode(input: string): string {
   return String(input || '').toUpperCase().replace(/[^A-Z0-9]/g, '');
 }
 
+// Legacy SHA-256 hex hash. Kept so existing stored hashes (from before
+// the bcrypt migration) can still be verified in `matchBackupCode`
+// without forcing every user to re-enrol their MFA device. New hashes
+// are produced by `hashBackupCodeBcrypt` below.
 export function hashBackupCode(input: string): string {
   return crypto.createHash('sha256').update(normalizeBackupCode(input)).digest('hex');
 }
 
+const BCRYPT_BACKUP_COST = 10;
+
+/**
+ * Hash a backup code with bcrypt for at-rest storage. Backup codes only
+ * have ~40 bits of entropy (8 hex chars) so a plain SHA-256 rainbow
+ * table cracks them in minutes if the DB ever leaks. bcrypt with a
+ * moderate cost raises that cost by ~3-4 orders of magnitude.
+ */
+export function hashBackupCodeBcrypt(input: string): string {
+  return bcrypt.hashSync(normalizeBackupCode(input), BCRYPT_BACKUP_COST);
+}
+
+/**
+ * Constant-time match of a plaintext backup code against a stored hash
+ * in either format (bcrypt or legacy SHA-256 hex). Used by login and
+ * password-reset flows; callers that need to CONSUME the matching
+ * entry should use this to find the index, then splice it out.
+ */
+export function matchBackupCode(plaintext: string, storedHash: string): boolean {
+  if (!storedHash) return false;
+  if (storedHash.startsWith('$2')) {
+    // bcrypt hash — compareSync is constant-time internally.
+    try { return bcrypt.compareSync(normalizeBackupCode(plaintext), storedHash); }
+    catch { return false; }
+  }
+  // Legacy SHA-256 hex. Compare the SHA-256 of the input against the
+  // stored hex with a constant-time comparator so timing can't leak.
+  const candidate = hashBackupCode(plaintext);
+  if (candidate.length !== storedHash.length) return false;
+  return crypto.timingSafeEqual(Buffer.from(candidate), Buffer.from(storedHash));
+}
+
 export function generateBackupCodes(count = MFA_BACKUP_CODE_COUNT): string[] {
   const codes: string[] = [];
   while (codes.length < count) {
@@ -260,7 +298,7 @@ export function getAppConfig(authenticatedUser: { id: number } | null) {
     require_mfa: requireMfaRow?.value === 'true',
     allowed_file_types: (db.prepare("SELECT value FROM app_settings WHERE key = 'allowed_file_types'").get() as { value: string } | undefined)?.value || 'jpg,jpeg,png,gif,webp,heic,pdf,doc,docx,xls,xlsx,txt,csv',
     demo_mode: isDemo,
-    demo_email: isDemo ? 'demo@trek.app' : undefined,
+    demo_email: isDemo ? DEMO_EMAIL_PRIMARY : undefined,
     demo_password: isDemo ? 'demo12345' : undefined,
     timezone: process.env.TZ || Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC',
     notification_channel: notifChannel,
@@ -283,7 +321,7 @@ export function demoLogin(): { error?: string; status?: number; token?: string;
   if (process.env.DEMO_MODE !== 'true') {
     return { error: 'Not found', status: 404 };
   }
-  const user = db.prepare('SELECT * FROM users WHERE email = ?').get('demo@trek.app') as User | undefined;
+  const user = db.prepare('SELECT * FROM users WHERE email = ?').get(DEMO_EMAIL_PRIMARY) as User | undefined;
   if (!user) return { error: 'Demo user not found', status: 500 };
   const token = generateToken(user);
   const safe = stripUserForClient(user) as Record<string, unknown>;
@@ -458,7 +496,7 @@ export function changePassword(
   if (isOidcOnlyMode()) {
     return { error: 'Password authentication is disabled.', status: 403 };
   }
-  if (process.env.DEMO_MODE === 'true' && userEmail === 'demo@trek.app') {
+  if (process.env.DEMO_MODE === 'true' && isDemoEmail(userEmail)) {
     return { error: 'Password change is disabled in demo mode.', status: 403 };
   }
 
@@ -480,7 +518,7 @@ export function changePassword(
 }
 
 export function deleteAccount(userId: number, userEmail: string, userRole: string): { error?: string; status?: number; success?: boolean } {
-  if (process.env.DEMO_MODE === 'true' && userEmail === 'demo@trek.app') {
+  if (process.env.DEMO_MODE === 'true' && isDemoEmail(userEmail)) {
     return { error: 'Account deletion is disabled in demo mode.', status: 403 };
   }
   if (userRole === 'admin') {
@@ -600,11 +638,13 @@ export function getSettings(userId: number): { error?: string; status?: number;
 // Avatar
 // ---------------------------------------------------------------------------
 
-export function saveAvatar(userId: number, filename: string) {
+export async function saveAvatar(userId: number, filename: string) {
   const current = db.prepare('SELECT avatar FROM users WHERE id = ?').get(userId) as { avatar: string | null } | undefined;
   if (current && current.avatar) {
+    // Fire-and-forget: leftover files are harmless; the DB update is
+    // the source of truth for which avatar is current.
     const oldPath = path.join(avatarDir, current.avatar);
-    if (fs.existsSync(oldPath)) fs.unlinkSync(oldPath);
+    await fs.promises.rm(oldPath, { force: true }).catch(() => {});
   }
 
   db.prepare('UPDATE users SET avatar = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?').run(filename, userId);
@@ -613,11 +653,11 @@ export function saveAvatar(userId: number, filename: string) {
   return { success: true, avatar_url: avatarUrl(updated || {}) };
 }
 
-export function deleteAvatar(userId: number) {
+export async function deleteAvatar(userId: number) {
   const current = db.prepare('SELECT avatar FROM users WHERE id = ?').get(userId) as { avatar: string | null } | undefined;
   if (current && current.avatar) {
     const filePath = path.join(avatarDir, current.avatar);
-    if (fs.existsSync(filePath)) fs.unlinkSync(filePath);
+    await fs.promises.rm(filePath, { force: true }).catch(() => {});
   }
   db.prepare('UPDATE users SET avatar = NULL, updated_at = CURRENT_TIMESTAMP WHERE id = ?').run(userId);
   return { success: true };
@@ -865,7 +905,7 @@ export function getTravelStats(userId: number) {
 // ---------------------------------------------------------------------------
 
 export function setupMfa(userId: number, userEmail: string): { error?: string; status?: number; secret?: string; otpauth_url?: string; qrPromise?: Promise<string> } {
-  if (process.env.DEMO_MODE === 'true' && userEmail === 'demo@nomad.app') {
+  if (process.env.DEMO_MODE === 'true' && isDemoEmail(userEmail)) {
     return { error: 'MFA is not available in demo mode.', status: 403 };
   }
   const row = db.prepare('SELECT mfa_enabled FROM users WHERE id = ?').get(userId) as { mfa_enabled: number } | undefined;
@@ -898,7 +938,7 @@ export function enableMfa(userId: number, code?: string): { error?: string; stat
     return { error: 'Invalid verification code', status: 401 };
   }
   const backupCodes = generateBackupCodes();
-  const backupHashes = backupCodes.map(hashBackupCode);
+  const backupHashes = backupCodes.map(hashBackupCodeBcrypt);
   const enc = encryptMfaSecret(pending);
   db.prepare('UPDATE users SET mfa_enabled = 1, mfa_secret = ?, mfa_backup_codes = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?').run(
     enc,
@@ -914,7 +954,7 @@ export function disableMfa(
   userEmail: string,
   body: { password?: string; code?: string }
 ): { error?: string; status?: number; success?: boolean; mfa_enabled?: boolean } {
-  if (process.env.DEMO_MODE === 'true' && userEmail === 'demo@nomad.app') {
+  if (process.env.DEMO_MODE === 'true' && isDemoEmail(userEmail)) {
     return { error: 'MFA cannot be changed in demo mode.', status: 403 };
   }
   const policy = db.prepare("SELECT value FROM app_settings WHERE key = 'require_mfa'").get() as { value: string } | undefined;
@@ -973,8 +1013,9 @@ export function verifyMfaLogin(body: {
     const okTotp = authenticator.verify({ token: tokenStr.replace(/\s/g, ''), secret });
     if (!okTotp) {
       const hashes = parseBackupCodeHashes(user.mfa_backup_codes);
-      const candidateHash = hashBackupCode(tokenStr);
+      // matchBackupCode handles both bcrypt and legacy SHA-256 hashes;
-      const idx = hashes.findIndex(h => h === candidateHash);
+      // any store older than the bcrypt migration keeps working.
+      const idx = hashes.findIndex((h) => matchBackupCode(tokenStr, h));
       if (idx === -1) {
         return { error: 'Invalid verification code', status: 401 };
       }
@@ -1166,8 +1207,7 @@ export function resetPassword(body: {
     const okTotp = authenticator.verify({ token: supplied.replace(/\s/g, ''), secret });
     if (!okTotp) {
       const hashes = parseBackupCodeHashes(user.mfa_backup_codes);
-      const candidateHash = hashBackupCode(supplied);
+      const idx = hashes.findIndex((h) => matchBackupCode(supplied, h));
-      const idx = hashes.findIndex(h => h === candidateHash);
       if (idx === -1) return { error: 'Invalid MFA code', status: 401 };
       backupCodeConsumedIndex = idx;
     }
@@ -1193,6 +1233,16 @@ export function resetPassword(body: {
       hashes.splice(backupCodeConsumedIndex, 1);
       db.prepare('UPDATE users SET mfa_backup_codes = ? WHERE id = ?').run(JSON.stringify(hashes), user.id);
     }
+    // Revoke every other credential class the user had. The
+    // password_version bump alone invalidates JWT cookie sessions, but
+    // MCP static tokens and OAuth 2.1 bearer tokens are separate stores
+    // that survive the bump unless we prune them here.
+    db.prepare('DELETE FROM mcp_tokens WHERE user_id = ?').run(user.id);
+    try {
+      db.prepare(
+        "UPDATE oauth_tokens SET revoked_at = CURRENT_TIMESTAMP WHERE user_id = ? AND revoked_at IS NULL"
+      ).run(user.id);
+    } catch { /* oauth_tokens table may not exist in very old installs */ }
   })();
 
   // Kick off any MCP/WS session cleanup — same hook the account-delete path uses.
@@ -1267,7 +1317,7 @@ export function createResourceToken(userId: number, purpose?: string): { error?:
 export function isDemoUser(userId: number): boolean {
   if (process.env.DEMO_MODE !== 'true') return false;
   const user = db.prepare('SELECT email FROM users WHERE id = ?').get(userId) as { email: string } | undefined;
-  return user?.email === 'demo@nomad.app';
+  return isDemoEmail(user?.email);
 }
 
 export function verifyMcpToken(rawToken: string): User | null {
@@ -1285,12 +1335,15 @@ export function verifyMcpToken(rawToken: string): User | null {
   return null;
 }
 
+/**
+ * Verify a JWT the same way `middleware/auth.ts#verifyJwtAndLoadUser`
+ * does — including the `password_version` check — so that stolen tokens
+ * lose access the moment the victim resets their password.
+ *
+ * This is the single entry point every non-cookie JWT verification path
+ * (MCP bearer, WebSocket handshake, file-download query tokens, photo
+ * route) should go through.
+ */
 export function verifyJwtToken(token: string): User | null {
-  try {
+  return verifyJwtAndLoadUser(token);
-    const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number };
-    const user = db.prepare('SELECT id, username, email, role FROM users WHERE id = ?').get(decoded.id) as User | undefined;
-    return user || null;
-  } catch {
-    return null;
-  }
 }

server/src/services/backupService.ts

@@ -5,6 +5,7 @@ import fs from 'fs';
 import Database from 'better-sqlite3';
 import { db, closeDb, reinitialize } from '../db/database';
 import * as scheduler from '../scheduler';
+import { invalidatePermissionsCache } from './permissions';
 
 // ---------------------------------------------------------------------------
 // Paths
@@ -246,6 +247,12 @@ export async function restoreFromZip(zipPath: string): Promise<RestoreResult> {
       }
     } finally {
       reinitialize();
+      // The restored DB has different permission-override rows from
+      // the pre-restore DB, but our process-local permissions cache
+      // still holds the pre-restore state. Any request using a cached
+      // permission would decide against the wrong grants until the
+      // next restart. Dropping the cache forces a fresh read.
+      invalidatePermissionsCache();
     }
 
     fs.rmSync(extractDir, { recursive: true, force: true });
@@ -253,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/cookie.ts

@@ -1,9 +1,32 @@
-import { Response } from 'express';
+import { Request, Response } from 'express';
 
 const COOKIE_NAME = 'trek_session';
 
-export function cookieOptions(clear = false) {
+/**
-  const secure = process.env.COOKIE_SECURE !== 'false' && (process.env.NODE_ENV === 'production' || process.env.FORCE_HTTPS === 'true');
+ * Decide whether the session cookie should carry the `Secure` flag.
+ *
+ * We previously only derived this from `NODE_ENV=production` or
+ * `FORCE_HTTPS=true`. That left behind a common self-host setup:
+ * TREK running behind Traefik / Caddy / Cloudflare Tunnel with
+ * `NODE_ENV=development` locally and no `FORCE_HTTPS` — the cookie
+ * went out without `Secure`, even though the public leg was https.
+ *
+ * Now we also honour `req.secure`, which Express derives from
+ * `X-Forwarded-Proto` once `trust proxy` is set (TREK sets it to `1`
+ * in production automatically). If Express sees the request was TLS
+ * on the outermost hop, the cookie is `Secure`. `COOKIE_SECURE=false`
+ * remains the explicit escape hatch for plain-HTTP LAN testing.
+ */
+export function cookieOptions(clear = false, req?: Request) {
+  if (process.env.COOKIE_SECURE === 'false') {
+    return buildOptions(clear, false);
+  }
+  const envSecure = process.env.NODE_ENV === 'production' || process.env.FORCE_HTTPS === 'true';
+  const requestSecure = req?.secure === true;
+  return buildOptions(clear, envSecure || requestSecure);
+}
+
+function buildOptions(clear: boolean, secure: boolean) {
   return {
     httpOnly: true,
     secure,
@@ -13,10 +36,10 @@ export function cookieOptions(clear = false) {
   };
 }
 
-export function setAuthCookie(res: Response, token: string): void {
+export function setAuthCookie(res: Response, token: string, req?: Request): void {
-  res.cookie(COOKIE_NAME, token, cookieOptions());
+  res.cookie(COOKIE_NAME, token, cookieOptions(false, req));
 }
 
-export function clearAuthCookie(res: Response): void {
+export function clearAuthCookie(res: Response, req?: Request): void {
-  res.clearCookie(COOKIE_NAME, cookieOptions(true));
+  res.clearCookie(COOKIE_NAME, cookieOptions(true, req));
 }

server/src/services/demo.ts

@@ -0,0 +1,24 @@
+// Central registry of demo-user email addresses.
+//
+// Historical: the demo account was seeded as "demo@trek.app" (see
+// authService.demoLogin), but several guards — demoUploadBlock in
+// middleware/auth.ts, the MFA/backup-code bypasses in authService —
+// were still checking the pre-rename "demo@nomad.app" string, so they
+// either never fired or silently diverged between call sites. Routing
+// every check through this constant keeps them aligned.
+
+export const DEMO_EMAIL_PRIMARY = 'demo@trek.app';
+
+/**
+ * All email addresses that should be treated as the demo account.
+ * Includes the historical `demo@nomad.app` identifier so instances that
+ * upgraded in place without resetting the DB still hit demo-mode guards.
+ */
+export const DEMO_EMAILS: ReadonlySet<string> = new Set([
+  DEMO_EMAIL_PRIMARY,
+  'demo@nomad.app',
+]);
+
+export function isDemoEmail(email: string | null | undefined): boolean {
+  return !!email && DEMO_EMAILS.has(email);
+}

server/src/services/fileService.ts

@@ -1,9 +1,8 @@
 import path from 'path';
 import fs from 'fs';
-import jwt from 'jsonwebtoken';
-import { JWT_SECRET } from '../config';
 import { db, canAccessTrip } from '../db/database';
 import { consumeEphemeralToken } from './ephemeralTokens';
+import { verifyJwtAndLoadUser } from '../middleware/auth';
 import { TripFile } from '../types';
 
 // ---------------------------------------------------------------------------
@@ -12,7 +11,18 @@ import { TripFile } from '../types';
 
 export const MAX_FILE_SIZE = 50 * 1024 * 1024; // 50 MB
 export const DEFAULT_ALLOWED_EXTENSIONS = 'jpg,jpeg,png,gif,webp,heic,pdf,doc,docx,xls,xlsx,txt,csv,pkpass';
-export const BLOCKED_EXTENSIONS = ['.svg', '.html', '.htm', '.xml'];
+// Single authoritative blocklist for every file-upload surface (main
+// file manager + collab attachments). When the admin setting
+// `allowed_file_types` is `*`, this list is still enforced so the
+// wildcard doesn't silently admit executables/scripts.
+export const BLOCKED_EXTENSIONS = [
+  // Server-rendered / scripted content that could XSS a viewer
+  '.svg', '.html', '.htm', '.xml', '.xhtml',
+  // Scripts
+  '.js', '.jsx', '.ts', '.tsx', '.mjs', '.cjs', '.php', '.py', '.rb', '.pl',
+  // Executables
+  '.exe', '.bat', '.sh', '.cmd', '.msi', '.dll', '.com', '.vbs', '.ps1', '.app',
+];
 export const filesDir = path.join(__dirname, '../../uploads/files');
 
 // ---------------------------------------------------------------------------
@@ -68,12 +78,12 @@ export function authenticateDownload(bearerToken: string | undefined, queryToken
   }
 
   if (bearerToken) {
-    try {
+    // Use the shared helper so the password_version gate applies here too;
-      const decoded = jwt.verify(bearerToken, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number };
+    // previously this bypassed the check and stolen download tokens stayed
-      return { userId: decoded.id };
+    // valid across a password reset.
-    } catch {
+    const user = verifyJwtAndLoadUser(bearerToken);
-      return { error: 'Invalid or expired token', status: 401 };
+    if (!user) return { error: 'Invalid or expired token', status: 401 };
-    }
+    return { userId: user.id };
   }
 
   const uid = consumeEphemeralToken(queryToken!, 'download');
@@ -193,24 +203,42 @@ export function restoreFile(id: string | number) {
   return formatFile(restored);
 }
 
-export function permanentDeleteFile(file: TripFile) {
+export async function permanentDeleteFile(file: TripFile): Promise<void> {
   const { resolved } = resolveFilePath(file.filename);
-  if (fs.existsSync(resolved)) {
+  // `force: true` swallows ENOENT, replacing the prior existsSync+unlink
-    try { fs.unlinkSync(resolved); } catch (e) { console.error('Error deleting file:', e); }
+  // double-call that blocked the event loop twice per deletion. Only
+  // 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 function emptyTrash(tripId: string | number): number {
+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[];
-  for (const file of trashed) {
+  // 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);
-    if (fs.existsSync(resolved)) {
+    try {
-      try { fs.unlinkSync(resolved); } catch (e) { console.error('Error deleting file:', e); }
+      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);
     }
+  }));
+  if (successfullyUnlinked.length > 0) {
+    const placeholders = successfullyUnlinked.map(() => '?').join(',');
+    db.prepare(`DELETE FROM trip_files WHERE id IN (${placeholders})`).run(...successfullyUnlinked);
   }
-  db.prepare('DELETE FROM trip_files WHERE trip_id = ? AND deleted_at IS NOT NULL').run(tripId);
+  return successfullyUnlinked.length;
-  return trashed.length;
 }
 
 // ---------------------------------------------------------------------------

server/src/services/oauthService.ts

@@ -582,10 +582,16 @@ export function validateAuthorizeRequest(
     return { valid: false, error: 'invalid_redirect_uri', error_description: 'redirect_uri does not match any registered URI' };
   }
 
-  // RFC 8707 resource indicator: if provided, must identify the TREK MCP endpoint exactly
+  // RFC 8707 resource indicator: if provided, must identify the TREK
+  // MCP endpoint exactly. If the client didn't supply `resource`, we
+  // bind the token to the MCP endpoint by default — previously this
+  // left `audience = null`, and the audience-bind check on MCP requests
+  // then treated a null audience as "valid for any resource".
   const mcpResource = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`;
-  const resource = params.resource ? params.resource.replace(/\/+$/, '') : null;
+  const resource = params.resource
-  if (resource !== null && resource !== mcpResource) {
+    ? params.resource.replace(/\/+$/, '')
+    : mcpResource;
+  if (resource !== mcpResource) {
     return { valid: false, error: 'invalid_target', error_description: 'Requested resource must be the TREK MCP endpoint' };
   }
 

server/src/services/oidcService.ts

@@ -14,6 +14,8 @@ export interface OidcDiscoveryDoc {
   authorization_endpoint: string;
   token_endpoint: string;
   userinfo_endpoint: string;
+  jwks_uri?: string;
+  issuer?: string;
   _issuer?: string;
 }
 
@@ -138,6 +140,12 @@ export async function discover(issuer: string, discoveryUrl?: string | null): Pr
   const res = await fetch(url);
   if (!res.ok) throw new Error('Failed to fetch OIDC discovery document');
   const doc = (await res.json()) as OidcDiscoveryDoc;
+  // Validate that the discovery doc's issuer matches the operator-configured
+  // one. A MITM or compromised doc could otherwise supply a crafted issuer
+  // that passes jwt.verify() because we used doc.issuer as the expected value.
+  if (doc.issuer && doc.issuer !== issuer) {
+    throw new Error(`OIDC discovery issuer mismatch: expected "${issuer}", got "${doc.issuer}"`);
+  }
   doc._issuer = url;
   discoveryCache = doc;
   discoveryCacheTime = Date.now();
@@ -221,6 +229,102 @@ export async function getUserInfo(userinfoEndpoint: string, accessToken: string)
   return (await res.json()) as OidcUserInfo;
 }
 
+// ---------------------------------------------------------------------------
+// id_token verification (signature + iss + aud + exp)
+// ---------------------------------------------------------------------------
+
+// 5 minute JWKS cache — short enough to pick up key rotation within a
+// reasonable window, long enough that normal login flow doesn't fetch
+// JWKS on every callback.
+const JWKS_TTL_MS = 5 * 60 * 1000;
+type JwksEntry = { keys: Array<Record<string, unknown>>; fetchedAt: number };
+const jwksCache = new Map<string, JwksEntry>();
+
+async function fetchJwks(jwksUri: string): Promise<Array<Record<string, unknown>>> {
+  const cached = jwksCache.get(jwksUri);
+  if (cached && Date.now() - cached.fetchedAt < JWKS_TTL_MS) return cached.keys;
+  const res = await fetch(jwksUri);
+  if (!res.ok) throw new Error(`JWKS fetch failed: HTTP ${res.status}`);
+  const json = (await res.json()) as { keys?: Array<Record<string, unknown>> };
+  const keys = json.keys ?? [];
+  jwksCache.set(jwksUri, { keys, fetchedAt: Date.now() });
+  return keys;
+}
+
+function base64UrlDecode(input: string): Buffer {
+  const padded = input.replace(/-/g, '+').replace(/_/g, '/') + '='.repeat((4 - (input.length % 4)) % 4);
+  return Buffer.from(padded, 'base64');
+}
+
+/**
+ * Verify an OIDC id_token end-to-end: signature against the provider's
+ * JWKS, issuer match, audience match, and exp/nbf. Does NOT verify a
+ * nonce — the server doesn't currently send one in the auth request;
+ * when that's added, pass the expected nonce here and check `claims.nonce`.
+ *
+ * Returning the claims lets callers cross-check `sub` / `email` against
+ * the userinfo response. A mismatch would mean the provider's userinfo
+ * endpoint is speaking for a different subject than the id_token — a
+ * classic IdP-side compromise signal worth refusing login over.
+ */
+export async function verifyIdToken(
+  idToken: string,
+  doc: OidcDiscoveryDoc,
+  clientId: string,
+  expectedIssuer: string,
+): Promise<{ ok: true; claims: Record<string, unknown> } | { ok: false; error: string }> {
+  if (!doc.jwks_uri) return { ok: false, error: 'no_jwks_uri' };
+  const parts = idToken.split('.');
+  if (parts.length !== 3) return { ok: false, error: 'malformed_token' };
+
+  let header: { kid?: string; alg?: string };
+  try { header = JSON.parse(base64UrlDecode(parts[0]!).toString('utf8')); }
+  catch { return { ok: false, error: 'bad_header' }; }
+
+  const alg = header.alg;
+  if (!alg || !/^(RS256|RS384|RS512|ES256|ES384|ES512|PS256|PS384|PS512)$/.test(alg)) {
+    return { ok: false, error: 'unsupported_alg' };
+  }
+
+  let keys: Array<Record<string, unknown>>;
+  try { keys = await fetchJwks(doc.jwks_uri); }
+  catch (e) { return { ok: false, error: 'jwks_fetch_failed' }; }
+
+  // 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;
+  try {
+    // Node 16+ understands JWK directly; no PEM conversion library needed.
+    // Node's crypto accepts a JWK object directly as `{ key, format: 'jwk' }`.
+    // The type signature isn't strict on our TS config so we cast through any.
+    publicKey = crypto.createPublicKey({ key: jwk as any, format: 'jwk' });
+  } catch {
+    return { ok: false, error: 'key_import_failed' };
+  }
+
+  let claims: Record<string, unknown>;
+  try {
+    const verified = jwt.verify(idToken, publicKey, {
+      algorithms: [alg as jwt.Algorithm],
+      issuer: expectedIssuer,
+      audience: clientId,
+    });
+    claims = typeof verified === 'string' ? {} : (verified as Record<string, unknown>);
+  } catch (err) {
+    const msg = err instanceof Error ? err.message : 'verify_failed';
+    return { ok: false, error: `signature_or_claim_mismatch: ${msg}` };
+  }
+
+  return { ok: true, claims };
+}
+
 // ---------------------------------------------------------------------------
 // Find or create user by OIDC sub / email
 // ---------------------------------------------------------------------------
@@ -286,21 +390,34 @@ export function findOrCreateUser(
   const existing = db.prepare('SELECT id FROM users WHERE LOWER(username) = LOWER(?)').get(username);
   if (existing) username = `${username}_${Date.now() % 10000}`;
 
-  const result = db.prepare(
+  // Atomic registration: if an invite was presented, the increment IS
-    'INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, ?, ?, 0)',
+  // the capacity check — UPDATE matches zero rows the moment another
-  ).run(username, email, hash, role, sub, config.issuer, process.env.APP_VERSION || '0.0.0');
+  // concurrent callback wins the last slot, and the transaction aborts
-
+  // the user INSERT. Without this, two parallel OIDC callbacks could
-  if (validInvite) {
+  // both pass the earlier SELECT-based check and each create a user.
-    const updated = db.prepare(
+  const inviteRaceError = new Error('invite_exhausted');
-      'UPDATE invite_tokens SET used_count = used_count + 1 WHERE id = ? AND (max_uses = 0 OR used_count < max_uses)',
+  try {
-    ).run(validInvite.id);
+    const createUser = db.transaction(() => {
-    if (updated.changes === 0) {
+      if (validInvite) {
-      console.warn(`[OIDC] Invite token ${inviteToken?.slice(0, 8)}... exceeded max_uses (race condition)`);
+        const updated = db.prepare(
+          'UPDATE invite_tokens SET used_count = used_count + 1 WHERE id = ? AND (max_uses = 0 OR used_count < max_uses)',
+        ).run(validInvite.id);
+        if (updated.changes === 0) throw inviteRaceError;
+      }
+      return db.prepare(
+        'INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, ?, ?, 0)',
+      ).run(username, email, hash, role, sub, config.issuer, process.env.APP_VERSION || '0.0.0');
+    });
+    const result = createUser() as { lastInsertRowid: number | bigint };
+    user = { id: Number(result.lastInsertRowid), username, email, role } as User;
+    return { user };
+  } catch (err) {
+    if (err === inviteRaceError) {
+      console.warn(`[OIDC] Invite token ${inviteToken?.slice(0, 8)}... exhausted — concurrent callback won the last slot`);
+      return { error: 'registration_disabled' };
     }
+    throw err;
   }
-
-  user = { id: Number(result.lastInsertRowid), username, email, role } as User;
-  return { user };
 }
 
 // ---------------------------------------------------------------------------

server/src/services/shareService.ts

@@ -44,9 +44,14 @@ export function createOrUpdateShareLink(
     return { token: existing.token, created: false };
   }
 
+  // New share links default to a 90-day TTL. Existing tokens that were
+  // created before the expires_at migration keep NULL here and remain
+  // valid indefinitely until the owner rotates them; that preserves
+  // behaviour for anyone who's already sharing a link.
   const token = crypto.randomBytes(24).toString('base64url');
-  db.prepare('INSERT INTO share_tokens (trip_id, token, created_by, share_map, share_bookings, share_packing, share_budget, share_collab) VALUES (?, ?, ?, ?, ?, ?, ?, ?)')
+  const expiresAt = new Date(Date.now() + 90 * 24 * 60 * 60 * 1000).toISOString();
-    .run(tripId, token, createdBy, share_map ? 1 : 0, share_bookings ? 1 : 0, share_packing ? 1 : 0, share_budget ? 1 : 0, share_collab ? 1 : 0);
+  db.prepare('INSERT INTO share_tokens (trip_id, token, created_by, share_map, share_bookings, share_packing, share_budget, share_collab, expires_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)')
+    .run(tripId, token, createdBy, share_map ? 1 : 0, share_bookings ? 1 : 0, share_packing ? 1 : 0, share_budget ? 1 : 0, share_collab ? 1 : 0, expiresAt);
   return { token, created: true };
 }
 
@@ -79,7 +84,9 @@ export function deleteShareLink(tripId: string): void {
  * permission flags. Returns null if the token is invalid or the trip is gone.
  */
 export function getSharedTripData(token: string): Record<string, any> | null {
-  const shareRow = db.prepare('SELECT * FROM share_tokens WHERE token = ?').get(token) as any;
+  const shareRow = db.prepare(
+    "SELECT * FROM share_tokens WHERE token = ? AND (expires_at IS NULL OR expires_at > datetime('now'))"
+  ).get(token) as any;
   if (!shareRow) return null;
 
   const tripId = shareRow.trip_id;

server/tests/integration/oidc.test.ts

@@ -44,6 +44,11 @@ vi.mock('../../src/services/oidcService', async (importOriginal) => {
     discover: vi.fn(),
     exchangeCodeForToken: vi.fn(),
     getUserInfo: vi.fn(),
+    // Bypass real JWKS fetch + signature verification in tests. Callers
+    // that exercise the security of verifyIdToken should unit-test the
+    // function directly instead; integration tests here focus on the
+    // callback flow, not the crypto.
+    verifyIdToken: vi.fn(),
   };
 });
 
@@ -58,6 +63,7 @@ import * as oidcService from '../../src/services/oidcService';
 const mockDiscover = vi.mocked(oidcService.discover);
 const mockExchangeCode = vi.mocked(oidcService.exchangeCodeForToken);
 const mockGetUserInfo = vi.mocked(oidcService.getUserInfo);
+const mockVerifyIdToken = vi.mocked(oidcService.verifyIdToken);
 
 const MOCK_DISCOVERY_DOC = {
   authorization_endpoint: 'https://oidc.example.com/auth',
@@ -142,9 +148,11 @@ describe('GET /api/auth/oidc/callback', () => {
     mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
     mockExchangeCode.mockResolvedValueOnce({
       access_token: 'test-access-token',
+      id_token: 'fake.id.token',
       _ok: true,
       _status: 200,
     });
+    mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub: 'sub-alice-123' } });
     mockGetUserInfo.mockResolvedValueOnce({
       sub: 'sub-alice-123',
       email: 'alice@example.com',
@@ -162,7 +170,8 @@ describe('GET /api/auth/oidc/callback', () => {
 
   it('OIDC-005: new user gets created when registration is open', async () => {
     mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
-    mockExchangeCode.mockResolvedValueOnce({ access_token: 'new-token', _ok: true, _status: 200 });
+    mockExchangeCode.mockResolvedValueOnce({ access_token: 'new-token', id_token: 'fake.id.token', _ok: true, _status: 200 });
+    mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub: 'sub-newuser-999' } });
     mockGetUserInfo.mockResolvedValueOnce({
       sub: 'sub-newuser-999',
       email: 'newuser@example.com',
@@ -214,6 +223,49 @@ describe('GET /api/auth/oidc/callback', () => {
     expect(res.headers.location).toContain('oidc_error=token_failed');
   });
 
+  it('OIDC-010a: missing id_token in token response → redirects with no_id_token error', async () => {
+    mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
+    mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', _ok: true, _status: 200 }); // no id_token
+
+    const state = oidcService.createState('http://localhost:3001/api/auth/oidc/callback');
+
+    const res = await request(app).get(`/api/auth/oidc/callback?code=anycode&state=${state}`);
+
+    expect(res.status).toBe(302);
+    expect(res.headers.location).toContain('oidc_error=no_id_token');
+  });
+
+  it('OIDC-010b: verifyIdToken failure → redirects with id_token_invalid error', async () => {
+    mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
+    mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', id_token: 'bad.id.token', _ok: true, _status: 200 });
+    mockVerifyIdToken.mockResolvedValueOnce({ ok: false, error: 'signature_or_claim_mismatch: invalid signature' });
+
+    const state = oidcService.createState('http://localhost:3001/api/auth/oidc/callback');
+
+    const res = await request(app).get(`/api/auth/oidc/callback?code=anycode&state=${state}`);
+
+    expect(res.status).toBe(302);
+    expect(res.headers.location).toContain('oidc_error=id_token_invalid');
+  });
+
+  it('OIDC-010c: userinfo.sub does not match id_token.sub → redirects with subject_mismatch error', async () => {
+    mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
+    mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', id_token: 'fake.id.token', _ok: true, _status: 200 });
+    mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub: 'sub-from-token' } });
+    mockGetUserInfo.mockResolvedValueOnce({
+      sub: 'sub-different-from-userinfo',
+      email: 'alice@example.com',
+      name: 'Alice',
+    });
+
+    const state = oidcService.createState('http://localhost:3001/api/auth/oidc/callback');
+
+    const res = await request(app).get(`/api/auth/oidc/callback?code=anycode&state=${state}`);
+
+    expect(res.status).toBe(302);
+    expect(res.headers.location).toContain('oidc_error=subject_mismatch');
+  });
+
   it('OIDC-010: registration disabled for new user → redirects with registration_disabled error', async () => {
     // Need at least one existing user so isFirstUser=false
     createUser(testDb, { email: 'existing@example.com' });
@@ -221,7 +273,8 @@ describe('GET /api/auth/oidc/callback', () => {
     testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('allow_registration', 'false')").run();
 
     mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
-    mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', _ok: true, _status: 200 });
+    mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', id_token: 'fake.id.token', _ok: true, _status: 200 });
+    mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub: 'sub-blocked-user' } });
     mockGetUserInfo.mockResolvedValueOnce({
       sub: 'sub-blocked-user',
       email: 'blocked@example.com',

server/tests/unit/middleware/idempotency.test.ts

@@ -8,12 +8,12 @@ const { rows, dbMock } = vi.hoisted(() => {
     db: {
       prepare: vi.fn((sql: string) => ({
         get: vi.fn((...args: unknown[]) => {
-          const [key, userId] = args;
+          const [key, userId, method, path] = args;
-          return rows[`${key}:${userId}`] ?? undefined;
+          return rows[`${key}:${userId}:${method}:${path}`] ?? undefined;
         }),
         run: vi.fn((...args: unknown[]) => {
-          const [key, userId, , , status_code, response_body] = args as [string, number, string, string, number, string];
+          const [key, userId, method, path, status_code, response_body] = args as [string, number, string, string, number, string];
-          const k = `${key}:${userId}`;
+          const k = `${key}:${userId}:${method}:${path}`;
           if (!rows[k]) rows[k] = { status_code, response_body };
         }),
       })),
@@ -28,8 +28,8 @@ vi.mock('../../../src/db/database', () => dbMock);
 import { applyIdempotency } from '../../../src/middleware/idempotency';
 import type { Request, Response, NextFunction } from 'express';
 
-function makeReq(method = 'POST', headers: Record<string, string> = {}): Request {
+function makeReq(method = 'POST', headers: Record<string, string> = {}, path = '/api/test'): Request {
-  return { method, path: '/api/test', headers } as unknown as Request;
+  return { method, path, headers } as unknown as Request;
 }
 
 function makeRes(statusCode = 200): Response {
@@ -64,8 +64,8 @@ describe('applyIdempotency', () => {
     expect(next).toHaveBeenCalledOnce();
   });
 
-  it('replays cached response when key+user already stored', () => {
+  it('replays cached response when key+user+method+path already stored', () => {
-    rows['cached-key:42'] = { status_code: 201, response_body: JSON.stringify({ id: 99 }) };
+    rows['cached-key:42:POST:/api/test'] = { status_code: 201, response_body: JSON.stringify({ id: 99 }) };
     const req = makeReq('POST', { 'x-idempotency-key': 'cached-key' });
     const res = makeRes();
     const next = vi.fn();
@@ -75,7 +75,7 @@ describe('applyIdempotency', () => {
   });
 
   it('different user same key does NOT replay', () => {
-    rows['cached-key:1'] = { status_code: 200, response_body: JSON.stringify({ ok: true }) };
+    rows['cached-key:1:POST:/api/test'] = { status_code: 200, response_body: JSON.stringify({ ok: true }) };
     const req = makeReq('POST', { 'x-idempotency-key': 'cached-key' });
     const res = makeRes();
     const next = vi.fn();
@@ -83,6 +83,33 @@ describe('applyIdempotency', () => {
     expect(next).toHaveBeenCalledOnce();
   });
 
+  it('same key+user on different path does NOT replay (scoped cache)', () => {
+    // Key 'dual-key' is cached under /api/a but reused against /api/b.
+    // Without the (key, user_id, method, path) scoping, /api/b would
+    // have replayed /api/a's body — a silent cross-endpoint leak.
+    rows['dual-key:7:POST:/api/a'] = { status_code: 200, response_body: JSON.stringify({ from: 'a' }) };
+    const req = makeReq('POST', { 'x-idempotency-key': 'dual-key' }, '/api/b');
+    const res = makeRes();
+    const next = vi.fn(() => {
+      (res.json as ReturnType<typeof vi.fn>)({ from: 'b' });
+    });
+    applyIdempotency(req, res, next, 7);
+    expect(next).toHaveBeenCalledOnce();
+    expect(rows['dual-key:7:POST:/api/b']).toBeDefined();
+    expect(JSON.parse(rows['dual-key:7:POST:/api/b'].response_body)).toEqual({ from: 'b' });
+    // /api/a's row is untouched.
+    expect(JSON.parse(rows['dual-key:7:POST:/api/a'].response_body)).toEqual({ from: 'a' });
+  });
+
+  it('same key+user+path but different method does NOT replay', () => {
+    rows['m-key:3:POST:/api/x'] = { status_code: 201, response_body: JSON.stringify({ m: 'post' }) };
+    const req = makeReq('PATCH', { 'x-idempotency-key': 'm-key' }, '/api/x');
+    const res = makeRes();
+    const next = vi.fn();
+    applyIdempotency(req, res, next, 3);
+    expect(next).toHaveBeenCalledOnce();
+  });
+
   it('stores 2xx response on first execution via wrapped res.json', () => {
     const req = makeReq('POST', { 'x-idempotency-key': 'new-key' });
     const res = makeRes(201);
@@ -92,9 +119,9 @@ describe('applyIdempotency', () => {
     });
     applyIdempotency(req, res, next, 7);
     expect(next).toHaveBeenCalledOnce();
-    expect(rows['new-key:7']).toBeDefined();
+    expect(rows['new-key:7:POST:/api/test']).toBeDefined();
-    expect(rows['new-key:7'].status_code).toBe(201);
+    expect(rows['new-key:7:POST:/api/test'].status_code).toBe(201);
-    expect(JSON.parse(rows['new-key:7'].response_body)).toEqual({ id: 5 });
+    expect(JSON.parse(rows['new-key:7:POST:/api/test'].response_body)).toEqual({ id: 5 });
   });
 
   it('does NOT store 4xx responses', () => {
@@ -104,7 +131,36 @@ describe('applyIdempotency', () => {
       (res.json as ReturnType<typeof vi.fn>)({ error: 'Invalid' });
     });
     applyIdempotency(req, res, next, 3);
-    expect(rows['fail-key:3']).toBeUndefined();
+    expect(rows['fail-key:3:POST:/api/test']).toBeUndefined();
+  });
+
+  it('returns 400 when X-Idempotency-Key exceeds 128 characters', () => {
+    const longKey = 'a'.repeat(129);
+    const req = makeReq('POST', { 'x-idempotency-key': longKey });
+    const res = makeRes();
+    const next = vi.fn();
+    applyIdempotency(req, res, next, 1);
+    expect(next).not.toHaveBeenCalled();
+    expect(res.json as ReturnType<typeof vi.fn>).toHaveBeenCalledWith(
+      expect.objectContaining({ error: expect.stringContaining('128') }),
+    );
+  });
+
+  it('does NOT cache response body exceeding 256 KiB', () => {
+    const req = makeReq('POST', { 'x-idempotency-key': 'big-key' });
+    const res = makeRes(200);
+    const originalJsonSpy = res.json as ReturnType<typeof vi.fn>;
+    const largePayload = { data: 'x'.repeat(256 * 1024 + 1) };
+    const next = vi.fn(() => {
+      // res.json is now the wrapper; calling it exercises the size-cap branch
+      res.json(largePayload);
+    });
+    applyIdempotency(req, res, next, 5);
+    expect(next).toHaveBeenCalledOnce();
+    // Underlying spy was called (response reached the client)
+    expect(originalJsonSpy).toHaveBeenCalledWith(largePayload);
+    // But NOT stored in the idempotency store
+    expect(rows['big-key:5:POST:/api/test']).toBeUndefined();
   });
 
   it('handles PUT, PATCH, and DELETE the same as POST', () => {