fix: harden login/logout — secure cookie, server-side session revocation, timing-safe login
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013PphXq1s43DNRj1uWKGXof
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user