From 145dea59bb3f41f7044d52d379482f829b56ef30 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 28 May 2024 17:38:44 +0200 Subject: [PATCH 1/4] fix: sanitize OriginalURL before displaying it While `repo.OriginalURL` is supposed to be sanitized, with username and passwords removed, it appears that is not always the case, and sometimes we may encounter original URLs that aren't sanitized. While that is possibly a historical artifact, we should still treat it with care. As such, before displaying `repo.OriginalURL` as an info flash when syncing a pull repository, sanitize the URL first, to be on the safe side. Signed-off-by: Gergely Nagy --- routers/web/repo/setting/setting.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 595fdace83..59e34baf1b 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -543,7 +543,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": From d00200dc3eab9156125a2399467396219af34c54 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 21 Aug 2025 03:55:29 +0200 Subject: [PATCH 2/4] chore: add integration test Demonstrate that the credential isn't shown in the flash message --- .../mirror.yml | 9 +++++ .../repository.yml | 34 +++++++++++++++++++ tests/integration/mirror_pull_test.go | 18 ++++++++++ 3 files changed, 61 insertions(+) create mode 100644 tests/integration/fixtures/TestPullMirrorRedactCredentials/mirror.yml create mode 100644 tests/integration/fixtures/TestPullMirrorRedactCredentials/repository.yml 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) +} From 9f955b300b11c64f79ef6a77c92fd6e1226eeb5a Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 21 Aug 2025 02:07:50 +0200 Subject: [PATCH 3/4] fix: don't allow credentials in migrate/push mirror URL Do not allow credentials to be present in the URLs that are provided for migrations and push mirrors. They have to be given via the dedicated input fields. Give a error when this happens. There's nothing wrong with trying have the backend "correct" this, but would be a larger patch than necessary in the context of a security fix. This can be done in public. --- 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 | 2 ++ services/forms/repo_form.go | 3 +++ 7 files changed, 16 insertions(+) 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 c897ad7ff2..9d39f40558 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…", "repo.settings.push_mirror.branch_filter.label": "Branch filter (optional)", 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 08ef68cbfc..fa2d5abb11 100644 --- a/routers/api/v1/repo/mirror.go +++ b/routers/api/v1/repo/mirror.go @@ -442,6 +442,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 59e34baf1b..ec18fa55a9 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -1116,6 +1116,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 d040b41395..11aac1fd52 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) } From 374a29fd358077c94b7963394d62cb42141f634c Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 21 Aug 2025 04:05:44 +0200 Subject: [PATCH 4/4] chore: add integration test Demonstrate that the it's not possible to migrate or add a push mirror from a URL that contains credentials. --- tests/integration/mirror_push_test.go | 41 ++++++++++++++++++++++++ tests/integration/repo_migrate_test.go | 44 ++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/tests/integration/mirror_push_test.go b/tests/integration/mirror_push_test.go index 73b275ed2d..23bb550d9f 100644 --- a/tests/integration/mirror_push_test.go +++ b/tests/integration/mirror_push_test.go @@ -30,6 +30,7 @@ import ( "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" @@ -42,6 +43,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"]) + }) +}