From dadcc83077ff2910667083d2155fd9b063919717 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Mon, 22 Jun 2026 00:29:37 -0400 Subject: [PATCH 01/10] refactor(auth): import os for configurable redirect-token TTL (#168) --- backend/routes/auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/routes/auth.py b/backend/routes/auth.py index 853dbb4f..ded94809 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -9,6 +9,7 @@ import base64 import hashlib import hmac as _hmac +import os import re import secrets import time as _time From cdf8c17a39be5f6be0bff068b01854a9f6ed7380 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Mon, 22 Jun 2026 00:29:51 -0400 Subject: [PATCH 02/10] refactor(auth): name the redirect-token TTL constant, clarifying it's not the session TTL (#168) --- backend/routes/auth.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/backend/routes/auth.py b/backend/routes/auth.py index ded94809..f072395c 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -57,6 +57,15 @@ 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. +_REDIRECT_TOKEN_TTL_SECONDS = int(os.getenv("SAPLING_AUTH_REDIRECT_TOKEN_TTL", "300")) + # 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]] = {} From 9baad76d8c57ac4515311bb518004731cb436eea Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Mon, 22 Jun 2026 00:30:05 -0400 Subject: [PATCH 03/10] refactor(auth): use _REDIRECT_TOKEN_TTL_SECONDS for the redirect token mint (#168) --- backend/routes/auth.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/backend/routes/auth.py b/backend/routes/auth.py index f072395c..07344d2e 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -404,11 +404,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("=") From 7f2cecbc097ef4a6d5964cb7f56f04736ccbfe02 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Mon, 22 Jun 2026 00:30:47 -0400 Subject: [PATCH 04/10] docs(auth): document session-token lifecycle + #168 verification outcome --- .../decisions/0018-session-token-lifecycle.md | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 docs/decisions/0018-session-token-lifecycle.md diff --git a/docs/decisions/0018-session-token-lifecycle.md b/docs/decisions/0018-session-token-lifecycle.md new file mode 100644 index 00000000..c1fc424c --- /dev/null +++ b/docs/decisions/0018-session-token-lifecycle.md @@ -0,0 +1,69 @@ +# 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** (scoped to + `COOKIE_DOMAIN` so it is also sent to the backend subdomain). + +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 + **byte-identical format** (`payload_b64.sig_b64`, base64url no padding, + HMAC-SHA256 over the payload bytes, JSON `{"user_id", "exp"}`) with a 30-day + `exp`, the backend accepts it for the full 30 days. + +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. + +## 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). +- **`COOKIE_DOMAIN` covers both subdomains** so the browser sends + `sapling_session` to the backend on cross-origin API calls (`credentials: + 'include'`). An unset/host-only cookie would not reach the backend. + +## 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` to lock the cross-service token + contract: a frontend-style 30-day token is accepted by the backend decoder, + expired/tampered tokens are rejected. + +## Follow-ups (not blocking, out of scope here) + +- 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. +- The two services independently re-implement the same token format; a shared + spec/fixture (this doc + the contract test) is the guard against drift. From 28674bf6dfd97b0db8e88097e93e07d8976f5f88 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Mon, 22 Jun 2026 00:31:46 -0400 Subject: [PATCH 05/10] test(auth): lock cross-service session-token contract; 30-day cookie accepted (#168) --- backend/tests/test_auth_session_contract.py | 99 +++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 backend/tests/test_auth_session_contract.py diff --git a/backend/tests/test_auth_session_contract.py b/backend/tests/test_auth_session_contract.py new file mode 100644 index 00000000..a86d5867 --- /dev/null +++ b/backend/tests/test_auth_session_contract.py @@ -0,0 +1,99 @@ +""" +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 +byte-compatible with what `auth_guard._decode_session` verifies. This test +reproduces the frontend's exact signing and asserts the backend accepts a +long-lived token (so sessions do NOT die at 5 minutes) and rejects +expired/tampered ones. + +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: + """Sign a token exactly like the backend mint AND the frontend signSession: + base64url(no pad) JSON {"user_id","exp"} . base64url(HMAC-SHA256(payload)).""" + 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_redirect_auth_token_query_param_is_accepted(): + # The short-lived redirect token arrives as ?auth_token while fresh. + 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 From 56d6393b5210ce30065b158aa51ac800c8862921 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Tue, 14 Jul 2026 23:36:29 -0500 Subject: [PATCH 06/10] fix(auth): clamp redirect-token TTL override to <=600s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _REDIRECT_TOKEN_TTL_SECONDS read SAPLING_AUTH_REDIRECT_TOKEN_TTL with no upper bound, so an operator (or a bad env) could set it to hours/days. That token is one-shot and travels in the OAuth-callback URL, so a long TTL widens the window in which an intercepted URL can be replayed to mint a session — it is not the session itself (ADR 0018). Clamp to [30, 600]s via a small pure helper and add a test that asserts an extreme override is clamped, so the "stays short" invariant holds at runtime, not just for the default (the previous test only checked the unset-env default). Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/routes/auth.py | 15 ++++++++++++++- backend/tests/test_auth_session_contract.py | 9 +++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/backend/routes/auth.py b/backend/routes/auth.py index 2f38b7d4..e0d4bd5f 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -78,7 +78,20 @@ def _stamp_last_sign_in_for_test(user_id: str) -> None: # 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. -_REDIRECT_TOKEN_TTL_SECONDS = int(os.getenv("SAPLING_AUTH_REDIRECT_TOKEN_TTL", "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)) + + +_REDIRECT_TOKEN_TTL_SECONDS = _clamp_redirect_ttl( + int(os.getenv("SAPLING_AUTH_REDIRECT_TOKEN_TTL", "300")) +) # Fallback in-memory store for environments without SESSION_SECRET; entries # are keyed by nonce and expire after _OAUTH_COOKIE_MAX_AGE seconds. diff --git a/backend/tests/test_auth_session_contract.py b/backend/tests/test_auth_session_contract.py index a86d5867..303b50c2 100644 --- a/backend/tests/test_auth_session_contract.py +++ b/backend/tests/test_auth_session_contract.py @@ -97,3 +97,12 @@ 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 From 09b95bdfeefb23ce6a46b1e7fe2ea852de67aa6a Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:42:08 -0400 Subject: [PATCH 07/10] fix(auth): don't crash the app on a malformed redirect-TTL override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _REDIRECT_TOKEN_TTL_SECONDS parsed SAPLING_AUTH_REDIRECT_TOKEN_TTL with a bare int() at module scope. routes/auth.py is imported at router-mount time, so `SAPLING_AUTH_REDIRECT_TOKEN_TTL=abc` — or, realistically, declaring the var in Railway/Wrangler with no value — raised ValueError at import and stopped the app from booting. Parse it in _parse_redirect_ttl() with try/except ValueError, falling back to the 300s default and logging a warning. _clamp_redirect_ttl's docstring said it defended against "a misconfigured override" but only clamped range, never parseability; the parse guard makes that claim true. --- backend/routes/auth.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/backend/routes/auth.py b/backend/routes/auth.py index e0d4bd5f..5543a1f7 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -9,6 +9,7 @@ import base64 import hashlib import hmac as _hmac +import logging import os import re import secrets @@ -41,6 +42,8 @@ except ImportError: GOOGLE_AVAILABLE = False +logger = logging.getLogger(__name__) + router = APIRouter() @@ -78,6 +81,9 @@ def _stamp_last_sign_in_for_test(user_id: str) -> None: # 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. @@ -89,8 +95,30 @@ def _clamp_redirect_ttl(seconds: int) -> int: return max(30, min(seconds, 600)) -_REDIRECT_TOKEN_TTL_SECONDS = _clamp_redirect_ttl( - int(os.getenv("SAPLING_AUTH_REDIRECT_TOKEN_TTL", "300")) +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 From 1c1adf5f1748a370cd267f7b4ba3659600ca5b4c Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:42:19 -0400 Subject: [PATCH 08/10] test(auth): cover the TTL parse guard and correct overstated claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add coverage for _parse_redirect_ttl: malformed overrides ("abc", "", None) fall back to 300s with a warning instead of raising at import; well-formed ones are still parsed and clamped to [30, 600]. Correct two claims the tests did not support: - _mint's docstring said it signs "exactly like the backend mint AND the frontend signSession". It does not: Python's json.dumps emits {"user_id": "x", "exp": 1} (spaces), JS JSON.stringify emits {"user_id":"x","exp":1} (none), so the payloads differ byte-wise. It is harmless — the verifier HMACs the received payload_b64 opaquely and never re-serializes — but _mint mirrors only the backend mint, and is itself a third Python re-implementation, so the suite proves a Python-minted token is accepted, not a real frontend-minted one. Say so rather than claiming to "lock the cross-service contract". - test_redirect_auth_token_query_param_is_accepted read as an endorsement of a constraint the code does not enforce. _decode_session reads ?auth_token= before the cookie with no ttl/purpose check, so a 30-day session token in the query string is accepted identically — and tokens in URLs leak via access logs, Referer, and history. No client sends it: the redirect token goes to the frontend, which POSTs it to the BFF in a JSON body. Rename to test_legacy_unused_auth_token_query_param_is_still_accepted and document it as characterization, so removing the channel is a deliberate, visible edit. --- backend/tests/test_auth_session_contract.py | 71 ++++++++++++++++++--- 1 file changed, 63 insertions(+), 8 deletions(-) diff --git a/backend/tests/test_auth_session_contract.py b/backend/tests/test_auth_session_contract.py index 303b50c2..ce2dee55 100644 --- a/backend/tests/test_auth_session_contract.py +++ b/backend/tests/test_auth_session_contract.py @@ -3,10 +3,15 @@ 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 -byte-compatible with what `auth_guard._decode_session` verifies. This test -reproduces the frontend's exact signing and asserts the backend accepts a -long-lived token (so sessions do NOT die at 5 minutes) and rejects -expired/tampered ones. +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. """ @@ -28,8 +33,19 @@ def _mint(user_id: str, ttl_seconds: int, secret: str) -> str: - """Sign a token exactly like the backend mint AND the frontend signSession: - base64url(no pad) JSON {"user_id","exp"} . base64url(HMAC-SHA256(payload)).""" + """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() @@ -61,8 +77,19 @@ def test_frontend_30_day_cookie_is_accepted_by_backend(): assert payload["exp"] - int(time.time()) > 29 * 24 * 3600 -def test_redirect_auth_token_query_param_is_accepted(): - # The short-lived redirect token arrives as ?auth_token while fresh. +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" @@ -106,3 +133,31 @@ def test_redirect_token_ttl_override_is_clamped(): 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 From 187f419728f1ed973383e8a9c40024f406fc727f Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:42:30 -0400 Subject: [PATCH 09/10] docs(adr): correct how the session cookie reaches the backend in 0018 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0018 documented the wrong mechanism. It claimed COOKIE_DOMAIN covers both subdomains "so the browser sends sapling_session to the backend on cross-origin API calls (credentials: 'include')", and that a host-only cookie would not reach the backend. Neither is true: - lib/api.ts sets API_URL = '', so all ~135 fetchJSON call sites are same-origin, and next.config.ts rewrites /api/:path* to BACKEND_URL server-side. The cookie reaches the backend because that server-side hop forwards the Cookie header. A host-only cookie would work fine. - No browser-side cross-origin authed call exists. The four NEXT_PUBLIC_API_URL fetches all omit credentials entirely, and middleware.ts runs server-side with a hand-set Cookie header. This mattered because an ADR is authoritative: as written it would teach a dev to point an authed fetch at NEXT_PUBLIC_API_URL, reintroducing the 2026-06-30 onboarding-loop bug — which is live at page.tsx:619 (#339), now cited as a cautionary example. It also contradicted frontend/.env.example, which tells you to leave NEXT_PUBLIC_API_URL empty in production. Replace the precondition with the genuinely load-bearing one the ADR never mentioned: BACKEND_URL must be set at *build* time for the CF Worker (next.config.ts bakes it into the rewrite), or /api/* falls back to localhost and 500s. Demote COOKIE_DOMAIN to what it actually governs — the cookie's domain attribute — noting it is set (wrangler.toml:19), so nothing is broken. Also drop the inaccurate "byte-identical format" claim (the two mints are interoperable, not identical) and record two follow-ups: removing the unused ?auth_token= channel, and a shared JSON fixture consumed by both test suites as the real cross-service lock. --- .../decisions/0018-session-token-lifecycle.md | 84 ++++++++++++++++--- 1 file changed, 71 insertions(+), 13 deletions(-) diff --git a/docs/decisions/0018-session-token-lifecycle.md b/docs/decisions/0018-session-token-lifecycle.md index c1fc424c..eca7be80 100644 --- a/docs/decisions/0018-session-token-lifecycle.md +++ b/docs/decisions/0018-session-token-lifecycle.md @@ -25,20 +25,36 @@ Verified by reading the full path across both services. The 300-second token is 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** (scoped to - `COOKIE_DOMAIN` so it is also sent to the backend subdomain). + `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 - **byte-identical format** (`payload_b64.sig_b64`, base64url no padding, - HMAC-SHA256 over the payload bytes, JSON `{"user_id", "exp"}`) with a 30-day - `exp`, the backend accepts it for the full 30 days. + **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: @@ -46,9 +62,29 @@ 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). -- **`COOKIE_DOMAIN` covers both subdomains** so the browser sends - `sapling_session` to the backend on cross-origin API calls (`credentials: - 'include'`). An unset/host-only cookie would not reach the backend. +- **`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. It is **live right now** at +> `frontend/src/app/page.tsx:619`, which posts to +> `${NEXT_PUBLIC_API_URL}/api/onboarding/profile` cross-origin with no +> `credentials` at all (tracked in #339). Authed calls go through `lib/api.ts` +> `fetchJSON`; see `frontend/.env.example`, which documents leaving +> `NEXT_PUBLIC_API_URL` empty in production. ## Decision @@ -57,13 +93,35 @@ 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` to lock the cross-service token - contract: a frontend-style 30-day token is accepted by the backend decoder, - expired/tampered tokens are rejected. +- 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. -- The two services independently re-implement the same token format; a shared - spec/fixture (this doc + the contract test) is the guard against drift. +- Fix `page.tsx:619` (#339) to route onboarding through `lib/api.ts` `fetchJSON`. From 350f1a77110b94f5cd7abe216568c1c5a80da0c2 Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:46:04 -0400 Subject: [PATCH 10/10] docs(adr): correct the cross-origin cautionary example in 0018 The example cited frontend/src/app/page.tsx:619 as a live bug tracked in #339. It is neither: the onboarding call was fixed weeks ago and now routes through submitOnboardingProfile() -> fetchJSON, and the file moved to (public)/page.tsx under the route-group refactor. #339 was filed against a 419-commit-stale branch and has been closed as invalid. Describe the 2026-06-30 bug in the past tense and point at the comment in (public)/page.tsx that records the real fix, which is a stronger cautionary example because it actually happened. --- docs/decisions/0018-session-token-lifecycle.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/decisions/0018-session-token-lifecycle.md b/docs/decisions/0018-session-token-lifecycle.md index eca7be80..1c12998d 100644 --- a/docs/decisions/0018-session-token-lifecycle.md +++ b/docs/decisions/0018-session-token-lifecycle.md @@ -79,12 +79,15 @@ reaches the backend, and the contract would hold without it. > 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. It is **live right now** at -> `frontend/src/app/page.tsx:619`, which posts to +> 2026-06-30 onboarding-loop bug: onboarding POSTed to > `${NEXT_PUBLIC_API_URL}/api/onboarding/profile` cross-origin with no -> `credentials` at all (tracked in #339). Authed calls go through `lib/api.ts` -> `fetchJSON`; see `frontend/.env.example`, which documents leaving -> `NEXT_PUBLIC_API_URL` empty in production. +> `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