Fix session cookie issues (#626, #549)

[tphakala]

Summary

This PR fixes two related session cookie issues from upstream:

• Issue Session Cookie not setting correctly markbates/goth#626: Duplicate Set-Cookie headers causing session to be immediately invalidated
• Issue Updating dependencies breaks login: securecookie: hash key is not set markbates/goth#549: "securecookie: hash key is not set" error after dependency upgrade

Changes

1. Removed unnecessary StoreInSession call in CompleteUserAuth

   • Prevents duplicate Set-Cookie headers (one valid, one expired)

2. Upgraded dependencies

   • github.com/gorilla/sessions v1.1.1 → v1.4.0
   • github.com/gorilla/securecookie v1.1.1 → v1.1.2

3. Added tests for regression prevention

Note

Also submitted as upstream PR: markbates#631

If upstream doesn't merge, this fork will have the fix.

Summary by CodeRabbit

• Bug Fixes

  • Fixed duplicate Set-Cookie header being emitted during OAuth authentication completion, improving session handling consistency and preventing header-related conflicts.

• Tests

  • Added comprehensive tests validating correct Set-Cookie header behavior and session error handling during authentication flows.

• Chores

  • Updated session management and security dependencies to latest stable versions for enhanced reliability and security.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The PR updates gorilla/sessions and securecookie dependencies, removes redundant session storage from CompleteUserAuth to prevent duplicate Set-Cookie headers during OAuth flow completion, and adds tests verifying error handling and single Set-Cookie header emission.

Changes

Cohort / File(s)	Summary
Dependency Updates go.mod	Upgraded github.com/gorilla/sessions from v1.1.1 to v1.4.0; updated indirect dependency github.com/gorilla/securecookie from v1.1.1 to v1.1.2
Core OAuth Flow gothic/gothic.go	Removed StoreInSession call in CompleteUserAuth function; added comment explaining session is not persisted because it is only used during OAuth flow and is cleared by deferred Logout, preventing duplicate Set-Cookie header
Test Coverage gothic/gothic_test.go	Added failingStore test double to simulate session save errors; introduced Test_CompleteUserAuth_SingleSetCookie to verify exactly one Set-Cookie header (logout cookie) is emitted; added tests for error propagation in StoreInSession and GetAuthURL

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix session cookie issues (markbates#626, markbates#549)' directly and clearly summarizes the main changes: fixing session cookie-related problems identified in two GitHub issues.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/session-cookie-issues-626-549

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 15b24f5 and 15ece9c.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (3)

• go.mod
• gothic/gothic.go
• gothic/gothic_test.go

🧰 Additional context used

🧬 Code graph analysis (1)

gothic/gothic_test.go (2)

gothic/gothic.go (5)

• Store (32-32)
• StoreInSession (260-268)
• GetAuthURL (117-148)
• SessionName (29-29)
• CompleteUserAuth (159-215)

session.go (1)

• Session (13-21)

🪛 ast-grep (0.40.3)

gothic/gothic_test.go

[warning] 363-363: A secret is hard-coded in the application. Secrets stored in source code, such as credentials, identifiers, and other types of sensitive data, can be leaked and used by internal or external malicious actors. It is recommended to rotate the secret and retrieve them from a secure secret vault or Hardware Security Module (HSM), alternatively environment variables can be used if allowed by your company policy.
Context: sessions.NewCookieStore([]byte("test-secret-key-32-bytes-long!!"))
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://owasp.org/Top10/A07_2021-Identification_and_Authentication_Failures

(gorilla-cookie-store-hardcoded-session-key-go)

🔇 Additional comments (6)

gothic/gothic.go (1)

207-211: Excellent fix with clear documentation.

The removal of the StoreInSession call is well-reasoned. Since Logout is deferred at Line 178 and clears the session, storing the updated session here would indeed produce a duplicate Set-Cookie header (one valid, one expired). The comment clearly documents the rationale and references the upstream issue.

gothic/gothic_test.go (4)

294-314: LGTM: Well-structured error propagation test.

The test properly verifies that StoreInSession propagates errors from session.Save(). Using a failingStore is an appropriate test double pattern, and the defer cleanup ensures the original store is restored.

---

316-337: LGTM: Comprehensive end-to-end error handling test.

This test validates that GetAuthURL correctly propagates session errors, addressing the securecookie hash key validation issue (markbates#549). The test structure mirrors real-world failure scenarios.

---

339-354: LGTM: Clean test helper implementation.

The failingStore is a well-designed test double that correctly implements the sessions.Store interface. It's minimal, focused, and reusable across multiple test cases.

---

356-411: LGTM: Thorough validation of the duplicate Set-Cookie fix.

This test comprehensively validates the fix for issue markbates#626 by:

1. Simulating the complete OAuth flow with a real cookie store
2. Verifying exactly one Set-Cookie header is emitted
3. Confirming it's the logout cookie with Max-Age=0

The test design correctly exercises the production code path and validates the desired behavior.

Note on static analysis warning (Line 364): The hardcoded secret "test-secret-key-32-bytes-long!!" is flagged by static analysis, but this is expected and safe in test code. Using a fixed secret in tests ensures reproducible behavior and is a common testing pattern.

go.mod (1)

10-10: Dependency versions are current and secure.

gorilla/sessions v1.4.0 (released 2024-08-20) and gorilla/securecookie v1.1.2 (released 2023-10-18) are both at their latest releases. No public security advisories or CVEs identified for these versions.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	github.com/​gorilla/​sessions@​v1.1.1 ⏵ v1.4.0

View full report

[gemini-code-assist]

Summary of Changes

Hello @tphakala, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the stability and correctness of session cookie handling within the application. It resolves two critical upstream issues: one that caused immediate session invalidation due to duplicate Set-Cookie headers, and another that led to "securecookie: hash key is not set" errors after dependency upgrades. The changes ensure that user sessions are managed reliably and prevent common authentication-related failures, contributing to a more robust user experience.

Highlights

• Fixes Duplicate Set-Cookie Headers: Addresses issue Session Cookie not setting correctly markbates/goth#626 by removing an unnecessary StoreInSession call within CompleteUserAuth, preventing the generation of duplicate Set-Cookie headers that caused immediate session invalidation.
• Resolves Securecookie Hash Key Error: Fixes issue Updating dependencies breaks login: securecookie: hash key is not set markbates/goth#549, which reported 'securecookie: hash key is not set' errors, by upgrading the gorilla/sessions and gorilla/securecookie dependencies to their latest versions.
• Dependency Upgrades: Updated github.com/gorilla/sessions from v1.1.1 to v1.4.0 and github.com/gorilla/securecookie from v1.1.1 to v1.1.2.
• Enhanced Test Coverage: Added new test cases to prevent regressions, specifically verifying error propagation from session operations in StoreInSession and GetAuthURL, and ensuring CompleteUserAuth produces only a single Set-Cookie header.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively resolves two session cookie issues by upgrading dependencies and removing a redundant session storage call. The changes are logical and well-justified. Upgrading gorilla/sessions and gorilla/securecookie addresses an error related to unset hash keys, while removing the StoreInSession call in CompleteUserAuth correctly prevents the issuance of duplicate Set-Cookie headers. The addition of new regression tests is a great improvement, as they validate both fixes, including proper error propagation from session save operations and ensuring only a single logout cookie is set upon completion of user authentication. The changes are sound and the test coverage for them is thorough.