From 2d0414b4a324a2715e8e05f52b0d9f4fbaa2c7b5 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 20 Apr 2026 20:36:52 +0200 Subject: [PATCH] =?UTF-8?q?security:=20internal=20audit=20=E2=80=94=20batc?= =?UTF-8?q?h=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',