fix: add O_NOFOLLOW to copyFile

[valoq]

copyFile currently opens the destination with O_CREATE|O_TRUNC
on shared storage, other users can place a symlink at the destination and cause the write operation to be redirected..
adding O_NOFOLLOW avoids this.

Note that O_NOFOLLOW does not exist on windows.

[valoq]

I update this PR to address one remaining case of the same issue

[joelim-work]

O_NOFOLLOW only works on the last component of a path.

Theoretically this means your patch will not address cases where other parts of the destination path contains a symbolic link. For example when copying foo/bar/baz/test.txt to /home/user/dest, the destination path will be /home/user/dest/foo/bar/baz/test.txt, and while O_NOFOLLOW will protect against test.txt being a symbolic link, it will not cover cases where foo, bar or baz is a symbolic link.

[valoq]

I changed the patch to use os.Root instead which is Go's standard library solution for safely writing files into a directory (added in go 1.24, but we already require 1.25 anyway)

os.Root methods reject file modes with non-permission bits, so info.Mode().Perm() is needed where the original code passed info.Mode() directly.

[valoq]

Thanks, that should all be addressed now.

[joelim-work]

Sorry I just thought of one edge case - if using os.Root, it's still possible for someone to create a symbolic link during the copy which points to a location inside the destination root, which results in the file at the target location being overwritten. So it is not completely foolproof. Using O_NOFOLLOW would also not fully address the issue either as the symbolic link might not be at the last component in the path. An example would be to copy src/ which contains src/subdir/test.txt and then create a symbolic link at subdir which points somewhere else.

Hi @CatsDeservePets, I was wondering if you had any thoughts on this PR.

[valoq]

Uh, this is becoming a bit too complicated for what I had in mind. The intention was to ensure reliable copy operations even on shared directories. And to mimic the same behavior as binutils cp enforces.

[valoq]

After looking into this a bit more, it turns out that completely preventing interference is not possible, and even coreutils 'cp' does not address this. Actually, the current behavior of lf is already equivalent to the threat model applied by cp. We could drop this entire PR if thats the objective.

The only reason to keep it is to address the cases where the improvement is easy and with low cost in terms of complexity. The focus should be on accidental writes that were not intended when symlinks are encountered.

I will look a bit more into this, but I would only merge this as a minimal change that adds useful protection against either a trivial and common attack on shared drives or if it prevents accidents.

At the moment, I tend towards dropping this PR, since using 'cp' behavior as baseline seems reasonable

[valoq]

Ok , so this PR keeps a copy operation contained inside the chosen destination folder. If the destination contains a shortcut that points to somewhere else on the disk, lf will not write through it. A common case is copying a folder into a directory that has an unexpected shortcut left over from a previous setup, where without the patch lf would scatter the copied files into the shortcut's target instead of into the destination. The question is if that is expected behavior or should this lf contain the operation? I am unsure if this breaks things.

@joelim-work thoughts?

[joelim-work]

A common case is copying a folder into a directory that has an unexpected shortcut left over from a previous setup

In this case I think lf would use dupfilefmt to ensure that the directory being copied does not conflict with an existing directory. So then the only issue would be if something/someone tries to trick lf by creating a symbolic link in the new directory while is is being copied, but that sounds uncommon.

Disallowing symbolic links (I'm not sure how trivial it is to implememnt, as O_NOFOLLOW alone is not enough) is a separate concept from using os.Root to prevent updates outside of the destination directory (but still allows symbolic links).

TBH I haven't heard of any issues from users before about the copy operation following symbolic links and overwriting other parts of the filesystem. Maybe it's worth checking other file managers like ranger/joshuto/yazi to see how they handle copying, and I don't mean just testing it locally, but also examining the actual source code to see how it is done and what precautions are taken if any.

[valoq]

After looking through the binutils cp code, I am rather hesitant to implement any deviation from how cp handles copies. There are many edge cases where the attempt to harden this could have side effects which I dont understand fully.
With #2557 lf will behave exactly like cp and I think that is a good baseline fo lf to follow. Other file managers like ranger handle some operations less reliable then cp and lf, so we are already better off in comparison.

Closing this for now.