fix: configure auth cookie secure flag by environment#102
fix: configure auth cookie secure flag by environment#102ManthanSiroya wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughAdds a ChangesCookie Domain and Secure Flag Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
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. Comment |
|
Hi @roshankumar0036singh , I am recreating this PR because the previous one was automatically closed after the head repository (my fork) was deleted accidentally. This PR contains the same implementation and changes as the original PR. Re-verified locally before submission. |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
@ManthanSiroya isProd source is inconsistent there is a bug risk here The main thing to fix is the inconsistency between h.isProd (new, config-based) and the pre-existing gin.Mode() == gin.ReleaseMode checks in the OAuth handlers pick one source of truth and use it everywhere |
|
All contributors have signed the CLA. ✅ Thank you! |
|
Addressed the inconsistency by updating OAuth cookie handling to use the config-based This makes production detection consistent across login and OAuth cookie flows. |
|
recheck |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/handler/auth_handler_test.go`:
- Line 23: The test file for auth_handler_test.go currently instantiates
NewAuthHandler with isProd=false and empty domain strings at multiple locations,
which fails to validate the production cookie security behavior added in this
PR. Add at least two new test functions: one that tests the login flow with
NewAuthHandler instantiated using isProd=true and a non-empty domain value, and
another that tests the OAuth-state flow with the same production parameters. In
each test, verify that the Set-Cookie response header contains both the Secure
flag and the expected Domain value to ensure the cookie security behavior works
correctly in production mode.
In `@internal/routes/routes.go`:
- Line 65: The production detection on line 65 in the NewAuthHandler call uses a
simple string equality check that is vulnerable to case sensitivity and
whitespace issues. Harden this check by normalizing cfg.App.Env before comparing
it to "production" — convert it to lowercase and trim any whitespace before the
equality comparison. This ensures that environment variables with different
casing or accidental whitespace do not silently disable cookie security in
production deployments.
🪄 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: 61b67b3d-138f-4de0-96bf-14f4ffe154dc
📒 Files selected for processing (6)
.env.exampleinternal/config/config.gointernal/handler/auth_handler.gointernal/handler/auth_handler_protected_test.gointernal/handler/auth_handler_test.gointernal/routes/routes.go
|
Addressed the production environment detection concern by making the |
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/handler/auth_handler_test.go (2)
260-334: 💤 Low valueExcellent test coverage for production cookie security!
This test successfully addresses the past review feedback and validates that
auth_tokencookies are markedSecurewith the correctDomainin production mode.Minor improvements:
Use
httptest.NewRequestWithContextinstead ofhttptest.NewRequest(Lines 288, 306): The linter correctly flags thatNewRequestWithContextshould be preferred for proper context propagation and cancellation support.Remove redundant nil check (Lines 330-333): The
if authCookie != nilguard is unnecessary becauserequire.NotNil(t, authCookie)on Line 328 already fails the test immediately if the cookie is nil.♻️ Suggested cleanup
req = httptest.NewRequest( http.MethodPost, "/api/auth/login", bytes.NewBuffer(loginJSON), ) + // Or use: req = httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/auth/login", bytes.NewBuffer(loginJSON)) req.Header.Set("Content-Type", "application/json")require.NotNil(t, authCookie) - if authCookie != nil { - assert.True(t, authCookie.Secure) - assert.Equal(t, "example.com", authCookie.Domain) - } + assert.True(t, authCookie.Secure) + assert.Equal(t, "example.com", authCookie.Domain)🤖 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 `@internal/handler/auth_handler_test.go` around lines 260 - 334, In the TestAuthHandler_Login_ProductionCookieSecurity test function, replace both httptest.NewRequest calls with httptest.NewRequestWithContext to enable proper context propagation and cancellation support as preferred by the linter. Additionally, remove the redundant if authCookie != nil block after the require.NotNil assertion since that assertion already guarantees the cookie is not nil and will fail the test immediately if it is.Source: Linters/SAST tools
336-389: 💤 Low valueExcellent test coverage for OAuth state cookie security!
This test successfully validates that
oauth_statecookies are markedSecurewith the correctDomainin production mode, even when the OAuth flow encounters an error.Minor improvements (same as the login test):
Use
httptest.NewRequestWithContextinstead ofhttptest.NewRequest(Line 364): PreferNewRequestWithContextfor proper context propagation.Remove redundant nil check (Lines 385-388): The
if stateCookie != nilguard is unnecessary becauserequire.NotNil(t, stateCookie)on Line 383 already fails the test if the cookie is nil.♻️ Suggested cleanup
req := httptest.NewRequest( http.MethodGet, "/api/auth/google/login?client_id=test-client", nil, ) + // Or use: req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/api/auth/google/login?client_id=test-client", nil)require.NotNil(t, stateCookie) - if stateCookie != nil { - assert.True(t, stateCookie.Secure) - assert.Equal(t, "example.com", stateCookie.Domain) - } + assert.True(t, stateCookie.Secure) + assert.Equal(t, "example.com", stateCookie.Domain)🤖 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 `@internal/handler/auth_handler_test.go` around lines 336 - 389, In the TestAuthHandler_GoogleLogin_ProductionCookieSecurity test function, replace the httptest.NewRequest call with httptest.NewRequestWithContext to ensure proper context propagation. Additionally, remove the redundant if stateCookie != nil check that guards the assertions at the end of the function, since the require.NotNil call immediately before it already validates that stateCookie is not nil and would fail the test if it were, making the additional guard unnecessary.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@internal/handler/auth_handler_test.go`:
- Around line 260-334: In the TestAuthHandler_Login_ProductionCookieSecurity
test function, replace both httptest.NewRequest calls with
httptest.NewRequestWithContext to enable proper context propagation and
cancellation support as preferred by the linter. Additionally, remove the
redundant if authCookie != nil block after the require.NotNil assertion since
that assertion already guarantees the cookie is not nil and will fail the test
immediately if it is.
- Around line 336-389: In the
TestAuthHandler_GoogleLogin_ProductionCookieSecurity test function, replace the
httptest.NewRequest call with httptest.NewRequestWithContext to ensure proper
context propagation. Additionally, remove the redundant if stateCookie != nil
check that guards the assertions at the end of the function, since the
require.NotNil call immediately before it already validates that stateCookie is
not nil and would fail the test if it were, making the additional guard
unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d31f7d17-9434-491d-b088-cc530f01de75
📒 Files selected for processing (3)
cmd/server/main.gointernal/handler/auth_handler_test.gointernal/routes/routes.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/routes/routes.go
|
@ManthanSiroya Fantastic work adding explicit test vectors (TestAuthHandler_Login_ProductionCookieSecurity) to cover our production cookie domain parameters! This directly ensures our sub-domain context isolation won't suffer from silent regressions in future rollouts. I have left two action items on the file: OAuth Test Realignment: We need to mock seed our test DB data rather than asserting against an HTTP 500 context block inside our Google Login security test case. This prevents pipeline test flakes down the road. Dry Optimization: Centralizing the environment string cleanup behavior straight into our core config structural pattern for better internal reuse This sanitization routine (strings.EqualFold(strings.TrimSpace(...))) is duplicated across our application entry layers.(reson for 2.5% duplication detection) |
|
I wanted to share an update regarding this PR. I’ve spent 5–6+ hours working through the cookie security fixes, production handling updates, review feedback, testing, and follow-up changes. I’m currently unable to continue further on the remaining review items for this PR. I’ve already pushed the latest completed fixes and tried to address the requested feedback as much as possible. If the completed work is acceptable, I’d really appreciate consideration for merge and recognition of the effort already contributed. For the remaining requested improvements/comments, I kindly request creating separate follow-up issues so they can be worked on independently later. Thanks for the reviews and guidance throughout the process. |
2 participants
Description
This PR fixes the issue where the auth cookie had
Secure: falsehardcoded, which could expose authentication cookies over unencrypted HTTP connections in production.Changes Made
Secureflag dynamically based on the environment.Secure: truein production mode.Why This Change?
Previously, the auth cookie was always created with
Secure: false, allowing cookies to be sent over non-HTTPS connections even in production. This change improves security by ensuring cookies are transmitted securely in production environments.Testing
go test ./...Checklist
I have read the CLA and agree to its terms.on this PR.Fixes #57
Summary by CodeRabbit
Release Notes
New Features
COOKIE_DOMAINconfiguration to control the domain used for authentication-related cookies.Secure) for production deployments.Bug Fixes
Tests
Secureattribute and domain for login and OAuth flows.