125 lines
5.3 KiB
Markdown
125 lines
5.3 KiB
Markdown
# 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<string, number>; userId: string };
|
|
```
|
|
|
|
3. `gameService.ts` — add `userId` to both function signatures:
|
|
|
|
```ts
|
|
export const createGameSession = async (
|
|
request: GameRequest,
|
|
store: GameSessionStore,
|
|
userId: string,
|
|
): Promise<GameSession>
|
|
```
|
|
|
|
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);
|
|
```
|