cleanup

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")
 	}

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")
 }

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)