Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions pkg/vendir/cmd/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,13 @@
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,

Check failure on line 136 in pkg/vendir/cmd/sync.go

View workflow job for this annotation

GitHub Actions / lint

add-constant: avoid magic numbers like '0', create a named constant for it (revive)
AllowAllSymlinkDestinations: o.AllowAllSymlinkDestinations,
}
newLockConfig := ctlconf.NewLockConfig()

Expand All @@ -145,12 +146,6 @@
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)
}
Expand Down
24 changes: 18 additions & 6 deletions pkg/vendir/directory/directory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 16 additions & 0 deletions pkg/vendir/directory/symlink.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@
"strings"
)

// RemoveDanglingSymlinks removes symlinks within path whose targets do not exist.
func RemoveDanglingSymlinks(path string) error {

Check failure on line 15 in pkg/vendir/directory/symlink.go

View workflow job for this annotation

GitHub Actions / lint

cognitive-complexity: function RemoveDanglingSymlinks has cognitive complexity 8 (> max enabled 7) (revive)
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)
Expand Down
46 changes: 46 additions & 0 deletions pkg/vendir/directory/symlink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,52 @@
"testing"
)

func TestRemoveDanglingSymlinks(t *testing.T) {

Check failure on line 12 in pkg/vendir/directory/symlink_test.go

View workflow job for this annotation

GitHub Actions / lint

cognitive-complexity: function TestRemoveDanglingSymlinks has cognitive complexity 8 (> max enabled 7) (revive)
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")

Check failure on line 54 in pkg/vendir/directory/symlink_test.go

View workflow job for this annotation

GitHub Actions / lint

unnecessary-format: unnecessary use of formatting function "t.Errorf", you can replace it with "t.Error" (revive)
}
}

func TestValidateSymlinks(t *testing.T) {
root, err := os.MkdirTemp("", "vendir-test")
if err != nil {
Expand Down
69 changes: 69 additions & 0 deletions test/e2e/invalid_symlink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,75 @@
"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))

Check failure on line 71 in test/e2e/invalid_symlink_test.go

View workflow job for this annotation

GitHub Actions / lint

add-constant: avoid magic numbers like '0666', create a named constant for it (revive)

_, 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"))

Check failure on line 81 in test/e2e/invalid_symlink_test.go

View workflow job for this annotation

GitHub Actions / lint

add-constant: string literal "result" appears, at least, 3 times, create a named constant for it (revive)
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{}}
Expand Down
Loading