From 522b0ed7ed51126d14ec3477422869297ccc85d3 Mon Sep 17 00:00:00 2001 From: gayathri <66356931+grvijayan@users.noreply.github.com> Date: Fri, 14 Aug 2026 06:12:04 +0200 Subject: [PATCH] Merge commit from fork The Login V1 MFA init/enrollment handlers acted on authReq.UserID via setUserContext without verifying the auth request had completed its first factor. A request that only submitted a loginname could enroll TOTP/SMS/Email/U2F factors and overwrite the victim's phone number, and the discrepant errors bypassed IgnoreUnknownUsernames. Gate the enrollment handlers on PossibleSteps[0] == *domain.MFAPromptStep, which nextSteps only yields after firstFactorChecked passes. Co-authored-by: Claude Opus 4.8 --- internal/api/ui/login/login.go | 26 +++++++ internal/api/ui/login/mfa_init_sms.go | 3 + internal/api/ui/login/mfa_init_test.go | 76 +++++++++++++++++++ internal/api/ui/login/mfa_init_u2f.go | 3 + .../api/ui/login/mfa_init_verify_handler.go | 3 + internal/api/ui/login/mfa_prompt_handler.go | 3 + 6 files changed, 114 insertions(+) create mode 100644 internal/api/ui/login/mfa_init_test.go diff --git a/internal/api/ui/login/login.go b/internal/api/ui/login/login.go index ee2f52654..5ce2e55de 100644 --- a/internal/api/ui/login/login.go +++ b/internal/api/ui/login/login.go @@ -26,6 +26,7 @@ import ( "github.com/zitadel/zitadel/internal/form" "github.com/zitadel/zitadel/internal/query" "github.com/zitadel/zitadel/internal/static" + "github.com/zitadel/zitadel/internal/zerrors" ) type Login struct { @@ -336,6 +337,31 @@ func setUserContext(ctx context.Context, userID, resourceOwner string) context.C return authz.SetCtxData(ctx, data) } +// checkMFAPromptStep ensures the auth request has legitimately reached the MFA prompt +// (enrollment) step before any MFA registration/initialization is performed. +// MFA enrollment promotes authReq.UserID to the request principal via setUserContext, so it +// must only be reachable once the preceding factors have been checked. nextSteps only yields +// an *domain.MFAPromptStep after firstFactorChecked has passed, so an unauthenticated request +// that merely submitted a loginname (or otherwise skipped the first factor) will not carry it. +func (l *Login) checkMFAPromptStep(w http.ResponseWriter, r *http.Request, authReq *domain.AuthRequest) bool { + if !isMFAPromptStep(authReq) { + l.renderError(w, r, authReq, zerrors.ThrowPreconditionFailed(nil, "LOGIN-s9GW3M", "Errors.User.NotAllowed")) + return false + } + return true +} + +// isMFAPromptStep reports whether the auth request's current step is the MFA prompt +// (enrollment) step. nextSteps only yields an *domain.MFAPromptStep once firstFactorChecked +// has passed, so this is false for a request that merely submitted a loginname. +func isMFAPromptStep(authReq *domain.AuthRequest) bool { + if authReq == nil || len(authReq.PossibleSteps) == 0 { + return false + } + _, ok := authReq.PossibleSteps[0].(*domain.MFAPromptStep) + return ok +} + func (l *Login) baseURL(ctx context.Context) string { return http_utils.DomainContext(ctx).Origin() + HandlerPrefix } diff --git a/internal/api/ui/login/mfa_init_sms.go b/internal/api/ui/login/mfa_init_sms.go index 0326d26a3..bcacdba4c 100644 --- a/internal/api/ui/login/mfa_init_sms.go +++ b/internal/api/ui/login/mfa_init_sms.go @@ -79,6 +79,9 @@ func (l *Login) handleRegisterSMSCheck(w http.ResponseWriter, r *http.Request) { l.renderError(w, r, authReq, err) return } + if !l.checkMFAPromptStep(w, r, authReq) { + return + } ctx := setUserContext(r.Context(), authReq.UserID, authReq.UserOrgID) // save the current state diff --git a/internal/api/ui/login/mfa_init_test.go b/internal/api/ui/login/mfa_init_test.go new file mode 100644 index 000000000..06bb054c7 --- /dev/null +++ b/internal/api/ui/login/mfa_init_test.go @@ -0,0 +1,76 @@ +package login + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/zitadel/zitadel/internal/domain" +) + +// Test_isMFAPromptStep guards the fix for the unauthenticated MFA-enrollment / phone-overwrite +// issue: MFA init/enrollment handlers must only proceed when the auth request has legitimately +// reached the MFA prompt step (i.e. after the first factor was checked). A request that only +// submitted a loginname carries a first-factor step, not an *domain.MFAPromptStep. +func Test_isMFAPromptStep(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + authReq *domain.AuthRequest + want bool + }{ + { + name: "nil auth request", + authReq: nil, + want: false, + }, + { + name: "no possible steps", + authReq: &domain.AuthRequest{}, + want: false, + }, + { + name: "loginname only - first factor (password) step", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{&domain.PasswordStep{}}, + }, + want: false, + }, + { + name: "existing factor - MFA verification step, not enrollment", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{ + &domain.MFAVerificationStep{MFAProviders: []domain.MFAType{domain.MFATypeTOTP}}, + }, + }, + want: false, + }, + { + name: "legitimate enrollment - MFA prompt step", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{ + &domain.MFAPromptStep{Required: true, MFAProviders: []domain.MFAType{domain.MFATypeTOTP}}, + }, + }, + want: true, + }, + { + name: "MFA prompt step not first - only the current step counts", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{ + &domain.PasswordStep{}, + &domain.MFAPromptStep{Required: true, MFAProviders: []domain.MFAType{domain.MFATypeTOTP}}, + }, + }, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, isMFAPromptStep(tc.authReq)) + }) + } +} \ No newline at end of file diff --git a/internal/api/ui/login/mfa_init_u2f.go b/internal/api/ui/login/mfa_init_u2f.go index 0e75bd1b6..297092a41 100644 --- a/internal/api/ui/login/mfa_init_u2f.go +++ b/internal/api/ui/login/mfa_init_u2f.go @@ -44,6 +44,9 @@ func (l *Login) handleRegisterU2F(w http.ResponseWriter, r *http.Request) { l.renderError(w, r, authReq, err) return } + if !l.checkMFAPromptStep(w, r, authReq) { + return + } credData, err := base64.URLEncoding.DecodeString(data.CredentialData) if err != nil { l.renderRegisterU2F(w, r, authReq, err) diff --git a/internal/api/ui/login/mfa_init_verify_handler.go b/internal/api/ui/login/mfa_init_verify_handler.go index e3488d391..b10194e6a 100644 --- a/internal/api/ui/login/mfa_init_verify_handler.go +++ b/internal/api/ui/login/mfa_init_verify_handler.go @@ -31,6 +31,9 @@ func (l *Login) handleMFAInitVerify(w http.ResponseWriter, r *http.Request) { l.renderError(w, r, authReq, err) return } + if !l.checkMFAPromptStep(w, r, authReq) { + return + } var verifyData *mfaVerifyData switch data.MFAType { case domain.MFATypeTOTP: diff --git a/internal/api/ui/login/mfa_prompt_handler.go b/internal/api/ui/login/mfa_prompt_handler.go index ca318741b..eef22d487 100644 --- a/internal/api/ui/login/mfa_prompt_handler.go +++ b/internal/api/ui/login/mfa_prompt_handler.go @@ -23,6 +23,9 @@ func (l *Login) handleMFAPrompt(w http.ResponseWriter, r *http.Request) { l.renderError(w, r, authReq, err) return } + if !l.checkMFAPromptStep(w, r, authReq) { + return + } if !data.Skip { mfaVerifyData := new(mfaVerifyData) mfaVerifyData.MFAType = data.MFAProvider