From ecf083268e0d0e21e7bfeea4b75fbd3236ce0d0f Mon Sep 17 00:00:00 2001 From: Abhishek Date: Tue, 3 Nov 2026 05:24:09 +0000 Subject: [PATCH] fix(ws-server): score submit-answer from the server clock and only for the open question submit-answer used the timeMs the client sent (only clamped) and never checked that the question was still open. Now it reads quiz:deadline and quiz:current-q from Redis, drops the answer if the question is not the live one or the deadline has passed, and computes elapsedMs from the deadline instead of using the payload value. The measured value is also what gets stored as the answer's timeMs. quiz:end and the "no more questions" path now clear the deadline keys, so answers arriving after the quiz ended are not buffered again. Adds handleMessages.submitAnswer.test.ts (fake clock) and updates the auth test mock for the new timeManager exports. ARCHITECTURE.md scoring notes updated to match. --- ARCHITECTURE.md | 13 +- .../src/utils/handleMessages.auth.test.ts | 9 + .../utils/handleMessages.submitAnswer.test.ts | 254 ++++++++++++++++++ apps/ws-server/src/utils/handleMessages.ts | 56 +++- apps/ws-server/src/utils/timeManager.ts | 14 +- 5 files changed, 328 insertions(+), 18 deletions(-) create mode 100644 apps/ws-server/src/utils/handleMessages.submitAnswer.test.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index e829658..8e049b5 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -198,8 +198,10 @@ Two security-relevant details: 1. **Questions are sanitized before broadcast.** The `quiz:question` payload maps answers down to `{ id, text }` only. The client literally cannot know which option is correct, so you cannot cheat by reading the socket. -2. **Scoring happens server-side.** The client sends `timeMs`; the server - decides correctness and points. +2. **Scoring happens server-side.** The server decides correctness and + points, and measures the time used itself from the question's deadline in + Redis. A `timeMs` in the message is ignored. Only an answer for the + question that is currently open, received before its deadline, counts. 3. **The organizer role is decided by the server.** A `join` that asks for `ORGANIZER` must carry the host's JWT. The ws-server verifies it with the same `JWT_SECRET` as http-server and checks that the user is an accepted @@ -212,11 +214,12 @@ Two security-relevant details: Scoring rewards speed: ```js -points = isCorrect ? Math.max(10, 1000 - timeMs) : 0 +points = isCorrect ? Math.max(1, Math.ceil(10 * (1 - elapsedMs / limitMs))) : 0 ``` -A correct answer in 200ms scores 800; one at 3s floors at 10. Wrong answers -score nothing. +`elapsedMs` is how much of the question's time limit had gone when the answer +reached the server. A correct answer scores 10 down to 1 across the time +limit. Wrong answers score nothing. ### genAI — AI question generation diff --git a/apps/ws-server/src/utils/handleMessages.auth.test.ts b/apps/ws-server/src/utils/handleMessages.auth.test.ts index 34522fb..2b99053 100644 --- a/apps/ws-server/src/utils/handleMessages.auth.test.ts +++ b/apps/ws-server/src/utils/handleMessages.auth.test.ts @@ -20,6 +20,7 @@ const mocks = vi.hoisted(() => ({ drainSession: vi.fn(), endQuizSession: vi.fn(), startQuestionTimer: vi.fn(), + closeQuestionWindow: vi.fn(), })); vi.mock("@repo/db", () => ({ prisma: mocks.prisma })); @@ -41,6 +42,8 @@ vi.mock("./cache.js", () => ({ vi.mock("./timeManager.js", () => ({ startQuestionTimer: mocks.startQuestionTimer, timerDeadlineKey: (id: string) => `deadline:${id}`, + currentQuestionKey: (id: string) => `current:${id}`, + closeQuestionWindow: mocks.closeQuestionWindow, finishQuestionEarly: vi.fn(), })); vi.mock("./answerQueue.js", () => ({ @@ -127,6 +130,7 @@ beforeEach(() => { mocks.drainSession, mocks.endQuizSession, mocks.startQuestionTimer, + mocks.closeQuestionWindow, ]) { mock.mockReset(); } @@ -371,6 +375,11 @@ describe("a verified organizer can run their quiz", () => { await handleMessage(host, { type: "quiz:end", payload: { sessionId } }); + // answers stop being accepted before the buffer is drained + expect(mocks.closeQuestionWindow).toHaveBeenCalledWith(sessionId); + expect(mocks.closeQuestionWindow.mock.invocationCallOrder[0]).toBeLessThan( + mocks.drainSession.mock.invocationCallOrder[0], + ); expect(mocks.drainSession).toHaveBeenCalledWith(sessionId); expect(mocks.endQuizSession).toHaveBeenCalledWith(sessionId); expect(mocks.broadcastToSession).toHaveBeenCalledWith(sessionId, "quiz:leaderboard", { diff --git a/apps/ws-server/src/utils/handleMessages.submitAnswer.test.ts b/apps/ws-server/src/utils/handleMessages.submitAnswer.test.ts new file mode 100644 index 0000000..afcb97b --- /dev/null +++ b/apps/ws-server/src/utils/handleMessages.submitAnswer.test.ts @@ -0,0 +1,254 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import WebSocket from "ws"; + +const mocks = vi.hoisted(() => ({ + cache: { get: vi.fn() }, + getCachedQuestions: vi.fn(), + enqueueAnswer: vi.fn(), + countAnswered: vi.fn(), + finishQuestionEarly: vi.fn(), + closeQuestionWindow: vi.fn(), + drainSession: vi.fn(), + broadcastToSession: vi.fn(), +})); + +vi.mock("@repo/db", () => ({ prisma: {} })); +vi.mock("../redis.js", () => ({ cache: mocks.cache })); +vi.mock("./broadcastTosession.js", () => ({ broadcastToSession: mocks.broadcastToSession })); +vi.mock("./endQuizSession.js", () => ({ endQuizSession: vi.fn() })); +vi.mock("./leaderboard.js", () => ({ getLeaderboard: vi.fn() })); +vi.mock("./cache.js", () => ({ + getCachedSession: vi.fn(), + getCachedQuestions: mocks.getCachedQuestions, + getCachedParticipants: vi.fn(), + getCachedLeaderboard: vi.fn().mockResolvedValue([]), + invalidateParticipants: vi.fn(), + appendCachedParticipant: vi.fn(), + invalidateLeaderboard: vi.fn(), + invalidateSession: vi.fn(), + clearSessionCache: vi.fn(), +})); +vi.mock("./timeManager.js", () => ({ + startQuestionTimer: vi.fn(), + timerDeadlineKey: (id: string) => `deadline:${id}`, + currentQuestionKey: (id: string) => `current:${id}`, + closeQuestionWindow: mocks.closeQuestionWindow, + finishQuestionEarly: mocks.finishQuestionEarly, +})); +vi.mock("./answerQueue.js", () => ({ + enqueueAnswer: mocks.enqueueAnswer, + countAnswered: mocks.countAnswered, + drainSession: mocks.drainSession, +})); + +import { handleMessage } from "./handleMessages.js"; +import type { Client } from "../index.js"; + +const SESSION = "s-1"; +const START = new Date("2026-01-01T00:00:00Z").getTime(); +const LIMIT_S = 20; + +const questions = [ + { + id: "q1", + timeLimit: LIMIT_S, + answers: [ + { id: "right", isCorrect: true }, + { id: "wrong", isCorrect: false }, + ], + }, + { + id: "q2", + timeLimit: LIMIT_S, + answers: [{ id: "right2", isCorrect: true }], + }, +]; + +/** What the timer leaves in Redis while a question is open. */ +function openQuestion(index: number, startedAt = START) { + const redis: Record = { + [`deadline:${SESSION}`]: String(startedAt + LIMIT_S * 1_000), + [`current:${SESSION}`]: String(index), + }; + mocks.cache.get.mockImplementation(async (key: string) => redis[key] ?? null); +} + +function player(): Client { + return { + ws: { readyState: WebSocket.OPEN, send: vi.fn(), terminate: vi.fn() } as unknown as WebSocket, + sessionId: SESSION, + role: "PARTICIPANT", + participantId: "p-1", + isAlive: true, + } as Client; +} + +function submit(overrides: Record = {}) { + return handleMessage(player(), { + type: "quiz:submit-answer", + payload: { + sessionId: SESSION, + participantId: "p-1", + questionId: "q1", + answerId: "right", + timeMs: 0, + ...overrides, + }, + }); +} + +function enqueued() { + return mocks.enqueueAnswer.mock.calls.map(([, answer]) => answer); +} + +beforeEach(() => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(START); + for (const mock of [ + mocks.cache.get, + mocks.getCachedQuestions, + mocks.enqueueAnswer, + mocks.countAnswered, + mocks.finishQuestionEarly, + ]) { + mock.mockReset(); + } + mocks.getCachedQuestions.mockResolvedValue(questions); + mocks.enqueueAnswer.mockReturnValue(true); + mocks.countAnswered.mockResolvedValue(0); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("quiz:submit-answer timing", () => { + it("ignores a client timeMs of 0 and scores from the server clock", async () => { + openQuestion(0); + vi.setSystemTime(START + 15_000); // 15s of 20s used + + await submit({ timeMs: 0 }); + + expect(enqueued()).toEqual([ + expect.objectContaining({ timeMs: 15_000, points: 3, isCorrect: true }), + ]); + }); + + it("gives the same score whatever timeMs the client claims", async () => { + openQuestion(0); + vi.setSystemTime(START + 5_000); + + await submit({ timeMs: 0 }); + await submit({ timeMs: 19_999 }); + await submit({ timeMs: -500 }); + await submit({ timeMs: "nope" }); + + expect(enqueued().map((a) => [a.timeMs, a.points])).toEqual([ + [5_000, 8], + [5_000, 8], + [5_000, 8], + [5_000, 8], + ]); + }); + + it("still pays the maximum for an answer the instant the question opens", async () => { + openQuestion(0); + + await submit(); + + expect(enqueued()).toEqual([expect.objectContaining({ timeMs: 0, points: 10 })]); + }); + + it("scores a wrong answer 0", async () => { + openQuestion(0); + vi.setSystemTime(START + 1_000); + + await submit({ answerId: "wrong" }); + + expect(enqueued()).toEqual([expect.objectContaining({ isCorrect: false, points: 0 })]); + }); + + it("accepts an answer right on the deadline and floors it at 1 point", async () => { + openQuestion(0); + vi.setSystemTime(START + LIMIT_S * 1_000); + + await submit(); + + expect(enqueued()).toEqual([expect.objectContaining({ timeMs: 20_000, points: 1 })]); + }); +}); + +describe("quiz:submit-answer is only accepted for the live question", () => { + it("rejects an answer after the deadline", async () => { + openQuestion(0); + vi.setSystemTime(START + LIMIT_S * 1_000 + 1); + + await submit(); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + }); + + it("rejects an answer once the question is closed (deadline key gone)", async () => { + mocks.cache.get.mockResolvedValue(null); + + await submit(); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + expect(mocks.finishQuestionEarly).not.toHaveBeenCalled(); + }); + + it("rejects an answer for a question that is not the current one", async () => { + openQuestion(1); + + await submit({ questionId: "q1" }); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + }); + + it("accepts the question that is current", async () => { + openQuestion(1); + + await submit({ questionId: "q2", answerId: "right2" }); + + expect(enqueued()).toEqual([expect.objectContaining({ questionId: "q2" })]); + }); + + it("rejects a question id that does not exist", async () => { + openQuestion(0); + + await submit({ questionId: "made-up" }); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + }); + + it("rejects when the current-question marker is missing", async () => { + const redis: Record = { + [`deadline:${SESSION}`]: String(START + 10_000), + }; + mocks.cache.get.mockImplementation(async (key: string) => redis[key] ?? null); + + await submit(); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + }); + + it("rejects a garbage deadline value", async () => { + const redis: Record = { + [`deadline:${SESSION}`]: "not-a-number", + [`current:${SESSION}`]: "0", + }; + mocks.cache.get.mockImplementation(async (key: string) => redis[key] ?? null); + + await submit(); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + }); + + it("still ignores a message with no participant id", async () => { + openQuestion(0); + + await submit({ participantId: undefined }); + + expect(mocks.enqueueAnswer).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/ws-server/src/utils/handleMessages.ts b/apps/ws-server/src/utils/handleMessages.ts index 0e18385..2377da7 100644 --- a/apps/ws-server/src/utils/handleMessages.ts +++ b/apps/ws-server/src/utils/handleMessages.ts @@ -17,7 +17,13 @@ import { invalidateSession, clearSessionCache, } from './cache.js'; -import { startQuestionTimer, timerDeadlineKey, finishQuestionEarly } from "./timeManager.js"; +import { + startQuestionTimer, + timerDeadlineKey, + currentQuestionKey, + closeQuestionWindow, + finishQuestionEarly, +} from "./timeManager.js"; import { enqueueAnswer, countAnswered, drainSession } from "./answerQueue.js"; import { authorizeOrganizerAction, @@ -372,6 +378,7 @@ export const handleMessage = async (client: Client, data: any) => { // Buffered answers first: the quiz is over, so anything left // in the queue has no later flush to land in and the final // scores would be missing them permanently. + await closeQuestionWindow(sessionId); await drainSession(sessionId); await endQuizSession(sessionId); await clearSessionCache(sessionId); @@ -429,7 +436,9 @@ export const handleMessage = async (client: Client, data: any) => { } case 'quiz:submit-answer': { - const { sessionId, participantId, questionId, answerId, timeMs } = payload; + // timeMs is deliberately not read: the client's own stopwatch can + // say anything. Time used is measured here from the deadline. + const { sessionId, participantId, questionId, answerId } = payload; if (!participantId) break; @@ -438,8 +447,28 @@ export const handleMessage = async (client: Client, data: any) => { // handled below. Asking the database first would add a round-trip // to every answer and still not settle a race. const questions = await getCachedQuestions(sessionId); - const question = questions.find((q: any) => q.id === questionId); - const answerMeta = question?.answers.find((a: any) => a.id === answerId); + const questionIndex = questions.findIndex((q: any) => q.id === questionId); + const question = questions[questionIndex]; + if (!question) break; + + // Only the question that is open right now, and only before its + // deadline, can be answered. finishQuestion deletes both keys the + // moment a question closes and closeQuestionWindow does the same + // when the quiz ends, so a missing deadline means nothing is + // open. Without this, the correct answers broadcast in + // quiz:question-results could be replayed for full points by + // anyone who had skipped the question. + const [rawDeadline, rawCurrent] = await Promise.all([ + cache.get(timerDeadlineKey(sessionId)), + cache.get(currentQuestionKey(sessionId)), + ]); + if (!rawDeadline || rawCurrent === null || Number(rawCurrent) !== questionIndex) break; + + const now = Date.now(); + const deadline = Number(rawDeadline); + if (!Number.isFinite(deadline) || now > deadline) break; + + const answerMeta = question.answers.find((a: any) => a.id === answerId); const isCorrect = !!answerMeta?.isCorrect; @@ -452,11 +481,11 @@ export const handleMessage = async (client: Client, data: any) => { // Now a correct answer is worth MAX_POINTS down to 1, scaled over // the time actually allowed; a wrong answer is worth 0. Answering // at all still scores, so the floor is 1 rather than 0. - const limitMs = Math.max(1, (question?.timeLimit ?? 15) * 1_000); - // timeMs comes from the client and is not trusted for anything - // beyond ordering: clamp it so a negative or oversized value - // cannot mint points or push the score below the floor. - const elapsedMs = Math.min(Math.max(Number(timeMs) || 0, 0), limitMs); + const limitMs = Math.max(1, (question.timeLimit ?? 15) * 1_000); + // Time used = time allowed minus time that was left when the + // answer reached the server. Clamped because the timer and the + // limit are read separately and can disagree by a few ms. + const elapsedMs = Math.min(Math.max(limitMs - (deadline - now), 0), limitMs); const points = isCorrect ? Math.max(1, Math.ceil(MAX_POINTS * (1 - elapsedMs / limitMs))) : 0; @@ -471,7 +500,7 @@ export const handleMessage = async (client: Client, data: any) => { // A false return is a duplicate submission already buffered, // which is what the unique constraint would have rejected anyway. if (!enqueueAnswer(sessionId, { - participantId, questionId, answerId, timeMs, isCorrect, points, + participantId, questionId, answerId, timeMs: elapsedMs, isCorrect, points, })) break; // The leaderboard cache has a 10s TTL and is invalidated again @@ -530,8 +559,7 @@ export const handleMessage = async (client: Client, data: any) => { // session cache is not invalidated when the organizer // advances, so its currentQuestionIndex can be an hour // stale and would close the wrong question. - const idx = questions.findIndex((q: any) => q.id === questionId); - if (idx >= 0) await finishQuestionEarly(sessionId, idx); + await finishQuestionEarly(sessionId, questionIndex); } } @@ -549,6 +577,10 @@ export const handleMessage = async (client: Client, data: any) => { sessionTimers.delete(sessionId); } + // Stop accepting answers before draining the buffer, so nothing + // can be queued after the final board is built. + await closeQuestionWindow(sessionId); + // Buffered answers must land before the final standings are // read, or the last question's scores are missing from the board // everyone finishes on. diff --git a/apps/ws-server/src/utils/timeManager.ts b/apps/ws-server/src/utils/timeManager.ts index 867b741..b42fce5 100644 --- a/apps/ws-server/src/utils/timeManager.ts +++ b/apps/ws-server/src/utils/timeManager.ts @@ -20,7 +20,7 @@ export const timerDeadlineKey = (sessionId: string) => // Which question is in flight, so an early finish can prove it is closing the // question it meant to and not one that already moved on. -const currentQuestionKey = (sessionId: string) => +export const currentQuestionKey = (sessionId: string) => `quiz:current-q:${sessionId}`; // Set once when a question closes, by whichever path gets there first, so the @@ -111,6 +111,18 @@ async function finishQuestion( }, 2_000); } +/** + * Mark no question as open for this session. + * + * Answers are only accepted while the deadline key exists, so removing it is + * what stops late answers once the host ends the quiz. The normal close path + * does this itself in finishQuestion; this is for the paths that stop the + * countdown without going through it. + */ +export async function closeQuestionWindow(sessionId: string) { + await cache.del(timerDeadlineKey(sessionId), currentQuestionKey(sessionId)); +} + /** * End the current question early because everyone present has answered. *