fix(verify): resolve phone change by one-time token, not phone alone - #2701
fix(verify): resolve phone change by one-time token, not phone alone#2701hamodywe wants to merge 1 commit into
Conversation
phone_change carries no uniqueness constraint, so several users can hold the same pending value at once (abandoned or still-unverified change requests). Verification looked the user up with FindUserByPhoneChangeAndAudience, which matches on phone_change and audience only and returns whichever row comes first — potentially a different user than the one the token was issued to. Email change already avoids this by resolving its user through the one-time token (FindUserForEmailChange). This applies the same approach to phone change: FindUserForPhoneChange looks the user up by the token hash verify.go already computes, and confirms the resolved user's audience and pending phone match before returning them. It falls back to the previous lookup when no one-time token matches, so paths that never write one (test OTPs, rows predating the table) behave exactly as before. The only behavior change is which user is selected when duplicates exist.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| if user.Aud == aud && user.PhoneChange == phone { | ||
| return user, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return FindUserByPhoneChangeAndAudience(tx, phone, aud) |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The submitted phone/OTP can resolve a token row whose owner no longer matches the requested phone or audience (for example, a stale or cross-audience token). This fallback then picks an arbitrary user sharing phone_change; smsVerify calls ConfirmPhoneChange and issues that user a refresh token.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: When a token record is found (ott != nil) but the user validation check fails (audience or pending phone does not match), the function must return an error immediately instead of falling through to the FindUserByPhoneChangeAndAudience fallback. Add return nil, UserNotFoundError{} inside the if ott != nil block, right after the failing validation check closes (after line 319). This ensures that once a token record has been matched, only the exact user that token belongs to can be returned — no other user can be substituted via the fallback lookup. The fallback at line 323 remains valid and untouched for the legitimate case where ott == nil (no token exists, e.g. test OTPs or legacy rows).
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if user.Aud == aud && user.PhoneChange == phone { | |
| return user, nil | |
| } | |
| } | |
| } | |
| return FindUserByPhoneChangeAndAudience(tx, phone, aud) | |
| if user.Aud == aud && user.PhoneChange == phone { | |
| return user, nil | |
| } | |
| return nil, UserNotFoundError{} | |
| } | |
| } | |
| return FindUserByPhoneChangeAndAudience(tx, phone, aud) |
| return nil, err | ||
| } | ||
| if ott != nil { | ||
| user, err := FindUserByID(tx, ott.UserID) |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
Resolving the token owner with FindUserByID bypasses the previous lookup's is_sso_user = false boundary. A submitted phone OTP for an SSO-managed account with a pending change can therefore reach ConfirmPhoneChange, mutate the SSO-controlled phone, and issue a session despite the local user-update policy forbidding such changes.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a !user.IsSSOUser check to the condition at line 317 (within the same if ott != nil block) so that SSO-managed accounts are excluded when resolving the user through the one-time token path, mirroring the is_sso_user = false constraint that FindUserByPhoneChangeAndAudience enforces at the SQL level. This prevents a phone-change OTP for an SSO user from reaching ConfirmPhoneChange and issuing a session.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| user, err := FindUserByID(tx, ott.UserID) | |
| if user.Aud == aud && user.PhoneChange == phone && !user.IsSSOUser { |
1 participant
Closes #2430.
phone_changecarries no uniqueness constraint, so several users can hold the same pending value at once (abandoned or still-unverified change requests). Verification looked the user up withFindUserByPhoneChangeAndAudience, which matches onphone_changeand audience only and returns whichever row comes first — potentially a different user than the one the token was issued to.Email change already avoids this by resolving its user through the one-time token (
FindUserForEmailChange). This applies the same approach to phone change:FindUserForPhoneChangelooks the user up by the token hashverify.goalready computes, and confirms the resolved user's audience and pending phone match before returning them.It falls back to the previous lookup when no one-time token matches, so paths that never write one (test OTPs, rows predating the table) behave exactly as before. The only behaviour change is which user is selected when duplicates exist.
Verification
must resolve the user the phone-change token was issued to— and passes with it.internal/modelspackage suite is green against a live database.internal/api/phone.gostorescrypto.GenerateTokenHash(phone, otp)as both the user column and the one-time token hash, which is exactly whatverify.gorecomputes from the submitted OTP.