fix: avoid scanning symlinks in whole repo on each app manifest operation (#26718)

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

reposerver/repository/repository.go

@@ -15,10 +15,12 @@ import (
 	"regexp"
 	"sort"
 	"strings"
+	gosync "sync"
 	"time"
 
 	"github.com/TomOnTime/utfutil"
 	imagev1 "github.com/opencontainers/image-spec/specs-go/v1"
+	gocache "github.com/patrickmn/go-cache"
 	"sigs.k8s.io/yaml"
 
 	"github.com/argoproj/argo-cd/v3/util/oci"
@@ -95,6 +97,8 @@ type Service struct {
 	newGitClient              func(rawRepoURL string, root string, creds git.Creds, insecure bool, enableLfs bool, proxy string, noProxy string, opts ...git.ClientOpts) (git.Client, error)
 	newHelmClient             func(repoURL string, creds helm.Creds, enableOci bool, proxy string, noProxy string, opts ...helm.ClientOpts) helm.Client
 	initConstants             RepoServerInitConstants
+	// stores cached symlink validation results
+	symlinksState *gocache.Cache
 	// now is usually just time.Now, but may be replaced by unit tests for testing purposes
 	now func() time.Time
 }
@@ -156,6 +160,7 @@ func NewService(metricsServer *metrics.MetricsServer, cache *cache.Cache, initCo
 		ociPaths:           ociRandomizedPaths,
 		gitRepoInitializer: directoryPermissionInitializer,
 		rootDir:            rootDir,
+		symlinksState:      gocache.New(12*time.Hour, time.Hour),
 	}
 }
 
@@ -395,7 +400,7 @@ func (s *Service) runRepoOperation(
 		defer utilio.Close(closer)
 
 		if !s.initConstants.AllowOutOfBoundsSymlinks {
-			err := apppathutil.CheckOutOfBoundsSymlinks(ociPath)
+			err := s.checkOutOfBoundsSymlinks(ociPath, revision, settings.noCache)
 			if err != nil {
 				oobError := &apppathutil.OutOfBoundsSymlinkError{}
 				if errors.As(err, &oobError) {
@@ -436,7 +441,7 @@ func (s *Service) runRepoOperation(
 		}
 		defer utilio.Close(closer)
 		if !s.initConstants.AllowOutOfBoundsSymlinks {
-			err := apppathutil.CheckOutOfBoundsSymlinks(chartPath)
+			err := s.checkOutOfBoundsSymlinks(chartPath, revision, settings.noCache)
 			if err != nil {
 				oobError := &apppathutil.OutOfBoundsSymlinkError{}
 				if errors.As(err, &oobError) {
@@ -465,7 +470,7 @@ func (s *Service) runRepoOperation(
 	defer utilio.Close(closer)
 
 	if !s.initConstants.AllowOutOfBoundsSymlinks {
-		err := apppathutil.CheckOutOfBoundsSymlinks(gitClient.Root())
+		err := s.checkOutOfBoundsSymlinks(gitClient.Root(), revision, settings.noCache, ".git")
 		if err != nil {
 			oobError := &apppathutil.OutOfBoundsSymlinkError{}
 			if errors.As(err, &oobError) {
@@ -589,6 +594,25 @@ func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.Applicat
 	return repoRefs, nil
 }
 
+// checkOutOfBoundsSymlinks validates symlinks and caches validation result in memory
+func (s *Service) checkOutOfBoundsSymlinks(rootPath string, version string, noCache bool, skipPaths ...string) error {
+	key := rootPath + "/" + version + "/" + strings.Join(skipPaths, ",")
+	ok := false
+	var checker any
+	if !noCache {
+		checker, ok = s.symlinksState.Get(key)
+	}
+
+	if !ok {
+		checker = gosync.OnceValue(func() error {
+			return apppathutil.CheckOutOfBoundsSymlinks(rootPath, skipPaths...)
+		})
+		s.symlinksState.Set(key, checker, gocache.DefaultExpiration)
+	}
+
+	return checker.(func() error)()
+}
+
 func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestRequest) (*apiclient.ManifestResponse, error) {
 	var res *apiclient.ManifestResponse
 	var err error
@@ -857,7 +881,7 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA,
 
 						// Symlink check must happen after acquiring lock.
 						if !s.initConstants.AllowOutOfBoundsSymlinks {
-							err := apppathutil.CheckOutOfBoundsSymlinks(gitClient.Root())
+							err := s.checkOutOfBoundsSymlinks(gitClient.Root(), commitSHA, q.NoCache, ".git")
 							if err != nil {
 								oobError := &apppathutil.OutOfBoundsSymlinkError{}
 								if errors.As(err, &oobError) {

util/app/path/path.go

@@ -45,20 +45,28 @@ func (e *OutOfBoundsSymlinkError) Error() string {
 // CheckOutOfBoundsSymlinks determines if basePath contains any symlinks that
 // are absolute or point to a path outside of the basePath. If found, an
 // OutOfBoundsSymlinkError is returned.
-func CheckOutOfBoundsSymlinks(basePath string) error {
+func CheckOutOfBoundsSymlinks(basePath string, skipPaths ...string) error {
 	absBasePath, err := filepath.Abs(basePath)
 	if err != nil {
 		return fmt.Errorf("failed to get absolute path: %w", err)
 	}
+	skipPathsSet := map[string]bool{}
+	for _, p := range skipPaths {
+		skipPathsSet[filepath.Join(absBasePath, p)] = true
+	}
+
 	return filepath.Walk(absBasePath, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
-			// Ignore "no such file or directory" errors than can happen with
+			// Ignore "no such file or directory" errors that can happen with
 			// temporary files such as .git/*.lock
 			if errors.Is(err, os.ErrNotExist) {
 				return nil
 			}
 			return fmt.Errorf("failed to walk for symlinks in %s: %w", absBasePath, err)
 		}
+		if skipPathsSet[path] {
+			return filepath.SkipDir
+		}
 		if files.IsSymlink(info) {
 			// We don't use filepath.EvalSymlinks because it fails without returning a path
 			// if the target doesn't exist.

util/app/path/path_test.go

@@ -83,6 +83,11 @@ func TestBadSymlinks3(t *testing.T) {
 	assert.Equal(t, "badlink", oobError.File)
 }
 
+func TestBadSymlinksExcluded(t *testing.T) {
+	err := CheckOutOfBoundsSymlinks("./testdata/badlink", "badlink")
+	assert.NoError(t, err)
+}
+
 // No absolute symlinks allowed
 func TestAbsSymlink(t *testing.T) {
 	const testDir = "./testdata/abslink"