From 43664f79b93fa7926c2d93536ee2522a40fb5b0c Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Sat, 30 Aug 2025 18:42:39 +0200 Subject: [PATCH] [v12.0/forgejo] fix: don't allow credentials in migrate/push mirror URL (#9078) **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. ## Release notes - Security bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/9064): don't allow credentials in migrate/push mirror URL Co-authored-by: Gergely Nagy Co-authored-by: Gusted Co-authored-by: Earl Warren Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9078 Reviewed-by: 0ko <0ko@noreply.codeberg.org> Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- 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 ebaa8a135d..99c8ded766 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 3262b49f6c..c92d9e9521 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -54,6 +54,7 @@ "other": "wants to merge %[1]d commits from %[2]s into %[3]s" }, "repo.form.cannot_create": "All spaces in which you can create repositories have reached the limit of repositories.", + "migrate.form.error.url_credentials": "The URL contains contains credentials, put them in the username and password fields respectively", "repo.issue_indexer.title": "Issue Indexer", "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.", diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go index a848a950db..e58545c2f6 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 6f35e19880..518ddc1b77 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -542,7 +542,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": @@ -1091,6 +1097,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 bb81e939b0..3e9ef171a2 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 c98af1b773..666ac3d3fd 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.Equal(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 1377d1eb1b..c4f8b76501 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"]) + }) +}