From 37585948969673018194036f28dbc3d54df3d2fe Mon Sep 17 00:00:00 2001 From: Indiana Date: Mon, 20 Jul 2026 15:34:12 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20harden=20login/logout=20=E2=80=94=20secu?= =?UTF-8?q?re=20cookie,=20server-side=20session=20revocation,=20timing-saf?= =?UTF-8?q?e=20login?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three Important-severity review findings inherited from Task 4's plan reference code: - login() now sets secure=True on the session cookie (safe behind the Cloudflare Tunnel, which terminates TLS at the edge). - logout() looks up and deletes the matching AuthSession row before clearing the cookie, so a leaked raw token can no longer be replayed after logout. - login() always performs exactly one verify_password call regardless of whether the username exists (against a module-level dummy hash for nonexistent users), removing the timing oracle that let unauthenticated requests distinguish registered from unregistered usernames. Adds two tests: nonexistent-username login rejection, and logout revoking the session server-side. Also adjusts two cookie-propagation touch points in test_auth.py to manually re-inject the qm_session cookie, since httpx's cookie jar won't auto-attach a Secure cookie to the test transport's plain http://test base_url (a real browser talking to the HTTPS tunnel edge wouldn't have this problem). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013PphXq1s43DNRj1uWKGXof --- backend/app/routes/auth.py | 26 +++++++++++++++++++++----- backend/tests/test_auth.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 0987b97..44eb135 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -1,18 +1,20 @@ from datetime import datetime, timezone -from fastapi import APIRouter, Depends, HTTPException, Response, status +from fastapi import APIRouter, Cookie, Depends, HTTPException, Response, status from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from app.db import get_db from app.deps import SESSION_COOKIE_NAME, get_current_user -from app.models.auth_session import AuthSession, SESSION_TTL, generate_session_token +from app.models.auth_session import AuthSession, SESSION_TTL, generate_session_token, hash_token from app.models.user import User from app.schemas import LoginRequest, RegisterRequest, UserOut from app.security import hash_password, verify_password router = APIRouter(prefix="/auth", tags=["auth"]) +_DUMMY_PASSWORD_HASH = hash_password("dummy-password-for-timing-safety") + @router.post("/register", response_model=UserOut, status_code=status.HTTP_201_CREATED) async def register(payload: RegisterRequest, db: AsyncSession = Depends(get_db)): @@ -34,7 +36,10 @@ async def register(payload: RegisterRequest, db: AsyncSession = Depends(get_db)) @router.post("/login", response_model=UserOut) async def login(payload: LoginRequest, response: Response, db: AsyncSession = Depends(get_db)): user = await db.scalar(select(User).where(User.username == payload.username)) - if user is None or not verify_password(payload.password, user.password_hash): + if user is None: + verify_password(payload.password, _DUMMY_PASSWORD_HASH) + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="invalid credentials") + if not verify_password(payload.password, user.password_hash): raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="invalid credentials") raw_token, token_hash = generate_session_token() @@ -51,14 +56,25 @@ async def login(payload: LoginRequest, response: Response, db: AsyncSession = De raw_token, httponly=True, samesite="lax", + secure=True, max_age=int(SESSION_TTL.total_seconds()), ) return user @router.post("/logout", status_code=status.HTTP_204_NO_CONTENT) -async def logout(response: Response): - response.delete_cookie(SESSION_COOKIE_NAME) +async def logout( + response: Response, + qm_session: str | None = Cookie(default=None), + db: AsyncSession = Depends(get_db), +): + if qm_session is not None: + token_hash = hash_token(qm_session) + session = await db.scalar(select(AuthSession).where(AuthSession.token_hash == token_hash)) + if session is not None: + await db.delete(session) + await db.commit() + response.delete_cookie(SESSION_COOKIE_NAME, httponly=True, samesite="lax", secure=True) @router.get("/me", response_model=UserOut) diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index fb1b427..ed99473 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -28,6 +28,11 @@ async def test_login_sets_cookie_and_me_returns_user(client): assert login_resp.status_code == 200 assert "qm_session" in login_resp.cookies + # secure=True cookies are only auto-attached by httpx's cookie jar to https + # requests; the test transport uses base_url="http://test", so re-inject + # the cookie manually to simulate what a browser talking to the real + # Cloudflare-Tunnel-terminated HTTPS endpoint would do automatically. + client.cookies.set("qm_session", login_resp.cookies["qm_session"]) me_resp = await client.get("/auth/me") assert me_resp.status_code == 200 assert me_resp.json()["username"] == "medium2" @@ -44,3 +49,26 @@ async def test_login_wrong_password_rejected(client): async def test_me_without_cookie_rejected(client): response = await client.get("/auth/me") assert response.status_code == 401 + + +@pytest.mark.asyncio +async def test_login_nonexistent_username_rejected(client): + response = await client.post("/auth/login", json={"username": "nosuchmedium", "password": "whatever123"}) + assert response.status_code == 401 + + +@pytest.mark.asyncio +async def test_logout_revokes_session_server_side(client): + await client.post("/auth/register", json={"username": "medium4", "password": "spookyspooky"}) + login_resp = await client.post("/auth/login", json={"username": "medium4", "password": "spookyspooky"}) + raw_token = login_resp.cookies["qm_session"] + + # secure=True cookies aren't auto-attached over the test transport's plain + # http://test base_url (see note above), so re-inject the cookie before + # the logout call itself, otherwise the server never sees a session to revoke. + client.cookies.set("qm_session", raw_token) + logout_resp = await client.post("/auth/logout") + assert logout_resp.status_code == 204 + + me_resp = await client.get("/auth/me") + assert me_resp.status_code == 401