From 37c488c518fd86937973f66a5a3acd4d9e86e6a3 Mon Sep 17 00:00:00 2001 From: Hamza <12420351+0xMH@users.noreply.github.com> Date: Wed, 4 Mar 2026 16:45:29 +0100 Subject: [PATCH 1/5] Reject paths with inner '..' in FileLoader.New to prevent silent misresolution --- api/internal/loader/fileloader.go | 27 +++++++++++++ api/internal/loader/fileloader_test.go | 54 ++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/api/internal/loader/fileloader.go b/api/internal/loader/fileloader.go index e4202815c9..4c9edad404 100644 --- a/api/internal/loader/fileloader.go +++ b/api/internal/loader/fileloader.go @@ -147,6 +147,26 @@ func newLoaderAtConfirmedDir( } } +// hasInnerDotDot reports whether path contains a ".." after a non-".." +// component, which causes filepath.Clean to silently absorb preceding +// segments (e.g. "../../base - ../../shared/prod" becomes "../../shared/prod"). +func hasInnerDotDot(path string) bool { + pastPrefix := false + for _, part := range strings.Split(filepath.ToSlash(path), "/") { + if part == "" || part == "." { + continue + } + if part == ".." { + if pastPrefix { + return true + } + continue + } + pastPrefix = true + } + return false +} + // New returns a new Loader, rooted relative to current loader, // or rooted in a temp directory holding a git repo clone. func (fl *FileLoader) New(path string) (ifc.Loader, error) { @@ -167,6 +187,13 @@ func (fl *FileLoader) New(path string) (ifc.Loader, error) { if filepath.IsAbs(path) { return nil, fmt.Errorf("new root '%s' cannot be absolute", path) } + if hasInnerDotDot(path) { + return nil, fmt.Errorf( + "path %q is ambiguous: resolves to %q after normalization, "+ + "which is likely not the intended target; check for YAML "+ + "indentation errors in your kustomization file", + path, filepath.Clean(path)) + } root, err := filesys.ConfirmDir(fl.fSys, fl.root.Join(path)) if err != nil { return nil, errors.WrapPrefixf(err, "%s", ErrRtNotDir.Error()) diff --git a/api/internal/loader/fileloader_test.go b/api/internal/loader/fileloader_test.go index 17e8fbfe5d..5fc0d2cb62 100644 --- a/api/internal/loader/fileloader_test.go +++ b/api/internal/loader/fileloader_test.go @@ -207,6 +207,60 @@ func TestNewEmptyLoader(t *testing.T) { require.Error(t, err) } +func TestLoaderRejectsMalformedPath(t *testing.T) { + // A YAML indentation error can collapse two resource entries into one: + // resources: + // - ../../base + // - ../../shared/prod + // becomes the single string: "../../base - ../../shared/prod" + // + // filepath.Clean normalizes this to "../../shared/prod", silently + // dropping the "../../base" reference. The loader must reject paths + // with inner ".." components that cause this silent absorption. + // See https://github.com/kubernetes-sigs/kustomize/issues/5979 + fSys := filesys.MakeFsInMemory() + require.NoError(t, fSys.MkdirAll("/base")) + require.NoError(t, fSys.MkdirAll("/shared/prod")) + require.NoError(t, fSys.MkdirAll("/overlays/prod1")) + + l1 := NewLoaderOrDie(RestrictionNone, fSys, "/overlays/prod1") + + // The exact bug from issue #5979. + _, err := l1.New("../../base - ../../shared/prod") + require.Error(t, err) + require.Contains(t, err.Error(), "ambiguous") + + // Same structural problem without the YAML artifact. + _, err = l1.New("a/b/../../other") + require.Error(t, err) + require.Contains(t, err.Error(), "ambiguous") +} + +func TestHasInnerDotDot(t *testing.T) { + cases := map[string]bool{ + // Safe: leading ".." only + "../base": false, + "../../shared/prod": false, + "..": false, + // Safe: no ".." at all + "foo/bar": false, + "foo/bar/": false, + "foo//bar": false, + "./foo/bar": false, + "https://root": false, + // Dangerous: inner ".." absorbs preceding components + "../../base - ../../shared/prod": true, + "a/b/../../c": true, + "foo/../bar": true, + "a/..": true, + } + for path, want := range cases { + t.Run(path, func(t *testing.T) { + require.Equal(t, want, hasInnerDotDot(path), "hasInnerDotDot(%q)", path) + }) + } +} + func TestNewRemoteLoaderDoesNotExist(t *testing.T) { _, err := makeLoader().New("https://example.com/org/repo") require.ErrorContains(t, err, "fetch") From eb979d8f4f72f33e145d89f5d467c4ab75f158c8 Mon Sep 17 00:00:00 2001 From: Hamza <12420351+0xMH@users.noreply.github.com> Date: Sat, 7 Mar 2026 12:02:52 +0100 Subject: [PATCH 2/5] Refactor hasInnerDotDot to two-phase loop eliminating mutable state --- api/internal/loader/fileloader.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/api/internal/loader/fileloader.go b/api/internal/loader/fileloader.go index 4c9edad404..d86f4c0ec7 100644 --- a/api/internal/loader/fileloader.go +++ b/api/internal/loader/fileloader.go @@ -151,18 +151,17 @@ func newLoaderAtConfirmedDir( // component, which causes filepath.Clean to silently absorb preceding // segments (e.g. "../../base - ../../shared/prod" becomes "../../shared/prod"). func hasInnerDotDot(path string) bool { - pastPrefix := false - for _, part := range strings.Split(filepath.ToSlash(path), "/") { - if part == "" || part == "." { - continue - } + parts := strings.Split(filepath.ToSlash(path), "/") + // Skip the leading ".." prefix (and empty/current-dir segments). + i := 0 + for i < len(parts) && (parts[i] == "" || parts[i] == "." || parts[i] == "..") { + i++ + } + // Any ".." after a real component is inner. + for _, part := range parts[i:] { if part == ".." { - if pastPrefix { - return true - } - continue + return true } - pastPrefix = true } return false } From c2c441cec74b315b540e59cffec91d703ee33914 Mon Sep 17 00:00:00 2001 From: Hamza <12420351+0xMH@users.noreply.github.com> Date: Sat, 7 Mar 2026 22:48:38 +0100 Subject: [PATCH 3/5] Narrow check to embedded '..' segments to allow legitimate winding paths --- api/internal/loader/fileloader.go | 29 ++++++++++---------- api/internal/loader/fileloader_test.go | 37 +++++++++++++------------- 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/api/internal/loader/fileloader.go b/api/internal/loader/fileloader.go index d86f4c0ec7..60ab0a0974 100644 --- a/api/internal/loader/fileloader.go +++ b/api/internal/loader/fileloader.go @@ -147,19 +147,20 @@ func newLoaderAtConfirmedDir( } } -// hasInnerDotDot reports whether path contains a ".." after a non-".." -// component, which causes filepath.Clean to silently absorb preceding -// segments (e.g. "../../base - ../../shared/prod" becomes "../../shared/prod"). -func hasInnerDotDot(path string) bool { - parts := strings.Split(filepath.ToSlash(path), "/") - // Skip the leading ".." prefix (and empty/current-dir segments). - i := 0 - for i < len(parts) && (parts[i] == "" || parts[i] == "." || parts[i] == "..") { - i++ - } - // Any ".." after a real component is inner. - for _, part := range parts[i:] { - if part == ".." { +// hasAmbiguousPathSegment reports whether path contains a segment with ".." +// embedded in it (not ".." itself). This pattern results from YAML indentation +// errors where two list entries merge into one scalar, e.g.: +// +// resources: +// - ../../base +// - ../../shared/prod +// +// YAML parses this as "../../base - ../../shared/prod", producing segment +// "base - .." which filepath.Clean absorbs, silently resolving to an +// unintended directory. +func hasAmbiguousPathSegment(path string) bool { + for _, part := range strings.Split(filepath.ToSlash(path), "/") { + if part != "" && part != "." && part != ".." && strings.Contains(part, "..") { return true } } @@ -186,7 +187,7 @@ func (fl *FileLoader) New(path string) (ifc.Loader, error) { if filepath.IsAbs(path) { return nil, fmt.Errorf("new root '%s' cannot be absolute", path) } - if hasInnerDotDot(path) { + if hasAmbiguousPathSegment(path) { return nil, fmt.Errorf( "path %q is ambiguous: resolves to %q after normalization, "+ "which is likely not the intended target; check for YAML "+ diff --git a/api/internal/loader/fileloader_test.go b/api/internal/loader/fileloader_test.go index 5fc0d2cb62..7e2c02d832 100644 --- a/api/internal/loader/fileloader_test.go +++ b/api/internal/loader/fileloader_test.go @@ -230,33 +230,34 @@ func TestLoaderRejectsMalformedPath(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "ambiguous") - // Same structural problem without the YAML artifact. - _, err = l1.New("a/b/../../other") - require.Error(t, err) - require.Contains(t, err.Error(), "ambiguous") } -func TestHasInnerDotDot(t *testing.T) { +func TestHasAmbiguousPathSegment(t *testing.T) { cases := map[string]bool{ - // Safe: leading ".." only + // Safe: ".." only as standalone segments "../base": false, "../../shared/prod": false, "..": false, - // Safe: no ".." at all - "foo/bar": false, - "foo/bar/": false, - "foo//bar": false, - "./foo/bar": false, - "https://root": false, - // Dangerous: inner ".." absorbs preceding components - "../../base - ../../shared/prod": true, - "a/b/../../c": true, - "foo/../bar": true, - "a/..": true, + "foo/bar": false, + "foo/bar/": false, + "foo//bar": false, + "./foo/bar": false, + "https://root": false, + "foo/../bar": false, + "a/b/../../c": false, + "a/..": false, + // Winding paths are legitimate (used by localizer) + "delta/../../../../a/b/../../alpha/beta/sibling": false, + // Dangerous: segment contains embedded ".." from YAML merge errors + "../../base - ../../shared/prod": true, + "../../base ../../shared/prod": true, + "foo/bar..baz/qux": true, + "a/b/c..d/e": true, + "../../base\t- ../../shared/prod": true, } for path, want := range cases { t.Run(path, func(t *testing.T) { - require.Equal(t, want, hasInnerDotDot(path), "hasInnerDotDot(%q)", path) + require.Equal(t, want, hasAmbiguousPathSegment(path), "hasAmbiguousPathSegment(%q)", path) }) } } From 01626f7745543ed2b14e43c28048330e466be1db Mon Sep 17 00:00:00 2001 From: Hamza <12420351+0xMH@users.noreply.github.com> Date: Mon, 9 Mar 2026 05:17:05 +0100 Subject: [PATCH 4/5] Fix gofmt alignment and trailing whitespace in new test functions --- api/internal/loader/fileloader_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/api/internal/loader/fileloader_test.go b/api/internal/loader/fileloader_test.go index 7e2c02d832..c5c810d9d0 100644 --- a/api/internal/loader/fileloader_test.go +++ b/api/internal/loader/fileloader_test.go @@ -229,7 +229,6 @@ func TestLoaderRejectsMalformedPath(t *testing.T) { _, err := l1.New("../../base - ../../shared/prod") require.Error(t, err) require.Contains(t, err.Error(), "ambiguous") - } func TestHasAmbiguousPathSegment(t *testing.T) { @@ -249,11 +248,11 @@ func TestHasAmbiguousPathSegment(t *testing.T) { // Winding paths are legitimate (used by localizer) "delta/../../../../a/b/../../alpha/beta/sibling": false, // Dangerous: segment contains embedded ".." from YAML merge errors - "../../base - ../../shared/prod": true, - "../../base ../../shared/prod": true, - "foo/bar..baz/qux": true, - "a/b/c..d/e": true, - "../../base\t- ../../shared/prod": true, + "../../base - ../../shared/prod": true, + "../../base ../../shared/prod": true, + "foo/bar..baz/qux": true, + "a/b/c..d/e": true, + "../../base\t- ../../shared/prod": true, } for path, want := range cases { t.Run(path, func(t *testing.T) { From 7fd88c21060ff010de4f5e453cf34f26cc8415c5 Mon Sep 17 00:00:00 2001 From: Hamza <12420351+0xMH@users.noreply.github.com> Date: Tue, 24 Mar 2026 22:11:33 +0100 Subject: [PATCH 5/5] Fix pre-existing lint errors in fileloader_test.go --- api/internal/loader/fileloader_test.go | 71 +++++++++++++------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/api/internal/loader/fileloader_test.go b/api/internal/loader/fileloader_test.go index c5c810d9d0..d2f83564bd 100644 --- a/api/internal/loader/fileloader_test.go +++ b/api/internal/loader/fileloader_test.go @@ -66,7 +66,7 @@ type testData struct { expectedContent string } -var testCases = []testData{ +var testCases = []testData{ //nolint:gochecknoglobals { path: "foo/project/fileA.yaml", expectedContent: "fileA content", @@ -88,7 +88,7 @@ var testCases = []testData{ func MakeFakeFs(td []testData) filesys.FileSystem { fSys := filesys.MakeFsInMemory() for _, x := range td { - fSys.WriteFile(x.path, []byte(x.expectedContent)) + _ = fSys.WriteFile(x.path, []byte(x.expectedContent)) } return fSys } @@ -161,31 +161,31 @@ func TestLoaderBadRelative(t *testing.T) { require.Equal("/foo/project/subdir1", l1.Root()) // Cannot cd into a file. - l2, err := l1.New("fileB.yaml") + _, err = l1.New("fileB.yaml") require.Error(err) // It's not okay to stay at the same place. - l2, err = l1.New(filesys.SelfDir) + _, err = l1.New(filesys.SelfDir) require.Error(err) // It's not okay to go up and back down into same place. - l2, err = l1.New("../subdir1") + _, err = l1.New("../subdir1") require.Error(err) // It's not okay to go up via a relative path. - l2, err = l1.New("..") + _, err = l1.New("..") require.Error(err) // It's not okay to go up via an absolute path. - l2, err = l1.New("/foo/project") + _, err = l1.New("/foo/project") require.Error(err) // It's not okay to go to the root. - l2, err = l1.New("/") + _, err = l1.New("/") require.Error(err) // It's okay to go up and down to a sibling. - l2, err = l1.New("../subdir2") + l2, err := l1.New("../subdir2") require.NoError(err) require.Equal("/foo/project/subdir2", l2.Root()) @@ -198,7 +198,7 @@ func TestLoaderBadRelative(t *testing.T) { // It's not OK to go over to a previously visited directory. // Must disallow going back and forth in a cycle. - l1, err = l2.New("../subdir1") + _, err = l2.New("../subdir1") require.Error(err) } @@ -319,23 +319,24 @@ const ( // └── exteriorData func commonSetupForLoaderRestrictionTest(t *testing.T) (string, filesys.FileSystem) { t.Helper() + require := require.New(t) fSys, tmpDir := setupOnDisk(t) dir := tmpDir.String() - fSys.Mkdir(filepath.Join(dir, "base")) + require.NoError(fSys.Mkdir(filepath.Join(dir, "base"))) - fSys.WriteFile( - filepath.Join(dir, "base", "okayData"), []byte(contentOk)) + require.NoError(fSys.WriteFile( + filepath.Join(dir, "base", "okayData"), []byte(contentOk))) - fSys.WriteFile( - filepath.Join(dir, "exteriorData"), []byte(contentExteriorData)) + require.NoError(fSys.WriteFile( + filepath.Join(dir, "exteriorData"), []byte(contentExteriorData))) - os.Symlink( + require.NoError(os.Symlink( filepath.Join(dir, "base", "okayData"), - filepath.Join(dir, "base", "symLinkToOkayData")) - os.Symlink( + filepath.Join(dir, "base", "symLinkToOkayData"))) + require.NoError(os.Symlink( filepath.Join(dir, "exteriorData"), - filepath.Join(dir, "base", "symLinkToExteriorData")) + filepath.Join(dir, "base", "symLinkToExteriorData"))) return dir, fSys } @@ -445,14 +446,14 @@ func TestNewLoaderAtGitClone(t *testing.T) { url := rootURL + "/" + pathInRepo coRoot := "/tmp" fSys := filesys.MakeFsInMemory() - fSys.MkdirAll(coRoot) - fSys.MkdirAll(coRoot + "/" + pathInRepo) - fSys.WriteFile( + require.NoError(fSys.MkdirAll(coRoot)) + require.NoError(fSys.MkdirAll(coRoot + "/" + pathInRepo)) + require.NoError(fSys.WriteFile( coRoot+"/"+pathInRepo+"/"+ konfig.DefaultKustomizationFileName(), []byte(` whatever -`)) +`))) repoSpec, err := git.NewRepoSpecFromURL(url) require.NoError(err) @@ -472,7 +473,7 @@ whatever require.Error(err) pathInRepo = "foo/overlay" - fSys.MkdirAll(coRoot + "/" + pathInRepo) + require.NoError(fSys.MkdirAll(coRoot + "/" + pathInRepo)) url = rootURL + "/" + pathInRepo l2, err := l.New(url) require.NoError(err) @@ -489,9 +490,9 @@ func TestLoaderDisallowsLocalBaseFromRemoteOverlay(t *testing.T) { topDir := "/whatever" cloneRoot := topDir + "/someClone" fSys := filesys.MakeFsInMemory() - fSys.MkdirAll(topDir + "/highBase") - fSys.MkdirAll(cloneRoot + "/foo/base") - fSys.MkdirAll(cloneRoot + "/foo/overlay") + require.NoError(fSys.MkdirAll(topDir + "/highBase")) + require.NoError(fSys.MkdirAll(cloneRoot + "/foo/base")) + require.NoError(fSys.MkdirAll(cloneRoot + "/foo/overlay")) var l1 ifc.Loader @@ -502,7 +503,7 @@ func TestLoaderDisallowsLocalBaseFromRemoteOverlay(t *testing.T) { require.Equal(cloneRoot+"/foo/overlay", l1.Root()) l2, err := l1.New("../base") - require.NoError(nil) + require.NoError(err) require.Equal(cloneRoot+"/foo/base", l2.Root()) l3, err := l2.New("../../../highBase") @@ -567,8 +568,8 @@ func TestLocalLoaderReferencingGitBase(t *testing.T) { topDir := "/whatever" cloneRoot := topDir + "/someClone" fSys := filesys.MakeFsInMemory() - fSys.MkdirAll(topDir) - fSys.MkdirAll(cloneRoot + "/foo/base") + require.NoError(fSys.MkdirAll(topDir)) + require.NoError(fSys.MkdirAll(cloneRoot + "/foo/base")) l1 := newLoaderAtConfirmedDir( RestrictionRootOnly, filesys.ConfirmedDir(topDir), fSys, nil, @@ -588,8 +589,8 @@ func TestRepoDirectCycleDetection(t *testing.T) { topDir := "/cycles" cloneRoot := topDir + "/someClone" fSys := filesys.MakeFsInMemory() - fSys.MkdirAll(topDir) - fSys.MkdirAll(cloneRoot) + require.NoError(fSys.MkdirAll(topDir)) + require.NoError(fSys.MkdirAll(cloneRoot)) l1 := newLoaderAtConfirmedDir( RestrictionRootOnly, filesys.ConfirmedDir(topDir), fSys, nil, @@ -610,8 +611,8 @@ func TestRepoIndirectCycleDetection(t *testing.T) { topDir := "/cycles" cloneRoot := topDir + "/someClone" fSys := filesys.MakeFsInMemory() - fSys.MkdirAll(topDir) - fSys.MkdirAll(cloneRoot) + require.NoError(fSys.MkdirAll(topDir)) + require.NoError(fSys.MkdirAll(cloneRoot)) l0 := newLoaderAtConfirmedDir( RestrictionRootOnly, filesys.ConfirmedDir(topDir), fSys, nil, @@ -683,7 +684,7 @@ func TestLoaderHTTP(t *testing.T) { u := req.URL.String() require.Equal(x.path, u) return &http.Response{ - StatusCode: 200, + StatusCode: http.StatusOK, Body: io.NopCloser(bytes.NewBufferString(x.expectedContent)), Header: make(http.Header), }