From 18a4f883f00c33f89efc294dcefed052b039e50c Mon Sep 17 00:00:00 2001 From: gayathri <66356931+grvijayan@users.noreply.github.com> Date: Mon, 21 Sep 2026 13:37:27 +0200 Subject: [PATCH] Merge commit from fork * fix(login): gate username change on the change-username step handleChangeUsername renamed the account identified by authReq.UserID without checking that the login flow had reached the change-username step or that any factor had succeeded. Because authReq.UserID is bound as soon as a login name is submitted (checkLoginName -> SetUserInfo), an unauthenticated caller could start a flow, type a victim's login name and then POST a new username to rename that account, including instance admins. Gate the handler on the current step being *domain.ChangeUsernameStep, mirroring the other login handlers. That step is only produced in nextSteps after the first factor and any required MFA have succeeded, so the gate also proves the caller owns the account. The legitimate forced-username-change flow is unaffected: the UI only ever renders that form when the step is at index 0, and PossibleSteps is recomputed on every request. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Aup17GXd4tQnXG36xr5UiZ * test(login): add integration reproduction for GHSA-4hgj-wm6c-q7p2 Drive the unauthenticated-rename attack over raw HTTP against a running instance, mirroring the sibling GHSA-738m external-login integration test in the same package: authorize (V1) -> load login page (scrape CSRF) -> POST the victim's login name (binds authReq.UserID, no factor) -> POST a new username. The test first proves the exploit precondition held - step 3 advanced the flow to the victim's password step, so authReq.UserID was bound - then asserts over gRPC that the victim's username is unchanged. The precondition check guards against a false green: without it, a broken upstream hop would make the unchanged-username assertion pass vacuously even against unpatched code. The unchanged-username poll and the sibling test's requireNeverUserByEmail now share a small pollFor helper in the package. Red -> green: fails against the unpatched handler (the rename lands), passes against the fixed handler (the ChangeUsernameStep gate rejects the POST before the command runs). Runs in the existing nx test-integration job via the integration build tag. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Aup17GXd4tQnXG36xr5UiZ --------- Co-authored-by: Claude Opus 4.8 --- .../integration_test/external_login_test.go | 25 +- .../integration_test/username_change_test.go | 214 ++++++++++++++++++ .../api/ui/login/username_change_handler.go | 19 ++ .../ui/login/username_change_handler_test.go | 75 ++++++ 4 files changed, 326 insertions(+), 7 deletions(-) create mode 100644 internal/api/ui/login/integration_test/username_change_test.go create mode 100644 internal/api/ui/login/username_change_handler_test.go diff --git a/internal/api/ui/login/integration_test/external_login_test.go b/internal/api/ui/login/integration_test/external_login_test.go index ed763ce9f..778cc9486 100644 --- a/internal/api/ui/login/integration_test/external_login_test.go +++ b/internal/api/ui/login/integration_test/external_login_test.go @@ -245,11 +245,26 @@ func doPostForm(t *testing.T, c *http.Client, rawURL string, form url.Values) st return string(b) } -// requireNeverUserByEmail fails if a user with the given email appears within waitFor. -func requireNeverUserByEmail(t *testing.T, email string, waitFor, tick time.Duration) { +// pollFor runs check every tick until waitFor elapses (and once more at the deadline). check should +// require an invariant that must hold for the whole window; it fails the test on violation via +// require.*. It proves a negative - that a forbidden change never appears even under projection lag - +// without assert.Never, whose background goroutine can outlive the test and panic on CTX cancel. +func pollFor(t *testing.T, waitFor, tick time.Duration, check func()) { t.Helper() deadline := time.Now().Add(waitFor) for { + check() + if !time.Now().Before(deadline) { + return + } + time.Sleep(tick) + } +} + +// requireNeverUserByEmail fails if a user with the given email appears within waitFor. +func requireNeverUserByEmail(t *testing.T, email string, waitFor, tick time.Duration) { + t.Helper() + pollFor(t, waitFor, tick, func() { resp, err := Instance.Client.UserV2.ListUsers(CTX, &user.ListUsersRequest{ Queries: []*user.SearchQuery{ {Query: &user.SearchQuery_EmailQuery{EmailQuery: &user.EmailQuery{EmailAddress: email}}}, @@ -258,9 +273,5 @@ func requireNeverUserByEmail(t *testing.T, email string, waitFor, tick time.Dura require.NoError(t, err) require.Empty(t, resp.GetResult(), "a user was created for the attacker-forged external identity %q - account pre-hijack succeeded", email) - if !time.Now().Before(deadline) { - return - } - time.Sleep(tick) - } + }) } diff --git a/internal/api/ui/login/integration_test/username_change_test.go b/internal/api/ui/login/integration_test/username_change_test.go new file mode 100644 index 000000000..d1c2a0ce4 --- /dev/null +++ b/internal/api/ui/login/integration_test/username_change_test.go @@ -0,0 +1,214 @@ +//go:build integration + +// This file contains an end-to-end reproduction of GHSA-4hgj-wm6c-q7p2, an unauthenticated +// account-rename in the server-rendered Login V1 UI. +// +// The vulnerability: POST /ui/login/username/change (handleChangeUsername) renamed the account +// identified by authReq.UserID without checking that the login flow had reached the change-username +// step or that any factor had succeeded. authReq.UserID is bound as soon as a login name is +// submitted (POST /ui/login/loginname -> checkLoginName -> SetUserInfo), which requires no secret. +// An UNAUTHENTICATED attacker could therefore start a flow, submit a victim's login name to bind +// authReq.UserID to that victim, and then POST a new username to rename the victim - including +// instance admins - locking them out and freeing their login name for squatting. +// +// This test drives the exact attack over raw HTTP against a running Zitadel instance: +// +// 1. authorize (create a V1 auth request) -> 2. load login page (grab CSRF) +// -> 3. POST the victim's login name (binds authReq.UserID, no factor) +// -> 4. POST a new username (the rename) +// +// It first proves the exploit precondition actually held - step 3 advanced the flow to the victim's +// password step, i.e. authReq.UserID was bound with no factor - and then asserts, over gRPC, that +// the victim's username is UNCHANGED. Against the unpatched code the second assertion fails (the +// rename lands); against the fixed code it passes (the gate on ChangeUsernameStep rejects the +// request before the command runs). That is the red -> green signal for the fix. +// +// The outcome assertion is behavioral (the account's username) rather than on the rendered page: the +// fixed handler rejects by re-rendering the current step with a nil error, so there is no distinctive +// error code to match, and the true security property is simply that no rename occurred. The step-3 +// precondition check guards against a false green: if any upstream hop silently broke (no V1 id, +// wrong CSRF, rejected login name), the rename would never fire even against unpatched code and the +// unchanged-username assertion would pass vacuously. +// +// Requires a running backend + database. Run with: +// +// go test -tags integration ./internal/api/ui/login/integration_test/... +package login_test + +import ( + "net/http" + "net/http/cookiejar" + "net/url" + "testing" + "time" + + "github.com/muhlemmer/gu" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zitadel/oidc/v3/pkg/oidc" + + "github.com/zitadel/zitadel/internal/api/ui/login" + "github.com/zitadel/zitadel/internal/integration" + "github.com/zitadel/zitadel/pkg/grpc/app" + feature "github.com/zitadel/zitadel/pkg/grpc/feature/v2" + object "github.com/zitadel/zitadel/pkg/grpc/object/v2" + user "github.com/zitadel/zitadel/pkg/grpc/user/v2" +) + +// TestChangeUsername_UnauthenticatedRename_IsRejected reproduces the core exploit and asserts the +// fix rejects it. It is the end-to-end, behavioral red -> green test for GHSA-4hgj-wm6c-q7p2. +func TestChangeUsername_UnauthenticatedRename_IsRejected(t *testing.T) { + // --- Server setup (all via gRPC as the IAM owner) ------------------------------------------- + + // Force Login V1. Three things select V1 at auth-request creation (internal/api/oidc/auth_request.go): + // (a) the instance feature LoginV2.Required must be false, (b) the OIDC app's login version must be + // unspecified/V1, and (c) the authorize request must NOT carry the x-zitadel-login-client header. + // We pin (a) here explicitly so the test does not depend on the instance default. + integration.EnsureInstanceFeature(t, CTX, Instance, + &feature.SetInstanceFeaturesRequest{LoginV2: &feature.LoginV2{Required: false}}, + func(tt *assert.CollectT, got *feature.GetInstanceFeaturesResponse) { + assert.False(tt, got.GetLoginV2().GetRequired()) + }, + ) + + // A project + OIDC app. loginVersion is left nil (unspecified => V1). The redirect URI only has to + // be registered; the attack never actually redeems a code there. + redirectURI := "http://localhost:9999/callback" + project := Instance.CreateProject(CTX, t, Instance.DefaultOrg.GetId(), integration.RandString(10), false, false) + oidcApp, err := Instance.CreateOIDCClient(CTX, redirectURI, "", project.GetId(), + app.OIDCAppType_OIDC_APP_TYPE_WEB, app.OIDCAuthMethodType_OIDC_AUTH_METHOD_TYPE_NONE, true) + require.NoError(t, err) + clientID := oidcApp.GetClientId() + + // The victim: an active human with a password (so its first-factor step is the password page). We + // create it with a known username so we can both submit its login name and detect a rename. + victimUsername := "victim-" + integration.RandString(8) + victimID, victimLoginName := createVictim(t, victimUsername) + + // --- Attacker HTTP client ------------------------------------------------------------------- + + // One shared cookie jar for the WHOLE flow. This is mandatory: the server issues a user-agent + // cookie (zitadel.useragent) on the authorize response and binds the created auth request to that + // user-agent id; the loginname/username-change handlers later resolve the auth request by + // (authRequestID, userAgentID). A different jar (or no jar) would make it unresolvable. The csrf + // cookie also lives here. + jar, err := cookiejar.New(nil) + require.NoError(t, err) + httpClient := &http.Client{ + Jar: jar, + // Never auto-follow redirects: we want each hop's response as-is. + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + } + issuer := Instance.OIDCIssuer() // http://: + + // Step 1: create a Login V1 auth request by hitting the OIDC authorize endpoint WITHOUT the + // login-client header. The response is a 302 to /ui/login/login?authRequestID=; we extract id. + authURL := issuer + "/oauth/v2/authorize?" + url.Values{ + "client_id": {clientID}, + "redirect_uri": {redirectURI}, + "response_type": {"code"}, + "scope": {"openid"}, + "state": {"attacker-state"}, + "code_challenge": {oidc.NewSHACodeChallenge(integration.CodeVerifier)}, + "code_challenge_method": {"S256"}, + }.Encode() + loc := getLocation(t, httpClient, authURL) + authRequestID := loc.Query().Get(login.QueryAuthRequestID) + require.NotEmpty(t, authRequestID, "expected a V1 auth request id in the authorize redirect %q", loc.String()) + + // Step 2: load the login page. This renders the login form and lets us scrape the CSRF token, + // valid for any subsequent POST in this session (it is bound to the csrf cookie in the jar). + loginPageURL := issuer + login.HandlerPrefix + login.EndpointLogin + "?" + login.QueryAuthRequestID + "=" + authRequestID + csrfToken := scrapeCSRF(t, httpClient, loginPageURL) + + // Step 3: the binding. POST the victim's login name. handleLoginNameCheck -> checkLoginName -> + // SetUserInfo binds authReq.UserID to the victim. NO factor is required - this is the whole point: + // an unauthenticated caller can point the auth request at any account by name alone. + loginNameForm := url.Values{ + login.QueryAuthRequestID: {authRequestID}, + "gorilla.csrf.Token": {csrfToken}, + "loginName": {victimLoginName}, + } + loginNameURL := issuer + login.HandlerPrefix + login.EndpointLoginName + loginNameBody := doPostForm(t, httpClient, loginNameURL, loginNameForm) + + // Prove the precondition held: the login name resolved and bound authReq.UserID to the victim, + // advancing the flow to the password (first-factor) step. renderNextStep renders that step inline + // (200), so the response body is the password page: it carries a name="password" input and echoes + // the bound account's login name. Both markers are locale-independent. Without this check a broken + // upstream hop would make the later unchanged-username assertion pass vacuously - a false green even + // against unpatched code. + require.Contains(t, loginNameBody, `name="password"`, + "login-name POST did not advance to the password step; the victim was not bound (a later green would be vacuous)") + require.Contains(t, loginNameBody, victimUsername, + "password step is not bound to the victim; login-name binding did not take effect") + + // Step 4: the attack. POST a new username. Before the fix this renamed the bound victim; the fix + // gates the handler on the current step being *domain.ChangeUsernameStep (only produced after the + // first factor and any required MFA), so this request is rejected. + newUsername := "pwned-" + integration.RandString(8) + changeForm := url.Values{ + login.QueryAuthRequestID: {authRequestID}, + "gorilla.csrf.Token": {csrfToken}, + "username": {newUsername}, + } + changeURL := issuer + login.HandlerPrefix + login.EndpointChangeUsername + doPostForm(t, httpClient, changeURL, changeForm) + + // The core security property: the victim's username must remain unchanged. Poll for a window so a + // vulnerable rename that projects late still fails the test (an immediate single read could race a + // slow projection and give a false green against unpatched code). + requireUsernameUnchanged(t, victimID, victimUsername, 5*time.Second, 250*time.Millisecond) +} + +// --- helpers specific to the username-change reproduction -------------------------------------- +// +// doGet, getLocation, scrapeCSRF, doPostForm and the CTX/Instance globals are defined in +// external_login_test.go, which shares this package and build tag. + +// createVictim creates an active human user with the given username, a verified email and a password +// (so its login flow's first factor is the password step) and returns its id and the preferred login +// name to submit at /ui/login/loginname. +func createVictim(t *testing.T, username string) (userID, loginName string) { + t.Helper() + resp, err := Instance.Client.UserV2.AddHumanUser(CTX, &user.AddHumanUserRequest{ + Organization: &object.Organization{ + Org: &object.Organization_OrgId{OrgId: Instance.DefaultOrg.GetId()}, + }, + Username: gu.Ptr(username), + Profile: &user.SetHumanProfile{ + GivenName: "Vic", + FamilyName: "Tim", + PreferredLanguage: gu.Ptr("en"), + }, + Email: &user.SetHumanEmail{ + Email: username + "@victim.test", + Verification: &user.SetHumanEmail_IsVerified{IsVerified: true}, + }, + PasswordType: &user.AddHumanUserRequest_Password{ + Password: &user.Password{Password: "Password1!", ChangeRequired: false}, + }, + }) + require.NoError(t, err) + Instance.TriggerUserByID(CTX, resp.GetUserId()) + + got, err := Instance.Client.UserV2.GetUserByID(CTX, &user.GetUserByIDRequest{UserId: resp.GetUserId()}) + require.NoError(t, err) + loginName = got.GetUser().GetPreferredLoginName() + require.NotEmpty(t, loginName) + require.Equal(t, username, got.GetUser().GetUsername()) + return resp.GetUserId(), loginName +} + +// requireUsernameUnchanged fails if the user's username changes away from want within waitFor. +func requireUsernameUnchanged(t *testing.T, userID, want string, waitFor, tick time.Duration) { + t.Helper() + pollFor(t, waitFor, tick, func() { + resp, err := Instance.Client.UserV2.GetUserByID(CTX, &user.GetUserByIDRequest{UserId: userID}) + require.NoError(t, err) + require.Equal(t, want, resp.GetUser().GetUsername(), + "victim username changed to %q - unauthenticated rename succeeded", resp.GetUser().GetUsername()) + }) +} diff --git a/internal/api/ui/login/username_change_handler.go b/internal/api/ui/login/username_change_handler.go index b932079dd..0b06c3954 100644 --- a/internal/api/ui/login/username_change_handler.go +++ b/internal/api/ui/login/username_change_handler.go @@ -28,6 +28,12 @@ func (l *Login) handleChangeUsername(w http.ResponseWriter, r *http.Request) { l.renderError(w, r, authReq, err) return } + // The rename may only proceed once the flow has reached the change-username step; + // isChangeUsernameStep documents why that also proves the caller is authenticated. + if !isChangeUsernameStep(authReq) { + l.renderError(w, r, authReq, nil) + return + } _, err = l.command.ChangeUsername(setContext(r.Context(), authReq.UserOrgID), authReq.UserOrgID, authReq.UserID, data.Username) if err != nil { l.renderChangeUsername(w, r, authReq, err) @@ -41,3 +47,16 @@ func (l *Login) renderChangeUsernameDone(w http.ResponseWriter, r *http.Request, data := l.getUserData(r, authReq, translator, "UsernameChangeDone.Title", "UsernameChangeDone.Description", nil) l.renderer.RenderTemplate(w, r, translator, l.renderer.Templates[tmplChangeUsernameDone], data, nil) } + +// isChangeUsernameStep reports whether the login flow's current step is the change-username +// step. ChangeUsernameStep is appended in nextSteps only after the first factor and any +// required MFA have succeeded, so gating on it guarantees the caller is the authenticated +// owner of authReq.UserID. Without this gate an unauthenticated caller could bind +// authReq.UserID to any account by submitting its login name and then rename it. +func isChangeUsernameStep(authReq *domain.AuthRequest) bool { + if authReq == nil || len(authReq.PossibleSteps) == 0 { + return false + } + _, ok := authReq.PossibleSteps[0].(*domain.ChangeUsernameStep) + return ok +} diff --git a/internal/api/ui/login/username_change_handler_test.go b/internal/api/ui/login/username_change_handler_test.go new file mode 100644 index 000000000..866ea9832 --- /dev/null +++ b/internal/api/ui/login/username_change_handler_test.go @@ -0,0 +1,75 @@ +package login + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/zitadel/zitadel/internal/domain" +) + +// Test_isChangeUsernameStep guards the fix for the unauthenticated account-rename issue: +// handleChangeUsername must only proceed when the auth request has legitimately reached the +// change-username step (which nextSteps appends only after the first factor and any required MFA have +// succeeded). A request that merely submitted a loginname carries a first-factor step, so an +// unauthenticated caller must not be able to rename the bound account. +func Test_isChangeUsernameStep(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: "not yet authenticated - MFA verification step", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{ + &domain.MFAVerificationStep{MFAProviders: []domain.MFAType{domain.MFATypeTOTP}}, + }, + }, + want: false, + }, + { + name: "legitimate change-username step", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{&domain.ChangeUsernameStep{}}, + }, + want: true, + }, + { + name: "change-username step not first - only the current step counts", + authReq: &domain.AuthRequest{ + PossibleSteps: []domain.NextStep{ + &domain.ChangePasswordStep{}, + &domain.ChangeUsernameStep{}, + }, + }, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, isChangeUsernameStep(tc.authReq)) + }) + } +}