From 28701cfc67b304dd407d1fd7db7fc31549848e52 Mon Sep 17 00:00:00 2001 From: 2GT-Rich <107219997+2GT-Rich@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:21:04 -0700 Subject: [PATCH] fix(security): harden 2FA setup, OIDC default role, and rate-limit fallback (v0.17.3-beta) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the actionable findings from an adversarial review: - 2FA enrollment now requires the current password (/totp/setup), so a hijacked session or XSS can't enable TOTP and lock the real user out. SSO-only accounts (no local password) are told to set one first. Frontend collects the password before starting setup. - OIDC config rejects default_role='admin': auto-provisioning unmatched users straight to admin is almost never intended — admin must come from an explicit group → role mapping. - Rate limiter no longer fails fully open on a Redis outage. Redis calls are bounded by an 800ms timeout (the client uses maxRetriesPerRequest:null, so commands would otherwise hang, not throw), and on failure a per-process in-memory sliding-window fallback keeps /login and other limited endpoints throttled. Verified with Redis stopped: attempts throttle (429) instead of hanging or passing unlimited. Findings intentionally not actioned: the "API-token auth is fire-and-forget" report was a false positive (Express awaits next(), which is called inside the promise), and the webhook DB-pool and JWT-revocation items are either overstated or a separate architectural decision. Co-Authored-By: Claude Opus 4.8 --- README.md | 2 +- backend/package.json | 2 +- backend/src/middleware/rateLimitRedis.ts | 64 ++++++++++++++++++++---- backend/src/routes/auth.ts | 18 ++++++- backend/src/routes/oidc.ts | 11 +++- frontend/package.json | 2 +- frontend/src/pages/SettingsPage.tsx | 19 +++++-- frontend/src/services/api.ts | 4 +- frontend/src/version.ts | 2 +- 9 files changed, 103 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index c69fc45..97b467e 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ A self-hosted, full-stack network management platform for MikroTik devices. Monitor, configure, and manage your entire MikroTik infrastructure — routers, switches, and wireless access points — from a single web interface. -![Version](https://img.shields.io/badge/version-0.17.2_Beta-blue) +![Version](https://img.shields.io/badge/version-0.17.3_Beta-blue) ![License](https://img.shields.io/badge/license-AGPLv3-blue) ![Docker](https://img.shields.io/badge/docker-compose-2496ED?logo=docker&logoColor=white) ![TypeScript](https://img.shields.io/badge/TypeScript-5.3-3178C6?logo=typescript&logoColor=white) diff --git a/backend/package.json b/backend/package.json index 1048882..3394b0c 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-backend", - "version": "0.17.2-beta", + "version": "0.17.3-beta", "description": "Mikrotik Network Management Platform - Backend API", "main": "dist/index.js", "scripts": { diff --git a/backend/src/middleware/rateLimitRedis.ts b/backend/src/middleware/rateLimitRedis.ts index 389b17a..6677f7c 100644 --- a/backend/src/middleware/rateLimitRedis.ts +++ b/backend/src/middleware/rateLimitRedis.ts @@ -2,6 +2,40 @@ import { Request, Response, NextFunction } from 'express'; import { redis } from '../config/redis'; import { query } from '../config/database'; +// Per-process sliding-window fallback used ONLY when Redis is unavailable, so +// throttling degrades gracefully (esp. for /login) instead of failing fully +// open exactly when infrastructure is under stress. It's best-effort and not +// shared across replicas, but far better than no limit during a Redis outage. +const memBuckets = new Map(); +let lastSweep = Date.now(); + +// The Redis client uses maxRetriesPerRequest:null, so commands QUEUE (hang) +// rather than throw when Redis is down. Bound each call so an outage fails fast +// into the in-memory fallback instead of stalling the request. +const REDIS_OP_TIMEOUT_MS = 800; +function withTimeout(p: Promise): Promise { + return Promise.race([ + p, + new Promise((_, reject) => setTimeout(() => reject(new Error('redis timeout')), REDIS_OP_TIMEOUT_MS)), + ]); +} + +function memoryLimitExceeded(key: string, windowSec: number, max: number): boolean { + const now = Date.now(); + const windowMs = windowSec * 1000; + if (now - lastSweep > 60_000) { + for (const [k, ts] of memBuckets) { + const kept = ts.filter((t) => now - t < windowMs); + if (kept.length) memBuckets.set(k, kept); else memBuckets.delete(k); + } + lastSweep = now; + } + const recent = (memBuckets.get(key) ?? []).filter((t) => now - t < windowMs); + recent.push(now); + memBuckets.set(key, recent); + return recent.length > max; +} + /** * Fixed-window rate limit using Redis INCR (shared across API replicas). */ @@ -21,19 +55,24 @@ export function rateLimitRedis(options: { const uid = req.user?.userId; const key = `rl:${keyPrefix}:${uid != null ? `u:${uid}` : `ip:${req.ip || 'unknown'}`}`; try { - const n = await redis.incr(key); + const n = await withTimeout(redis.incr(key)); if (n === 1) { - await redis.expire(key, windowSec); + await withTimeout(redis.expire(key, windowSec)); } if (n > max) { - const ttl = await redis.ttl(key); + const ttl = await withTimeout(redis.ttl(key)); const retryAfterSec = Math.max(1, ttl > 0 ? ttl : windowSec); res.setHeader('Retry-After', String(retryAfterSec)); res.status(429).json({ error: 'Too many requests. Please try again later.' }); return; } } catch (e) { - console.warn('[rateLimitRedis] Redis error, allowing request:', (e as Error).message); + console.warn('[rateLimitRedis] Redis error, using in-memory fallback:', (e as Error).message); + if (memoryLimitExceeded(key, windowSec, max)) { + res.setHeader('Retry-After', String(windowSec)); + res.status(429).json({ error: 'Too many requests. Please try again later.' }); + return; + } } next(); }; @@ -68,21 +107,28 @@ export function loginRateLimit(): (req: Request, res: Response, next: NextFuncti return async (req: Request, res: Response, next: NextFunction): Promise => { const ip = req.ip || 'unknown'; const key = `rl:login:ip:${ip}`; + // Resolved outside the try so the fallback can reuse the limits on Redis failure + // (getLoginLimits has its own catch and never throws). + const { windowSec, max } = await getLoginLimits(); try { - const { windowSec, max } = await getLoginLimits(); - const n = await redis.incr(key); + const n = await withTimeout(redis.incr(key)); if (n === 1) { - await redis.expire(key, windowSec); + await withTimeout(redis.expire(key, windowSec)); } if (n > max) { - const ttl = await redis.ttl(key); + const ttl = await withTimeout(redis.ttl(key)); const retryAfterSec = Math.max(1, ttl > 0 ? ttl : windowSec); res.setHeader('Retry-After', String(retryAfterSec)); res.status(429).json({ error: 'Too many login attempts. Please try again later.' }); return; } } catch (e) { - console.warn('[loginRateLimit] Redis error, allowing request:', (e as Error).message); + console.warn('[loginRateLimit] Redis error, using in-memory fallback:', (e as Error).message); + if (memoryLimitExceeded(key, windowSec, max)) { + res.setHeader('Retry-After', String(windowSec)); + res.status(429).json({ error: 'Too many login attempts. Please try again later.' }); + return; + } } next(); }; diff --git a/backend/src/routes/auth.ts b/backend/src/routes/auth.ts index b856645..4c0e5fe 100644 --- a/backend/src/routes/auth.ts +++ b/backend/src/routes/auth.ts @@ -99,8 +99,24 @@ router.post('/totp/verify', rateLimitRedis({ windowSec: 60, max: 5, keyPrefix: ' return res.json({ token, user: { id: user.id, username: user.username, role: user.role } }); }); -// Generate a new TOTP secret + QR code for the current user (does not enable yet) +// Generate a new TOTP secret + QR code for the current user (does not enable yet). +// Requires the current password so a hijacked session can't enroll 2FA and lock +// the legitimate user out. router.post('/totp/setup', requireAuth, async (req: Request, res: Response) => { + const { password } = req.body as { password?: string }; + if (!password) return res.status(400).json({ error: 'Current password is required to set up 2FA' }); + + const account = await queryOne<{ password_hash: string | null }>( + `SELECT password_hash FROM users WHERE id = $1`, + [req.user!.userId] + ); + if (!account?.password_hash) { + return res.status(400).json({ error: 'This account has no local password (SSO). Set a password before enabling 2FA.' }); + } + if (!(await bcrypt.compare(password, account.password_hash))) { + return res.status(401).json({ error: 'Incorrect password' }); + } + const secret = new OTPAuth.Secret({ size: 20 }); const totp = new OTPAuth.TOTP({ issuer: 'MikroTik Manager', diff --git a/backend/src/routes/oidc.ts b/backend/src/routes/oidc.ts index 7a31d6c..179dec2 100644 --- a/backend/src/routes/oidc.ts +++ b/backend/src/routes/oidc.ts @@ -86,8 +86,15 @@ router.put('/config', requireAuth, requireAdmin, async (req: Request, res: Respo const body = req.body as Partial & { client_secret?: string }; const validRole = (r: unknown): r is AppRole => APP_ROLES.includes(r as AppRole); - if (body.default_role !== undefined && !validRole(body.default_role)) { - return res.status(400).json({ error: 'Invalid default_role' }); + if (body.default_role !== undefined) { + if (!validRole(body.default_role)) { + return res.status(400).json({ error: 'Invalid default_role' }); + } + // Auto-provisioning strangers straight to admin is almost never intended — + // admin must be granted via an explicit group mapping, not the fallback. + if (body.default_role === 'admin') { + return res.status(400).json({ error: 'default_role cannot be admin — grant admin via an explicit group → role mapping instead' }); + } } if (body.group_role_map !== undefined) { if (typeof body.group_role_map !== 'object' || body.group_role_map === null) { diff --git a/frontend/package.json b/frontend/package.json index 6962123..0af62b7 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-frontend", - "version": "0.17.2-beta", + "version": "0.17.3-beta", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/pages/SettingsPage.tsx b/frontend/src/pages/SettingsPage.tsx index c76034d..1cf23bd 100644 --- a/frontend/src/pages/SettingsPage.tsx +++ b/frontend/src/pages/SettingsPage.tsx @@ -137,6 +137,7 @@ export default function SettingsPage() { // ─── TOTP / 2FA ──────────────────────────────────────────────────────────── const [totpSetupData, setTotpSetupData] = useState<{ secret: string; uri: string; qr: string } | null>(null); const [totpConfirmCode, setTotpConfirmCode] = useState(''); + const [totpSetupPassword, setTotpSetupPassword] = useState(''); const [totpDisablePassword, setTotpDisablePassword] = useState(''); const [totpMsg, setTotpMsg] = useState(''); const [totpError, setTotpError] = useState(''); @@ -148,8 +149,8 @@ export default function SettingsPage() { }); const startTotpSetupMutation = useMutation({ - mutationFn: () => authApi.totpSetup(), - onSuccess: (res) => { setTotpSetupData(res.data); setTotpConfirmCode(''); setTotpError(''); }, + mutationFn: () => authApi.totpSetup(totpSetupPassword), + onSuccess: (res) => { setTotpSetupData(res.data); setTotpConfirmCode(''); setTotpSetupPassword(''); setTotpError(''); }, onError: (err: unknown) => { const msg = (err as { response?: { data?: { error?: string } } })?.response?.data?.error; setTotpError(msg || 'Failed to start TOTP setup'); @@ -1157,14 +1158,26 @@ export default function SettingsPage() { ) : ( /* Not enabled, not in setup */ +
+
+ + setTotpSetupPassword(e.target.value)} + autoComplete="current-password" + /> +
+
)} diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index cfb1499..b2cad56 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -54,8 +54,8 @@ export const authApi = { ), totpVerify: (totp_token: string, code: string) => api.post<{ token: string; user: import('../types').User }>('/auth/totp/verify', { totp_token, code }), - totpSetup: () => - api.post<{ secret: string; uri: string; qr: string }>('/auth/totp/setup'), + totpSetup: (password: string) => + api.post<{ secret: string; uri: string; qr: string }>('/auth/totp/setup', { password }), totpConfirm: (code: string) => api.post<{ ok: boolean }>('/auth/totp/confirm', { code }), totpDisable: (password: string) => diff --git a/frontend/src/version.ts b/frontend/src/version.ts index 682b463..0f2cac6 100644 --- a/frontend/src/version.ts +++ b/frontend/src/version.ts @@ -1 +1 @@ -export const APP_VERSION = 'v0.17.2 Beta'; +export const APP_VERSION = 'v0.17.3 Beta';