[PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[brian m. carlson]

The recent defense-in-depth patches to restrict hooks while cloning
broke Git LFS because it installs necessary hooks when it is invoked by
Git's smudge filter.  This means that currently, anyone with Git LFS
installed who attempts to clone a repository with at least one LFS file
will see a message like the following (fictitious example):

----
$ git clone https://github.com/octocat/xyzzy.git
Cloning into 'pull-bug'...
remote: Enumerating objects: 1275, done.
remote: Counting objects: 100% (343/343), done.
remote: Compressing objects: 100% (136/136), done.
remote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932
Receiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done.
Resolving deltas: 100% (226/226), done.
Filtering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done.
fatal: active `post-checkout` hook found during `git clone`:
        /home/octocat/xyzzy/.git/hooks/post-checkout
For security reasons, this is disallowed by default.
If this is intentional and the hook should actually be run, please
run the command again with `GIT_CLONE_PROTECTION_ACTIVE=false`
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry with 'git restore --source=HEAD :/'
----

This causes most CI systems to be broken in such a case, as well as a
confusing message for the user.

It's not really possible to avoid the need to install the hooks at this
location because the post-checkout hook must be ready during the
checkout that's part of the clone in order to properly adjust
permissions on files.  Thus, we'll need to revert the changes to
restrict hooks while cloning, which this series does.

brian m. carlson (2):
  Revert "clone: prevent hooks from running during a clone"
  Revert "core.hooksPath: add some protection while cloning"

 builtin/clone.c  |  5 -----
 config.c         | 13 +-----------
 hook.c           | 32 ------------------------------
 t/t1800-hook.sh  | 15 --------------
 t/t5601-clone.sh | 51 ------------------------------------------------
 5 files changed, 1 insertion(+), 115 deletions(-)

[PATCH 1/2] Revert "clone: prevent hooks from running during a clone"

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

This change breaks Git LFS, which relies on the pre-checkout hook to run
in a clone.  This is to allow files which have the lockable attribute,
which are usually large binary assets which cannot be merged, to
be marked read-only when a user doesn't have a lock so as to prevent
conflicts.

Revert this change since it causes functional problems.

This reverts commit 8db1e8743c0f1ed241f6a1b8bf55b6fef07d6751.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 builtin/clone.c  |  5 -----
 hook.c           | 32 ------------------------------
 t/t5601-clone.sh | 51 ------------------------------------------------
 3 files changed, 88 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0e7cf198f5..c66a32feb3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -987,12 +987,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		usage_msg_opt(_("You must specify a repository to clone."),
 			builtin_clone_usage, builtin_clone_options);
 
-	xsetenv("GIT_CLONE_PROTECTION_ACTIVE", "true", 0 /* allow user override */);
 	template_dir = get_template_dir(option_template);
-	if (*template_dir && !is_absolute_path(template_dir))
-		template_dir = template_dir_dup =
-			absolute_pathdup(template_dir);
-	xsetenv("GIT_CLONE_TEMPLATE_DIR", template_dir, 1);
 
 	if (option_depth || option_since || option_not.nr)
 		deepen = 1;
diff --git a/hook.c b/hook.c
index eebc4d4473..f5f46e844e 100644
--- a/hook.c
+++ b/hook.c
@@ -11,30 +11,6 @@
 #include "setup.h"
 #include "copy.h"
 
-static int identical_to_template_hook(const char *name, const char *path)
-{
-	const char *env = getenv("GIT_CLONE_TEMPLATE_DIR");
-	const char *template_dir = get_template_dir(env && *env ? env : NULL);
-	struct strbuf template_path = STRBUF_INIT;
-	int found_template_hook, ret;
-
-	strbuf_addf(&template_path, "%s/hooks/%s", template_dir, name);
-	found_template_hook = access(template_path.buf, X_OK) >= 0;
-#ifdef STRIP_EXTENSION
-	if (!found_template_hook) {
-		strbuf_addstr(&template_path, STRIP_EXTENSION);
-		found_template_hook = access(template_path.buf, X_OK) >= 0;
-	}
-#endif
-	if (!found_template_hook)
-		return 0;
-
-	ret = do_files_match(template_path.buf, path);
-
-	strbuf_release(&template_path);
-	return ret;
-}
-
 const char *find_hook(const char *name)
 {
 	static struct strbuf path = STRBUF_INIT;
@@ -70,14 +46,6 @@ const char *find_hook(const char *name)
 		}
 		return NULL;
 	}
