Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lockable files write permissions and lock state not managed correctly in all cases #5891

Open
chrisd8088 opened this issue Oct 18, 2024 · 0 comments
Labels

Comments

@chrisd8088
Copy link
Member

Describe the bug

As shown in the first of the reproduction examples below, the git lfs unlock command permits users to unlock modified files without supplying the --force option in some cases. Specifically, if the current working directory is not the root of the Git working tree, the git lfs unlock command permits locked files to be unlocked without the --force option even if the files have local modifications that have not yet been committed.

The other two reproduction examples below demonstrate that if lockable files' names (or parent directories' names) contain quotation marks or certain other non-alphanumberic characters, or if the current commit lacks parents in the Git history, the Git LFS hooks do not change the files' write permissions.

As well, the git lfs unlock command permits locked files to be unlocked without the --force option even if the files have local modifications that have not yet been committed, if the files' names or paths contain certain special characters like quotation marks.

To Reproduce

Steps to reproduce the behaviour when the current working directory is not the root of the Git working tree:

$ git clone https://github.com/chrisd8088/test
$ cd test

$ git lfs track --lockable '*.bin'
$ git add .gitattributes
$ git commit -m attrs

$ mkdir sub

$ echo foo >sub/foo.bin
$ git add sub/foo.bin
$ git commit -m foo

$ git check-attr lockable -- sub/foo.bin
sub/foo.bin: lockable: set

$ git lfs lock sub/foo.bin

$ echo bar >sub/foo.bin
$ git lfs unlock sub/foo.bin
Cannot unlock file with uncommitted changes

$ cd sub
$ git lfs unlock foo.bin
Unlocked sub/foo.bin

Steps to reproduce the behaviour when the current commit is an initial commit in the Git history:

$ git init test && cd test
$ git lfs track --lockable '*.bin'

$ echo foo >foo.bin
$ git add foo.bin
$ git commit -m foo

$ git check-attr lockable -- foo.bin
foo.bin: lockable: set

$ ls -l foo.bin | awk '{ print $1 " " $9 }'
-rw-r--r-- foo.bin

$ echo bar >bar.bin
$ git add bar.bin
$ git commit -m bar

$ git check-attr lockable -- bar.bin
bar.bin: lockable: set

$ ls -l bar.bin | awk '{ print $1 " " $9 }'
-r--r--r-- bar.bin

Steps to reproduce the behaviour when a filename contains special characters:

$ git clone https://github.com/chrisd8088/test
$ cd test

$ git lfs track --lockable '*.bin'
$ git add .gitattributes
$ git commit -m attrs

$ echo foo >foo.bin
$ echo bar >'bar"bar.bin'
$ git add *.bin
$ git commit -m foobar

$ git check-attr lockable -- *.bin
"bar\"bar.bin": lockable: set
foo.bin: lockable: set

$ ls -l *.bin | awk '{ print $1 " " $9 }'
-rw-r--r-- bar"bar.bin
-r--r--r-- foo.bin

$ git lfs lock *.bin

$ echo bar >foo.bin
$ echo foo >'bar"bar.bin'
$ git lfs unlock *.bin
Unlocked bar"bar.bin
Cannot unlock file with uncommitted changes

$ git lfs locks
foo.bin	chrisd8088	ID:5677262

$ git checkout -- .
$ git lfs unlock foo.bin

$ git checkout HEAD^
$ ls *.bin
ls: *.bin: No such file or directory

$ git checkout main
-rw-r--r-- bar"bar.bin
-r--r--r-- foo.bin

Expected behaviour

In all cases, the git lfs unlock command should not allow a modified file to be unlocked unless the --force option is provided, regardless of the current working directory or any special characters in the file's name or path.

And in all cases, the write permissions of lockable files should be correctly set or reset, regardless of any special characters in the files' names or paths, or whether the current commit has a parent or not.

System environment

The examples above were performed on macOS, but should behave the same way on any platform.

Output of git lfs env

git-lfs/3.5.1 (GitHub; darwin arm64; go 1.22.1)
git version 2.46.0

Additional context

The git lfs unlock command relies on the IsFileModified() function in our git package, which executes a git status --porcelain -- <pathspec> command, passing the full path to the file as the <pathspec> argument. In the example above, this would be sub/foo.bin. However, the git status command respects the current working directory and prepends any portions of that path which are within the Git working tree to the <pathspec> argument. Thus, running git status --porcelain -- sub/foo.bin from within the sub directory results in no output except a warning: could not open directory 'sub/sub/': No such file or directory message. As a result, our IsFileModified() command incorrectly asserts that the file is not modified, and the git lfs unlock command proceeds and allows a user to unlock a modified file without supplying the --force option.

To fix this issue we need to prefix the <pathspec> we pass to the git status command with the Git "magic word" top, e.g., :(top)sub/foo.bin.

The git lfs post-commit command utilizes the GetFilesChanged() function in our git package, passing just the single ref HEAD, which runs a git diff-tree HEAD command, expecting that the command will return details of the differences between the current commit and its parent(s). However, if the current commit has no parent, nothing is returned. In this case, the git lfs post-commit command just passes an empty list to the FixLockableFileWriteFlags() method of the Client structure in our locking package.

To resolve this issue we need to pass the --root option to the git diff-tree command so that it will diff an initial commit against a NULL tree and, as per the Git documentation, treat it as a "big creation event".

The git lfs post-commit command, the git lfs post-checkout command, and the git lfs unlock command use the IsFileModified() and GetFilesChanged() functions in our git package, which as described above, run the git status and git diff-tree commands, respectively. Both functions pass the core.quotePath=false configuration option to the Git commands with the explicit comment that this should "handle special chars in filenames". However, while the Git documentation explains that this setting means that bytes higher than 0x80 will not be quoted, this does not mean that "raw" filenames are output:

Double-quotes, backslash and control characters are always escaped regardless of the setting of this variable.

In the example above, the output of git status with the core.quotePath option set to false therefore returns:

$  git -c core.quotepath=false status --porcelain --
 M "bar\"bar.bin"

Since our IsFileModified() and GetFilesChanged() functions do not process the returned output to unescape special characters like double-quotes, they fail to match the Git output with the actual filenames, and as a result, the git lfs unlock command proceeds and allows a user to unlock a modified file without supplying the --force option, while the git lfs post-commit and git lfs post-checkout commands fail to remove the write permissions on those files.

To fix these issues we need to drop the core.quotePath=false Git configuration option in favour of the -z option, as we already use in many other functions. We then need to split the Git command output on null bytes, again following our existing usage in other functions.

Further, we likely want to prepend the literal "magic word" to the <pathspec> we pass to git status in our IsFileModified() function, along with the top magic word discussed above, e.g., :(top,literal)sub/foo.bin. This will ensure that if the file name or path contains * or ? characters that they are treated literally and not as "glob" matches, which might return multiple results.

@chrisd8088 chrisd8088 added the bug label Oct 18, 2024
@chrisd8088 chrisd8088 moved this to Bugs in Backlog Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Bugs
Development

No branches or pull requests

1 participant