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

Git LFS pointers permit extension entries before the version header line #5702

Open
chrisd8088 opened this issue Apr 7, 2024 · 1 comment
Labels
Projects
Milestone

Comments

@chrisd8088
Copy link
Contributor

Describe the bug
Git LFS pointers permit extension entries before the version header line.

To Reproduce
Steps to reproduce the behaviour:

$ cat <<EOF | git lfs pointer --check --stdin && echo ok
ext-0-foo sha256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c
version https://git-lfs.github.com/spec/v1
oid sha256:0123456789012345678901234567890123456789012345678901234567890123
ext-1-bar sha256:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730
size 4
EOF
ok

Expected behaviour
The Git LFS client should expect the version header line as the first entry in any Git LFS pointer, even in its more permissive parsing modes.

System environment
The example above is performed on macOS with Git installed via Homebrew, but applies to any system.

Output of git lfs env

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

Additional context
We have likely accepted extension entries which precede the version entry since the introduction of support for Git LFS extensions in PR #486. This is presumably an oversight of the current (and original) implementation whereby we expect to encounter the version, oid, and size entries in that order, and if a line entry does not match the next expected one, we allow it only if it is an ext-* extension entry, but we don't validate that those appear after the version entry or in any particular order.

The example above does not output ok if the --strict option is passed to git lfs pointer, which makes sense as the canonical pointer format would be:

version https://git-lfs.github.com/spec/v1
ext-0-foo sha256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c
ext-1-bar sha256:7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730
oid sha256:0123456789012345678901234567890123456789012345678901234567890123
size 4

However, even in a non-strict parsing mode, we ought to require that the version entry be listed first; otherwise any future changes to the format (such as enhancements to ext-* lines) will be more difficult to parse as we will not know if certain lines are valid until we encounter the version entry.

While it seems improbable that, in practice, there would be users who have Git LFS pointer files with extension entries ahead of the version entry, adding this requirement now would technically constitute a backwards-incompatible change, so we probably have to wait for Git LFS v4.0.0 to implement any such requirement.

Moreover, we might need to bump the Git LFS pointer version as well, but that's a discussion we can have when we reach the point of a v4.0.0 client release.

@chrisd8088 chrisd8088 added the bug label Apr 7, 2024
@chrisd8088 chrisd8088 added this to the v4.0.0 milestone Apr 7, 2024
@chrisd8088 chrisd8088 added this to Bugs in Backlog Apr 7, 2024
@chrisd8088
Copy link
Contributor Author

As an addendum to this report, we also permit non-alphanumeric characters after the ext-<n>-<name> key values because our regular expression isn't terminated with a \z. For example:

$ cat <<EOF | git lfs pointer --check --stdin && echo ok
version https://git-lfs.github.com/spec/v1
ext-0-foo:()[] sha256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c
oid sha256:0123456789012345678901234567890123456789012345678901234567890123
size 4
EOF
ok

So while [a-zA-Z0-9_] are parsed as the "name" of the extension entry, miscellaneous non-whitespace characters are then allowed after that. This is also something we can fix in a major release, and again possibly something where we may want to also bump the pointer version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

1 participant