Fix #1667: require password verification for account email changes#1741
Closed
Yourdaylight wants to merge 1 commit into
Closed
Fix #1667: require password verification for account email changes#1741Yourdaylight wants to merge 1 commit into
Yourdaylight wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a re-authentication requirement for email changes in the account settings page. When users attempt to update their email, they must provide their current password. The system validates credentials via sign-in, handling MFA and invalid password errors, then proceeds with the email update on successful re-authentication. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Form as Form Component
participant Auth as Authentication Service
participant Email as Email Service
participant State as Component State
User->>Form: Submit with email + current_password
Form->>Form: Validate current_password provided
alt Password Missing
Form->>User: Show "Password required" error
else Password Provided
Form->>Auth: signInWithPassword(current_password)
alt MFA Required
Auth->>User: MFA challenge required
Form->>User: Show "MFA verification needed" message
else Invalid Password
Auth->>User: Authentication failed
Form->>User: Show "Invalid password" error
else Success
Auth->>Form: Re-authentication successful
Form->>Email: Update email address
Email->>Form: Email updated
Form->>State: Reset password field & state
Form->>User: Show success confirmation
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/settings/account/index.vue (2)
12-16:⚠️ Potential issue | 🟡 MinorFix import order to keep ESLint green.
iconPasswordshould be ordered beforeiconNameto satisfyperfectionist/sort-imports.Suggested diff
import iconEmail from '~icons/heroicons/envelope?raw' import iconFlag from '~icons/heroicons/flag?raw' +import iconPassword from '~icons/heroicons/key?raw' import iconName from '~icons/heroicons/user?raw' -import iconPassword from '~icons/heroicons/key?raw'As per coding guidelines:
src/**/*.{vue,ts,js}Frontend ESLint linting must pass before commits usingbun lint:fixcommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/index.vue` around lines 12 - 16, The imports in this file are out of order for the perfectionist/sort-imports rule: move the import of iconPassword (from '~icons/heroicons/key?raw') to appear before iconName (from '~icons/heroicons/user?raw') so that the import order is corrected; locate the import block containing IconVersion, iconEmail, iconFlag, iconName, iconPassword and reorder only the iconPassword and iconName lines (keeping other imports in place) then run bun lint:fix to verify ESLint passes.
445-455:⚠️ Potential issue | 🟠 MajorFail fast on any
updateUsererror before syncingusers.email.Only checking
AuthApiErroris too narrow; other failures can still fall through and persistform.emailinusers, desynchronizing auth and profile state.Suggested diff
- const data = await supabase.auth.updateUser({ email: form.email }) + const data = await supabase.auth.updateUser({ email: form.email }) reset('update-account', useMainStore().user) - if (data.error && data.error.name === 'AuthApiError') { + if (data.error) { isLoading.value = false - return toast.error('email already taken') + setErrors('update-account', [t('account-error')], {}) + return } toast.success('A confirmation email was sent click to link to confirm your new email', { duration: 10000, }) updateData.email = form.email🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/index.vue` around lines 445 - 455, The code calls supabase.auth.updateUser(...) then immediately resets state and later only checks for AuthApiError by name; move the error handling to immediately after await updateUser and fail fast on any data.error (not just when data.error.name === 'AuthApiError'): if data.error is truthy, set isLoading.value = false and return toast.error(...) without calling reset or updating updateData.email; only when there is no data.error call reset('update-account', useMainStore().user), show the success toast, and set updateData.email = form.email.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/settings/account/index.vue`:
- Around line 425-426: The hardcoded user-facing strings passed into setErrors
and other places (e.g., the 'Current password is required to change your email.'
message used with setErrors('update-account', ...), plus the other messages at
the same block and at the positions referenced) must be replaced with i18n
lookups and new locale keys; update the calls to use this.$t or the page's i18n
function (for example this.$t('settings.account.currentPasswordRequired'))
instead of raw English, add corresponding keys in the locale JSON/YAML (e.g.,
settings.account.currentPasswordRequired, settings.account.emailChangeHelp,
settings.account.passwordMismatch, etc.), and ensure you pass the translated
strings into setErrors('update-account', [this.$t('...')], {}) and any other UI
text spots listed so all new messages are localized consistently with the rest
of the page.
- Around line 422-431: The password verification step is using the profile email
(main.user?.email) which may be out-of-sync with the auth identity; update the
signInWithPassword call to use the auth identity email (main.auth?.email)
instead of main.user?.email, and add a guard that handles the case where
main.auth?.email is missing (setErrors('update-account', [...], {}) and stop the
flow). Update the call site where supabase.auth.signInWithPassword is invoked
(alongside form.current_password) and keep existing isLoading and setErrors
handling consistent.
---
Outside diff comments:
In `@src/pages/settings/account/index.vue`:
- Around line 12-16: The imports in this file are out of order for the
perfectionist/sort-imports rule: move the import of iconPassword (from
'~icons/heroicons/key?raw') to appear before iconName (from
'~icons/heroicons/user?raw') so that the import order is corrected; locate the
import block containing IconVersion, iconEmail, iconFlag, iconName, iconPassword
and reorder only the iconPassword and iconName lines (keeping other imports in
place) then run bun lint:fix to verify ESLint passes.
- Around line 445-455: The code calls supabase.auth.updateUser(...) then
immediately resets state and later only checks for AuthApiError by name; move
the error handling to immediately after await updateUser and fail fast on any
data.error (not just when data.error.name === 'AuthApiError'): if data.error is
truthy, set isLoading.value = false and return toast.error(...) without calling
reset or updating updateData.email; only when there is no data.error call
reset('update-account', useMainStore().user), show the success toast, and set
updateData.email = form.email.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52f3d26e-9b48-4426-9966-6aaa74f35693
📒 Files selected for processing (1)
src/pages/settings/account/index.vue
Comment on lines
422
to
+431
| if (main.user?.email !== form.email) { | ||
| if (!form.current_password?.trim()) { | ||
| isLoading.value = false | ||
| setErrors('update-account', ['Current password is required to change your email.'], {}) | ||
| return | ||
| } | ||
|
|
||
| const { error: verifyError } = await supabase.auth.signInWithPassword({ | ||
| email: main.user.email, | ||
| password: form.current_password, |
Contributor
There was a problem hiding this comment.
Use auth identity email for password verification, not profile email.
This branch authenticates with main.user.email, which is profile data and can drift from the active auth identity. Use main.auth?.email as the verification principal.
Suggested diff
if (main.user?.email !== form.email) {
+ if (!main.auth?.email) {
+ isLoading.value = false
+ setErrors('update-account', [t('account-error')], {})
+ return
+ }
+
if (!form.current_password?.trim()) {
isLoading.value = false
setErrors('update-account', ['Current password is required to change your email.'], {})
return
}
const { error: verifyError } = await supabase.auth.signInWithPassword({
- email: main.user.email,
+ email: main.auth.email,
password: form.current_password,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/settings/account/index.vue` around lines 422 - 431, The password
verification step is using the profile email (main.user?.email) which may be
out-of-sync with the auth identity; update the signInWithPassword call to use
the auth identity email (main.auth?.email) instead of main.user?.email, and add
a guard that handles the case where main.auth?.email is missing
(setErrors('update-account', [...], {}) and stop the flow). Update the call site
where supabase.auth.signInWithPassword is invoked (alongside
form.current_password) and keep existing isLoading and setErrors handling
consistent.
Comment on lines
+425
to
+426
| setErrors('update-account', ['Current password is required to change your email.'], {}) | ||
| return |
Contributor
There was a problem hiding this comment.
Localize the newly added user-facing strings.
These new messages/help text are hardcoded English; please move them to i18n keys for consistency with the rest of this page.
Also applies to: 437-437, 451-452, 581-581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/settings/account/index.vue` around lines 425 - 426, The hardcoded
user-facing strings passed into setErrors and other places (e.g., the 'Current
password is required to change your email.' message used with
setErrors('update-account', ...), plus the other messages at the same block and
at the positions referenced) must be replaced with i18n lookups and new locale
keys; update the calls to use this.$t or the page's i18n function (for example
this.$t('settings.account.currentPasswordRequired')) instead of raw English, add
corresponding keys in the locale JSON/YAML (e.g.,
settings.account.currentPasswordRequired, settings.account.emailChangeHelp,
settings.account.passwordMismatch, etc.), and ensure you pass the translated
strings into setErrors('update-account', [this.$t('...')], {}) and any other UI
text spots listed so all new messages are localized consistently with the rest
of the page.
Member
|
AI spam |
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
2 participants
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.
Summary (AI generated)
updateUser({ email }).current_passwordinput in account settings and show explicit errors for missing/invalid verification.Motivation (AI generated)
Issue #1667 tracks security hardening tasks. Changing a sensitive identity field (email) without password re-verification weakens account takeover protections when a session is exposed.
Business Impact (AI generated)
This reduces account hijack risk, improves trust in account security settings, and aligns email change flow with expected security controls for sensitive profile operations.
Test Plan (AI generated)
bun test:backend(cannot run in this environment: Bun runtime is unavailable)Closes #1667
/claim #1667
Generated with AI
Summary by CodeRabbit
Release Notes
Security
Improvements