feat: add ownership check to evaluateAnswer, AuthenticatedRequest type
This commit is contained in:
parent
fdeb769640
commit
1e30f04e81
8 changed files with 189 additions and 39 deletions
|
|
@ -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 { GameRequestSchema, AnswerSubmissionSchema } from "@lila/shared";
|
||||||
import { createGameSession, evaluateAnswer } from "../services/gameService.js";
|
import { createGameSession, evaluateAnswer } from "../services/gameService.js";
|
||||||
import { ValidationError } from "../errors/AppError.js";
|
import { ValidationError } from "../errors/AppError.js";
|
||||||
import type { GameSessionStore } from "../gameSessionStore/index.js";
|
import type { GameSessionStore } from "../gameSessionStore/index.js";
|
||||||
|
|
||||||
export const createGameController = (store: GameSessionStore) => ({
|
export const createGameController = (store: GameSessionStore) => ({
|
||||||
createGame: async (req: Request, res: Response, next: NextFunction) => {
|
createGame: async (
|
||||||
|
req: AuthenticatedRequest,
|
||||||
|
res: Response,
|
||||||
|
next: NextFunction,
|
||||||
|
) => {
|
||||||
try {
|
try {
|
||||||
const gameSettings = GameRequestSchema.safeParse(req.body);
|
const gameSettings = GameRequestSchema.safeParse(req.body);
|
||||||
if (!gameSettings.success) {
|
if (!gameSettings.success) {
|
||||||
throw new ValidationError(gameSettings.error.message);
|
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 });
|
res.json({ success: true, data: gameQuestions });
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
next(error);
|
next(error);
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
|
||||||
submitAnswer: async (req: Request, res: Response, next: NextFunction) => {
|
submitAnswer: async (
|
||||||
|
req: AuthenticatedRequest,
|
||||||
|
res: Response,
|
||||||
|
next: NextFunction,
|
||||||
|
) => {
|
||||||
try {
|
try {
|
||||||
const submission = AnswerSubmissionSchema.safeParse(req.body);
|
const submission = AnswerSubmissionSchema.safeParse(req.body);
|
||||||
if (!submission.success) {
|
if (!submission.success) {
|
||||||
throw new ValidationError(submission.error.message);
|
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 });
|
res.json({ success: true, data: result });
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
next(error);
|
next(error);
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,4 @@
|
||||||
export type GameSessionData = { answers: Map<string, number> };
|
export type GameSessionData = { answers: Map<string, number>; userId: string };
|
||||||
|
|
||||||
export interface GameSessionStore {
|
export interface GameSessionStore {
|
||||||
create(
|
create(
|
||||||
|
|
|
||||||
|
|
@ -14,14 +14,14 @@ describe("InMemoryGameSessionStore", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns session data after creation", async () => {
|
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);
|
await store.create("session-1", data, 60_000);
|
||||||
const result = await store.get("session-1");
|
const result = await store.get("session-1");
|
||||||
expect(result).toEqual(data);
|
expect(result).toEqual(data);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns null after the session is deleted", async () => {
|
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.create("session-1", data, 60_000);
|
||||||
await store.delete("session-1");
|
await store.delete("session-1");
|
||||||
const result = await store.get("session-1");
|
const result = await store.get("session-1");
|
||||||
|
|
@ -29,7 +29,7 @@ describe("InMemoryGameSessionStore", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns null after TTL expires", async () => {
|
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 store.create("session-1", data, 1);
|
||||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||||
const result = await store.get("session-1");
|
const result = await store.get("session-1");
|
||||||
|
|
@ -37,7 +37,7 @@ describe("InMemoryGameSessionStore", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns session data before TTL expires", async () => {
|
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);
|
await store.create("session-1", data, 60_000);
|
||||||
const result = await store.get("session-1");
|
const result = await store.get("session-1");
|
||||||
expect(result).not.toBeNull();
|
expect(result).not.toBeNull();
|
||||||
|
|
|
||||||
|
|
@ -12,8 +12,8 @@ export const createGameRouter = (store: GameSessionStore): Router => {
|
||||||
router.use(requireAuth);
|
router.use(requireAuth);
|
||||||
router.use(gameLimiter);
|
router.use(gameLimiter);
|
||||||
|
|
||||||
router.post("/start", controller.createGame);
|
router.post("/start", controller.createGame as express.RequestHandler);
|
||||||
router.post("/answer", controller.submitAnswer);
|
router.post("/answer", controller.submitAnswer as express.RequestHandler);
|
||||||
|
|
||||||
return router;
|
return router;
|
||||||
};
|
};
|
||||||
|
|
|
||||||
|
|
@ -36,7 +36,6 @@ beforeEach(() => {
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("createGameSession", () => {
|
describe("createGameSession", () => {
|
||||||
|
|
||||||
let store: InMemoryGameSessionStore;
|
let store: InMemoryGameSessionStore;
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
|
|
@ -44,14 +43,14 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns a session with the correct number of questions", async () => {
|
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.sessionId).toBeDefined();
|
||||||
expect(session.questions).toHaveLength(3);
|
expect(session.questions).toHaveLength(3);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("each question has exactly 4 options", async () => {
|
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) {
|
for (const question of session.questions) {
|
||||||
expect(question.options).toHaveLength(4);
|
expect(question.options).toHaveLength(4);
|
||||||
|
|
@ -59,14 +58,14 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("each question has a unique questionId", async () => {
|
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);
|
const ids = session.questions.map((q) => q.questionId);
|
||||||
|
|
||||||
expect(new Set(ids).size).toBe(ids.length);
|
expect(new Set(ids).size).toBe(ids.length);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("options have sequential optionIds 0-3", async () => {
|
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) {
|
for (const question of session.questions) {
|
||||||
const optionIds = question.options.map((o) => o.optionId);
|
const optionIds = question.options.map((o) => o.optionId);
|
||||||
|
|
@ -75,7 +74,7 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("the correct answer is always among the options", async () => {
|
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++) {
|
for (let i = 0; i < session.questions.length; i++) {
|
||||||
const question = session.questions[i]!;
|
const question = session.questions[i]!;
|
||||||
|
|
@ -87,7 +86,7 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("distractors are never the correct answer", async () => {
|
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++) {
|
for (let i = 0; i < session.questions.length; i++) {
|
||||||
const question = session.questions[i]!;
|
const question = session.questions[i]!;
|
||||||
|
|
@ -103,7 +102,7 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("sets the prompt from the source text", async () => {
|
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[0]!.prompt).toBe("dog");
|
||||||
expect(session.questions[1]!.prompt).toBe("cat");
|
expect(session.questions[1]!.prompt).toBe("cat");
|
||||||
|
|
@ -111,14 +110,14 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("passes gloss through (null or string)", async () => {
|
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[0]!.gloss).toBeNull();
|
||||||
expect(session.questions[2]!.gloss).toBe("a building for living in");
|
expect(session.questions[2]!.gloss).toBe("a building for living in");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("calls getGameTerms with the correct arguments", async () => {
|
it("calls getGameTerms with the correct arguments", async () => {
|
||||||
await createGameSession(validRequest, store);
|
await createGameSession(validRequest, store, "user-1");
|
||||||
|
|
||||||
expect(mockGetGameTerms).toHaveBeenCalledWith(
|
expect(mockGetGameTerms).toHaveBeenCalledWith(
|
||||||
"en",
|
"en",
|
||||||
|
|
@ -130,7 +129,7 @@ describe("createGameSession", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("calls getDistractors once per question", async () => {
|
it("calls getDistractors once per question", async () => {
|
||||||
await createGameSession(validRequest, store);
|
await createGameSession(validRequest, store, "user-1");
|
||||||
|
|
||||||
expect(mockGetDistractors).toHaveBeenCalledTimes(3);
|
expect(mockGetDistractors).toHaveBeenCalledTimes(3);
|
||||||
});
|
});
|
||||||
|
|
@ -138,23 +137,21 @@ describe("createGameSession", () => {
|
||||||
it("propagates unexpected errors from getGameTerms", async () => {
|
it("propagates unexpected errors from getGameTerms", async () => {
|
||||||
mockGetGameTerms.mockRejectedValue(new Error("connection refused"));
|
mockGetGameTerms.mockRejectedValue(new Error("connection refused"));
|
||||||
|
|
||||||
await expect(createGameSession(validRequest, store)).rejects.toThrow(
|
await expect(
|
||||||
"connection refused",
|
createGameSession(validRequest, store, "user-1"),
|
||||||
);
|
).rejects.toThrow("connection refused");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("evaluateAnswer", () => {
|
describe("evaluateAnswer", () => {
|
||||||
|
|
||||||
let store: InMemoryGameSessionStore;
|
let store: InMemoryGameSessionStore;
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
store = new InMemoryGameSessionStore();
|
store = new InMemoryGameSessionStore();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
||||||
it("returns isCorrect: true when the correct option is selected", async () => {
|
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 question = session.questions[0]!;
|
||||||
const correctText = fakeTerms[0]!.targetText;
|
const correctText = fakeTerms[0]!.targetText;
|
||||||
const correctOption = question.options.find((o) => o.text === correctText)!;
|
const correctOption = question.options.find((o) => o.text === correctText)!;
|
||||||
|
|
@ -166,6 +163,7 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: correctOption.optionId,
|
selectedOptionId: correctOption.optionId,
|
||||||
},
|
},
|
||||||
store,
|
store,
|
||||||
|
"user-1",
|
||||||
);
|
);
|
||||||
|
|
||||||
expect(result.isCorrect).toBe(true);
|
expect(result.isCorrect).toBe(true);
|
||||||
|
|
@ -174,7 +172,7 @@ describe("evaluateAnswer", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns isCorrect: false when a wrong option is selected", async () => {
|
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 question = session.questions[0]!;
|
||||||
const correctText = fakeTerms[0]!.targetText;
|
const correctText = fakeTerms[0]!.targetText;
|
||||||
const correctOption = question.options.find((o) => o.text === correctText)!;
|
const correctOption = question.options.find((o) => o.text === correctText)!;
|
||||||
|
|
@ -187,6 +185,7 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: wrongOption.optionId,
|
selectedOptionId: wrongOption.optionId,
|
||||||
},
|
},
|
||||||
store,
|
store,
|
||||||
|
"user-1",
|
||||||
);
|
);
|
||||||
|
|
||||||
expect(result.isCorrect).toBe(false);
|
expect(result.isCorrect).toBe(false);
|
||||||
|
|
@ -201,13 +200,13 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: 0,
|
selectedOptionId: 0,
|
||||||
};
|
};
|
||||||
|
|
||||||
await expect(evaluateAnswer(submission, store)).rejects.toThrow(
|
await expect(evaluateAnswer(submission, store, "user-1")).rejects.toThrow(
|
||||||
"Game session not found",
|
"Game session not found",
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("throws NotFoundError for a non-existent question", async () => {
|
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 = {
|
const submission: AnswerSubmission = {
|
||||||
sessionId: session.sessionId,
|
sessionId: session.sessionId,
|
||||||
|
|
@ -215,13 +214,13 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: 0,
|
selectedOptionId: 0,
|
||||||
};
|
};
|
||||||
|
|
||||||
await expect(evaluateAnswer(submission, store)).rejects.toThrow(
|
await expect(evaluateAnswer(submission, store, "user-1")).rejects.toThrow(
|
||||||
"Question not found",
|
"Question not found",
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("throws NotFoundError when the same question is submitted twice", async () => {
|
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]!;
|
const question = session.questions[0]!;
|
||||||
|
|
||||||
await evaluateAnswer(
|
await evaluateAnswer(
|
||||||
|
|
@ -231,6 +230,7 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: 0,
|
selectedOptionId: 0,
|
||||||
},
|
},
|
||||||
store,
|
store,
|
||||||
|
"user-1",
|
||||||
);
|
);
|
||||||
|
|
||||||
await expect(
|
await expect(
|
||||||
|
|
@ -241,12 +241,13 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: 0,
|
selectedOptionId: 0,
|
||||||
},
|
},
|
||||||
store,
|
store,
|
||||||
|
"user-1",
|
||||||
),
|
),
|
||||||
).rejects.toThrow("Question not found");
|
).rejects.toThrow("Question not found");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("deletes the session after the last question is answered", async () => {
|
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) {
|
for (const question of session.questions) {
|
||||||
await evaluateAnswer(
|
await evaluateAnswer(
|
||||||
|
|
@ -256,6 +257,7 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: 0,
|
selectedOptionId: 0,
|
||||||
},
|
},
|
||||||
store,
|
store,
|
||||||
|
"user-1",
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -267,6 +269,7 @@ describe("evaluateAnswer", () => {
|
||||||
selectedOptionId: 0,
|
selectedOptionId: 0,
|
||||||
},
|
},
|
||||||
store,
|
store,
|
||||||
|
"user-1",
|
||||||
),
|
),
|
||||||
).rejects.toThrow("Game session not found");
|
).rejects.toThrow("Game session not found");
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,7 @@ import { shuffleArray } from "../lib/utils.js";
|
||||||
export const createGameSession = async (
|
export const createGameSession = async (
|
||||||
request: GameRequest,
|
request: GameRequest,
|
||||||
store: GameSessionStore,
|
store: GameSessionStore,
|
||||||
|
userId: string,
|
||||||
): Promise<GameSession> => {
|
): Promise<GameSession> => {
|
||||||
const terms = await getGameTerms(
|
const terms = await getGameTerms(
|
||||||
request.source_language,
|
request.source_language,
|
||||||
|
|
@ -59,7 +60,7 @@ export const createGameSession = async (
|
||||||
);
|
);
|
||||||
|
|
||||||
const sessionId = randomUUID();
|
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 };
|
return { sessionId, questions };
|
||||||
};
|
};
|
||||||
|
|
@ -67,10 +68,11 @@ export const createGameSession = async (
|
||||||
export const evaluateAnswer = async (
|
export const evaluateAnswer = async (
|
||||||
submission: AnswerSubmission,
|
submission: AnswerSubmission,
|
||||||
store: GameSessionStore,
|
store: GameSessionStore,
|
||||||
|
userId: string,
|
||||||
): Promise<AnswerResult> => {
|
): Promise<AnswerResult> => {
|
||||||
const session = await store.get(submission.sessionId);
|
const session = await store.get(submission.sessionId);
|
||||||
|
|
||||||
if (!session) {
|
if (!session || session.userId !== userId) {
|
||||||
throw new NotFoundError(`Game session not found: ${submission.sessionId}`);
|
throw new NotFoundError(`Game session not found: ${submission.sessionId}`);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
5
apps/api/src/types/express.d.ts
vendored
5
apps/api/src/types/express.d.ts
vendored
|
|
@ -1,3 +1,4 @@
|
||||||
|
import type { Request } from "express";
|
||||||
import type { Session, User } from "better-auth";
|
import type { Session, User } from "better-auth";
|
||||||
|
|
||||||
declare global {
|
declare global {
|
||||||
|
|
@ -14,4 +15,6 @@ declare module "ws" {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
export {};
|
export type AuthenticatedRequest = Request & {
|
||||||
|
session: { session: Session; user: User };
|
||||||
|
};
|
||||||
|
|
|
||||||
125
documentation/tickets/t00006.md
Normal file
125
documentation/tickets/t00006.md
Normal file
|
|
@ -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<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);
|
||||||
|
```
|
||||||
Loading…
Add table
Add a link
Reference in a new issue