-	if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) &&
-	    !identical_to_template_hook(name, path.buf))
-		die(_("active `%s` hook found during `git clone`:\n\t%s\n"
-		      "For security reasons, this is disallowed by default.\n"
-		      "If this is intentional and the hook should actually "
-		      "be run, please\nrun the command again with "
-		      "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
-		    name, path.buf);
 	return path.buf;
 }
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index deb1c282c7..cc0b953f14 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -788,57 +788,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client
 '
 
-test_expect_success 'clone with init.templatedir runs hooks' '
-	git init tmpl/hooks &&
-	write_script tmpl/hooks/post-checkout <<-EOF &&
-	echo HOOK-RUN >&2
-	echo I was here >hook.run
-	EOF
-	git -C tmpl/hooks add . &&
-	test_tick &&
-	git -C tmpl/hooks commit -m post-checkout &&
-
-	test_when_finished "git config --global --unset init.templateDir || :" &&
-	test_when_finished "git config --unset init.templateDir || :" &&
-	(
-		sane_unset GIT_TEMPLATE_DIR &&
-		NO_SET_GIT_TEMPLATE_DIR=t &&
-		export NO_SET_GIT_TEMPLATE_DIR &&
-
-		git -c core.hooksPath="$(pwd)/tmpl/hooks" \
-			clone tmpl/hooks hook-run-hookspath 2>err &&
-		test_grep ! "active .* hook found" err &&
-		test_path_is_file hook-run-hookspath/hook.run &&
-
-		git -c init.templateDir="$(pwd)/tmpl" \
-			clone tmpl/hooks hook-run-config 2>err &&
-		test_grep ! "active .* hook found" err &&
-		test_path_is_file hook-run-config/hook.run &&
-
-		git clone --template=tmpl tmpl/hooks hook-run-option 2>err &&
-		test_grep ! "active .* hook found" err &&
-		test_path_is_file hook-run-option/hook.run &&
-
-		git config --global init.templateDir "$(pwd)/tmpl" &&
-		git clone tmpl/hooks hook-run-global-config 2>err &&
-		git config --global --unset init.templateDir &&
-		test_grep ! "active .* hook found" err &&
-		test_path_is_file hook-run-global-config/hook.run &&
-
-		# clone ignores local `init.templateDir`; need to create
-		# a new repository because we deleted `.git/` in the
-		# `setup` test case above
-		git init local-clone &&
-		cd local-clone &&
-
-		git config init.templateDir "$(pwd)/../tmpl" &&
-		git clone ../tmpl/hooks hook-run-local-config 2>err &&
-		git config --unset init.templateDir &&
-		test_grep ! "active .* hook found" err &&
-		test_path_is_missing hook-run-local-config/hook.run
-	)
-'
-
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 

[PATCH 2/2] Revert "core.hooksPath: add some protection while cloning"

[brian m. carlson]

From: "brian m. carlson" <bk2204@github.com>

The original commit breaks Git LFS, which installs hooks when it is
invoked during the smudge process as part of checkout.  This is required
to install a post-checkout hook that causes files which are set as
lockable (which are typically large binary assets that cannot be merged)
to be read-only unless they've been locked.  In addition, Git LFS
requires the pre-push hook to be installed so that LFS objects can be
pushed as part of the invocation of git push.

Without the ability to install these hooks, the locking functionality
would not work until the user invoked Git LFS again and did a completely
new checkout with all files changed, since Git LFS optimizes for only
changed files.  In addition, an invocation of git push might not push
anything LFS files all to the remote, potentially causing data loss.

Note that this affects all clone operations with a repository with Git
LFS files in it, even if they are configured not to smudge data by
default, so it breaks all automated clones (which will see "die" called)
without the relevant environment variable specified.

Revert this change to restore functionality.

This reverts commit 20f3588efc6cbcae5bbaabf65ee12df87b51a9ea.

Signed-off-by: brian m. carlson <bk2204@github.com>
---
 config.c        | 13 +------------
 t/t1800-hook.sh | 15 ---------------
 2 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/config.c b/config.c
