PermissionDenied checking Windows symlink target is misinterpreted as collision #1421
Comments
|
Thanks a lot for the detailed report! It should be straightforward to reproduce this in a unit test and fix the issue subsequently. Apparently there is no test for that as I thought I was aware that Git falls back to assuming a file-symlink, and thought that would be implemented here as well already. Edit: I just saw in #1420 that a PR for that is coming up, which probably fixes this issue as well. |
|
Yes, I've opened #1425, whose fix covers both #1420 and this automatically. While #1425 contains specific tests for #1420 (as well as a test related to #1422), it does not contain a specific test for this issue. It is feasible to add one, though I am unsure how straightforward. It turned out that, remarkably, I didn't have to use any Windows-specific facilities for the tests in #1425. Tests specifically for the condition described here would have to do so. Furthermore, the Windows permissions system is very versatile, and I am not confident the unit tests are never run in environments where they are prohibited to change permissions inside their own temporary directories, though I don't know how common such a restriction would be. If tests for this specific issue are needed, I can think of three ways to do it:
Such tests could be regarded as blocking the merge of #1425 until added, or they could be added at some point in the future, or they could never be added. I'm not sure what is best. |
Having a single test for directory creation, like added in the PR, should be good enough for that PR and I'd once again skip over more test-cases specific to Windows until there is an actual issue around them. Instead, I usually try to learn from Git and see where it put its resources - if Git doesn't have these very special tests, it's likely not a problem to anybody (yet). |
When the metadata of a symlink's target cannot be obtained, even if the error is something other than `NotFound`, this falls back to creating file symbolic links. This only affects scenarios where either the checkout would fail entirely or where the symlink would have been treated as a collision and skipped (even though it was not really a collision, since only its target had an error). Other cases are not affected, and all exisitng scenarios where directory symlink would be created will still create directory symlinks. This builds on 31d02a8 (GitoxideLabs#1363) by supporting dangling symlinks even when the target filenames are unusual, such as when the name is invalid or reserved. Windows permits such symlinks to be created, and going ahead and creating the matches the Git behavior. This should also support other errors beisdes `NotFound`. For example, some permissions-related errors, in some cases where traversal or acccess (even to access metadata) are not allowed, would fail to create a symlink. This should address that as well. This works by using `Path::is_dir()` in the standard library, which automatically converts all errors (not just `NotFound`) into `false`. The logic here is thus quite similar to what was already present, just more tolerant, even though the code itself is shorter and simpler. This fixes GitoxideLabs#1420, and also fixes GitoxideLabs#1421.
Current behavior 😯
Background
Checking out files into a working tree skips any individual files that already exist, reporting them correctly as collisions. This happens when overwriting is prohibited, as is the case in
gix clone. For example, if a separate application creates files inside the working treegix cloneis checking out, and such a file would be overwritten if checked out,gix clonedeclines to check out that specific file and reports it as a collision. This behavior is correct and should not be changed.Like other operating systems that support symbolic links, Windows permits a symbolic link to point to a location that is inaccessible to the user creating the symbolic link, including to a location where, due to permissions, traversal to the location will fail due to permissions. The user will not be able to deference that symbolic link other than to inspect the target path, not even to check it's target's existence and metadata, but the symlink itself is no problem.
Unlike other operating systems that support symbolic links, Windows symbolic links come in two varieties: file and directory symbolic links. For this reason, gitoxide attempts to check the metadata of the symlink's target, to decide which type of symlink to create.
The problem
The problem is that, when attempting to check the metadata of the symlink's target fails with a
PermissionDeniederror, this is wrongly misinterpreted as a collision, even though there is no collision because the error happens in reading what the index specifies is the symlink's target, not when creating a new file. A wrong or unusual target of a symlink is never a real collision, because checkout does not create or mutate anything at the target.This is what it looks like to use
gix cloneto clone a repository with such a symlink:The repository is created, but due to the symlink wrongly being detected as a collision, no symlink is created there:
Relationship to #1420
This is related to #1420. As there, the error occurs when attempting to access metadata on the symlink's target, which is only done on Windows, so this bug does not affect non-Windows systems, and which is only done when actually attempting to create symbolic links, so this bug does not affect uses of
gix clonewherecore.symlinkshas been forced tofalse(though, as there, #1353 affects this).Depending on how #1420 is fixed, the fix may automatically take care of this as well, by making it so no
PermissionDeniederror can ever be propagated upward from a position where it doesn't represent a real collision. I had originally planned to report this as part of #1420. However, on reflection I decided these are separate bugs, because:Expected behavior 🤔
Symbolic links to locations to which the application cannot traverse, including when the error is
PermissionDenied, should be created, unless a specific rationale is known for declining to do so. Such a rationale would have to overcome the following factors:As for dangling symlinks, these can be created as file symlinks. As in dangling symlinks, this is imperfect, but not inherently different from the situation of dangling symlinks where the target may later created as a directory. In addition, there may be existing conditions where a file or directory target is hidden from the process rather than access being denied explicitly, in which case that is already treated as dangling.
If for some reason that is not to be done, another aspect of the expected behavior is that these are not collisions and should not be reported as such.
Git behavior
Git creates the symlinks. It creates them as file symlinks, even when the inaccessible targets are directories, since it cannot know they are directories and it falls back to creating file symlinks when the target type cannot be discerned:
The referred-to
MutableBackupentry is actually a directory, but thegitprocess has no way of knowing that because it is not allowed to access metadata on that item or list the contents of its parent directory. So Git has created a file symlink.This can be seen because the
dirbuiltin incmd.exeshows<SYMLINK>, whereas for a directory symlink it would show<SYMLINKD>.Details on that particular path and why I used it for testing are included below.
Steps to reproduce 🕹
1. Create a repository with a symlink into an inaccessible location
Whether such a location suitable for testing already exists or has to be created may vary by system.
On current desktop versions of Windows, the Microsoft Store keeps its Store applications in the
WindowsAppssubdirectory of theProgram Filesdirectory, which has restrictive permissions such that even administrators cannot list the directory's contents or access information on specific known subdirectory, at least if UAC is enabled and elevation has not been performed. (On server versions of Windows, the Microsoft Store is absent.)The specific paths vary across systems. I decided to use the drive-relative path to a known subdirectory of
WindowsApps. Although I actually performed more steps because I tried some other locations and ways of specifying the path--and although it is fairly straightforward to create such a repository in a native Windows system--I created the repository on an Ubuntu system (WSL is sufficient) in a manner similar to this, which is sufficient:This is a symlink to the
MutableBackupsubdirectory ofWindowsApps, which users cannot typically access, if the repository is cloned on the system drive (or otherwise on the same drive that contains aProgram Filesdirectory that containsWindowsApps, in the rare case that this is different).I made the test repository this way for convenience, so that the steps to verify this bug are simple in most cases, once the repository is already available. This can alternatively be tested by creating a new directory tree and setting restrictive ACL permissions.
The repository is available on GitHub. The following steps assume it is used and will have to be modified otherwise. The output shown reflects the presence of additional files in the repository, such as
README.md, which are present mainly as documentation, though they also help verify that other files, besides the symlink, are still checked out ingix clone.2. Optionally, clone the repo with
git cloneto verify Git behaviorThe
git clonebehavior, including the specific command, is shown above in the "Git behavior" section.The created
symlink-to-inacessibledirectory should be deleted before testing again withgix clone(if it is to be run in the same location), which in PowerShell can be done with:3. Clone the repo with
gix cloneto observe the bugThis is shown above in "Current behavior," showing the error:
After checking the output, and that the working tree exists with other files besides the symlink that is wrongly taken to be a collision, the created directory should be removed again before running further tests, to avoid clashes.
4. Optionally, clone with
--tracefor more outputThis is mainly to help verify that nothing else unusual is happening. It can also be compared to the output in #1420.
The text was updated successfully, but these errors were encountered: