fix: set cookies alongside header when SendJWTHeader is enabled #262
Conversation
When SendJWTHeader is true, now sets both the JWT header AND cookies. This fixes OAuth authentication flows where HTTP headers don't survive browser redirects. Cookies are needed for the OAuth callback to complete successfully, while headers are still set for direct API calls. Fixes umputun/remark42#1877
Pull Request Test Coverage Report for Build 20402130242Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 20442452487Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR fixes OAuth authentication failures when SendJWTHeader is enabled by setting both JWT headers and cookies instead of only headers. The issue occurs because HTTP headers don't survive browser redirects during OAuth callback flows, causing the JWT token to be lost.
Key Changes:
- Modified JWT token setting behavior to always set cookies alongside headers when
SendJWTHeader=true - Updated tests to verify both cookies and headers are set correctly in header mode
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| token/jwt.go | Removed early return in Set() function to allow cookie setting after header is set |
| v2/token/jwt.go | Same fix as token/jwt.go for the v2 module version |
| token/jwt_test.go | Updated TestJWT_SendJWTHeader to verify both cookies and header are set |
| v2/token/jwt_test.go | Same test update as token/jwt_test.go for the v2 module version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require.Equal(t, 2, len(cookies), "cookies set alongside header") | ||
| assert.Equal(t, "JWT", cookies[0].Name) | ||
| assert.Equal(t, testJwtValid, cookies[0].Value) | ||
| assert.Equal(t, "XSRF-TOKEN", cookies[1].Name) |
There was a problem hiding this comment.
The test verifies that the XSRF-TOKEN cookie exists but does not check its value. For consistency with other tests (like TestJWT_SetWithDomain at line 241), this should also verify that the XSRF token value matches the expected claims ID ("random id" from testClaims). Consider adding an assertion like: assert.Equal(t, "random id", cookies[1].Value)
| require.Equal(t, 2, len(cookies), "cookies set alongside header") | ||
| assert.Equal(t, "JWT", cookies[0].Name) | ||
| assert.Equal(t, testJwtValid, cookies[0].Value) | ||
| assert.Equal(t, "XSRF-TOKEN", cookies[1].Name) |
There was a problem hiding this comment.
The test verifies that the XSRF-TOKEN cookie exists but does not check its value. For consistency with other tests (like TestJWT_SetWithDomain at line 241), this should also verify that the XSRF token value matches the expected claims ID ("random id" from testClaims). Consider adding an assertion like: assert.Equal(t, "random id", cookies[1].Value)
verify XSRF-TOKEN cookie value matches claims ID for consistency with TestJWT_SetWithDomain
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
LGTM - fixes OAuth redirect flow when SendJWTHeader is enabled, test improved to verify XSRF cookie value
Summary
SendJWTHeaderistrue, now sets both the JWT header AND cookiesProblem
When
SendJWTHeader=true, OAuth authentication fails because:Solution
Always set cookies alongside the header when
SendJWTHeader=true. This way:This is a minimal, backwards-compatible change - clients expecting headers still get them.
Might fix umputun/remark42#1877, not sure before testing.