index 77a0fd2d80..ae3652b08f 100644
--- a/config.c
+++ b/config.c
@@ -1416,19 +1416,8 @@ static int git_default_core_config(const char *var, const char *value,
 	if (!strcmp(var, "core.attributesfile"))
 		return git_config_pathname(&git_attributes_file, var, value);
 
-	if (!strcmp(var, "core.hookspath")) {
-		if (ctx->kvi && ctx->kvi->scope == CONFIG_SCOPE_LOCAL &&
-		    git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0))
-			die(_("active `core.hooksPath` found in the local "
-			      "repository config:\n\t%s\nFor security "
-			      "reasons, this is disallowed by default.\nIf "
-			      "this is intentional and the hook should "
-			      "actually be run, please\nrun the command "
-			      "again with "
-			      "`GIT_CLONE_PROTECTION_ACTIVE=false`"),
-			    value);
+	if (!strcmp(var, "core.hookspath"))
 		return git_config_pathname(&git_hooks_path, var, value);
-	}
 
 	if (!strcmp(var, "core.bare")) {
 		is_bare_repository_cfg = git_config_bool(var, value);
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 1894ebeb0e..8b0234cf2d 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -185,19 +185,4 @@ test_expect_success 'stdin to hooks' '
 	test_cmp expect actual
 '
 
-test_expect_success 'clone protections' '
-	test_config core.hooksPath "$(pwd)/my-hooks" &&
-	mkdir -p my-hooks &&
-	write_script my-hooks/test-hook <<-\EOF &&
-	echo Hook ran $1
-	EOF
-
-	git hook run test-hook 2>err &&
-	test_grep "Hook ran" err &&
-	test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \
-		git hook run test-hook 2>err &&
-	test_grep "active .core.hooksPath" err &&
-	test_grep ! "Hook ran" err
-'
-
 test_done

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[Johannes Schindelin]

brian,

On Tue, 14 May 2024, brian m. carlson wrote:

> The recent defense-in-depth patches to restrict hooks while cloning
> broke Git LFS because it installs necessary hooks when it is invoked by
> Git's smudge filter.  This means that currently, anyone with Git LFS
> installed who attempts to clone a repository with at least one LFS file
> will see a message like the following (fictitious example):
>
> ----
> $ git clone https://github.com/octocat/xyzzy.git
> Cloning into 'pull-bug'...
> remote: Enumerating objects: 1275, done.
> remote: Counting objects: 100% (343/343), done.
> remote: Compressing objects: 100% (136/136), done.
> remote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932
> Receiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done.
> Resolving deltas: 100% (226/226), done.
> Filtering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done.
> fatal: active `post-checkout` hook found during `git clone`:
>         /home/octocat/xyzzy/.git/hooks/post-checkout
> For security reasons, this is disallowed by default.
> If this is intentional and the hook should actually be run, please
> run the command again with `GIT_CLONE_PROTECTION_ACTIVE=false`
> warning: Clone succeeded, but checkout failed.
> You can inspect what was checked out with 'git status'
> and retry with 'git restore --source=HEAD :/'
> ----
>
> This causes most CI systems to be broken in such a case, as well as a
> confusing message for the user.

When using `actions/checkout` in GitHub workflows, nothing is broken
because `actions/checkout` uses a fetch + checkout (to allow for things
like sparse checkout), which obviously lacks the clone protections because
it is not a clone.

> It's not really possible to avoid the need to install the hooks at this
> location because the post-checkout hook must be ready during the
> checkout that's part of the clone in order to properly adjust
> permissions on files.  Thus, we'll need to revert the changes to
> restrict hooks while cloning, which this series does.

Dropping protections is in general a bad idea. While previously, hackers
wishing to exploit weaknesses in Git might have been unaware of the
particular attack vector we want to prevent with these defense-in-depth
measurements, we now must assume that they are fully aware. Reverting
those protections can be seen as a very public invitation to search for
ways to exploit the now re-introduced avenues to craft Remote Code
Execution attacks.

I have pointed out several times that there are alternatives while
discussing this under embargo, even sent them to the git-security list
before the embargo was lifted, and have not received any reply. One
proposal was to introduce a way to cross-check the SHA-256 of hooks that
_were_ written during a clone operation against a list of known-good ones.
Another alternative was to special-case Git LFS by matching the hooks'
contents against a regular expression that matches Git LFS' current
hooks'.

Both alternatives demonstrate that we are far from _needing_ to revert the
changes that were designed to prevent future vulnerabilities from
immediately becoming critical Remote Code Executions. It might be an
easier way to address the Git LFS breakage, but "easy" does not equal
"right".

I did not yet get around to sending these patches to the Git mailing list
solely because I am still busy with a lot of follow-up work of the
embargoed release. It was an unwelcome surprise to see this here patch
series in my inbox and still no reply to the patches I had sent to the
git-security list for comments.

I am still busy wrapping up follow-up work and won't be able to
participate in this here mail thread meaningfully for the next hours. I do
want to invite you to think about alternative ways to address the Git LFS
issues, alternatives that do not re-open weaknesses we had hoped to
address for good.

I do want to extend the invitation to work with me on that, for example by
reviewing those patches I sent to the git-security mailing list (or even
to send them to the Git mailing list for public review on my behalf, that
would be helpful).

Ciao,
Johannes

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 5710 bytes --]

