From 3252ccb2ec829cf924184244ee0233195d530703 Mon Sep 17 00:00:00 2001 From: ameer Date: Mon, 4 May 2026 16:22:59 +0800 Subject: [PATCH] fix(anti-hijack): validate cookie_id against DB on every authed read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the post-recovery re-attack window. Previously cookies were authenticated purely cryptographically — once a hijacker received a signed cookie for student_id=X, that cookie remained valid forever (until QUIZ_SECRET_KEY rotated), even after admin clear-student + legit re-claim issued a fresh cookie_id for X. Now /me, /event, and /ws/student all check that the cookie's cookie_id matches participants.cookie_id for the (sid, student_id). Mismatch -> 401 + Set-Cookie clearing for HTTP, ws.close(4001) for WS. The legit re-claim wins because admin clear_student deletes the row and the next join inserts the new student's cookie_id; the hijacker's cookie now fails the DB check on every subsequent request. Test: tests/test_anti_cheat.py::test_post_recovery_old_cookie_is_dead covers the full hijack -> clear -> re-claim -> hijacker-locked-out sequence end to end. --- app/room.py | 16 ++++++++++++++++ app/routes_student.py | 27 +++++++++++++++++++-------- tests/test_anti_cheat.py | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/app/room.py b/app/room.py index 7ea9d0f..72c2525 100644 --- a/app/room.py +++ b/app/room.py @@ -189,6 +189,22 @@ class RoomManager: + sum(len(clients) for clients in self.projector_clients.values()) ) + async def cookie_id_matches(self, sid: str, student_id: str, cookie_id: str) -> bool: + """Check the student's signed cookie_id against the DB participant + row. Used to defend against the post-recovery re-attack: after + admin clears a hijacked id and the legitimate student re-joins + with a fresh cookie_id, the original hijacker's cookie is still + cryptographically valid (the secret key is unchanged), but the + DB cookie_id now belongs to the legit student. We reject any + request whose cookie_id doesn't match the current row.""" + async with connect(self.settings.db_path) as db: + cur = await db.execute( + "SELECT cookie_id FROM participants WHERE sid = ? AND student_id = ?", + (sid, student_id), + ) + row = await cur.fetchone() + return row is not None and row["cookie_id"] == cookie_id + async def add_participant(self, sid: str, student_id: str, name: str, cookie_id: str) -> None: """First-claim-wins. Raises DuplicateStudentId if this student_id is already in the participants table for this sid (an attempt to diff --git a/app/routes_student.py b/app/routes_student.py index 5ab7a2e..1b143a0 100644 --- a/app/routes_student.py +++ b/app/routes_student.py @@ -101,6 +101,10 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter: identity = auth.get_student_identity(settings, request, sid) if not identity: raise HTTPException(status_code=401, detail="Student cookie required") + if not await rooms.cookie_id_matches(sid, identity["student_id"], identity["cookie_id"]): + # Same defence as /me: a stale post-recovery cookie should + # not be able to pollute the audit log. + raise HTTPException(status_code=401, detail="Re-join required") await rooms.log_event( sid, student_id=identity["student_id"], @@ -120,17 +124,18 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter: identity = auth.get_student_identity(settings, request, sid) if not identity: raise HTTPException(status_code=401, detail="Student cookie required") - try: - return await rooms.me(sid, identity["student_id"]) - except KeyError: - # Cookie's student_id is no longer in the DB (e.g. session reset - # or DB rebuilt while the cookie persisted). Send 401 with the - # cookie cleared so the client renders the join form. We build - # the JSONResponse directly because raising HTTPException would - # bypass the cookie mutation. + # Validate cookie_id against DB. Two cases this catches: + # (a) participant row is gone (session reset, admin clear, DB + # rebuild) → cookie_id_matches returns False → 401 + cleared. + # (b) participant row exists but with a different cookie_id (a + # prior hijacker's cookie still cryptographically valid + # after the legit student re-claimed via admin recovery) + # → 401 + cleared. The hijacker's stale cookie is now dead. + if not await rooms.cookie_id_matches(sid, identity["student_id"], identity["cookie_id"]): resp = JSONResponse({"detail": "Re-join required"}, status_code=401) resp.delete_cookie(auth.STUDENT_COOKIE, path="/") return resp + return await rooms.me(sid, identity["student_id"]) @api.get("/api/session/{sid}/stats") async def stats(sid: str, request: Request, question_idx: int | None = None): @@ -145,6 +150,12 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter: if not identity or not await rooms.session_exists(sid): await websocket.close(code=4001) return + # cookie_id-vs-DB check closes the post-recovery re-attack window: + # a hijacker's WS won't authenticate after the legit student has + # re-claimed their id via admin clear-student. + if not await rooms.cookie_id_matches(sid, identity["student_id"], identity["cookie_id"]): + await websocket.close(code=4001) + return await rooms.student_ws(websocket, sid, identity) # ---- Projector view (public, read-only) ------------------------------- diff --git a/tests/test_anti_cheat.py b/tests/test_anti_cheat.py index 468d031..3849a31 100644 --- a/tests/test_anti_cheat.py +++ b/tests/test_anti_cheat.py @@ -74,6 +74,42 @@ def test_admin_clear_student_404s_when_no_match(client, sid): assert client.delete("/admin/api/students/nobody").status_code == 404 +def test_post_recovery_old_cookie_is_dead(client, sid): + """Hijack -> recovery flow: after admin clears a hijacked id and the + legitimate student re-claims with a fresh cookie_id, the original + hijacker's still-cryptographically-valid cookie must NOT continue to + authenticate. The DB cookie_id check is what closes that gap.""" + Hijacker = client.__class__ + hijacker = Hijacker(client.app) + response = hijacker.post(f"/api/session/{sid}/join", json={"student_id": "alice", "name": "Hijacker"}) + assert response.status_code == 200 + # Hijacker's cookie/me works while they hold the slot. + assert hijacker.get(f"/api/session/{sid}/me").status_code == 200 + + # Admin clears the hijacked id; legit student re-claims with a fresh + # browser (= fresh cookie jar = fresh signed cookie_id). + admin_login(client) + assert client.delete(f"/admin/api/students/alice").status_code == 200 + legit = Hijacker(client.app) + response = legit.post(f"/api/session/{sid}/join", json={"student_id": "alice", "name": "Real Alice"}) + assert response.status_code == 200 + assert legit.get(f"/api/session/{sid}/me").json()["name"] == "Real Alice" + + # The hijacker's old (still-cryptographically-valid) cookie now fails + # auth because its cookie_id doesn't match the DB row anymore. + response = hijacker.get(f"/api/session/{sid}/me") + assert response.status_code == 401 + # And the cookie should be cleared so their browser bounces back to + # the join form rather than retrying with the dead cookie. + assert any( + h.lower() == "set-cookie" and "qz_student" in v and ("max-age=0" in v.lower() or "expires=" in v.lower()) + for h, v in response.headers.items() + ), response.headers + # Same goes for the audit-event endpoint. + response = hijacker.post(f"/api/session/{sid}/event", json={"kind": "blur"}) + assert response.status_code == 401 + + def test_submit_lockout_is_server_enforced(client, sid): """Server-side: a second submit for the same (sid, student_id, qidx) returns the *original* ack rather than overwriting the answer. The