From 7a3f070e2169e04f41c963087ff68ba4a24ebefa Mon Sep 17 00:00:00 2001 From: robert dennis <31261583+robertdrakedennis@users.noreply.github.com> Date: Sat, 30 May 2026 00:19:23 -0400 Subject: [PATCH 01/11] empty folders in uploaded artifacts should be preserved --- server/filesystem/compress.go | 13 ++-- server/filesystem/compress_test.go | 97 ++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/server/filesystem/compress.go b/server/filesystem/compress.go index f2775cb31..0044365b3 100644 --- a/server/filesystem/compress.go +++ b/server/filesystem/compress.go @@ -258,14 +258,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.unixFS.MkdirAll(p, 0o755); err != nil { + return wrapError(err, opts.FileName) + } + return nil + } r, err := f.Open() if err != nil { return err diff --git a/server/filesystem/compress_test.go b/server/filesystem/compress_test.go index 80cf70800..202cc678d 100644 --- a/server/filesystem/compress_test.go +++ b/server/filesystem/compress_test.go @@ -1,6 +1,10 @@ package filesystem import ( + "archive/tar" + "archive/zip" + "bytes" + "compress/gzip" "context" "os" "testing" @@ -52,3 +56,96 @@ func TestFilesystem_DecompressFile(t *testing.T) { }) }) } + +// Empty directories have no file to create them implicitly, so extraction must +// create them explicitly or they are dropped. +func TestFilesystem_DecompressFileEmptyDirectory(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Decompress", func() { + archives := []struct { + name string + build func() ([]byte, error) + }{ + {"empty.zip", zipWithEmptyDir}, + {"empty.tar.gz", tarGzWithEmptyDir}, + } + + for _, a := range archives { + g.It("preserves an empty directory in a "+a.name, func() { + content, err := a.build() + g.Assert(err).IsNil() + err = rfs.CreateServerFile("./"+a.name, content) + g.Assert(err).IsNil() + + err = fs.DecompressFile(context.Background(), "/", a.name) + g.Assert(err).IsNil() + + // The empty directory must exist, and the sibling file must still extract. + st, err := rfs.StatServerFile("empty") + g.Assert(err).IsNil() + g.Assert(st.IsDir()).IsTrue() + + _, err = rfs.StatServerFile("outside.txt") + g.Assert(err).IsNil() + }) + } + + g.AfterEach(func() { + _ = fs.TruncateRootDirectory() + }) + }) +} + +// zipWithEmptyDir builds a zip holding one file and an empty directory ("empty/"). +func zipWithEmptyDir() ([]byte, error) { + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + dh := &zip.FileHeader{Name: "empty/"} + dh.SetMode(os.ModeDir | 0o755) + if _, err := zw.CreateHeader(dh); err != nil { + return nil, err + } + + w, err := zw.Create("outside.txt") + if err != nil { + return nil, err + } + if _, err := w.Write([]byte("hello")); err != nil { + return nil, err + } + + if err := zw.Close(); err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +// tarGzWithEmptyDir builds a tar.gz holding one file and an empty directory ("empty/"). +func tarGzWithEmptyDir() ([]byte, error) { + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + + if err := tw.WriteHeader(&tar.Header{Name: "empty/", Typeflag: tar.TypeDir, Mode: 0o755}); err != nil { + return nil, err + } + + content := []byte("hello") + if err := tw.WriteHeader(&tar.Header{Name: "outside.txt", Typeflag: tar.TypeReg, Mode: 0o644, Size: int64(len(content))}); err != nil { + return nil, err + } + if _, err := tw.Write(content); err != nil { + return nil, err + } + + if err := tw.Close(); err != nil { + return nil, err + } + if err := gw.Close(); err != nil { + return nil, err + } + return buf.Bytes(), nil +} From 8929a7f854ce2f711953a55ffa5bc845f062ee93 Mon Sep 17 00:00:00 2001 From: Robert Dennis <31261583+robertdrakedennis@users.noreply.github.com> Date: Sun, 31 May 2026 15:26:37 -0400 Subject: [PATCH 02/11] Update CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 955a24712..e2d0b6f71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## v1.12.3 +### Fixed +* Support properly restricting configuration in egg templating + ## v1.12.2 ### Fixed * Fixes a bug where `fs.Chmod` would change the symlink target possibly allowing a malicious user to modify files outside their home directory. From 8e49c7c0eda815d3ada171831876a1c14c493026 Mon Sep 17 00:00:00 2001 From: William Venner Date: Tue, 2 Jun 2026 20:04:46 +0100 Subject: [PATCH 03/11] Fix length check when accepting SFTP connections --- sftp/server.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sftp/server.go b/sftp/server.go index 3dbe563da..e2263a4af 100644 --- a/sftp/server.go +++ b/sftp/server.go @@ -143,7 +143,8 @@ func (c *SFTPServer) AcceptInbound(conn net.Conn, config *ssh.ServerConfig) erro // Channels have a type that is dependent on the protocol. For SFTP // this is "subsystem" with a payload that (should) be "sftp". Discard // anything else we receive ("pty", "shell", etc) - _ = req.Reply(req.Type == "subsystem" && string(req.Payload[4:]) == "sftp", nil) + ok := req.Type == "subsystem" && len(req.Payload) >= 4 && string(req.Payload[4:]) == "sftp" + _ = req.Reply(ok, nil) } }(requests) From 1a3c462e89d27c68e5a8820c43960bd987bd5e44 Mon Sep 17 00:00:00 2001 From: robert dennis <31261583+robertdrakedennis@users.noreply.github.com> Date: Wed, 3 Jun 2026 13:57:10 -0400 Subject: [PATCH 04/11] properly close filesystem copy --- server/filesystem/filesystem.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index 42c56f2de..69ccbb405 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -346,6 +346,7 @@ func (fs *Filesystem) Copy(p string) error { if err != nil { return err } + defer dst.Close() // Do not use CopyBuffer here, it is wasteful as the file implements // io.ReaderFrom, which causes it to not use the buffer anyways. From f2e06b9f4020954fd50efebae78c2e7f84194af8 Mon Sep 17 00:00:00 2001 From: robert dennis <31261583+robertdrakedennis@users.noreply.github.com> Date: Wed, 3 Jun 2026 14:06:46 -0400 Subject: [PATCH 05/11] close after compression --- server/filesystem/compress.go | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/server/filesystem/compress.go b/server/filesystem/compress.go index 0044365b3..aa9a470bc 100644 --- a/server/filesystem/compress.go +++ b/server/filesystem/compress.go @@ -51,29 +51,29 @@ func (fs *Filesystem) CompressFiles(dir string, paths []string) (ufs.FileInfo, e return f.Stat() } -func (fs *Filesystem) archiverFileSystem(ctx context.Context, p string) (iofs.FS, error) { +func (fs *Filesystem) archiverFileSystem(ctx context.Context, p string) (iofs.FS, io.Closer, error) { f, err := fs.unixFS.Open(p) if err != nil { - return nil, err + return nil, nil, err } // Do not use defer to close `f`, it will likely be used later. format, _, err := archives.Identify(ctx, filepath.Base(p), f) if err != nil && !errors.Is(err, archives.NoMatch) { _ = f.Close() - return nil, err + return nil, nil, err } // Reset the file reader. if _, err := f.Seek(0, io.SeekStart); err != nil { _ = f.Close() - return nil, err + return nil, nil, err } info, err := f.Stat() if err != nil { _ = f.Close() - return nil, err + return nil, nil, err } if format != nil { @@ -83,15 +83,20 @@ func (fs *Filesystem) archiverFileSystem(ctx context.Context, p string) (iofs.FS // and zip.Reader can open several content files concurrently because of io.ReaderAt requirement // while ArchiveFS can't. // zip.Reader doesn't suffer from issue #330 and #310 according to local test (but they should be fixed anyway) - return zip.NewReader(f, info.Size()) + reader, err := zip.NewReader(f, info.Size()) + if err != nil { + _ = f.Close() + return nil, nil, err + } + return reader, f, nil case archives.Extraction: - return &archives.ArchiveFS{Stream: io.NewSectionReader(f, 0, info.Size()), Format: ff, Context: ctx}, nil + return &archives.ArchiveFS{Stream: io.NewSectionReader(f, 0, info.Size()), Format: ff, Context: ctx}, f, nil case archives.Compression: - return archiverext.FileFS{File: f, Compression: ff}, nil + return archiverext.FileFS{File: f, Compression: ff}, f, nil } } _ = f.Close() - return nil, archives.NoMatch + return nil, nil, archives.NoMatch } // SpaceAvailableForDecompression looks through a given archive and determines @@ -103,13 +108,14 @@ func (fs *Filesystem) SpaceAvailableForDecompression(ctx context.Context, dir st return nil } - fsys, err := fs.archiverFileSystem(ctx, filepath.Join(dir, file)) + fsys, archive, err := fs.archiverFileSystem(ctx, filepath.Join(dir, file)) if err != nil { if errors.Is(err, archives.NoMatch) { return newFilesystemError(ErrCodeUnknownArchive, err) } return err } + defer archive.Close() var size atomic.Int64 return iofs.WalkDir(fsys, ".", func(path string, d iofs.DirEntry, err error) error { From 8ee23b1de486a6c45285195377cb048afeba35a3 Mon Sep 17 00:00:00 2001 From: robert dennis <31261583+robertdrakedennis@users.noreply.github.com> Date: Wed, 3 Jun 2026 14:29:43 -0400 Subject: [PATCH 06/11] close docker response --- environment/docker/api.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/environment/docker/api.go b/environment/docker/api.go index 4bb5b11f2..82080db02 100644 --- a/environment/docker/api.go +++ b/environment/docker/api.go @@ -77,8 +77,10 @@ func (e *Environment) ContainerInspect(ctx context.Context) (types.ContainerJSON if res == nil { return st, errdefs.Unknown(err) } + _ = res.Body.Close() return st, errdefs.FromStatusCode(err, res.StatusCode) } + defer res.Body.Close() body, err := io.ReadAll(res.Body) if err != nil { From fdc31981e83877a12364f8e65f5d1b2cab28c42e Mon Sep 17 00:00:00 2001 From: William Venner Date: Thu, 4 Jun 2026 17:52:21 +0100 Subject: [PATCH 07/11] maxTextScanTokenSize: reasonable 64 MB max buffer size for line scanning --- parser/parser.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/parser/parser.go b/parser/parser.go index d6f51c40d..ebe4e11e4 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -31,6 +31,9 @@ const ( Xml = "xml" ) +// maxTextScanTokenSize bounds how large a single line the "file" parser will buffer. +const maxTextScanTokenSize = 64 * 1024 * 1024 + type ReplaceValue struct { value []byte valueType jsonparser.ValueType @@ -474,6 +477,7 @@ func (f *ConfigurationFile) parseYamlFile(file ufs.File) error { func (f *ConfigurationFile) parseTextFile(file ufs.File) error { b := bytes.NewBuffer(nil) s := bufio.NewScanner(file) + s.Buffer(make([]byte, 0, 64*1024), maxTextScanTokenSize) var replaced bool for s.Scan() { line := s.Bytes() @@ -492,6 +496,9 @@ func (f *ConfigurationFile) parseTextFile(file ufs.File) error { } b.WriteByte('\n') } + if err := s.Err(); err != nil { + return errors.Wrap(err, "parser: failed to scan text file for configuration update") + } if _, err := file.Seek(0, io.SeekStart); err != nil { return err From 3317ce1a6e869fb6851c13a43d075a8b158f9c4c Mon Sep 17 00:00:00 2001 From: William Venner Date: Thu, 4 Jun 2026 18:07:04 +0100 Subject: [PATCH 08/11] `file` config parser: dont create files if they dont exist yet --- server/config_parser.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/config_parser.go b/server/config_parser.go index f7f230232..503893df9 100644 --- a/server/config_parser.go +++ b/server/config_parser.go @@ -3,9 +3,11 @@ package server import ( "runtime" + "emperror.dev/errors" "github.com/gammazero/workerpool" "github.com/pterodactyl/wings/internal/ufs" + "github.com/pterodactyl/wings/parser" ) // UpdateConfigurationFiles updates all the defined configuration files for @@ -20,6 +22,13 @@ func (s *Server) UpdateConfigurationFiles() { f := cf pool.Submit(func() { + if f.Parser == parser.File { + if _, err := s.Filesystem().UnixFS().Stat(f.FileName); errors.Is(err, ufs.ErrNotExist) { + s.Log().WithField("file_name", f.FileName).Debug("skipping text configuration file that does not exist yet") + return + } + } + file, err := s.Filesystem().UnixFS().Touch(f.FileName, ufs.O_RDWR|ufs.O_CREATE, 0o644) if err != nil { s.Log().WithField("file_name", f.FileName).WithField("error", err).Error("failed to open file for configuration") From 5f71f65711b6b9e6f913bec94a7b36d9a5eaae49 Mon Sep 17 00:00:00 2001 From: William Venner Date: Thu, 4 Jun 2026 19:56:13 +0100 Subject: [PATCH 09/11] maxConfigFileSize: reasonable 64 MB limit for config file parsing --- parser/parser.go | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/parser/parser.go b/parser/parser.go index ebe4e11e4..96fd6c9a4 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -34,6 +34,9 @@ const ( // maxTextScanTokenSize bounds how large a single line the "file" parser will buffer. const maxTextScanTokenSize = 64 * 1024 * 1024 +// maxConfigFileSize caps how large a configuration file we'll attempt to parse. +const maxConfigFileSize = 64 * 1024 * 1024 + type ReplaceValue struct { value []byte valueType jsonparser.ValueType @@ -212,6 +215,16 @@ func newTemplatableConfig(c *config.Configuration) templatableConfig { func (f *ConfigurationFile) Parse(file ufs.File) error { // log.WithField("path", path).WithField("parser", f.Parser.String()).Debug("parsing server configuration file") + // Refuse to parse files larger than the cap. Every parser below buffers the + // whole file in memory, and the contents are untrusted server-owned input, so + // this guards the daemon against being OOM'd by an oversized config. The + // server is still free to boot with the file as-is; we just don't rewrite it. + if info, err := file.Stat(); err != nil { + return err + } else if info.Size() > maxConfigFileSize { + return errors.Errorf("parser: refusing to parse configuration file %q: size %d exceeds limit of %d bytes", file.Name(), info.Size(), maxConfigFileSize) + } + if mb, err := json.Marshal(newTemplatableConfig(config.Get())); err != nil { return err } else { @@ -240,7 +253,7 @@ func (f *ConfigurationFile) Parse(file ufs.File) error { // Parses an xml file. func (f *ConfigurationFile) parseXmlFile(file ufs.File) error { doc := etree.NewDocument() - if _, err := doc.ReadFrom(file); err != nil { + if _, err := doc.ReadFrom(io.LimitReader(file, maxConfigFileSize)); err != nil { return err } @@ -319,7 +332,7 @@ func (f *ConfigurationFile) parseXmlFile(file ufs.File) error { // Parses an ini file. func (f *ConfigurationFile) parseIniFile(file ufs.File) error { // Wrap the file in a NopCloser so the ini package doesn't close the file. - cfg, err := ini.Load(io.NopCloser(file)) + cfg, err := ini.Load(io.NopCloser(io.LimitReader(file, maxConfigFileSize))) if err != nil { return err } @@ -399,7 +412,7 @@ func (f *ConfigurationFile) parseIniFile(file ufs.File) error { // value is set regardless in the file. See the commentary in parseYamlFile for more details // about what is happening during this process. func (f *ConfigurationFile) parseJsonFile(file ufs.File) error { - b, err := io.ReadAll(file) + b, err := io.ReadAll(io.LimitReader(file, maxConfigFileSize)) if err != nil { return err } @@ -426,7 +439,7 @@ func (f *ConfigurationFile) parseJsonFile(file ufs.File) error { // Parses a yaml file and updates any matching key/value pairs before persisting // it back to the disk. func (f *ConfigurationFile) parseYamlFile(file ufs.File) error { - b, err := io.ReadAll(file) + b, err := io.ReadAll(io.LimitReader(file, maxConfigFileSize)) if err != nil { return err } @@ -476,7 +489,7 @@ func (f *ConfigurationFile) parseYamlFile(file ufs.File) error { // than this function where possible. func (f *ConfigurationFile) parseTextFile(file ufs.File) error { b := bytes.NewBuffer(nil) - s := bufio.NewScanner(file) + s := bufio.NewScanner(io.LimitReader(file, maxConfigFileSize)) s.Buffer(make([]byte, 0, 64*1024), maxTextScanTokenSize) var replaced bool for s.Scan() { @@ -541,7 +554,7 @@ func (f *ConfigurationFile) parseTextFile(file ufs.File) error { // @see https://github.com/pterodactyl/panel/issues/2308 (original) // @see https://github.com/pterodactyl/panel/issues/3009 ("bug" introduced as result) func (f *ConfigurationFile) parsePropertiesFile(file ufs.File) error { - b, err := io.ReadAll(file) + b, err := io.ReadAll(io.LimitReader(file, maxConfigFileSize)) if err != nil { return err } From 15739cace02895c224cdd25ab3c8d6f146bfd84d Mon Sep 17 00:00:00 2001 From: SkyMulley Date: Sun, 14 Jun 2026 23:53:46 +0100 Subject: [PATCH 10/11] fix: Directories created via panel are owned by root:root (#328) * bugfix: gather pending directories and chown after creation on file write * bugfix: chowning for archive compression --- server/filesystem/compress.go | 2 +- server/filesystem/filesystem.go | 47 ++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/server/filesystem/compress.go b/server/filesystem/compress.go index aa9a470bc..ac8ea9620 100644 --- a/server/filesystem/compress.go +++ b/server/filesystem/compress.go @@ -272,7 +272,7 @@ func (fs *Filesystem) extractStream(ctx context.Context, opts extractStreamOptio // 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.unixFS.MkdirAll(p, 0o755); err != nil { + if err := fs.mkdirAll(p, 0o755); err != nil { return wrapError(err, opts.FileName) } return nil diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index 69ccbb405..16d31b737 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -157,6 +157,10 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM return err } + // Fetch all directories that are due to be created by Touch so + // we can chown them after Touch creates them. + createdDirs, _ := fs.pendingDirs(filepath.Dir(p)) + // 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. @@ -182,6 +186,14 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM if err := fs.chownFile(p); err != nil { return err } + + // Chown any parent directories that Touch created above so they are owned + // by the server user instead of the user Wings runs as. + for _, dir := range createdDirs { + if err := fs.chownFile(dir); err != nil { + return err + } + } // Return any remaining error. return err } @@ -189,7 +201,7 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM // 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 { @@ -210,6 +222,39 @@ func (fs *Filesystem) chownFile(name string) error { return fs.unixFS.Lchown(name, uid, gid) } +// pendingDirs returns the directories along path p that do not yet exist, from +// deepest to shallowest. It is used to discover exactly which directories a +// following MkdirAll or Touch will create, so only those can be chowned without +// re-touching directories that already exist. +func (fs *Filesystem) pendingDirs(p string) ([]string, error) { + var pending []string + for cur := filepath.Clean(p); cur != "." && cur != "/" && cur != ""; cur = filepath.Dir(cur) { + if _, err := fs.unixFS.Lstat(cur); err == nil { + // This directory (and therefore every parent of it) already exists. + break + } else if !errors.Is(err, ufs.ErrNotExist) { + return nil, err + } + pending = append(pending, cur) + } + return pending, nil +} + +// mkdirAll creates the directory p along with any missing parents, then chowns +// every directory it created to the server user. +func (fs *Filesystem) mkdirAll(p string, mode ufs.FileMode) error { + created, _ := fs.pendingDirs(p) + if err := fs.unixFS.MkdirAll(p, mode); err != nil { + return err + } + 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 From 6a7fc51a2361d83cad994f659a0fa6c458a33bd5 Mon Sep 17 00:00:00 2001 From: robert dennis <31261583+robertdrakedennis@users.noreply.github.com> Date: Sun, 14 Jun 2026 18:43:13 -0400 Subject: [PATCH 11/11] make mkdirall hand back the dirs it created so we chown just those old fix restated the tree with pendingdirs to guess what touch would make now ufs mkdirall returns what it actually created so write createdirectory and archive extract all chown new parents through one helper also stops dropping the mkdir errors and fills in the mkdirall test --- internal/ufs/filesystem.go | 15 ++++---- internal/ufs/fs_unix.go | 18 +++++----- internal/ufs/fs_unix_test.go | 61 +++++++++++++++++++++++++++++---- internal/ufs/mkdir_unix.go | 23 ++++++++----- server/filesystem/filesystem.go | 45 +++++++----------------- 5 files changed, 97 insertions(+), 65 deletions(-) diff --git a/internal/ufs/filesystem.go b/internal/ufs/filesystem.go index 3fa168244..e0f3ce87e 100644 --- a/internal/ufs/filesystem.go +++ b/internal/ufs/filesystem.go @@ -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. // diff --git a/internal/ufs/fs_unix.go b/internal/ufs/fs_unix.go index 6f62b4cb9..97224c305 100644 --- a/internal/ufs/fs_unix.go +++ b/internal/ufs/fs_unix.go @@ -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) } @@ -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) @@ -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 } diff --git a/internal/ufs/fs_unix_test.go b/internal/ufs/fs_unix_test.go index e64bb823b..8327092db 100644 --- a/internal/ufs/fs_unix_test.go +++ b/internal/ufs/fs_unix_test.go @@ -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 } @@ -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 } @@ -324,12 +324,59 @@ 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) + } - // TODO: stat sanity check + // 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) + } + }) + + 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) { diff --git a/internal/ufs/mkdir_unix.go b/internal/ufs/mkdir_unix.go index eb2942b3a..79afb2488 100644 --- a/internal/ufs/mkdir_unix.go +++ b/internal/ufs/mkdir_unix.go @@ -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 { @@ -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. @@ -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 } } @@ -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 } diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index 16d31b737..636615e5f 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -157,9 +157,13 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM return err } - // Fetch all directories that are due to be created by Touch so - // we can chown them after Touch creates them. - createdDirs, _ := fs.pendingDirs(filepath.Dir(p)) + // 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 @@ -186,14 +190,6 @@ func (fs *Filesystem) Write(p string, r io.Reader, newSize int64, mode ufs.FileM if err := fs.chownFile(p); err != nil { return err } - - // Chown any parent directories that Touch created above so they are owned - // by the server user instead of the user Wings runs as. - for _, dir := range createdDirs { - if err := fs.chownFile(dir); err != nil { - return err - } - } // Return any remaining error. return err } @@ -222,29 +218,12 @@ func (fs *Filesystem) chownFile(name string) error { return fs.unixFS.Lchown(name, uid, gid) } -// pendingDirs returns the directories along path p that do not yet exist, from -// deepest to shallowest. It is used to discover exactly which directories a -// following MkdirAll or Touch will create, so only those can be chowned without -// re-touching directories that already exist. -func (fs *Filesystem) pendingDirs(p string) ([]string, error) { - var pending []string - for cur := filepath.Clean(p); cur != "." && cur != "/" && cur != ""; cur = filepath.Dir(cur) { - if _, err := fs.unixFS.Lstat(cur); err == nil { - // This directory (and therefore every parent of it) already exists. - break - } else if !errors.Is(err, ufs.ErrNotExist) { - return nil, err - } - pending = append(pending, cur) - } - return pending, nil -} - -// mkdirAll creates the directory p along with any missing parents, then chowns -// every directory it created to the server user. +// 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, _ := fs.pendingDirs(p) - if err := fs.unixFS.MkdirAll(p, mode); err != nil { + created, err := fs.unixFS.MkdirAll(p, mode) + if err != nil { return err } for _, dir := range created {