Skip to content

Commit 3f99981

Browse files
committed
fix(auth): bind OAuth state to session + harden review fixes
Address PR #73 review feedback: - auth: bind state nonce to HttpOnly cookie carrying {nonce, cv, popup_id} (HMAC-signed via SESSION_SECRET); reject mismatched/missing state with invalid_state. Closes login-CSRF where attacker-crafted state could log victim into attacker's account. - auth: wrap flow.fetch_token in try/except → oauth_exchange_failed so popup self-closes instead of returning 500 on replayed/expired codes. - auth: validate popup_id charset/length ([A-Za-z0-9_-]{1,128}); invalid input degrades to non-popup mode. - SignInModal: split popupUrl (with popup_id) from sameTabUrl (without) so popup-blocker fallback redirects locally instead of broadcasting into a dead channel. - SignInModal: don't require data.name on success — /auth/callback legitimately broadcasts empty name when /api/auth/me has no display. - models: tighten model_pref to Optional[Literal["fast","smart"]] so garbage input 422s; fix misleading default-comment. - ModelToggle: static aria-label so screen readers hear the description even when the visual tooltip is hidden. - tests: add 23 unit tests for auth state cookie/nonce/popup_id helpers and 6 for _resolve_tutor_model.
1 parent 90ba796 commit 3f99981

6 files changed

Lines changed: 396 additions & 24 deletions

File tree

backend/models/__init__.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
from typing import Optional, Union, List
1+
from typing import Optional, Union, List, Literal
22
from pydantic import BaseModel, Field
33

44

@@ -10,7 +10,7 @@ class StartSessionBody(BaseModel):
1010
mode: str = "socratic"
1111
use_shared_context: bool = True
1212
course_id: Optional[str] = None # Direct course_id lookup instead of resolving from topic
13-
model_pref: Optional[str] = None # "smart" (default, gemini-2.5-pro) or "fast" (gemini-2.5-flash)
13+
model_pref: Optional[Literal["fast", "smart"]] = None # "fast" (default, gemini-2.5-flash) or "smart" (gemini-2.5-pro)
1414

1515

1616
class ChatBody(BaseModel):
@@ -19,7 +19,7 @@ class ChatBody(BaseModel):
1919
message: str
2020
mode: str = "socratic"
2121
use_shared_context: bool = True
22-
model_pref: Optional[str] = None
22+
model_pref: Optional[Literal["fast", "smart"]] = None # "fast" (default, gemini-2.5-flash) or "smart" (gemini-2.5-pro)
2323

2424

2525
class EndSessionBody(BaseModel):
@@ -33,7 +33,7 @@ class ActionBody(BaseModel):
3333
action_type: str = "hint"
3434
mode: str = "socratic"
3535
use_shared_context: bool = True
36-
model_pref: Optional[str] = None
36+
model_pref: Optional[Literal["fast", "smart"]] = None # "fast" (default, gemini-2.5-flash) or "smart" (gemini-2.5-pro)
3737

3838

3939
# ── Quiz ──────────────────────────────────────────────────────────────────────

backend/routes/auth.py

Lines changed: 141 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import base64
1010
import hashlib
1111
import hmac as _hmac
12+
import re
1213
import secrets
1314
import time as _time
1415
from urllib.parse import urlencode
@@ -39,6 +40,14 @@
3940

4041
router = APIRouter()
4142

43+
OAUTH_STATE_COOKIE = "sapling_oauth_state"
44+
_OAUTH_COOKIE_MAX_AGE = 600
45+
_POPUP_ID_RE = re.compile(r"^[A-Za-z0-9_-]{1,128}$")
46+
47+
# Fallback in-memory store for environments without SESSION_SECRET; entries
48+
# are keyed by nonce and expire after _OAUTH_COOKIE_MAX_AGE seconds.
49+
_OAUTH_FALLBACK_STORE: dict[str, tuple[float, dict]] = {}
50+
4251

