Fix session cookie issues (#626, #549)#631
Open
tphakala wants to merge 2 commits intomarkbates:masterfrom
Open
Fix session cookie issues (#626, #549)#631tphakala wants to merge 2 commits intomarkbates:masterfrom
tphakala wants to merge 2 commits intomarkbates:masterfrom
Conversation
This commit fixes two related session cookie issues: 1. Issue markbates#626: Duplicate Set-Cookie headers - CompleteUserAuth was calling StoreInSession before the deferred Logout - This caused two Set-Cookie headers: one valid, one expired (1970) - The browser received both, with the expired one overriding the valid one - Fix: Remove the unnecessary StoreInSession call since Logout clears the session anyway 2. Issue markbates#549: "securecookie: hash key is not set" after dependency upgrade - Upgraded gorilla/sessions v1.1.1 → v1.4.0 - Upgraded gorilla/securecookie v1.1.1 → v1.1.2 - The newer securecookie properly validates hash keys instead of silently failing - Error propagation was already working correctly Tests added: - Test_CompleteUserAuth_SingleSetCookie: Verifies only one Set-Cookie header - Test_StoreInSession_ReturnsErrorOnSaveFailure: Verifies error propagation - Test_GetAuthURL_PropagatesSessionErrors: Verifies end-to-end error handling Fixes markbates#626 Fixes markbates#549
- Add lint job to CI workflow using golangci-lint-action v6 - Add .golangci.yml configuration file - Configure linter to pass on gothic/ package - Exclude common false positives (fmt.Fprint*, defer Close, etc.) - Add nolint directive for deferred Logout call - Upgrade actions/cache to v4 Note: Providers have legacy linter issues that are out of scope for this change. The gothic/ core package passes lint cleanly.
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.
Summary
This PR fixes two related session cookie issues:
Set-Cookieheaders causing session to be immediately invalidatedRoot Cause Analysis
Issue #626: Duplicate Set-Cookie Headers
In
CompleteUserAuth, the execution flow was:StoreInSession(line 207) → saves session → firstSet-Cookieheader (valid, 30 days)defer Logoutexecutes → secondSet-Cookieheader (expires 1970)The browser received both cookies, with the expired one overriding the valid one.
Issue #549: Hash Key Error
The library pinned to old gorilla/securecookie v1.1.1 which silently accepted empty
SESSION_SECRET. After users upgraded dependencies, securecookie v1.1.2+ properly validates hash keys, causing the error.Changes
Removed unnecessary
StoreInSessioncall inCompleteUserAuthLogoutanywaySet-CookieheadersUpgraded dependencies
github.com/gorilla/sessionsv1.1.1 → v1.4.0git.832008.xyz/gorilla/securecookiev1.1.1 → v1.1.2Added tests
Test_CompleteUserAuth_SingleSetCookie: Verifies only one Set-Cookie headerTest_StoreInSession_ReturnsErrorOnSaveFailure: Verifies error propagationTest_GetAuthURL_PropagatesSessionErrors: Verifies end-to-end error handlingTest plan
go vetpassesgo test ./...) passesFixes #626
Fixes #549