Why is the fs.WalkDirFunc error parameter being ignored?

This seems related to #56172. There's a similar question in these about how to handle broken writes and missing directories.

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.

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

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.

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.

Have all remaining concerns about this proposal been addressed?

The sample code has os.MkdirAll(targ, 0777). Should that use info, err := d.Info() and os.MkdirAll(targ, info.Mode()) instead?

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.

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

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.

@qmuntal I imagine that would need #49580 to land first.

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.

Symbolic links are so awful. I am going to come back to this after Go 1.22 is frozen.

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?

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.

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

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

Change https://go.dev/cl/564295 mentions this issue: internal/safefilepath: add IsLocal

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.

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.

Change https://go.dev/cl/600775 mentions this issue: os: document CopyFS behavior for symlinks in destination

Change https://go.dev/cl/600815 mentions this issue: [release-branch.go1.23] os: document CopyFS behavior for symlinks in destination

Updated documentation to state that symlinks in the destination are followed.