mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-21 22:46:32 +00:00
refactor(server): move the JWT verify into nest/auth, delete four dead middlewares (#1828)
middleware/auth.ts exported seven things. Two of them — extractToken and verifyJwtAndLoadUser — are the canonical session check every auth surface goes through. The other five (authenticate, requireCookieAuth, optionalAuth, adminOnly, demoUploadBlock) are Express middlewares with no production caller left; the guards in nest/auth replaced them and only comments still mention them. The two live functions move to nest/auth/jwt-verify.ts and the file goes, along with middleware/validate.ts (nothing but its own test imported it) and middleware/idempotency.ts (its only caller was `authenticate`, and IdempotencyInterceptor has replaced it). AuthRequest and OptionalAuthRequest go with them — src/typings/express.d.ts is what types req.user now. Eight importers repoint: the three guards, auth/files/plugins-proxy services, platform.routes and the global MFA policy. Two deliberate departures from the plan for this package. JWT_SECRET does not move to app-config. It is not an env value: it is generated into data/.jwt_secret, and the admin panel rotates it at runtime through updateJwtSecret. `export let` is precisely what makes a rotation take effect in-process — a registerAs token would freeze the boot value and silently break rotation. The verify also keeps reading the db singleton rather than DatabaseService. That change is not a one-line swap: it gives the three guards a constructor dependency, which means registering them as providers everywhere they are used. Measured: 38 directories use those guards and 21 of them do not import AuthModule, on top of 34 e2e tests that each build a container around a single domain module. That is its own PR, not a rider on one that deletes dead files. tests/unit/middleware/auth.test.ts covered real behaviour, so it is not simply deleted. Its extractToken cases and the five verify cases it drove through `authenticate` come back as tests/unit/nest/auth/jwt-verify.test.ts, now hitting verifyJwtAndLoadUser directly. Four cases are new: the password_version gate — the whole reason this function exists — had no direct coverage at all. Its three adminOnly cases are dropped because AdminGuard, their successor, is covered by three e2e suites.
This commit is contained in:
Maurice
@@ -1,120 +0,0 @@
|
||||
import { Request, Response, NextFunction } from 'express';
|
||||
import jwt from 'jsonwebtoken';
|
||||
import { readEnv } from '../app-config';
|
||||
import { db } from '../db/database';
|
||||
import { JWT_SECRET } from '../config';
|
||||
import { AuthRequest, OptionalAuthRequest, User } from '../types';
|
||||
import { applyIdempotency } from './idempotency';
|
||||
import { isDemoEmail } from '../nest/common/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; purpose?: string };
|
||||
// Purpose-scoped tokens (e.g. the short-lived mfa_login token) share this
|
||||
// secret but are not full session tokens — only their dedicated endpoint
|
||||
// may accept them, so reject any token carrying a purpose claim here.
|
||||
if (decoded.purpose) return null;
|
||||
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 (readEnv().demo.enabled && 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 };
|
||||
@@ -1,80 +0,0 @@
|
||||
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;
|
||||
response_body: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Called from within `authenticate` after req.user is set.
|
||||
*
|
||||
* For mutating requests carrying X-Idempotency-Key:
|
||||
* - 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 {
|
||||
if (!MUTATING_METHODS.has(req.method)) {
|
||||
next();
|
||||
return;
|
||||
}
|
||||
|
||||
const key = req.headers['x-idempotency-key'] as string | undefined;
|
||||
if (!key) {
|
||||
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 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 = ? 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));
|
||||
return;
|
||||
}
|
||||
|
||||
// Wrap res.json to capture the response on first successful execution
|
||||
const originalJson = res.json.bind(res);
|
||||
res.json = function (body: unknown): Response {
|
||||
if (res.statusCode >= 200 && res.statusCode < 300) {
|
||||
try {
|
||||
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
|
||||
}
|
||||
}
|
||||
return originalJson(body);
|
||||
};
|
||||
|
||||
next();
|
||||
}
|
||||
@@ -1,7 +1,7 @@
|
||||
import { Request, Response, NextFunction } from 'express';
|
||||
import { readEnv } from '../app-config';
|
||||
import { db } from '../db/database';
|
||||
import { extractToken, verifyJwtAndLoadUser } from './auth';
|
||||
import { extractToken, verifyJwtAndLoadUser } from '../nest/auth/jwt-verify';
|
||||
import { DEMO_EMAILS } from '../nest/common/demo';
|
||||
|
||||
/** Paths that never require MFA (public or pre-auth). */
|
||||
|
||||
@@ -1,26 +0,0 @@
|
||||
import { Request, Response, NextFunction } from 'express';
|
||||
|
||||
function maxLength(field: string, max: number) {
|
||||
return (req: Request, res: Response, next: NextFunction): void => {
|
||||
if (req.body[field] && typeof req.body[field] === 'string' && req.body[field].length > max) {
|
||||
res.status(400).json({ error: `${field} must be ${max} characters or less` });
|
||||
return;
|
||||
}
|
||||
next();
|
||||
};
|
||||
}
|
||||
|
||||
function validateStringLengths(maxLengths: Record<string, number>) {
|
||||
return (req: Request, res: Response, next: NextFunction): void => {
|
||||
for (const [field, max] of Object.entries(maxLengths)) {
|
||||
const value = req.body[field];
|
||||
if (value && typeof value === 'string' && value.length > max) {
|
||||
res.status(400).json({ error: `${field} must be ${max} characters or less` });
|
||||
return;
|
||||
}
|
||||
}
|
||||
next();
|
||||
};
|
||||
}
|
||||
|
||||
export { maxLength, validateStringLengths };
|
||||
@@ -71,8 +71,8 @@ import { IdempotencyInterceptor } from './common/idempotency.interceptor';
|
||||
// equivalent of the legacy Express app.get('*') catch-all). @Catch(NotFoundException)
|
||||
// is more specific than TrekExceptionFilter, so Nest routes 404s here.
|
||||
{ provide: APP_FILTER, useClass: SpaFallbackFilter },
|
||||
// Replays the X-Idempotency-Key the client sends on every write, matching
|
||||
// the legacy applyIdempotency middleware so retried mutations don't double-apply.
|
||||
// Replays the X-Idempotency-Key the client sends on every write, so retried
|
||||
// mutations don't double-apply.
|
||||
{ provide: APP_INTERCEPTOR, useClass: IdempotencyInterceptor },
|
||||
// Global Zod validation: any parameter typed with a createZodDto class
|
||||
// (the <domain>.dto.ts wrappers over @trek/shared schemas) is validated;
|
||||
|
||||
@@ -25,7 +25,7 @@ import { startTripReminders } from '../../scheduler';
|
||||
import { deleteUserCompletely } from '../../services/userCleanupService';
|
||||
import { emitUserDeleted } from '../../plugin-user-lifecycle';
|
||||
import { getFlightDistanceKm } from '../../services/distanceService';
|
||||
import { verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { verifyJwtAndLoadUser } from './jwt-verify';
|
||||
import { User } from '../../types';
|
||||
import { DEMO_EMAIL_PRIMARY, isDemoEmail } from '../common/demo';
|
||||
import { avatarUrl } from '../common/avatarUrl';
|
||||
@@ -1410,7 +1410,7 @@ export class AuthService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Verify a JWT the same way `middleware/auth.ts#verifyJwtAndLoadUser`
|
||||
* Verify a JWT the same way `auth/jwt-verify.ts#verifyJwtAndLoadUser`
|
||||
* does — including the `password_version` check — so that stolen tokens
|
||||
* lose access the moment the victim resets their password.
|
||||
*
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { CanActivate, ExecutionContext, HttpException, Injectable } from '@nestjs/common';
|
||||
import type { Request } from 'express';
|
||||
import { verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { verifyJwtAndLoadUser } from './jwt-verify';
|
||||
|
||||
/**
|
||||
* Mirrors the legacy `requireCookieAuth` middleware: accepts ONLY the httpOnly
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { CanActivate, ExecutionContext, HttpException, Injectable } from '@nestjs/common';
|
||||
import type { Request } from 'express';
|
||||
import { extractToken, verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { extractToken, verifyJwtAndLoadUser } from './jwt-verify';
|
||||
|
||||
/**
|
||||
* Validates TREK's existing JWT session — the same httpOnly `trek_session`
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
import type { Request } from 'express';
|
||||
import jwt from 'jsonwebtoken';
|
||||
import { db } from '../../db/database';
|
||||
import { JWT_SECRET } from '../../config';
|
||||
import type { User } from '../../types';
|
||||
|
||||
/**
|
||||
* The canonical JWT session check. Every auth surface goes through here — the
|
||||
* three guards, MCP bearer tokens, the file-download query token, the
|
||||
* photo-serving route and the global MFA policy — so the secret, the
|
||||
* password_version gate and the loaded user cannot drift between them.
|
||||
*
|
||||
* Free functions rather than a provider: the MFA policy in
|
||||
* middleware/mfaPolicy.ts and the platform routes run outside the container and
|
||||
* would have no way to inject one. Note JWT_SECRET is deliberately a live
|
||||
* binding from src/config, not an app-config value: the admin panel rotates it
|
||||
* at runtime and `export let` is what makes a rotation take effect in-process.
|
||||
*/
|
||||
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.
|
||||
*
|
||||
* 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. Several paths used to call `jwt.verify` directly and skip
|
||||
* 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; purpose?: string };
|
||||
// Purpose-scoped tokens (e.g. the short-lived mfa_login token) share this
|
||||
// secret but are not full session tokens — only their dedicated endpoint
|
||||
// may accept them, so reject any token carrying a purpose claim here.
|
||||
if (decoded.purpose) return null;
|
||||
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 verify.
|
||||
const { password_version: _pv, ...user } = row;
|
||||
return user as User;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
@@ -1,6 +1,6 @@
|
||||
import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common';
|
||||
import type { Request } from 'express';
|
||||
import { extractToken, verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { extractToken, verifyJwtAndLoadUser } from './jwt-verify';
|
||||
|
||||
/**
|
||||
* Mirrors the legacy `optionalAuth` middleware: populates req.user with the
|
||||
|
||||
@@ -2,7 +2,8 @@
|
||||
//
|
||||
// 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 —
|
||||
// the upload block that used to live 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.
|
||||
|
||||
@@ -4,9 +4,8 @@ import { Observable, of } from 'rxjs';
|
||||
import { DatabaseService } from '../database/database.service';
|
||||
|
||||
/**
|
||||
* Nest counterpart of the legacy `applyIdempotency` middleware
|
||||
* (server/src/middleware/idempotency.ts), which the Express `authenticate`
|
||||
* middleware runs on every authenticated request.
|
||||
* Replaces the `applyIdempotency` middleware the Express `authenticate` ran on
|
||||
* every authenticated request. Both are gone; this is the only implementation.
|
||||
*
|
||||
* The TREK client attaches an `X-Idempotency-Key` to ALL write operations (see
|
||||
* client/src/api/client.ts) and the offline sync queue replays mutations with
|
||||
|
||||
@@ -7,7 +7,7 @@ import { RealtimeService } from '../realtime/realtime.service';
|
||||
import { PermissionsService } from '../permissions/permissions.service';
|
||||
import { avatarUrl } from '../common/avatarUrl';
|
||||
import { consumeEphemeralToken } from '../../services/ephemeralTokens';
|
||||
import { verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { verifyJwtAndLoadUser } from '../auth/jwt-verify';
|
||||
import type { User, TripFile } from '../../types';
|
||||
import { DatabaseService, type TripAccess } from '../database/database.service';
|
||||
import { DEFAULT_ALLOWED_EXTENSIONS, filesDir } from './files.constants';
|
||||
|
||||
@@ -3,7 +3,7 @@ import path from 'node:path';
|
||||
import fs from 'node:fs';
|
||||
|
||||
import { readEnv } from '../../app-config';
|
||||
import { verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { verifyJwtAndLoadUser } from '../auth/jwt-verify';
|
||||
import { db } from '../../db/database';
|
||||
import { mcpHandler } from '../../mcp';
|
||||
import { trekOAuthProvider, trekClientsStore } from '../../mcp/oauthProvider';
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { All, Controller, Param, Req, Res } from '@nestjs/common';
|
||||
import type { Request, Response } from 'express';
|
||||
import { extractToken, verifyJwtAndLoadUser } from '../../middleware/auth';
|
||||
import { extractToken, verifyJwtAndLoadUser } from '../auth/jwt-verify';
|
||||
import { pluginsEnabled } from './kill-switch';
|
||||
import { PluginRuntimeService } from './plugin-runtime.service';
|
||||
|
||||
|
||||
@@ -307,15 +307,6 @@ export interface Setting {
|
||||
value?: string | null;
|
||||
}
|
||||
|
||||
export interface AuthRequest extends Request {
|
||||
user: User;
|
||||
trip?: { id: number; user_id: number };
|
||||
}
|
||||
|
||||
export interface OptionalAuthRequest extends Request {
|
||||
user: User | null;
|
||||
}
|
||||
|
||||
export interface AssignmentRow extends DayAssignment {
|
||||
place_name: string;
|
||||
place_description: string | null;
|
||||
|
||||
Reference in New Issue
Block a user