4352
def _google_client_config() -> dict:
4453
return {
@@ -73,6 +82,68 @@ def _generate_pkce_pair():
7382
return code_verifier, code_challenge
7483

7584

85+
def _clean_popup_id(s: str | None) -> str | None:
86+
if not s:
87+
return None
88+
return s if _POPUP_ID_RE.match(s) else None
89+
90+
91+
def _encode_oauth_cookie(payload: dict) -> str:
92+
raw = json.dumps(payload, separators=(",", ":")).encode()
93+
payload_b64 = base64.urlsafe_b64encode(raw).decode().rstrip("=")
94+
if SESSION_SECRET:
95+
sig_bytes = _hmac.new(SESSION_SECRET.encode(), payload_b64.encode(), hashlib.sha256).digest()
96+
sig_b64 = base64.urlsafe_b64encode(sig_bytes).decode().rstrip("=")
97+
return f"{payload_b64}.{sig_b64}"
98+
nonce = payload.get("n", "")
99+
if nonce:
100+
_OAUTH_FALLBACK_STORE[nonce] = (_time.monotonic() + _OAUTH_COOKIE_MAX_AGE, payload)
101+
_prune_fallback_store()
102+
return payload_b64
103+
104+
105+
def _decode_oauth_cookie(cookie_value: str | None) -> dict | None:
106+
if not cookie_value:
107+
return None
108+
if SESSION_SECRET:
109+
if "." not in cookie_value:
110+
return None
111+
try:
112+
payload_b64, sig_b64 = cookie_value.rsplit(".", 1)
113+
except ValueError:
114+
return None
115+
expected = _hmac.new(SESSION_SECRET.encode(), payload_b64.encode(), hashlib.sha256).digest()
116+
expected_b64 = base64.urlsafe_b64encode(expected).decode().rstrip("=")
117+
if not _hmac.compare_digest(expected_b64, sig_b64):
118+
return None
119+
try:
120+
padded = payload_b64 + "=" * (-len(payload_b64) % 4)
121+
payload = json.loads(base64.urlsafe_b64decode(padded.encode()).decode())
122+
except Exception:
123+
return None
124+
return payload if isinstance(payload, dict) else None
125+
try:
126+
padded = cookie_value + "=" * (-len(cookie_value) % 4)
127+
payload = json.loads(base64.urlsafe_b64decode(padded.encode()).decode())
128+
except Exception:
129+
return None
130+
if not isinstance(payload, dict):
131+
return None
132+
nonce = payload.get("n")
133+
_prune_fallback_store()
134+
entry = _OAUTH_FALLBACK_STORE.get(nonce or "")
135+
if not entry:
136+
return None
137+
return entry[1]
138+
139+
140+
def _prune_fallback_store() -> None:
141+
now = _time.monotonic()
142+
expired = [k for k, (exp, _) in _OAUTH_FALLBACK_STORE.items() if exp < now]
143+
for k in expired:
144+
_OAUTH_FALLBACK_STORE.pop(k, None)
145+
146+
76147
@router.get("/me")
77148
def get_me(request: Request):
78149
"""Return approval and onboarding status for a given user_id."""
@@ -142,42 +213,78 @@ def google_login(popup_id: str = Query(None)):
142213
raise HTTPException(status_code=400, detail="Google OAuth not configured")
143214

144215
code_verifier, code_challenge = _generate_pkce_pair()
216+
nonce = secrets.token_urlsafe(32)
217+
clean_popup = _clean_popup_id(popup_id)
218+
145219
flow = Flow.from_client_config(_google_client_config(), scopes=AUTH_SCOPES)
146220
flow.redirect_uri = GOOGLE_AUTH_REDIRECT_URI
147-
state_payload = {"action": "signin", "cv": code_verifier}
148-
if popup_id:
149-
state_payload["popup_id"] = popup_id
150221
auth_url, _ = flow.authorization_url(
151222
prompt="consent",
152223
access_type="offline",
153-
state=_encode_state(state_payload),
224+
state=_encode_state({"action": "signin", "n": nonce}),
154225
code_challenge=code_challenge,
155226
code_challenge_method="S256",
156227
)
157-
return RedirectResponse(auth_url)
228+
229+
cookie_value = _encode_oauth_cookie({
230+
"n": nonce,
231+
"cv": code_verifier,
232+
"popup_id": clean_popup,
233+
})
234+
response = RedirectResponse(auth_url)
235+
response.set_cookie(
236+
key=OAUTH_STATE_COOKIE,
237+
value=cookie_value,
238+
max_age=_OAUTH_COOKIE_MAX_AGE,
239+
httponly=True,
240+
secure=True,
241+
samesite="lax",
242+
path="/",
243+
)
244+
return response
158245

159246

160247
@router.get("/google/callback")
161-
def google_callback(code: str = Query(...), state: str = Query(None)):
248+
def google_callback(request: Request, code: str = Query(...), state: str = Query(None)):
162249
"""Exchange auth code for tokens, validate @bu.edu, upsert user."""
163-
state_data = _decode_state(state) if state else {}
164-
code_verifier = state_data.get("cv")
165-
popup_id = state_data.get("popup_id")
250+
cookie_payload = _decode_oauth_cookie(request.cookies.get(OAUTH_STATE_COOKIE))
251+
code_verifier = cookie_payload.get("cv") if cookie_payload else None
252+
popup_id = _clean_popup_id(cookie_payload.get("popup_id")) if cookie_payload else None
253+
cookie_nonce = cookie_payload.get("n") if cookie_payload else None
166254

167255
def _fail_redirect(error_code: str, fallback_path: str = "/auth") -> RedirectResponse:
168256
# In popup mode, route failures through /auth/callback so the popup
169257
# can broadcast the error and self-close instead of stranding the opener.
170258
if popup_id:
171259
params = urlencode({"error": error_code, "popup_id": popup_id})
172-
return RedirectResponse(f"{FRONTEND_URL}/auth/callback?{params}")
173-
return RedirectResponse(f"{FRONTEND_URL}{fallback_path}?error={error_code}")
260+
resp = RedirectResponse(f"{FRONTEND_URL}/auth/callback?{params}")
261+
else:
262+
resp = RedirectResponse(f"{FRONTEND_URL}{fallback_path}?error={error_code}")
263+
resp.set_cookie(
264+
key=OAUTH_STATE_COOKIE,
265+
value="",
266+
max_age=0,
267+
httponly=True,
268+
secure=True,
269+
samesite="lax",
270+
path="/",
271+
)
272+
return resp
174273

175274
if not GOOGLE_AVAILABLE:
176275
return _fail_redirect("google_not_configured")
177276

277+
state_data = _decode_state(state) if state else {}
278+
state_nonce = state_data.get("n")
279+
if not cookie_payload or not cookie_nonce or not state_nonce or not _hmac.compare_digest(str(state_nonce), str(cookie_nonce)):
280+
return _fail_redirect("invalid_state")
281+
178282
flow = Flow.from_client_config(_google_client_config(), scopes=AUTH_SCOPES)
179283
flow.redirect_uri = GOOGLE_AUTH_REDIRECT_URI
180-
flow.fetch_token(code=code, code_verifier=code_verifier)
284+
try:
285+
flow.fetch_token(code=code, code_verifier=code_verifier)
286+
except Exception:
287+
return _fail_redirect("oauth_exchange_failed")
181288
creds = flow.credentials
182289

183290
# Fetch user info from Google
@@ -245,7 +352,17 @@ def _fail_redirect(error_code: str, fallback_path: str = "/auth") -> RedirectRes
245352
if not is_approved:
246353
if popup_id:
247354
return _fail_redirect("not_approved")
248-
return RedirectResponse(f"{FRONTEND_URL}/pending")
355+
resp = RedirectResponse(f"{FRONTEND_URL}/pending")
356+
resp.set_cookie(
357+
key=OAUTH_STATE_COOKIE,
358+
value="",
359+
max_age=0,
360+
httponly=True,
361+
secure=True,
362+
samesite="lax",
363+
path="/",
364+
)
365+
return resp
249366

250367
# Build a short-lived HMAC token so the frontend can verify this redirect
251368
# without a second round-trip to the backend.
@@ -264,4 +381,14 @@ def _fail_redirect(error_code: str, fallback_path: str = "/auth") -> RedirectRes
264381
**({"auth_token": auth_token} if auth_token else {}),
265382
**({"popup_id": popup_id} if popup_id else {}),
266383
})
267-
return RedirectResponse(f"{FRONTEND_URL}/auth/callback?{params}")
384+
resp = RedirectResponse(f"{FRONTEND_URL}/auth/callback?{params}")
385+
resp.set_cookie(
386+
key=OAUTH_STATE_COOKIE,
387+
value="",
388+
max_age=0,
389+
httponly=True,
390+
secure=True,
391+
samesite="lax",
392+
path="/",
393+
)
394+
return resp

0 commit comments

Comments
 (0)