-
Notifications
You must be signed in to change notification settings - Fork 140
CBG-4962 create cookie with SameSite=None if CORS enabled #7841
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
Conversation
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.
Pull Request Overview
This PR modifies cookie creation to set SameSite=None when CORS is enabled, allowing cross-origin authenticated requests while preserving existing behavior when CORS is disabled.
- Updated
MakeSessionCookieto accept asameSiteparameter - Modified session creation to set
SameSite=Nonewhen CORS is configured - Added test coverage to verify cookie attributes for both CORS-enabled and CORS-disabled scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rest/session_api.go | Added logic to set SameSite=None when CORS is enabled during session cookie creation |
| auth/session.go | Updated MakeSessionCookie function signature to accept sameSite parameter |
| rest/session_test.go | Added tests verifying SameSite attribute behavior with and without CORS configuration |
| rest/cors_test.go | Added assertion to verify SameSite=None in existing CORS test |
| auth/session_test.go | Updated all test calls to pass http.SameSiteDefaultMode parameter |
rest/session_api.go
Outdated
| if !h.getCORSConfig().IsEmpty() { | ||
| sameSite = http.SameSiteNoneMode | ||
| } | ||
| cookie := auth.MakeSessionCookie(session, h.db.Options.SecureCookieOverride, h.db.Options.SessionCookieHttpOnly, sameSite) |
Copilot
AI
Oct 28, 2025
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.
When setting SameSite=None, the Secure flag must also be set to true for the cookie to be accepted by browsers. The current implementation doesn't enforce this constraint. Consider adding validation to ensure SecureCookieOverride is true when CORS is enabled, or document this requirement.
| if !h.getCORSConfig().IsEmpty() { | |
| sameSite = http.SameSiteNoneMode | |
| } | |
| cookie := auth.MakeSessionCookie(session, h.db.Options.SecureCookieOverride, h.db.Options.SessionCookieHttpOnly, sameSite) | |
| secure := h.db.Options.SecureCookieOverride | |
| if !h.getCORSConfig().IsEmpty() { | |
| sameSite = http.SameSiteNoneMode | |
| // When SameSite=None, Secure must be true for browser compatibility. | |
| secure = true | |
| } | |
| cookie := auth.MakeSessionCookie(session, secure, h.db.Options.SessionCookieHttpOnly, sameSite) |
Redocly previews |
adamcfraser
left a comment
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.
Looks good to me. One documentation suggestion, otherwise looks fine.
| Expires: session.Expiration, | ||
| Secure: secureCookie, | ||
| HttpOnly: httpOnly, | ||
| SameSite: sameSite, |
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.
I had to go verify that http.Cookie doesn't emit the SameSite attribute when SameSiteDefaultMode is specified, to ensure we won't see any functional change for users not using CORS or setting the flag.
It's possible that's worth mentioning the default handling in a function description comment, to make that clear. (on the fence about the necessity of that, take it or leave it)
CBG-4962 create cookie with SameSite=None if CORS enabled
Preserve existing behavior unless CORS is enabled. If CORS is enabled, pass
SameSite=Noneto allow cross origin requests.Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/api