After discussion on https://github.com/golang/go/issues/45757#issuecomment-1646172720 and #61386, it would help many different use cases to add os.CopyFS that copies an fsys.FS into the local file system, safely. I propose to add that function.
The signature and implementation are roughly:
func CopyFS(dir string, fsys fs.FS) error {
return fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error {
targ := filepath.Join(dir, filepath.FromSlash(path))
if d.IsDir() {
if err := os.MkdirAll(targ, 0777); err != nil {
return err
}
return nil
}
r, err := fsys.Open(path)
if err != nil {
return err
}
defer r.Close()
info, err := r.Stat()
if err != nil {
return err
}
w, err := os.OpenFile(targ, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0666|info.Mode()&0777)
if err != nil {
return err
}
if _, err := io.Copy(w, r); err != nil {
w.Close()
return fmt.Errorf("copying %s: %v", path, err)
}
if err := w.Close(); err != nil {
return err
}
return nil
})
}
with perhaps some extra checks using filepath.IsLocal to guard against bad file names.
Comment From: icholy
Why is the fs.WalkDirFunc error parameter being ignored?
Comment From: earthboundkid
This seems related to #56172. There's a similar question in these about how to handle broken writes and missing directories.
Comment From: earthboundkid
Maybe for this and #56172 there could be a "create directories" argument. Maybe it could be a mode and if the mode is non-zero directories are created. For os.CopyFS, the mode could be &'d with the mode of the source directory.
Maybe there should also be os.CopyFSFile to just copy one fs.File.
Comment From: rsc
This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group
Comment From: CAFxX
In #56172 AFAICT the consensus was that the copy operation should attempt to use FICLONE (or equivalent) whenever possible; this does not currently seem to be captured in this superseding proposal though.
Comment From: rsc
There are questions about whether to create directories, modes, and so on. If we go down that road, it should be a third-party package (lots of them) so that everyone can use the combination they want. The basic os.CopyFS should be the simple one that's good enough the other 99% of the time.
In theory os.CopyFS could detect an os.DirFS source and use its own special-case code to take advantage of FICLONE if that was an important optimization.
Comment From: rsc
Have all remaining concerns about this proposal been addressed?
Comment From: earthboundkid
The sample code has os.MkdirAll(targ, 0777). Should that use info, err := d.Info() and os.MkdirAll(targ, info.Mode()) instead?
Comment From: rsc
I think it should use 0777, the same way that os.OpenFile uses 0666. Let the umask set the permissions, same as if we were using the shell's mkdir and 'cat >file'. The only bits being copied out are the execute bits on regular files.
Comment From: rsc
I think the complete docs for CopyFS are:
// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error
Comment From: qmuntal
Should CopyFS also copy symlinks? Something like this:
switch m := d.Mode(); {
case m.IsRegular():
...
case m&os.ModeSymlink != 0:
link, err := os.Readlink(path)
if err != nil {
return err
}
if err := os.Symlink(link, targ); err != nil {
return err
}
}
If so, we should make sure that it works reasonable well with Windows symbolic links, or at least that the outcome is well understood and consistent. This is related to #63703.
Comment From: mvdan
@qmuntal I imagine that would need https://github.com/golang/go/issues/49580 to land first.
Comment From: rsc
It's very unclear to me what CopyFS should do with symlinks. Perhaps we should un-accept #49580. :-) Seriously, if the symlink is to something outside dir, that's probably an error, right? Creating a symlink to /etc/passwd seems like it violates the spirit of using filepath.IsLocal for the rest of the files.
Comment From: rsc
Symbolic links are so awful. I am going to come back to this after Go 1.22 is frozen.
Comment From: rsc
Getting back to symlinks, if we don't want to unaccept #49580 (which would be fine with me but I'm not proposing it), then it seems like we should only accept symlinks in CopyFS that stay within the fs. Symlinks pointing outside the FS tree would result in an error from the overall CopyFS operation. So for example you cannot CopyFS and create a symlink to /etc/passwd.
Does that seem reasonable?
Comment From: mvdan
That seems reasonable to me. Leaving #49580 as accepted also seems like the pragmatic option - even if it's awkward and adds lots more edge cases, the reality is that users will want symlink support. Either we add support ourselves, or soon enough there will be external packages like go-symlinkfs that don't have good interoperability with the rest of the ecosystem.
Comment From: rsc
I think the revised complete docs for CopyFS are:
// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// If a symbolic link would lexically point outside dir or
// cannot be created, an error is returned.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error
Comment From: rsc
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group
Adding to package os:
// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error
Comment From: rsc
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
Adding to package os:
// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error
Comment From: gopherbot
Change https://go.dev/cl/558995 mentions this issue: os: implement CopyFS
Comment From: gopherbot
Change https://go.dev/cl/564295 mentions this issue: internal/safefilepath: add IsLocal
Comment From: neild
I'm reopening this issue to bring up the question of what CopyFS should do if the destination contains a symlink.
If I CopyFS("dir", fsys), fsys contains a file etc/evil, and dir/etc is a symlink to /etc, should CopyFS write /etc/evil? I believe that at the moment it will. (CopyFS opens files with O_CREATE, so it won't overwrite an existing file like /etc/passwd, but there are cases where creating a file in a privileged directory is sufficient to commit shenanigans.)
If this is the intended behavior, then I think the documentation should mention that symlinks in the destination are resolved.
If this isn't the intended behavior, then we have the problem that the os package does not currently contain a robust, TOCTOU-safe way to open a file while avoiding symlink traversal. (#67002 aims to add one, but won't be available until 1.24 at the earliest.) One option might be to document that the CopyFS destination should not contain symlinks, check for and reject symlinks in a TOCTOU-unsafe fashion in 1.23, and use the more robust APIs from #67002 when they become available.
Comment From: rsc
Yes, if you unpack dir/etc/evil into dir/etc->/etc then CopyFS should write etc/evil, the same way that any other code would. It is not CopyFS's job to protect against buggy destinations any more than it is Create's job to stop people from opening dir/etc/passwd.
Analogous to Create, I think it would be fine for the Dir type being discussed over on #67002 to have a CopyFS method that does enforce restrictions, since that's what that type does.
Comment From: gopherbot
Change https://go.dev/cl/600775 mentions this issue: os: document CopyFS behavior for symlinks in destination
Comment From: gopherbot
Change https://go.dev/cl/600815 mentions this issue: [release-branch.go1.23] os: document CopyFS behavior for symlinks in destination
Comment From: neild
Updated documentation to state that symlinks in the destination are followed.