On 2024-05-14 at 19:07:28, Johannes Schindelin wrote:
> brian,
> 
> On Tue, 14 May 2024, brian m. carlson wrote:
> > This causes most CI systems to be broken in such a case, as well as a
> > confusing message for the user.
> 
> When using `actions/checkout` in GitHub workflows, nothing is broken
> because `actions/checkout` uses a fetch + checkout (to allow for things
> like sparse checkout), which obviously lacks the clone protections because
> it is not a clone.

I'm not saying all CI systems do this, but we probably have broken quite
a few.

> > It's not really possible to avoid the need to install the hooks at this
> > location because the post-checkout hook must be ready during the
> > checkout that's part of the clone in order to properly adjust
> > permissions on files.  Thus, we'll need to revert the changes to
> > restrict hooks while cloning, which this series does.
> 
> Dropping protections is in general a bad idea. While previously, hackers
> wishing to exploit weaknesses in Git might have been unaware of the
> particular attack vector we want to prevent with these defense-in-depth
> measurements, we now must assume that they are fully aware. Reverting
> those protections can be seen as a very public invitation to search for
> ways to exploit the now re-introduced avenues to craft Remote Code
> Execution attacks.

If these protections hadn't broken things, I'd agree that we should keep
them.  However, they have broken things and they've introduced a
serious regression breaking a major project, and we should revert them.

I'm not opposed to us exploring other alternatives for defense in depth
measures on the list.  I definitely think such work is valuable and
important, but I want to make sure the changes we make don't regress
important functionality.

> I have pointed out several times that there are alternatives while
> discussing this under embargo, even sent them to the git-security list
> before the embargo was lifted, and have not received any reply. One
> proposal was to introduce a way to cross-check the SHA-256 of hooks that
> _were_ written during a clone operation against a list of known-good ones.
> Another alternative was to special-case Git LFS by matching the hooks'
> contents against a regular expression that matches Git LFS' current
> hooks'.

I have replied to those on the security list and to the general idea.  I
don't think we should special-case Git LFS here.  That's antithetical to
the long-standing ethos of the project.  While Git LFS is _one_ known
project to have been broken, there may be others, or people may have
forks of Git LFS under other names, and we want that tooling to be able
to take advantage of all of the same features.

I'm also opposed to any kind of pinning of the Git LFS hooks to Git in
general, whatever the approach is.  Git LFS runs against multiple
versions of Git in our CI, and if we change the hooks in a way that Git
hasn't pinned, either via SHA-256 hash or regex, then Git LFS (and its
CI) is broken until Git gets an update.  We've already had to deal with
small incompatible changes that have broken Git LFS, and I don't think
coupling the projects in this way is a good idea.

Finally, it's very easy to work around the protections by merely placing
a malicious binary called "git-lfs" earlier in the PATH, so we're not
necessarily getting a substantial amount of improved security by
requiring that binary.

> Both alternatives demonstrate that we are far from _needing_ to revert the
> changes that were designed to prevent future vulnerabilities from
> immediately becoming critical Remote Code Executions. It might be an
> easier way to address the Git LFS breakage, but "easy" does not equal
> "right".
> 
> I did not yet get around to sending these patches to the Git mailing list
> solely because I am still busy with a lot of follow-up work of the
> embargoed release. It was an unwelcome surprise to see this here patch
> series in my inbox and still no reply to the patches I had sent to the
> git-security list for comments.

