diff --git a/backend/routes/auth.py b/backend/routes/auth.py index 4ea19f8b..5543a1f7 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -9,6 +9,8 @@ import base64 import hashlib import hmac as _hmac +import logging +import os import re import secrets import time as _time @@ -40,6 +42,8 @@ except ImportError: GOOGLE_AVAILABLE = False +logger = logging.getLogger(__name__) + router = APIRouter() @@ -70,6 +74,53 @@ def _stamp_last_sign_in_for_test(user_id: str) -> None: _OAUTH_COOKIE_MAX_AGE = 600 _POPUP_ID_RE = re.compile(r"^[A-Za-z0-9_-]{1,128}$") +# TTL of the one-shot HMAC token handed to the frontend on the OAuth-callback +# redirect. This is NOT the session lifetime (#168): the frontend session BFF +# (frontend/src/app/api/auth/session) verifies this token once and re-mints a +# long-lived `sapling_session` cookie (SESSION_MAX_AGE = 30 days) in the same +# backend-compatible HMAC format, which `auth_guard._decode_session` accepts. +# So this token only needs to outlive the redirect round-trip. Configurable for +# environments with slow OAuth hops. See docs/decisions/0018-session-token-lifecycle.md. +_DEFAULT_REDIRECT_TOKEN_TTL_SECONDS = 300 + + +def _clamp_redirect_ttl(seconds: int) -> int: + """Clamp the redirect-handoff token TTL to [30, 600]s. + + This one-shot token travels in the OAuth-callback URL, so a misconfigured + override must not be able to turn it into a long-lived credential — it is + NOT the session (see docs/decisions/0018-session-token-lifecycle.md). The + floor keeps slow OAuth hops working; the ceiling keeps the handoff short. + """ + return max(30, min(seconds, 600)) + + +def _parse_redirect_ttl(raw: str | None) -> int: + """Parse the SAPLING_AUTH_REDIRECT_TOKEN_TTL override into a clamped TTL. + + A non-numeric or empty override (e.g. the var declared in Railway/Wrangler + with no value) must not take the whole app down: this module is imported at + router-mount time, so an unguarded int() would raise ValueError and stop the + app from booting. Fall back to the default and warn instead. + """ + if raw is None: + return _DEFAULT_REDIRECT_TOKEN_TTL_SECONDS + try: + return _clamp_redirect_ttl(int(raw)) + except ValueError: + logger.warning( + "Ignoring malformed SAPLING_AUTH_REDIRECT_TOKEN_TTL=%r (expected an " + "integer number of seconds); falling back to %ss.", + raw, + _DEFAULT_REDIRECT_TOKEN_TTL_SECONDS, + ) + return _DEFAULT_REDIRECT_TOKEN_TTL_SECONDS + + +_REDIRECT_TOKEN_TTL_SECONDS = _parse_redirect_ttl( + os.getenv("SAPLING_AUTH_REDIRECT_TOKEN_TTL") +) + # Fallback in-memory store for environments without SESSION_SECRET; entries # are keyed by nonce and expire after _OAUTH_COOKIE_MAX_AGE seconds. _OAUTH_FALLBACK_STORE: dict[str, tuple[float, dict]] = {} @@ -408,11 +459,15 @@ def _fail_redirect(error_code: str, fallback_path: str = "/auth") -> RedirectRes ) return resp - # Build a short-lived HMAC token so the frontend can verify this redirect - # without a second round-trip to the backend. + # One-shot HMAC token so the frontend can verify this redirect without a + # second round-trip. The frontend exchanges it for the real, long-lived + # `sapling_session` cookie (see _REDIRECT_TOKEN_TTL_SECONDS above) — it is + # NOT the session itself, so it expires quickly. auth_token = "" if SESSION_SECRET: - payload = json.dumps({"user_id": user_id, "exp": int(_time.time()) + 300}).encode() + payload = json.dumps( + {"user_id": user_id, "exp": int(_time.time()) + _REDIRECT_TOKEN_TTL_SECONDS} + ).encode() payload_b64 = base64.urlsafe_b64encode(payload).decode().rstrip("=") sig_bytes = _hmac.new(SESSION_SECRET.encode(), payload_b64.encode(), hashlib.sha256).digest() sig_b64 = base64.urlsafe_b64encode(sig_bytes).decode().rstrip("=") diff --git a/backend/tests/test_auth_session_contract.py b/backend/tests/test_auth_session_contract.py new file mode 100644 index 00000000..ce2dee55 --- /dev/null +++ b/backend/tests/test_auth_session_contract.py @@ -0,0 +1,163 @@ +""" +Cross-service session-token contract (#168). + +The backend never sets the `sapling_session` cookie itself — the frontend +session BFF mints it (30-day `SESSION_MAX_AGE`) in a format that must stay +compatible with what `auth_guard._decode_session` verifies. These tests assert +the backend accepts a long-lived token in that format (so sessions do NOT die at +5 minutes) and rejects expired/tampered ones. + +Scope caveat: `_mint` below is a Python re-implementation of the format, so this +suite verifies the backend decoder against a locally-minted token — it does not +execute the frontend's `signSession`, and so cannot catch drift on the frontend +side. A shared JSON fixture consumed by both this suite and a frontend test +would be the real cross-service lock; see the ADR follow-ups. + +See docs/decisions/0018-session-token-lifecycle.md. +""" +import base64 +import hashlib +import hmac +import json +import time + +import pytest +from fastapi import HTTPException +from starlette.requests import Request + +from services import auth_guard + +# Mirrors frontend/src/lib/sessionToken.ts SESSION_MAX_AGE. +FRONTEND_SESSION_MAX_AGE = 2592000 # 30 days +SHARED_SECRET = "shared-session-secret-at-least-32-bytes-long!!" + + +def _mint(user_id: str, ttl_seconds: int, secret: str) -> str: + """Mint a token in the shape both services use: + base64url(no pad) JSON {"user_id","exp"} . base64url(HMAC-SHA256(payload_b64)). + + This mirrors the *backend* mint (routes/auth.py) byte-for-byte. It is close + to, but not byte-identical with, the frontend's signSession: Python's + json.dumps emits `{"user_id": "x", "exp": 1}` (with spaces) where JS + JSON.stringify emits `{"user_id":"x","exp":1}` (without). That difference is + immaterial to the contract under test — the verifier HMACs the received + `payload_b64` opaquely and never re-serializes the payload — but it does mean + this helper is a third Python re-implementation of the format, so these tests + prove a *Python-minted* token is accepted, not a real frontend-minted one. + See the ADR follow-ups for the shared-fixture idea that would close that gap. + """ + payload = json.dumps({"user_id": user_id, "exp": int(time.time()) + ttl_seconds}).encode() + payload_b64 = base64.urlsafe_b64encode(payload).decode().rstrip("=") + sig = hmac.new(secret.encode(), payload_b64.encode(), hashlib.sha256).digest() + sig_b64 = base64.urlsafe_b64encode(sig).decode().rstrip("=") + return f"{payload_b64}.{sig_b64}" + + +def _request(cookie: str | None = None, query: str = "") -> Request: + headers = [] + if cookie is not None: + headers.append((b"cookie", f"sapling_session={cookie}".encode())) + scope = { + "type": "http", "method": "GET", "path": "/", + "headers": headers, "query_string": query.encode(), + } + return Request(scope) + + +@pytest.fixture(autouse=True) +def _shared_secret(monkeypatch): + monkeypatch.setattr(auth_guard, "SESSION_SECRET", SHARED_SECRET) + + +def test_frontend_30_day_cookie_is_accepted_by_backend(): + token = _mint("user_alice", FRONTEND_SESSION_MAX_AGE, SHARED_SECRET) + payload = auth_guard._real_decode_session(_request(cookie=token)) + assert payload["user_id"] == "user_alice" + # The token is valid far beyond 5 minutes — no premature "Session expired". + assert payload["exp"] - int(time.time()) > 29 * 24 * 3600 + + +def test_legacy_unused_auth_token_query_param_is_still_accepted(): + """Characterization of a legacy credential channel that no client uses. + + `_decode_session` reads `?auth_token=` before the cookie, but nothing sends + it to the backend: routes/auth.py puts the redirect token in a URL pointing + at the *frontend*, and frontend/src/app/auth/callback/page.tsx reads it there + and POSTs it to the session BFF in a JSON body. This test pins current + behaviour, it does not endorse the channel — the decoder applies no + ttl/purpose check here, so a 30-day session token in `?auth_token=` is + accepted identically, and session tokens in URLs leak via access logs, + Referer headers, and browser history. Removing the channel is a follow-up + (out of scope for #168, which is about session lifetime). + """ + token = _mint("user_bob", 300, SHARED_SECRET) + payload = auth_guard._real_decode_session(_request(query=f"auth_token={token}")) + assert payload["user_id"] == "user_bob" + + +def test_expired_token_is_rejected(): + token = _mint("user_alice", -10, SHARED_SECRET) + with pytest.raises(HTTPException) as exc: + auth_guard._real_decode_session(_request(cookie=token)) + assert exc.value.status_code == 401 + assert exc.value.detail == "Session expired" + + +def test_tampered_signature_is_rejected(): + token = _mint("user_alice", FRONTEND_SESSION_MAX_AGE, SHARED_SECRET) + payload_b64, sig_b64 = token.split(".") + flipped = "A" if sig_b64[0] != "A" else "B" + tampered = f"{payload_b64}.{flipped}{sig_b64[1:]}" + with pytest.raises(HTTPException) as exc: + auth_guard._real_decode_session(_request(cookie=tampered)) + assert exc.value.status_code == 401 + + +def test_token_signed_with_a_different_secret_is_rejected(): + token = _mint("user_alice", FRONTEND_SESSION_MAX_AGE, "some-other-secret-value-32-bytes-xxxxx") + with pytest.raises(HTTPException) as exc: + auth_guard._real_decode_session(_request(cookie=token)) + assert exc.value.status_code == 401 + + +def test_redirect_token_ttl_default_is_short(): + import routes.auth as auth + # The redirect handoff token must stay short — it is not the session. + assert auth._REDIRECT_TOKEN_TTL_SECONDS <= 600 + + +def test_redirect_token_ttl_override_is_clamped(): + import routes.auth as auth + # A misconfigured/hostile env override cannot lengthen the URL-borne + # handoff token beyond the 600s ceiling, and cannot drop below the 30s floor. + assert auth._clamp_redirect_ttl(86400) == 600 + assert auth._clamp_redirect_ttl(5) == 30 + assert auth._clamp_redirect_ttl(300) == 300 + + +@pytest.mark.parametrize("raw", ["abc", "", " ", "300s", "1e3", None]) +def test_malformed_redirect_ttl_override_falls_back_instead_of_crashing(raw): + """A malformed override must not break module import. + + routes.auth is imported at router-mount time, so an unguarded int() on the + env var would raise ValueError and stop the app from booting. Empty-string is + the realistic case: declaring the var in Railway/Wrangler without a value. + """ + import routes.auth as auth + assert auth._parse_redirect_ttl(raw) == 300 + + +@pytest.mark.parametrize( + ("raw", "expected"), + [("60", 60), ("86400", 600), ("5", 30), (" 300 ", 300)], +) +def test_wellformed_redirect_ttl_override_is_parsed_and_clamped(raw, expected): + import routes.auth as auth + assert auth._parse_redirect_ttl(raw) == expected + + +def test_malformed_redirect_ttl_override_warns(caplog): + import routes.auth as auth + with caplog.at_level("WARNING", logger=auth.logger.name): + auth._parse_redirect_ttl("abc") + assert "SAPLING_AUTH_REDIRECT_TOKEN_TTL" in caplog.text diff --git a/docs/decisions/0018-session-token-lifecycle.md b/docs/decisions/0018-session-token-lifecycle.md new file mode 100644 index 00000000..1c12998d --- /dev/null +++ b/docs/decisions/0018-session-token-lifecycle.md @@ -0,0 +1,130 @@ +# 0018 — Session token lifecycle (verification of #168) + +**Status:** documented · **Issue:** #168 (filed as a flagged "verify" finding) + +## The claim + +#168 raised the concern that the backend session has a hard 5-minute lifetime +with no refresh path: the only place the backend mints a token is the OAuth +callback with `exp = now + 300`, there is no `/refresh` route, and the +`sapling_session` cookie the decoder accepts is never `set_cookie`'d by the +backend. If accurate, every request would 401 with "Session expired" five +minutes after sign-in and force a full Google re-login. + +## What actually happens + +Verified by reading the full path across both services. The 300-second token is +**not** the session — it is a one-shot handoff token: + +1. **Backend — OAuth callback** (`routes/auth.py`) mints a short-lived HMAC + token (`{user_id, exp}`, `exp = now + _REDIRECT_TOKEN_TTL_SECONDS`, default + 300s) and redirects to `FRONTEND_URL/auth/callback?auth_token=…`. It only + needs to survive the redirect round-trip. + +2. **Frontend — session BFF** (`frontend/src/app/api/auth/session/route.ts`) + receives that `auth_token`, verifies its HMAC + expiry against the shared + `SESSION_SECRET`, and on success re-mints a **30-day** token + (`signSession`, `SESSION_MAX_AGE = 2592000`) which it sets as the + `httpOnly`, `Secure`, `SameSite=Lax` **`sapling_session` cookie**. + +3. **Backend — every authed request** (`services/auth_guard.py::_decode_session`) + reads `sapling_session` from the cookie, verifies the HMAC with the same + `SESSION_SECRET`, and checks `exp`. Because the frontend signed it in a + **compatible format** (`payload_b64.sig_b64`, base64url no padding, + HMAC-SHA256 over the `payload_b64` *string*, JSON `{"user_id", "exp"}`) with + a 30-day `exp`, the backend accepts it for the full 30 days. + + The two mints are compatible, not byte-identical: Python's `json.dumps` emits + `{"user_id": "x", "exp": 1}` (with spaces), JS `JSON.stringify` emits + `{"user_id":"x","exp":1}` (without). This is immaterial — the verifier HMACs + the received `payload_b64` opaquely and never re-serializes — but the formats + are *interoperable*, not identical, and nothing should be built on assuming + the latter. + +So the effective session lifetime is **30 days**, refreshed implicitly on the +next sign-in. The 5-minute death scenario does not occur in the deployed +frontend-BFF topology. + +## How the cookie actually reaches the backend + +**Authed API calls are same-origin.** `frontend/src/lib/api.ts` sets +`API_URL = ''`, so all ~135 `fetchJSON` call sites request same-origin paths like +`/api/graph/…`. `frontend/next.config.ts` rewrites `/api/:path*` → +`${BACKEND_URL}/api/:path*` **server-side**, and that server-side hop forwards the +`Cookie` header to the backend. The browser never makes a cross-origin authed API +call, so the cookie's `domain` attribute is not what carries it to the backend — +a host-only cookie would reach the backend just fine. + +## Preconditions (operational) + +The contract holds only if both are true in production: + +- **`SESSION_SECRET` is identical** on the frontend and backend deployments + (the BFF returns 401 "Invalid or expired auth token (SESSION_SECRET likely + does not match the backend)" if not). +- **`BACKEND_URL` is set at _build_ time** on the Cloudflare Worker. It is read + in `next.config.ts` at build time and baked into the rewrite, *not* resolved + per-request from the runtime env. Set only as a runtime var, the rewrite falls + back to `http://localhost:5000` and every `/api/*` call 500s — which looks like + a session bug but is a config bug. This is the genuinely load-bearing + precondition. + +`COOKIE_DOMAIN` governs only the cookie's `domain` attribute (widening it from +host-only to all `*.saplinglearn.com` hosts). It is set — `.saplinglearn.com` in +`frontend/wrangler.toml`, validated through `sanitizeCookieDomain` (#190) — so +nothing is broken today, but it is **not** the mechanism by which the session +reaches the backend, and the contract would hold without it. + +> **Do not "fix" an authed call by pointing it at `NEXT_PUBLIC_API_URL`.** That +> makes the request cross-origin, and the browser will not attach +> `sapling_session` unless the call also sets `credentials: 'include'` *and* the +> backend runs the matching CORS + cookie-domain setup. This is exactly the +> 2026-06-30 onboarding-loop bug: onboarding POSTed to +> `${NEXT_PUBLIC_API_URL}/api/onboarding/profile` cross-origin with no +> `credentials`, so the cookie was dropped, `require_self` 401'd, and +> `onboarding_completed` never flipped — trapping the user in "Get Started" on +> every sign-in. It was fixed by routing through `submitOnboardingProfile()` → +> `fetchJSON`; `frontend/src/app/(public)/page.tsx` still carries a comment +> recording it. Authed calls go through `lib/api.ts` `fetchJSON`; see +> `frontend/.env.example`, which documents leaving `NEXT_PUBLIC_API_URL` empty in +> production. + +## Decision + +No backend session-lifetime bug to fix. Changes made under #168: + +- Named the magic `300` as `_REDIRECT_TOKEN_TTL_SECONDS` (env-overridable via + `SAPLING_AUTH_REDIRECT_TOKEN_TTL`) and corrected the comment to state it is + the redirect-handoff TTL, not the session TTL. +- Added `tests/test_auth_session_contract.py` covering the backend half of the + token contract: a frontend-*style* 30-day token is accepted by the backend + decoder, expired/tampered/wrong-secret tokens are rejected. +- Hardened the `SAPLING_AUTH_REDIRECT_TOKEN_TTL` override: it is parsed inside a + `try/except ValueError` and falls back to 300s with a warning. `routes/auth.py` + is imported at router-mount time, so a malformed value (`abc`, or the var + declared with an empty value) previously raised at import and stopped the app + from booting. + +## Follow-ups (not blocking, out of scope here) + +- **`?auth_token=` is a live but unused credential channel.** + `auth_guard._decode_session` reads `request.query_params["auth_token"]` *before* + the cookie, but no client sends it to the backend: the backend puts the redirect + token in a URL pointing at the *frontend*, and `auth/callback/page.tsx` reads it + there and POSTs it to the BFF in a JSON body. The decoder applies no + ttl/purpose distinction, so a 30-day session token in `?auth_token=` is accepted + identically — and tokens in URLs leak via access logs, `Referer`, and history. + Removing the channel is its own change (it is not a session-lifetime issue); + `test_legacy_unused_auth_token_query_param_is_still_accepted` characterizes + today's behaviour so the removal is a deliberate, visible edit. +- **The contract test is not yet a true cross-service lock.** Its `_mint` helper + is a *third* Python re-implementation of the format, so it proves a + Python-minted token is accepted, not a real frontend-minted one. There is no + `frontend/src/lib/sessionToken.test.ts`, and the BFF (`route.ts`) hand-rolls + `verifyAuthToken` as a duplicate of `sessionToken.ts::verifySession` — two + frontend copies to drift from. The real lock would be a **shared JSON fixture** + (tokens + expected verdicts) checked into the repo and consumed by both the + pytest suite and a frontend test. +- Sliding refresh: the 30-day token is fixed-window, not sliding. If a sliding + session is desired, the BFF should re-mint on activity. Frontend scope. +- Fix `page.tsx:619` (#339) to route onboarding through `lib/api.ts` `fetchJSON`.