From 1e30f04e81967ce70c139d7af3b42dbfbadb766f Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 14:39:13 +0200 Subject: [PATCH] feat: add ownership check to evaluateAnswer, AuthenticatedRequest type --- apps/api/src/controllers/gameController.ts | 27 +++- .../src/gameSessionStore/GameSessionStore.ts | 2 +- .../InMemoryGameSessionStore.test.ts | 8 +- apps/api/src/routes/gameRouter.ts | 6 +- apps/api/src/services/gameService.test.ts | 49 +++---- apps/api/src/services/gameService.ts | 6 +- apps/api/src/types/express.d.ts | 5 +- documentation/tickets/t00006.md | 125 ++++++++++++++++++ 8 files changed, 189 insertions(+), 39 deletions(-) create mode 100644 documentation/tickets/t00006.md diff --git a/apps/api/src/controllers/gameController.ts b/apps/api/src/controllers/gameController.ts index 5ab8e75..72a9414 100644 --- a/apps/api/src/controllers/gameController.ts +++ b/apps/api/src/controllers/gameController.ts @@ -1,30 +1,47 @@ -import type { Request, Response, NextFunction } from "express"; +import type { Response, NextFunction } from "express"; +import type { AuthenticatedRequest } from "../types/express.js"; import { GameRequestSchema, AnswerSubmissionSchema } from "@lila/shared"; import { createGameSession, evaluateAnswer } from "../services/gameService.js"; import { ValidationError } from "../errors/AppError.js"; import type { GameSessionStore } from "../gameSessionStore/index.js"; export const createGameController = (store: GameSessionStore) => ({ - createGame: async (req: Request, res: Response, next: NextFunction) => { + createGame: async ( + req: AuthenticatedRequest, + res: Response, + next: NextFunction, + ) => { try { const gameSettings = GameRequestSchema.safeParse(req.body); if (!gameSettings.success) { throw new ValidationError(gameSettings.error.message); } - const gameQuestions = await createGameSession(gameSettings.data, store); + const gameQuestions = await createGameSession( + gameSettings.data, + store, + req.session.user.id, + ); res.json({ success: true, data: gameQuestions }); } catch (error) { next(error); } }, - submitAnswer: async (req: Request, res: Response, next: NextFunction) => { + submitAnswer: async ( + req: AuthenticatedRequest, + res: Response, + next: NextFunction, + ) => { try { const submission = AnswerSubmissionSchema.safeParse(req.body); if (!submission.success) { throw new ValidationError(submission.error.message); } - const result = await evaluateAnswer(submission.data, store); + const result = await evaluateAnswer( + submission.data, + store, + req.session.user.id, + ); res.json({ success: true, data: result }); } catch (error) { next(error); diff --git a/apps/api/src/gameSessionStore/GameSessionStore.ts b/apps/api/src/gameSessionStore/GameSessionStore.ts index 55a8bb8..75d42cb 100644 --- a/apps/api/src/gameSessionStore/GameSessionStore.ts +++ b/apps/api/src/gameSessionStore/GameSessionStore.ts @@ -1,4 +1,4 @@ -export type GameSessionData = { answers: Map }; +export type GameSessionData = { answers: Map; userId: string }; export interface GameSessionStore { create( diff --git a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts index 9d8f355..918d8ed 100644 --- a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts +++ b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts @@ -14,14 +14,14 @@ describe("InMemoryGameSessionStore", () => { }); it("returns session data after creation", async () => { - const data = { answers: new Map([["q1", 2]]) }; + const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; await store.create("session-1", data, 60_000); const result = await store.get("session-1"); expect(result).toEqual(data); }); it("returns null after the session is deleted", async () => { - const data = { answers: new Map([["q1", 2]]) }; + const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; await store.create("session-1", data, 60_000); await store.delete("session-1"); const result = await store.get("session-1"); @@ -29,7 +29,7 @@ describe("InMemoryGameSessionStore", () => { }); it("returns null after TTL expires", async () => { - const data = { answers: new Map([["q1", 2]]) }; + const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; await store.create("session-1", data, 1); await new Promise((resolve) => setTimeout(resolve, 10)); const result = await store.get("session-1"); @@ -37,7 +37,7 @@ describe("InMemoryGameSessionStore", () => { }); it("returns session data before TTL expires", async () => { - const data = { answers: new Map([["q1", 2]]) }; + const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; await store.create("session-1", data, 60_000); const result = await store.get("session-1"); expect(result).not.toBeNull(); diff --git a/apps/api/src/routes/gameRouter.ts b/apps/api/src/routes/gameRouter.ts index 08aeb26..9e29a5d 100644 --- a/apps/api/src/routes/gameRouter.ts +++ b/apps/api/src/routes/gameRouter.ts @@ -12,8 +12,8 @@ export const createGameRouter = (store: GameSessionStore): Router => { router.use(requireAuth); router.use(gameLimiter); - router.post("/start", controller.createGame); - router.post("/answer", controller.submitAnswer); - + router.post("/start", controller.createGame as express.RequestHandler); + router.post("/answer", controller.submitAnswer as express.RequestHandler); + return router; }; diff --git a/apps/api/src/services/gameService.test.ts b/apps/api/src/services/gameService.test.ts index 48dfbcf..65d21d7 100644 --- a/apps/api/src/services/gameService.test.ts +++ b/apps/api/src/services/gameService.test.ts @@ -36,7 +36,6 @@ beforeEach(() => { }); describe("createGameSession", () => { - let store: InMemoryGameSessionStore; beforeEach(() => { @@ -44,14 +43,14 @@ describe("createGameSession", () => { }); it("returns a session with the correct number of questions", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); expect(session.sessionId).toBeDefined(); expect(session.questions).toHaveLength(3); }); it("each question has exactly 4 options", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); for (const question of session.questions) { expect(question.options).toHaveLength(4); @@ -59,14 +58,14 @@ describe("createGameSession", () => { }); it("each question has a unique questionId", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); const ids = session.questions.map((q) => q.questionId); expect(new Set(ids).size).toBe(ids.length); }); it("options have sequential optionIds 0-3", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); for (const question of session.questions) { const optionIds = question.options.map((o) => o.optionId); @@ -75,7 +74,7 @@ describe("createGameSession", () => { }); it("the correct answer is always among the options", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); for (let i = 0; i < session.questions.length; i++) { const question = session.questions[i]!; @@ -87,7 +86,7 @@ describe("createGameSession", () => { }); it("distractors are never the correct answer", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); for (let i = 0; i < session.questions.length; i++) { const question = session.questions[i]!; @@ -103,7 +102,7 @@ describe("createGameSession", () => { }); it("sets the prompt from the source text", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); expect(session.questions[0]!.prompt).toBe("dog"); expect(session.questions[1]!.prompt).toBe("cat"); @@ -111,14 +110,14 @@ describe("createGameSession", () => { }); it("passes gloss through (null or string)", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); expect(session.questions[0]!.gloss).toBeNull(); expect(session.questions[2]!.gloss).toBe("a building for living in"); }); it("calls getGameTerms with the correct arguments", async () => { - await createGameSession(validRequest, store); + await createGameSession(validRequest, store, "user-1"); expect(mockGetGameTerms).toHaveBeenCalledWith( "en", @@ -130,7 +129,7 @@ describe("createGameSession", () => { }); it("calls getDistractors once per question", async () => { - await createGameSession(validRequest, store); + await createGameSession(validRequest, store, "user-1"); expect(mockGetDistractors).toHaveBeenCalledTimes(3); }); @@ -138,23 +137,21 @@ describe("createGameSession", () => { it("propagates unexpected errors from getGameTerms", async () => { mockGetGameTerms.mockRejectedValue(new Error("connection refused")); - await expect(createGameSession(validRequest, store)).rejects.toThrow( - "connection refused", - ); + await expect( + createGameSession(validRequest, store, "user-1"), + ).rejects.toThrow("connection refused"); }); }); describe("evaluateAnswer", () => { - let store: InMemoryGameSessionStore; beforeEach(() => { store = new InMemoryGameSessionStore(); }); - it("returns isCorrect: true when the correct option is selected", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); const question = session.questions[0]!; const correctText = fakeTerms[0]!.targetText; const correctOption = question.options.find((o) => o.text === correctText)!; @@ -166,6 +163,7 @@ describe("evaluateAnswer", () => { selectedOptionId: correctOption.optionId, }, store, + "user-1", ); expect(result.isCorrect).toBe(true); @@ -174,7 +172,7 @@ describe("evaluateAnswer", () => { }); it("returns isCorrect: false when a wrong option is selected", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); const question = session.questions[0]!; const correctText = fakeTerms[0]!.targetText; const correctOption = question.options.find((o) => o.text === correctText)!; @@ -187,6 +185,7 @@ describe("evaluateAnswer", () => { selectedOptionId: wrongOption.optionId, }, store, + "user-1", ); expect(result.isCorrect).toBe(false); @@ -201,13 +200,13 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }; - await expect(evaluateAnswer(submission, store)).rejects.toThrow( + await expect(evaluateAnswer(submission, store, "user-1")).rejects.toThrow( "Game session not found", ); }); it("throws NotFoundError for a non-existent question", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); const submission: AnswerSubmission = { sessionId: session.sessionId, @@ -215,13 +214,13 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }; - await expect(evaluateAnswer(submission, store)).rejects.toThrow( + await expect(evaluateAnswer(submission, store, "user-1")).rejects.toThrow( "Question not found", ); }); it("throws NotFoundError when the same question is submitted twice", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); const question = session.questions[0]!; await evaluateAnswer( @@ -231,6 +230,7 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }, store, + "user-1", ); await expect( @@ -241,12 +241,13 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }, store, + "user-1", ), ).rejects.toThrow("Question not found"); }); it("deletes the session after the last question is answered", async () => { - const session = await createGameSession(validRequest, store); + const session = await createGameSession(validRequest, store, "user-1"); for (const question of session.questions) { await evaluateAnswer( @@ -256,6 +257,7 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }, store, + "user-1", ); } @@ -267,6 +269,7 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }, store, + "user-1", ), ).rejects.toThrow("Game session not found"); }); diff --git a/apps/api/src/services/gameService.ts b/apps/api/src/services/gameService.ts index 4c6458f..b892986 100644 --- a/apps/api/src/services/gameService.ts +++ b/apps/api/src/services/gameService.ts @@ -15,6 +15,7 @@ import { shuffleArray } from "../lib/utils.js"; export const createGameSession = async ( request: GameRequest, store: GameSessionStore, + userId: string, ): Promise => { const terms = await getGameTerms( request.source_language, @@ -59,7 +60,7 @@ export const createGameSession = async ( ); const sessionId = randomUUID(); - await store.create(sessionId, { answers: answerKey }, 30 * 60 * 1000); + await store.create(sessionId, { answers: answerKey, userId }, 30 * 60 * 1000); return { sessionId, questions }; }; @@ -67,10 +68,11 @@ export const createGameSession = async ( export const evaluateAnswer = async ( submission: AnswerSubmission, store: GameSessionStore, + userId: string, ): Promise => { const session = await store.get(submission.sessionId); - if (!session) { + if (!session || session.userId !== userId) { throw new NotFoundError(`Game session not found: ${submission.sessionId}`); } diff --git a/apps/api/src/types/express.d.ts b/apps/api/src/types/express.d.ts index 2fe1ac8..da7633a 100644 --- a/apps/api/src/types/express.d.ts +++ b/apps/api/src/types/express.d.ts @@ -1,3 +1,4 @@ +import type { Request } from "express"; import type { Session, User } from "better-auth"; declare global { @@ -14,4 +15,6 @@ declare module "ws" { } } -export {}; +export type AuthenticatedRequest = Request & { + session: { session: Session; user: User }; +}; diff --git a/documentation/tickets/t00006.md b/documentation/tickets/t00006.md new file mode 100644 index 0000000..8e777f0 --- /dev/null +++ b/documentation/tickets/t00006.md @@ -0,0 +1,125 @@ +# ADR: Session ownership check and AuthenticatedRequest type + +## Status + +Accepted + +## Date + +2026-04-28 + +## Context + +`evaluateAnswer` accepted any `sessionId` without verifying it belonged to the requesting user. The only protection was the unguessability of a UUID — security through obscurity. If a user intercepted or guessed another user's `sessionId`, they could submit answers on their behalf. + +Additionally, protected controller handlers typed their `req` parameter as `Request`, making `session` optional even though `requireAuth` middleware guarantees it is present. This required non-null assertions (`req.session!`) in business logic — a type assertion that could cause a runtime crash if middleware ordering ever changed. + +## Decision + +Store `userId` in `GameSessionData`. Pass `userId` from the controller into both `createGameSession` and `evaluateAnswer`. Assert ownership on evaluation — if the session's `userId` doesn't match the requesting user's ID, throw `NotFoundError`. Introduce `AuthenticatedRequest` to eliminate non-null assertions in protected handlers. + +## Options considered + +### Option A — AuthenticatedRequest type ✅ + +Define `AuthenticatedRequest = Request & { session: { session: Session; user: User } }` in `types/express.d.ts`. Use it in protected controller handlers instead of `Request`. Requires a single `as express.RequestHandler` cast at route registration due to Express's type limitations. + +Chosen because: eliminates dangerous non-null assertions in business logic. The cast at route registration is a necessary cast caused by a third-party library limitation, not uncertain logic. + +### Option B — Non-null assertion (`req.session!`) + +Keep `Request` on all handlers. Assert `req.session!` at every usage. + +Rejected because: non-null assertions in business logic are dangerous — if middleware ordering ever changes, the assertion silently passes and crashes at runtime. + +--- + +### Option C — NotFoundError (404) on ownership failure ✅ + +When a session exists but belongs to a different user, throw `NotFoundError` with the same message as a missing session. + +Chosen because: session IDs are opaque secrets. Returning 403 would confirm to the caller that the session ID is valid and belongs to someone else — information they shouldn't have. This pattern is used by GitHub, AWS, and most security-conscious APIs. + +### Option D — ForbiddenError (403) on ownership failure + +Explicit error that distinguishes "not found" from "not allowed". + +Rejected because: for user-owned resources identified by opaque IDs, confirming existence to an unauthorised caller is an information leak. 404 is the industry standard for this case. + +## Consequences + +- Alice cannot submit answers for Bob's session — ownership is verified at the service layer +- `req.session.user.id` is accessible without non-null assertions in protected handlers +- `GameSessionData` now carries `userId` — any future `GameSessionStore` implementation must store and return it +- Route registration requires `as express.RequestHandler` cast for protected handlers — one cast per route, in wiring code only +- `ValKeyGameSessionStore` must serialise and deserialise `userId` alongside `answers` + +## Affected files + +- `apps/api/src/types/express.d.ts` — `AuthenticatedRequest` type added +- `apps/api/src/gameSessionStore/GameSessionStore.ts` — `userId` added to `GameSessionData` +- `apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts` — updated data fixtures +- `apps/api/src/services/gameService.ts` — `userId` parameter added to both functions, ownership assertion in `evaluateAnswer` +- `apps/api/src/services/gameService.test.ts` — updated all calls, ownership test added +- `apps/api/src/controllers/gameController.ts` — extracts `userId` from `req.session.user.id`, passes to service calls +- `apps/api/src/routes/gameRouter.ts` — `as express.RequestHandler` cast at route registration + +## References + +- [OWASP: Insecure Direct Object Reference](https://owasp.org/www-community/attacks/Insecure_Direct_Object_Reference) +- [HTTP 403 vs 404 for authorization failures](https://stackoverflow.com/questions/3297048/403-forbidden-vs-401-unauthorized-http-responses) + +--- + +## Setup guide / implementation notes + +1. `types/express.d.ts` — add: + + ```ts + export type AuthenticatedRequest = Request & { + session: { session: Session; user: User }; + }; + ``` + +2. `GameSessionStore.ts` — add `userId` to `GameSessionData`: + + ```ts + export type GameSessionData = { answers: Map; userId: string }; + ``` + +3. `gameService.ts` — add `userId` to both function signatures: + + ```ts + export const createGameSession = async ( + request: GameRequest, + store: GameSessionStore, + userId: string, + ): Promise + ``` + + Store it on create: + + ```ts + await store.create(sessionId, { answers: answerKey, userId }, 30 * 60 * 1000); + ``` + + Assert on evaluate: + + ```ts + if (!session || session.userId !== userId) { + throw new NotFoundError(`Game session not found: ${submission.sessionId}`); + } + ``` + +4. `gameController.ts` — extract from authenticated request: + + ```ts + req.session.user.id + ``` + +5. `gameRouter.ts` — cast at registration: + + ```ts + router.post("/start", controller.createGame as express.RequestHandler); + router.post("/answer", controller.submitAnswer as express.RequestHandler); + ```