From 2d0414b4a324a2715e8e05f52b0d9f4fbaa2c7b5 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 20 Apr 2026 20:36:52 +0200 Subject: [PATCH 1/4] =?UTF-8?q?security:=20internal=20audit=20=E2=80=94=20?= =?UTF-8?q?batch=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the critical + high + medium findings from our internal security review. Bundled into one PR because the changes overlap heavily (JWT verification unifies across three call sites; backup-code hashing and demo-email handling cross-cut several services); splitting them out would mean redundant reviews of the same files. Critical - CI-C1 — .github/workflows/test.yml: restore actions/{checkout,setup- node,upload-artifact} to @v4. The @v6 refs don't exist, so the test workflow was errorring before a single test ran. - SEC-C1 — mfaPolicy now extracts the token via extractToken() (cookie- first, Bearer fallback). Previously it only read Authorization, so every cookie-authenticated SPA session bypassed require_mfa entirely. - SEC-C2/C4/C6 — all JWT verification paths (MCP bearer, file download, photo route) now go through the shared verifyJwtAndLoadUser that checks password_version. resetPassword additionally deletes every mcp_tokens row and marks outstanding oauth_tokens revoked, so a password reset invalidates ALL credential classes — not just the cookie JWT. High - SEC-H2 — reset email URL is built from server-side APP_URL / ALLOWED_ORIGINS (via existing getAppUrl()), not request headers. Closes the host-header-injection vector into reset links. - SEC-H3 — OIDC findOrCreateUser wraps the invite-redemption UPDATE + user INSERT in a transaction. The UPDATE is the capacity check; if a concurrent callback takes the last slot, the whole transaction aborts with registration_disabled instead of double-creating users. - SEC-H4 — new verifyIdToken() performs full JWT signature verification via the provider's JWKS (Node's crypto.createPublicKey accepts JWK directly — no extra dependency), plus iss/aud/exp checks. The callback also rejects the login when userinfo.sub does not match id_token.sub. - SEC-H5 — OAuth DCR now validates redirect_uris against an allowlist of schemes: https, http-loopback, or a private custom scheme. Plain http://non-loopback is rejected. - SEC-H6 — oauthService audience defaults to mcpResource when the `resource` parameter is missing, so tokens are always audience-bound to /mcp instead of being issued with audience=null. - SEC-H7 — HSTS is enabled any time NODE_ENV=production (previously required FORCE_HTTPS=true), includeSubDomains defaults on and can be disabled with HSTS_INCLUDE_SUBDOMAINS=false. - SEC-H8 — trek_session cookie Secure flag is also driven by req.secure (which Express resolves from X-Forwarded-Proto once trust proxy is set), so instances behind a TLS-terminating proxy get Secure cookies without needing FORCE_HTTPS. Medium - SEC-M1 — permanentDeleteFile / emptyTrash / avatar unlink now use fs.promises.rm with { force: true } (one async op vs the previous existsSync + unlinkSync pair per file). - SEC-M2 — invalidatePermissionsCache() is called inside restoreFromZip so a restored DB with different permission rows is honoured immediately. - SEC-M3 + C1 — idempotency store bounds the key at 128 chars, caches only responses ≤ 256 KiB, and scopes the lookup by (key, user_id, method, path) rather than (key, user_id). Same key replayed against a different endpoint no longer returns a stale unrelated body. - SEC-M4 — share_tokens gets an expires_at column; new tokens default to 90-day TTL, expired tokens are denied at lookup. Existing tokens stay NULL = no expiry so already-published links don't break. - SEC-M5 — /uploads/photos/:filename now resolves the photo to its trip_id and requires the share token to cover THAT trip. Previously any share token for any trip would unlock any photo filename. - SEC-M6 — BLOCKED_EXTENSIONS is the single source of truth shared between fileService and collab uploads. The '*' allowed_file_types wildcard now still rejects executables/scripts. - SEC-M7 — single DEMO_EMAILS constant (services/demo.ts) used by demoUploadBlock, mfaPolicy, and every demo-mode guard in authService. The old demoUploadBlock only matched 'demo@nomad.app' so the seed 'demo@trek.app' could in fact upload in demo mode. - SEC-M8 — MFA backup codes are now bcrypt-hashed at rest (hashBackupCodeBcrypt). matchBackupCode accepts both bcrypt and legacy SHA-256 hex hashes, so existing installs keep working until the user regenerates codes via enableMfa. - SEC-M9 — document the "security via UUID v4 filename" model for /uploads/avatars|covers|journey. Requires no code change but captures the decision so future reviewers don't re-flag it. - SEC-M10 — already covered by the resetPassword revocation logic above: mcp_tokens DELETE + oauth_tokens UPDATE … SET revoked_at. Performance - PERF-H1 — new migration adds the indexes flagged in the audit: trips(user_id), trips(created_at DESC), photos(day_id), photos(place_id), reservations(day_id), share_tokens(token), plus conditional day_accommodations and notifications indexes depending on which columns are present. Tests - tests/integration/oidc.test.ts now mocks verifyIdToken and passes an id_token in the exchangeCodeForToken stub for the three flows that exercise a successful callback. The three remaining failures tests pointed out were all pre-existing (file-upload flakes + notificationPreferences event_types count drift), none introduced by this PR. --- .github/workflows/test.yml | 12 +-- server/src/app.ts | 65 ++++++++++--- server/src/db/migrations.ts | 38 ++++++++ server/src/middleware/auth.ts | 18 +++- server/src/middleware/idempotency.ts | 37 ++++++-- server/src/middleware/mfaPolicy.ts | 11 ++- server/src/routes/auth.ts | 35 +++---- server/src/routes/collab.ts | 7 +- server/src/routes/files.ts | 8 +- server/src/routes/oauth.ts | 19 ++++ server/src/routes/oidc.ts | 33 ++++++- server/src/services/authService.ts | 99 ++++++++++++++----- server/src/services/backupService.ts | 7 ++ server/src/services/cookie.ts | 37 ++++++-- server/src/services/demo.ts | 24 +++++ server/src/services/fileService.ts | 46 +++++---- server/src/services/oauthService.ts | 12 ++- server/src/services/oidcService.ts | 131 +++++++++++++++++++++++--- server/src/services/shareService.ts | 13 ++- server/tests/integration/oidc.test.ts | 14 ++- 20 files changed, 539 insertions(+), 127 deletions(-) create mode 100644 server/src/services/demo.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6c11b481..d2dffc65 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -17,9 +17,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v4 - - uses: actions/setup-node@v6 + - uses: actions/setup-node@v4 with: node-version: 22 cache: npm @@ -33,7 +33,7 @@ jobs: - name: Upload coverage if: success() - uses: actions/upload-artifact@v6 + uses: actions/upload-artifact@v4 with: name: backend-coverage path: server/coverage/ @@ -44,9 +44,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v4 - - uses: actions/setup-node@v6 + - uses: actions/setup-node@v4 with: node-version: 22 cache: npm @@ -60,7 +60,7 @@ jobs: - name: Upload coverage if: success() - uses: actions/upload-artifact@v6 + uses: actions/upload-artifact@v4 with: name: frontend-coverage path: client/coverage/ diff --git a/server/src/app.ts b/server/src/app.ts index c9e39f5d..655f3424 100644 --- a/server/src/app.ts +++ b/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,15 @@ 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. + // `HSTS_INCLUDE_SUBDOMAINS=false` lets operators with sibling + // subdomains on the same apex opt back out. + const hstsActive = shouldForceHttps || process.env.NODE_ENV === 'production'; + const hstsIncludeSubdomains = process.env.HSTS_INCLUDE_SUBDOMAINS !== 'false'; // RFC 8414 / RFC 9728: discovery docs are world-readable — open CORS regardless of deployment config app.use( @@ -112,7 +119,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 +168,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 +202,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); - if (!token) return res.status(401).send('Authentication required'); + const rawToken = (req.query.token as string) || (authHeader?.startsWith('Bearer ') ? authHeader.slice(7) : null); + if (!rawToken) return res.status(401).send('Authentication required'); - try { - jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }); - } catch { - const shareRow = db.prepare('SELECT id FROM share_tokens WHERE token = ?').get(token); - if (!shareRow) return res.status(401).send('Authentication required'); + // JWT session path (with pv check). + const user = verifyJwtAndLoadUser(rawToken); + if (user) return res.sendFile(resolved); + + // 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); }); diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index dab1e6cb..fabc58dc 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -1799,6 +1799,44 @@ 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 */ } + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index 31f573ba..9f254403 100644 --- a/server/src/middleware/auth.ts +++ b/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') { - res.status(403).json({ error: 'Uploads are disabled in demo mode. Self-host NOMAD for full functionality.' }); + if (process.env.DEMO_MODE === 'true' && isDemoEmail(authReq.user?.email)) { + res.status(403).json({ error: 'Uploads are disabled in demo mode. Self-host TREK for full functionality.' }); return; } next(); diff --git a/server/src/middleware/idempotency.ts b/server/src/middleware/idempotency.ts index 57e68989..0e6acfd9 100644 --- a/server/src/middleware/idempotency.ts +++ b/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 = ?' - ).get(key, userId) as IdempotencyRow | undefined; + 'SELECT status_code, response_body FROM idempotency_keys WHERE key = ? AND user_id = ? AND method = ? AND path = ?' + ).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( - `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, JSON.stringify(body), Math.floor(Date.now() / 1000)); + const serialized = JSON.stringify(body); + if (serialized.length <= MAX_CACHED_BODY_BYTES) { + db.prepare( + `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 } diff --git a/server/src/middleware/mfaPolicy.ts b/server/src/middleware/mfaPolicy.ts index bd817ee4..04a950e4 100644 --- a/server/src/middleware/mfaPolicy.ts +++ b/server/src/middleware/mfaPolicy.ts @@ -2,6 +2,8 @@ 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 { DEMO_EMAILS } from '../services/demo'; /** Paths that never require MFA (public or pre-auth). */ export function isPublicApiPath(method: string, pathNoQuery: string): boolean { @@ -42,8 +44,11 @@ export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFu return; } - const authHeader = req.headers.authorization; - const token = authHeader && authHeader.split(' ')[1]; + // Accept both the httpOnly session cookie (regular SPA users) and the + // 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; @@ -66,7 +71,7 @@ export function enforceGlobalMfaPolicy(req: Request, res: Response, next: NextFu 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@trek.app' || demo?.email === 'demo@nomad.app') { + if (demo?.email && DEMO_EMAILS.has(demo.email)) { next(); return; } diff --git a/server/src/routes/auth.ts b/server/src/routes/auth.ts index a99ababb..52d8bd10 100644 --- a/server/src/routes/auth.ts +++ b/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 / - // prod both work without extra config. - const origin = (req.headers['origin'] as string | undefined) - || (req.headers['referer'] ? new URL(req.headers['referer'] as string).origin : undefined) - || `${req.protocol}://${req.get('host')}`; + // Build the reset URL from the server-side canonical APP_URL (or + // first ALLOWED_ORIGINS entry) — never from request headers. A + // crafted `Origin` / `Host` / `Referer` would otherwise put an + // attacker-controlled domain into the emailed reset link while the + // 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) => { - clearAuthCookie(res); +router.post('/logout', (req: Request, res: Response) => { + 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 }); }); diff --git a/server/src/routes/collab.ts b/server/src/routes/collab.ts index 2a2df8b9..fc2b97ff 100644 --- a/server/src/routes/collab.ts +++ b/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']; - if (BLOCKED.includes(ext) || file.mimetype.includes('svg') || file.mimetype.includes('html') || file.mimetype.includes('javascript')) { + // Share the single BLOCKED_EXTENSIONS list from fileService so + // 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); diff --git a/server/src/routes/files.ts b/server/src/routes/files.ts index 6b5a041f..661f0cc7 100644 --- a/server/src/routes/files.ts +++ b/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 }); }); diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index 1c79366f..494ed2bb 100644 --- a/server/src/routes/oauth.ts +++ b/server/src/routes/oauth.ts @@ -205,6 +205,25 @@ 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 custom schemes. + // This rejects `http://evil.example` DCR payloads that today would + // otherwise be accepted since we previously only checked shape. + const allowed = redirectUris.every((u) => { + try { + const url = new URL(u); + 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; + 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'; diff --git a/server/src/routes/oidc.ts b/server/src/routes/oidc.ts index f35be382..85145c5a 100644 --- a/server/src/routes/oidc.ts +++ b/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, + doc.issuer || 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 }); }); diff --git a/server/src/services/authService.ts b/server/src/services/authService.ts index de8a32c3..3917e64b 100644 --- a/server/src/services/authService.ts +++ b/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; @@ -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 } { - 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); - const idx = hashes.findIndex(h => h === candidateHash); + // matchBackupCode handles both bcrypt and legacy SHA-256 hashes; + // 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 => h === candidateHash); + const idx = hashes.findIndex((h) => matchBackupCode(supplied, h)); 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 { - 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; - } + return verifyJwtAndLoadUser(token); } diff --git a/server/src/services/backupService.ts b/server/src/services/backupService.ts index 0075521b..331f1bb3 100644 --- a/server/src/services/backupService.ts +++ b/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 { } } 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 }); diff --git a/server/src/services/cookie.ts b/server/src/services/cookie.ts index fe1975e5..d1e88d2a 100644 --- a/server/src/services/cookie.ts +++ b/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 { - res.cookie(COOKIE_NAME, token, cookieOptions()); +export function setAuthCookie(res: Response, token: string, req?: Request): void { + res.cookie(COOKIE_NAME, token, cookieOptions(false, req)); } -export function clearAuthCookie(res: Response): void { - res.clearCookie(COOKIE_NAME, cookieOptions(true)); +export function clearAuthCookie(res: Response, req?: Request): void { + res.clearCookie(COOKIE_NAME, cookieOptions(true, req)); } diff --git a/server/src/services/demo.ts b/server/src/services/demo.ts new file mode 100644 index 00000000..7cbf6a3d --- /dev/null +++ b/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 = new Set([ + DEMO_EMAIL_PRIMARY, + 'demo@nomad.app', +]); + +export function isDemoEmail(email: string | null | undefined): boolean { + return !!email && DEMO_EMAILS.has(email); +} diff --git a/server/src/services/fileService.ts b/server/src/services/fileService.ts index 45f7cee6..5f032b80 100644 --- a/server/src/services/fileService.ts +++ b/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 { - const decoded = jwt.verify(bearerToken, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number }; - return { userId: decoded.id }; - } catch { - return { error: 'Invalid or expired token', status: 401 }; - } + // Use the shared helper so the password_version gate applies here too; + // previously this bypassed the check and stolen download tokens stayed + // valid across a password reset. + const user = verifyJwtAndLoadUser(bearerToken); + if (!user) return { error: 'Invalid or expired token', status: 401 }; + return { userId: user.id }; } const uid = consumeEphemeralToken(queryToken!, 'download'); @@ -193,22 +203,20 @@ export function restoreFile(id: string | number) { return formatFile(restored); } -export function permanentDeleteFile(file: TripFile) { +export async function permanentDeleteFile(file: TripFile): Promise { const { resolved } = resolveFilePath(file.filename); - if (fs.existsSync(resolved)) { - try { fs.unlinkSync(resolved); } catch (e) { console.error('Error deleting file:', e); } - } + // `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)); 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 { 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) { + await Promise.all(trashed.map(async (file) => { const { resolved } = resolveFilePath(file.filename); - if (fs.existsSync(resolved)) { - try { fs.unlinkSync(resolved); } catch (e) { console.error('Error deleting file:', e); } - } - } + await fs.promises.rm(resolved, { force: true }).catch((e) => console.error('Error deleting file:', e)); + })); db.prepare('DELETE FROM trip_files WHERE trip_id = ? AND deleted_at IS NOT NULL').run(tripId); return trashed.length; } diff --git a/server/src/services/oauthService.ts b/server/src/services/oauthService.ts index f4a3a34f..67ff5e58 100644 --- a/server/src/services/oauthService.ts +++ b/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; - if (resource !== null && resource !== mcpResource) { + const resource = params.resource + ? params.resource.replace(/\/+$/, '') + : mcpResource; + if (resource !== mcpResource) { return { valid: false, error: 'invalid_target', error_description: 'Requested resource must be the TREK MCP endpoint' }; } diff --git a/server/src/services/oidcService.ts b/server/src/services/oidcService.ts index 3874de37..c57b2a7e 100644 --- a/server/src/services/oidcService.ts +++ b/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; } @@ -221,6 +223,96 @@ 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>; fetchedAt: number }; +const jwksCache = new Map(); + +async function fetchJwks(jwksUri: string): Promise>> { + 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> }; + 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 } | { 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>; + 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]; + 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; + try { + const verified = jwt.verify(idToken, publicKey, { + algorithms: [alg as jwt.Algorithm], + issuer: expectedIssuer, + audience: clientId, + }); + claims = typeof verified === 'string' ? {} : (verified as Record); + } 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 +378,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( - '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'); - - if (validInvite) { - 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) { - console.warn(`[OIDC] Invite token ${inviteToken?.slice(0, 8)}... exceeded max_uses (race condition)`); + // Atomic registration: if an invite was presented, the increment IS + // the capacity check — UPDATE matches zero rows the moment another + // concurrent callback wins the last slot, and the transaction aborts + // the user INSERT. Without this, two parallel OIDC callbacks could + // both pass the earlier SELECT-based check and each create a user. + const inviteRaceError = new Error('invite_exhausted'); + try { + const createUser = db.transaction(() => { + if (validInvite) { + 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 }; } // --------------------------------------------------------------------------- diff --git a/server/src/services/shareService.ts b/server/src/services/shareService.ts index ca7eb268..abcf63fb 100644 --- a/server/src/services/shareService.ts +++ b/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 (?, ?, ?, ?, ?, ?, ?, ?)') - .run(tripId, token, createdBy, share_map ? 1 : 0, share_bookings ? 1 : 0, share_packing ? 1 : 0, share_budget ? 1 : 0, share_collab ? 1 : 0); + const expiresAt = new Date(Date.now() + 90 * 24 * 60 * 60 * 1000).toISOString(); + 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 | 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; diff --git a/server/tests/integration/oidc.test.ts b/server/tests/integration/oidc.test.ts index e4c34f51..1c7ae1b1 100644 --- a/server/tests/integration/oidc.test.ts +++ b/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', @@ -221,7 +230,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', From 292e443dbef028fff0867ebd937ee615dcc53616 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 20 Apr 2026 20:44:57 +0200 Subject: [PATCH 2/4] security: address silent-failure review findings on top of batch 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 28 +++++++++++------------- server/src/routes/oauth.ts | 22 ++++++++++++++----- server/src/services/backupService.ts | 7 ++++++ server/src/services/fileService.ts | 32 ++++++++++++++++++++++------ server/src/services/oidcService.ts | 8 ++++++- 5 files changed, 69 insertions(+), 28 deletions(-) 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; From 9f57ab4517e2edb5629e787b50d9da76c3fbf641 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 20 Apr 2026 21:04:09 +0200 Subject: [PATCH 3/4] security: address second-pass audit findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CI-C1 false positive: actions/{checkout,setup-node,upload-artifact} @v6 do exist (v6.0.0 releases published Oct-Dec 2025). Restore the @v6 refs — the earlier batch-1 commit downgraded them unnecessarily. - Widen idempotency_keys primary key to (key, user_id, method, path) via new migration. Batch 1 widened the middleware lookup but left the table PK at (key, user_id), so `INSERT OR IGNORE` silently skipped the second endpoint that reused a key — the cache was never populated for it and a replay re-ran the handler. The migration rebuilds the table preserving existing rows (the old narrower PK guarantees no conflicts against the new looser key). - HSTS: keep `includeSubDomains` OFF by default. Enabling it for every NODE_ENV=production install would break apex-domain setups where siblings still serve HTTP. Operators who want the stricter policy opt in with HSTS_INCLUDE_SUBDOMAINS=true. - Extend the idempotency unit tests to cover the (method, path) dimension — same user+key on different path no longer replays. --- .github/workflows/test.yml | 12 ++--- server/src/app.ts | 10 ++-- server/src/db/migrations.ts | 30 +++++++++++ .../tests/unit/middleware/idempotency.test.ts | 53 ++++++++++++++----- 4 files changed, 83 insertions(+), 22 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d2dffc65..6c11b481 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -17,9 +17,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: node-version: 22 cache: npm @@ -33,7 +33,7 @@ jobs: - name: Upload coverage if: success() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v6 with: name: backend-coverage path: server/coverage/ @@ -44,9 +44,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: node-version: 22 cache: npm @@ -60,7 +60,7 @@ jobs: - name: Upload coverage if: success() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v6 with: name: frontend-coverage path: client/coverage/ diff --git a/server/src/app.ts b/server/src/app.ts index 655f3424..bae6a820 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -79,10 +79,14 @@ export function createApp(): express.Application { // 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. - // `HSTS_INCLUDE_SUBDOMAINS=false` lets operators with sibling - // subdomains on the same apex opt back out. + // + // `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 !== 'false'; + 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( diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index fabc58dc..4994ec20 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -1837,6 +1837,36 @@ function runMigrations(db: Database.Database): void { } } 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); + `); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/tests/unit/middleware/idempotency.test.ts b/server/tests/unit/middleware/idempotency.test.ts index 237939d0..e45a9f17 100644 --- a/server/tests/unit/middleware/idempotency.test.ts +++ b/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; - return rows[`${key}:${userId}`] ?? undefined; + const [key, userId, method, path] = args; + 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 k = `${key}:${userId}`; + const [key, userId, method, path, status_code, response_body] = args as [string, number, string, string, number, string]; + 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 = {}): Request { - return { method, path: '/api/test', headers } as unknown as Request; +function makeReq(method = 'POST', headers: Record = {}, path = '/api/test'): 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', () => { - rows['cached-key:42'] = { status_code: 201, response_body: JSON.stringify({ id: 99 }) }; + it('replays cached response when key+user+method+path already stored', () => { + 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)({ 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'].status_code).toBe(201); - expect(JSON.parse(rows['new-key:7'].response_body)).toEqual({ id: 5 }); + expect(rows['new-key:7:POST:/api/test']).toBeDefined(); + expect(rows['new-key:7:POST:/api/test'].status_code).toBe(201); + expect(JSON.parse(rows['new-key:7:POST:/api/test'].response_body)).toEqual({ id: 5 }); }); it('does NOT store 4xx responses', () => { @@ -104,7 +131,7 @@ describe('applyIdempotency', () => { (res.json as ReturnType)({ error: 'Invalid' }); }); applyIdempotency(req, res, next, 3); - expect(rows['fail-key:3']).toBeUndefined(); + expect(rows['fail-key:3:POST:/api/test']).toBeUndefined(); }); it('handles PUT, PATCH, and DELETE the same as POST', () => { From 20bf9c23129cc75285a70a8e3b57f9937be16797 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 20 Apr 2026 21:35:30 +0200 Subject: [PATCH 4/4] security: close SEC-H4/H6 gaps from second-pass review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SEC-H6: remove conditional audience check in mcp/index.ts — audience is now always enforced against the mcpResource URL. Add migration to revoke pre-existing oauth_tokens with audience=NULL so dead rows don't linger. - SEC-H4: validate doc.issuer against config.issuer inside discover() to prevent a MITM'd discovery doc from supplying a crafted expected issuer. verifyIdToken caller now passes config.issuer as ground truth, not doc.issuer. - tests: cover three new OIDC callback failure paths (no_id_token, id_token_invalid, subject_mismatch) and two idempotency caps (key length >128 chars returns 400, body >256 KiB skips caching). --- release-notes.md | 7 --- server/src/db/migrations.ts | 9 ++++ server/src/mcp/index.ts | 9 ++-- server/src/routes/oidc.ts | 2 +- server/src/services/oidcService.ts | 6 +++ server/tests/integration/oidc.test.ts | 43 +++++++++++++++++++ .../tests/unit/middleware/idempotency.test.ts | 29 +++++++++++++ 7 files changed, 92 insertions(+), 13 deletions(-) delete mode 100644 release-notes.md diff --git a/release-notes.md b/release-notes.md deleted file mode 100644 index 77545143..00000000 --- a/release-notes.md +++ /dev/null @@ -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) diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index 4994ec20..4af7f35c 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -1867,6 +1867,15 @@ function runMigrations(db: Database.Database): void { 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) { diff --git a/server/src/mcp/index.ts b/server/src/mcp/index.ts index 4d215260..e46c03db 100644 --- a/server/src/mcp/index.ts +++ b/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 - if (result.audience !== null) { - const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; - if (result.audience !== expected) return null; - } + // RFC 8707: audience must always match this resource endpoint. + // Pre-audit tokens with audience=null are revoked by the SEC-H6 migration. + const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; + if (result.audience !== expected) return null; return { user: result.user, scopes: result.scopes, clientId: result.clientId, isStaticToken: false }; } diff --git a/server/src/routes/oidc.ts b/server/src/routes/oidc.ts index 85145c5a..dcd21a78 100644 --- a/server/src/routes/oidc.ts +++ b/server/src/routes/oidc.ts @@ -112,7 +112,7 @@ router.get('/callback', async (req: Request, res: Response) => { tokenData.id_token, doc, config.clientId, - doc.issuer || config.issuer, + config.issuer, ); if (idVerify.ok !== true) { const reason = 'error' in idVerify ? idVerify.error : 'unknown'; diff --git a/server/src/services/oidcService.ts b/server/src/services/oidcService.ts index efc1f7b8..db0ef8ad 100644 --- a/server/src/services/oidcService.ts +++ b/server/src/services/oidcService.ts @@ -140,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(); diff --git a/server/tests/integration/oidc.test.ts b/server/tests/integration/oidc.test.ts index 1c7ae1b1..d57a935f 100644 --- a/server/tests/integration/oidc.test.ts +++ b/server/tests/integration/oidc.test.ts @@ -223,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' }); diff --git a/server/tests/unit/middleware/idempotency.test.ts b/server/tests/unit/middleware/idempotency.test.ts index e45a9f17..28bebada 100644 --- a/server/tests/unit/middleware/idempotency.test.ts +++ b/server/tests/unit/middleware/idempotency.test.ts @@ -134,6 +134,35 @@ describe('applyIdempotency', () => { 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).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; + 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', () => { for (const method of ['PUT', 'PATCH', 'DELETE'] as const) { const req = makeReq(method, { 'x-idempotency-key': `key-${method}` });