fs: Adds tests to prove we allow non-compliant FS implementations

Signed-off-by: Adrian Cole <adrian@tetrate.io>

internal/syscallfs/adapter.go

@@ -8,7 +8,13 @@ import (
 	"syscall"
 )
 
-// Adapt returns a read-only FS unless the input is already one.
+// Adapt adapts the input to FS unless it is already one. NewDirFS should be
+// used instead, if the input is os.DirFS.
+//
+// Note: This performs no flag verification on FS.OpenFile. fs.FS cannot read
+// flags as there is no parameter to pass them through with. Moreover, fs.FS
+// documentation does not require the file to be present. In summary, we can't
+// enforce flag behavior.
 func Adapt(fs fs.FS) FS {
 	if sys, ok := fs.(FS); ok {
 		return sys
@@ -32,17 +38,16 @@ func (ro *adapter) Path() string {
 
 // OpenFile implements FS.OpenFile
 func (ro *adapter) OpenFile(path string, flag int, perm fs.FileMode) (fs.File, error) {
-	if flag != 0 && flag != os.O_RDONLY {
-		return nil, syscall.ENOSYS
-	}
-
 	path = cleanPath(path)
 	f, err := ro.fs.Open(path)
+
 	if err != nil {
-		// wrapped is fine while FS.OpenFile emulates os.OpenFile vs syscall.OpenFile.
 		return nil, err
+	} else if osF, ok := f.(*os.File); ok {
+		// If this is an OS file, it has same portability issues as dirFS.
+		return maybeWrapFile(osF), nil
 	}
-	return maskForReads(f), nil
+	return f, nil
 }
 
 func cleanPath(name string) string {

internal/syscallfs/adapter_test.go

@@ -12,26 +12,24 @@ import (
 )
 
 func TestAdapt_MkDir(t *testing.T) {
-	dir := t.TempDir()
-
-	testFS := Adapt(os.DirFS(dir))
+	testFS := Adapt(os.DirFS(t.TempDir()))
 
 	err := testFS.Mkdir("mkdir", fs.ModeDir)
 	require.Equal(t, syscall.ENOSYS, err)
 }
 
 func TestAdapt_Rename(t *testing.T) {
-	tmpDir := t.TempDir()
-	testFS := Adapt(os.DirFS(tmpDir))
+	rootDir := t.TempDir()
+	testFS := Adapt(os.DirFS(rootDir))
 
 	file1 := "file1"
-	file1Path := pathutil.Join(tmpDir, file1)
+	file1Path := pathutil.Join(rootDir, file1)
 	file1Contents := []byte{1}
 	err := os.WriteFile(file1Path, file1Contents, 0o600)
 	require.NoError(t, err)
 
 	file2 := "file2"
-	file2Path := pathutil.Join(tmpDir, file2)
+	file2Path := pathutil.Join(rootDir, file2)
 	file2Contents := []byte{2}
 	err = os.WriteFile(file2Path, file2Contents, 0o600)
 	require.NoError(t, err)
@@ -41,54 +39,49 @@ func TestAdapt_Rename(t *testing.T) {
 }
 
 func TestAdapt_Rmdir(t *testing.T) {
-	dir := t.TempDir()
-
-	testFS := Adapt(os.DirFS(dir))
+	rootDir := t.TempDir()
+	testFS := Adapt(os.DirFS(rootDir))
 
 	path := "rmdir"
-	realPath := pathutil.Join(dir, path)
+	realPath := pathutil.Join(rootDir, path)
 	require.NoError(t, os.Mkdir(realPath, 0o700))
 
 	err := testFS.Rmdir(path)
 	require.Equal(t, syscall.ENOSYS, err)
 }
 
 func TestAdapt_Unlink(t *testing.T) {
-	dir := t.TempDir()
-
-	testFS := Adapt(os.DirFS(dir))
+	rootDir := t.TempDir()
+	testFS := Adapt(os.DirFS(rootDir))
 
 	path := "unlink"
-	realPath := pathutil.Join(dir, path)
+	realPath := pathutil.Join(rootDir, path)
 	require.NoError(t, os.WriteFile(realPath, []byte{}, 0o600))
 
 	err := testFS.Unlink(path)
 	require.Equal(t, syscall.ENOSYS, err)
 }
 
 func TestAdapt_Utimes(t *testing.T) {
-	dir := t.TempDir()
-
-	testFS := Adapt(os.DirFS(dir))
+	rootDir := t.TempDir()
+	testFS := Adapt(os.DirFS(rootDir))
 
 	path := "utimes"
-	realPath := pathutil.Join(dir, path)
+	realPath := pathutil.Join(rootDir, path)
 	require.NoError(t, os.WriteFile(realPath, []byte{}, 0o600))
 
 	err := testFS.Utimes(path, 1, 1)
 	require.Equal(t, syscall.ENOSYS, err)
 }
 
 func TestAdapt_Open_Read(t *testing.T) {
-	tmpDir := t.TempDir()
-
 	// Create a subdirectory, so we can test reads outside the FS root.
-	tmpDir = pathutil.Join(tmpDir, t.Name())
-	require.NoError(t, os.Mkdir(tmpDir, 0o700))
-
-	testFS := Adapt(os.DirFS(tmpDir))
+	rootDir := t.TempDir()
+	rootDir = pathutil.Join(rootDir, t.Name())
+	require.NoError(t, os.Mkdir(rootDir, 0o700))
+	testFS := Adapt(os.DirFS(rootDir))
 
-	testFS_Open_Read(t, tmpDir, testFS)
+	testOpen_Read(t, rootDir, testFS)
 
 	t.Run("path outside root invalid", func(t *testing.T) {
 		_, err := testFS.OpenFile("../foo", os.O_RDONLY, 0)
@@ -97,3 +90,30 @@ func TestAdapt_Open_Read(t *testing.T) {
 		require.True(t, errors.Is(err, fs.ErrInvalid))
 	})
 }
+
+// hackFS cheats the fs.FS contract by opening for write (os.O_RDWR).
+//
+// Until we have an alternate public interface for filesystems, some users will
+// rely on this. Via testing, we ensure we don't accidentally break them.
+type hackFS string
+
+func (dir hackFS) Open(name string) (fs.File, error) {
+	path := ensureTrailingPathSeparator(string(dir)) + name
+
+	if f, err := os.OpenFile(path, os.O_RDWR, 0); err == nil {
+		return f, nil
+	} else if errors.Is(err, syscall.EISDIR) {
+		return os.OpenFile(path, os.O_RDONLY, 0)
+	} else {
+		return nil, err
+	}
+}
+
+// TestAdapt_HackedWrites ensures we allow writes even if they violate the
+// fs.FS contract.
+func TestAdapt_HackedWrites(t *testing.T) {
+	rootDir := t.TempDir()
+	testFS := Adapt(hackFS(rootDir))
+
+	testOpen_O_RDWR(t, rootDir, testFS)
+}

internal/syscallfs/dirfs.go

@@ -4,11 +4,15 @@ import (
 	"fmt"
 	"io/fs"
 	"os"
-	"path"
 	"syscall"
 )
 
 func NewDirFS(dir string) (FS, error) {
+	if dir == "" {
+		panic("empty dir")
+	}
+	// For easier OS-specific concatenation later, append the path separator.
+	dir = ensureTrailingPathSeparator(dir)
 	if stat, err := os.Stat(dir); err != nil {
 		return nil, syscall.ENOENT
 	} else if !stat.IsDir() {
@@ -17,6 +21,13 @@ func NewDirFS(dir string) (FS, error) {
 	return dirFS(dir), nil
 }
 
+func ensureTrailingPathSeparator(dir string) string {
+	if dir[len(dir)-1] != os.PathSeparator {
+		return dir + string(os.PathSeparator)
+	}
+	return dir
+}
+
 // dirFS currently validates each path, which means that input paths cannot
 // escape the directory, except via symlink. We may want to relax this in the
 // future, especially as we decoupled from fs.FS which has this requirement.
@@ -34,20 +45,16 @@ func (dir dirFS) Path() string {
 
 // OpenFile implements FS.OpenFile
 func (dir dirFS) OpenFile(name string, flag int, perm fs.FileMode) (fs.File, error) {
-	f, err := os.OpenFile(path.Join(string(dir), name), flag, perm)
+	f, err := os.OpenFile(dir.join(name), flag, perm)
 	if err != nil {
 		return nil, err
 	}
-
-	if flag == 0 || flag == os.O_RDONLY {
-		return maskForReads(f), nil
-	}
-	return f, nil
+	return maybeWrapFile(f), nil
 }
 
 // Mkdir implements FS.Mkdir
 func (dir dirFS) Mkdir(name string, perm fs.FileMode) error {
-	err := os.Mkdir(path.Join(string(dir), name), perm)
+	err := os.Mkdir(dir.join(name), perm)
 	return adjustMkdirError(err)
 }
 
@@ -56,25 +63,31 @@ func (dir dirFS) Rename(from, to string) error {
 	if from == to {
 		return nil
 	}
-	return rename(path.Join(string(dir), from), path.Join(string(dir), to))
+	return rename(dir.join(from), dir.join(to))
 }
 
 // Rmdir implements FS.Rmdir
 func (dir dirFS) Rmdir(name string) error {
-	err := syscall.Rmdir(path.Join(string(dir), name))
+	err := syscall.Rmdir(dir.join(name))
 	return adjustRmdirError(err)
 }
 
 // Unlink implements FS.Unlink
 func (dir dirFS) Unlink(name string) error {
-	err := syscall.Unlink(path.Join(string(dir), name))
+	err := syscall.Unlink(dir.join(name))
 	return adjustUnlinkError(err)
 }
 
 // Utimes implements FS.Utimes
 func (dir dirFS) Utimes(name string, atimeNsec, mtimeNsec int64) error {
-	return syscall.UtimesNano(path.Join(string(dir), name), []syscall.Timespec{
+	return syscall.UtimesNano(dir.join(name), []syscall.Timespec{
 		syscall.NsecToTimespec(atimeNsec),
 		syscall.NsecToTimespec(mtimeNsec),
 	})
 }
+
+func (dir dirFS) join(name string) string {
+	// TODO: Enforce similar to safefilepath.FromFS(name), but be careful as
+	// relative path inputs are allowed. e.g. dir or name == ../
+	return string(dir) + name
+}