Repository navigation
fix(auth): invalidate JWT tokens on password change or reset - #2199
Conversation
Add PasswordChangedAt field to User and embed it as a Unix-second claim in JWT tokens. GetUserFromToken rejects any token issued before the user's last password change, closing the window where an attacker with a stolen token retains access after the victim resets their password. Old tokens (no pwd_changed_at claim) map to epoch and are rejected once the user changes their password for the first time. Tokens issued after a password change remain valid. Users who never change their password are unaffected.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements password-change-based token invalidation. The ChangesPassword-change-based token invalidation
Sequence DiagramsequenceDiagram
participant Client
participant GenerateToken
participant GetUserFromToken
participant UserStore
Client->>GenerateToken: request token
GenerateToken->>UserStore: fetch user
GenerateToken-->>Client: token with pwd_changed_at_ns (maybe zero)
Client->>Client: user password changed
Client->>GetUserFromToken: validate token
GetUserFromToken->>UserStore: fetch current user
UserStore-->>GetUserFromToken: user with PasswordChangedAt set
GetUserFromToken-->>Client: ErrInvalidToken (if token's pwd_changed_at_ns < user's PasswordChangedAt.UnixNano())
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/auth/service_test.go (1)
640-744: ⚡ Quick winAdd token-invalidation coverage for the
UpdateUserpassword path.These new cases are great, but they don’t cover the third password mutation flow (
UpdateUserwithPassword) that now setsPasswordChangedAtin service logic. A focused test here would close the remaining invalidation gap.🤖 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/service/auth/service_test.go` around lines 640 - 744, Add a test that covers the UpdateUser password mutation path: create a user via CreateUser, issue a token with GenerateToken, call svc.UpdateUser(...) to set a new Password (the path that should set PasswordChangedAt), then assert the old token is rejected by GetUserFromToken (ErrInvalidToken) and that a freshly generated token for the updated user (fetch with GetUser, then GenerateToken) is accepted; reference UpdateUser, GenerateToken, GetUserFromToken and GetUser to find where to add the test modeled after the existing password-change/reset tests.
🤖 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/service/auth/service.go`:
- Around line 185-188: The code currently truncates PasswordChangedAt to second
precision (pwdChangedAt = user.PasswordChangedAt.Unix() and
time.Now().UTC().Truncate(time.Second)) which allows same-second password
changes to collide; change the token claim and comparisons to use nanosecond
precision (UnixNano) instead: populate pwdChangedAt with
user.PasswordChangedAt.UnixNano(), stop truncating now to seconds, decode
claims.PasswordChangedAt as nanoseconds via time.Unix(0,
claims.PasswordChangedAt), and perform the invalidation check using the
high-precision times (e.g., tokenPwdChangedAt.Before(user.PasswordChangedAt)
with nanosecond resolution) so two changes within the same second cannot produce
equal timestamps; update any variables/names referencing pwdChangedAt,
claims.PasswordChangedAt, time.Now().UTC().Truncate, and time.Unix(...)
accordingly.
---
Nitpick comments:
In `@internal/service/auth/service_test.go`:
- Around line 640-744: Add a test that covers the UpdateUser password mutation
path: create a user via CreateUser, issue a token with GenerateToken, call
svc.UpdateUser(...) to set a new Password (the path that should set
PasswordChangedAt), then assert the old token is rejected by GetUserFromToken
(ErrInvalidToken) and that a freshly generated token for the updated user (fetch
with GetUser, then GenerateToken) is accepted; reference UpdateUser,
GenerateToken, GetUserFromToken and GetUser to find where to add the test
modeled after the existing password-change/reset tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29e0c716-57bd-4199-8cf6-1bc281aa1306
📒 Files selected for processing (3)
internal/auth/user.gointernal/service/auth/service.gointernal/service/auth/service_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56cd720c2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return fmt.Errorf("failed to hash password: %w", err) | ||
| } | ||
|
|
||
| now := time.Now().UTC().Truncate(time.Second) |
There was a problem hiding this comment.
PasswordChangedAt is truncated to whole seconds, so two password changes for the same user within one second can produce identical values; in that case a token minted after the first change is still accepted after the second change because GetUserFromToken only rejects when tokenPwdChangedAt.Before(*user.PasswordChangedAt). This leaves a real revocation gap for rapid consecutive ChangePassword/ResetPassword operations (for example automated admin resets).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b09565a. Switched to UnixNano() for both storage and JWT claim (renamed to pwd_changed_at_ns), removed Truncate(time.Second) from all three password-update paths, and replaced the Before() comparison with direct int64 comparison. Two password changes within the same nanosecond are now the only remaining collision scenario (effectively impossible in practice).
Switch PasswordChangedAt claim from Unix seconds to UnixNano to close a collision window where two password changes within the same second produce equal timestamps, allowing a token issued between them to bypass invalidation. Rename claim key from pwd_changed_at to pwd_changed_at_ns to make the unit explicit. Remove Truncate(time.Second) from all three password-update paths.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
After password reset, the old JWT is correctly rejected (401) because GetUserFromToken now enforces PasswordChangedAt. The test must obtain a fresh token with the new credentials before testing RBAC gating (403).
1 participant
Summary
PasswordChangedAt *time.Timeto theUserstruct (andUserForStoragefor persistence), stored with second-level precisionpwd_changed_atUnix-second claim in every newly issued JWTGetUserFromTokenrejects any token whosepwd_changed_atclaim predates the user's recordedPasswordChangedAt— closing the 24-hour window where a stolen token stays valid after the victim changes their passwordChangePassword,ResetPassword, and the password-update path inUpdateUserall stampPasswordChangedAtBackward compatibility
omitempty— existing stored user records round-trip without changepwd_changed_atclaim) decode to0(epoch), which is treated as "before any real password change" — they are rejected the first time the user changes their password, and remain valid indefinitely for users who never change their passwordTest plan
TestService_GetUserFromToken_PasswordChangeInvalidatesToken— old token rejected afterChangePasswordTestService_GetUserFromToken_NewTokenValidAfterPasswordChange— token issued after password change is acceptedTestService_GetUserFromToken_ResetPasswordInvalidatesToken— old token rejected after adminResetPasswordTestService_GetUserFromToken_NoPasswordChangeTokenStillValid— token stays valid when user never changed passwordSummary by CodeRabbit
New Features
Tests