From f7f7d086e4950fc6bd80450cd64623588de07dd3 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 24 Aug 2025 02:30:23 +0200 Subject: [PATCH 1/5] fix: use credentials helpers for git clones - When cloning with credentials is used, don't set the credentials in the URL and pass that to Git, instead use Git credential helper to pass the credential. This avoids the credentials to be leaked through the process list. --- modules/git/repo.go | 88 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 77 insertions(+), 11 deletions(-) diff --git a/modules/git/repo.go b/modules/git/repo.go index 23e9337615..4f2b05bca5 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -18,6 +18,7 @@ import ( "strings" "time" + "forgejo.org/modules/log" "forgejo.org/modules/proxy" "forgejo.org/modules/setting" "forgejo.org/modules/util" @@ -160,24 +161,89 @@ func CloneWithArgs(ctx context.Context, args TrustedCmdArgs, from, to string, op if len(opts.Branch) > 0 { cmd.AddArguments("-b").AddDynamicArguments(opts.Branch) } - cmd.AddDashesAndList(from, to) - if strings.Contains(from, "://") && strings.Contains(from, "@") { - cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, util.SanitizeCredentialURLs(from), to, opts.Shared, opts.Mirror, opts.Depth)) - } else { - cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, from, to, opts.Shared, opts.Mirror, opts.Depth)) + envs := os.Environ() + parsedFromURL, err := url.Parse(from) + if err == nil { + envs = proxy.EnvWithProxy(parsedFromURL) } + fromURL := from + sanitizedFrom := from + + // If the clone URL has credentials, sanitize it and store the credentials in + // a temporary file that git will access. + if strings.Contains(from, "://") && strings.Contains(from, "@") { + sanitizedFrom = util.SanitizeCredentialURLs(from) + if parsedFromURL != nil { + if pwd, has := parsedFromURL.User.Password(); has { + parsedFromURL.User = url.User(parsedFromURL.User.Username()) + fromURL = parsedFromURL.String() + + credentialsFile, err := os.CreateTemp(os.TempDir(), "forgejo-clone-credentials") + if err != nil { + return err + } + credentialsPath := credentialsFile.Name() + + defer func() { + _ = credentialsFile.Close() + if err := util.Remove(credentialsPath); err != nil { + log.Warn("Unable to remove temporary file %q: %v", credentialsPath, err) + } + }() + + // Make it read-write. + if err := credentialsFile.Chmod(0o600); err != nil { + return err + } + + // Write the password. + if _, err := fmt.Fprint(credentialsFile, pwd); err != nil { + return err + } + + askpassFile, err := os.CreateTemp(os.TempDir(), "forgejo-askpass") + if err != nil { + return err + } + askpassPath := askpassFile.Name() + + defer func() { + _ = askpassFile.Close() + if err := util.Remove(askpassPath); err != nil { + log.Warn("Unable to remove temporary file %q: %v", askpassPath, err) + } + }() + + // Make it executable. + if err := askpassFile.Chmod(0o700); err != nil { + return err + } + + // Write the password script. + if _, err := fmt.Fprintf(askpassFile, "exec cat %s", credentialsPath); err != nil { + return err + } + + // Close it, so that Git can use it and no busy errors arise. + _ = askpassFile.Close() + _ = credentialsFile.Close() + + // Use environments to specify that git should ask for credentials, this + // takes precedences over anything else https://git-scm.com/docs/gitcredentials#_requesting_credentials. + envs = append(envs, "GIT_ASKPASS="+askpassPath) + } + } + } + + cmd.SetDescription(fmt.Sprintf("clone branch %s from %s to %s (shared: %t, mirror: %t, depth: %d)", opts.Branch, sanitizedFrom, to, opts.Shared, opts.Mirror, opts.Depth)) + cmd.AddDashesAndList(fromURL, to) + if opts.Timeout <= 0 { opts.Timeout = -1 } - envs := os.Environ() - u, err := url.Parse(from) - if err == nil { - envs = proxy.EnvWithProxy(u) - } - stderr := new(bytes.Buffer) if err = cmd.Run(&RunOpts{ Timeout: opts.Timeout, From 900ea0ce5a447ab6ba953927237495ae80f5ea01 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 24 Aug 2025 02:30:36 +0200 Subject: [PATCH 2/5] chore: add clone credential unit test Verify it still works and the askpass file implementation is employed by Git. --- modules/git/repo_test.go | 85 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index c3971b2a5f..b07fc3732d 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -4,7 +4,15 @@ package git import ( + "bytes" + "encoding/base64" + "io/fs" + "net/http" + "net/http/httptest" + "net/url" + "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -54,3 +62,80 @@ func TestRepoGetDivergingCommits(t *testing.T) { Behind: 2, }, do) } + +func TestCloneCredentials(t *testing.T) { + calledWithoutPassword := false + askpassFile := "" + credentialsFile := "" + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.URL.Path != "/info/refs" { + return + } + + // Get basic authorization. + auth, ok := strings.CutPrefix(req.Header.Get("Authorization"), "Basic ") + if !ok { + w.Header().Set("WWW-Authenticate", `Basic realm="Forgejo"`) + http.Error(w, "require credentials", http.StatusUnauthorized) + return + } + + rawAuth, err := base64.StdEncoding.DecodeString(auth) + require.NoError(t, err) + + user, password, ok := bytes.Cut(rawAuth, []byte{':'}) + assert.True(t, ok) + + // First time around Git tries without password (that's specified in the clone URL). + // It demonstrates it doesn't immediately uses askpass. + if len(password) == 0 { + assert.EqualValues(t, "oauth2", user) + calledWithoutPassword = true + + w.Header().Set("WWW-Authenticate", `Basic realm="Forgejo"`) + http.Error(w, "require credentials", http.StatusUnauthorized) + return + } + + assert.EqualValues(t, "oauth2", user) + assert.EqualValues(t, "some_token", password) + + tmpDir := os.TempDir() + + // Verify that the askpass implementation was used. + files, err := fs.Glob(os.DirFS(tmpDir), "forgejo-askpass*") + require.NoError(t, err) + for _, fileName := range files { + fileContent, err := os.ReadFile(filepath.Join(tmpDir, fileName)) + require.NoError(t, err) + + credentialsPath, ok := bytes.CutPrefix(fileContent, []byte(`exec cat `)) + assert.True(t, ok) + + fileContent, err = os.ReadFile(string(credentialsPath)) + require.NoError(t, err) + assert.EqualValues(t, "some_token", fileContent) + + askpassFile = filepath.Join(tmpDir, fileName) + credentialsFile = string(credentialsPath) + } + })) + + serverURL, err := url.Parse(server.URL) + require.NoError(t, err) + + serverURL.User = url.UserPassword("oauth2", "some_token") + + require.NoError(t, Clone(t.Context(), serverURL.String(), t.TempDir(), CloneRepoOptions{})) + + assert.True(t, calledWithoutPassword) + assert.NotEmpty(t, askpassFile) + assert.NotEmpty(t, credentialsFile) + + // Check that the helper files are gone. + _, err = os.Stat(askpassFile) + require.ErrorIs(t, err, fs.ErrNotExist) + _, err = os.Stat(credentialsFile) + require.ErrorIs(t, err, fs.ErrNotExist) +} From 1c66c4e11a0d7d706052d6abc04ed9accb9608a0 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 24 Aug 2025 02:31:39 +0200 Subject: [PATCH 3/5] chore: add extra shell escape tests --- modules/util/shellquote_test.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/modules/util/shellquote_test.go b/modules/util/shellquote_test.go index 969998c592..6c1b778a08 100644 --- a/modules/util/shellquote_test.go +++ b/modules/util/shellquote_test.go @@ -3,7 +3,11 @@ package util -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestShellEscape(t *testing.T) { tests := []struct { @@ -79,13 +83,23 @@ func TestShellEscape(t *testing.T) { "Single quotes don't need to escape except for '...", "~/ ${gitea} `gitea` (gitea) !gitea! \"gitea\" \\gitea\\ 'gitea'", "~/' ${gitea} `gitea` (gitea) !gitea! \"gitea\" \\gitea\\ '\\''gitea'\\'", + }, { + "Inline command", + "some`echo foo`thing", + "\"some\\`echo foo\\`thing\"", + }, { + "Substitution", + `;${HOME}`, + `";\${HOME}"`, + }, { + "ANSI Escape codes (not escaped)", + "\033[31;1;4mHello\033[0m", + "\"\x1b[31;1;4mHello\x1b[0m\"", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := ShellEscape(tt.toEscape); got != tt.want { - t.Errorf("ShellEscape(%q):\nGot: %s\nWanted: %s", tt.toEscape, got, tt.want) - } + assert.Equal(t, tt.want, ShellEscape(tt.toEscape)) }) } } From 9fb75a141d812b6c39e88a51eb22ce17d7569ec6 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 24 Aug 2025 02:38:45 +0200 Subject: [PATCH 4/5] chore: add migration credentials integration test --- .../repo_migrate_credentials_test.go | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 tests/integration/repo_migrate_credentials_test.go diff --git a/tests/integration/repo_migrate_credentials_test.go b/tests/integration/repo_migrate_credentials_test.go new file mode 100644 index 0000000000..b63ca9b29e --- /dev/null +++ b/tests/integration/repo_migrate_credentials_test.go @@ -0,0 +1,95 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "net/http" + "net/url" + "testing" + + "forgejo.org/models/admin" + repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/setting" + "forgejo.org/modules/structs" + "forgejo.org/modules/test" + "forgejo.org/services/migrations" + + "github.com/stretchr/testify/require" +) + +func TestRepoMigrateWithCredentials(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)() + require.NoError(t, migrations.Init()) + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, "user2") + + t.Run("Incorrect credentials", func(t *testing.T) { + session.MakeRequest(t, NewRequestWithValues(t, "POST", "/repo/migrate", map[string]string{ + "_csrf": GetCSRF(t, session, "/repo/migrate?service_type=1"), + "clone_addr": u.JoinPath("/user2/repo2").String(), + "auth_username": "user2", + "auth_password": userPassword + "1", + "uid": "2", + "repo_name": "migrating-with-credentials", + "service": "1", + }), http.StatusSeeOther) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "migrating-with-credentials"}, "is_empty = true") + unittest.AssertExistsAndLoadBean(t, &admin.Task{ + RepoID: repo.ID, + Type: structs.TaskTypeMigrateRepo, + Status: structs.TaskStatusFailed, + }) + }) + + t.Run("Normal", func(t *testing.T) { + session.MakeRequest(t, NewRequestWithValues(t, "POST", "/repo/migrate", map[string]string{ + "_csrf": GetCSRF(t, session, "/repo/migrate?service_type=1"), + "clone_addr": u.JoinPath("/user2/repo2").String(), + "auth_username": "user2", + "auth_password": userPassword, + "uid": "2", + "repo_name": "migrating-with-credentials-2", + "service": "1", + }), http.StatusSeeOther) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "migrating-with-credentials-2"}, "is_empty = false") + unittest.AssertExistsAndLoadBean(t, &admin.Task{ + RepoID: repo.ID, + Type: structs.TaskTypeMigrateRepo, + Status: structs.TaskStatusFinished, + }) + }) + + t.Run("Dangerous credential", func(t *testing.T) { + // Temporaily change the password + dangerousPassword := "some`echo foo`thing" + require.NoError(t, user2.SetPassword(dangerousPassword)) + require.NoError(t, user_model.UpdateUserCols(t.Context(), user2, "passwd", "passwd_hash_algo", "salt")) + + session = loginUserWithPassword(t, "user2", dangerousPassword) + + session.MakeRequest(t, NewRequestWithValues(t, "POST", "/repo/migrate", map[string]string{ + "_csrf": GetCSRF(t, session, "/repo/migrate?service_type=1"), + "clone_addr": u.JoinPath("/user2/repo2").String(), + "auth_username": "user2", + "auth_password": dangerousPassword, + "uid": "2", + "repo_name": "migrating-with-credentials-3", + "service": "1", + }), http.StatusSeeOther) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "migrating-with-credentials-3"}, "is_empty = false") + unittest.AssertExistsAndLoadBean(t, &admin.Task{ + RepoID: repo.ID, + Type: structs.TaskTypeMigrateRepo, + Status: structs.TaskStatusFinished, + }) + }) + }) +} From f7fb1226a4eee0ff2c0c093c38ee9c74a376e85a Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 24 Aug 2025 02:44:22 +0200 Subject: [PATCH 5/5] chore: unbreak existing tests Because the user:password is no longer automatically set as upstream origin, we have to set it manually if we want push to work. --- tests/integration/codeowner_test.go | 1 + tests/integration/git_helper_for_declarative_test.go | 12 ++++++++++++ tests/integration/patch_status_test.go | 1 + tests/integration/pull_diff_test.go | 1 + tests/integration/pull_review_test.go | 1 + 5 files changed, 16 insertions(+) diff --git a/tests/integration/codeowner_test.go b/tests/integration/codeowner_test.go index b85a5f213d..51468ffe09 100644 --- a/tests/integration/codeowner_test.go +++ b/tests/integration/codeowner_test.go @@ -47,6 +47,7 @@ func CodeOwnerTestCommon(t *testing.T, u *url.URL, codeownerTest CodeownerTest) cloneURL, _ := url.Parse(r) cloneURL.User = url.UserPassword("user2", userPassword) require.NoError(t, git.CloneWithArgs(t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{})) + doGitSetRemoteURL(dstPath, "origin", cloneURL)(t) t.Run("Normal", func(t *testing.T) { defer tests.PrintCurrentTest(t)() diff --git a/tests/integration/git_helper_for_declarative_test.go b/tests/integration/git_helper_for_declarative_test.go index 89453296ca..5be5c6088c 100644 --- a/tests/integration/git_helper_for_declarative_test.go +++ b/tests/integration/git_helper_for_declarative_test.go @@ -105,6 +105,8 @@ func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { exist, err := util.IsExist(filepath.Join(dstLocalPath, "README.md")) require.NoError(t, err) assert.True(t, exist) + // Set user:password + doGitSetRemoteURL(dstLocalPath, "origin", u)(t) } } @@ -117,6 +119,8 @@ func doPartialGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { exist, err := util.IsExist(filepath.Join(dstLocalPath, "README.md")) require.NoError(t, err) assert.True(t, exist) + // Set user:password + doGitSetRemoteURL(dstLocalPath, "origin", u)(t) } } @@ -162,6 +166,14 @@ func doGitAddRemote(dstPath, remoteName string, u *url.URL) func(*testing.T) { } } +func doGitSetRemoteURL(dstPath, remoteName string, u *url.URL) func(*testing.T) { + return func(t *testing.T) { + t.Helper() + _, _, err := git.NewCommand(git.DefaultContext, "remote", "set-url").AddDynamicArguments(remoteName, u.String()).RunStdString(&git.RunOpts{Dir: dstPath}) + require.NoError(t, err) + } +} + func doGitPushTestRepository(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { t.Helper() diff --git a/tests/integration/patch_status_test.go b/tests/integration/patch_status_test.go index 3ce1dc4cb9..4a5edccf59 100644 --- a/tests/integration/patch_status_test.go +++ b/tests/integration/patch_status_test.go @@ -78,6 +78,7 @@ func TestPatchStatus(t *testing.T) { // Clone repository. dstPath := t.TempDir() require.NoError(t, git.Clone(t.Context(), u.String(), dstPath, git.CloneRepoOptions{})) + doGitSetRemoteURL(dstPath, "origin", u)(t) // Add `fork` remote. u.Path = forkRepo.FullName() diff --git a/tests/integration/pull_diff_test.go b/tests/integration/pull_diff_test.go index 7442909aec..0f28bbbd49 100644 --- a/tests/integration/pull_diff_test.go +++ b/tests/integration/pull_diff_test.go @@ -93,6 +93,7 @@ func TestPullDiff_AGitNotEditable(t *testing.T) { cloneURL, _ := url.Parse(clone) cloneURL.User = url.UserPassword("user2", userPassword) require.NoError(t, git.CloneWithArgs(t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{})) + doGitSetRemoteURL(dstPath, "origin", cloneURL)(t) return dstPath } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 35f287958c..b385916f96 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -791,6 +791,7 @@ func TestPullRequestStaleReview(t *testing.T) { cloneURL, _ := url.Parse(clone) cloneURL.User = url.UserPassword("user2", userPassword) require.NoError(t, git.CloneWithArgs(t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{})) + doGitSetRemoteURL(dstPath, "origin", cloneURL)(t) return dstPath }