From 24a1c6c4277d6ee8159199e0d138cac90b01932b Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 16 Oct 2026 13:11:41 +0200 Subject: [PATCH] fix(multi-user): make the server-wide system operations admin-only docs/multi-user-plan.md lists the system operations as admin-only, but these routes had no role check, so any signed-in regular user could run them in multi-user mode: - POST /api/auth/revoke with an empty body cleared EVERY login session on the server, every admin's included. A non-admin now revokes only their own logins, and a named sessionToken only when it is one of theirs. - POST /api/system/update (update and restart the server for everyone), POST /api/system/span-displays (a window on the server's desktop), POST /api/tunnel/qr/regenerate (clears every user's pending QR login code) and POST /api/cleanup-state (drops every user's stale session records) now answer 403 FORBIDDEN to a non-admin. - GET /api/tunnel/info lists only a non-admin's own logins (it showed the address and browser of every user's sessions). Admins and single-user mode are unchanged. The tunnel panel's Regenerate QR button no longer reports success for a refused request. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/web/public/settings-ui.js | 6 +- src/web/routes/system-routes.ts | 38 +++- ...ystem-routes-system-ops-admin-only.test.ts | 215 ++++++++++++++++++ test/settings-admin-only-multiuser.test.ts | 27 ++- 4 files changed, 279 insertions(+), 7 deletions(-) create mode 100644 test/routes/system-routes-system-ops-admin-only.test.ts diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 00b6ed2c..cbc5ab21 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -1884,8 +1884,12 @@ Object.assign(CodemanApp.prototype, { regenBtn.textContent = 'Regenerate QR'; regenBtn.style.cssText = 'margin-top:8px;padding:4px 12px;background:var(--bg-elevated);border:1px solid var(--border);border-radius:4px;color:var(--text-secondary);cursor:pointer;font-size:11px'; regenBtn.onclick = () => { + // A refusal is not a success: in multi-user mode this is admin-only (403). fetch('/api/tunnel/qr/regenerate', { method: 'POST' }) - .then(() => this.showToast('QR code regenerated', 'success')) + .then((res) => { + if (!res.ok) throw new Error(`HTTP ${res.status}`); + this.showToast('QR code regenerated', 'success'); + }) .catch(() => this.showToast('Failed to regenerate QR', 'error')); }; const card = container.parentElement; diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index a8dc9e7e..e71c8d03 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -17,7 +17,7 @@ import { ApiErrorCode, createErrorResponse, getErrorMessage, type NiceConfig } f import { isUnauthenticatedNetworkAcknowledged } from '../network-auth-policy.js'; import { isMultiUserMode } from '../../config/multiuser.js'; import { findUser, canUsernameRunPrivilegedCommands } from '../../user-store.js'; -import { getAuthUser, isAdmin, requireAdmin, canAccessOwned } from '../route-helpers.js'; +import { getAuthUser, isAdmin, requireAdmin, canAccessOwned, revokeUserSessions } from '../route-helpers.js'; import { ConfigUpdateSchema, SettingsUpdateSchema, @@ -185,12 +185,17 @@ export function registerSystemRoutes( app.get('/api/tunnel/status', async () => ctx.tunnelManager.getStatus()); - app.get('/api/tunnel/info', async () => { + app.get('/api/tunnel/info', async (req) => { const status = ctx.tunnelManager.getStatus(); const sseClients = ctx.getSseClientCount(); const sessions: Array<{ ip: string; ua: string; createdAt: number; method: string }> = []; + // Multi-user mode: a non-admin is shown only their own logins (address and browser of + // every other user's sessions are not theirs to read), matching what their Revoke All + // can revoke (POST /api/auth/revoke). null = everyone's (admin, single-user mode). + const onlyUser = isMultiUserMode() && !isAdmin(req) ? getAuthUser(req).username : null; if (ctx.authSessions) { for (const [, record] of ctx.authSessions) { + if (onlyUser !== null && record.username !== onlyUser) continue; sessions.push({ ip: record.ip, ua: record.ua, createdAt: record.createdAt, method: record.method }); } } @@ -322,7 +327,10 @@ export function registerSystemRoutes( // ========== QR Regeneration ========== - app.post('/api/tunnel/qr/regenerate', async () => { + app.post('/api/tunnel/qr/regenerate', async (req, reply) => { + // Clears EVERY outstanding QR login code (each user's minted one too) and rotates the + // server's token: admin-only in multi-user mode, like the other tunnel controls. + if (isMultiUserMode() && !requireAdmin(req, reply)) return; ctx.tunnelManager.regenerateQrToken(); return {}; }); @@ -331,6 +339,19 @@ export function registerSystemRoutes( app.post('/api/auth/revoke', async (req) => { const result = RevokeSessionSchema.safeParse(req.body); + // Multi-user mode: a NON-ADMIN revokes only their own login sessions. The bare form + // below clears every session on the server, so for them it used to sign out every + // user, admins included; a named token is revoked only when it is one of theirs. + if (isMultiUserMode() && !isAdmin(req)) { + const username = getAuthUser(req).username; + const token = result.success ? result.data.sessionToken : undefined; + if (token) { + if (ctx.authSessions?.peek(token)?.username === username) ctx.authSessions?.delete(token); + } else { + revokeUserSessions(ctx.authSessions, username); + } + return {}; + } if (result.success && result.data.sessionToken) { ctx.authSessions?.delete(result.data.sessionToken); } else { @@ -349,6 +370,8 @@ export function registerSystemRoutes( // panels can be dragged across the physical monitor seam. macOS only; needs // the one-time "Displays have separate Spaces" OFF prerequisite (see script). app.post('/api/system/span-displays', async (req, reply) => { + // Launches a browser window on the SERVER's desktop: admin-only in multi-user mode. + if (isMultiUserMode() && !requireAdmin(req, reply)) return; // macOS only: the launcher uses osascript + Finder desktop bounds and Chrome // --app geometry flags. Fail clearly elsewhere instead of spawning a bash // that errors out invisibly (the toast would otherwise lie "Opening…"). @@ -401,7 +424,9 @@ export function registerSystemRoutes( // Kick off a detached update to the latest release. Returns immediately; the // browser then polls /api/system/update/status across the service restart. - app.post('/api/system/update', async (_req, reply) => { + app.post('/api/system/update', async (req, reply) => { + // Updates and restarts the server under every user: admin-only in multi-user mode. + if (isMultiUserMode() && !requireAdmin(req, reply)) return; const result = await startUpdate(); if (result.ok) { return { updateId: result.updateId, toTag: result.toTag, toVersion: result.toVersion }; @@ -743,7 +768,10 @@ export function registerSystemRoutes( // ========== State & Lifecycle ========== - app.post('/api/cleanup-state', async () => { + app.post('/api/cleanup-state', async (req, reply) => { + // Drops every user's stale session records (and their tab-layout entries): admin-only + // in multi-user mode, like the other server-wide maintenance operations. + if (isMultiUserMode() && !requireAdmin(req, reply)) return; const activeSessionIds = new Set(ctx.sessions.keys()); const result = await ctx.tabLayouts.runStaleSessionCleanup(activeSessionIds, (ids) => ctx.store.cleanupSessionsByIds(ids) diff --git a/test/routes/system-routes-system-ops-admin-only.test.ts b/test/routes/system-routes-system-ops-admin-only.test.ts new file mode 100644 index 00000000..ae905e15 --- /dev/null +++ b/test/routes/system-routes-system-ops-admin-only.test.ts @@ -0,0 +1,215 @@ +/** + * @fileoverview Multi-user mode: the server-wide system operations are admin-only. + * + * docs/multi-user-plan.md lists the system operations (self-update, the tunnel, + * span-displays) as admin-only, but these routes had no role check, so any signed-in + * NON-ADMIN could run them. Pinned here: + * - POST /api/system/update, POST /api/system/span-displays, POST /api/tunnel/qr/regenerate + * and POST /api/cleanup-state answer 403 FORBIDDEN to a non-admin, and nothing runs; + * - POST /api/auth/revoke: a non-admin revokes only their OWN login sessions (the bare + * form used to clear every session on the server, every admin's included), and a named + * token only when it is one of theirs; + * - GET /api/tunnel/info lists only the caller's own logins for a non-admin; + * - an admin, and single-user mode, behave as before. + * + * Uses app.inject(), so no real HTTP port is needed. Port: N/A. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { createRouteTestHarness, type RouteTestHarness } from './_route-test-utils.js'; +import { registerSystemRoutes } from '../../src/web/routes/system-routes.js'; +import { StaleExpirationMap } from '../../src/utils/stale-expiration-map.js'; +import type { AuthSessionRecord } from '../../src/web/ports/auth-port.js'; + +const { startUpdate } = vi.hoisted(() => ({ + startUpdate: vi.fn(async () => ({ ok: true, updateId: 'u1', toTag: 'v9.9.9', toVersion: '9.9.9' })), +})); + +vi.mock('../../src/web/self-update.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, startUpdate }; +}); + +const NON_ADMIN = { username: 'bob', role: 'user' } as const; +const ADMIN = { username: 'root', role: 'admin' } as const; + +function makeTunnelManager() { + return { + regenerateQrToken: vi.fn(), + isRunning: vi.fn(() => true), + getUrl: vi.fn(() => 'https://example.trycloudflare.com'), + getStatus: vi.fn(() => ({ running: true, url: 'https://example.trycloudflare.com' })), + }; +} + +function record(username: string | undefined, ip: string): AuthSessionRecord { + return { + ip, + ua: 'test-agent', + createdAt: 1, + method: 'basic', + username, + role: username === 'root' ? 'admin' : 'user', + }; +} + +describe('server-wide system operations are admin-only in multi-user mode', () => { + const savedMultiUser = process.env.CODEMAN_MULTIUSER; + let harness: RouteTestHarness | undefined; + let tunnel: ReturnType; + let sessions: StaleExpirationMap; + + async function as(user: typeof NON_ADMIN | typeof ADMIN | null) { + if (user) process.env.CODEMAN_MULTIUSER = '1'; + else delete process.env.CODEMAN_MULTIUSER; + harness = await createRouteTestHarness(registerSystemRoutes, user ? { authUser: user } : undefined); + tunnel = makeTunnelManager(); + sessions = new StaleExpirationMap({ ttlMs: 60_000 }); + // Two logins of the non-admin, one of an admin, one single-user (no identity). + sessions.set('bob-laptop', record('bob', '10.0.0.2')); + sessions.set('bob-phone', record('bob', '10.0.0.3')); + sessions.set('root-desk', record('root', '10.0.0.1')); + Object.assign(harness.ctx as unknown as Record, { tunnelManager: tunnel, authSessions: sessions }); + return harness; + } + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(async () => { + await harness?.app.close(); + harness = undefined; + sessions?.dispose(); + if (savedMultiUser === undefined) delete process.env.CODEMAN_MULTIUSER; + else process.env.CODEMAN_MULTIUSER = savedMultiUser; + }); + + const forbidden = (res: { statusCode: number; body: string }) => { + expect(res.statusCode).toBe(403); + const body = JSON.parse(res.body); + expect(body.success).toBe(false); + expect(body.errorCode).toBe('FORBIDDEN'); + }; + + describe('POST /api/system/update', () => { + it('refuses a non-admin, and no update starts', async () => { + const { app } = await as(NON_ADMIN); + forbidden(await app.inject({ method: 'POST', url: '/api/system/update', payload: {} })); + expect(startUpdate).not.toHaveBeenCalled(); + }); + + for (const [label, user] of [ + ['an admin in multi-user mode', ADMIN], + ['single-user mode', null], + ] as const) { + it(`starts it for ${label}`, async () => { + const { app } = await as(user); + const res = await app.inject({ method: 'POST', url: '/api/system/update', payload: {} }); + expect(res.statusCode).toBe(200); + expect(startUpdate).toHaveBeenCalledTimes(1); + }); + } + }); + + describe('POST /api/system/span-displays', () => { + it('refuses a non-admin before anything else', async () => { + const { app } = await as(NON_ADMIN); + forbidden(await app.inject({ method: 'POST', url: '/api/system/span-displays' })); + }); + + it('lets an admin through the gate (off macOS the route then answers its own 400)', async () => { + const { app } = await as(ADMIN); + const res = await app.inject({ method: 'POST', url: '/api/system/span-displays' }); + if (process.platform === 'darwin') expect(res.statusCode).not.toBe(403); + else expect(res.statusCode).toBe(400); + }); + }); + + describe('POST /api/tunnel/qr/regenerate', () => { + it("refuses a non-admin, and nobody's QR codes are cleared", async () => { + const { app } = await as(NON_ADMIN); + forbidden(await app.inject({ method: 'POST', url: '/api/tunnel/qr/regenerate' })); + expect(tunnel.regenerateQrToken).not.toHaveBeenCalled(); + }); + + for (const [label, user] of [ + ['an admin in multi-user mode', ADMIN], + ['single-user mode', null], + ] as const) { + it(`regenerates for ${label}`, async () => { + const { app } = await as(user); + const res = await app.inject({ method: 'POST', url: '/api/tunnel/qr/regenerate' }); + expect(res.statusCode).toBe(200); + expect(tunnel.regenerateQrToken).toHaveBeenCalledTimes(1); + }); + } + }); + + describe('POST /api/cleanup-state', () => { + it("refuses a non-admin, and no user's session records are touched", async () => { + const { app, ctx } = await as(NON_ADMIN); + forbidden(await app.inject({ method: 'POST', url: '/api/cleanup-state' })); + expect(ctx.tabLayouts.runStaleSessionCleanup).not.toHaveBeenCalled(); + expect(ctx.store.cleanupSessionsByIds).not.toHaveBeenCalled(); + }); + + it('runs for an admin', async () => { + const { app, ctx } = await as(ADMIN); + const res = await app.inject({ method: 'POST', url: '/api/cleanup-state' }); + expect(res.statusCode).toBe(200); + expect(ctx.tabLayouts.runStaleSessionCleanup).toHaveBeenCalledTimes(1); + }); + }); + + describe('POST /api/auth/revoke', () => { + const tokens = () => [...sessions.keys()].sort(); + + it('the bare form signs a non-admin out of their own sessions only, never anyone else', async () => { + const { app } = await as(NON_ADMIN); + const res = await app.inject({ method: 'POST', url: '/api/auth/revoke', payload: {} }); + expect(res.statusCode).toBe(200); + expect(tokens()).toEqual(['root-desk']); + }); + + it("a non-admin's named token is revoked only when it is theirs", async () => { + const { app } = await as(NON_ADMIN); + await app.inject({ method: 'POST', url: '/api/auth/revoke', payload: { sessionToken: 'root-desk' } }); + expect(tokens()).toEqual(['bob-laptop', 'bob-phone', 'root-desk']); + await app.inject({ method: 'POST', url: '/api/auth/revoke', payload: { sessionToken: 'bob-phone' } }); + expect(tokens()).toEqual(['bob-laptop', 'root-desk']); + }); + + for (const [label, user] of [ + ['an admin in multi-user mode', ADMIN], + ['single-user mode', null], + ] as const) { + it(`the bare form still clears every session for ${label}, a named token any session`, async () => { + const { app } = await as(user); + await app.inject({ method: 'POST', url: '/api/auth/revoke', payload: { sessionToken: 'bob-phone' } }); + expect(tokens()).toEqual(['bob-laptop', 'root-desk']); + const res = await app.inject({ method: 'POST', url: '/api/auth/revoke', payload: {} }); + expect(res.statusCode).toBe(200); + expect(tokens()).toEqual([]); + }); + } + }); + + describe('GET /api/tunnel/info', () => { + const ips = (res: { body: string }) => + (JSON.parse(res.body).authSessions as { ip: string }[]).map((s) => s.ip).sort(); + + it("lists only the non-admin's own logins", async () => { + const { app } = await as(NON_ADMIN); + const res = await app.inject({ method: 'GET', url: '/api/tunnel/info' }); + expect(res.statusCode).toBe(200); + expect(ips(res)).toEqual(['10.0.0.2', '10.0.0.3']); + }); + + it('lists every login for an admin', async () => { + const { app } = await as(ADMIN); + const res = await app.inject({ method: 'GET', url: '/api/tunnel/info' }); + expect(ips(res)).toEqual(['10.0.0.1', '10.0.0.2', '10.0.0.3']); + }); + }); +}); diff --git a/test/settings-admin-only-multiuser.test.ts b/test/settings-admin-only-multiuser.test.ts index 8e700bf4..e6c18120 100644 --- a/test/settings-admin-only-multiuser.test.ts +++ b/test/settings-admin-only-multiuser.test.ts @@ -11,7 +11,9 @@ * key and case names (an admin still reads the file as it is); * - a non-admin's quick-start does not record its case as `lastUsedCase` (a key every * user can read through GET /api/settings), while an admin's still does; - * - an admin's PUT /api/settings still writes. + * - an admin's PUT /api/settings still writes; + * - a non-admin's bare POST /api/auth/revoke signs out only their own logins: an admin's + * cookie still works afterwards (it used to clear every login on the server). */ import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; @@ -143,6 +145,29 @@ describe('a non-admin and the server-wide settings.json', () => { }); }); +describe("a non-admin's Revoke All", () => { + /** Sign in with Basic auth and keep only the session cookie it mints. */ + async function cookieFor(headers: Record) { + const res = await fetch(url('/api/status'), { headers }); + expect(res.status).toBe(200); + const cookie = (res.headers.get('set-cookie') || '').split(';')[0]; + expect(cookie).toMatch(/^codeman_session=/); + return { Cookie: cookie }; + } + + it("signs out only the non-admin's own logins, never an admin's", async () => { + const rootCookie = await cookieFor(root); + const bobCookie = await cookieFor(bob); + expect((await fetch(url('/api/status'), { headers: rootCookie })).status).toBe(200); + + const res = await send('POST', '/api/auth/revoke', bobCookie, {}); + expect(res.status).toBe(200); + + expect((await fetch(url('/api/status'), { headers: rootCookie })).status).toBe(200); + expect((await fetch(url('/api/status'), { headers: bobCookie })).status).toBe(401); + }); +}); + describe('an admin and the server-wide settings.json', () => { it('still writes it', async () => { const res = await send('PUT', '/api/settings', root, { autoNameSessions: true });