Reject ambiguous resource paths with inner ".." to prevent silent misresolution (#6087)

* Reject paths with inner '..' in FileLoader.New to prevent silent misresolution

* Refactor hasInnerDotDot to two-phase loop eliminating mutable state

* Narrow check to embedded '..' segments to allow legitimate winding paths

* Fix gofmt alignment and trailing whitespace in new test functions

* Fix pre-existing lint errors in fileloader_test.go

Author: 0xMH

api/internal/loader/fileloader.go

@@ -147,6 +147,26 @@ func newLoaderAtConfirmedDir(
 	}
 }
 
+// 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
+		}
+	}
+	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 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 "+
+				"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())

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)
 }
 
@@ -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")
+}
+
+func TestHasAmbiguousPathSegment(t *testing.T) {
+	cases := map[string]bool{
+		// Safe: ".." only as standalone segments
+		"../base":           false,
+		"../../shared/prod": false,
+		"..":                false,
+		"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, hasAmbiguousPathSegment(path), "hasAmbiguousPathSegment(%q)", path)
+		})
+	}
+}
+
 func TestNewRemoteLoaderDoesNotExist(t *testing.T) {
 	_, err := makeLoader().New("https://example.com/org/repo")
 	require.ErrorContains(t, err, "fetch")
@@ -265,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
 }
 
@@ -391,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)
@@ -418,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)
@@ -435,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
 
@@ -448,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")
@@ -513,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,
@@ -534,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,
@@ -556,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,
@@ -629,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),
 			}