diff --git a/src/web/route-helpers.ts b/src/web/route-helpers.ts index 75a92fcb..15966e7b 100644 --- a/src/web/route-helpers.ts +++ b/src/web/route-helpers.ts @@ -324,11 +324,33 @@ function sessionNotFoundError(sessionId: string): Error & { statusCode: number; * Look up a session by ID or throw a structured error. * Replaces the pattern: `const session = sessions.get(id); if (!session) return createErrorResponse(...)`. * - * When `req` is passed in multi-user mode, a session the caller does not own is - * reported as NOT_FOUND (never 403), so existence of other users' sessions is not - * leaked. Single-user / admin callers are unaffected. + * In multi-user mode a session the caller does not own is reported as NOT_FOUND + * (never 403), so existence of other users' sessions is not leaked. Single-user / + * admin callers are unaffected (`canAccessOwned` short-circuits when multi-user is off). + * + * `req` is REQUIRED. When it was optional, forgetting it compiled cleanly and + * silently performed an UNSCOPED lookup. A caller with no request in flight must + * say so at the call site via {@link findSessionUnscopedOrFail}. */ -export function findSessionOrFail(ctx: SessionPort, sessionId: string, req?: FastifyRequest): Session { +export function findSessionOrFail(ctx: SessionPort, sessionId: string, req: FastifyRequest): Session { + return lookupSessionOrFail(ctx, sessionId, req); +} + +/** + * Look up a session WITHOUT ownership scoping. Deliberately verbose and greppable: + * every call is a decision to skip the multi-user ownership check and should carry + * a comment saying why (e.g. a startup path with no request in flight). Inside a + * request handler use `findSessionOrFail(ctx, id, req)` instead. + * + * It has no production callers today: every lookup in `src/` is request-scoped. It + * exists so the next genuinely request-less caller has somewhere to go other than + * making `req` optional again. + */ +export function findSessionUnscopedOrFail(ctx: SessionPort, sessionId: string): Session { + return lookupSessionOrFail(ctx, sessionId, undefined); +} + +function lookupSessionOrFail(ctx: SessionPort, sessionId: string, req: FastifyRequest | undefined): Session { const session = ctx.sessions.get(sessionId); if (!session) throw sessionNotFoundError(sessionId); if (req && !canAccessOwned(getAuthUser(req), session.owner)) throw sessionNotFoundError(sessionId); diff --git a/test/ownership-scoping.test.ts b/test/ownership-scoping.test.ts index 986e9500..62060c0b 100644 --- a/test/ownership-scoping.test.ts +++ b/test/ownership-scoping.test.ts @@ -10,10 +10,16 @@ import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import fs from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; +import ts from 'typescript'; import { WebServer } from '../src/web/server.js'; import { TmuxManager } from '../src/tmux-manager.js'; import { createUser, invalidateUsersCache } from '../src/user-store.js'; -import { canAccessOwned, findSessionOrFail, sessionCapacityState } from '../src/web/route-helpers.js'; +import { + canAccessOwned, + findSessionOrFail, + findSessionUnscopedOrFail, + sessionCapacityState, +} from '../src/web/route-helpers.js'; vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true); @@ -154,6 +160,26 @@ describe('session-scoping helpers (multi-user)', () => { expect(findSessionOrFail(ctxWith(map), 's1', reqAs('root', 'admin'))).toBeDefined(); }); + it('findSessionUnscopedOrFail is the explicit unscoped lookup: returns a foreign session, 404s an unknown id', () => { + const map = new Map([['s1', fakeSession('alice')]]); + expect(findSessionUnscopedOrFail(ctxWith(map), 's1')).toBeDefined(); + expect(() => findSessionUnscopedOrFail(ctxWith(map), 'missing')).toThrow(); + }); + + it('findSessionOrFail declares req as REQUIRED, so an unscoped call must use the explicit helper', async () => { + // tsc does not typecheck test/, so pin the signature from the source AST: an + // optional `req` would make forgetting it compile into an UNSCOPED lookup. + const file = path.join(import.meta.dirname, '../src/web/route-helpers.ts'); + const src = ts.createSourceFile(file, await fs.readFile(file, 'utf8'), ts.ScriptTarget.Latest, true); + const decl = src.statements.find( + (s): s is ts.FunctionDeclaration => ts.isFunctionDeclaration(s) && s.name?.text === 'findSessionOrFail' + ); + const req = decl?.parameters.find((p) => p.name.getText(src) === 'req'); + expect(req).toBeDefined(); + expect(req?.questionToken).toBeUndefined(); + expect(req?.initializer).toBeUndefined(); + }); + it('per-user session cap counts only the owner sessions', () => { const map = new Map([ ['a', fakeSession('alice')],