helm: add more test coverage for secureloader

Signed-off-by: Hidde Beydals <hello@hidde.co>

internal/helm/chart/secureloader/directory.go

@@ -26,7 +26,9 @@ package secureloader
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -51,158 +53,204 @@ var (
 // symlinks without including files outside root.
 type SecureDirLoader struct {
 	root    string
-	dir     string
+	path    string
 	maxSize int
 }
 
 // NewSecureDirLoader returns a new SecureDirLoader, configured to the scope of the
 // root and provided dir. Max size configures the maximum size a file must not
-// exceed to be loaded. If 0 it defaults to defaultMaxFileSize, it can be
+// exceed to be loaded. If 0 it defaults to DefaultMaxFileSize, it can be
 // disabled using a negative integer.
-func NewSecureDirLoader(root string, dir string, maxSize int) SecureDirLoader {
+func NewSecureDirLoader(root string, path string, maxSize int) SecureDirLoader {
 	if maxSize == 0 {
 		maxSize = DefaultMaxFileSize
 	}
 	return SecureDirLoader{
 		root:    root,
-		dir:     dir,
+		path:    path,
 		maxSize: maxSize,
 	}
 }
 
 // Load loads and returns the chart.Chart, or an error.
 func (l SecureDirLoader) Load() (*chart.Chart, error) {
-	return SecureLoadDir(l.root, l.dir, l.maxSize)
+	return SecureLoadDir(l.root, l.path, l.maxSize)
 }
 
-// SecureLoadDir securely loads from a directory, without going outside root.
+// SecureLoadDir securely loads a chart from the path relative to root, without
-func SecureLoadDir(root, dir string, maxSize int) (*chart.Chart, error) {
+// traversing outside root. When maxSize >= 0, files are not allowed to exceed
+// this size, or an error is returned.
+func SecureLoadDir(root, path string, maxSize int) (*chart.Chart, error) {
 	root, err := filepath.Abs(root)
 	if err != nil {
 		return nil, err
 	}
 
-	topDir, err := filepath.Abs(dir)
+	// Ensure path is relative
+	if filepath.IsAbs(path) {
+		relChartPath, err := filepath.Rel(root, path)
+		if err != nil {
+			return nil, err
+		}
+		path = relChartPath
+	}
+
+	// Resolve secure absolute path
+	absChartName, err := securejoin.SecureJoin(root, path)
 	if err != nil {
 		return nil, err
 	}
 
-	// Confirm topDir is actually relative to root
+	// Load ignore rules
-	if _, err = isSecureSymlinkPath(root, topDir); err != nil {
+	rules, err := secureLoadIgnoreRules(root, path)
-		return nil, fmt.Errorf("cannot load chart from dir: %w", err)
-	}
-
-	// Just used for errors
-	c := &chart.Chart{}
-
-	// Get the absolute location of the .helmignore file
-	relDirPath, err := filepath.Rel(root, topDir)
 	if err != nil {
-		// We are not expected to be returning this error, as the above call to
+		return nil, fmt.Errorf("cannot load ignore rules for chart: %w", err)
-		// isSecureSymlinkPath already does the same. However, especially
-		// because we are dealing with security aspects here, we check it
-		// anyway in case this assumption changes.
-		return nil, err
 	}
-	iFile, err := securejoin.SecureJoin(root, filepath.Join(relDirPath, ignore.HelmIgnore))
 
-	// Load the .helmignore rules
+	// Lets go for a walk...
-	rules := ignore.Empty()
+	fileWalker := newSecureFileWalker(root, absChartName, maxSize, rules)
-	if _, err = os.Stat(iFile); err == nil {
+	if err = sympath.Walk(fileWalker.absChartPath, fileWalker.walk); err != nil {
-		r, err := ignore.ParseFile(iFile)
+		return nil, fmt.Errorf("failed to load files from %s: %w", strings.TrimPrefix(fileWalker.absChartPath, fileWalker.root), err)
-		if err != nil {
-			return c, err
-		}
-		rules = r
 	}
-	rules.AddDefaults()
 
-	var files []*loader.BufferedFile
+	loaded, err := loader.LoadFiles(fileWalker.files)
-	topDir += string(filepath.Separator)
+	if err != nil {
-
+		return nil, fmt.Errorf("failed to load chart from %s: %w", strings.TrimPrefix(fileWalker.absChartPath, fileWalker.root), err)
-	walk := func(name, absoluteName string, fi os.FileInfo, err error) error {
-		n := strings.TrimPrefix(name, topDir)
-		if n == "" {
-			// No need to process top level. Avoid bug with helmignore .* matching
-			// empty names. See issue 1779.
-			return nil
-		}
-
-		// Normalize to / since it will also work on Windows
-		n = filepath.ToSlash(n)
-
-		if err != nil {
-			return err
-		}
-		if fi.IsDir() {
-			// Directory-based ignore rules should involve skipping the entire
-			// contents of that directory.
-			if rules.Ignore(n, fi) {
-				return filepath.SkipDir
-			}
-			// Check after excluding ignores to provide the user with an option
-			// to opt-out from including certain paths.
-			if _, err := isSecureSymlinkPath(root, absoluteName); err != nil {
-				return fmt.Errorf("cannot load '%s' directory: %w", n, err)
-			}
-			return nil
-		}
-
-		// If a .helmignore file matches, skip this file.
-		if rules.Ignore(n, fi) {
-			return nil
-		}
-
-		// Check after excluding ignores to provide the user with an option
-		// to opt-out from including certain paths.
-		if _, err := isSecureSymlinkPath(root, absoluteName); err != nil {
-			return fmt.Errorf("cannot load '%s' file: %w", n, err)
-		}
-
-		// Irregular files include devices, sockets, and other uses of files that
-		// are not regular files. In Go they have a file mode type bit set.
-		// See https://golang.org/pkg/os/#FileMode for examples.
-		if !fi.Mode().IsRegular() {
-			return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n)
-		}
-
-		if fileSize := fi.Size(); maxSize > 0 && fileSize > int64(maxSize) {
-			return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, maxSize)
-		}
-
-		data, err := os.ReadFile(name)
-		if err != nil {
-			return fmt.Errorf("error reading %s: %w", n, err)
-		}
-		data = bytes.TrimPrefix(data, utf8bom)
-
-		files = append(files, &loader.BufferedFile{Name: n, Data: data})
-		return nil
 	}
-	if err = sympath.Walk(topDir, walk); err != nil {
+	return loaded, nil
-		return c, err
-	}
-	return loader.LoadFiles(files)
 }
 
-// isSecureSymlinkPath attempts to make the given absolute path relative to
+// secureLoadIgnoreRules attempts to load the ignore.HelmIgnore file from the
+// chart path relative to root. If the file is a symbolic link, it is evaluated
+// with the given root treated as root of the filesystem.
+// If the ignore file does not exist, or points to a location outside of root,
+// default ignore.Rules are returned. Any error other than fs.ErrNotExist is
+// returned.
+func secureLoadIgnoreRules(root, chartPath string) (*ignore.Rules, error) {
+	rules := ignore.Empty()
+
+	iFile, err := securejoin.SecureJoin(root, filepath.Join(chartPath, ignore.HelmIgnore))
+	if err != nil {
+		return nil, err
+	}
+	_, err = os.Stat(iFile)
+	if err != nil && !errors.Is(err, fs.ErrNotExist) {
+		return nil, err
+	}
+	if err == nil {
+		if rules, err = ignore.ParseFile(iFile); err != nil {
+			return nil, err
+		}
+	}
+
+	rules.AddDefaults()
+	return rules, nil
+}
+
+// secureFileWalker does the actual walking over the directory, any file loaded
+// by walk is appended to files.
+type secureFileWalker struct {
+	root         string
+	absChartPath string
+	maxSize      int
+	rules        *ignore.Rules
+	files        []*loader.BufferedFile
+}
+
+func newSecureFileWalker(root, absChartPath string, maxSize int, rules *ignore.Rules) *secureFileWalker {
+	absChartPath = filepath.Clean(absChartPath) + string(filepath.Separator)
+	return &secureFileWalker{
+		root:         root,
+		absChartPath: absChartPath,
+		maxSize:      maxSize,
+		rules:        rules,
+		files:        make([]*loader.BufferedFile, 0),
+	}
+}
+
+func (w *secureFileWalker) walk(name, absName string, fi os.FileInfo, err error) error {
+	n := strings.TrimPrefix(name, w.absChartPath)
+	if n == "" {
+		// No need to process top level. Avoid bug with helmignore .* matching
+		// empty names. See issue 1779.
+		return nil
+	}
+
+	if err != nil {
+		return err
+	}
+
+	// Normalize to / since it will also work on Windows
+	n = filepath.ToSlash(n)
+
+	if fi.IsDir() {
+		// Directory-based ignore rules should involve skipping the entire
+		// contents of that directory.
+		if w.rules.Ignore(n, fi) {
+			return filepath.SkipDir
+		}
+		// Check after excluding ignores to provide the user with an option
+		// to opt-out from including certain paths.
+		if _, err := isSecureAbsolutePath(w.root, absName); err != nil {
+			return fmt.Errorf("cannot load '%s' directory: %w", n, err)
+		}
+		return nil
+	}
+
+	// If a .helmignore file matches, skip this file.
+	if w.rules.Ignore(n, fi) {
+		return nil
+	}
+
+	// Check after excluding ignores to provide the user with an option
+	// to opt-out from including certain paths.
+	if _, err := isSecureAbsolutePath(w.root, absName); err != nil {
+		return fmt.Errorf("cannot load '%s' file: %w", n, err)
+	}
+
+	// Irregular files include devices, sockets, and other uses of files that
+	// are not regular files. In Go they have a file mode type bit set.
+	// See https://golang.org/pkg/os/#FileMode for examples.
+	if !fi.Mode().IsRegular() {
+		return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n)
+	}
+
+	// Confirm size it not outside boundaries
+	if fileSize := fi.Size(); w.maxSize > 0 && fileSize > int64(w.maxSize) {
+		return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, w.maxSize)
+	}
+
+	data, err := os.ReadFile(absName)
+	if err != nil {
+		if pathErr := new(fs.PathError); errors.As(err, &pathErr) {
+			err = &fs.PathError{Op: pathErr.Op, Path: strings.TrimPrefix(absName, w.root), Err: pathErr.Err}
+		}
+		return fmt.Errorf("error reading %s: %w", n, err)
+	}
+	data = bytes.TrimPrefix(data, utf8bom)
+
+	w.files = append(w.files, &loader.BufferedFile{Name: n, Data: data})
+	return nil
+}
+
+// isSecureAbsolutePath attempts to make the given absolute path relative to
 // root and securely joins this with root. If the result equals absolute path,
 // it is safe to use.
-func isSecureSymlinkPath(root, absPath string) (bool, error) {
+func isSecureAbsolutePath(root, absPath string) (bool, error) {
 	root, absPath = filepath.Clean(root), filepath.Clean(absPath)
 	if root == "/" {
 		return true, nil
 	}
 	unsafePath, err := filepath.Rel(root, absPath)
 	if err != nil {
-		return false, fmt.Errorf("cannot calculate path relative to root for resolved symlink")
+		return false, fmt.Errorf("cannot calculate path relative to root for absolute path")
 	}
 	safePath, err := securejoin.SecureJoin(root, unsafePath)
 	if err != nil {
-		return false, fmt.Errorf("cannot securely join root with resolved relative symlink path")
+		return false, fmt.Errorf("cannot securely join root with resolved relative path")
 	}
 	if safePath != absPath {
-		return false, fmt.Errorf("symlink traverses outside root boundary: relative path to root %s", unsafePath)
+		return false, fmt.Errorf("absolute path traverses outside root boundary: relative path to root %s", unsafePath)
 	}
 	return true, nil
 }

internal/helm/chart/secureloader/directory_test.go

@@ -17,12 +17,349 @@ limitations under the License.
 package secureloader
 
 import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
+	"testing/fstest"
 
+	"github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore"
 	. "github.com/onsi/gomega"
+	"helm.sh/helm/v3/pkg/chart"
+	"sigs.k8s.io/yaml"
 )
 
-func Test_isSecureSymlinkPath(t *testing.T) {
+func TestSecureDirLoader_Load(t *testing.T) {
+	metadata := chart.Metadata{
+		Name:       "test",
+		APIVersion: "v2",
+		Version:    "1.0",
+		Type:       "application",
+	}
+
+	t.Run("chart", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		m := metadata
+		b, err := yaml.Marshal(&m)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed())
+
+		got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load()
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(got).ToNot(BeNil())
+		g.Expect(got.Name()).To(Equal(m.Name))
+	})
+
+	t.Run("chart with absolute path", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		m := metadata
+		b, err := yaml.Marshal(&m)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed())
+
+		got, err := (NewSecureDirLoader(tmpDir, tmpDir, DefaultMaxFileSize)).Load()
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(got).ToNot(BeNil())
+		g.Expect(got.Name()).To(Equal(m.Name))
+	})
+
+	t.Run("chart with illegal path", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+
+		m := metadata
+		b, err := yaml.Marshal(&m)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed())
+
+		root := filepath.Join(tmpDir, "root")
+		g.Expect(os.Mkdir(root, 0o700)).To(Succeed())
+
+		got, err := (NewSecureDirLoader(root, "../", DefaultMaxFileSize)).Load()
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing"))
+		g.Expect(got).To(BeNil())
+
+		got, err = (NewSecureDirLoader(root, tmpDir, DefaultMaxFileSize)).Load()
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing"))
+		g.Expect(got).To(BeNil())
+	})
+
+	t.Run("chart with .helmignore", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		m := metadata
+		b, err := yaml.Marshal(&m)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "file.txt"), []byte("not included"), 0o644)).To(Succeed())
+
+		got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load()
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(got).ToNot(BeNil())
+		g.Expect(got.Name()).To(Equal(m.Name))
+		g.Expect(got.Raw).To(HaveLen(2))
+	})
+}
+
+func Test_secureLoadIgnoreRules(t *testing.T) {
+	t.Run("defaults", func(t *testing.T) {
+		g := NewWithT(t)
+
+		r, err := secureLoadIgnoreRules("/workdir", "")
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(r.Ignore("file.txt", nil)).To(BeFalse())
+		g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue())
+	})
+
+	t.Run("with "+ignore.HelmIgnore, func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed())
+
+		r, err := secureLoadIgnoreRules(tmpDir, "")
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(r.Ignore("file.txt", nil)).To(BeTrue())
+		g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue())
+		g.Expect(r.Ignore("other.txt", nil)).To(BeFalse())
+	})
+
+	t.Run("with chart path and "+ignore.HelmIgnore, func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		chartPath := "./sub/chart"
+		g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, chartPath, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed())
+
+		r, err := secureLoadIgnoreRules(tmpDir, chartPath)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(r.Ignore("file.txt", nil)).To(BeTrue())
+	})
+
+	t.Run("with relative "+ignore.HelmIgnore+" symlink", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		chartPath := "sub/chart"
+		g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "symlink"), []byte("file.txt"), 0o644)).To(Succeed())
+		g.Expect(os.Symlink("../../symlink", filepath.Join(tmpDir, chartPath, ignore.HelmIgnore)))
+
+		r, err := secureLoadIgnoreRules(tmpDir, chartPath)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(r.Ignore("file.txt", nil)).To(BeTrue())
+	})
+
+	t.Run("with illegal "+ignore.HelmIgnore+" symlink", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		chartPath := "/sub/chart"
+		g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed())
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, "symlink"), []byte("file.txt"), 0o644)).To(Succeed())
+		g.Expect(os.Symlink("../../symlink", filepath.Join(tmpDir, chartPath, ignore.HelmIgnore)))
+
+		r, err := secureLoadIgnoreRules(filepath.Join(tmpDir, chartPath), "")
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue())
+		g.Expect(r.Ignore("file.txt", nil)).To(BeFalse())
+	})
+
+	t.Run("with "+ignore.HelmIgnore+" parsing error", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tmpDir := t.TempDir()
+		g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("**"), 0o644)).To(Succeed())
+
+		_, err := secureLoadIgnoreRules(tmpDir, "")
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("syntax is not supported"))
+	})
+}
+
+func Test_secureFileWalker_walk(t *testing.T) {
+	g := NewWithT(t)
+
+	const (
+		root      = "/fake/root"
+		chartPath = "/fake/root/dir"
+	)
+
+	fakeDirName := "fake-dir"
+	fakeFileName := "fake-file"
+	fakeDeviceFileName := "fake-device"
+	fakeFS := fstest.MapFS{
+		fakeDirName:        &fstest.MapFile{Mode: fs.ModeDir},
+		fakeFileName:       &fstest.MapFile{Data: []byte("a couple bytes")},
+		fakeDeviceFileName: &fstest.MapFile{Mode: fs.ModeDevice},
+	}
+
+	// Safe to further re-use this for other paths
+	fakeDirInfo, err := fakeFS.Stat(fakeDirName)
+	g.Expect(err).ToNot(HaveOccurred())
+	fakeFileInfo, err := fakeFS.Stat(fakeFileName)
+	g.Expect(err).ToNot(HaveOccurred())
+	fakeDeviceInfo, err := fakeFS.Stat(fakeDeviceFileName)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	t.Run("given name equals top dir", func(t *testing.T) {
+		g := NewWithT(t)
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty())
+		g.Expect(w.walk(chartPath+"/", chartPath, nil, nil)).To(BeNil())
+	})
+
+	t.Run("given error is returned", func(t *testing.T) {
+		g := NewWithT(t)
+
+		err := errors.New("error argument")
+		got := (&secureFileWalker{}).walk("name", "/name", nil, err)
+		g.Expect(got).To(HaveOccurred())
+		g.Expect(got).To(Equal(err))
+	})
+
+	t.Run("ignore rule matches dir", func(t *testing.T) {
+		g := NewWithT(t)
+
+		rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/"))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules)
+		g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir))
+	})
+
+	t.Run("absolute path match ignored", func(t *testing.T) {
+		g := NewWithT(t)
+
+		rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/"))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules)
+		g.Expect(w.walk(filepath.Join(w.absChartPath, "symlink"), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil())
+	})
+
+	t.Run("ignore rule not applicable to dir", func(t *testing.T) {
+		g := NewWithT(t)
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty())
+		g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil())
+	})
+
+	t.Run("absolute path outside root", func(t *testing.T) {
+		g := NewWithT(t)
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty())
+		err := w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-dir' directory: absolute path traverses outside root boundary"))
+	})
+
+	t.Run("dir ignore rules before secure path check", func(t *testing.T) {
+		g := NewWithT(t)
+
+		rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/"))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules)
+		g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir))
+	})
+
+	t.Run("ignore rule matches file", func(t *testing.T) {
+		g := NewWithT(t)
+
+		rules, err := ignore.Parse(strings.NewReader(fakeFileName))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules)
+		g.Expect(w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join(w.absChartPath, fakeFileName), fakeFileInfo, nil)).To(BeNil())
+	})
+
+	t.Run("file path outside root", func(t *testing.T) {
+		g := NewWithT(t)
+
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty())
+		err := w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join("/fake/another/root/", fakeFileName), fakeFileInfo, nil)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-file' file: absolute path traverses outside root boundary"))
+	})
+
+	t.Run("irregular file", func(t *testing.T) {
+		w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty())
+		err := w.walk(fakeDeviceFileName, filepath.Join(w.absChartPath), fakeDeviceInfo, nil)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("cannot load irregular file fake-device as it has file mode type bits set"))
+	})
+
+	t.Run("file exceeds max size", func(t *testing.T) {
+		w := newSecureFileWalker(root, chartPath, 5, ignore.Empty())
+		err := w.walk(fakeFileName, filepath.Join(w.absChartPath), fakeFileInfo, nil)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(Equal(fmt.Sprintf("cannot load file fake-file as file size (%d) exceeds limit (%d)", fakeFileInfo.Size(), w.maxSize)))
+	})
+
+	t.Run("file is appended", func(t *testing.T) {
+		g := NewWithT(t)
+		tmpDir := t.TempDir()
+
+		fileName := "append-file"
+		fileData := []byte("append-file-data")
+		absFilePath := filepath.Join(tmpDir, fileName)
+		g.Expect(os.WriteFile(absFilePath, fileData, 0o644)).To(Succeed())
+		fileInfo, err := os.Lstat(absFilePath)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty())
+		g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed())
+		g.Expect(w.files).To(HaveLen(1))
+		g.Expect(w.files[0].Name).To(Equal(fileName))
+		g.Expect(w.files[0].Data).To(Equal(fileData))
+	})
+
+	t.Run("utf8bom is removed from file data", func(t *testing.T) {
+		g := NewWithT(t)
+		tmpDir := t.TempDir()
+
+		fileName := "append-file"
+		fileData := []byte("append-file-data")
+		fileDataWithBom := append(utf8bom, fileData...)
+		absFilePath := filepath.Join(tmpDir, fileName)
+		g.Expect(os.WriteFile(absFilePath, fileDataWithBom, 0o644)).To(Succeed())
+		fileInfo, err := os.Lstat(absFilePath)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty())
+		g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed())
+		g.Expect(w.files).To(HaveLen(1))
+		g.Expect(w.files[0].Name).To(Equal(fileName))
+		g.Expect(w.files[0].Data).To(Equal(fileData))
+	})
+
+	t.Run("file does not exist", func(t *testing.T) {
+		g := NewWithT(t)
+		tmpDir := t.TempDir()
+
+		w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty())
+		err := w.walk(filepath.Join(w.absChartPath, "invalid"), filepath.Join(w.absChartPath, "invalid"), fakeFileInfo, nil)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(errors.Is(err, fs.ErrNotExist)).To(BeTrue())
+		g.Expect(err.Error()).To(ContainSubstring("error reading invalid: open /invalid: no such file or directory"))
+	})
+}
+
+func Test_isSecureAbsolutePath(t *testing.T) {
 	tests := []struct {
 		name    string
 		root    string
@@ -42,7 +379,7 @@ func Test_isSecureSymlinkPath(t *testing.T) {
 			root:    "/working/dir",
 			absPath: "/working/in/another/dir",
 			safe:    false,
-			wantErr: "symlink traverses outside root boundary",
+			wantErr: "absolute path traverses outside root boundary",
 		},
 		{
 			name:    "abs path relative to root",
@@ -55,21 +392,21 @@ func Test_isSecureSymlinkPath(t *testing.T) {
 			root:    "/working/dir",
 			absPath: "/working/dir/../but/not/really",
 			safe:    false,
-			wantErr: "symlink traverses outside root boundary",
+			wantErr: "absolute path traverses outside root boundary",
 		},
 		{
 			name:    "illegal root",
 			root:    "working/dir/",
 			absPath: "/working/dir",
 			safe:    false,
-			wantErr: "cannot calculate path relative to root for resolved symlink",
+			wantErr: "cannot calculate path relative to root for absolute path",
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			got, err := isSecureSymlinkPath(tt.root, tt.absPath)
+			got, err := isSecureAbsolutePath(tt.root, tt.absPath)
 			g.Expect(got).To(Equal(tt.safe))
 			if tt.wantErr != "" {
 				g.Expect(err).To(HaveOccurred())

internal/helm/chart/secureloader/loader.go

@@ -22,6 +22,7 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 
 	securejoin "github.com/cyphar/filepath-securejoin"
 	"helm.sh/helm/v3/pkg/chart"
@@ -34,14 +35,19 @@ import (
 // Name can be an absolute or relative path, but always has to be inside
 // root.
 func Loader(root, name string) (loader.ChartLoader, error) {
-	root, name = filepath.Clean(root), filepath.Clean(name)
+	root, err := filepath.Abs(root)
-	relName := name
+	if err != nil {
+		return nil, err
+	}
+
+	relName := filepath.Clean(name)
 	if filepath.IsAbs(relName) {
 		var err error
 		if relName, err = filepath.Rel(root, name); err != nil {
 			return nil, err
 		}
 	}
+
 	secureName, err := securejoin.SecureJoin(root, relName)
 	if err != nil {
 		return nil, err
@@ -49,12 +55,13 @@ func Loader(root, name string) (loader.ChartLoader, error) {
 	fi, err := os.Lstat(secureName)
 	if err != nil {
 		if pathErr := new(fs.PathError); errors.As(err, &pathErr) {
-			return nil, &fs.PathError{Op: pathErr.Op, Path: name, Err: pathErr.Err}
+			return nil, &fs.PathError{Op: pathErr.Op, Path: strings.TrimPrefix(secureName, root), Err: pathErr.Err}
 		}
 		return nil, err
 	}
+
 	if fi.IsDir() {
-		return NewSecureDirLoader(root, secureName, 0), nil
+		return NewSecureDirLoader(root, relName, DefaultMaxFileSize), nil
 	}
 	return FileLoader(secureName), nil
 }

internal/helm/chart/secureloader/loader_test.go

@@ -23,7 +23,9 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
+	"sigs.k8s.io/yaml"
 )
 
 func TestLoader(t *testing.T) {
@@ -33,22 +35,56 @@ func TestLoader(t *testing.T) {
 	fakeChart := filepath.Join(tmpDir, "fake.tgz")
 	g.Expect(os.WriteFile(fakeChart, []byte(""), 0o644)).To(Succeed())
 
-	got, err := Loader(tmpDir, fakeChart)
+	t.Run("file loader", func(t *testing.T) {
-	g.Expect(err).ToNot(HaveOccurred())
+		g := NewWithT(t)
-	g.Expect(got).To(Equal(loader.FileLoader(fakeChart)))
 
-	fakeChartPath := filepath.Join(tmpDir, "fake")
+		got, err := Loader(tmpDir, fakeChart)
-	g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed())
+		g.Expect(err).ToNot(HaveOccurred())
-	got, err = Loader(tmpDir, "fake")
+		g.Expect(got).To(Equal(loader.FileLoader(fakeChart)))
-	g.Expect(err).ToNot(HaveOccurred())
+	})
-	g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, dir: fakeChartPath, maxSize: DefaultMaxFileSize}))
 
-	symlinkRoot := filepath.Join(tmpDir, "symlink")
+	t.Run("dir loader", func(t *testing.T) {
-	g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed())
+		g := NewWithT(t)
-	symlinkPath := filepath.Join(symlinkRoot, "fake.tgz")
+
-	g.Expect(os.Symlink(fakeChart, symlinkPath))
+		fakeChartPath := filepath.Join(tmpDir, "fake")
-	got, err = Loader(symlinkRoot, symlinkPath)
+		g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed())
-	g.Expect(err).To(HaveOccurred())
+
-	g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{}))
+		got, err := Loader(tmpDir, "fake")
-	g.Expect(got).To(BeNil())
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, path: "fake", maxSize: DefaultMaxFileSize}))
+	})
+
+	t.Run("illegal path", func(t *testing.T) {
+		g := NewWithT(t)
+
+		symlinkRoot := filepath.Join(tmpDir, "symlink")
+		g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed())
+		symlinkPath := filepath.Join(symlinkRoot, "fake.tgz")
+		g.Expect(os.Symlink(fakeChart, symlinkPath))
+
+		got, err := Loader(symlinkRoot, symlinkPath)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{}))
+		g.Expect(got).To(BeNil())
+	})
+}
+
+func TestLoad(t *testing.T) {
+	g := NewWithT(t)
+
+	tmpDir := t.TempDir()
+	metadata := chart.Metadata{
+		Name:       "test",
+		APIVersion: "v2",
+		Version:    "1.0",
+		Type:       "application",
+	}
+	b, err := yaml.Marshal(&metadata)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed())
+
+	got, err := Load(tmpDir, "")
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(got).ToNot(BeNil())
+	g.Expect(got.Name()).To(Equal(metadata.Name))
 }