From 4f9274af3a9cb2dcd6b2103c89610e5a17d40e8a Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Thu, 22 Oct 2026 13:36:41 -0400 Subject: [PATCH] fix(auth): require the request on findSessionOrFail findSessionOrFail took `req` as optional, so the quiet default was the unscoped lookup: a route that forgot to pass it compiled cleanly and, in multi-user mode, returned another user's session. `req` is now required, and a caller with no request in flight must use the explicit, greppable findSessionUnscopedOrFail instead. Every current caller in src/ already passes req (verified by grep and tsc), so behaviour is unchanged; the new helper has no production callers and exists so the next request-less caller does not make `req` optional again. Tests pin the unscoped helper's behaviour and assert, from the source AST, that `req` is a required parameter (tsc does not typecheck test/). --- src/web/route-helpers.ts | 30 ++++++++++++++++++++++++++---- test/ownership-scoping.test.ts | 28 +++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 5 deletions(-) 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')],