From 35d8e41852c09ed98cc1793a987fa04cbea11b27 Mon Sep 17 00:00:00 2001 From: zokki Date: Fri, 7 Nov 2025 21:12:47 +0100 Subject: [PATCH] fix: endless redirection loop between /user/settings/change_password and /user/settings/security (#10002) Fixes forgejo/forgejo#9980 ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. ## Release notes - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/10002): endless redirection loop between /user/settings/change_password and /user/settings/security Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10002 Reviewed-by: Mathieu Fenniak Co-authored-by: zokki Co-committed-by: zokki (cherry picked from commit dc0a63efe25f80b80191e4550180765e4b1bd67b) --- .../TestTwoFactorWithPasswordChange/user.yml | 37 +++++++++++ routers/web/web.go | 8 ++- tests/integration/signin_test.go | 62 +++++++++++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 models/fixtures/TestTwoFactorWithPasswordChange/user.yml diff --git a/models/fixtures/TestTwoFactorWithPasswordChange/user.yml b/models/fixtures/TestTwoFactorWithPasswordChange/user.yml new file mode 100644 index 0000000000..72fb301e67 --- /dev/null +++ b/models/fixtures/TestTwoFactorWithPasswordChange/user.yml @@ -0,0 +1,37 @@ +- + id: 2001 + lower_name: user2001 + name: user2001 + full_name: "user2001" + email: user2001@example.com + keep_email_private: false + email_notifications_preference: onmention + passwd: ZogKvWdyEx:password + passwd_hash_algo: dummy + must_change_password: true + login_source: 0 + login_name: user2001 + type: 0 + salt: ZogKvWdyEx + max_repo_creation: -1 + is_active: true + is_admin: false + is_restricted: false + allow_git_hook: false + allow_import_local: false + allow_create_organization: true + prohibit_login: false + avatar: "" + avatar_email: user2001@example.com + use_custom_avatar: true + num_followers: 0 + num_following: 0 + num_stars: 0 + num_repos: 0 + num_teams: 0 + num_members: 0 + visibility: 0 + repo_admin_change_team_access: false + theme: "" + keep_activity_private: false + created_unix: 1759086716 diff --git a/routers/web/web.go b/routers/web/web.go index 20d5376cfe..162055ef0b 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -167,12 +167,14 @@ func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Cont return } } else if ctx.Req.URL.Path == "/user/settings/change_password" { + if ctx.Doer.MustHaveTwoFactor() { + ctx.Redirect(setting.AppSubURL + "/user/settings/security") + return + } // make sure that the form cannot be accessed by users who don't need this ctx.Redirect(setting.AppSubURL + "/") return - } - - if ctx.Doer.MustHaveTwoFactor() && !strings.HasPrefix(ctx.Req.URL.Path, "/user/settings/security") { + } else if ctx.Doer.MustHaveTwoFactor() && !strings.HasPrefix(ctx.Req.URL.Path, "/user/settings/security") { hasTwoFactor, err := auth_model.HasTwoFactorByUID(ctx, ctx.Doer.ID) if err != nil { log.Error("Error getting 2fa: %s", err) diff --git a/tests/integration/signin_test.go b/tests/integration/signin_test.go index 5e8dead5b7..77539bb754 100644 --- a/tests/integration/signin_test.go +++ b/tests/integration/signin_test.go @@ -17,6 +17,7 @@ import ( "forgejo.org/modules/setting" "forgejo.org/modules/test" "forgejo.org/modules/translation" + "forgejo.org/services/forms" "forgejo.org/tests" "github.com/stretchr/testify/assert" @@ -276,3 +277,64 @@ func TestGlobalTwoFactorRequirement(t *testing.T) { }) }) } + +func TestTwoFactorWithPasswordChange(t *testing.T) { + defer unittest.OverrideFixtures("models/fixtures/TestTwoFactorWithPasswordChange")() + + normalUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + changePasswordUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{MustChangePassword: true}) + + runTest := func(t *testing.T, user *user_model.User, requireTOTP bool) { + t.Helper() + defer unittest.AssertSuccessfulDelete(t, &auth.TwoFactor{UID: user.ID}) + + session := loginUser(t, user.Name) + + if user.MustChangePassword { + req := NewRequest(t, "GET", fmt.Sprintf("/%s", user.Name)) + resp := session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/user/settings/change_password", resp.Header().Get("Location")) + + req = NewRequest(t, "GET", "/user/settings/security") + resp = session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/user/settings/change_password", resp.Header().Get("Location")) + + req = NewRequestWithJSON(t, "POST", "/user/settings/change_password", forms.MustChangePasswordForm{ + Password: "password", + Retype: "password", + }) + resp = session.MakeRequest(t, req, http.StatusOK) + assert.Equal(t, "/user/settings/security", resp.Header().Get("Location")) + } + + if requireTOTP { + req := NewRequest(t, "GET", fmt.Sprintf("/%s", user.Name)) + resp := session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/user/settings/security", resp.Header().Get("Location")) + + req = NewRequest(t, "GET", "/user/settings/change_password") + resp = session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/user/settings/security", resp.Header().Get("Location")) + + session.EnrollTOTP(t) + } + + req := NewRequest(t, "GET", fmt.Sprintf("/%s", user.Name)) + session.MakeRequest(t, req, http.StatusOK) + } + + t.Run("Don't require TwoFactor", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + runTest(t, normalUser, false) + runTest(t, changePasswordUser, false) + }) + + t.Run("Require TwoFactor", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + defer test.MockVariableValue(&setting.GlobalTwoFactorRequirement, setting.AllTwoFactorRequirement)() + + runTest(t, normalUser, true) + runTest(t, changePasswordUser, true) + }) +}