From 79e5e20beaece5c3dee0a5bb112a99899b561c66 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 30 Aug 2025 18:53:14 +0200 Subject: [PATCH] [v11.0/forgejo] fix: don't allow credentials in migrate/push mirror URL (#9065) **Backport: https://codeberg.org/forgejo/forgejo/pulls/9064** It is no longer possible to specify the user and password when providing a URL for migrating a repository, the fields dedicated to that purpose on the form must be used instead. This is to prevent that those credentials are displayed in the repository settings that are visible by the repository admins, in the case where the migration is a mirror. Co-authored-by: Gergely Nagy Co-authored-by: Gusted Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9065 Reviewed-by: Gusted Reviewed-by: 0ko <0ko@noreply.codeberg.org> --- models/error.go | 4 ++ options/locale_next/locale_en-US.json | 1 + routers/api/v1/repo/migrate.go | 2 + routers/api/v1/repo/mirror.go | 2 + routers/web/repo/migrate.go | 2 + routers/web/repo/setting/setting.go | 10 ++++- services/forms/repo_form.go | 3 ++ .../mirror.yml | 9 ++++ .../repository.yml | 34 ++++++++++++++ tests/integration/mirror_pull_test.go | 18 ++++++++ tests/integration/mirror_push_test.go | 43 ++++++++++++++++++ tests/integration/repo_migrate_test.go | 44 +++++++++++++++++++ 12 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 tests/integration/fixtures/TestPullMirrorRedactCredentials/mirror.yml create mode 100644 tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml diff --git a/models/error.go b/models/error.go index e8962f386b..be26a6abe2 100644 --- a/models/error.go +++ b/models/error.go @@ -121,6 +121,7 @@ type ErrInvalidCloneAddr struct { IsInvalidPath bool IsProtocolInvalid bool IsPermissionDenied bool + HasCredentials bool LocalPath bool } @@ -143,6 +144,9 @@ func (err *ErrInvalidCloneAddr) Error() string { if err.IsURLError { return fmt.Sprintf("migration/cloning from '%s' is not allowed: the provided url is invalid", err.Host) } + if err.HasCredentials { + return fmt.Sprintf("migration/cloning from '%s' is not allowed: the provided url contains credentials", err.Host) + } return fmt.Sprintf("migration/cloning from '%s' is not allowed", err.Host) } diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 4f3c65a68f..0fd3632f39 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -12,6 +12,7 @@ "one": "wants to merge %[1]d commit from %[2]s into %[3]s", "other": "wants to merge %[1]d commits from %[2]s into %[3]s" }, + "migrate.form.error.url_credentials": "The URL contains contains credentials, put them in the username and password fields respectively", "search.milestone_kind": "Search milestones…", "incorrect_root_url": "This Forgejo instance is configured to be served on \"%s\". You are currently viewing Forgejo through a different URL, which may cause parts of the application to break. The canonical URL is controlled by Forgejo admins via the ROOT_URL setting in the app.ini.", "themes.names.forgejo-auto": "Forgejo (follow system theme)", diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go index 75e772dfe2..5a32d5dec8 100644 --- a/routers/api/v1/repo/migrate.go +++ b/routers/api/v1/repo/migrate.go @@ -283,6 +283,8 @@ func handleRemoteAddrError(ctx *context.APIContext, err error) { } case addrErr.IsInvalidPath: ctx.Error(http.StatusUnprocessableEntity, "", "Invalid local path, it does not exist or not a directory.") + case addrErr.HasCredentials: + ctx.Error(http.StatusUnprocessableEntity, "", "The URL contains credentials.") default: ctx.Error(http.StatusInternalServerError, "ParseRemoteAddr", "Unknown error type (ErrInvalidCloneAddr): "+err.Error()) } diff --git a/routers/api/v1/repo/mirror.go b/routers/api/v1/repo/mirror.go index bc48c6acb7..8412b4a95b 100644 --- a/routers/api/v1/repo/mirror.go +++ b/routers/api/v1/repo/mirror.go @@ -441,6 +441,8 @@ func HandleRemoteAddressError(ctx *context.APIContext, err error) { ctx.Error(http.StatusBadRequest, "CreatePushMirror", "Invalid Url ") case addrErr.IsPermissionDenied: ctx.Error(http.StatusUnauthorized, "CreatePushMirror", "Permission denied") + case addrErr.HasCredentials: + ctx.Error(http.StatusBadRequest, "CreatePushMirror", "The URL contains credentials") default: ctx.Error(http.StatusBadRequest, "CreatePushMirror", "Unknown error") } diff --git a/routers/web/repo/migrate.go b/routers/web/repo/migrate.go index 86d2461e94..3a5cf30dbe 100644 --- a/routers/web/repo/migrate.go +++ b/routers/web/repo/migrate.go @@ -138,6 +138,8 @@ func handleMigrateRemoteAddrError(ctx *context.Context, err error, tpl base.TplN } case addrErr.IsInvalidPath: ctx.RenderWithErr(ctx.Tr("repo.migrate.invalid_local_path"), tpl, form) + case addrErr.HasCredentials: + ctx.RenderWithErr(ctx.Tr("migrate.form.error.url_credentials"), tpl, form) default: log.Error("Error whilst updating url: %v", err) ctx.RenderWithErr(ctx.Tr("form.url_error", "unknown"), tpl, form) diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index d35f4bdd8a..b046e6b847 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -539,7 +539,13 @@ func SettingsPost(ctx *context.Context) { mirror_service.AddPullMirrorToQueue(repo.ID) - ctx.Flash.Info(ctx.Tr("repo.settings.pull_mirror_sync_in_progress", repo.OriginalURL)) + sanitizedOriginalURL, err := util.SanitizeURL(repo.OriginalURL) + if err != nil { + ctx.ServerError("SanitizeURL", err) + return + } + + ctx.Flash.Info(ctx.Tr("repo.settings.pull_mirror_sync_in_progress", sanitizedOriginalURL)) ctx.Redirect(repo.Link() + "/settings") case "push-mirror-sync": @@ -1086,6 +1092,8 @@ func handleSettingRemoteAddrError(ctx *context.Context, err error, form *forms.R } case addrErr.IsInvalidPath: ctx.RenderWithErr(ctx.Tr("repo.migrate.invalid_local_path"), tplSettingsOptions, form) + case addrErr.HasCredentials: + ctx.RenderWithErr(ctx.Tr("migrate.form.error.url_credentials"), tplSettingsOptions, form) default: ctx.ServerError("Unknown error", err) } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index c39c6a7b36..e62a872252 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -105,6 +105,9 @@ func ParseRemoteAddr(remoteAddr, authUsername, authPassword string) (string, err if err != nil { return "", &models.ErrInvalidCloneAddr{IsURLError: true, Host: remoteAddr} } + if u.User != nil { + return "", &models.ErrInvalidCloneAddr{Host: remoteAddr, HasCredentials: true} + } if len(authUsername)+len(authPassword) > 0 { u.User = url.UserPassword(authUsername, authPassword) } diff --git a/tests/integration/fixtures/TestPullMirrorRedactCredentials/mirror.yml b/tests/integration/fixtures/TestPullMirrorRedactCredentials/mirror.yml new file mode 100644 index 0000000000..d5010d433f --- /dev/null +++ b/tests/integration/fixtures/TestPullMirrorRedactCredentials/mirror.yml @@ -0,0 +1,9 @@ +- + id: 1001 + repo_id: 1 + interval: 3600 + enable_prune: false + updated_unix: 0 + next_update_unix: 0 + lfs_enabled: false + lfs_endpoint: "" diff --git a/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml b/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml new file mode 100644 index 0000000000..ea76a569ea --- /dev/null +++ b/tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml @@ -0,0 +1,34 @@ +- + id: 1001 + owner_id: 2 + owner_name: user2 + lower_name: repo1001 + name: repo1001 + default_branch: master + original_url: https://:TOKEN@example.com/example/example.git + num_watches: 0 + num_stars: 0 + num_forks: 0 + num_issues: 0 + num_closed_issues: 0 + num_pulls: 0 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_projects: 0 + num_closed_projects: 0 + is_private: false + is_empty: false + is_archived: false + is_mirror: true + status: 0 + is_fork: false + fork_id: 0 + is_template: false + template_id: 0 + size: 0 + is_fsck_enabled: true + close_issues_via_commit_in_any_branch: false + created_unix: 1751320800 + updated_unix: 1751320800 + topics: '[]' diff --git a/tests/integration/mirror_pull_test.go b/tests/integration/mirror_pull_test.go index c9c8037e27..7e1cb36181 100644 --- a/tests/integration/mirror_pull_test.go +++ b/tests/integration/mirror_pull_test.go @@ -1,9 +1,11 @@ // Copyright 2019 The Gitea Authors. All rights reserved. +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package integration import ( + "net/http" "testing" "forgejo.org/models/db" @@ -13,6 +15,7 @@ import ( "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" "forgejo.org/modules/migration" + forgejo_context "forgejo.org/services/context" mirror_service "forgejo.org/services/mirror" release_service "forgejo.org/services/release" repo_service "forgejo.org/services/repository" @@ -101,3 +104,18 @@ func TestMirrorPull(t *testing.T) { require.NoError(t, err) assert.EqualValues(t, initCount, count) } + +func TestPullMirrorRedactCredentials(t *testing.T) { + defer unittest.OverrideFixtures("tests/integration/fixtures/TestPullMirrorRedactCredentials")() + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user2") + session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1001/settings", map[string]string{ + "_csrf": GetCSRF(t, session, "/user2/repo1001/settings"), + "action": "mirror-sync", + }), http.StatusSeeOther) + + flashCookie := session.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "info%3DPulling%2Bchanges%2Bfrom%2Bthe%2Bremote%2Bhttps%253A%252F%252Fexample.com%252Fexample%252Fexample.git%2Bat%2Bthe%2Bmoment.", flashCookie.Value) +} diff --git a/tests/integration/mirror_push_test.go b/tests/integration/mirror_push_test.go index ca8d536e46..97be462213 100644 --- a/tests/integration/mirror_push_test.go +++ b/tests/integration/mirror_push_test.go @@ -17,6 +17,7 @@ import ( "time" asymkey_model "forgejo.org/models/asymkey" + auth_model "forgejo.org/models/auth" "forgejo.org/models/db" repo_model "forgejo.org/models/repo" "forgejo.org/models/unit" @@ -26,7 +27,9 @@ import ( "forgejo.org/modules/gitrepo" "forgejo.org/modules/optional" "forgejo.org/modules/setting" + api "forgejo.org/modules/structs" "forgejo.org/modules/test" + "forgejo.org/modules/translation" gitea_context "forgejo.org/services/context" doctor "forgejo.org/services/doctor" "forgejo.org/services/migrations" @@ -38,6 +41,46 @@ import ( "github.com/stretchr/testify/require" ) +func TestPushMirrorRedactCredential(t *testing.T) { + defer test.MockVariableValue(&setting.Mirror.Enabled, true)() + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user2") + cloneAddr := "https://:TOKEN@example.com/example/example.git" + + t.Run("Web route", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/settings", map[string]string{ + "_csrf": GetCSRF(t, session, "/user2/repo1/settings"), + "action": "push-mirror-add", + "push_mirror_address": cloneAddr, + "push_mirror_interval": "0", + }), http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + assert.Contains(t, + htmlDoc.doc.Find(".ui.negative.message").Text(), + translation.NewLocale("en-US").Tr("migrate.form.error.url_credentials"), + ) + }) + + t.Run("API route", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + resp := MakeRequest(t, NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/push_mirrors", &api.CreatePushMirrorOption{ + RemoteAddress: cloneAddr, + Interval: "0", + }).AddTokenAuth(token), http.StatusBadRequest) + + var respBody map[string]any + DecodeJSON(t, resp, &respBody) + + assert.Equal(t, "The URL contains credentials", respBody["message"]) + }) +} + func TestMirrorPush(t *testing.T) { onGiteaRun(t, testMirrorPush) } diff --git a/tests/integration/repo_migrate_test.go b/tests/integration/repo_migrate_test.go index 233a55ef8f..90c6779bb9 100644 --- a/tests/integration/repo_migrate_test.go +++ b/tests/integration/repo_migrate_test.go @@ -1,4 +1,5 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package integration @@ -9,7 +10,9 @@ import ( "net/http/httptest" "testing" + auth_model "forgejo.org/models/auth" "forgejo.org/modules/structs" + "forgejo.org/modules/translation" "forgejo.org/tests" "github.com/stretchr/testify/assert" @@ -55,3 +58,44 @@ func TestRepoMigrate(t *testing.T) { }) } } + +func TestRepoMigrateCredentials(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user2") + cloneAddr := "https://:TOKEN@example.com/example/example.git" + + t.Run("Web route", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + resp := session.MakeRequest(t, NewRequestWithValues(t, "POST", "/repo/migrate?service_type=1", map[string]string{ + "_csrf": GetCSRF(t, session, "/repo/migrate?service_type=1"), + "clone_addr": cloneAddr, + "uid": "2", + "repo_name": "example", + "service": "1", + }), http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + assert.Contains(t, + htmlDoc.doc.Find(".ui.negative.message").Text(), + translation.NewLocale("en-US").Tr("migrate.form.error.url_credentials"), + ) + }) + + t.Run("API route", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + resp := MakeRequest(t, NewRequestWithJSON(t, "POST", "/api/v1/repos/migrate", &structs.MigrateRepoOptions{ + CloneAddr: cloneAddr, + RepoOwnerID: 2, + RepoName: "example", + }).AddTokenAuth(token), http.StatusUnprocessableEntity) + + var respBody map[string]any + DecodeJSON(t, resp, &respBody) + + assert.Equal(t, "The URL contains credentials.", respBody["message"]) + }) +}