From 0b653699d671b6ee5b88474fa53a7aaa1b4dc32a Mon Sep 17 00:00:00 2001 From: German Lashevich Date: Wed, 18 Mar 2026 15:44:55 +0100 Subject: [PATCH] fix: validate and remove dangling in-bindle symlinks fixes #57 Signed-off-by: German Lashevich --- pkg/vendir/cmd/sync.go | 19 +++----- pkg/vendir/directory/directory.go | 24 +++++++--- pkg/vendir/directory/symlink.go | 16 +++++++ pkg/vendir/directory/symlink_test.go | 46 +++++++++++++++++++ test/e2e/invalid_symlink_test.go | 69 ++++++++++++++++++++++++++++ 5 files changed, 156 insertions(+), 18 deletions(-) diff --git a/pkg/vendir/cmd/sync.go b/pkg/vendir/cmd/sync.go index 4e2cc816..126f8d9b 100644 --- a/pkg/vendir/cmd/sync.go +++ b/pkg/vendir/cmd/sync.go @@ -128,12 +128,13 @@ func (o *SyncOptions) Run() error { return fmt.Errorf("Unable to create cache: %s", err) } syncOpts := ctldir.SyncOpts{ - RefFetcher: ctldir.NewNamedRefFetcher(secrets, configMaps), - GithubAPIToken: os.Getenv("VENDIR_GITHUB_API_TOKEN"), - HelmBinary: os.Getenv("VENDIR_HELM_BINARY"), - Cache: cache, - Lazy: o.Lazy, - Partial: len(dirs) > 0, + RefFetcher: ctldir.NewNamedRefFetcher(secrets, configMaps), + GithubAPIToken: os.Getenv("VENDIR_GITHUB_API_TOKEN"), + HelmBinary: os.Getenv("VENDIR_HELM_BINARY"), + Cache: cache, + Lazy: o.Lazy, + Partial: len(dirs) > 0, + AllowAllSymlinkDestinations: o.AllowAllSymlinkDestinations, } newLockConfig := ctlconf.NewLockConfig() @@ -145,12 +146,6 @@ func (o *SyncOptions) Run() error { if err != nil { return fmt.Errorf("Syncing directory '%s': %s", dirConf.Path, err) } - if !o.AllowAllSymlinkDestinations { - err = ctldir.ValidateSymlinks(dirConf.Path) - if err != nil { - return err - } - } newLockConfig.Directories = append(newLockConfig.Directories, dirLockConf) } diff --git a/pkg/vendir/directory/directory.go b/pkg/vendir/directory/directory.go index 5ee173c5..f58bbeb3 100644 --- a/pkg/vendir/directory/directory.go +++ b/pkg/vendir/directory/directory.go @@ -38,12 +38,13 @@ func NewDirectory(opts ctlconf.Directory, lockDirectory ctlconf.LockDirectory, u } type SyncOpts struct { - RefFetcher ctlfetch.RefFetcher - GithubAPIToken string - HelmBinary string - Cache ctlcache.Cache - Lazy bool - Partial bool + RefFetcher ctlfetch.RefFetcher + GithubAPIToken string + HelmBinary string + Cache ctlcache.Cache + Lazy bool + Partial bool + AllowAllSymlinkDestinations bool } func createConfigDigest(contents ctlconf.DirectoryContents) (string, error) { @@ -230,11 +231,22 @@ func (d *Directory) Sync(syncOpts SyncOpts) (ctlconf.LockDirectory, error) { return lockConfig, fmt.Errorf("Unknown contents type for directory '%s'", contents.Path) } + if !syncOpts.AllowAllSymlinkDestinations { + err = ValidateSymlinks(stagingDstPath) + if err != nil { + return lockConfig, fmt.Errorf("Validating symlinks in directory '%s': %s", contents.Path, err) + } + } + if !skipFileFilter { err = FileFilter{contents}.Apply(stagingDstPath) if err != nil { return lockConfig, fmt.Errorf("Filtering paths in directory '%s': %s", contents.Path, err) } + err = RemoveDanglingSymlinks(stagingDstPath) + if err != nil { + return lockConfig, fmt.Errorf("Removing dangling symlinks in directory '%s': %s", contents.Path, err) + } } if !skipNewRootPath && len(contents.NewRootPath) > 0 { diff --git a/pkg/vendir/directory/symlink.go b/pkg/vendir/directory/symlink.go index f899805b..af6f5cfb 100644 --- a/pkg/vendir/directory/symlink.go +++ b/pkg/vendir/directory/symlink.go @@ -11,6 +11,22 @@ import ( "strings" ) +// RemoveDanglingSymlinks removes symlinks within path whose targets do not exist. +func RemoveDanglingSymlinks(path string) error { + return filepath.WalkDir(path, func(entryPath string, info fs.DirEntry, err error) error { + if err != nil { + return err + } + if info.Type()&os.ModeSymlink == os.ModeSymlink { + _, statErr := os.Stat(entryPath) + if statErr != nil && os.IsNotExist(statErr) { + return os.Remove(entryPath) + } + } + return nil + }) +} + // ValidateSymlinks enforces that symlinks inside the given path resolve to inside the path func ValidateSymlinks(path string) error { absRoot, err := filepath.Abs(path) diff --git a/pkg/vendir/directory/symlink_test.go b/pkg/vendir/directory/symlink_test.go index 0748bae3..2d77127c 100644 --- a/pkg/vendir/directory/symlink_test.go +++ b/pkg/vendir/directory/symlink_test.go @@ -9,6 +9,52 @@ import ( "testing" ) +func TestRemoveDanglingSymlinks(t *testing.T) { + root, err := os.MkdirTemp("", "vendir-test") + if err != nil { + t.Fatalf("failed to create tmpdir: %v", err) + } + defer os.RemoveAll(root) + + root, err = filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("failed to read link tmpdir: %v", err) + } + + // existing file that a valid symlink points to + existingFile := filepath.Join(root, "existing.txt") + f, err := os.Create(existingFile) + if err != nil { + t.Fatalf("failed to create file: %v", err) + } + f.Close() + + // valid symlink: points to existing file + validLink := filepath.Join(root, "valid_link") + if err = os.Symlink(existingFile, validLink); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + // dangling symlink: points to non-existent file + danglingLink := filepath.Join(root, "dangling_link") + if err = os.Symlink(filepath.Join(root, "nonexistent.txt"), danglingLink); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + if err = RemoveDanglingSymlinks(root); err != nil { + t.Fatalf("RemoveDanglingSymlinks() error = %v", err) + } + + // valid symlink must still exist + if _, err = os.Lstat(validLink); err != nil { + t.Errorf("valid symlink was unexpectedly removed: %v", err) + } + // dangling symlink must be gone + if _, err = os.Lstat(danglingLink); err == nil { + t.Errorf("dangling symlink was not removed") + } +} + func TestValidateSymlinks(t *testing.T) { root, err := os.MkdirTemp("", "vendir-test") if err != nil { diff --git a/test/e2e/invalid_symlink_test.go b/test/e2e/invalid_symlink_test.go index bafd3291..932205a1 100644 --- a/test/e2e/invalid_symlink_test.go +++ b/test/e2e/invalid_symlink_test.go @@ -13,6 +13,75 @@ import ( "sigs.k8s.io/yaml" ) +// TestSymlinkWithIncludePaths verifies that a sync with includePaths succeeds even when +// the filtered content contains symlinks pointing to paths that are excluded by includePaths. +// The dangling symlinks must be silently removed rather than causing an error. +func TestSymlinkWithIncludePaths(t *testing.T) { + env := BuildEnv(t) + vendir := Vendir{t, env.BinaryPath, Logger{}} + + tmpDir, err := os.MkdirTemp("", "vendir-test") + require.NoError(t, err) + defer os.RemoveAll(tmpDir) + + tmpDir, err = filepath.EvalSymlinks(tmpDir) + require.NoError(t, err) + + // Source layout: + // src/ + // dir1/ + // kept.txt <- included by includePaths + // link_to_other -> ../dir2/other.txt (will dangle after filtering) + // dir2/ + // other.txt <- NOT included by includePaths + srcDir := filepath.Join(tmpDir, "src") + dir1 := filepath.Join(srcDir, "dir1") + dir2 := filepath.Join(srcDir, "dir2") + require.NoError(t, os.MkdirAll(dir1, os.ModePerm)) + require.NoError(t, os.MkdirAll(dir2, os.ModePerm)) + + keptFile, err := os.Create(filepath.Join(dir1, "kept.txt")) + require.NoError(t, err) + keptFile.Close() + + otherFile, err := os.Create(filepath.Join(dir2, "other.txt")) + require.NoError(t, err) + otherFile.Close() + + // Symlink uses a relative path (../dir2/other.txt) so it is internal to the bundle + require.NoError(t, os.Symlink("../dir2/other.txt", filepath.Join(dir1, "link_to_other"))) + + cfg := config.Config{ + APIVersion: "vendir.k14s.io/v1alpha1", + Kind: "Config", + Directories: []config.Directory{{ + Path: "result", + Contents: []config.DirectoryContents{{ + Path: "out", + Directory: &config.DirectoryContentsDirectory{ + Path: "src", + }, + IncludePaths: []string{"dir1/**"}, + }}, + }}, + } + + cfgBytes, err := yaml.Marshal(cfg) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "vendir.yml"), cfgBytes, 0666)) + + _, err = vendir.RunWithOpts([]string{"sync"}, RunOpts{Dir: tmpDir, AllowError: true}) + require.NoError(t, err, "sync should succeed even though includePaths causes a dangling symlink") + + // The dangling symlink must have been removed + _, err = os.Lstat(filepath.Join(tmpDir, "result", "out", "dir1", "link_to_other")) + require.True(t, os.IsNotExist(err), "dangling symlink should have been removed after filtering") + + // The kept file must still be present + _, err = os.Stat(filepath.Join(tmpDir, "result", "out", "dir1", "kept.txt")) + require.NoError(t, err, "kept.txt should be present after filtering") +} + func TestInvalidSymlink(t *testing.T) { env := BuildEnv(t) vendir := Vendir{t, env.BinaryPath, Logger{}}