chore: do not leak global repository unit defaults (#11470)

The [test disabling the issue unit][0] took care of [reseting the disabled units][1]. However, it overlooked that calling [`LoadUnitConfig`][2] also has a [side effect on `DefaultRepoUnits`][3]. It happens when [`validateDefaultRepoUnits` has a side effect][4] on the array backing the slice, when it is [not recreated][5]. As a result the issue unit is disabled for all tests that run after this one.

The subtle side effect is harmless because it only happens in tests, the `LoadUnitConfig` is otherwise only called once. For clarity `LoadUnitConfig` is modified to clone the unit array being validated so that the returned slice is never backed by the same array as the argument.

As the global variables used for repository units should be saved and restored as a whole, a dedicated test function (`SaveUnits`) is provided to be used by both integration tests and unit tests. The test of the unit model is refactored to be a blackbox test in order to avoid an import cycle.

[0]: cce5f868ce/tests/integration/repo_settings_test.go (L258)
[1]: cce5f868ce/tests/integration/repo_settings_test.go (L253)
[2]: cce5f868ce/models/unit/unit.go (L171)
[3]: cce5f868ce/models/unit/unit.go (L182)
[4]: cce5f868ce/models/unit/unit.go (L162)
[5]: cce5f868ce/models/unit/unit.go (L148)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11470
Reviewed-by: Ellen Εμιλία Άννα Zscheile <fogti@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: limiting-factor <limiting-factor@posteo.com>
Co-committed-by: limiting-factor <limiting-factor@posteo.com>

models/unit/tests/units.go

@@ -0,0 +1,35 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package tests
+
+import (
+	unit_model "forgejo.org/models/unit"
+	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
+)
+
+func SaveUnits() func() {
+	disabledGlobal := unit_model.DisabledRepoUnitsGet()
+	restoreDisabledGlobal := func() {
+		unit_model.DisabledRepoUnitsSet(disabledGlobal)
+	}
+	restoreDisabledRepo := test.MockProtect(&setting.Repository.DisabledRepoUnits)
+
+	restoreDefaultGlobal := test.MockProtect(&unit_model.DefaultRepoUnits)
+	restoreDefaultRepo := test.MockProtect(&setting.Repository.DefaultRepoUnits)
+
+	restoreForkGlobal := test.MockProtect(&unit_model.DefaultForkRepoUnits)
+	restoreForkRepo := test.MockProtect(&setting.Repository.DefaultForkRepoUnits)
+
+	return func() {
+		restoreDisabledGlobal()
+		restoreDisabledRepo()
+
+		restoreDefaultGlobal()
+		restoreDefaultRepo()
+
+		restoreForkGlobal()
+		restoreForkRepo()
+	}
+}

models/unit/tests/units_test.go

@@ -0,0 +1,37 @@
+// Copyright 2026 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package tests
+
+import (
+	"testing"
+
+	unit_model "forgejo.org/models/unit"
+	"forgejo.org/modules/setting"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSaveUnits(t *testing.T) {
+	restoreUnits := SaveUnits()
+
+	unit_model.DisabledRepoUnitsSet([]unit_model.Type{unit_model.TypeInvalid})
+	setting.Repository.DisabledRepoUnits = []string{"invalid"}
+
+	unit_model.DefaultRepoUnits = []unit_model.Type{unit_model.TypeInvalid}
+	setting.Repository.DefaultRepoUnits = []string{"invalid"}
+
+	unit_model.DefaultForkRepoUnits = []unit_model.Type{unit_model.TypeInvalid}
+	setting.Repository.DefaultForkRepoUnits = []string{"invalid"}
+
+	restoreUnits()
+
+	assert.NotEqual(t, []unit_model.Type{unit_model.TypeInvalid}, unit_model.DisabledRepoUnitsGet())
+	assert.NotEqual(t, []string{"invalid"}, setting.Repository.DisabledRepoUnits)
+
+	assert.NotEqual(t, []unit_model.Type{unit_model.TypeInvalid}, unit_model.DefaultRepoUnits)
+	assert.NotEqual(t, []string{"invalid"}, setting.Repository.DefaultRepoUnits)
+
+	assert.NotEqual(t, []unit_model.Type{unit_model.TypeInvalid}, unit_model.DefaultForkRepoUnits)
+	assert.NotEqual(t, []string{"invalid"}, setting.Repository.DefaultForkRepoUnits)
+}

models/unit/unit.go

@@ -7,6 +7,7 @@ package unit
 import (
 	"errors"
 	"fmt"
+	"slices"
 	"strings"
 	"sync/atomic"
 
@@ -141,7 +142,7 @@ func DisabledRepoUnitsSet(v []Type) {
 
 // Get valid set of default repository units from settings
 func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type {
-	units := defaultUnits
+	units := slices.Clone(defaultUnits)
 
 	// Use setting if not empty
 	if len(settingDefaultUnits) > 0 {

models/unit/unit_test.go

@@ -1,11 +1,13 @@
 // Copyright 2023 The Gitea Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
-package unit
+package unit_test
 
 import (
 	"testing"
 
+	unit_model "forgejo.org/models/unit"
+	"forgejo.org/models/unit/tests"
 	"forgejo.org/modules/setting"
 
 	"github.com/stretchr/testify/assert"
@@ -14,83 +16,47 @@ import (
 
 func TestLoadUnitConfig(t *testing.T) {
 	t.Run("regular", func(t *testing.T) {
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) {
+		defer tests.SaveUnits()()
-			DisabledRepoUnitsSet(disabledRepoUnits)
-			DefaultRepoUnits = defaultRepoUnits
-			DefaultForkRepoUnits = defaultForkRepoUnits
-		}(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits)
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) {
-			setting.Repository.DisabledRepoUnits = disabledRepoUnits
-			setting.Repository.DefaultRepoUnits = defaultRepoUnits
-			setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits
-		}(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits)
 
 		setting.Repository.DisabledRepoUnits = []string{"repo.issues"}
 		setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls"}
 		setting.Repository.DefaultForkRepoUnits = []string{"repo.releases"}
-		require.NoError(t, LoadUnitConfig())
+		require.NoError(t, unit_model.LoadUnitConfig())
-		assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet())
+		assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet())
-		assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypeReleases, unit_model.TypePullRequests}, unit_model.DefaultRepoUnits)
-		assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits)
 	})
 	t.Run("invalid", func(t *testing.T) {
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) {
+		defer tests.SaveUnits()()
-			DisabledRepoUnitsSet(disabledRepoUnits)
-			DefaultRepoUnits = defaultRepoUnits
-			DefaultForkRepoUnits = defaultForkRepoUnits
-		}(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits)
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) {
-			setting.Repository.DisabledRepoUnits = disabledRepoUnits
-			setting.Repository.DefaultRepoUnits = defaultRepoUnits
-			setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits
-		}(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits)
 
 		setting.Repository.DisabledRepoUnits = []string{"repo.issues", "invalid.1"}
 		setting.Repository.DefaultRepoUnits = []string{"repo.code", "invalid.2", "repo.releases", "repo.issues", "repo.pulls"}
 		setting.Repository.DefaultForkRepoUnits = []string{"invalid.3", "repo.releases"}
-		require.NoError(t, LoadUnitConfig())
+		require.NoError(t, unit_model.LoadUnitConfig())
-		assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet())
+		assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet())
-		assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypeReleases, unit_model.TypePullRequests}, unit_model.DefaultRepoUnits)
-		assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits)
 	})
 	t.Run("duplicate", func(t *testing.T) {
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) {
+		defer tests.SaveUnits()()
-			DisabledRepoUnitsSet(disabledRepoUnits)
-			DefaultRepoUnits = defaultRepoUnits
-			DefaultForkRepoUnits = defaultForkRepoUnits
-		}(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits)
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) {
-			setting.Repository.DisabledRepoUnits = disabledRepoUnits
-			setting.Repository.DefaultRepoUnits = defaultRepoUnits
-			setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits
-		}(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits)
 
 		setting.Repository.DisabledRepoUnits = []string{"repo.issues", "repo.issues"}
 		setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls", "repo.code"}
 		setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"}
