Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 0
fix(auth): verify + document session-token lifecycle; lock the cross-service contract (#168)#255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
dadcc83
refactor(auth): import os for configurable redirect-token TTL (#168)
Jose-Gael-Cruz-Lopez cdf8c17
refactor(auth): name the redirect-token TTL constant, clarifying it's…
Jose-Gael-Cruz-Lopez 9baad76
refactor(auth): use _REDIRECT_TOKEN_TTL_SECONDS for the redirect toke…
Jose-Gael-Cruz-Lopez 7f2cecb
docs(auth): document session-token lifecycle + #168 verification outcome
Jose-Gael-Cruz-Lopez 28674bf
test(auth): lock cross-service session-token contract; 30-day cookie …
Jose-Gael-Cruz-Lopez 8758dc6
Merge remote-tracking branch 'origin/main' into fix/session-token-lif…
Jose-Gael-Cruz-Lopez 56d6393
fix(auth): clamp redirect-token TTL override to <=600s
Jose-Gael-Cruz-Lopez 09b95bd
fix(auth): don't crash the app on a malformed redirect-TTL override
AndresL230 1c1adf5
test(auth): cover the TTL parse guard and correct overstated claims
AndresL230 187f419
docs(adr): correct how the session cookie reaches the backend in 0018
AndresL230 350f1a7
docs(adr): correct the cross-origin cautionary example in 0018
AndresL230 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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`. | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the hash or add a prefix to prevent markdown linting errors.
Starting a line with
#168causes markdown linters to interpret it as a malformed ATX heading (triggering theMD018rule). Consider escaping the hash or prefixing it with a word to ensure it renders correctly as text.🛠️ Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 7-7: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Source: Linters/SAST tools