As you may know, I don't read the git or git-security lists except on
my personal computer using my personal email, and I have been at work
most of the day putting out fires related to the Git LFS breakage above.
Thus, I haven't seen them until just recently, when I did try to get a
reply out.

> I am still busy wrapping up follow-up work and won't be able to
> participate in this here mail thread meaningfully for the next hours. I do
> want to invite you to think about alternative ways to address the Git LFS
> issues, alternatives that do not re-open weaknesses we had hoped to
> address for good.

I don't want to put undue pressure on you.  Please feel free to respond
or not at your leisure.

> I do want to extend the invitation to work with me on that, for example by
> reviewing those patches I sent to the git-security mailing list (or even
> to send them to the Git mailing list for public review on my behalf, that
> would be helpful).

I think the substance of my response was the same one that I gave above.

With all due respect, I don't think your patches are the right way to
address the problem, and I fear that by bringing them to the list, I'd
be giving the list the misleading impression that I endorsed them with
my sign-off.  While I respect you as a colleague and a contributor, even
if we may disagree on this or other issues, I don't agree with the
approach in the patches, and I don't think it would be a good idea to
apply my sign-off in sending them.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[Joey Hess]

brian m. carlson wrote:
> If these protections hadn't broken things, I'd agree that we should keep
> them.  However, they have broken things and they've introduced a
> serious regression breaking a major project, and we should revert them.

More than one major project; they also broke git-annex in the case where
a git-annex repository, which contains symlinks into
.git/annex/objects/, is pushed to a bare repository with
receive.fsckObjects set. (Gitlab is currently affected[1].)


BTW, do I understand correctly that the defence in depth patch set was
developed under embargo and has never been publically reviewed?

Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed
that its PATH_MAX check is also dodgy due to that having values ranging
from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git
repositories containing legitimate, working symlinks can now fail to be
pushed depending on what OS happens to host a reciving bare repository.

+                               if (is_ntfs_dotgit(p))

This means that symlinks to eg "git~1" are also warned about,
which seems strange behavior on eg Linux.

+                               backslash = memchr(p, '\\', slash - p);

This and other backslash handling code for some reason is also run on
linux, so a symlink to eg "ummmm\\git~1" is also warned about.