-		require.NoError(t, LoadUnitConfig())
+		require.NoError(t, unit_model.LoadUnitConfig())
-		assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet())
+		assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet())
-		assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypeReleases, unit_model.TypePullRequests}, unit_model.DefaultRepoUnits)
-		assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits)
 	})
 	t.Run("empty_default", func(t *testing.T) {
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) {
+		defer tests.SaveUnits()()
-			DisabledRepoUnitsSet(disabledRepoUnits)
-			DefaultRepoUnits = defaultRepoUnits
-			DefaultForkRepoUnits = defaultForkRepoUnits
-		}(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits)
-		defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) {
-			setting.Repository.DisabledRepoUnits = disabledRepoUnits
-			setting.Repository.DefaultRepoUnits = defaultRepoUnits
-			setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits
-		}(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits)
 
 		setting.Repository.DisabledRepoUnits = []string{"repo.issues", "repo.issues"}
 		setting.Repository.DefaultRepoUnits = []string{}
 		setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"}
-		require.NoError(t, LoadUnitConfig())
+		require.NoError(t, unit_model.LoadUnitConfig())
-		assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet())
+		assert.Equal(t, []unit_model.Type{unit_model.TypeIssues}, unit_model.DisabledRepoUnitsGet())
-		assert.ElementsMatch(t, []Type{TypeCode, TypePullRequests, TypeReleases, TypeWiki, TypePackages, TypeProjects, TypeActions}, DefaultRepoUnits)
+		assert.ElementsMatch(t, []unit_model.Type{unit_model.TypeCode, unit_model.TypePullRequests, unit_model.TypeReleases, unit_model.TypeWiki, unit_model.TypePackages, unit_model.TypeProjects, unit_model.TypeActions}, unit_model.DefaultRepoUnits)
-		assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits)
+		assert.Equal(t, []unit_model.Type{unit_model.TypeReleases}, unit_model.DefaultForkRepoUnits)
 	})
 }

