From 492a9145b775f1ec32188c5dc4fc7b0f7e61d72e Mon Sep 17 00:00:00 2001 From: Nicolas Date: Mon, 15 Jun 2026 22:24:30 +0200 Subject: [PATCH] cleanup --- modules/git/repo.go | 9 +++------ modules/git/repo_test.go | 23 ++++++----------------- services/repository/migrate.go | 18 ++++++++---------- 3 files changed, 17 insertions(+), 33 deletions(-) diff --git a/modules/git/repo.go b/modules/git/repo.go index 7be2a91bc9..289033332b 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -111,9 +111,6 @@ type CloneRepoOptions struct { SkipTLSVerify bool SingleBranch bool Env []string - // NoFollowRedirects refuses HTTP redirects during the clone. It is used for - // migrations to stop a remote redirecting to an otherwise-blocked address (SSRF). - NoFollowRedirects bool } // Clone clones original repository to target path. @@ -124,12 +121,12 @@ func Clone(ctx context.Context, from, to string, opts CloneRepoOptions) error { } cmd := gitcmd.NewCommand().AddArguments("clone") + // Never follow HTTP redirects: no clone caller needs them, and a remote redirecting to an + // otherwise-blocked address would be an SSRF vector (e.g. migrating from an attacker URL). + cmd.AddArguments("-c", "http.followRedirects=false") if opts.SkipTLSVerify { cmd.AddArguments("-c", "http.sslVerify=false") } - if opts.NoFollowRedirects { - cmd.AddArguments("-c", "http.followRedirects=false") - } if opts.Mirror { cmd.AddArguments("--mirror") } diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index 76acac0b76..be0a21a83d 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -23,10 +23,9 @@ func TestRepoIsEmpty(t *testing.T) { assert.True(t, isEmpty) } -// TestCloneNoFollowRedirects ensures the migration clone refuses HTTP redirects, -// so a remote cannot redirect to an otherwise-blocked address (SSRF). Without the -// option git follows the redirect and reaches the target. -func TestCloneNoFollowRedirects(t *testing.T) { +// TestCloneRefusesRedirects ensures Clone never follows HTTP redirects, so a remote +// cannot redirect to an otherwise-blocked address (SSRF, e.g. during migration). +func TestCloneRefusesRedirects(t *testing.T) { var targetHit atomic.Bool target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { targetHit.Store(true) @@ -39,17 +38,7 @@ func TestCloneNoFollowRedirects(t *testing.T) { })) defer redirect.Close() - t.Run("FollowsRedirectByDefault", func(t *testing.T) { - targetHit.Store(false) - err := Clone(t.Context(), redirect.URL, filepath.Join(t.TempDir(), "dst"), CloneRepoOptions{}) - assert.Error(t, err) - assert.True(t, targetHit.Load(), "git should reach the redirect target without the protection") - }) - - t.Run("RefusesRedirect", func(t *testing.T) { - targetHit.Store(false) - err := Clone(t.Context(), redirect.URL, filepath.Join(t.TempDir(), "dst"), CloneRepoOptions{NoFollowRedirects: true}) - assert.Error(t, err) - assert.False(t, targetHit.Load(), "git must not follow the redirect to the target") - }) + err := Clone(t.Context(), redirect.URL, filepath.Join(t.TempDir(), "dst"), CloneRepoOptions{}) + assert.Error(t, err) + assert.False(t, targetHit.Load(), "git must not follow the redirect to the target") } diff --git a/services/repository/migrate.go b/services/repository/migrate.go index d59b81e046..a0182769f4 100644 --- a/services/repository/migrate.go +++ b/services/repository/migrate.go @@ -45,11 +45,10 @@ func cloneWiki(ctx context.Context, repo *repo_model.Repository, opts migration. } } if err := gitrepo.CloneExternalRepo(ctx, wikiRemoteURL, storageRepo, git.CloneRepoOptions{ - Mirror: true, - Quiet: true, - Timeout: migrateTimeout, - SkipTLSVerify: setting.Migrations.SkipTLSVerify, - NoFollowRedirects: true, + Mirror: true, + Quiet: true, + Timeout: migrateTimeout, + SkipTLSVerify: setting.Migrations.SkipTLSVerify, }); err != nil { log.Error("Clone wiki failed, err: %v", err) cleanIncompleteWikiPath() @@ -92,11 +91,10 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User, } if err := gitrepo.CloneExternalRepo(ctx, opts.CloneAddr, repo, git.CloneRepoOptions{ - Mirror: true, - Quiet: true, - Timeout: migrateTimeout, - SkipTLSVerify: setting.Migrations.SkipTLSVerify, - NoFollowRedirects: true, + Mirror: true, + Quiet: true, + Timeout: migrateTimeout, + SkipTLSVerify: setting.Migrations.SkipTLSVerify, }); err != nil { if errors.Is(err, context.DeadlineExceeded) { return repo, fmt.Errorf("clone timed out, consider increasing [git.timeout] MIGRATE in app.ini, underlying err: %w", err)