diff --git a/api-nei/alembic/versions/b9e4c7f2a1d8_add_oauth_state_table.py b/api-nei/alembic/versions/b9e4c7f2a1d8_add_oauth_state_table.py new file mode 100644 index 000000000..32c6938a2 --- /dev/null +++ b/api-nei/alembic/versions/b9e4c7f2a1d8_add_oauth_state_table.py @@ -0,0 +1,46 @@ +"""Add oauth_state table for server-side PKCE state storage + +Revision ID: b9e4c7f2a1d8 +Revises: 3f1a2b4c5d6e +Create Date: 2026-06-17 00:00:00.000000 + +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = "b9e4c7f2a1d8" +down_revision = "3f1a2b4c5d6e" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "oauth_state", + sa.Column("state", sa.String(), nullable=False), + sa.Column("verifier", sa.String(), nullable=True), + sa.Column("nonce", sa.String(), nullable=True), + sa.Column("redirect", sa.String(), nullable=True), + sa.Column("expires_at", sa.DateTime(), nullable=False), + sa.PrimaryKeyConstraint("state", name=op.f("pk_oauth_state")), + schema="nei", + ) + op.create_index( + op.f("ix_oauth_state_expires_at"), + "oauth_state", + ["expires_at"], + unique=False, + schema="nei", + ) + + +def downgrade() -> None: + op.drop_index( + op.f("ix_oauth_state_expires_at"), + table_name="oauth_state", + schema="nei", + ) + op.drop_table("oauth_state", schema="nei") diff --git a/api-nei/app/api/api_v1/auth/oidc.py b/api-nei/app/api/api_v1/auth/oidc.py index 83d36e421..e02646820 100644 --- a/api-nei/app/api/api_v1/auth/oidc.py +++ b/api-nei/app/api/api_v1/auth/oidc.py @@ -30,6 +30,7 @@ import json import secrets +from datetime import datetime, timedelta from typing import Annotated, Any, Optional from urllib.parse import quote, unquote @@ -47,6 +48,7 @@ from app import crud from app.api import deps from app.core.config import settings +from app.models.oauth_state import OAuthState from app.models.user import User from app.models.user.user_email import UserEmail from app.schemas.user import ScopeEnum, UserCreate @@ -88,7 +90,7 @@ # --------------------------------------------------------------------------- _STATE_COOKIE = "oauth_state" -_STATE_MAX_AGE = 600 # 10 minutes +_STATE_MAX_AGE = 1800 # 30 minutes — enough time for slow email delivery def _signer() -> URLSafeTimedSerializer: @@ -103,6 +105,7 @@ def _set_state_cookie( redirect_to: Optional[str] = None, user_id: Optional[int] = None, code_verifier: Optional[str] = None, + db: Optional[Session] = None, ) -> None: payload: dict = {"s": state} if oidc_nonce: @@ -121,29 +124,82 @@ def _set_state_cookie( samesite="lax", max_age=_STATE_MAX_AGE, ) - - -def _verify_and_pop_state_cookie(request: Request, response: Response, state: str) -> dict: + if db is not None: + try: + db.query(OAuthState).filter(OAuthState.state == state).delete() + db.add(OAuthState( + state=state, + verifier=code_verifier, + nonce=oidc_nonce, + redirect=redirect_to, + expires_at=datetime.utcnow() + timedelta(seconds=_STATE_MAX_AGE), + )) + db.commit() + except Exception: + db.rollback() + logger.warning("Failed to persist OAuth state to DB — cookie-only fallback applies") + + +def _verify_and_pop_state_cookie( + request: Request, + response: Response, + state: str, + *, + db: Optional[Session] = None, +) -> dict: cookie_val = request.cookies.get(_STATE_COOKIE) response.delete_cookie(_STATE_COOKIE, httponly=True, samesite="lax") - if not cookie_val: - raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Missing OAuth state cookie") - - try: - payload = _signer().loads(cookie_val, max_age=_STATE_MAX_AGE) - except SignatureExpired: - raise HTTPException(status.HTTP_401_UNAUTHORIZED, "OAuth state expired — please try again") - except BadSignature: - raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Invalid OAuth state") - - # Constant-time comparison; also handle the pre-migration "n" key for - # cookies issued by the previous version of this code. - stored = payload.get("s") or payload.get("n") - if not stored or not secrets.compare_digest(stored, state): - raise HTTPException(status.HTTP_401_UNAUTHORIZED, "OAuth state mismatch") - - return payload + if cookie_val: + try: + payload = _signer().loads(cookie_val, max_age=_STATE_MAX_AGE) + except SignatureExpired: + raise HTTPException(status.HTTP_401_UNAUTHORIZED, "OAuth state expired — please try again") + except BadSignature: + raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Invalid OAuth state") + + # Constant-time comparison; also handle the pre-migration "n" key for + # cookies issued by the previous version of this code. + stored = payload.get("s") or payload.get("n") + if not stored or not secrets.compare_digest(stored, state): + raise HTTPException(status.HTTP_401_UNAUTHORIZED, "OAuth state mismatch") + + if db is not None: + try: + db.query(OAuthState).filter(OAuthState.state == state).delete() + db.commit() + except Exception: + db.rollback() + + return payload + + # Cookie missing (e.g. callback arrived in a different browser after email + # verification). Try the DB-backed state as a fallback. + if db is not None: + row = db.query(OAuthState).filter(OAuthState.state == state).first() + if row and row.expires_at > datetime.utcnow(): + payload = {"s": row.state} + if row.nonce: + payload["n"] = row.nonce + if row.redirect: + payload["r"] = row.redirect + if row.verifier: + payload["v"] = row.verifier + try: + db.delete(row) + db.commit() + except Exception: + db.rollback() + return payload + + raise HTTPException(status.HTTP_401_UNAUTHORIZED, "missing_state_cookie") + + +def cleanup_expired_oauth_states(db: Session) -> int: + """Delete expired OAuthState rows. Returns the number of rows deleted.""" + deleted = db.query(OAuthState).filter(OAuthState.expires_at < datetime.utcnow()).delete() + db.commit() + return deleted # --------------------------------------------------------------------------- @@ -508,6 +564,7 @@ def _login_error_redirect(code: str) -> RedirectResponse: ) async def oidc_login( response: Response, + db: DbSession, redirect_to: Optional[str] = None, ): _require_oidc() @@ -542,6 +599,7 @@ async def oidc_login( oidc_nonce=oidc_nonce, redirect_to=safe_redirect, code_verifier=auth_url_data.get("code_verifier"), + db=db, ) return redirect_response @@ -562,7 +620,7 @@ async def oidc_callback( ): _require_oidc() - state_payload = _verify_and_pop_state_cookie(request, response, state) + state_payload = _verify_and_pop_state_cookie(request, response, state, db=db) raw_redirect = state_payload.get("r") redirect_to = raw_redirect if _is_safe_redirect(raw_redirect) else None code_verifier = state_payload.get("v") @@ -626,9 +684,17 @@ async def oidc_callback( raise error_code = { status.HTTP_403_FORBIDDEN: "unverified", - status.HTTP_401_UNAUTHORIZED: "session", status.HTTP_400_BAD_REQUEST: "invalid_response", - }.get(e.status_code, "unknown") + }.get(e.status_code) + if error_code is None: + if e.status_code == status.HTTP_401_UNAUTHORIZED: + error_code = ( + "email_verified_relogin" + if getattr(e, "detail", "") == "missing_state_cookie" + else "session" + ) + else: + error_code = "unknown" logger.info(f"OIDC callback failed ({e.status_code}): {e.detail}") return _login_error_redirect(error_code) except httpx.HTTPError as e: diff --git a/api-nei/app/main.py b/api-nei/app/main.py index a61cafa20..457cd51d3 100644 --- a/api-nei/app/main.py +++ b/api-nei/app/main.py @@ -5,8 +5,11 @@ from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import ORJSONResponse +from loguru import logger + from app.api.api_v1 import router as api_v1_router from app.db.init_db import init_db +from app.db.session import SessionLocal from app.core.logging import init_logging from app.core.config import settings from app.core.extension_scopes import load_scopes_from_manifests @@ -21,6 +24,18 @@ async def lifespan(_: FastAPI): load_scopes_from_manifests() # Update OAuth2 scheme with extension scopes dynamic_oauth2_scheme.update_scopes() + # Remove OAuth state rows left over from previous sessions + try: + from app.api.api_v1.auth.oidc import cleanup_expired_oauth_states + db = SessionLocal() + try: + n = cleanup_expired_oauth_states(db) + if n: + logger.info(f"Cleaned up {n} expired OAuth state row(s) on startup") + finally: + db.close() + except Exception: + logger.warning("OAuth state cleanup on startup failed — continuing") yield diff --git a/api-nei/app/models/__init__.py b/api-nei/app/models/__init__.py index fffcf8a87..75cb956ac 100644 --- a/api-nei/app/models/__init__.py +++ b/api-nei/app/models/__init__.py @@ -1,3 +1,4 @@ +from .oauth_state import OAuthState from .user import User from .event import Event from .faina import FainaMember, FainaRole, Faina diff --git a/api-nei/app/models/oauth_state.py b/api-nei/app/models/oauth_state.py new file mode 100644 index 000000000..be83115e1 --- /dev/null +++ b/api-nei/app/models/oauth_state.py @@ -0,0 +1,13 @@ +from datetime import datetime +from sqlalchemy import Column, DateTime, String +from app.db.base_class import Base + + +class OAuthState(Base): + __tablename__ = "oauth_state" + + state = Column(String, primary_key=True) + verifier = Column(String, nullable=True) + nonce = Column(String, nullable=True) + redirect = Column(String, nullable=True) + expires_at = Column(DateTime, nullable=False) diff --git a/web-nei/src/pages/auth/Login/index.jsx b/web-nei/src/pages/auth/Login/index.jsx index afc096f7d..b1bb1d24a 100644 --- a/web-nei/src/pages/auth/Login/index.jsx +++ b/web-nei/src/pages/auth/Login/index.jsx @@ -6,6 +6,12 @@ import config from "config"; // Error codes surfaced by the backend's oidc_callback. Keep in sync with // _login_error_redirect in api-nei/app/api/api_v1/auth/oidc.py. const ERROR_COPY = { + // success:true renders a green notice instead of the default error card + email_verified_relogin: { + title: "Your account is verified!", + body: "Your email was confirmed successfully. Sign in to continue.", + success: true, + }, unverified: { title: "Verify your email to continue", body: "We sent you a verification link. Check your inbox (and spam folder), click the link, and come back to sign in.", @@ -61,13 +67,13 @@ export function Component() { : "/auth/login"; return (
{copy.body}