Validate users before confirming new accounts#4456
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #2916 by requiring authentication and validating the logged-in user before allowing account confirmation via /users/confirm/:token, preventing confirmation with only possession of the link.
Changes:
- Moved
GET/POST /users/confirm/:tokenbehind:require_authenticated_userin the router. - Added controller checks to ensure the confirmation token’s user matches
current_user, with error handling for mismatches/invalid tokens. - Updated controller tests to cover unauthenticated redirects and wrong-user token usage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/lightning_web/router.ex |
Moves confirm-by-token routes into the authenticated browser scope. |
lib/lightning_web/controllers/user_confirmation_controller.ex |
Validates token ownership against current_user for edit/update flows. |
lib/lightning/accounts.ex |
Adds get_user_by_confirmation_token/1 helper to resolve token → user. |
test/lightning_web/controllers/user_confirmation_controller_test.exs |
Updates tests for authentication requirement and identity enforcement. |
CHANGELOG.md |
Documents the security fix under “Fixed”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I have addressed the Copilot AI comments:
All tests are now passing locally. |
midigofrank
left a comment
There was a problem hiding this comment.
Hey @sakibsadmanshajib , great job here. I have 2 change requests:
- For security reasons, we should not disclose that the
tokenbelongs to another user, let's just tell them that it's an invalid token - Can we extract out the logic that verifies the token into a function plug? That way it can be reused by the
updateaction. Something like:
plug :verify_confirmation_token when action in [:edit, :update]
defp verify_confirmation_token(conn, _opts) do
end0eb4885 to
956689d
Compare
midigofrank
left a comment
There was a problem hiding this comment.
Nicely done @sakibsadmanshajib
stuartc
left a comment
There was a problem hiding this comment.
Looks good, thanks! I'm adding one test for the silent redirect if the user is already confirmed.
Description
This PR fixes #2916 by adding user validation before confirming new accounts. Previously, a user clicking a confirmation link could confirm an account without being logged in or proving their identity, leading to potential security risks (e.g., confirming someone else's account if they had the link).
The changes include:
GET /users/confirm/:tokenandPOST /users/confirm/:tokenroutes behind the:require_authenticated_userplug inlib/lightning_web/router.ex. This ensures that users must be logged in to access the confirmation page.UserConfirmationController.edit/2andUserConfirmationController.update/2to verify that the logged-in user matches the user associated with the confirmation token. If they do not match, an error is displayed and the user is redirected.UserConfirmationControllerTest.exsto reflect these changes, testing both the redirection for unauthenticated users and the identity check for logged-in users.Validation steps
Additional notes for the reviewer
The
UserConfirmationControllerlogic now explicitly checksconn.assigns.current_user.idagainst the token's user ID. This check is redundant with the router'srequire_authenticated_userfor the presence of a user, but critical for ensuring the correct user is confirming the account.AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)