mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
* 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 <[email protected]> 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01Aup17GXd4tQnXG36xr5UiZ --------- Co-authored-by: Claude Opus 4.8 <[email protected]>
76 lines
1.9 KiB
Go
76 lines
1.9 KiB
Go
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))
|
|
})
|
|
}
|
|
}
|