tests/integration/repo_settings_test.go

@@ -12,6 +12,7 @@ import (
 	git_model "forgejo.org/models/git"
 	repo_model "forgejo.org/models/repo"
 	unit_model "forgejo.org/models/unit"
+	unit_tests "forgejo.org/models/unit/tests"
 	"forgejo.org/models/unittest"
 	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/optional"
@@ -222,13 +223,12 @@ func TestRepoAddMoreUnits(t *testing.T) {
 
 	t.Run("no add more if unit is globally disabled", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
+		defer unit_tests.SaveUnits()()
 		defer func() {
 			repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, []repo_model.RepoUnit{{
 				RepoID: repo.ID,
 				Type:   unit_model.TypePackages,
 			}}, nil)
-			setting.Repository.DisabledRepoUnits = []string{}
-			unit_model.LoadUnitConfig()
 		}()
 
 		// Disable the Packages unit globally
@@ -245,13 +245,12 @@ func TestRepoAddMoreUnits(t *testing.T) {
 
 	t.Run("issues & ext tracker globally disabled", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
+		defer unit_tests.SaveUnits()()
 		defer func() {
 			repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, []repo_model.RepoUnit{{
 				RepoID: repo.ID,
 				Type:   unit_model.TypeIssues,
 			}}, nil)
-			setting.Repository.DisabledRepoUnits = []string{}
-			unit_model.LoadUnitConfig()
 		}()
 
 		// Disable both Issues and ExternalTracker units globally