Remove Cookies from JSON responses and configure session timeouts#425
Remove Cookies from JSON responses and configure session timeouts#425P4sca1 wants to merge 4 commits into
Conversation
- Remove Cookies field from HandleInfo struct in structs.go - Remove all JSON cookie emission in HandleSet2fa and HandleLogin - Session cookies are already set via http.SetCookie() headers
Signed-off-by: Pascal Sthamer <pascal+github@sthamer.xyz>
Signed-off-by: Pascal Sthamer <pascal+github@sthamer.xyz>
Signed-off-by: Pascal Sthamer <pascal+github@sthamer.xyz>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| func expireSession(ctx context.Context, resp http.ResponseWriter, user *User, reason string) error { | ||
| if resp != nil { | ||
| newCookie := constructSessionDeleteCookie() | ||
| http.SetCookie(resp, newCookie) |
There was a problem hiding this comment.
False positive. constructSessionDeleteCookie() calls ConstructSessionCookie under the hood, which sets the Secure Attribute when SHUFFLE_COOKIE_SECURE=true.
| newCookie := constructSessionDeleteCookie() | ||
| http.SetCookie(resp, newCookie) | ||
| newCookie.Name = "__session" | ||
| http.SetCookie(resp, newCookie) |
There was a problem hiding this comment.
False positive. constructSessionDeleteCookie() calls ConstructSessionCookie under the hood, which sets the Secure Attribute when SHUFFLE_COOKIE_SECURE=true.
| // Slide the cookie's Expires forward so the browser also enforces the idle timeout at the HTTP level. | ||
| if resp != nil { | ||
| refreshedCookie := ConstructSessionCookie(user.Session, getSessionExpiration()) | ||
| http.SetCookie(resp, refreshedCookie) |
There was a problem hiding this comment.
False positive. ConstructSessionCookie sets the Secure Attribute when SHUFFLE_COOKIE_SECURE=true.
| refreshedCookie := ConstructSessionCookie(user.Session, getSessionExpiration()) | ||
| http.SetCookie(resp, refreshedCookie) | ||
| refreshedCookie.Name = "__session" | ||
| http.SetCookie(resp, refreshedCookie) |
There was a problem hiding this comment.
False positive. ConstructSessionCookie sets the Secure Attribute when SHUFFLE_COOKIE_SECURE=true.
| return | ||
| } | ||
|
|
||
| loginData = fmt.Sprintf(`{"success": true, "cookies": [{"key": "session_token", "value": "%s", "expiration": %d}], "region_url": "%s"}`, sessionToken, expiration.Unix(), regionUrl) |
There was a problem hiding this comment.
oh well, we will need region_url for the cloud frontend.
i went through everything, my fears for this PR begins with the way this fits into the cloud multi-region system.
There was a problem hiding this comment.
if we just remove region_url, this would screw up the frontend getting region_url
the multi-region system is basically the same backend spread across many regions. looking at your PR, from what i see, cookies should carry over and be passed to the other backends too (since they are hosted on .shuffler.io)
There was a problem hiding this comment.
I agree. Let me rework this PR so that cloud stays unchanged.
There was a problem hiding this comment.
Hey @0x0elliot, I rechecked this PR. The loginData assignment was removed, because it gets overriden with string(newData) anyways and is not used in the meantime.
4 participants
This pull request introduces robust session management improvements, focusing on enforcing session idle timeouts and maximum lifetimes, as well as standardizing session expiration handling across authentication and login flows. The changes ensure that user sessions are automatically expired both in the database and at the HTTP cookie level based on configurable environment variables.
Session Timeout and Expiration Management:
SHUFFLE_SESSION_IDLE_TIMEOUT,SHUFFLE_SESSION_MAX_LIFETIME), and to calculate session expiration times, enabling configurable session duration and inactivity limits.HandleApiAuthentication, expiring sessions both in the database and via cookies if limits are exceeded, and refreshing cookies on each request to slide the expiration window.expireSessionhelper to centralize logic for clearing sessions from both server and client, ensuring consistent cleanup and error reporting.Consistent Session Handling in Authentication Flows:
SetSessionto use a pointer to the user object, ensuring session metadata is properly updated and persisted.Session Metadata Tracking:
SessionCreatedAtandSessionLastActivityAttimestamps, supporting accurate enforcement of idle and max lifetime policies.Remove cookies from JSON responses:
Sending the cookies in a JSON response completely defeats the security benefits of HTTPOnly cookies. Cookies are no longer returned in JSON responses after this PR gets merged. This is technically a breaking change, as it requires changes to the Shuffle frontend (which I propose in a PR to Shuffle/Shuffle).
These changes collectively enhance the security and reliability of session management throughout the application.