Skip to content
Merged
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
15 changes: 8 additions & 7 deletions internal/ufs/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,14 @@ type Filesystem interface {
Mkdir(name string, perm FileMode) error

// MkdirAll creates a directory named path, along with any necessary
// parents, and returns nil, or else returns an error.
//
// The permission bits perm (before umask) are used for all
// directories that MkdirAll creates.
// If path is already a directory, MkdirAll does nothing
// and returns nil.
MkdirAll(path string, perm FileMode) error
// parents, and returns the directories it created, or else returns an
// error.
//
// The returned directories are ordered from shallowest to deepest. The
// permission bits perm (before umask) are used for all directories that
// MkdirAll creates. If path is already a directory, MkdirAll does nothing
// and returns no created directories.
MkdirAll(path string, perm FileMode) ([]string, error)

// Open opens the named file for reading.
//
Expand Down
18 changes: 9 additions & 9 deletions internal/ufs/fs_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,17 +207,17 @@ func (fs *UnixFS) mkdirat(op string, dirfd int, name string, mode FileMode) erro
}

// MkdirAll creates a directory named path, along with any necessary
// parents, and returns nil, or else returns an error.
// parents, and returns the directories it created, or else returns an error.
//
// The permission bits perm (before umask) are used for all
// directories that MkdirAll creates.
// If path is already a directory, MkdirAll does nothing
// and returns nil.
func (fs *UnixFS) MkdirAll(name string, mode FileMode) error {
// The returned directories are ordered from shallowest to deepest. The
// permission bits perm (before umask) are used for all directories that
// MkdirAll creates. If path is already a directory, MkdirAll does nothing and
// returns no created directories
func (fs *UnixFS) MkdirAll(name string, mode FileMode) ([]string, error) {
// Ensure name is somewhat clean before continuing.
name, err := fs.unsafePath(name)
if err != nil {
return err
return nil, err
}
return fs.mkdirAll(name, mode)
}
Expand Down Expand Up @@ -471,7 +471,7 @@ func (fs *UnixFS) Rename(oldpath, newpath string) error {
if !errors.As(err, &pathErr) {
return err
}
if err := fs.MkdirAll(pathErr.Path, 0o755); err != nil {
if _, err := fs.MkdirAll(pathErr.Path, 0o755); err != nil {
return err
}
newdirfd, newname, closeFd2, err = fs.safePath(newpath)
Expand Down Expand Up @@ -623,7 +623,7 @@ func (fs *UnixFS) TouchPath(path string) (int, string, func(), error, bool) {
if !errors.As(err, &pathErr) {
return dirfd, name, closeFd, err, false
}
if err := fs.MkdirAll(pathErr.Path, 0o755); err != nil {
if _, err := fs.MkdirAll(pathErr.Path, 0o755); err != nil {
return dirfd, name, closeFd, err, false
}

Expand Down
60 changes: 53 additions & 7 deletions internal/ufs/fs_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ func TestUnixFS(t *testing.T) {
}

// Create multiple nested directories.
if err := fs.MkdirAll("ima_directory/ima_directory/ima_directory/ima_directory", 0o755); err != nil {
if _, err := fs.MkdirAll("ima_directory/ima_directory/ima_directory/ima_directory", 0o755); err != nil {
t.Error(err)
return
}
Expand All @@ -174,7 +174,7 @@ func TestUnixFS(t *testing.T) {
}

// Test creating a directory under a symlink with a pre-existing directory.
if err := fs.MkdirAll("ima_bad_link/ima_directory/ima_bad_directory/ima_bad_directory", 0o755); err == nil {
if _, err := fs.MkdirAll("ima_bad_link/ima_directory/ima_bad_directory/ima_bad_directory", 0o755); err == nil {
t.Error("expected an error")
return
}
Expand Down Expand Up @@ -324,12 +324,58 @@ func TestUnixFS_MkdirAll(t *testing.T) {
}
defer fs.Cleanup()

if err := fs.MkdirAll("/a/bunch/of/directories", 0o755); err != nil {
t.Error(err)
return
}
t.Run("creates and reports every missing directory", func(t *testing.T) {
created, err := fs.MkdirAll("/a/bunch/of/directories", 0o755)
if err != nil {
t.Fatal(err)
}

want := []string{"a", "a/bunch", "a/bunch/of", "a/bunch/of/directories"}
if !slices.Equal(created, want) {
t.Errorf("created = %v, want %v", created, want)
}
// Sanity check that everything we reported actually exists on disk.
for _, dir := range want {
st, err := os.Lstat(filepath.Join(fs.Root, dir))
if err != nil {
t.Errorf("Lstat %q: %v", dir, err)
continue
}
if !st.IsDir() {
t.Errorf("%q is not a directory", dir)
}
}
})

t.Run("only reports the directories it creates", func(t *testing.T) {
if _, err := fs.MkdirAll("partial/exists", 0o755); err != nil {
t.Fatalf("seeding directories: %v", err)
}

created, err := fs.MkdirAll("partial/exists/and/more", 0o755)
if err != nil {
t.Fatal(err)
}

want := []string{"partial/exists/and", "partial/exists/and/more"}
if !slices.Equal(created, want) {
t.Errorf("created = %v, want %v", created, want)
}
})

// TODO: stat sanity check
t.Run("reports nothing when the directory already exists", func(t *testing.T) {
if _, err := fs.MkdirAll("already/here", 0o755); err != nil {
t.Fatalf("seeding directories: %v", err)
}

created, err := fs.MkdirAll("already/here", 0o755)
if err != nil {
t.Fatal(err)
}
if len(created) != 0 {
t.Errorf("created = %v, want no directories", created)
}
})
}

func TestUnixFS_Open(t *testing.T) {
Expand Down
23 changes: 14 additions & 9 deletions internal/ufs/mkdir_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@
package ufs

// mkdirAll is a recursive Mkdir implementation that properly handles symlinks.
func (fs *UnixFS) mkdirAll(name string, mode FileMode) error {
//
// It returns the directories it created, ordered from shallowest to deepest, so
// callers can act on exactly the paths that were new (for example to change
// their ownership). Directories that already existed are not included.
func (fs *UnixFS) mkdirAll(name string, mode FileMode) ([]string, error) {
// Fast path: if we can tell whether path is a directory or file, stop with success or error.
dir, err := fs.Lstat(name)
if err == nil {
Expand All @@ -20,13 +24,13 @@ func (fs *UnixFS) mkdirAll(name string, mode FileMode) error {
// to check instead.
dir, err = fs.Stat(name)
if err != nil {
return err
return nil, err
}
}
if dir.IsDir() {
return nil
return nil, nil
}
return &PathError{Op: "mkdir", Path: name, Err: ErrNotDirectory}
return nil, &PathError{Op: "mkdir", Path: name, Err: ErrNotDirectory}
}

// Slow path: make sure parent exists and then call Mkdir for path.
Expand All @@ -40,11 +44,12 @@ func (fs *UnixFS) mkdirAll(name string, mode FileMode) error {
j--
}

var created []string
if j > 1 {
// Create parent.
err = fs.mkdirAll(name[:j-1], mode)
created, err = fs.mkdirAll(name[:j-1], mode)
if err != nil {
return err
return created, err
}
}

Expand All @@ -55,9 +60,9 @@ func (fs *UnixFS) mkdirAll(name string, mode FileMode) error {
// double-checking that directory doesn't exist.
dir, err1 := fs.Lstat(name)
if err1 == nil && dir.IsDir() {
return nil
return created, nil
}
return err
return created, err
}
return nil
return append(created, name), nil
}
13 changes: 9 additions & 4 deletions server/filesystem/compress.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,14 +369,19 @@ func (fs *Filesystem) extractStream(ctx context.Context, opts extractStreamOptio

// Decompress and extract archive
return ex.Extract(ctx, opts.Reader, func(ctx context.Context, f archives.FileInfo) error {
if f.IsDir() {
return nil
}
p := filepath.Join(opts.Directory, f.NameInArchive)
// If it is ignored, just don't do anything with the file and skip over it.
// If it is ignored, just don't do anything with the entry and skip over it.
if err := fs.IsIgnored(p); err != nil {
return nil
}
// Create directories explicitly; an empty one has no file to create it
// implicitly and would otherwise be dropped during extraction.
if f.IsDir() {
if err := fs.mkdirAll(p, 0o755); err != nil {
return wrapError(err, opts.FileName)
}
return nil
}
r, err := f.Open()
if err != nil {
return err
Expand Down
27 changes: 26 additions & 1 deletion server/filesystem/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,14 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM
return err
}

// Ensure the parent directories exist and are owned by the server user
// before creating the file. Touch would create any missing parents
// implicitly, but as the user Wings runs as; creating them here lets us
// chown the ones we add.
if err := fs.mkdirAll(filepath.Dir(p), 0o755); err != nil {
return err
}

// Touch the file and return the handle to it at this point. This will
// create or truncate the file, and create any necessary parent directories
// if they are missing.
Expand All @@ -185,14 +193,15 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM
if err := fs.chownFile(p); err != nil {
return err
}

// Return any remaining error.
return err
}

// CreateDirectory creates a new directory (name) at a specified path (p) for
// the server.
func (fs *Filesystem) CreateDirectory(name string, p string) error {
return fs.unixFS.MkdirAll(filepath.Join(p, name), 0o755)
return fs.mkdirAll(filepath.Join(p, name), 0o755)
}

func (fs *Filesystem) Rename(oldpath, newpath string) error {
Expand All @@ -213,6 +222,22 @@ func (fs *Filesystem) chownFile(name string) error {
return fs.unixFS.Lchown(name, uid, gid)
}

// mkdirAll creates the directory p along with any missing parents, chowning
// every directory it creates to the server user so they are not left owned by
// the user Wings runs as.
func (fs *Filesystem) mkdirAll(p string, mode ufs.FileMode) error {
created, err := fs.unixFS.MkdirAll(p, mode)
if err != nil {
return err
}
Comment on lines +229 to +232

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Chown partially created directories before returning MkdirAll errors.

UnixFS.MkdirAll returns the partial created list when a later mkdir fails, but this wrapper returns on err before chowning those paths. A failed or raced nested create can therefore leave newly created parents owned by the process user, making retries hit the same ownership problem.

Proposed fix
 func (fs *Filesystem) mkdirAll(p string, mode ufs.FileMode) error {
-	created, err := fs.unixFS.MkdirAll(p, mode)
-	if err != nil {
-		return err
-	}
+	created, mkdirErr := fs.unixFS.MkdirAll(p, mode)
 	for _, dir := range created {
 		if err := fs.chownFile(dir); err != nil {
+			if mkdirErr != nil {
+				return fmt.Errorf("error creating directories: %w; additionally failed to chown %q: %v", mkdirErr, dir, err)
+			}
 			return err
 		}
 	}
-	return nil
+	return mkdirErr
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
created, err := fs.unixFS.MkdirAll(p, mode)
if err != nil {
return err
}
func (fs *Filesystem) mkdirAll(p string, mode ufs.FileMode) error {
created, mkdirErr := fs.unixFS.MkdirAll(p, mode)
for _, dir := range created {
if err := fs.chownFile(dir); err != nil {
if mkdirErr != nil {
return fmt.Errorf("error creating directories: %w; additionally failed to chown %q: %v", mkdirErr, dir, err)
}
return err
}
}
return mkdirErr
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/filesystem/filesystem.go` around lines 229 - 232, The MkdirAll
function returns a partial list of created directories when an error occurs, but
the current code returns immediately on error before chowning those partially
created paths. To fix this, capture the created directories list from the
unixFS.MkdirAll call, and ensure all directories in that created list are
chowned before returning any error. This way, even if MkdirAll fails partway
through, the directories it did create will have the correct ownership,
preventing ownership-related failures on retries.

for _, dir := range created {
if err := fs.chownFile(dir); err != nil {
return err
}
}
return nil
}

// Chown recursively iterates over a file or directory and sets the permissions on all of the
// underlying files. Iterate over all of the files and directories. If it is a file just
// go ahead and perform the chown operation. Otherwise dig deeper into the directory until
Expand Down
Loading