mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-19 21:31:46 +00:00
2d0414b4a3
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.
116 lines
4.5 KiB
TypeScript
116 lines
4.5 KiB
TypeScript
import { Request, Response, NextFunction } from 'express';
|
|
import jwt from 'jsonwebtoken';
|
|
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)
|
|
const cookieToken = (req as any).cookies?.trek_session;
|
|
if (cookieToken) return cookieToken;
|
|
const authHeader = req.headers['authorization'];
|
|
return (authHeader && authHeader.split(' ')[1]) || 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(
|
|
'SELECT id, username, email, role, password_version FROM users WHERE id = ?'
|
|
).get(decoded.id) as (User & { password_version?: number }) | undefined;
|
|
if (!row) return null;
|
|
// Session invalidation: any token whose embedded password_version
|
|
// predates the user's current one is rejected. Tokens issued before
|
|
// the `pv` claim existed (decoded.pv === undefined) are treated as
|
|
// version 0 so legacy sessions keep working until the user resets.
|
|
const tokenPv = typeof decoded.pv === 'number' ? decoded.pv : 0;
|
|
const currentPv = typeof row.password_version === 'number' ? row.password_version : 0;
|
|
if (tokenPv !== currentPv) return null;
|
|
// Don't leak password_version beyond the middleware.
|
|
const { password_version: _pv, ...user } = row;
|
|
return user as User;
|
|
} catch {
|
|
return null;
|
|
}
|
|
}
|
|
|
|
const authenticate = (req: Request, res: Response, next: NextFunction): void => {
|
|
const token = extractToken(req);
|
|
|
|
if (!token) {
|
|
res.status(401).json({ error: 'Access token required', code: 'AUTH_REQUIRED' });
|
|
return;
|
|
}
|
|
|
|
const user = verifyJwtAndLoadUser(token);
|
|
if (!user) {
|
|
res.status(401).json({ error: 'Invalid or expired token', code: 'AUTH_REQUIRED' });
|
|
return;
|
|
}
|
|
(req as AuthRequest).user = user;
|
|
applyIdempotency(req, res, next, user.id);
|
|
};
|
|
|
|
/** Like `authenticate` but rejects requests that don't carry an httpOnly session cookie.
|
|
* Used on state-mutating OAuth endpoints (consent POST, client CRUD, session revoke)
|
|
* to prevent Bearer JWT tokens obtained by other means from managing OAuth clients. */
|
|
const requireCookieAuth = (req: Request, res: Response, next: NextFunction): void => {
|
|
const cookieToken = (req as any).cookies?.trek_session;
|
|
if (!cookieToken) {
|
|
res.status(401).json({ error: 'Cookie session required for this endpoint', code: 'COOKIE_AUTH_REQUIRED' });
|
|
return;
|
|
}
|
|
const user = verifyJwtAndLoadUser(cookieToken);
|
|
if (!user) {
|
|
res.status(401).json({ error: 'Invalid or expired session', code: 'AUTH_REQUIRED' });
|
|
return;
|
|
}
|
|
(req as AuthRequest).user = user;
|
|
next();
|
|
};
|
|
|
|
const optionalAuth = (req: Request, res: Response, next: NextFunction): void => {
|
|
const token = extractToken(req);
|
|
|
|
if (!token) {
|
|
(req as OptionalAuthRequest).user = null;
|
|
return next();
|
|
}
|
|
|
|
(req as OptionalAuthRequest).user = verifyJwtAndLoadUser(token) || null;
|
|
next();
|
|
};
|
|
|
|
const adminOnly = (req: Request, res: Response, next: NextFunction): void => {
|
|
const authReq = req as AuthRequest;
|
|
if (!authReq.user || authReq.user.role !== 'admin') {
|
|
res.status(403).json({ error: 'Admin access required' });
|
|
return;
|
|
}
|
|
next();
|
|
};
|
|
|
|
const demoUploadBlock = (req: Request, res: Response, next: NextFunction): void => {
|
|
const authReq = req as AuthRequest;
|
|
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();
|
|
};
|
|
|
|
export { authenticate, requireCookieAuth, optionalAuth, adminOnly, demoUploadBlock };
|