diff --git a/backend/src/db/migrate.ts b/backend/src/db/migrate.ts index 7bb0402..ba6df3a 100644 --- a/backend/src/db/migrate.ts +++ b/backend/src/db/migrate.ts @@ -992,6 +992,41 @@ CREATE TABLE IF NOT EXISTS sites ( -- devices in it. The delete endpoint refuses while devices remain. ALTER TABLE devices ADD COLUMN IF NOT EXISTS site_id INTEGER REFERENCES sites(id); CREATE INDEX IF NOT EXISTS idx_devices_site ON devices(site_id); + +-- Mission 02 (SEC-001): server-side session revocation. +-- +-- JWTs are stateless by design (no per-request DB write), but logout, a +-- password change, disabling a compromised account, or an admin "sign this +-- user out everywhere" action all need existing tokens to stop working +-- without waiting out the 24h expiry. token_version is the mechanism: every +-- signed JWT embeds the version current at sign time; requireAuth rejects a +-- token whose embedded version no longer matches the user's row (see +-- utils/sessionAuth.ts). Bumping the column is the "revoke" action. +ALTER TABLE users ADD COLUMN IF NOT EXISTS token_version INTEGER NOT NULL DEFAULT 0; +-- A deactivated account must stop authenticating immediately, same path as +-- token_version -- both are checked together in one cached lookup. +ALTER TABLE users ADD COLUMN IF NOT EXISTS active BOOLEAN NOT NULL DEFAULT true; + +-- Mission 02 (SEC-002): explicit user <-> site authorization. +-- +-- 'admin' is a global role and bypasses this table entirely (see +-- middleware/site.ts). Every other role must hold an explicit row here for a +-- site to be visible or writable -- no row means no access, not "unscoped". +CREATE TABLE IF NOT EXISTS user_sites ( + user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + site_id INTEGER NOT NULL REFERENCES sites(id) ON DELETE CASCADE, + created_at TIMESTAMPTZ DEFAULT NOW(), + PRIMARY KEY (user_id, site_id) +); +CREATE INDEX IF NOT EXISTS idx_user_sites_site ON user_sites(site_id); + +-- Mission 02 (SEC-003): TLS trust-on-first-use pinning for the RouterOS +-- API-SSL connection (see services/mikrotik/tlsTrust.ts). A device's first +-- successful connection records the peer certificate's fingerprint; every +-- later connection must present the same one, or the connection is refused +-- until an admin explicitly re-trusts it (POST /api/devices/:id/tls/reset). +ALTER TABLE devices ADD COLUMN IF NOT EXISTS tls_fingerprint VARCHAR(95); +ALTER TABLE devices ADD COLUMN IF NOT EXISTS tls_mode VARCHAR(20) NOT NULL DEFAULT 'pinned'; `; const DEFAULT_SETTINGS = [ @@ -1084,6 +1119,31 @@ export async function runMigrations(): Promise { console.log('⚠️ Please change the default password after first login!'); } + // user_sites backfill (SEC-002 migration), run exactly once. An existing + // install had no concept of per-user site access -- every user could see + // every site. Preserve that for accounts that predate this table by + // granting every non-admin user access to every site that already exists + // at the moment this runs. Gated by a settings flag rather than "has zero + // rows" -- a user created *after* the backfill is meant to start with no + // site access until explicitly granted, and would otherwise look + // identical to a pre-existing, never-backfilled user on every later boot. + const backfillFlag = await client.query( + `SELECT 1 FROM app_settings WHERE key = 'user_sites_backfilled_v1'` + ); + if (backfillFlag.rows.length === 0) { + await client.query( + `INSERT INTO user_sites (user_id, site_id) + SELECT u.id, s.id FROM users u CROSS JOIN sites s + WHERE u.role <> 'admin' + ON CONFLICT DO NOTHING` + ); + await client.query( + `INSERT INTO app_settings (key, value) VALUES ('user_sites_backfilled_v1', 'true') + ON CONFLICT (key) DO NOTHING` + ); + console.log('user_sites backfilled for pre-existing non-admin users'); + } + console.log('Database migrations completed successfully'); } finally { client.release(); diff --git a/backend/src/index.ts b/backend/src/index.ts index 804055c..2beeb27 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -28,6 +28,8 @@ import { import { netflowCollector } from './services/netflow/NetflowCollector'; import { verifyToken, type AuthPayload } from './middleware/auth'; import { rateLimitRedis } from './middleware/rateLimitRedis'; +import { getSessionState } from './utils/sessionAuth'; +import { userCanAccessDevice } from './middleware/site'; import { initSecrets } from './utils/secrets'; import { reencryptStaleCredentials } from './db/reencryptCredentials'; import { reconcileStaleGuards } from './services/changeGuard/ChangeGuard'; @@ -160,11 +162,24 @@ terminalNs.use((socket, next) => { if (!token) return next(new Error('No token')); try { const payload = verifyToken(token); - if (!TERMINAL_ROLES.has(payload.role)) { - return next(new Error('Console access denied for this role')); - } - socket.data.user = payload; - next(); + // SEC-001: the terminal namespace authenticates over its own handshake, + // outside the HTTP requireAuth pipeline, so it needs its own session + // revocation check -- otherwise logout/deactivate/revoke-sessions would + // not apply to an already-open or newly-opened console. + getSessionState(payload.userId) + .then((state) => { + if (!state || !state.active || (payload.tokenVersion ?? 0) !== state.tokenVersion) { + next(new Error('Session no longer valid')); + return; + } + if (!TERMINAL_ROLES.has(state.role)) { + next(new Error('Console access denied for this role')); + return; + } + socket.data.user = { ...payload, role: state.role }; + next(); + }) + .catch(() => next(new Error('Could not verify session'))); } catch { next(new Error('Invalid or expired token')); } @@ -189,6 +204,13 @@ terminalNs.on('connection', (socket) => { socket.emit('error', 'Too many terminal sessions started. Please wait a moment and try again.'); return; } + // SEC-002: a restricted user can't open a shell on a device outside + // their authorized sites just by knowing/guessing its id. + if (!(await userCanAccessDevice(user, deviceId))) { + socket.emit('error', 'You do not have access to this device'); + return; + } + const device = await queryOne( `SELECT id, name, ip_address, ssh_port, ssh_username, ssh_password_encrypted, api_username, api_password_encrypted diff --git a/backend/src/middleware/__tests__/auditFindings.test.ts b/backend/src/middleware/__tests__/auditFindings.test.ts index 1c31bc4..9eddb4f 100644 --- a/backend/src/middleware/__tests__/auditFindings.test.ts +++ b/backend/src/middleware/__tests__/auditFindings.test.ts @@ -5,33 +5,16 @@ * asserting an opinion. They are not fixes — see the audit report for * classification, impact and recommendation of each finding. */ -import { signToken, verifyToken, AuthPayload } from '../auth'; +import { AuthPayload } from '../auth'; import { resolveSiteId } from '../../utils/siteScope'; -// ── SEC-001: JWTs are not invalidated by a password/role change ───────────── +// ── SEC-001: FIXED in Mission 02 ───────────────────────────────────────────── // -// signToken/verifyToken never consult the database — a JWT is self-contained -// and valid for the full 24h `expiresIn` window regardless of what happens to -// the account afterward. There is no token version, jti, or revocation list -// consulted anywhere in requireAuth (see middleware/auth.ts). This test proves -// the mechanism: a token minted for a role has no way to be invalidated short -// of rotating JWT_SECRET (which invalidates every session at once). -describe('SEC-001: JWT sessions carry no revocation mechanism', () => { - it('a token remains valid and keeps its original role after being issued, ' + - 'with nothing in verifyToken that could observe a later password/role change', () => { - const payload: AuthPayload = { userId: 42, username: 'ops', role: 'admin' }; - const token = signToken(payload); - - // Simulate time passing / the account changing in the DB — verifyToken - // takes only the token, so it cannot know about that change. - const decoded = verifyToken(token); - - expect(decoded.role).toBe('admin'); - expect(decoded.userId).toBe(42); - // No exp check failure, no DB round-trip: the token is still authoritative - // for up to 24h from signing, whatever happened to the account since. - }); -}); +// This finding ("a JWT could not be revoked before its 24h expiry") is now +// covered by dedicated regression tests in middleware/__tests__/auth.test.ts +// ("SEC-001 regression coverage": revoked-version, deactivated-account, and +// deleted-user cases), backed by utils/sessionAuth.ts and the users.token_version +// / users.active columns. See docs/AUDITORIA_BASE_2GT.md for status. // ── SEC-002: "sites" are a display filter, not an authorization boundary ──── // diff --git a/backend/src/middleware/__tests__/auth.test.ts b/backend/src/middleware/__tests__/auth.test.ts index 9bb8cf1..e88c57b 100644 --- a/backend/src/middleware/__tests__/auth.test.ts +++ b/backend/src/middleware/__tests__/auth.test.ts @@ -1,8 +1,14 @@ +jest.mock('../../config/database'); + import { Request, Response, NextFunction } from 'express'; import jwt from 'jsonwebtoken'; import { signToken, verifyToken, requireAuth, requireAdmin, requireWrite, AuthPayload } from '../auth'; +import { queryOne } from '../../config/database'; +import { _resetSessionCacheForTest } from '../../utils/sessionAuth'; -const TEST_PAYLOAD: AuthPayload = { userId: 1, username: 'alice', role: 'admin' }; +const mockedQueryOne = jest.mocked(queryOne); + +const TEST_PAYLOAD: AuthPayload = { userId: 1, username: 'alice', role: 'admin', tokenVersion: 0 }; // Minimal Express mock helpers function mockReq(authHeader?: string): Request { @@ -16,6 +22,19 @@ function mockRes() { return res; } +/** requireAuth resolves its session check asynchronously; wait for `next`/`res.status`. */ +function waitForAuth(req: Request, res: Response, next: jest.Mock): Promise { + requireAuth(req, res, next as unknown as NextFunction); + return new Promise((resolve) => setImmediate(resolve)); +} + +beforeEach(() => { + jest.clearAllMocks(); + _resetSessionCacheForTest(); + // Default: session is current and active, matching TEST_PAYLOAD. + mockedQueryOne.mockResolvedValue({ token_version: 0, active: true, role: 'admin' } as never); +}); + // ── signToken / verifyToken ───────────────────────────────────────────────── describe('signToken / verifyToken', () => { @@ -25,6 +44,7 @@ describe('signToken / verifyToken', () => { expect(decoded.userId).toBe(TEST_PAYLOAD.userId); expect(decoded.username).toBe(TEST_PAYLOAD.username); expect(decoded.role).toBe(TEST_PAYLOAD.role); + expect(decoded.tokenVersion).toBe(0); }); it('throws on an expired token', () => { @@ -69,16 +89,72 @@ describe('requireAuth', () => { expect(next).not.toHaveBeenCalled(); }); - it('calls next and sets req.user for a valid token', () => { + it('calls next and sets req.user for a valid, current-version token', async () => { const token = signToken(TEST_PAYLOAD); const req = mockReq(`Bearer ${token}`); const res = mockRes(); - const next = jest.fn() as unknown as NextFunction; - requireAuth(req, res, next); + const next = jest.fn(); + await waitForAuth(req, res, next); expect(next).toHaveBeenCalled(); expect(req.user?.userId).toBe(TEST_PAYLOAD.userId); expect(req.user?.role).toBe('admin'); }); + + // ── SEC-001 regression coverage ─────────────────────────────────────────── + + it('rejects a token whose session was revoked (token_version bumped)', async () => { + const token = signToken(TEST_PAYLOAD); // signed with tokenVersion: 0 + mockedQueryOne.mockResolvedValue({ token_version: 1, active: true, role: 'admin' } as never); + + const req = mockReq(`Bearer ${token}`); + const res = mockRes(); + const next = jest.fn(); + await waitForAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + }); + + it('rejects a token for a deactivated account', async () => { + const token = signToken(TEST_PAYLOAD); + mockedQueryOne.mockResolvedValue({ token_version: 0, active: false, role: 'admin' } as never); + + const req = mockReq(`Bearer ${token}`); + const res = mockRes(); + const next = jest.fn(); + await waitForAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + }); + + it('rejects a token for a user that no longer exists', async () => { + const token = signToken(TEST_PAYLOAD); + mockedQueryOne.mockResolvedValue(null as never); + + const req = mockReq(`Bearer ${token}`); + const res = mockRes(); + const next = jest.fn(); + await waitForAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(401); + }); + + it('uses the current DB role rather than the one embedded in the token', async () => { + // Token was signed while the user was an operator; the account has since + // been promoted to admin. The request must see the current role. + const staleToken = signToken({ userId: 1, username: 'alice', role: 'operator', tokenVersion: 0 }); + mockedQueryOne.mockResolvedValue({ token_version: 0, active: true, role: 'admin' } as never); + + const req = mockReq(`Bearer ${staleToken}`); + const res = mockRes(); + const next = jest.fn(); + await waitForAuth(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.user?.role).toBe('admin'); + }); }); // ── requireAdmin ───────────────────────────────────────────────────────────── diff --git a/backend/src/middleware/__tests__/site.test.ts b/backend/src/middleware/__tests__/site.test.ts new file mode 100644 index 0000000..b2339ce --- /dev/null +++ b/backend/src/middleware/__tests__/site.test.ts @@ -0,0 +1,128 @@ +jest.mock('../../config/database'); + +import { Request, Response, NextFunction } from 'express'; +import { query, queryOne } from '../../config/database'; +import { + siteContext, enforceSiteAccess, isDeviceAccessible, unauthorizedDeviceIds, + invalidateUserSiteCache, deviceIdSiteGuard, +} from '../site'; +import type { AuthPayload } from '../auth'; + +const mockedQuery = jest.mocked(query); +const mockedQueryOne = jest.mocked(queryOne); + +function mockReqRes(user: AuthPayload | undefined, siteIdHeader?: string) { + const req = { + headers: siteIdHeader !== undefined ? { 'x-site-id': siteIdHeader } : {}, + query: {}, + user, + } as unknown as Request; + const res = {} as Response; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + return { req, res }; +} + +beforeEach(() => { + jest.clearAllMocks(); + invalidateUserSiteCache(1); + invalidateUserSiteCache(2); +}); + +describe('SEC-002: enforceSiteAccess', () => { + it('leaves admins unrestricted (siteScope mirrors the raw request)', async () => { + const { req, res } = mockReqRes({ userId: 1, username: 'admin', role: 'admin' }, '5'); + siteContext(req, res, (() => {}) as NextFunction); + const next = jest.fn(); + await enforceSiteAccess(req, res, next as unknown as NextFunction); + expect(next).toHaveBeenCalled(); + expect(req.siteScope).toBe(5); + }); + + it('scopes a restricted user with no explicit site to their allowed sites', async () => { + mockedQuery.mockResolvedValueOnce([{ site_id: 2 }, { site_id: 3 }] as never); + const { req, res } = mockReqRes({ userId: 2, username: 'op', role: 'operator' }); + siteContext(req, res, (() => {}) as NextFunction); + const next = jest.fn(); + await enforceSiteAccess(req, res, next as unknown as NextFunction); + expect(next).toHaveBeenCalled(); + expect(req.siteScope).toEqual([2, 3]); + }); + + it('403s a restricted user requesting a site they are not a member of (IDOR)', async () => { + mockedQuery.mockResolvedValueOnce([{ site_id: 2 }] as never); // only site 2 + const { req, res } = mockReqRes({ userId: 2, username: 'op', role: 'operator' }, '99'); + siteContext(req, res, (() => {}) as NextFunction); + const next = jest.fn(); + await enforceSiteAccess(req, res, next as unknown as NextFunction); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('allows a restricted user requesting a site they are a member of', async () => { + mockedQuery.mockResolvedValueOnce([{ site_id: 2 }] as never); + const { req, res } = mockReqRes({ userId: 2, username: 'op', role: 'operator' }, '2'); + siteContext(req, res, (() => {}) as NextFunction); + const next = jest.fn(); + await enforceSiteAccess(req, res, next as unknown as NextFunction); + expect(next).toHaveBeenCalled(); + expect(req.siteScope).toBe(2); + }); + + it('scopes a viewer with zero site grants to nothing, not the whole fleet', async () => { + mockedQuery.mockResolvedValueOnce([] as never); + const { req, res } = mockReqRes({ userId: 2, username: 'v', role: 'viewer' }); + siteContext(req, res, (() => {}) as NextFunction); + const next = jest.fn(); + await enforceSiteAccess(req, res, next as unknown as NextFunction); + expect(next).toHaveBeenCalled(); + expect(req.siteScope).toEqual([]); + }); +}); + +describe('SEC-002: isDeviceAccessible / deviceIdSiteGuard (cross-site device IDOR)', () => { + it('denies a restricted user reaching a device in a site they do not belong to', async () => { + const req = { siteScope: [2, 3], user: { userId: 2, role: 'operator' } } as unknown as Request; + mockedQueryOne.mockResolvedValueOnce({ site_id: 99 } as never); // device belongs to site 99 + await expect(isDeviceAccessible(req, 42)).resolves.toBe(false); + }); + + it('allows a restricted user reaching a device in one of their sites', async () => { + const req = { siteScope: [2, 3], user: { userId: 2, role: 'operator' } } as unknown as Request; + mockedQueryOne.mockResolvedValueOnce({ site_id: 3 } as never); + await expect(isDeviceAccessible(req, 42)).resolves.toBe(true); + }); + + it('never restricts an admin (siteScope null)', async () => { + const req = { siteScope: null, user: { userId: 1, role: 'admin' } } as unknown as Request; + await expect(isDeviceAccessible(req, 42)).resolves.toBe(true); + expect(mockedQueryOne).not.toHaveBeenCalled(); + }); + + it('deviceIdSiteGuard 403s the request end-to-end for a cross-site device id', async () => { + const req = { siteScope: [2] } as unknown as Request; + const res = { status: jest.fn().mockReturnThis(), json: jest.fn() } as unknown as Response; + const next = jest.fn(); + mockedQueryOne.mockResolvedValueOnce({ site_id: 7 } as never); // not in [2] + await deviceIdSiteGuard(req, res, next as unknown as NextFunction, '42'); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); +}); + +describe('SEC-002: unauthorizedDeviceIds (bulk operations)', () => { + it('flags device ids outside the caller scope', async () => { + const req = { siteScope: [2] } as unknown as Request; + mockedQuery.mockResolvedValueOnce([ + { id: 1, site_id: 2 }, + { id: 2, site_id: 9 }, + ] as never); + await expect(unauthorizedDeviceIds(req, [1, 2])).resolves.toEqual([2]); + }); + + it('returns an empty list for an unrestricted (admin) scope', async () => { + const req = { siteScope: null } as unknown as Request; + await expect(unauthorizedDeviceIds(req, [1, 2, 3])).resolves.toEqual([]); + expect(mockedQuery).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/src/middleware/auth.ts b/backend/src/middleware/auth.ts index b9cc989..9e6d69c 100644 --- a/backend/src/middleware/auth.ts +++ b/backend/src/middleware/auth.ts @@ -3,6 +3,7 @@ import jwt from 'jsonwebtoken'; import { createHash } from 'crypto'; import { query } from '../config/database'; import { jwtSigningSecret, jwtVerifierSecrets } from '../utils/secrets'; +import { getSessionState } from '../utils/sessionAuth'; export interface AuthPayload { userId: number; @@ -10,6 +11,13 @@ export interface AuthPayload { role: string; /** Set when the request authenticated with an API token instead of a session. */ tokenAuth?: boolean; + /** + * The user's token_version at the moment this JWT was signed (SEC-001). + * Absent on tokens from before this field existed -- those are treated as + * version 0, which is also where every account's counter starts, so + * pre-existing sessions keep working until the next revoking action. + */ + tokenVersion?: number; } declare global { @@ -90,6 +98,8 @@ export function requireAuth(req: Request, res: Response, next: NextFunction): vo const token = authHeader.slice(7); if (token.startsWith('mtm_')) { + // API tokens are revoked by deleting the row (see routes/automation.ts); + // they carry no JWT-style version to check here. authenticateApiToken(token) .then((payload) => { if (!payload) { res.status(401).json({ error: 'Invalid or expired API token' }); return; } @@ -100,12 +110,41 @@ export function requireAuth(req: Request, res: Response, next: NextFunction): vo return; } + let payload: AuthPayload; try { - req.user = verifyToken(token); - next(); + payload = verifyToken(token); } catch { res.status(401).json({ error: 'Invalid or expired token' }); + return; } + + // SEC-001: reject a token whose embedded session has since been revoked + // (logout-everywhere, password change, TOTP disable, deactivation, or an + // admin ending the session) rather than trusting the signature alone until + // natural expiry. getSessionState is cached for a few seconds, so this adds + // no DB round trip on the common path. + getSessionState(payload.userId) + .then((state) => { + if (!state || !state.active) { + res.status(401).json({ error: 'Session no longer valid' }); + return; + } + if ((payload.tokenVersion ?? 0) !== state.tokenVersion) { + res.status(401).json({ error: 'Session was revoked' }); + return; + } + // The role in the DB is authoritative going forward -- a demotion/ + // promotion takes effect on the next request rather than waiting for + // the token to expire, at the same cached-lookup cost as the version + // check above. + req.user = { ...payload, role: state.role }; + next(); + }) + .catch(() => { + // Fail closed: if we can't confirm the session is still valid, don't + // authenticate the request. + res.status(401).json({ error: 'Could not verify session' }); + }); } export function requireAdmin(req: Request, res: Response, next: NextFunction): void { diff --git a/backend/src/middleware/site.ts b/backend/src/middleware/site.ts index 00b23db..2c54b99 100644 --- a/backend/src/middleware/site.ts +++ b/backend/src/middleware/site.ts @@ -1,12 +1,20 @@ import { Request, Response, NextFunction } from 'express'; -import { resolveSiteId } from '../utils/siteScope'; +import { resolveSiteId, SiteScope } from '../utils/siteScope'; +import { queryOne, query } from '../config/database'; declare global { // eslint-disable-next-line @typescript-eslint/no-namespace namespace Express { interface Request { - /** Active site, or null for the whole fleet (all-sites view). */ + /** Raw site id requested via header/query, before authorization. */ siteId?: number | null; + /** + * The effective, authorized scope for this request (SEC-002): + * `null` = unrestricted (admin/API token), a `number` = one authorized + * site, `number[]` = every site a restricted user may see when no + * explicit site was requested. Set by enforceSiteAccess. + */ + siteScope?: SiteScope; } } } @@ -14,9 +22,11 @@ declare global { /** * Reads the active site from the X-Site-Id header (issue #130). * - * An absent, empty or 'all' header means unscoped. That default is what keeps - * every pre-existing API consumer -- scripts, API tokens, the docs' curl - * examples -- behaving exactly as it did before sites existed. + * An absent, empty or 'all' header means "no explicit selection". That + * default is what keeps every pre-existing API consumer -- scripts, API + * tokens, the docs' curl examples -- behaving exactly as it did before sites + * existed, *for an admin*. For a restricted user it is narrowed to their + * accessible sites by enforceSiteAccess below, not left unscoped. * * A query-string ?site_id= is also honoured, for links and for anything that * cannot set headers. @@ -28,7 +38,168 @@ export function siteContext(req: Request, _res: Response, next: NextFunction): v next(); } -/** Convenience accessor so routes don't repeat the `?? null`. */ -export function activeSite(req: Request): number | null { - return req.siteId ?? null; +// Cached per user for a few seconds -- same trade-off as sessionAuth's cache: +// a grant/revoke of site access takes a few seconds to propagate, in exchange +// for not hitting the DB on every request of every route that scopes by site. +const SITE_CACHE_TTL_MS = 5_000; +const siteCache = new Map(); + +async function getUserSiteIds(userId: number): Promise { + const cached = siteCache.get(userId); + if (cached && Date.now() - cached.cachedAt < SITE_CACHE_TTL_MS) return cached.ids; + const rows = await query<{ site_id: number }>(`SELECT site_id FROM user_sites WHERE user_id = $1`, [userId]); + const ids = rows.map((r) => r.site_id); + siteCache.set(userId, { ids, cachedAt: Date.now() }); + return ids; +} + +/** Test/admin-action seam: force the next read to hit the DB. */ +export function invalidateUserSiteCache(userId: number): void { + siteCache.delete(userId); +} + +/** + * Same authorization rule as isDeviceAccessible, usable outside an Express + * request -- the Socket.IO terminal namespace authenticates over its own + * handshake and has no `req` to hang a scope on. + */ +export async function userCanAccessDevice( + user: { userId: number; role: string; tokenAuth?: boolean }, deviceId: number +): Promise { + if (user.role === 'admin' || user.tokenAuth) return true; + const device = await queryOne<{ site_id: number | null }>(`SELECT site_id FROM devices WHERE id = $1`, [deviceId]); + if (!device || device.site_id === null) return true; // let the caller 404, or unassigned = visible + const allowed = await getUserSiteIds(user.userId); + return allowed.includes(device.site_id); +} + +/** + * Authorize the site requested by siteContext against the authenticated user + * (SEC-002). Must run after both requireAuth and siteContext. + * + * - admin and API-token callers are unrestricted (role model predates sites; + * scoping tokens is tracked as residual work, see docs/AUDITORIA_BASE_2GT.md). + * - a restricted user requesting a specific site must be a member of it, or + * the request is refused (403) -- this is what closes the IDOR: sending + * X-Site-Id for a site you don't belong to no longer works. + * - a restricted user requesting no specific site ("all") is scoped to every + * site they belong to, not the whole fleet. + */ +export async function enforceSiteAccess(req: Request, res: Response, next: NextFunction): Promise { + const user = req.user; + if (!user) { res.status(401).json({ error: 'No token provided' }); return; } + + if (user.role === 'admin' || user.tokenAuth) { + req.siteScope = req.siteId ?? null; + next(); + return; + } + + try { + const allowed = await getUserSiteIds(user.userId); + if (req.siteId !== null && req.siteId !== undefined) { + if (!allowed.includes(req.siteId)) { + res.status(403).json({ error: 'You do not have access to this site' }); + return; + } + req.siteScope = req.siteId; + } else { + req.siteScope = allowed; // may be [] -- "no access to anything", not "all" + } + next(); + } catch (e) { + res.status(500).json({ error: `Could not resolve site access: ${(e as Error).message}` }); + } +} + +/** Convenience accessor so routes don't repeat the optional chaining. */ +export function activeSite(req: Request): SiteScope { + return req.siteScope ?? null; +} + +/** + * Direct-by-id guard for routes keyed on a specific device (SEC-002). + * + * Meant to be wired with `router.param('id', ...)` (or called explicitly for + * a body/query-supplied deviceId, e.g. bulk commands and the terminal) so a + * restricted user can't reach a device outside their sites by simply knowing + * or guessing its numeric id -- the classic IDOR this mission calls out. + * + * Returns true if access is allowed (or the device doesn't exist -- the + * handler's own 404 is the right response, not a 403 that would confirm the + * id exists in someone else's site). + */ +export async function isDeviceAccessible(req: Request, deviceId: number): Promise { + const scope = activeSite(req); + if (scope === null) return true; // unrestricted (admin/token), or explicit single site already checked below + const device = await queryOne<{ site_id: number | null }>(`SELECT site_id FROM devices WHERE id = $1`, [deviceId]); + if (!device) return true; // let the handler 404 + if (device.site_id === null) return true; // unassigned devices are visible to everyone (should not occur post-migration) + return Array.isArray(scope) ? scope.includes(device.site_id) : scope === device.site_id; +} + +/** + * Express `router.param('id', ...)` handler that 403s a request for a device + * id outside the caller's authorized sites. Mount with: + * router.param('id', deviceIdSiteGuard); + * on any router whose `:id` is a device id. + */ +export async function deviceIdSiteGuard( + req: Request, res: Response, next: NextFunction, idParam: string +): Promise { + const id = parseInt(idParam, 10); + if (Number.isNaN(id)) { next(); return; } // let the handler reject the malformed id + try { + if (!(await isDeviceAccessible(req, id))) { + res.status(403).json({ error: 'You do not have access to this device' }); + return; + } + next(); + } catch (e) { + res.status(500).json({ error: `Could not resolve device access: ${(e as Error).message}` }); + } +} + +/** + * Bulk-operation guard (SEC-002): given a list of device ids supplied in a + * request body (commands, bulk backup/sync, the terminal's single deviceId), + * returns the subset the caller is NOT authorized for. An empty result means + * every id is in scope. Used where router.param can't help because the ids + * don't come from the URL. + */ +export async function unauthorizedDeviceIds(req: Request, deviceIds: number[]): Promise { + const scope = activeSite(req); + if (scope === null || deviceIds.length === 0) return []; + const rows = await query<{ id: number; site_id: number | null }>( + `SELECT id, site_id FROM devices WHERE id = ANY($1::int[])`, [deviceIds] + ); + const allowed = new Set(Array.isArray(scope) ? scope : [scope]); + return rows.filter((r) => r.site_id !== null && !allowed.has(r.site_id)).map((r) => r.id); +} + +/** + * Same idea as deviceIdSiteGuard, for a route param that identifies a *row* + * belonging to a device rather than the device itself (a backup, a config + * snapshot, a command run...). `resolveDeviceId` looks up that row's owning + * device id; returning null (row not found) lets the handler's own 404 win, + * same reasoning as isDeviceAccessible. + */ +export function ownedByDeviceSiteGuard( + resolveDeviceId: (rowId: number) => Promise +): (req: Request, res: Response, next: NextFunction, idParam: string) => Promise { + return async (req, res, next, idParam) => { + const rowId = parseInt(idParam, 10); + if (Number.isNaN(rowId)) { next(); return; } + try { + const deviceId = await resolveDeviceId(rowId); + if (deviceId === null) { next(); return; } // handler will 404 + if (!(await isDeviceAccessible(req, deviceId))) { + res.status(403).json({ error: 'You do not have access to this resource' }); + return; + } + next(); + } catch (e) { + res.status(500).json({ error: `Could not resolve site access: ${(e as Error).message}` }); + } + }; } diff --git a/backend/src/routes/auth.ts b/backend/src/routes/auth.ts index 4c0e5fe..5731a21 100644 --- a/backend/src/routes/auth.ts +++ b/backend/src/routes/auth.ts @@ -7,6 +7,7 @@ import { signToken, signRawToken, verifyRawToken, requireAuth } from '../middlew import { getSecretsInfo } from '../utils/secrets'; import { loginRateLimit, rateLimitRedis } from '../middleware/rateLimitRedis'; import { validatePassword } from '../utils/passwordPolicy'; +import { bumpTokenVersion } from '../utils/sessionAuth'; const router = Router(); @@ -50,9 +51,14 @@ router.post('/login', loginRateLimit(), async (req: Request, res: Response) => { password_hash: string; role: string; totp_enabled: boolean; - }>(`SELECT id, username, password_hash, role, totp_enabled FROM users WHERE username = $1`, [username]); + active: boolean; + token_version: number; + }>(`SELECT id, username, password_hash, role, totp_enabled, active, token_version FROM users WHERE username = $1`, [username]); - if (!user || !(await bcrypt.compare(password, user.password_hash))) { + // Same generic error for "no such user", "wrong password" and "deactivated" + // -- distinguishing them would leak which usernames exist and which are + // disabled. + if (!user || !user.active || !(await bcrypt.compare(password, user.password_hash))) { return res.status(401).json({ error: 'Invalid credentials' }); } @@ -61,7 +67,7 @@ router.post('/login', loginRateLimit(), async (req: Request, res: Response) => { return res.json({ requires_totp: true, totp_token: totpToken }); } - const token = signToken({ userId: user.id, username: user.username, role: user.role }); + const token = signToken({ userId: user.id, username: user.username, role: user.role, tokenVersion: user.token_version }); return res.json({ token, user: { id: user.id, username: user.username, role: user.role } }); }); @@ -81,11 +87,14 @@ router.post('/totp/verify', rateLimitRedis({ windowSec: 60, max: 5, keyPrefix: ' return res.status(401).json({ error: 'Invalid token type' }); } - const user = await queryOne<{ id: number; username: string; role: string; totp_secret: string | null; totp_enabled: boolean }>( - `SELECT id, username, role, totp_secret, totp_enabled FROM users WHERE id = $1`, + const user = await queryOne<{ + id: number; username: string; role: string; totp_secret: string | null; totp_enabled: boolean; + active: boolean; token_version: number; + }>( + `SELECT id, username, role, totp_secret, totp_enabled, active, token_version FROM users WHERE id = $1`, [payload.userId] ); - if (!user || !user.totp_enabled || !user.totp_secret) { + if (!user || !user.active || !user.totp_enabled || !user.totp_secret) { return res.status(401).json({ error: 'TOTP not configured' }); } @@ -95,7 +104,7 @@ router.post('/totp/verify', rateLimitRedis({ windowSec: 60, max: 5, keyPrefix: ' return res.status(401).json({ error: 'Invalid TOTP code' }); } - const token = signToken({ userId: user.id, username: user.username, role: user.role }); + const token = signToken({ userId: user.id, username: user.username, role: user.role, tokenVersion: user.token_version }); return res.json({ token, user: { id: user.id, username: user.username, role: user.role } }); }); @@ -165,6 +174,8 @@ router.post('/totp/disable', requireAuth, async (req: Request, res: Response) => } await query(`UPDATE users SET totp_enabled = false, totp_secret = NULL WHERE id = $1`, [req.user!.userId]); + // SEC-001: disabling 2FA is a security-relevant auth change; force re-auth. + await bumpTokenVersion(req.user!.userId); return res.json({ ok: true }); }); @@ -181,8 +192,14 @@ router.get('/me', requireAuth, (req: Request, res: Response) => { res.json({ user: req.user }); }); -router.post('/logout', requireAuth, (_req: Request, res: Response) => { - // JWT is stateless; client just discards token +// Logout revokes the session server-side (SEC-001) by bumping token_version, +// rather than relying on the client to discard the token. This invalidates +// every token issued for the account, not just the one presented here -- +// there is no per-device/per-tab session identity to revoke selectively, and +// "sign out everywhere" is an acceptable, documented trade for the added +// safety (see docs/AUDITORIA_BASE_2GT.md, SEC-001). +router.post('/logout', requireAuth, async (req: Request, res: Response) => { + await bumpTokenVersion(req.user!.userId); res.json({ message: 'Logged out' }); }); @@ -207,6 +224,10 @@ router.put('/password', requireAuth, async (req: Request, res: Response) => { const hash = await bcrypt.hash(newPassword, 12); await query(`UPDATE users SET password_hash = $1 WHERE id = $2`, [hash, req.user!.userId]); + // SEC-001: a password change is exactly the moment a session might be + // compromised -- kill every existing token immediately rather than let + // them ride out their 24h expiry. + await bumpTokenVersion(req.user!.userId); return res.json({ message: 'Password updated' }); }); diff --git a/backend/src/routes/backups.ts b/backend/src/routes/backups.ts index fa5878f..bb0a764 100644 --- a/backend/src/routes/backups.ts +++ b/backend/src/routes/backups.ts @@ -4,11 +4,22 @@ import * as path from 'path'; import { query, queryOne } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeByDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess, ownedByDeviceSiteGuard } from '../middleware/site'; import { BackupService } from '../services/BackupService'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); + +// SEC-002: :id/:fromId/:toId here are backup ids, not device ids -- resolve +// each to its owning device before letting a handler run. +const backupSiteGuard = ownedByDeviceSiteGuard(async (backupId) => { + const row = await queryOne<{ device_id: number }>(`SELECT device_id FROM backups WHERE id = $1`, [backupId]); + return row?.device_id ?? null; +}); +router.param('id', backupSiteGuard); +router.param('fromId', backupSiteGuard); +router.param('toId', backupSiteGuard); const backupService = new BackupService(); diff --git a/backend/src/routes/clients.ts b/backend/src/routes/clients.ts index 70ae868..8766202 100644 --- a/backend/src/routes/clients.ts +++ b/backend/src/routes/clients.ts @@ -4,7 +4,7 @@ import { query } from '../config/database'; import { logSafe } from '../utils/logSafe'; import { requireAuth, requireAdmin, requireWrite } from '../middleware/auth'; import { siteScopeByDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { PollerService } from '../services/PollerService'; import { getQueryApi, bucket } from '../config/influxdb'; import { fingerprintClient, DEVICE_CATEGORIES, DeviceCategory } from '../utils/clientFingerprint'; @@ -23,6 +23,7 @@ function withCategory>(row: T): T & { device_c const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); let pollerService: PollerService | null = null; export function setPollerService(p: PollerService): void { pollerService = p; } diff --git a/backend/src/routes/commands.ts b/backend/src/routes/commands.ts index f112b96..20ab6a8 100644 --- a/backend/src/routes/commands.ts +++ b/backend/src/routes/commands.ts @@ -9,12 +9,13 @@ import { Router } from 'express'; import type { Request, Response } from 'express'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeByDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess, unauthorizedDeviceIds } from '../middleware/site'; import { query, queryOne } from '../config/database'; import { commandRunner } from '../services/CommandRunner'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); /** Commands that change how a device is reached, and so deserve a warning. */ const RISKY_PATTERNS: { pattern: RegExp; why: string }[] = [ @@ -46,6 +47,10 @@ router.post('/preview', async (req: Request, res: Response) => { if (!Array.isArray(device_ids) || device_ids.length === 0) { return res.status(400).json({ error: 'device_ids array is required' }); } + const forbidden = await unauthorizedDeviceIds(req, device_ids); + if (forbidden.length) { + return res.status(403).json({ error: 'One or more devices are outside your authorized sites', device_ids: forbidden }); + } const size = Math.max(1, Math.min(50, Number(wave_size) || 1)); const devices = await query<{ id: number; name: string; ip_address: string; status: string }>( @@ -85,6 +90,10 @@ router.post('/runs', requireWrite, async (req: Request, res: Response) => { if (!Array.isArray(device_ids) || device_ids.length === 0) { return res.status(400).json({ error: 'device_ids array is required' }); } + const forbidden = await unauthorizedDeviceIds(req, device_ids); + if (forbidden.length) { + return res.status(403).json({ error: 'One or more devices are outside your authorized sites', device_ids: forbidden }); + } const size = Math.max(1, Math.min(50, Number(wave_size) || 1)); const run = await queryOne<{ id: number }>( diff --git a/backend/src/routes/configHistory.ts b/backend/src/routes/configHistory.ts index ff19476..bc97419 100644 --- a/backend/src/routes/configHistory.ts +++ b/backend/src/routes/configHistory.ts @@ -1,11 +1,14 @@ import { Router, Request, Response } from 'express'; import { query, queryOne } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; +import { enforceSiteAccess, deviceIdSiteGuard } from '../middleware/site'; import { BackupService } from '../services/BackupService'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); +router.param('deviceId', deviceIdSiteGuard); const backupService = new BackupService(); diff --git a/backend/src/routes/credentialPresets.ts b/backend/src/routes/credentialPresets.ts index f73fd9f..ffa7e6b 100644 --- a/backend/src/routes/credentialPresets.ts +++ b/backend/src/routes/credentialPresets.ts @@ -3,6 +3,7 @@ import { query, queryOne } from '../config/database'; import { requireAuth, requireAdmin } from '../middleware/auth'; import { encrypt } from '../utils/crypto'; import { rateLimitRedis } from '../middleware/rateLimitRedis'; +import { validateRouterOsUsername } from '../utils/routerOsUsername'; const router = Router(); router.use(requireAuth); @@ -106,6 +107,8 @@ router.post('/', requireAdmin, presetMutationLimiter, async (req: Request, res: .status(400) .json({ error: 'name, api_username, and api_password are required' }); } + const sshUsernameError = validateRouterOsUsername(ssh_username); + if (sshUsernameError) return res.status(400).json({ error: sshUsernameError }); const existing = await queryOne<{ id: number }>( `SELECT id FROM credential_presets WHERE name = $1`, @@ -184,6 +187,8 @@ router.put('/:id', requireAdmin, presetMutationLimiter, async (req: Request, res ); if (clash) return res.status(409).json({ error: 'A preset with this name already exists' }); } + const sshUsernameError = validateRouterOsUsername(ssh_username); + if (sshUsernameError) return res.status(400).json({ error: sshUsernameError }); const newApiPass = api_password ? encrypt(api_password) : existing.api_password_encrypted; let newSshPass: string | null = existing.ssh_password_encrypted; diff --git a/backend/src/routes/devices.ts b/backend/src/routes/devices.ts index 5c5a2bd..48b6b30 100644 --- a/backend/src/routes/devices.ts +++ b/backend/src/routes/devices.ts @@ -2,7 +2,8 @@ import { Router, Request, Response } from 'express'; import { randomUUID } from 'crypto'; import { Client as SshClient } from 'ssh2'; import { query, queryOne } from '../config/database'; -import { requireAuth, requireWrite } from '../middleware/auth'; +import { requireAuth, requireWrite, requireAdmin } from '../middleware/auth'; +import { deviceIdSiteGuard } from '../middleware/site'; import { encrypt, decrypt } from '../utils/crypto'; import { RouterOSClient } from '../services/mikrotik/RouterOSClient'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; @@ -12,6 +13,7 @@ import { createDeviceFromBody, type CreateDeviceInput, type CreateDeviceContext import { parsePort } from '../utils/parsePort'; import { safeConnectionError } from '../utils/safeClientError'; import { detectLockoutRisk } from '../utils/firewallSafety'; +import { validateRouterOsUsername } from '../utils/routerOsUsername'; import { withSafeApply, probeCapability, probeUndoCapability, type GuardDevice } from '../services/changeGuard/ChangeGuard'; import { captureSnapshot, resolveManagementPath } from '../services/changeGuard/pathModel'; import { analyzeChange, type PlannedChange } from '../services/changeGuard/analyzeChange'; @@ -22,8 +24,8 @@ import { enqueueBulkAddJob, getBulkAddJobState } from '../services/DeviceBulkAdd import { logSafe } from '../utils/logSafe'; import { DEVICE_BASE_COLUMNS } from '../services/deviceColumns'; import { buildSegments, summarise, summariseBands } from '../utils/lteDwell'; -import { siteScopeDevices, andSite } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { siteScopeDevices, andSite, singleSiteOrNull } from '../utils/siteScope'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; /** Postgres interval text to milliseconds, for the few windows we offer. */ function intervalToMs(interval: string): number { @@ -70,6 +72,12 @@ async function loadCredentialPreset( const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); +// SEC-002: every :id in this router is a device id -- refuse it up front if +// the device belongs to a site the caller isn't authorized for, before any +// handler runs. Closes the IDOR of reaching another site's device by guessing +// or enumerating its numeric id. +router.param('id', deviceIdSiteGuard); let pollerService: PollerService | null = null; export function setPollerService(p: PollerService): void { @@ -285,7 +293,7 @@ router.post('/', requireWrite, async (req: Request, res: Response) => { const result = await createDeviceFromBody(req.body, pollerService, { requestingUserRole: req.user?.role, // A device added while viewing a site joins that site (issue #130). - siteId: activeSite(req), + siteId: singleSiteOrNull(activeSite(req)), }); return res.status(result.status).json(result.body); }); @@ -316,7 +324,7 @@ router.post('/bulk-add/jobs', requireWrite, async (req: Request, res: Response) BULK_ADD_META_TTL_SEC ); await redis.set(`device-bulk-add:${jobId}:results`, '[]', 'EX', BULK_ADD_META_TTL_SEC); - await enqueueBulkAddJob(jobId, items as CreateDeviceInput[], activeSite(req)); + await enqueueBulkAddJob(jobId, items as CreateDeviceInput[], singleSiteOrNull(activeSite(req))); return res.status(202).json({ job_id: jobId, total: items.length }); }); @@ -450,6 +458,11 @@ router.put('/:id', requireWrite, async (req: Request, res: Response) => { const ssh_username = preset ? preset.ssh_username : req.body.ssh_username; const ssh_password = preset ? preset.ssh_password : req.body.ssh_password; + // SEC-006: reject an unsafe ssh_username before it can ever reach the + // RouterOS CLI command SshKeyService builds from it. + const sshUsernameError = validateRouterOsUsername(ssh_username); + if (sshUsernameError) return res.status(400).json({ error: sshUsernameError }); + // If the user is changing the IP (or port / username / password), verify the // RouterOS API is still reachable with the new values before persisting — // otherwise we could silently brick the device record. diff --git a/backend/src/routes/events.ts b/backend/src/routes/events.ts index e5735a3..fb78931 100644 --- a/backend/src/routes/events.ts +++ b/backend/src/routes/events.ts @@ -2,10 +2,11 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeByNullableDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // GET /api/events /** diff --git a/backend/src/routes/firmware.ts b/backend/src/routes/firmware.ts index aa9fa7f..3090a27 100644 --- a/backend/src/routes/firmware.ts +++ b/backend/src/routes/firmware.ts @@ -2,7 +2,7 @@ import { Router, Request, Response } from 'express'; import { query, queryOne } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeDevices, siteScopeByDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; import { firmwareOrchestrator } from '../services/FirmwareOrchestrator'; import { clampConcurrency, MAX_WAVE_CONCURRENCY } from '../utils/concurrency'; @@ -10,6 +10,7 @@ import { findUpstreamWithinSelection } from '../utils/rolloutTopology'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // GET /api/firmware/overview — fleet versions + latest rollout router.get('/overview', async (req: Request, res: Response) => { diff --git a/backend/src/routes/metrics.ts b/backend/src/routes/metrics.ts index f8356b9..f696201 100644 --- a/backend/src/routes/metrics.ts +++ b/backend/src/routes/metrics.ts @@ -2,11 +2,13 @@ import { Router, Request, Response } from 'express'; import { getQueryApi, bucket } from '../config/influxdb'; import { query } from '../config/database'; import { requireAuth } from '../middleware/auth'; -import { siteScopeDevices, siteScopeByDevice, siteScopeByNullableDevice, clientSeriesTag } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { siteScopeDevices, siteScopeByDevice, siteScopeByNullableDevice, clientSeriesTag, singleSiteOrNull } from '../utils/siteScope'; +import { activeSite, enforceSiteAccess, deviceIdSiteGuard } from '../middleware/site'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); +router.param('deviceId', deviceIdSiteGuard); function rangeToFlux(range: string): string { const allowed = ['1h', '3h', '6h', '12h', '24h', '7d', '30d']; @@ -25,7 +27,15 @@ router.get('/clients-over-time', async (req: Request, res: Response) => { // the global series there is not a shortcut -- it keeps the chart's full // history, which the per-site series only accumulates from the upgrade // onward. That covers every single-site install. - const siteId = activeSite(req); + // clientSeriesTag needs one concrete site id or the true fleet-wide case. + // SEC-002: a restricted user viewing several sites at once has neither -- + // there is no single dedup series for "these particular sites", and + // falling back to `_global` would hand them the whole fleet's count. Such a + // scope skips the dedup series entirely and goes straight to the per-device + // fallback below, filtered to exactly the devices they may see. + const scope = activeSite(req); + const siteId = singleSiteOrNull(scope); + const restrictedToMultipleSites = Array.isArray(scope); let seriesTag = '_global'; if (siteId !== null) { const counts = await query<{ in_site: number; total: number }>( @@ -40,29 +50,31 @@ router.get('/clients-over-time', async (req: Request, res: Response) => { // Use the global deduplicated metric (_global tag) written by DeviceCollector. // This avoids double-counting clients seen by multiple devices simultaneously. - const fluxQuery = ` - from(bucket: "${bucket}") - |> range(start: -${range}) - |> filter(fn: (r) => r._measurement == "client_counts") - |> filter(fn: (r) => r._field == "total_clients") - |> filter(fn: (r) => r.device_id == "${seriesTag}") - |> aggregateWindow(every: 5m, fn: last, createEmpty: false) - |> yield(name: "clients_over_time") - `; - const rawPoints: { time: string; value: number }[] = []; - try { - await queryApi.collectRows(fluxQuery, (row, tableMeta) => { - const time = tableMeta.get(row, '_time') as string; - const value = tableMeta.get(row, '_value') as number; - rawPoints.push({ time, value: Math.round(value) }); - }); - } catch { - // InfluxDB might not have data yet + if (!restrictedToMultipleSites) { + const fluxQuery = ` + from(bucket: "${bucket}") + |> range(start: -${range}) + |> filter(fn: (r) => r._measurement == "client_counts") + |> filter(fn: (r) => r._field == "total_clients") + |> filter(fn: (r) => r.device_id == "${seriesTag}") + |> aggregateWindow(every: 5m, fn: last, createEmpty: false) + |> yield(name: "clients_over_time") + `; + try { + await queryApi.collectRows(fluxQuery, (row, tableMeta) => { + const time = tableMeta.get(row, '_time') as string; + const value = tableMeta.get(row, '_value') as number; + rawPoints.push({ time, value: Math.round(value) }); + }); + } catch { + // InfluxDB might not have data yet + } } // If no deduplicated series exists yet (first run, or a site whose history - // predates the per-site series), fall back to max across real devices. max() + // predates the per-site series), or the caller is scoped to several sites + // at once, fall back to max across the exact devices they may see. max() // is better than sum() since the device with the most clients is the gateway // that sees everyone — summing would double-count clients visible from // multiple devices. @@ -72,12 +84,16 @@ router.get('/clients-over-time', async (req: Request, res: Response) => { // it matters: without it the fleet-wide fallback would fold the per-site // totals back in and count everyone twice. let deviceFilter: string | null = `|> filter(fn: (r) => not (r.device_id =~ /^_/))`; - if (siteId !== null) { - const ids = await query<{ id: number }>( - `SELECT id FROM devices WHERE site_id = $1`, [siteId] - ); - // An empty site has no series to fall back to, and must show an empty - // chart rather than the fleet's (issue #130). + if (scope !== null) { + const scopePredicate = Array.isArray(scope) ? scope : [scope]; + const ids = scopePredicate.length + ? await query<{ id: number }>( + `SELECT id FROM devices WHERE site_id = ANY($1::int[])`, [scopePredicate] + ) + : []; + // An empty site (or a viewer authorized for zero sites) has no series + // to fall back to, and must show an empty chart rather than the fleet's + // (issue #130 / SEC-002). deviceFilter = ids.length ? `|> filter(fn: (r) => ${ids.map((d) => `r.device_id == "${d.id}"`).join(' or ')})` : null; diff --git a/backend/src/routes/networkServices.ts b/backend/src/routes/networkServices.ts index f1170bf..f892d4b 100644 --- a/backend/src/routes/networkServices.ts +++ b/backend/src/routes/networkServices.ts @@ -2,12 +2,13 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeDevices } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; import { netflowCollector } from '../services/netflow/NetflowCollector'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // ─── Helper ─────────────────────────────────────────────────────────────────── diff --git a/backend/src/routes/oidc.ts b/backend/src/routes/oidc.ts index 179dec2..e4fc580 100644 --- a/backend/src/routes/oidc.ts +++ b/backend/src/routes/oidc.ts @@ -10,6 +10,7 @@ import { Router, Request, Response } from 'express'; import { Issuer } from 'openid-client'; import { requireAuth, requireAdmin, signToken } from '../middleware/auth'; +import { queryOne } from '../config/database'; import { rateLimitRedis } from '../middleware/rateLimitRedis'; import { loadOidcConfig, saveOidcConfig, maskedConfig, APP_ROLES, type AppRole, type OidcConfig, @@ -65,7 +66,14 @@ router.get( const params = req.query as Record; if (params.error) throw new Error(params.error_description || params.error); const { user, returnTo } = await completeLogin(params); - const token = signToken({ userId: user.id, username: user.username, role: user.role }); + // SEC-001: SSO users go through the same revocable-session mechanism as + // local accounts -- embed the current token_version so a "revoke + // sessions" action or deactivation applies to them too. + const current = await queryOne<{ token_version: number; active: boolean }>( + `SELECT token_version, active FROM users WHERE id = $1`, [user.id] + ); + if (!current || !current.active) throw new Error('Account is deactivated'); + const token = signToken({ userId: user.id, username: user.username, role: user.role, tokenVersion: current.token_version }); const dest = safeReturnTo(returnTo); // Hand the token to the SPA via fragment (never sent to the server/logs). res.redirect(`/auth/callback#token=${encodeURIComponent(token)}&returnTo=${encodeURIComponent(dest)}`); diff --git a/backend/src/routes/operations.ts b/backend/src/routes/operations.ts index 3a769f5..03559e6 100644 --- a/backend/src/routes/operations.ts +++ b/backend/src/routes/operations.ts @@ -2,13 +2,14 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { getQueryApi } from '../config/influxdb'; import { requireAuth, requireWrite } from '../middleware/auth'; -import { siteScopeDevices, siteScopeByDevice, siteScopeByNullableDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { siteScopeDevices, siteScopeByDevice, siteScopeByNullableDevice, singleSiteOrNull, SiteScope } from '../utils/siteScope'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; import { BackupService, BackupDevice } from '../services/BackupService'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); const backupService = new BackupService(); @@ -454,7 +455,7 @@ async function influxGroupValues(flux: string): Promise> { return out; } -async function detectAnomalies(siteId: number | null): Promise { +async function detectAnomalies(siteId: SiteScope): Promise { const items: AttentionItem[] = []; const hour = new Date().getUTCHours(); const nameById = new Map(); @@ -556,7 +557,7 @@ async function detectAnomalies(siteId: number | null): Promise // who did what to the install -- they carry no device and so belong to no site, // and repeating "admin changed a setting" into every site would misattribute it. // Device-scoped history (config changes, events) is filtered normally. -async function buildActivity(siteId: number | null) { +async function buildActivity(siteId: SiteScope) { const cfgFilter = siteScopeByDevice(siteId, 'dc.device_id'); const evtFilter = siteScopeByNullableDevice(siteId, 'e.device_id'); const [configs, audits, events] = await Promise.all([ diff --git a/backend/src/routes/routers.ts b/backend/src/routes/routers.ts index 0221aec..f255356 100644 --- a/backend/src/routes/routers.ts +++ b/backend/src/routes/routers.ts @@ -2,11 +2,12 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeDevices } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // GET /api/routers/lldp — LLDP enabled/disabled status per online router router.get('/lldp', async (req: Request, res: Response) => { diff --git a/backend/src/routes/search.ts b/backend/src/routes/search.ts index d5b6941..db5bde3 100644 --- a/backend/src/routes/search.ts +++ b/backend/src/routes/search.ts @@ -2,10 +2,11 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { requireAuth } from '../middleware/auth'; import { siteScopeDevices, siteScopeByNullableDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // GET /api/search?q= router.get('/', async (req: Request, res: Response) => { diff --git a/backend/src/routes/settings.ts b/backend/src/routes/settings.ts index b8f9ed1..956edb2 100644 --- a/backend/src/routes/settings.ts +++ b/backend/src/routes/settings.ts @@ -5,6 +5,7 @@ import { requireAuth, requireAdmin } from '../middleware/auth'; import { netflowCollector } from '../services/netflow/NetflowCollector'; import { validatePassword } from '../utils/passwordPolicy'; import { OIDC_SETTINGS_KEY } from '../services/oidc/oidcConfig'; +import { bumpTokenVersion } from '../utils/sessionAuth'; const router = Router(); router.use(requireAuth); @@ -47,7 +48,7 @@ router.put('/', requireAdmin, async (req: Request, res: Response) => { // GET /api/settings/users router.get('/users', requireAdmin, async (_req: Request, res: Response) => { const users = await query( - `SELECT id, username, email, role, auth_provider, created_at FROM users ORDER BY username` + `SELECT id, username, email, role, active, auth_provider, created_at FROM users ORDER BY username` ); res.json(users); }); @@ -82,10 +83,10 @@ router.post('/users', requireAdmin, async (req: Request, res: Response) => { return res.status(201).json(created); }); -// PUT /api/settings/users/:id - update role and/or reset password +// PUT /api/settings/users/:id - update role, active flag and/or reset password router.put('/users/:id', requireAdmin, async (req: Request, res: Response) => { const userId = parseInt(req.params.id); - const { role, password } = req.body; + const { role, password, active } = req.body as { role?: string; password?: string; active?: boolean }; const existing = await queryOne<{ id: number; role: string }>( `SELECT id, role FROM users WHERE id = $1`, @@ -97,14 +98,29 @@ router.put('/users/:id', requireAdmin, async (req: Request, res: Response) => { if (userId === req.user!.userId && role && role !== 'admin') { return res.status(400).json({ error: 'Cannot change your own role away from admin' }); } + // Cannot deactivate your own account (mirrors the self-delete guard below; + // an admin locking themselves out would need another admin to undo it). + if (userId === req.user!.userId && active === false) { + return res.status(400).json({ error: 'Cannot deactivate your own account' }); + } const validRoles = ['admin', 'operator', 'viewer']; if (role && !validRoles.includes(role)) { return res.status(400).json({ error: 'Invalid role' }); } + // A role change or deactivation is exactly the case SEC-001 exists for: + // the account's existing sessions must stop working immediately, not at + // their natural 24h expiry. + let mustRevoke = false; + if (role) { await query(`UPDATE users SET role = $1 WHERE id = $2`, [role, userId]); + mustRevoke = true; + } + if (typeof active === 'boolean') { + await query(`UPDATE users SET active = $1 WHERE id = $2`, [active, userId]); + if (!active) mustRevoke = true; } if (password) { const pwError = validatePassword(password); @@ -113,15 +129,28 @@ router.put('/users/:id', requireAdmin, async (req: Request, res: Response) => { } const hash = await bcrypt.hash(password, 12); await query(`UPDATE users SET password_hash = $1 WHERE id = $2`, [hash, userId]); + mustRevoke = true; } + if (mustRevoke) await bumpTokenVersion(userId); const updated = await queryOne( - `SELECT id, username, role, created_at FROM users WHERE id = $1`, + `SELECT id, username, role, active, created_at FROM users WHERE id = $1`, [userId] ); return res.json(updated); }); +// POST /api/settings/users/:id/revoke-sessions — end every session for a user +// right now, without changing their password or role (SEC-001). Useful for +// "this laptop was stolen" without also forcing an unrelated password reset. +router.post('/users/:id/revoke-sessions', requireAdmin, async (req: Request, res: Response) => { + const userId = parseInt(req.params.id); + const existing = await queryOne<{ id: number }>(`SELECT id FROM users WHERE id = $1`, [userId]); + if (!existing) return res.status(404).json({ error: 'User not found' }); + await bumpTokenVersion(userId); + return res.json({ message: 'All sessions for this user have been revoked' }); +}); + // DELETE /api/settings/users/:id router.delete('/users/:id', requireAdmin, async (req: Request, res: Response) => { if (parseInt(req.params.id) === req.user!.userId) { diff --git a/backend/src/routes/sites.ts b/backend/src/routes/sites.ts index 4ccff37..d84ca16 100644 --- a/backend/src/routes/sites.ts +++ b/backend/src/routes/sites.ts @@ -1,6 +1,7 @@ import { Router, Request, Response } from 'express'; import { query, queryOne } from '../config/database'; -import { requireAuth, requireWrite } from '../middleware/auth'; +import { requireAuth, requireWrite, requireAdmin } from '../middleware/auth'; +import { invalidateUserSiteCache } from '../middleware/site'; const router = Router(); router.use(requireAuth); @@ -36,8 +37,15 @@ function normaliseCoords(lat: unknown, lng: unknown): { lat: number | null; lng: return ok ? { lat: latN, lng: lngN } : { lat: null, lng: null }; } -// GET /api/sites — list with device counts, for the selector and the all-sites table -router.get('/', async (_req: Request, res: Response) => { +// GET /api/sites — list with device counts, for the selector and the all-sites table. +// SEC-002: a restricted (non-admin) caller only sees sites they're a member +// of -- otherwise every viewer could enumerate every other client's site +// names regardless of the X-Site-Id enforcement on device data itself. +router.get('/', async (req: Request, res: Response) => { + const restricted = req.user!.role !== 'admin' && !req.user!.tokenAuth; + const memberFilter = restricted + ? `WHERE s.id IN (SELECT site_id FROM user_sites WHERE user_id = $1)` + : ''; const sites = await query( `SELECT s.id, s.name, s.address, s.location_lat::float8 AS location_lat, @@ -47,8 +55,10 @@ router.get('/', async (_req: Request, res: Response) => { COUNT(d.id) FILTER (WHERE d.status = 'online')::text AS online_count FROM sites s LEFT JOIN devices d ON d.site_id = s.id + ${memberFilter} GROUP BY s.id - ORDER BY s.is_default DESC, LOWER(s.name) ASC` + ORDER BY s.is_default DESC, LOWER(s.name) ASC`, + restricted ? [req.user!.userId] : [] ); res.json(sites.map((s) => ({ ...s, @@ -180,4 +190,62 @@ router.post('/:id/devices', requireWrite, async (req: Request, res: Response) => res.json({ ok: true, moved: ids.length }); }); +// ─── SEC-002: site membership management (admin only) ─────────────────────── + +// GET /api/sites/:id/users — who currently has access to this site +router.get('/:id/users', requireAdmin, async (req: Request, res: Response) => { + const id = parseInt(req.params.id, 10); + if (!Number.isInteger(id)) { res.status(400).json({ error: 'Invalid site id' }); return; } + const users = await query<{ id: number; username: string; role: string }>( + `SELECT u.id, u.username, u.role FROM user_sites us + JOIN users u ON u.id = us.user_id + WHERE us.site_id = $1 + ORDER BY u.username`, + [id] + ); + res.json(users); +}); + +// POST /api/sites/:id/users — grant a user access to this site +router.post('/:id/users', requireAdmin, async (req: Request, res: Response) => { + const id = parseInt(req.params.id, 10); + const userId = parseInt(String((req.body as { userId?: unknown }).userId), 10); + if (!Number.isInteger(id) || !Number.isInteger(userId)) { + res.status(400).json({ error: 'Invalid site id or userId' }); + return; + } + const [site, user] = await Promise.all([ + queryOne<{ id: number }>(`SELECT id FROM sites WHERE id = $1`, [id]), + queryOne<{ id: number; role: string }>(`SELECT id, role FROM users WHERE id = $1`, [userId]), + ]); + if (!site) { res.status(404).json({ error: 'Site not found' }); return; } + if (!user) { res.status(404).json({ error: 'User not found' }); return; } + if (user.role === 'admin') { + // Admins already see every site implicitly; a membership row for one + // would be dead data and would misleadingly suggest their access is + // scoped. + res.status(400).json({ error: 'Admins already have access to every site' }); + return; + } + await query( + `INSERT INTO user_sites (user_id, site_id) VALUES ($1, $2) ON CONFLICT DO NOTHING`, + [userId, id] + ); + invalidateUserSiteCache(userId); + res.status(201).json({ ok: true }); +}); + +// DELETE /api/sites/:id/users/:userId — revoke a user's access to this site +router.delete('/:id/users/:userId', requireAdmin, async (req: Request, res: Response) => { + const id = parseInt(req.params.id, 10); + const userId = parseInt(req.params.userId, 10); + if (!Number.isInteger(id) || !Number.isInteger(userId)) { + res.status(400).json({ error: 'Invalid site id or userId' }); + return; + } + await query(`DELETE FROM user_sites WHERE site_id = $1 AND user_id = $2`, [id, userId]); + invalidateUserSiteCache(userId); + res.json({ ok: true }); +}); + export default router; diff --git a/backend/src/routes/switches.ts b/backend/src/routes/switches.ts index 8bc530d..79aead7 100644 --- a/backend/src/routes/switches.ts +++ b/backend/src/routes/switches.ts @@ -2,11 +2,12 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeDevices } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // GET /api/switches — all switch devices with port statistics router.get('/', async (req: Request, res: Response) => { diff --git a/backend/src/routes/topology.ts b/backend/src/routes/topology.ts index 6d7a6a5..7c729a7 100644 --- a/backend/src/routes/topology.ts +++ b/backend/src/routes/topology.ts @@ -2,7 +2,7 @@ import { Router, Request, Response } from 'express'; import { query, queryOne } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeDevices, siteScopeByDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { PollerService } from '../services/PollerService'; import { buildTopology, @@ -13,6 +13,7 @@ import { const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); let pollerService: PollerService | null = null; export function setPollerService(p: PollerService): void { diff --git a/backend/src/routes/wireless.ts b/backend/src/routes/wireless.ts index cbfb0fc..cffc929 100644 --- a/backend/src/routes/wireless.ts +++ b/backend/src/routes/wireless.ts @@ -2,13 +2,14 @@ import { Router, Request, Response } from 'express'; import { query } from '../config/database'; import { requireAuth, requireWrite } from '../middleware/auth'; import { siteScopeDevices, siteScopeByDevice } from '../utils/siteScope'; -import { activeSite } from '../middleware/site'; +import { activeSite, enforceSiteAccess } from '../middleware/site'; import { DeviceCollector, DeviceRow } from '../services/mikrotik/DeviceCollector'; import { lookupVendor } from '../utils/oui'; import { analyzeSpectrum, summarize, BAND_RANGES, parseChannelSpec } from '../utils/rfSpectrum'; const router = Router(); router.use(requireAuth); +router.use(enforceSiteAccess); // ─── Helper ─────────────────────────────────────────────────────────────────── diff --git a/backend/src/services/SshKeyService.ts b/backend/src/services/SshKeyService.ts index 0b8feb3..37ab714 100644 --- a/backend/src/services/SshKeyService.ts +++ b/backend/src/services/SshKeyService.ts @@ -17,6 +17,7 @@ import { encrypt, decrypt } from '../utils/crypto'; import { generateDeviceKeyPair, keyFileName, keyComment, isUsablePrivateKey, type KeyStatus, } from '../utils/sshKeys'; +import { isValidRouterOsUsername } from '../utils/routerOsUsername'; import { RouterOSClient } from './mikrotik/RouterOSClient'; const CONNECT_TIMEOUT_MS = 20_000; @@ -173,6 +174,13 @@ export class SshKeyService { async deploy(target: SshTarget, opts: { rotate?: boolean } = {}): Promise { const username = target.ssh_username?.trim(); if (!username) throw new Error('No SSH username configured for this device'); + // SEC-006: username is interpolated unescaped into a RouterOS CLI command + // below (`user=${username}`). Validating here, at the point of use, is + // defense in depth even though device writes already reject an invalid + // username before it reaches storage (routes/devices.ts, deviceCreation.ts). + if (!isValidRouterOsUsername(username)) { + throw new Error('Stored SSH username contains characters that are not safe to use — fix it in device settings before deploying a key'); + } const existing = await this.getKey(target.id); const canUseExistingKey = existing && existing.status === 'verified'; diff --git a/backend/src/services/deviceCreation.ts b/backend/src/services/deviceCreation.ts index eff1777..dda98d4 100644 --- a/backend/src/services/deviceCreation.ts +++ b/backend/src/services/deviceCreation.ts @@ -2,6 +2,7 @@ import { query, queryOne } from '../config/database'; import { encrypt, decrypt } from '../utils/crypto'; import { parsePort } from '../utils/parsePort'; import { safeConnectionError } from '../utils/safeClientError'; +import { validateRouterOsUsername } from '../utils/routerOsUsername'; import { RouterOSClient } from './mikrotik/RouterOSClient'; import type { PollerService } from './PollerService'; import type { CredentialPresetRow } from '../routes/credentialPresets'; @@ -112,6 +113,13 @@ export async function createDeviceFromBody( body: { error: 'name, ip_address, api_username, api_password are required' }, }; } + // SEC-006: ssh_username is later interpolated unescaped into a RouterOS CLI + // command (services/SshKeyService.ts). Reject an unsafe value at the point + // it's first stored, not just where it happens to be used. + const sshUsernameError = validateRouterOsUsername(ssh_username); + if (sshUsernameError) { + return { ok: false, status: 400, body: { error: sshUsernameError } }; + } const testClient = new RouterOSClient(ip_address, api_port, api_username, api_password, 10_000); let detectedSerial: string | null; diff --git a/backend/src/utils/__tests__/routerOsUsername.test.ts b/backend/src/utils/__tests__/routerOsUsername.test.ts new file mode 100644 index 0000000..ce60264 --- /dev/null +++ b/backend/src/utils/__tests__/routerOsUsername.test.ts @@ -0,0 +1,40 @@ +import { isValidRouterOsUsername, validateRouterOsUsername } from '../routerOsUsername'; + +describe('SEC-006: RouterOS SSH username allowlist', () => { + it('accepts ordinary usernames', () => { + for (const u of ['admin', 'svc-rmm', 'ops_user', 'user.name', 'a1', 'A']) { + expect(isValidRouterOsUsername(u)).toBe(true); + } + }); + + it('rejects command-injection payloads that rely on RouterOS token separators', () => { + const payloads = [ + 'admin;/system/reboot', + 'admin /system/reboot', + 'admin\n/system/reboot', + 'admin\t/system/reboot', + 'admin"; /system/reboot; "', + 'admin$(reboot)', + 'admin`reboot`', + 'admin/../root', + 'admin=x comment=y', + '', + ' ', + 'a'.repeat(33), // over the length cap + ]; + for (const p of payloads) { + expect(isValidRouterOsUsername(p)).toBe(false); + } + }); + + it('validateRouterOsUsername treats an absent value as fine (field is optional)', () => { + expect(validateRouterOsUsername(undefined)).toBeNull(); + expect(validateRouterOsUsername(null)).toBeNull(); + expect(validateRouterOsUsername('')).toBeNull(); + }); + + it('validateRouterOsUsername rejects a non-string and an unsafe string with a message', () => { + expect(validateRouterOsUsername(123)).toMatch(/string/i); + expect(validateRouterOsUsername('admin; reboot')).toMatch(/letters, digits/i); + }); +}); diff --git a/backend/src/utils/routerOsUsername.ts b/backend/src/utils/routerOsUsername.ts new file mode 100644 index 0000000..a42291f --- /dev/null +++ b/backend/src/utils/routerOsUsername.ts @@ -0,0 +1,33 @@ +/** + * Positive-allowlist validation for a RouterOS SSH/API username (SEC-006). + * + * The username is stored by an operator and later interpolated directly into + * RouterOS CLI commands sent over an SSH exec channel (see + * services/SshKeyService.ts: `/user/ssh-keys/import ... user=${username}`). + * Escaping after the fact is fragile — RouterOS's own command parser has its + * own quoting rules, not shell rules, so a shell-style escape can still leave + * a value that RouterOS itself parses as more than one token. Validating + * against a known-safe character set before it is ever stored is simpler and + * closes the injection regardless of where the value is later used. + * + * RouterOS itself limits usernames to letters, digits, and a handful of + * punctuation characters, with no whitespace — this allowlist is deliberately + * narrower than what the device might tolerate, because "narrower than + * necessary but provably safe" is the right trade for a value that ends up + * unescaped in a command line. + */ +const VALID_USERNAME = /^[A-Za-z0-9][A-Za-z0-9._-]{0,31}$/; + +export function isValidRouterOsUsername(username: string): boolean { + return VALID_USERNAME.test(username); +} + +/** Returns an error message if invalid, or null if the username is acceptable. */ +export function validateRouterOsUsername(username: unknown): string | null { + if (username === null || username === undefined || username === '') return null; // optional field + if (typeof username !== 'string') return 'Username must be a string'; + if (!isValidRouterOsUsername(username)) { + return 'Username may only contain letters, digits, "." "_" "-", must start with a letter or digit, and be at most 32 characters'; + } + return null; +} diff --git a/backend/src/utils/sessionAuth.ts b/backend/src/utils/sessionAuth.ts new file mode 100644 index 0000000..c30d470 --- /dev/null +++ b/backend/src/utils/sessionAuth.ts @@ -0,0 +1,72 @@ +/** + * Server-side JWT revocation (SEC-001). + * + * JWTs stay stateless for signing/verifying (no DB write on every request), + * but every session must be killable before its natural expiry: logout, + * a password change, disabling an account, or an admin "sign this user out + * everywhere" action. The mechanism is a `token_version` column on `users`: + * every signed token embeds the version current at sign time, and a request + * is rejected the moment that version no longer matches the row -- bumping + * the column *is* the revoke. + * + * Checking on every request would mean a DB round trip per authenticated + * call. A short in-process cache (a few seconds) keeps the common case at + * zero extra latency while capping how long a revoked token can keep working + * to a small, documented window -- far better than the 24h it had before. + */ +import { query, queryOne } from '../config/database'; + +export interface SessionState { + tokenVersion: number; + active: boolean; + role: string; +} + +interface CacheEntry { + state: SessionState | null; // null = user no longer exists + cachedAt: number; +} + +const CACHE_TTL_MS = 5_000; +const cache = new Map(); + +async function fetchState(userId: number): Promise { + const row = await queryOne<{ token_version: number; active: boolean; role: string }>( + `SELECT token_version, active, role FROM users WHERE id = $1`, + [userId] + ); + return row ? { tokenVersion: row.token_version, active: row.active, role: row.role } : null; +} + +/** Current session state for a user, cached for a few seconds. */ +export async function getSessionState(userId: number): Promise { + const cached = cache.get(userId); + if (cached && Date.now() - cached.cachedAt < CACHE_TTL_MS) { + return cached.state; + } + const state = await fetchState(userId); + cache.set(userId, { state, cachedAt: Date.now() }); + return state; +} + +/** + * Bump a user's token_version, invalidating every JWT issued before now for + * that account. Used by logout, password change, TOTP disable, account + * deactivation, and the admin "revoke sessions" action. + * + * Returns the new version so callers that also sign a fresh token (e.g. + * password-change-while-staying-logged-in flows, if ever added) can embed it. + */ +export async function bumpTokenVersion(userId: number): Promise { + const rows = await query<{ token_version: number }>( + `UPDATE users SET token_version = token_version + 1 WHERE id = $1 RETURNING token_version`, + [userId] + ); + cache.delete(userId); // don't serve a stale version for the next CACHE_TTL_MS + return rows[0]?.token_version ?? 0; +} + +/** Test seam: clear the cache between tests / after direct DB mutation in tests. */ +export function _resetSessionCacheForTest(): void { + cache.clear(); +} diff --git a/backend/src/utils/siteScope.ts b/backend/src/utils/siteScope.ts index 958735b..79706ed 100644 --- a/backend/src/utils/siteScope.ts +++ b/backend/src/utils/siteScope.ts @@ -1,22 +1,35 @@ /** - * Site scoping helpers (issue #130). + * Site scoping helpers (issue #130), extended for per-user authorization in + * Mission 02 (SEC-002). * * Every device-derived table hangs off devices(id), so a single site_id on * devices scopes clients, events, topology, backups and the rest transitively. - * These helpers turn an active site into the SQL predicate that does it. + * These helpers turn an *effective* site scope into the SQL predicate that + * does it. * - * The site id is inlined rather than parameterised, and that is a deliberate + * A scope is one of: + * - `null` unrestricted (the whole fleet) -- admins and API tokens. + * - `number` exactly one site -- either an explicit selection the caller + * has already been authorized for (middleware/site.ts), or a + * restricted user's only accessible site. + * - `number[]` a restricted user viewing "all sites" (no X-Site-Id sent): + * scoped to every site they're a member of, which may be + * empty -- meaning no access to anything, not "everything". + * + * The id(s) are inlined rather than parameterised, and that is a deliberate * choice worth justifying: the collection routes build their WHERE clauses by * incrementing a shared $n counter, so injecting a parameter mid-build would * mean renumbering every filter in ~23 routes -- exactly the kind of manual * bookkeeping that produces silent off-by-one bugs. Inlining is safe here - * because resolveSiteId() below admits nothing but a positive integer; a value - * that is not one becomes null (unscoped), never text that reaches SQL. + * because every id passing through this module is validated as a positive + * integer first (resolveSiteId, or read back from user_sites via a parameterised + * query) -- never raw text reaching SQL. */ +export type SiteScope = number | number[] | null; /** * Normalise an untrusted site id (header, query string) into either a positive - * integer or null, where null means "no scoping -- show the whole fleet". + * integer or null, where null means "no explicit selection". * * 'all' is the explicit all-sites view. Anything unparseable is also treated as * unscoped rather than rejected, so an existing API consumer that knows nothing @@ -27,7 +40,7 @@ export function resolveSiteId(raw: unknown): number | null { const s = String(raw).trim(); if (s === '' || s.toLowerCase() === 'all') return null; // Matched as a literal digit string rather than via Number(), deliberately. - // Number() would accept '1e3', '0x1' and even Arabic-Indic '\u0661\u0662' as + // Number() would accept '1e3', '0x1' and even Arabic-Indic '١٢' as // integers -- all harmless here, but it makes the safety of inlining rest on // coercion trivia. A digits-only regex makes it self-evident instead. if (!/^\d+$/.test(s)) return null; @@ -36,23 +49,41 @@ export function resolveSiteId(raw: unknown): number | null { return n; } +/** + * Collapse a scope to a single concrete site id, or null. For an unrestricted + * caller (null) or a restricted user viewing several sites at once (array), + * there is no single "the site this belongs to" answer -- callers that need + * one (e.g. "which site does a newly created device join") get null and fall + * back to their own default-site behavior, same as an admin always has. + */ +export function singleSiteOrNull(scope: SiteScope): number | null { + return typeof scope === 'number' ? scope : null; +} + +function idList(ids: number[]): string { + // An empty IN () is invalid SQL and, more importantly, its intent ("match + // nothing") is easy to get backwards. Spell it out instead. + return ids.length ? `(${ids.join(',')})` : '(NULL)'; +} + /** * Predicate for a query that selects from `devices` itself. * Returns null when unscoped, so callers can skip pushing a filter. */ -export function siteScopeDevices(siteId: number | null, alias = ''): string | null { - if (siteId === null) return null; +export function siteScopeDevices(scope: SiteScope, alias = ''): string | null { + if (scope === null) return null; const col = alias ? `${alias}.site_id` : 'site_id'; - return `${col} = ${siteId}`; + return Array.isArray(scope) ? `${col} IN ${idList(scope)}` : `${col} = ${scope}`; } /** * Predicate for a query on any table carrying a device_id. * `column` is the fully-qualified device id column, e.g. 'c.device_id'. */ -export function siteScopeByDevice(siteId: number | null, column = 'device_id'): string | null { - if (siteId === null) return null; - return `${column} IN (SELECT id FROM devices WHERE site_id = ${siteId})`; +export function siteScopeByDevice(scope: SiteScope, column = 'device_id'): string | null { + if (scope === null) return null; + const inner = Array.isArray(scope) ? `site_id IN ${idList(scope)}` : `site_id = ${scope}`; + return `${column} IN (SELECT id FROM devices WHERE ${inner})`; } /** @@ -61,9 +92,10 @@ export function siteScopeByDevice(siteId: number | null, column = 'device_id'): * site is selected -- showing them in every site would misrepresent them as * that site's own. */ -export function siteScopeByNullableDevice(siteId: number | null, column = 'device_id'): string | null { - if (siteId === null) return null; - return `(${column} IS NOT NULL AND ${column} IN (SELECT id FROM devices WHERE site_id = ${siteId}))`; +export function siteScopeByNullableDevice(scope: SiteScope, column = 'device_id'): string | null { + if (scope === null) return null; + const inner = Array.isArray(scope) ? `site_id IN ${idList(scope)}` : `site_id = ${scope}`; + return `(${column} IS NOT NULL AND ${column} IN (SELECT id FROM devices WHERE ${inner}))`; } /** @@ -87,6 +119,8 @@ export function andSite(base: string, predicate: string | null): string { * * When a site holds every device, its total *is* the global total -- an * identity, not an approximation -- so `_global` is both correct and complete. + * Only meaningful for a single explicit site; a restricted multi-site scope + * has no single tag and callers should fall back to per-site queries instead. */ export function clientSeriesTag( siteId: number | null,