Uh oh!
There was an error while loading. Please reload this page.
fix(auth): verify + document session-token lifecycle; lock the cross-service contract (#168) - #255
Conversation
… not the session TTL (#168)
📝 WalkthroughWalkthroughThe OAuth callback now uses a configurable, bounded redirect-token TTL. Contract tests validate long-lived session cookies and token rejection behavior, while a decision record documents the frontend/backend session lifecycle and operational requirements. ChangesSession token lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs | frontend | 28674bf | Jun 22 2026, 04:32 AM |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 350f1a7 | Commit Preview URL Branch Preview URL | Jul 15 2026, 05:50 AM |
_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) <noreply@anthropic.com>
_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.
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.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.
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.
AndresL230
commented
Jul 15, 2026
@Jose-Gael-Cruz-Lopez I pushed 4 commits to this branch as part of a review sweep. Fast-forward only — nothing of yours was rewritten. Happy to back any of it out. First, the headline: your central claim is correct and I verified it independently. Sessions last 30 days, not 5 minutes. The 300s token is a one-shot redirect handoff; the BFF ( The changes are confined to the ADR prose, one test name, and one small guard.
|
AndresL230
left a comment
There was a problem hiding this comment.
Approving. To restate what I said in the comment: the code and tests here were already sound, and the central finding is correct — sessions last 30 days, not 5 minutes, and #168 is properly closed as "not a bug." I verified the tests are real by mutation rather than reading (breaking the expiry check and the signature check each fail the right test). The changes above are the ADR mechanism fix, one import-time crash guard, and a test rename — nothing that touches the substance of your work. CI green on 350f1a7.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/decisions/0018-session-token-lifecycle.md`:
- Line 7: Update the line beginning with “#168” in the session token lifecycle
document so it no longer starts with a bare hash; prefix the reference with
descriptive text or escape the hash while preserving the issue reference and
sentence meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5066a8b6-9219-4344-8b9f-e81226c0ea6d
📒 Files selected for processing (3)
backend/routes/auth.pybackend/tests/test_auth_session_contract.pydocs/decisions/0018-session-token-lifecycle.md
| ## The claim | ||
| #168 raised the concern that the backend session has a hard 5-minute lifetime |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the hash or add a prefix to prevent markdown linting errors.
Starting a line with #168 causes markdown linters to interpret it as a malformed ATX heading (triggering the MD018 rule). Consider escaping the hash or prefixing it with a word to ensure it renders correctly as text.
🛠️ Proposed fix
- `#168` raised the concern that the backend session has a hard 5-minute lifetime+ Issue `#168` raised the concern that the backend session has a hard 5-minute lifetime📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #168 raised the concern that the backend session has a hard 5-minute lifetime | |
| Issue `#168` raised the concern that the backend session has a hard 5-minute lifetime |
🧰 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/decisions/0018-session-token-lifecycle.md` at line 7, Update the line
beginning with “#168” in the session token lifecycle document so it no longer
starts with a bare hash; prefix the reference with descriptive text or escape
the hash while preserving the issue reference and sentence meaning.
Source: Linters/SAST tools
#168 was filed as a flagged "verify" finding: does the backend session really die after 5 minutes with no refresh? I traced the full path across both services. The worst case is not real — the 300s token is only a one-shot redirect handoff.
What actually happens
exp = now + 300) and redirects to the frontend with?auth_token=….frontend/src/app/api/auth/session/route.ts) verifies it and re-mints a 30-day token (SESSION_MAX_AGE = 2592000) in a byte-identical format, set as thehttpOnly/Securesapling_sessioncookie (scoped viaCOOKIE_DOMAIN).auth_guard._decode_sessionreads that cookie, verifies the HMAC with the sharedSESSION_SECRET, and accepts it for the full 30 days.So sessions persist 30 days, not 5 minutes. It holds as long as
SESSION_SECRETmatches across services andCOOKIE_DOMAINreaches the backend subdomain (both documented).Changes
backend/routes/auth.py— named the magic300as_REDIRECT_TOKEN_TTL_SECONDS(env-overridable viaSAPLING_AUTH_REDIRECT_TOKEN_TTL) and corrected the comment to say it's the redirect-handoff TTL, not the session TTL.docs/decisions/0018-session-token-lifecycle.md— documents the full lifecycle, the verification outcome, the operational preconditions, and follow-ups (sliding refresh is a frontend-BFF concern, out of scope).backend/tests/test_auth_session_contract.py— locks the cross-service contract: a frontend-style 30-day token is accepted by the backend decoder; expired, tampered, and wrong-secret tokens are rejected; the redirect TTL stays short.Verification
ruff check .clean; gated suite green (+6 new). The acceptance criterion ("confirmed behavior documented; sessions persist for a configured TTL") is met.Closes#168.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests