Repository navigation
fix: auth loop between web dashboard and api subdomains - #1640
Merged
Merged
Conversation
Change session cookie sameSite to 'none' in production so the cookie is sent on credentialed JS fetch() across subdomains (lucky.* → lucky-api.*). Add 30s cooldown guard to the 401 interceptor to prevent redirect loops. - session.ts: sameSite: isProduction ? 'none' : 'lax' - api.ts: sessionStorage-based redirect cooldown - session.test.ts: SameSite regression tests
📝 WalkthroughWalkthroughThree areas change: backend session cookies now use environment-dependent ChangesSession & Auth Fixes
Architecture Decision Records
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Failed to generate code suggestions for PR |
|
Size Change: +135 B (+0.03%) Total Size: 472 kB 📦 View Changed
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/backend/tests/unit/middleware/session.test.ts (1)
110-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
NODE_ENVin afinallyblock.If either assertion fails, these tests leave
process.env.NODE_ENVoverridden and can skew the rest of the suite. Wrap the env mutation intry/finallyso cleanup always runs.Also applies to: 131-144
🤖 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 `@packages/backend/tests/unit/middleware/session.test.ts` around lines 110 - 125, The session middleware tests mutate process.env.NODE_ENV without guaranteed cleanup, which can leak state into later tests. Update the affected cases in session.test.ts around setupSessionMiddleware and the source assertion to wrap the NODE_ENV override in a try/finally so the original value is always restored, even if an expectation fails. Use the existing originalEnv variable and keep the cleanup local to each test block.
🤖 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 `@decisions/2026-06-27-standardize-test-runner-vitest.md`:
- Around line 28-30: Tighten the SonarCloud rationale in the decision note so it
does not imply Jest lacks native lcov output or depends on a separate Sonar
plugin. Reword the Sonar section to acknowledge the current Jest config already
emits lcov, and keep the emphasis on standardizing on Vitest for consistent
tooling and coverage format across packages; update the wording in the
SonarCloud consistency bullet in the decision document.
In `@packages/backend/tests/unit/middleware/session.test.ts`:
- Around line 119-123: The session middleware tests are asserting the source
text of session.ts instead of the behavior of setupSessionMiddleware(). Update
the relevant cases in session.test.ts to invoke setupSessionMiddleware() and
capture the options passed to express-session, then assert cookie.sameSite and
cookie.secure directly for each environment. Use the setupSessionMiddleware
symbol and the express-session mock/spy in the test to verify the middleware
options rather than grepping for ternary strings.
---
Nitpick comments:
In `@packages/backend/tests/unit/middleware/session.test.ts`:
- Around line 110-125: The session middleware tests mutate process.env.NODE_ENV
without guaranteed cleanup, which can leak state into later tests. Update the
affected cases in session.test.ts around setupSessionMiddleware and the source
assertion to wrap the NODE_ENV override in a try/finally so the original value
is always restored, even if an expectation fails. Use the existing originalEnv
variable and keep the cleanup local to each test block.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a65b113-8bad-43c8-9eed-b939dc4bdec9
📒 Files selected for processing (5)
decisions/2026-06-27-eslint-legacy-config-cleanup.mddecisions/2026-06-27-standardize-test-runner-vitest.mdpackages/backend/src/middleware/session.tspackages/backend/tests/unit/middleware/session.test.tspackages/frontend/src/services/api.ts
The production session middleware now uses sameSite:'none' (was 'lax') so the tests asserting SameSite=Lax in production scenarios were failing. Updated both locations to expect SameSite=None to match the middleware.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/tests/integration/routes/auth.test.ts (1)
131-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrust the proxy before asserting on the forwarded HTTPS cookie. This
express()instance never enablestrust proxy, sox-forwarded-proto: httpswon’t exercise the sameSecurecookie path as production. AddlocalApp.set('trust proxy', 1)here or build the app the same way as the production bootstrap.🤖 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 `@packages/backend/tests/integration/routes/auth.test.ts` around lines 131 - 159, The secure-session test for forwarded HTTPS is using a bare express app without proxy trust, so it is not exercising the same cookie path as production. Update the local app setup in the auth integration test to enable proxy trust before calling setupSessionMiddleware and setupAuthRoutes, or otherwise construct it through the same bootstrap path used in production so x-forwarded-proto is respected when asserting the Secure and SameSite=None session cookie behavior.
🤖 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.
Outside diff comments:
In `@packages/backend/tests/integration/routes/auth.test.ts`:
- Around line 131-159: The secure-session test for forwarded HTTPS is using a
bare express app without proxy trust, so it is not exercising the same cookie
path as production. Update the local app setup in the auth integration test to
enable proxy trust before calling setupSessionMiddleware and setupAuthRoutes, or
otherwise construct it through the same bootstrap path used in production so
x-forwarded-proto is respected when asserting the Secure and SameSite=None
session cookie behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87ff9674-1ba0-4bae-ad5b-73c7e57c5af6
📒 Files selected for processing (1)
packages/backend/tests/integration/routes/auth.test.ts
… transient API failure)"
|
Merged
5 tasks done
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Hotfix for production auth loop: web dashboard redirects to OAuth and returns to landing page in an infinite loop.
Root cause: Session cookie uses
SameSite=Lax, which browsers don't send on credentialed JSfetch()between subdomains (lucky.lucassantana.tech→lucky-api.lucassantana.tech) in certain conditions (ITP, Lax enforcement edge cases). After OAuth redirects back to the frontend, the SPA'scheckAuth()fetch doesn't carry the session cookie, so the user appears unauthenticated and ends up back on the landing page.Fix (two layers):
session.ts — Set
sameSite: 'none'in production (with existingsecure: trueas required by the spec). In development, keepsameSite: 'lax'for localhost compat. This ensures the cookie is consistently sent on credentialed cross-subdomain requests.api.ts — Add a 30s
sessionStoragecooldown to the 401 redirect interceptor. If a redirect was already attempted within the last 30 seconds, don't redirect again. This breaks the infinite loop as a safety net.Checklist
@lucky/shared/utilsresolution issue in jest — same 6 failures on cleanmain)Destructive / irreversible interaction (Tier A)
Skip — no irreversible Discord action added.
Feature-removal sweep
Skip — no feature removed.
Summary by cubic
Fixes the production OAuth redirect loop between the dashboard and API by sending the session cookie across subdomains and adding a 30s redirect cooldown. Also adds ADRs documenting ESLint legacy config cleanup and standardizing tests on Vitest.
packages/backend: Set session cookiesameSiteto'none'in production (keepssecure: true);'lax'in development.packages/frontend: Add a 30ssessionStoragecooldown in the 401 interceptor before redirecting to${NORMALIZED_API_BASE}/auth/discord.packages/backend: Update integration tests to expectSameSite=Nonein production and add unit tests to lock thesameSitetoggle.Written for commit 5861675. Summary will update on new commits.
Summary by CodeRabbit
SameSitefor production so it usesNone(instead ofLax).SameSitebehavior.