fix: change session cookie SameSite from Strict to Lax#9
Conversation
SameSite=Strict prevents session cookies from being sent on cross-site
top-level navigations (e.g. clicking a link from another site). This
caused resolveFrontendTheme to return different themes on consecutive
requests within a redirect chain, creating infinite 302 loops:
1. External link to /dashboard → no session cookie (Strict blocks it)
→ GetTheme() returns system default (e.g. 'classic')
→ MapFrontendPath redirects /dashboard → /console (302)
2. 302 redirect to /console → session cookie still not sent (Strict
blocks cookies for the entire cross-site navigation chain)
→ But if session somehow resolves to user's DB theme ('default')
→ MapFrontendPath redirects /console → /dashboard (302)
→ Infinite loop
SameSite=Lax allows cookies on cross-site top-level GET navigations
while still blocking them on cross-site POST requests, iframe loads,
and AJAX calls — providing strong CSRF protection without breaking
normal link-following.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMoves frontend theme persistence from cookies to localStorage across client components and routing, removes server-side/profile theme persistence and related redirects, and updates the session cookie to SameSite=Lax and Secure=true only in release mode. ChangesFrontend theme & session changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the session cookie configuration in main.go by changing the SameSite attribute from StrictMode to LaxMode. Feedback was provided regarding a security concern where the Secure attribute is set to false; it is recommended to enable it in production environments to prevent session hijacking.
| HttpOnly: true, | ||
| Secure: false, | ||
| SameSite: http.SameSiteStrictMode, | ||
| SameSite: http.SameSiteLaxMode, |
There was a problem hiding this comment.
The change to http.SameSiteLaxMode is appropriate for resolving the redirect loop issues described. However, I noticed that the Secure attribute is set to false on line 183. For session cookies, it is a security best practice to set Secure: true in production environments to ensure the cookie is only transmitted over HTTPS, preventing session hijacking via man-in-the-middle attacks. Consider making this value conditional based on the environment (e.g., Secure: os.Getenv("GIN_MODE") == "release").
…end sync - Replace cookie-based theme storage with localStorage (EnabledClassicFrontend key) - Remove backend API calls for theme preference (/api/user/self frontend_theme) - Remove server-side theme resolution from cookies in web-router.go - Update classic theme UserContext and PreferencesSettings to use localStorage - Update default frontend theme logic to use localStorage only - Set session cookie SameSite=Lax and Secure conditional on GIN_MODE - Simplify resolveFrontendTheme to only use common.GetTheme()
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/classic/src/context/User/index.jsx (1)
37-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing theme preferences during the localStorage cutover.
This effect still parses
state.user.setting, but it no longer seedsEnabledClassicFrontendfrom the legacysettings.frontend_themevalue. Users who previously saved"classic"on their profile and don't already have the new localStorage key will be treated as"default"after this ships, so the new redirect flow will send them to the wrong frontend until they switch again. Please add a one-time fallback here that initializes localStorage fromsettings.frontend_themeonly when the new key is absent.💡 Suggested migration fallback
+const ENABLED_CLASSIC_FRONTEND_KEY = 'EnabledClassicFrontend'; + useEffect(() => { if (state.user?.setting) { try { const settings = JSON.parse(state.user.setting); + if (localStorage.getItem(ENABLED_CLASSIC_FRONTEND_KEY) == null) { + if (settings.frontend_theme === 'classic') { + localStorage.setItem(ENABLED_CLASSIC_FRONTEND_KEY, 'true'); + } else if (settings.frontend_theme === 'default') { + localStorage.removeItem(ENABLED_CLASSIC_FRONTEND_KEY); + } + } const normalizedLanguage = normalizeLanguage(settings.language); if (normalizedLanguage && normalizedLanguage !== i18n.language) { i18n.changeLanguage(normalizedLanguage); } if (normalizedLanguage) {🤖 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 `@web/classic/src/context/User/index.jsx` around lines 37 - 51, Add a one-time fallback in the existing useEffect that, after parsing settings (state.user.setting) and inside the try block, checks localStorage for the new key (EnabledClassicFrontend) and if it is absent initializes it from settings.frontend_theme (e.g., set EnabledClassicFrontend = 'true' when settings.frontend_theme === 'classic'), without overwriting an existing localStorage value; update the code in the same useEffect (the function referenced by useEffect and the parsed settings variable) so this migration runs only when the new key is missing.
🧹 Nitpick comments (1)
web/default/src/features/auth/hooks/use-auth-redirect.ts (1)
57-61: ⚡ Quick winPrefer
navigateoverwindow.location.replacein login redirect flow.Line 60 should use router navigation for consistency and type-safe route transitions in the default app.
Suggested change
const theme = getFrontendTheme() if (theme === 'classic') { setFrontendTheme('classic') - window.location.replace('/console') + navigate({ to: '/console', replace: true }) return }As per coding guidelines, "Use
useNavigateorLinkfor navigation to maintain type safety; avoid directwindow.locationmanipulation".🤖 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 `@web/default/src/features/auth/hooks/use-auth-redirect.ts` around lines 57 - 61, The redirect currently uses window.location.replace in the use-auth-redirect hook (after checking getFrontendTheme() and calling setFrontendTheme('classic')) which bypasses router handling; replace this with the router navigate function by importing useNavigate from react-router-dom, obtaining navigate inside the hook, and calling navigate('/console', { replace: true }) instead of window.location.replace to preserve router state and type-safe route transitions.
🤖 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 `@main.go`:
- Line 183: The session cookie `Secure` flag is being derived from
os.Getenv("GIN_MODE") instead of Gin's effective runtime mode, causing
mismatches (e.g., when `GIN_MODE` is unset or `test`); update the `Secure:`
assignment in main.go (the struct field currently using os.Getenv("GIN_MODE") ==
"release") to use Gin's runtime mode check, e.g. `gin.Mode() == gin.ReleaseMode`
(or equivalent), so the cookie behavior used by middleware/auth.go aligns with
the actual Gin mode.
---
Outside diff comments:
In `@web/classic/src/context/User/index.jsx`:
- Around line 37-51: Add a one-time fallback in the existing useEffect that,
after parsing settings (state.user.setting) and inside the try block, checks
localStorage for the new key (EnabledClassicFrontend) and if it is absent
initializes it from settings.frontend_theme (e.g., set EnabledClassicFrontend =
'true' when settings.frontend_theme === 'classic'), without overwriting an
existing localStorage value; update the code in the same useEffect (the function
referenced by useEffect and the parsed settings variable) so this migration runs
only when the new key is missing.
---
Nitpick comments:
In `@web/default/src/features/auth/hooks/use-auth-redirect.ts`:
- Around line 57-61: The redirect currently uses window.location.replace in the
use-auth-redirect hook (after checking getFrontendTheme() and calling
setFrontendTheme('classic')) which bypasses router handling; replace this with
the router navigate function by importing useNavigate from react-router-dom,
obtaining navigate inside the hook, and calling navigate('/console', { replace:
true }) instead of window.location.replace to preserve router state and
type-safe route transitions.
🪄 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: 8c3bb808-31e1-40ea-9ff3-718fbdf8dd69
📒 Files selected for processing (8)
main.gorouter/web-router.goweb/classic/src/components/settings/personal/cards/PreferencesSettings.jsxweb/classic/src/context/User/index.jsxweb/default/src/features/auth/hooks/use-auth-redirect.tsweb/default/src/features/profile/components/frontend-theme-card.tsxweb/default/src/lib/frontend-theme.tsweb/default/src/routes/_authenticated/route.tsx
💤 Files with no reviewable changes (2)
- router/web-router.go
- web/default/src/features/profile/components/frontend-theme-card.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Problem
Session cookie was set with \SameSite=Strict, which prevents cookies from being sent on cross-site top-level navigations (e.g. clicking a link from another site). This caused
esolveFrontendTheme\ to return different themes on consecutive requests within a redirect chain, creating infinite 302 loops.
Fix
Change \SameSite\ from \Strict\ to \Lax.
\SameSite=Lax\ allows cookies on cross-site top-level GET navigations (clicking links) while still blocking them on:
This provides strong CSRF protection without breaking normal link-following behavior.
Summary by CodeRabbit