+               if (!buf || size > PATH_MAX) {

I suspect, but have not confirmed, that this is allows a symlink
target 1 byte longer than the OS supports, because PATH_MAX includes
a trailing NUL.


All in all, this seems to need more review and a more careful
consideration of breakage now that the security holes are not under
embargo.

-- 
see shy jo

[1] https://forum.gitlab.com/t/recent-git-v2-45-1-breaks-git-annex-compatibility-because-of-apparent-fsck-symlinkpointstogitdir-error-on-gitlab/104909

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

brian m. carlson wrote:
> > proposal was to introduce a way to cross-check the SHA-256 of hooks that
> > _were_ written during a clone operation against a list of known-good ones.
> > Another alternative was to special-case Git LFS by matching the hooks'
> > contents against a regular expression that matches Git LFS' current
> > hooks'.
> 
> I have replied to those on the security list and to the general idea.  I
> don't think we should special-case Git LFS here.  That's antithetical to
> the long-standing ethos of the project.

I was surprised today to find that git-annex also triggers the hook
problem. In particular, a git clone that uses git-remote-annex can
cause several hooks to get created.

I think the hook check is already scheduled for reversion, but in case
not, here's another data point against hard-coding known-good hooks as a
solution.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[Johannes Schindelin]

Hi Joey,

On Wed, 22 May 2024, Joey Hess wrote:

> brian m. carlson wrote:
> > If these protections hadn't broken things, I'd agree that we should keep
> > them.  However, they have broken things and they've introduced a
> > serious regression breaking a major project, and we should revert them.
>
> More than one major project; they also broke git-annex in the case where
> a git-annex repository, which contains symlinks into
> .git/annex/objects/, is pushed to a bare repository with
> receive.fsckObjects set. (Gitlab is currently affected[1].)

This added fsck functionality was specifically marked as `WARN` instead of
`ERROR`, though. So it should not have failed.

> BTW, do I understand correctly that the defence in depth patch set was
> developed under embargo and has never been publically reviewed?
>
> Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed
> that its PATH_MAX check is also dodgy due to that having values ranging
> from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git
> repositories containing legitimate, working symlinks can now fail to be
> pushed depending on what OS happens to host a reciving bare repository.

Likewise, this fsck functionality was specifically marked as `WARN`
instead of `ERROR`, to prevent exactly the issue you are seeing.

Are you saying that Gitlab is upgrading fsck warnings to errors? If so, I
fear we need to ask Gitlab to stop doing that.

> +                               if (is_ntfs_dotgit(p))
>
> This means that symlinks to eg "git~1" are also warned about,
> which seems strange behavior on eg Linux.

Only until you realize that there are many cross-platform projects, and
that Windows Subsystem for Linux is a thing.

> +                               backslash = memchr(p, '\\', slash - p);
>
> This and other backslash handling code for some reason is also run on
> linux, so a symlink to eg "ummmm\\git~1" is also warned about.

Right. As far as I can tell, there are very few Linux-only projects left,
so this is in line with many (most?) projects being cross-platform.

> +               if (!buf || size > PATH_MAX) {
>
> I suspect, but have not confirmed, that this is allows a symlink
> target 1 byte longer than the OS supports, because PATH_MAX includes
> a trailing NUL.

True. That condition is basically imitating the `size >
ATTR_MAX_FILE_SIZE` one a couple of lines earlier, but it should be `>=`
here because `PATH_MAX` is supposed to accommodate the trailing NUL.

Ciao,
Johannes

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

Johannes Schindelin wrote:
> > More than one major project; they also broke git-annex in the case where
> > a git-annex repository, which contains symlinks into
> > .git/annex/objects/, is pushed to a bare repository with
> > receive.fsckObjects set. (Gitlab is currently affected[1].)
> 
> This added fsck functionality was specifically marked as `WARN` instead of
> `ERROR`, though. So it should not have failed.

A git push into a bare repository with receive.fsckobjects = true fails:

joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck
receive.fsckobjects=true
joey@darkstar:~/tmp/bench/bar.git>cd ..
joey@darkstar:~/tmp/bench>cd foo
joey@darkstar:~/tmp/bench/foo>git push ../bar.git master
Enumerating objects: 4, done.
Counting objects: 100% (4/4), done.
Delta compression using up to 12 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir
remote: fatal: fsck error in pack objects
error: remote unpack failed: unpack-objects abnormal exit
To ../bar.git
 ! [remote rejected] master -> master (unpacker error)
error: failed to push some refs to '../bar.git'

So I guess that the WARN doesn't work like you expected it to in this case of
receive.fsckobjects checking.

> > This means that symlinks to eg "git~1" are also warned about,
> > which seems strange behavior on eg Linux.
> 
> Only until you realize that there are many cross-platform projects, and
> that Windows Subsystem for Linux is a thing.

I realize that of course, but I also reserve the right to make git repos that
contain files named eg "CON" if I want to. Git should not demand
filename interoperability with arbitrary OSes.

> > +                               backslash = memchr(p, '\\', slash - p);
> >
> > This and other backslash handling code for some reason is also run on
> > linux, so a symlink to eg "ummmm\\git~1" is also warned about.
> 
> Right. As far as I can tell, there are very few Linux-only projects left,
> so this is in line with many (most?) projects being cross-platform.

We may have very different lived experiences then.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2024-05-28 at 02:13:55, Joey Hess wrote:
>> Johannes Schindelin wrote:
>> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.
>> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
>> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir
>> remote: fatal: fsck error in pack objects
>> error: remote unpack failed: unpack-objects abnormal exit
>> To ../bar.git
>>  ! [remote rejected] master -> master (unpacker error)
>> error: failed to push some refs to '../bar.git'
>> 
>> So I guess that the WARN doesn't work like you expected it to in this case of
>> receive.fsckobjects checking.
>
> Then my guess is that this will affect most forges.

FWIW, it is detected as "error" according to the above.

In any case, a33fea08 (fsck: warn about symlink pointing inside a
gitdir, 2024-04-10) adds two ERRORs, in addition to two WARNs:

+	FUNC(SYMLINK_TARGET_MISSING, ERROR) \
+	FUNC(SYMLINK_TARGET_BLOB, ERROR) \
+	FUNC(SYMLINK_TARGET_LENGTH, WARN) \
+	FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \

so "they are only warnings and won't break" is not quite what I see
in the change, but what is causing the above error does look like
the one that is marked as WARN in the patch.

Thanks.