{"thread":{"id":"61475","subject":"[PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","startedAt":"2024-05-14T18:16:46Z","lastAt":"2024-05-30T08:17:26Z","messageCount":14,"participants":["brian m. carlson","Johannes Schindelin","Joey Hess","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"494730","messageId":"20240514181641.150112-1-sandals@crustytoothpaste.net","threadId":"61475","inReplyTo":null,"subject":"[PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-05-14T18:16:39Z","receivedAt":"2024-05-14T18:16:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The recent defense-in-depth patches to restrict hooks while cloning\nbroke Git LFS because it installs necessary hooks when it is invoked by\nGit's smudge filter.  This means that currently, anyone with Git LFS\ninstalled who attempts to clone a repository with at least one LFS file\nwill see a message like the following (fictitious example):\n\n----\n$ git clone https://github.com/octocat/xyzzy.git\nCloning into 'pull-bug'...\nremote: Enumerating objects: 1275, done.\nremote: Counting objects: 100% (343/343), done.\nremote: Compressing objects: 100% (136/136), done.\nremote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932\nReceiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done.\nResolving deltas: 100% (226/226), done.\nFiltering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done.\nfatal: active `post-checkout` hook found during `git clone`:\n        /home/octocat/xyzzy/.git/hooks/post-checkout\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`\nwarning: Clone succeeded, but checkout failed.\nYou can inspect what was checked out with 'git status'\nand retry with 'git restore --source=HEAD :/'\n----\n\nThis causes most CI systems to be broken in such a case, as well as a\nconfusing message for the user.\n\nIt's not really possible to avoid the need to install the hooks at this\nlocation because the post-checkout hook must be ready during the\ncheckout that's part of the clone in order to properly adjust\npermissions on files.  Thus, we'll need to revert the changes to\nrestrict hooks while cloning, which this series does.\n\nbrian m. carlson (2):\n  Revert \"clone: prevent hooks from running during a clone\"\n  Revert \"core.hooksPath: add some protection while cloning\"\n\n builtin/clone.c  |  5 -----\n config.c         | 13 +-----------\n hook.c           | 32 ------------------------------\n t/t1800-hook.sh  | 15 --------------\n t/t5601-clone.sh | 51 ------------------------------------------------\n 5 files changed, 1 insertion(+), 115 deletions(-)\n\n"},{"id":"494731","messageId":"20240514181641.150112-3-sandals@crustytoothpaste.net","threadId":"61475","inReplyTo":"20240514181641.150112-1-sandals@crustytoothpaste.net","subject":"[PATCH 2/2] Revert \"core.hooksPath: add some protection while cloning\"","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-05-14T18:16:41Z","receivedAt":"2024-05-14T18:16:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" <bk2204@github.com>\n\nThe original commit breaks Git LFS, which installs hooks when it is\ninvoked during the smudge process as part of checkout.  This is required\nto install a post-checkout hook that causes files which are set as\nlockable (which are typically large binary assets that cannot be merged)\nto be read-only unless they've been locked.  In addition, Git LFS\nrequires the pre-push hook to be installed so that LFS objects can be\npushed as part of the invocation of git push.\n\nWithout the ability to install these hooks, the locking functionality\nwould not work until the user invoked Git LFS again and did a completely\nnew checkout with all files changed, since Git LFS optimizes for only\nchanged files.  In addition, an invocation of git push might not push\nanything LFS files all to the remote, potentially causing data loss.\n\nNote that this affects all clone operations with a repository with Git\nLFS files in it, even if they are configured not to smudge data by\ndefault, so it breaks all automated clones (which will see \"die\" called)\nwithout the relevant environment variable specified.\n\nRevert this change to restore functionality.\n\nThis reverts commit 20f3588efc6cbcae5bbaabf65ee12df87b51a9ea.\n\nSigned-off-by: brian m. carlson <bk2204@github.com>\n---\n config.c        | 13 +------------\n t/t1800-hook.sh | 15 ---------------\n 2 files changed, 1 insertion(+), 27 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 77a0fd2d80..ae3652b08f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1416,19 +1416,8 @@ static int git_default_core_config(const char *var, const char *value,\n \tif (!strcmp(var, \"core.attributesfile\"))\n \t\treturn git_config_pathname(&git_attributes_file, var, value);\n \n-\tif (!strcmp(var, \"core.hookspath\")) {\n-\t\tif (ctx->kvi && ctx->kvi->scope == CONFIG_SCOPE_LOCAL &&\n-\t\t    git_env_bool(\"GIT_CLONE_PROTECTION_ACTIVE\", 0))\n-\t\t\tdie(_(\"active `core.hooksPath` found in the local \"\n-\t\t\t      \"repository config:\\n\\t%s\\nFor security \"\n-\t\t\t      \"reasons, this is disallowed by default.\\nIf \"\n-\t\t\t      \"this is intentional and the hook should \"\n-\t\t\t      \"actually be run, please\\nrun the command \"\n-\t\t\t      \"again with \"\n-\t\t\t      \"`GIT_CLONE_PROTECTION_ACTIVE=false`\"),\n-\t\t\t    value);\n+\tif (!strcmp(var, \"core.hookspath\"))\n \t\treturn git_config_pathname(&git_hooks_path, var, value);\n-\t}\n \n \tif (!strcmp(var, \"core.bare\")) {\n \t\tis_bare_repository_cfg = git_config_bool(var, value);\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 1894ebeb0e..8b0234cf2d 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -185,19 +185,4 @@ test_expect_success 'stdin to hooks' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'clone protections' '\n-\ttest_config core.hooksPath \"$(pwd)/my-hooks\" &&\n-\tmkdir -p my-hooks &&\n-\twrite_script my-hooks/test-hook <<-\\EOF &&\n-\techo Hook ran $1\n-\tEOF\n-\n-\tgit hook run test-hook 2>err &&\n-\ttest_grep \"Hook ran\" err &&\n-\ttest_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \\\n-\t\tgit hook run test-hook 2>err &&\n-\ttest_grep \"active .core.hooksPath\" err &&\n-\ttest_grep ! \"Hook ran\" err\n-'\n-\n test_done\n"},{"id":"494732","messageId":"20240514181641.150112-2-sandals@crustytoothpaste.net","threadId":"61475","inReplyTo":"20240514181641.150112-1-sandals@crustytoothpaste.net","subject":"[PATCH 1/2] Revert \"clone: prevent hooks from running during a clone\"","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-05-14T18:16:40Z","receivedAt":"2024-05-14T18:16:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" <bk2204@github.com>\n\nThis change breaks Git LFS, which relies on the pre-checkout hook to run\nin a clone.  This is to allow files which have the lockable attribute,\nwhich are usually large binary assets which cannot be merged, to\nbe marked read-only when a user doesn't have a lock so as to prevent\nconflicts.\n\nRevert this change since it causes functional problems.\n\nThis reverts commit 8db1e8743c0f1ed241f6a1b8bf55b6fef07d6751.\n\nSigned-off-by: brian m. carlson <bk2204@github.com>\n---\n builtin/clone.c  |  5 -----\n hook.c           | 32 ------------------------------\n t/t5601-clone.sh | 51 ------------------------------------------------\n 3 files changed, 88 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0e7cf198f5..c66a32feb3 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -987,12 +987,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tusage_msg_opt(_(\"You must specify a repository to clone.\"),\n \t\t\tbuiltin_clone_usage, builtin_clone_options);\n \n-\txsetenv(\"GIT_CLONE_PROTECTION_ACTIVE\", \"true\", 0 /* allow user override */);\n \ttemplate_dir = get_template_dir(option_template);\n-\tif (*template_dir && !is_absolute_path(template_dir))\n-\t\ttemplate_dir = template_dir_dup =\n-\t\t\tabsolute_pathdup(template_dir);\n-\txsetenv(\"GIT_CLONE_TEMPLATE_DIR\", template_dir, 1);\n \n \tif (option_depth || option_since || option_not.nr)\n \t\tdeepen = 1;\ndiff --git a/hook.c b/hook.c\nindex eebc4d4473..f5f46e844e 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -11,30 +11,6 @@\n #include \"setup.h\"\n #include \"copy.h\"\n \n-static int identical_to_template_hook(const char *name, const char *path)\n-{\n-\tconst char *env = getenv(\"GIT_CLONE_TEMPLATE_DIR\");\n-\tconst char *template_dir = get_template_dir(env && *env ? env : NULL);\n-\tstruct strbuf template_path = STRBUF_INIT;\n-\tint found_template_hook, ret;\n-\n-\tstrbuf_addf(&template_path, \"%s/hooks/%s\", template_dir, name);\n-\tfound_template_hook = access(template_path.buf, X_OK) >= 0;\n-#ifdef STRIP_EXTENSION\n-\tif (!found_template_hook) {\n-\t\tstrbuf_addstr(&template_path, STRIP_EXTENSION);\n-\t\tfound_template_hook = access(template_path.buf, X_OK) >= 0;\n-\t}\n-#endif\n-\tif (!found_template_hook)\n-\t\treturn 0;\n-\n-\tret = do_files_match(template_path.buf, path);\n-\n-\tstrbuf_release(&template_path);\n-\treturn ret;\n-}\n-\n const char *find_hook(const char *name)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n@@ -70,14 +46,6 @@ const char *find_hook(const char *name)\n \t\t}\n \t\treturn NULL;\n \t}\n-\tif (!git_hooks_path && git_env_bool(\"GIT_CLONE_PROTECTION_ACTIVE\", 0) &&\n-\t    !identical_to_template_hook(name, path.buf))\n-\t\tdie(_(\"active `%s` hook found during `git clone`:\\n\\t%s\\n\"\n-\t\t      \"For security reasons, this is disallowed by default.\\n\"\n-\t\t      \"If this is intentional and the hook should actually \"\n-\t\t      \"be run, please\\nrun the command again with \"\n-\t\t      \"`GIT_CLONE_PROTECTION_ACTIVE=false`\"),\n-\t\t    name, path.buf);\n \treturn path.buf;\n }\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex deb1c282c7..cc0b953f14 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -788,57 +788,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe\n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client\n '\n \n-test_expect_success 'clone with init.templatedir runs hooks' '\n-\tgit init tmpl/hooks &&\n-\twrite_script tmpl/hooks/post-checkout <<-EOF &&\n-\techo HOOK-RUN >&2\n-\techo I was here >hook.run\n-\tEOF\n-\tgit -C tmpl/hooks add . &&\n-\ttest_tick &&\n-\tgit -C tmpl/hooks commit -m post-checkout &&\n-\n-\ttest_when_finished \"git config --global --unset init.templateDir || :\" &&\n-\ttest_when_finished \"git config --unset init.templateDir || :\" &&\n-\t(\n-\t\tsane_unset GIT_TEMPLATE_DIR &&\n-\t\tNO_SET_GIT_TEMPLATE_DIR=t &&\n-\t\texport NO_SET_GIT_TEMPLATE_DIR &&\n-\n-\t\tgit -c core.hooksPath=\"$(pwd)/tmpl/hooks\" \\\n-\t\t\tclone tmpl/hooks hook-run-hookspath 2>err &&\n-\t\ttest_grep ! \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-hookspath/hook.run &&\n-\n-\t\tgit -c init.templateDir=\"$(pwd)/tmpl\" \\\n-\t\t\tclone tmpl/hooks hook-run-config 2>err &&\n-\t\ttest_grep ! \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-config/hook.run &&\n-\n-\t\tgit clone --template=tmpl tmpl/hooks hook-run-option 2>err &&\n-\t\ttest_grep ! \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-option/hook.run &&\n-\n-\t\tgit config --global init.templateDir \"$(pwd)/tmpl\" &&\n-\t\tgit clone tmpl/hooks hook-run-global-config 2>err &&\n-\t\tgit config --global --unset init.templateDir &&\n-\t\ttest_grep ! \"active .* hook found\" err &&\n-\t\ttest_path_is_file hook-run-global-config/hook.run &&\n-\n-\t\t# clone ignores local `init.templateDir`; need to create\n-\t\t# a new repository because we deleted `.git/` in the\n-\t\t# `setup` test case above\n-\t\tgit init local-clone &&\n-\t\tcd local-clone &&\n-\n-\t\tgit config init.templateDir \"$(pwd)/../tmpl\" &&\n-\t\tgit clone ../tmpl/hooks hook-run-local-config 2>err &&\n-\t\tgit config --unset init.templateDir &&\n-\t\ttest_grep ! \"active .* hook found\" err &&\n-\t\ttest_path_is_missing hook-run-local-config/hook.run\n-\t)\n-'\n-\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n"},{"id":"494733","messageId":"0f7597aa-6697-9a70-0405-3dcbb9649d68@gmx.de","threadId":"61475","inReplyTo":"20240514181641.150112-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-05-14T19:07:28Z","receivedAt":"2024-05-14T19:07:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"brian,\n\nOn Tue, 14 May 2024, brian m. carlson wrote:\n\n> The recent defense-in-depth patches to restrict hooks while cloning\n> broke Git LFS because it installs necessary hooks when it is invoked by\n> Git's smudge filter.  This means that currently, anyone with Git LFS\n> installed who attempts to clone a repository with at least one LFS file\n> will see a message like the following (fictitious example):\n>\n> ----\n> $ git clone https://github.com/octocat/xyzzy.git\n> Cloning into 'pull-bug'...\n> remote: Enumerating objects: 1275, done.\n> remote: Counting objects: 100% (343/343), done.\n> remote: Compressing objects: 100% (136/136), done.\n> remote: Total 1275 (delta 221), reused 327 (delta 206), pack-reused 932\n> Receiving objects: 100% (1275/1275), 290.78 KiB | 2.88 MiB/s, done.\n> Resolving deltas: 100% (226/226), done.\n> Filtering content: 100% (504/504), 1.86 KiB | 0 bytes/s, done.\n> fatal: active `post-checkout` hook found during `git clone`:\n>         /home/octocat/xyzzy/.git/hooks/post-checkout\n> For security reasons, this is disallowed by default.\n> If this is intentional and the hook should actually be run, please\n> run the command again with `GIT_CLONE_PROTECTION_ACTIVE=false`\n> warning: Clone succeeded, but checkout failed.\n> You can inspect what was checked out with 'git status'\n> and retry with 'git restore --source=HEAD :/'\n> ----\n>\n> This causes most CI systems to be broken in such a case, as well as a\n> confusing message for the user.\n\nWhen using `actions/checkout` in GitHub workflows, nothing is broken\nbecause `actions/checkout` uses a fetch + checkout (to allow for things\nlike sparse checkout), which obviously lacks the clone protections because\nit is not a clone.\n\n> It's not really possible to avoid the need to install the hooks at this\n> location because the post-checkout hook must be ready during the\n> checkout that's part of the clone in order to properly adjust\n> permissions on files.  Thus, we'll need to revert the changes to\n> restrict hooks while cloning, which this series does.\n\nDropping protections is in general a bad idea. While previously, hackers\nwishing to exploit weaknesses in Git might have been unaware of the\nparticular attack vector we want to prevent with these defense-in-depth\nmeasurements, we now must assume that they are fully aware. Reverting\nthose protections can be seen as a very public invitation to search for\nways to exploit the now re-introduced avenues to craft Remote Code\nExecution attacks.\n\nI have pointed out several times that there are alternatives while\ndiscussing this under embargo, even sent them to the git-security list\nbefore the embargo was lifted, and have not received any reply. One\nproposal was to introduce a way to cross-check the SHA-256 of hooks that\n_were_ written during a clone operation against a list of known-good ones.\nAnother alternative was to special-case Git LFS by matching the hooks'\ncontents against a regular expression that matches Git LFS' current\nhooks'.\n\nBoth alternatives demonstrate that we are far from _needing_ to revert the\nchanges that were designed to prevent future vulnerabilities from\nimmediately becoming critical Remote Code Executions. It might be an\neasier way to address the Git LFS breakage, but \"easy\" does not equal\n\"right\".\n\nI did not yet get around to sending these patches to the Git mailing list\nsolely because I am still busy with a lot of follow-up work of the\nembargoed release. It was an unwelcome surprise to see this here patch\nseries in my inbox and still no reply to the patches I had sent to the\ngit-security list for comments.\n\nI am still busy wrapping up follow-up work and won't be able to\nparticipate in this here mail thread meaningfully for the next hours. I do\nwant to invite you to think about alternative ways to address the Git LFS\nissues, alternatives that do not re-open weaknesses we had hoped to\naddress for good.\n\nI do want to extend the invitation to work with me on that, for example by\nreviewing those patches I sent to the git-security mailing list (or even\nto send them to the Git mailing list for public review on my behalf, that\nwould be helpful).\n\nCiao,\nJohannes\n"},{"id":"494736","messageId":"ZkO-b6Nswrn9H7Ed@tapette.crustytoothpaste.net","threadId":"61475","inReplyTo":"0f7597aa-6697-9a70-0405-3dcbb9649d68@gmx.de","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-05-14T19:41:35Z","receivedAt":"2024-05-14T19:41:37Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-05-14 at 19:07:28, Johannes Schindelin wrote:\n> brian,\n> \n> On Tue, 14 May 2024, brian m. carlson wrote:\n> > This causes most CI systems to be broken in such a case, as well as a\n> > confusing message for the user.\n> \n> When using `actions/checkout` in GitHub workflows, nothing is broken\n> because `actions/checkout` uses a fetch + checkout (to allow for things\n> like sparse checkout), which obviously lacks the clone protections because\n> it is not a clone.\n\nI'm not saying all CI systems do this, but we probably have broken quite\na few.\n\n> > It's not really possible to avoid the need to install the hooks at this\n> > location because the post-checkout hook must be ready during the\n> > checkout that's part of the clone in order to properly adjust\n> > permissions on files.  Thus, we'll need to revert the changes to\n> > restrict hooks while cloning, which this series does.\n> \n> Dropping protections is in general a bad idea. While previously, hackers\n> wishing to exploit weaknesses in Git might have been unaware of the\n> particular attack vector we want to prevent with these defense-in-depth\n> measurements, we now must assume that they are fully aware. Reverting\n> those protections can be seen as a very public invitation to search for\n> ways to exploit the now re-introduced avenues to craft Remote Code\n> Execution attacks.\n\nIf these protections hadn't broken things, I'd agree that we should keep\nthem.  However, they have broken things and they've introduced a\nserious regression breaking a major project, and we should revert them.\n\nI'm not opposed to us exploring other alternatives for defense in depth\nmeasures on the list.  I definitely think such work is valuable and\nimportant, but I want to make sure the changes we make don't regress\nimportant functionality.\n\n> I have pointed out several times that there are alternatives while\n> discussing this under embargo, even sent them to the git-security list\n> before the embargo was lifted, and have not received any reply. One\n> proposal was to introduce a way to cross-check the SHA-256 of hooks that\n> _were_ written during a clone operation against a list of known-good ones.\n> Another alternative was to special-case Git LFS by matching the hooks'\n> contents against a regular expression that matches Git LFS' current\n> hooks'.\n\nI have replied to those on the security list and to the general idea.  I\ndon't think we should special-case Git LFS here.  That's antithetical to\nthe long-standing ethos of the project.  While Git LFS is _one_ known\nproject to have been broken, there may be others, or people may have\nforks of Git LFS under other names, and we want that tooling to be able\nto take advantage of all of the same features.\n\nI'm also opposed to any kind of pinning of the Git LFS hooks to Git in\ngeneral, whatever the approach is.  Git LFS runs against multiple\nversions of Git in our CI, and if we change the hooks in a way that Git\nhasn't pinned, either via SHA-256 hash or regex, then Git LFS (and its\nCI) is broken until Git gets an update.  We've already had to deal with\nsmall incompatible changes that have broken Git LFS, and I don't think\ncoupling the projects in this way is a good idea.\n\nFinally, it's very easy to work around the protections by merely placing\na malicious binary called \"git-lfs\" earlier in the PATH, so we're not\nnecessarily getting a substantial amount of improved security by\nrequiring that binary.\n\n> Both alternatives demonstrate that we are far from _needing_ to revert the\n> changes that were designed to prevent future vulnerabilities from\n> immediately becoming critical Remote Code Executions. It might be an\n> easier way to address the Git LFS breakage, but \"easy\" does not equal\n> \"right\".\n> \n> I did not yet get around to sending these patches to the Git mailing list\n> solely because I am still busy with a lot of follow-up work of the\n> embargoed release. It was an unwelcome surprise to see this here patch\n> series in my inbox and still no reply to the patches I had sent to the\n> git-security list for comments.\n\nAs you may know, I don't read the git or git-security lists except on\nmy personal computer using my personal email, and I have been at work\nmost of the day putting out fires related to the Git LFS breakage above.\nThus, I haven't seen them until just recently, when I did try to get a\nreply out.\n\n> I am still busy wrapping up follow-up work and won't be able to\n> participate in this here mail thread meaningfully for the next hours. I do\n> want to invite you to think about alternative ways to address the Git LFS\n> issues, alternatives that do not re-open weaknesses we had hoped to\n> address for good.\n\nI don't want to put undue pressure on you.  Please feel free to respond\nor not at your leisure.\n\n> I do want to extend the invitation to work with me on that, for example by\n> reviewing those patches I sent to the git-security mailing list (or even\n> to send them to the Git mailing list for public review on my behalf, that\n> would be helpful).\n\nI think the substance of my response was the same one that I gave above.\n\nWith all due respect, I don't think your patches are the right way to\naddress the problem, and I fear that by bringing them to the list, I'd\nbe giving the list the misleading impression that I endorsed them with\nmy sign-off.  While I respect you as a colleague and a contributor, even\nif we may disagree on this or other issues, I don't agree with the\napproach in the patches, and I don't think it would be a good idea to\napply my sign-off in sending them.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"495293","messageId":"Zk2_mJpE7tJgqxSp@kitenet.net","threadId":"61475","inReplyTo":"ZkO-b6Nswrn9H7Ed@tapette.crustytoothpaste.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2024-05-22T09:49:12Z","receivedAt":"2024-05-22T10:32:14Z","isPatch":true,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"brian m. carlson wrote:\n> If these protections hadn't broken things, I'd agree that we should keep\n> them.  However, they have broken things and they've introduced a\n> serious regression breaking a major project, and we should revert them.\n\nMore than one major project; they also broke git-annex in the case where\na git-annex repository, which contains symlinks into\n.git/annex/objects/, is pushed to a bare repository with\nreceive.fsckObjects set. (Gitlab is currently affected[1].)\n\n\nBTW, do I understand correctly that the defence in depth patch set was\ndeveloped under embargo and has never been publically reviewed?\n\nLooking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed\nthat its PATH_MAX check is also dodgy due to that having values ranging\nfrom 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git\nrepositories containing legitimate, working symlinks can now fail to be\npushed depending on what OS happens to host a reciving bare repository.\n\n+                               if (is_ntfs_dotgit(p))\n\nThis means that symlinks to eg \"git~1\" are also warned about,\nwhich seems strange behavior on eg Linux.\n\n+                               backslash = memchr(p, '\\\\', slash - p);\n\nThis and other backslash handling code for some reason is also run on\nlinux, so a symlink to eg \"ummmm\\\\git~1\" is also warned about.\n\n+               if (!buf || size > PATH_MAX) {\n\nI suspect, but have not confirmed, that this is allows a symlink\ntarget 1 byte longer than the OS supports, because PATH_MAX includes\na trailing NUL.\n\n\nAll in all, this seems to need more review and a more careful\nconsideration of breakage now that the security holes are not under\nembargo.\n\n-- \nsee shy jo\n\n[1] https://forum.gitlab.com/t/recent-git-v2-45-1-breaks-git-annex-compatibility-because-of-apparent-fsck-symlinkpointstogitdir-error-on-gitlab/104909\n"},{"id":"495580","messageId":"ZlDQdXh5i3MCjTmr@kitenet.net","threadId":"61475","inReplyTo":"ZkO-b6Nswrn9H7Ed@tapette.crustytoothpaste.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2024-05-24T17:37:57Z","receivedAt":"2024-05-24T17:38:07Z","isPatch":true,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"brian m. carlson wrote:\n> > proposal was to introduce a way to cross-check the SHA-256 of hooks that\n> > _were_ written during a clone operation against a list of known-good ones.\n> > Another alternative was to special-case Git LFS by matching the hooks'\n> > contents against a regular expression that matches Git LFS' current\n> > hooks'.\n> \n> I have replied to those on the security list and to the general idea.  I\n> don't think we should special-case Git LFS here.  That's antithetical to\n> the long-standing ethos of the project.\n\nI was surprised today to find that git-annex also triggers the hook\nproblem. In particular, a git clone that uses git-remote-annex can\ncause several hooks to get created.\n\nI think the hook check is already scheduled for reversion, but in case\nnot, here's another data point against hard-coding known-good hooks as a\nsolution.\n\n-- \nsee shy jo\n"},{"id":"495706","messageId":"fbb89826-0d83-d4f9-bab4-9fba69e0e22d@gmx.de","threadId":"61475","inReplyTo":"Zk2_mJpE7tJgqxSp@kitenet.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-05-27T19:35:03Z","receivedAt":"2024-05-27T19:35:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Joey,\n\nOn Wed, 22 May 2024, Joey Hess wrote:\n\n> brian m. carlson wrote:\n> > If these protections hadn't broken things, I'd agree that we should keep\n> > them.  However, they have broken things and they've introduced a\n> > serious regression breaking a major project, and we should revert them.\n>\n> More than one major project; they also broke git-annex in the case where\n> a git-annex repository, which contains symlinks into\n> .git/annex/objects/, is pushed to a bare repository with\n> receive.fsckObjects set. (Gitlab is currently affected[1].)\n\nThis added fsck functionality was specifically marked as `WARN` instead of\n`ERROR`, though. So it should not have failed.\n\n> BTW, do I understand correctly that the defence in depth patch set was\n> developed under embargo and has never been publically reviewed?\n>\n> Looking at commit a33fea0886cfa016d313d2bd66bdd08615bffbc9, I noticed\n> that its PATH_MAX check is also dodgy due to that having values ranging\n> from 260 (Windows) to 1024 (Freebsd) to 4096 (Linux), which means git\n> repositories containing legitimate, working symlinks can now fail to be\n> pushed depending on what OS happens to host a reciving bare repository.\n\nLikewise, this fsck functionality was specifically marked as `WARN`\ninstead of `ERROR`, to prevent exactly the issue you are seeing.\n\nAre you saying that Gitlab is upgrading fsck warnings to errors? If so, I\nfear we need to ask Gitlab to stop doing that.\n\n> +                               if (is_ntfs_dotgit(p))\n>\n> This means that symlinks to eg \"git~1\" are also warned about,\n> which seems strange behavior on eg Linux.\n\nOnly until you realize that there are many cross-platform projects, and\nthat Windows Subsystem for Linux is a thing.\n\n> +                               backslash = memchr(p, '\\\\', slash - p);\n>\n> This and other backslash handling code for some reason is also run on\n> linux, so a symlink to eg \"ummmm\\\\git~1\" is also warned about.\n\nRight. As far as I can tell, there are very few Linux-only projects left,\nso this is in line with many (most?) projects being cross-platform.\n\n> +               if (!buf || size > PATH_MAX) {\n>\n> I suspect, but have not confirmed, that this is allows a symlink\n> target 1 byte longer than the OS supports, because PATH_MAX includes\n> a trailing NUL.\n\nTrue. That condition is basically imitating the `size >\nATTR_MAX_FILE_SIZE` one a couple of lines earlier, but it should be `>=`\nhere because `PATH_MAX` is supposed to accommodate the trailing NUL.\n\nCiao,\nJohannes\n"},{"id":"495712","messageId":"ZlU94wcstaAHv_HZ@kitenet.net","threadId":"61475","inReplyTo":"fbb89826-0d83-d4f9-bab4-9fba69e0e22d@gmx.de","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2024-05-28T02:13:55Z","receivedAt":"2024-05-28T02:14:03Z","isPatch":true,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Johannes Schindelin wrote:\n> > More than one major project; they also broke git-annex in the case where\n> > a git-annex repository, which contains symlinks into\n> > .git/annex/objects/, is pushed to a bare repository with\n> > receive.fsckObjects set. (Gitlab is currently affected[1].)\n> \n> This added fsck functionality was specifically marked as `WARN` instead of\n> `ERROR`, though. So it should not have failed.\n\nA git push into a bare repository with receive.fsckobjects = true fails:\n\njoey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck\nreceive.fsckobjects=true\njoey@darkstar:~/tmp/bench/bar.git>cd ..\njoey@darkstar:~/tmp/bench>cd foo\njoey@darkstar:~/tmp/bench/foo>git push ../bar.git master\nEnumerating objects: 4, done.\nCounting objects: 100% (4/4), done.\nDelta compression using up to 12 threads\nCompressing objects: 100% (2/2), done.\nWriting objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.\nTotal 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)\nremote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir\nremote: fatal: fsck error in pack objects\nerror: remote unpack failed: unpack-objects abnormal exit\nTo ../bar.git\n ! [remote rejected] master -> master (unpacker error)\nerror: failed to push some refs to '../bar.git'\n\nSo I guess that the WARN doesn't work like you expected it to in this case of\nreceive.fsckobjects checking.\n\n> > This means that symlinks to eg \"git~1\" are also warned about,\n> > which seems strange behavior on eg Linux.\n> \n> Only until you realize that there are many cross-platform projects, and\n> that Windows Subsystem for Linux is a thing.\n\nI realize that of course, but I also reserve the right to make git repos that\ncontain files named eg \"CON\" if I want to. Git should not demand\nfilename interoperability with arbitrary OSes.\n\n> > +                               backslash = memchr(p, '\\\\', slash - p);\n> >\n> > This and other backslash handling code for some reason is also run on\n> > linux, so a symlink to eg \"ummmm\\\\git~1\" is also warned about.\n> \n> Right. As far as I can tell, there are very few Linux-only projects left,\n> so this is in line with many (most?) projects being cross-platform.\n\nWe may have very different lived experiences then.\n\n-- \nsee shy jo\n"},{"id":"495772","messageId":"xmqq7cfd33f6.fsf@gitster.g","threadId":"61475","inReplyTo":"ZlZSZ1-0F2DEp9yV@tapette.crustytoothpaste.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T23:46:53Z","receivedAt":"2024-05-28T23:46:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2024-05-28 at 02:13:55, Joey Hess wrote:\n>> Johannes Schindelin wrote:\n>> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.\n>> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)\n>> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir\n>> remote: fatal: fsck error in pack objects\n>> error: remote unpack failed: unpack-objects abnormal exit\n>> To ../bar.git\n>>  ! [remote rejected] master -> master (unpacker error)\n>> error: failed to push some refs to '../bar.git'\n>> \n>> So I guess that the WARN doesn't work like you expected it to in this case of\n>> receive.fsckobjects checking.\n>\n> Then my guess is that this will affect most forges.\n\nFWIW, it is detected as \"error\" according to the above.\n\nIn any case, a33fea08 (fsck: warn about symlink pointing inside a\ngitdir, 2024-04-10) adds two ERRORs, in addition to two WARNs:\n\n+\tFUNC(SYMLINK_TARGET_MISSING, ERROR) \\\n+\tFUNC(SYMLINK_TARGET_BLOB, ERROR) \\\n+\tFUNC(SYMLINK_TARGET_LENGTH, WARN) \\\n+\tFUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \\\n\nso \"they are only warnings and won't break\" is not quite what I see\nin the change, but what is causing the above error does look like\nthe one that is marked as WARN in the patch.\n\nThanks.\n\n"},{"id":"495799","messageId":"20240529085401.GA1098944@coredump.intra.peff.net","threadId":"61475","inReplyTo":"ZlU94wcstaAHv_HZ@kitenet.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-29T08:54:01Z","receivedAt":"2024-05-29T08:54:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 27, 2024 at 10:13:55PM -0400, Joey Hess wrote:\n\n> Johannes Schindelin wrote:\n> > > More than one major project; they also broke git-annex in the case where\n> > > a git-annex repository, which contains symlinks into\n> > > .git/annex/objects/, is pushed to a bare repository with\n> > > receive.fsckObjects set. (Gitlab is currently affected[1].)\n> > \n> > This added fsck functionality was specifically marked as `WARN` instead of\n> > `ERROR`, though. So it should not have failed.\n> \n> A git push into a bare repository with receive.fsckobjects = true fails:\n> \n> joey@darkstar:~/tmp/bench/bar.git>git config --list |grep fsck\n> receive.fsckobjects=true\n> joey@darkstar:~/tmp/bench/bar.git>cd ..\n> joey@darkstar:~/tmp/bench>cd foo\n> joey@darkstar:~/tmp/bench/foo>git push ../bar.git master\n> Enumerating objects: 4, done.\n> Counting objects: 100% (4/4), done.\n> Delta compression using up to 12 threads\n> Compressing objects: 100% (2/2), done.\n> Writing objects: 100% (3/3), 324 bytes | 324.00 KiB/s, done.\n> Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)\n> remote: error: object ea461949b973a70f2163bb501b9d74652bde9e30: symlinkPointsToGitDir: symlink target points to git dir\n> remote: fatal: fsck error in pack objects\n> error: remote unpack failed: unpack-objects abnormal exit\n> To ../bar.git\n>  ! [remote rejected] master -> master (unpacker error)\n> error: failed to push some refs to '../bar.git'\n> \n> So I guess that the WARN doesn't work like you expected it to in this case of\n> receive.fsckobjects checking.\n\nThis is a long-standing weirdness with the fsck severities. The\nfsck_options struct used for fetches/pushes has the \"strict\" flag set,\nwhich upgrades warnings to errors. But if you manually configure a\nseverity to \"warn\", then we respect that.\n\nFor example, try:\n\n  git init\n  git commit --allow-empty -m 'message with NUL'\n  commit=$(git cat-file commit HEAD |\n         perl -pe 's/NUL/\\0/' |\n\t git hash-object -w --stdin -t commit --literally)\n  git update-ref HEAD $commit\n\nwhich is defined as WARN in fsck.h. And hence:\n\n  $ git fsck; echo $?\n  warning in commit 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body\n  Checking object directories: 100% (256/256), done.\n  0\n\nBut that's upgraded to ERROR for transfers:\n\n  $ git init --bare dst.git\n  $ git -C dst.git config transfer.fsckObjects true\n  $ git push dst.git\n  ...\n  remote: error: object e6db180f21250e03b633a3684f593ceb7b9cd844: nulInCommit: NUL byte in the commit object body\n  remote: fatal: fsck error in packed object\n  error: remote unpack failed: unpack-objects abnormal exit\n  To dst.git\n   ! [remote rejected] main -> main (unpacker error)\n  error: failed to push some refs to 'dst.git'\n\nUnless we override it:\n\n  $ git -C dst.git config receive.fsck.nulInCommit warn\n  $ git push dst.git\n  remote: warning: object 09b2d5bda87ffda7a0f36ea80c4b542edf9b9374: nulInCommit: NUL byte in the commit object body\n  To dst.git\n   * [new branch]      main -> main\n\nBut of course most sites just use the defaults, so all warnings are\neffectively errors.\n\nI think it's been this way at least since c99ba492f1 (fsck: introduce\nidentifiers for fsck messages, 2015-06-22). We've discussed it once or\ntwice on the list. It mostly seemed like a cosmetic issue to me, but in\nthis case it looks like it caused functional confusion.\n\nI don't think just turning off the \"strict\" flag is a good idea, though.\nThe current severities are all over the place. A missing space in an\nident line is an error, but a tree with a \".git\" directory is just a\nwarning!\n\nSo I think we'd first want to straighten out the severities, and then\nthink about letting warnings bypass transfer fscks. Though it's not\nclear to me what hosters would want; pushing to a public site is a great\ntime to let people know their objects are broken _before_ everyone else\nsees them, even if it's \"just\" a warning. But when you do have old\nhistory with broken objects, the control and incentives are in the wrong\nplace; every person who wants to interact with the repo has to loosen\ntheir fsck config. So it's not clear to me how aggressive transfer-level\nfsck-ing should be.\n\nIn the meantime, we also have an \"INFO\" severity which gets reported but\nnot upgraded via strict. It sounds like that's what was intended here.\nIt should be available in all backport versions if we want it; it was\nintroduced in f27d05b170 (fsck: allow upgrading fsck warnings to errors,\n2015-06-22).\n\n-Peff\n"},{"id":"495812","messageId":"1cbdeb41-2ad3-05e4-ab27-1f84086b7f43@gmx.de","threadId":"61475","inReplyTo":"20240529085401.GA1098944@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-05-29T12:17:41Z","receivedAt":"2024-05-29T12:18:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jeff,\n\nOn Wed, 29 May 2024, Jeff King wrote:\n\n> [...] But of course most sites just use the defaults, so all warnings\n> are effectively errors.\n\nI wish that had been pointed out on the git-security mailing list when I\noffered this patch up for review.\n\n> In the meantime, we also have an \"INFO\" severity which gets reported but\n> not upgraded via strict. It sounds like that's what was intended here.\n\nPrecisely.\n\nSo this is what the fix-up patch would look like to make the code match my\nintention:\n\n-- snipsnap --\nSubject: [PATCH] fsck: demote the newly-introduced symlink issues from WARN -> IGNORE\n\nThe idea of the symlink check to prevent overly-long symlink targets and\ntargets inside the `.git/` directory was to _warn_, but not to prevent\nany operation.\n\nHowever, that's not how Git works, I was confused by the label `WARN`.\nWhat we need instead is the `IGNORE` label, which still warns\n(confusingly so ;-)), but does not prevent any operations from\ncontinuing.\n\nAdjust t1450 accordingly, documenting that `git fsck` unfortunately no\nlonger warns about these issues by default.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/fsck-msgids.txt |  4 ++--\n fsck.h                        |  4 ++--\n t/t1450-fsck.sh               | 13 ++++++++++++-\n 3 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt\nindex b06ec385aff..f5016ecda6a 100644\n--- a/Documentation/fsck-msgids.txt\n+++ b/Documentation/fsck-msgids.txt\n@@ -158,13 +158,13 @@\n \t(WARN) Tree contains entries pointing to a null sha1.\n\n `symlinkPointsToGitDir`::\n-\t(WARN) Symbolic link points inside a gitdir.\n+\t(INFO) Symbolic link points inside a gitdir.\n\n `symlinkTargetBlob`::\n \t(ERROR) A non-blob found instead of a symbolic link's target.\n\n `symlinkTargetLength`::\n-\t(WARN) Symbolic link target longer than maximum path length.\n+\t(INFO) Symbolic link target longer than maximum path length.\n\n `symlinkTargetMissing`::\n \t(ERROR) Unable to read symbolic link target's blob.\ndiff --git a/fsck.h b/fsck.h\nindex 130fa8d8f91..d41ec98064b 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -74,8 +74,6 @@ enum fsck_msg_type {\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n-\tFUNC(SYMLINK_TARGET_LENGTH, WARN) \\\n-\tFUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_FILEMODE, INFO) \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n@@ -84,6 +82,8 @@ enum fsck_msg_type {\n \tFUNC(MAILMAP_SYMLINK, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n+\tFUNC(SYMLINK_TARGET_LENGTH, INFO) \\\n+\tFUNC(SYMLINK_POINTS_TO_GIT_DIR, INFO) \\\n \t/* ignored (elevated when requested) */ \\\n \tFUNC(EXTRA_HEADER_ENTRY, IGNORE)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 5669872bc80..8339e60efb2 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -1032,7 +1032,18 @@ test_expect_success 'fsck warning on symlink target with excessive length' '\n \twarning in blob $symlink_target: symlinkTargetLength: symlink target too long\n \tEOF\n \tgit fsck --no-dangling >actual 2>&1 &&\n-\ttest_cmp expected actual\n+\ttest_cmp expected actual &&\n+\n+\ttest_when_finished \"git tag -d symlink-target-length\" &&\n+\tgit tag symlink-target-length $tree &&\n+\ttest_when_finished \"rm -rf throwaway.git\" &&\n+\tgit init --bare throwaway.git &&\n+\tgit --git-dir=throwaway.git config receive.fsckObjects true &&\n+\tgit --git-dir=throwaway.git config receive.fsck.symlinkTargetLength error &&\n+\ttest_must_fail git push throwaway.git symlink-target-length &&\n+\tgit --git-dir=throwaway.git config --unset receive.fsck.symlinkTargetLength &&\n+\tgit push throwaway.git symlink-target-length 2>err &&\n+\tgrep \"warning.*symlinkTargetLength\" err\n '\n\n test_expect_success 'fsck warning on symlink target pointing inside git dir' '\n"},{"id":"495840","messageId":"xmqq4jagzj6v.fsf@gitster.g","threadId":"61475","inReplyTo":"1cbdeb41-2ad3-05e4-ab27-1f84086b7f43@gmx.de","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-29T16:17:28Z","receivedAt":"2024-05-29T16:17:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Wed, 29 May 2024, Jeff King wrote:\n>\n>> [...] But of course most sites just use the defaults, so all warnings\n>> are effectively errors.\n>\n> I wish that had been pointed out on the git-security mailing list when I\n> offered this patch up for review.\n\nI sympathize with the sentiment, but there are things that becomes\nmuch clearer once you know what to look for by getting specific\ncomplaints, and I am sure that you would have come to \"ah, there is\nthis strict thing in addition to the msg_type\" yourself, without\nanybody pointing it out to you, once you looked, if we had Joey's\nreport while working on the patch.  I would have noticed it with a\nbreakage example back when the patch was first floated on the\nsecurity list, but of course I didn't, because the patch was only on\nthe security list without wider testers.\n\nThe take home lesson from this episode should not be \"people should\nspeak up more in the security list\".  It instead is \"let's try to\nlimit the work under embargo to absolute minimum, and work in the\nopen for anything on top\".\n\n\"We saw an issue that we followed a symlink when we shouldn't, which\nwe are going to fix here, but it became high severity because of\nwhere that symlink pointed at\" may be a valid sentiment to have, but\nwe should stop at \"fixing\" it under embargo, and addressing the \"but\n... because\" issue on top is better done in the open.  Even if we\npropose \"let's not allow symlink at all---that way even if we wrote\nthrough symlinks by mistake, we won't damage anything\", there will\nbe more people to correct us when we worked in the open.\n\nIn any case, let's clean up the mess we created in 2.45.1 and\nfriends quickly to prepare a solid foundation to allow us do\nadditional work on top.  The reverts are in 'next' and I plan to\nmerge it down to 'master', which hopefully allows us to do the\nfollow up releases soonish.\n"},{"id":"495903","messageId":"20240530081725.GH1949834@coredump.intra.peff.net","threadId":"61475","inReplyTo":"1cbdeb41-2ad3-05e4-ab27-1f84086b7f43@gmx.de","subject":"Re: [PATCH 0/2] Revert defense-in-depth patches breaking Git LFS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-30T08:17:25Z","receivedAt":"2024-05-30T08:17:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2024 at 02:17:41PM +0200, Johannes Schindelin wrote:\n\n> On Wed, 29 May 2024, Jeff King wrote:\n> \n> > [...] But of course most sites just use the defaults, so all warnings\n> > are effectively errors.\n> \n> I wish that had been pointed out on the git-security mailing list when I\n> offered this patch up for review.\n\nMe too. But I agree with everything Junio already responded here.\n\n> So this is what the fix-up patch would look like to make the code match my\n> intention:\n> \n> -- snipsnap --\n> Subject: [PATCH] fsck: demote the newly-introduced symlink issues from WARN -> IGNORE\n\nI think you mean s/IGNORE/INFO/ here and elsewhere in the commit\nmessage? The actual code change looks correct.\n\n-Peff\n"}]}