fix: profile password change now works and is verified (#99)#106
Open
christianromeni wants to merge 2 commits into
Open
fix: profile password change now works and is verified (#99)#106christianromeni wants to merge 2 commits into
christianromeni wants to merge 2 commits into
Conversation
The profile page sent current_password/new_password to PATCH /users/:id, whose body struct only knows "password", so the fields were silently dropped: the handler returned 200 while the stored hash never changed. The generic update path also performed no current-password check. Add a dedicated self-service endpoint POST /me/password that verifies the current password with bcrypt before changing it, rejects SSO/ password-less accounts, and enforces the 8..128 length bounds. On success it revokes the user's other sessions (RevokeUserSessionsExcept) while keeping the current one valid in both the DB and the cache, and writes an audit event. SSO/no-password paths burn a bcrypt comparison so response timing does not reveal the account type. The profile UI calls the new endpoint and surfaces real errors instead of always reporting success. Fixes #99
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
bcrypt rejects inputs over 72 bytes, so a 73-128 char new_password passed validation and then 500ed. Cap at 72 bytes and treat ErrPasswordTooLong as 400. Session revocation was best-effort: if it failed the handler still returned 200, leaving other sessions live in the DB to re-enter the cache on refresh. Password update and other-session revocation now run in one transaction (ChangePasswordAndRevokeOtherSessions); on failure nothing commits and the handler returns 500 instead of a false success.
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
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.
Fixes #99
Problem
The profile page sent
{current_password, new_password}toPATCH /users/:id, whose body struct only declarespassword. Go's JSON decoder silently drops the unknown fields, soreq.Password == nil: the handler ran no password update and returned 200, the UI showed "password changed", and the stored hash never changed. The generic update path also never verified the current password.Fix
Dedicated self-service endpoint
POST /me/password:current_passwordwith bcrypt before changing anything; wrong current → 400 "current password is incorrect".RevokeUserSessionsExcept) while keeping the current session valid in both the DB and the cache - so the user stays logged in on the current device and any stolen session elsewhere is killed.The profile UI calls the new endpoint and surfaces the real backend error instead of always reporting success.
Known limitation
There is no per-attempt throttle on the current-password check yet (the caller already holds a valid session token). This will be wired through the login-throttle infrastructure from #104 once that lands, rather than adding a second throttle here.
Tests
go test -race ./internal/api/admin/ ./internal/db/andtsc --noEmitclean.