diff --git a/server/src/middleware/mfaPolicy.ts b/server/src/middleware/mfaPolicy.ts index 04a950e4..b253acc0 100644 --- a/server/src/middleware/mfaPolicy.ts +++ b/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 } from './auth'; +import { extractToken, verifyJwtAndLoadUser } 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; - try { - const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; - userId = decoded.id; - } catch { + // Use the shared verify helper so the `password_version` gate applies + // here too — a JWT stolen before a password reset would otherwise + // continue to satisfy this middleware until its natural 24h expiry. + const verified = verifyJwtAndLoadUser(token); + 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') { - const demo = db.prepare('SELECT email FROM users WHERE id = ?').get(userId) as { email: string } | undefined; - if (demo?.email && DEMO_EMAILS.has(demo.email)) { - next(); - return; - } + if (process.env.DEMO_MODE === 'true' && verified.email && DEMO_EMAILS.has(verified.email)) { + next(); + return; } - const row = db.prepare('SELECT mfa_enabled, role FROM users WHERE id = ?').get(userId) as - | { mfa_enabled: number | boolean; role: string } + const row = db.prepare('SELECT mfa_enabled FROM users WHERE id = ?').get(userId) as + | { mfa_enabled: number | boolean } | undefined; if (!row) { next(); diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index 494ed2bb..8d890faf 100644 --- a/server/src/routes/oauth.ts +++ b/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. - // This rejects `http://evil.example` DCR payloads that today would - // otherwise be accepted since we previously only checked shape. + // 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 custom scheme for native/MCP clients (e.g. "myapp://cb") - if (!/^https?:$/.test(url.protocol) && url.protocol.endsWith(':') && !url.protocol.includes(' ')) 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; diff --git a/server/src/services/backupService.ts b/server/src/services/backupService.ts index 331f1bb3..0fc3409d 100644 --- a/server/src/services/backupService.ts +++ b/server/src/services/backupService.ts @@ -260,6 +260,13 @@ export async function restoreFromZip(zipPath: string): Promise { } 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; } } diff --git a/server/src/services/fileService.ts b/server/src/services/fileService.ts index 5f032b80..58c5fd26 100644 --- a/server/src/services/fileService.ts +++ b/server/src/services/fileService.ts @@ -205,20 +205,40 @@ export function restoreFile(id: string | number) { export async function permanentDeleteFile(file: TripFile): Promise { const { resolved } = resolveFilePath(file.filename); - // `force: true` swallows ENOENT, removing the prior existsSync+unlink - // double-call that blocked the event loop twice per deletion. - await fs.promises.rm(resolved, { force: true }).catch((e) => console.error('Error deleting file:', e)); + // `force: true` swallows ENOENT, replacing the prior existsSync+unlink + // 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 async function emptyTrash(tripId: string | number): Promise { 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); - return trashed.length; + if (successfullyUnlinked.length > 0) { + const placeholders = successfullyUnlinked.map(() => '?').join(','); + db.prepare(`DELETE FROM trip_files WHERE id IN (${placeholders})`).run(...successfullyUnlinked); + } + return successfullyUnlinked.length; } // --------------------------------------------------------------------------- diff --git a/server/src/services/oidcService.ts b/server/src/services/oidcService.ts index c57b2a7e..efc1f7b8 100644 --- a/server/src/services/oidcService.ts +++ b/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;