{"thread":{"id":"54345","subject":"[PATCH 0/7] forbidding symlinked .gitattributes and .gitignore","startedAt":"2020-10-05T07:17:53Z","lastAt":"2020-10-28T23:08:43Z","messageCount":55,"participants":["Jeff King","Jonathan Nieder","Johannes Schindelin","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"406894","messageId":"20201005071751.GA2290770@coredump.intra.peff.net","threadId":"54345","inReplyTo":null,"subject":"[PATCH 0/7] forbidding symlinked .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:17:51Z","receivedAt":"2020-10-05T07:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"About 2 years ago as part of a security release we made it illegal to\nhave a symlinked .gitmodules file (refusing it both in the index and via\nfsck). At the time we discussed (on the security list) outlawing\nsymlinks for other .git files in the same way, but we decided not to do\nso as part of the security release, as it wasn't strictly necessary.\n\nWe publicly revisited the topic in:\n\n https://lore.kernel.org/git/20190114230902.GG162110@google.com/\n\nbut there were a few fixes needed, and it got forgotten. So here it is\nagain, with those fixes:\n\n [1/7]: fsck_tree(): fix shadowed variable\n [2/7]: fsck_tree(): wrap some long lines\n\n These first two are actually an unrelated fix and cleanup in the\n nearby code. Could be picked up independently.\n\n [3/7]: t7415: rename to expand scope\n [4/7]: t7450: test verify_path() handling of gitmodules\n\n Preparatory test cleanup and improvement for existing features.\n\n [5/7]: t0060: test obscured .gitattributes and .gitignore matching\n [6/7]: verify_path(): disallow symlinks in .gitattributes and .gitignore\n [7/7]: fsck: complain when .gitattributes or .gitignore is a symlink\n\n The actual feature, covering the index and fsck.\n\n fsck.c | 79 ++++++++++++++-----\n read-cache.c | 12 ++-\n t/helper/test-path-utils.c | 41 +++++++---\n t/t0060-path-utils.sh | 20 +++++\n ...odule-names.sh => t7450-bad-meta-files.sh} | 69 ++++++++++++++--\n 5 files changed, 179 insertions(+), 42 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (77%)\n\n-Peff\n"},{"id":"406895","messageId":"20201005071905.GA2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 1/7] fsck_tree(): fix shadowed variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:19:05Z","receivedAt":"2020-10-05T07:19:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\nfsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\nfsck_tree(), and we pass it to the report() function when we find\nproblems. However, that is shadowed within the tree-walking loop by the\nexisting \"oid\" variable which we use to store the oid of each tree\nentry. As a result, we may report the wrong oid for some problems we\ndetect within the loop (the entry oid, instead of the tree oid).\n\nOur tests didn't catch this because they checked only that we found the\nexpected fsck problem, not that it was attached to the correct object.\n\nLet's rename both variables in the function to avoid confusion. This\nmakes the diff a little noisy (e.g., all of the report() calls outside\nthe loop wee already correct but need touched), but makes sure we catch\nall cases and will avoid similar confusion in the future.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 40 +++++++++++++++++++-------------------\n t/t7415-submodule-names.sh | 5 +++--\n 2 files changed, 23 insertions(+), 22 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f82e2fe9e3..46a108839f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -633,7 +633,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(const struct object_id *oid,\n+static int fsck_tree(const struct object_id *tree_oid,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -654,7 +654,7 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -664,11 +664,11 @@ static int fsck_tree(const struct object_id *oid,\n \twhile (desc.size) {\n \t\tunsigned short mode;\n \t\tconst char *name, *backslash;\n-\t\tconst struct object_id *oid;\n+\t\tconst struct object_id *entry_oid;\n \n-\t\toid = tree_entry_extract(&desc, &name, &mode);\n+\t\tentry_oid = tree_entry_extract(&desc, &name, &mode);\n \n-\t\thas_null_sha1 |= is_null_oid(oid);\n+\t\thas_null_sha1 |= is_null_oid(entry_oid);\n \t\thas_full_path |= !!strchr(name, '/');\n \t\thas_empty_name |= !*name;\n \t\thas_dot |= !strcmp(name, \".\");\n@@ -678,10 +678,10 @@ static int fsck_tree(const struct object_id *oid,\n \n \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n \t\t\tif (!S_ISLNK(mode))\n-\t\t\t\toidset_insert(&gitmodules_found, oid);\n+\t\t\t\toidset_insert(&gitmodules_found, entry_oid);\n \t\t\telse\n \t\t\t\tretval += report(options,\n-\t\t\t\t\t\t oid, OBJ_TREE,\n+\t\t\t\t\t\t tree_oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n@@ -692,9 +692,9 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\t\thas_dotgit |= is_ntfs_dotgit(backslash);\n \t\t\t\tif (is_ntfs_dotgitmodules(backslash)) {\n \t\t\t\t\tif (!S_ISLNK(mode))\n-\t\t\t\t\t\toidset_insert(&gitmodules_found, oid);\n+\t\t\t\t\t\toidset_insert(&gitmodules_found, entry_oid);\n \t\t\t\t\telse\n-\t\t\t\t\t\tretval += report(options, oid, OBJ_TREE,\n+\t\t\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t\t\t}\n@@ -703,7 +703,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -751,25 +751,25 @@ static int fsck_tree(const struct object_id *oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex f70368bc2e..5c95247180 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -148,13 +148,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} | git mktree &&\n+\t\t} >bad-tree &&\n+\t\ttree=$(git mktree output &&\n-\t\ttest_i18ngrep gitmodulesSymlink output\n+\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n \t)\n '\n \n-- \n2.28.0.1295.g4824feede7\n\n"},{"id":"406896","messageId":"20201005071935.GB2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 2/7] fsck_tree(): wrap some long lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:19:35Z","receivedAt":"2020-10-05T07:19:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many calls to report() in fsck_tree() are kept on a single line and are\nquite long. Most were pretty big to begin with, but have gotten even\nlonger over the years as we've added more parameters. Let's accept the\nchurn of wrapping them in order to conform to our usual line limits.\n\nSigned-off-by: Jeff King \n---\nObviously not necessary for the rest of the serious, but this has been\nbugging me for years. I'm not sure what made me finally break down and\nwrap them.\n\n fsck.c | 48 ++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 36 insertions(+), 12 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 46a108839f..024810139b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -654,7 +654,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -703,7 +705,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t\t \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -751,25 +755,45 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_NULL_SHA1,\n+\t\t\t\t \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_FULL_PATHNAME,\n+\t\t\t\t \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_EMPTY_NAME,\n+\t\t\t\t \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOT,\n+\t\t\t\t \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTDOT,\n+\t\t\t\t \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTGIT,\n+\t\t\t\t \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_ZERO_PADDED_FILEMODE,\n+\t\t\t\t \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_FILEMODE,\n+\t\t\t\t \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_DUPLICATE_ENTRIES,\n+\t\t\t\t \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n+\t\t\t\t \"not properly sorted\");\n \treturn retval;\n }\n \n-- \n2.28.0.1295.g4824feede7\n\n"},{"id":"406897","messageId":"20201005071954.GC2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 3/7] t7415: rename to expand scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:19:54Z","receivedAt":"2020-10-05T07:19:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This script has already expanded beyond its original intent of \".. in\nsubmodule names\" to include other malicious submodule bits. Let's update\nthe name and description to reflect that, as well as the fact that we'll\nsoon be adding similar tests for other meta-files (.gitattributes, etc).\nWe'll also renumber it to move it out of the group of submodule-specific\ntests.\n\nSigned-off-by: Jeff King \n---\n ...5-submodule-names.sh => t7450-bad-meta-files.sh} | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7450-bad-meta-files.sh\nsimilarity index 95%\nrename from t/t7415-submodule-names.sh\nrename to t/t7450-bad-meta-files.sh\nindex 5c95247180..6b703b12bc 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7450-bad-meta-files.sh\n@@ -1,9 +1,16 @@\n #!/bin/sh\n \n-test_description='check handling of .. in submodule names\n+test_description='check forbidden or malicious patterns in .git* files\n \n-Exercise the name-checking function on a variety of names, and then give a\n-real-world setup that confirms we catch this in practice.\n+Such as:\n+\n+ - presence of .. in submodule names;\n+ Exercise the name-checking function on a variety of names, and then give a\n+ real-world setup that confirms we catch this in practice.\n+\n+ - nested submodule names\n+\n+ - symlinked .gitmodules, etc\n '\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-pack.sh\n-- \n2.28.0.1295.g4824feede7\n\n"},{"id":"406898","messageId":"20201005072015.GD2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 4/7] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:20:15Z","receivedAt":"2020-10-05T07:20:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n2018-05-04) made it impossible to load a symlink .gitmodules file into\nthe index. However, there are no tests of this behavior. Let's make sure\nthis case is covered. We can easily reuse the test setup created by\nthe matching b7b1fca175 (fsck: complain when .gitmodules is a symlink,\n2018-05-04).\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-meta-files.sh | 17 ++++++++++++++---\n 1 file changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7450-bad-meta-files.sh b/t/t7450-bad-meta-files.sh\nindex 6b703b12bc..b73985157f 100755\n--- a/t/t7450-bad-meta-files.sh\n+++ b/t/t7450-bad-meta-files.sh\n@@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'fsck detects symlinked .gitmodules file' '\n+test_expect_success 'create repo with symlinked .gitmodules file' '\n \tgit init symlink &&\n \t(\n \t\tcd symlink &&\n@@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree &&\n-\t\ttree=$(git mktree bad-tree\n+\t) &&\n+\ttree=$(git -C symlink mktree err &&\n+\ttest_i18ngrep \"invalid path.*gitmodules\" err\n+'\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.28.0.1295.g4824feede7\n\n"},{"id":"406899","messageId":"20201005072102.GE2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:21:02Z","receivedAt":"2020-10-05T07:21:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have tests that cover various filesystem-specific spellings of\n\".gitmodules\", because we need to reliably identify that path for some\nsecurity checks. These are from dc2d9ba318 (is_{hfs,ntfs}_dotgitmodules:\nadd tests, 2018-05-12), with the actual code coming from e7cb0b4455\n(is_ntfs_dotgit: match other .git files, 2018-05-11) and 0fc333ba20\n(is_hfs_dotgit: match other .git files, 2018-05-02).\n\nThose latter two commits also added similar matching functions for\n.gitattributes and .gitignore. These ended up not being used in the\nfinal series, and are currently dead code. But in preparation for them\nbeing used, let's make sure they actually work by throwing a few basic\nchecks at them.\n\nI didn't bother with the whole battery of tests that we cover for\n.gitmodules. These functions are all based on the same generic matcher,\nso it's sufficient to test most of the corner cases just once.\n\nNote that the ntfs magic prefix names in the tests come from the\nalgorithm described in e7cb0b4455 (and are different for each file).\n\nSigned-off-by: Jeff King \n---\n t/helper/test-path-utils.c | 41 ++++++++++++++++++++++++++------------\n t/t0060-path-utils.sh | 20 +++++++++++++++++++\n 2 files changed, 48 insertions(+), 13 deletions(-)\n\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 313a153209..9e253f8058 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -172,9 +172,22 @@ static struct test_data dirname_data[] = {\n \t{ NULL, NULL }\n };\n \n-static int is_dotgitmodules(const char *path)\n+static int check_dotgitx(const char *x, const char **argv,\n+\t\t\t int (*is_hfs)(const char *),\n+\t\t\t int (*is_ntfs)(const char *))\n {\n-\treturn is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);\n+\tint res = 0, expect = 1;\n+\tfor (; *argv; argv++) {\n+\t\tif (!strcmp(\"--not\", *argv))\n+\t\t\texpect = !expect;\n+\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n+\t\t\t res = error(\"'%s' is %s.%s\", *argv,\n+\t\t\t\t expect ? \"not \" : \"\", x);\n+\t\telse\n+\t\t\tfprintf(stderr, \"ok: '%s' is %s.%s\\n\",\n+\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n+\t}\n+\treturn !!res;\n }\n \n static int cmp_by_st_size(const void *a, const void *b)\n@@ -382,17 +395,19 @@ int cmd__path_utils(int argc, const char **argv)\n \t\treturn test_function(dirname_data, posix_dirname, argv[1]);\n \n \tif (argc > 2 && !strcmp(argv[1], \"is_dotgitmodules\")) {\n-\t\tint res = 0, expect = 1, i;\n-\t\tfor (i = 2; i < argc; i++)\n-\t\t\tif (!strcmp(\"--not\", argv[i]))\n-\t\t\t\texpect = !expect;\n-\t\t\telse if (expect != is_dotgitmodules(argv[i]))\n-\t\t\t\tres = error(\"'%s' is %s.gitmodules\", argv[i],\n-\t\t\t\t\t expect ? \"not \" : \"\");\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"ok: '%s' is %s.gitmodules\\n\",\n-\t\t\t\t\targv[i], expect ? \"\" : \"not \");\n-\t\treturn !!res;\n+\t\treturn check_dotgitx(\"gitmodules\", argv + 2,\n+\t\t\t\t is_hfs_dotgitmodules,\n+\t\t\t\t is_ntfs_dotgitmodules);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitignore\")) {\n+\t\treturn check_dotgitx(\"gitignore\", argv + 2,\n+\t\t\t\t is_hfs_dotgitignore,\n+\t\t\t\t is_ntfs_dotgitignore);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitattributes\")) {\n+\t\treturn check_dotgitx(\"gitattributes\", argv + 2,\n+\t\t\t\t is_hfs_dotgitattributes,\n+\t\t\t\t is_ntfs_dotgitattributes);\n \t}\n \n \tif (argc > 2 && !strcmp(argv[1], \"file-size\")) {\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 56db5c8aba..b2e3cf3f4c 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -468,6 +468,26 @@ test_expect_success 'match .gitmodules' '\n \t\t.gitmodules,:\\$DATA\n '\n \n+test_expect_success 'match .gitattributes' '\n+\ttest-tool path-utils is_dotgitattributes \\\n+\t\t.gitattributes \\\n+\t\t.git${u200c}attributes \\\n+\t\t.Gitattributes \\\n+\t\t.gitattributeS \\\n+\t\tGITATT~1 \\\n+\t\tGI7D29~1\n+'\n+\n+test_expect_success 'match .gitignore' '\n+\ttest-tool path-utils is_dotgitignore \\\n+\t\t.gitignore \\\n+\t\t.git${u200c}ignore \\\n+\t\t.Gitignore \\\n+\t\t.gitignorE \\\n+\t\tGITIGN~1 \\\n+\t\tGI250A~1\n+'\n+\n test_expect_success MINGW 'is_valid_path() on Windows' '\n \ttest-tool path-utils is_valid_path \\\n \t\twin32 \\\n-- \n2.28.0.1295.g4824feede7\n\n"},{"id":"406900","messageId":"20201005072406.GF2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 6/7] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:24:06Z","receivedAt":"2020-10-05T07:24:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n2018-05-04) we made it impossible to load a .gitmodules file that's a\nsymlink into the index. The security reasons for doing so are described\nthere. We also discussed forbidding symlinks of other .git files as part\nof that fix, but the tradeoff was less compelling:\n\n 1. Unlike .gitmodules, the other files don't have content-level fsck\n checks. So an attacker using symlinks to evade those checks isn't a\n problem.\n\n 2. Unlike .gitmodules, Git will never write .gitignore or\n .gitattributes itself, making it much less likely to use them to\n write outside the repo. They could be used for out-of-repo reads,\n however.\n\n 3. The .gitmodules change was part of a critical bug-fix that was\n not publicly disclosed until it was released. Changing the other\n files was not needed for the minimal fix.\n\nHowever, it's still a reasonable idea to forbid symlinks for these\nfiles:\n\n - As noted, they can still be used to read out-of-repo files (which is\n fairly restricted, but in some circumstances you can probe file\n content by speculatively creating files and seeing if they get\n ignored)\n\n - They don't currently behave well in all cases. We sometimes read\n these files from the index, where we _don't_ follow symlinks (we'd\n just treat the symlink target as the .gitignore or .gitattributes\n content, which is actively wrong).\n\nThis patch forbids symlinked versions of these files from entering the\nindex. We already have helpers for obscured forms of the names from\ne7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11) and\n0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which\nwere done as part of the series touching .gitmodules.\n\nSigned-off-by: Jeff King \n---\nI note that neither these new tests nor the existing .gitmodules ones\nconfirm that we catch the obscured ntfs/hfs forms in the actual code\npaths (instead, we feed them to a synthetic test-tool helper in t0060).\nI think that's OK, but if we wanted to be super-paranoid we could beef\nup these tests with trickier names.\n\n read-cache.c | 12 +++++++++---\n t/t7450-bad-meta-files.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 38 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex ecf6f68994..63aec6c35d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -947,7 +947,9 @@ static int verify_dotfile(const char *rest, unsigned mode)\n \t\t\treturn 0;\n \t\tif (S_ISLNK(mode)) {\n \t\t\trest += 3;\n-\t\t\tif (skip_iprefix(rest, \"modules\", &rest) &&\n+\t\t\tif ((skip_iprefix(rest, \"modules\", &rest) ||\n+\t\t\t skip_iprefix(rest, \"ignore\", &rest) ||\n+\t\t\t skip_iprefix(rest, \"attributes\", &rest)) &&\n \t\t\t (*rest == '\\0' || is_dir_sep(*rest)))\n \t\t\t\treturn 0;\n \t\t}\n@@ -980,7 +982,9 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t\tif (is_hfs_dotgit(path))\n \t\t\t\t\treturn 0;\n \t\t\t\tif (S_ISLNK(mode)) {\n-\t\t\t\t\tif (is_hfs_dotgitmodules(path))\n+\t\t\t\t\tif (is_hfs_dotgitmodules(path) ||\n+\t\t\t\t\t is_hfs_dotgitignore(path) ||\n+\t\t\t\t\t is_hfs_dotgitattributes(path))\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\n@@ -992,7 +996,9 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t\tif (is_ntfs_dotgit(path))\n \t\t\t\t\treturn 0;\n \t\t\t\tif (S_ISLNK(mode)) {\n-\t\t\t\t\tif (is_ntfs_dotgitmodules(path))\n+\t\t\t\t\tif (is_ntfs_dotgitmodules(path) ||\n+\t\t\t\t\t is_ntfs_dotgitignore(path) ||\n+\t\t\t\t\t is_ntfs_dotgitattributes(path))\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\ndiff --git a/t/t7450-bad-meta-files.sh b/t/t7450-bad-meta-files.sh\nindex b73985157f..6a038ed55b 100755\n--- a/t/t7450-bad-meta-files.sh\n+++ b/t/t7450-bad-meta-files.sh\n@@ -267,4 +267,33 @@ test_expect_success 'git dirs of sibling submodules must not be nested' '\n \ttest_i18ngrep \"is inside git dir\" err\n '\n \n+test_expect_success 'create repo with symlinked .gitattributes file' '\n+\tgit init symlink-attr &&\n+\ttarget=$(echo target | git -C symlink-attr hash-object -w --stdin) &&\n+\ttree=$(\n+\t\tprintf \"120000 blob $target\\t.gitattributes\\n\" |\n+\t\tgit -C symlink-attr mktree\n+\t)\n+'\n+\n+test_expect_success 'refuse to load symlinked .gitattributes into index' '\n+\ttest_must_fail git -C symlink-attr read-tree $tree 2>err &&\n+\ttest_i18ngrep \"invalid path.*gitattributes\" err\n+'\n+\n+test_expect_success 'create repo with symlinked .gitignore file' '\n+\tgit init symlink-ignore &&\n+\ttarget=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&\n+\ttree=$(\n+\t\tprintf \"120000 blob $target\\t.gitignore\\n\" |\n+\t\tgit -C symlink-ignore mktree\n+\t)\n+'\n+\n+test_expect_success 'refuse to load symlinked .gitignore into index' '\n+\ttest_must_fail git -C symlink-ignore read-tree $tree 2>err &&\n+\ttest_i18ngrep \"invalid path.*gitignore\" err\n+'\n+\n+\n test_done\n-- \n2.28.0.1295.g4824feede7\n\n"},{"id":"406901","messageId":"20201005072503.GG2291074@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH 7/7] fsck: complain when .gitattributes or .gitignore is a symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T07:25:03Z","receivedAt":"2020-10-05T07:25:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit made it impossible to have a symlinked\n.gitattributes or .gitignore file via verify_path(). Let's add the same\ncheck to fsck, which matches how we handle .gitmodules symlinks, via\nb7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).\n\nNote that we won't add these to the existing gitmodules block. Its logic\nis a bit more complicated, as we also check the content of non-symlink\ninstances we find. But for these new files, there is no content check;\nwe're just looking at the name and mode of the tree entry (and we can\navoid even the complicated name checks in the common case that the mode\ndoesn't indicate a symlink).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 15 +++++++++++++++\n t/t7450-bad-meta-files.sh | 9 +++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 024810139b..fcd3f268b1 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -67,6 +67,8 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(GITMODULES_URL, ERROR) \\\n \tFUNC(GITMODULES_PATH, ERROR) \\\n \tFUNC(GITMODULES_UPDATE, ERROR) \\\n+\tFUNC(GITIGNORE_SYMLINK, ERROR) \\\n+\tFUNC(GITATTRIBUTES_SYMLINK, ERROR) \\\n \t/* warnings */ \\\n \tFUNC(BAD_FILEMODE, WARN) \\\n \tFUNC(EMPTY_NAME, WARN) \\\n@@ -688,6 +690,19 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n \n+\t\tif (S_ISLNK(mode)) {\n+\t\t\tif (is_hfs_dotgitignore(name) ||\n+\t\t\t is_ntfs_dotgitignore(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITIGNORE_SYMLINK,\n+\t\t\t\t\t\t \".gitignore is a symlink\");\n+\t\t\tif (is_hfs_dotgitattributes(name) ||\n+\t\t\t is_ntfs_dotgitattributes(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITATTRIBUTES_SYMLINK,\n+\t\t\t\t\t\t \".gitattributes is a symlink\");\n+\t\t}\n+\n \t\tif ((backslash = strchr(name, '\\\\'))) {\n \t\t\twhile (backslash) {\n \t\t\t\tbackslash++;\ndiff --git a/t/t7450-bad-meta-files.sh b/t/t7450-bad-meta-files.sh\nindex 6a038ed55b..c7201803ec 100755\n--- a/t/t7450-bad-meta-files.sh\n+++ b/t/t7450-bad-meta-files.sh\n@@ -281,6 +281,11 @@ test_expect_success 'refuse to load symlinked .gitattributes into index' '\n \ttest_i18ngrep \"invalid path.*gitattributes\" err\n '\n \n+test_expect_success 'fsck detects symlinked .gitattributes file' '\n+\ttest_must_fail git -C symlink-attr fsck 2>err &&\n+\ttest_i18ngrep \"tree $tree: gitattributesSymlink\" err\n+'\n+\n test_expect_success 'create repo with symlinked .gitignore file' '\n \tgit init symlink-ignore &&\n \ttarget=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&\n@@ -295,5 +300,9 @@ test_expect_success 'refuse to load symlinked .gitignore into index' '\n \ttest_i18ngrep \"invalid path.*gitignore\" err\n '\n \n+test_expect_success 'fsck detects symlinked .gitignore file' '\n+\ttest_must_fail git -C symlink-ignore fsck 2>err &&\n+\ttest_i18ngrep \"tree $tree: gitignoreSymlink\" err\n+'\n \n test_done\n-- \n2.28.0.1295.g4824feede7\n"},{"id":"406902","messageId":"20201005073202.GC1166820@google.com","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"Re: [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T07:32:02Z","receivedAt":"2020-10-05T07:32:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> About 2 years ago as part of a security release we made it illegal to\n> have a symlinked .gitmodules file (refusing it both in the index and via\n> fsck). At the time we discussed (on the security list) outlawing\n> symlinks for other .git files in the same way, but we decided not to do\n> so as part of the security release, as it wasn't strictly necessary.\n>\n> We publicly revisited the topic in:\n>\n> https://lore.kernel.org/git/20190114230902.GG162110@google.com/\n>\n> but there were a few fixes needed, and it got forgotten. So here it is\n> again, with those fixes:\n\nOh! I'm excited --- at $DAYJOB we've been carrying that patch since\nthen; I'll be happy to drop it. :)\n\nJonathan\n"},{"id":"406903","messageId":"20201005074404.GD1166820@google.com","threadId":"54345","inReplyTo":"20201005071905.GA2291074@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] fsck_tree(): fix shadowed variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T07:44:04Z","receivedAt":"2020-10-05T07:44:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\n> fsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\n> fsck_tree(), and we pass it to the report() function when we find\n> problems. However, that is shadowed within the tree-walking loop by the\n> existing \"oid\" variable which we use to store the oid of each tree\n> entry. As a result, we may report the wrong oid for some problems we\n> detect within the loop (the entry oid, instead of the tree oid).\n>\n> Our tests didn't catch this because they checked only that we found the\n> expected fsck problem, not that it was attached to the correct object.\n\nOh, goodness. Does this mean we should be similarly checking oid in\nthe rest of the fsck test scripts? (I'm not saying this patch should\ndo so, just curious about what you think on the subject.)\n\n> Let's rename both variables in the function to avoid confusion. This\n> makes the diff a little noisy (e.g., all of the report() calls outside\n> the loop wee already correct but need touched), but makes sure we catch\n\nnit: s/wee/are/, s/need touched/need to be touched/\n\n> all cases and will avoid similar confusion in the future.\n\nThanks for doing that --- it does make reviewing easier.\n\n> Signed-off-by: Jeff King \n> ---\n> fsck.c | 40 +++++++++++++++++++-------------------\n> t/t7415-submodule-names.sh | 5 +++--\n> 2 files changed, 23 insertions(+), 22 deletions(-)\n> \n> diff --git a/fsck.c b/fsck.c\n> index f82e2fe9e3..46a108839f 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -633,7 +633,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n> \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n> }\n> \n> -static int fsck_tree(const struct object_id *oid,\n> +static int fsck_tree(const struct object_id *tree_oid,\n\noptional: we could call it \"tree\".\n\n> \t\t const char *buffer, unsigned long size,\n> \t\t struct fsck_options *options)\n> {\n> @@ -654,7 +654,7 @@ static int fsck_tree(const struct object_id *oid,\n> \tstruct name_stack df_dup_candidates = { NULL };\n> \n> \tif (init_tree_desc_gently(&desc, buffer, size)) {\n> -\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n> +\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n> \t\treturn retval;\n> \t}\n> \n> @@ -664,11 +664,11 @@ static int fsck_tree(const struct object_id *oid,\n> \twhile (desc.size) {\n> \t\tunsigned short mode;\n> \t\tconst char *name, *backslash;\n> -\t\tconst struct object_id *oid;\n> +\t\tconst struct object_id *entry_oid;\n> \n> -\t\toid = tree_entry_extract(&desc, &name, &mode);\n> +\t\tentry_oid = tree_entry_extract(&desc, &name, &mode);\n\noptional: could call it \"child\".\n\n> \n> -\t\thas_null_sha1 |= is_null_oid(oid);\n> +\t\thas_null_sha1 |= is_null_oid(entry_oid);\n> \t\thas_full_path |= !!strchr(name, '/');\n> \t\thas_empty_name |= !*name;\n> \t\thas_dot |= !strcmp(name, \".\");\n> @@ -678,10 +678,10 @@ static int fsck_tree(const struct object_id *oid,\n> \n> \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n> \t\t\tif (!S_ISLNK(mode))\n> -\t\t\t\toidset_insert(&gitmodules_found, oid);\n> +\t\t\t\toidset_insert(&gitmodules_found, entry_oid);\n> \t\t\telse\n> \t\t\t\tretval += report(options,\n> -\t\t\t\t\t\t oid, OBJ_TREE,\n> +\t\t\t\t\t\t tree_oid, OBJ_TREE,\n> \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n> \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n\nRight, this is a property of the enclosing tree, not the blob\nrepresenting the symlink target. Good.\n\nnot about this patch: Could report() notice when the oid doesn't match\nthe type passed in, to more easily catch this kind of mistake?\n\n[...]\n> @@ -692,9 +692,9 @@ static int fsck_tree(const struct object_id *oid,\n> \t\t\t\thas_dotgit |= is_ntfs_dotgit(backslash);\n> \t\t\t\tif (is_ntfs_dotgitmodules(backslash)) {\n> \t\t\t\t\tif (!S_ISLNK(mode))\n> -\t\t\t\t\t\toidset_insert(&gitmodules_found, oid);\n> +\t\t\t\t\t\toidset_insert(&gitmodules_found, entry_oid);\n> \t\t\t\t\telse\n> -\t\t\t\t\t\tretval += report(options, oid, OBJ_TREE,\n> +\t\t\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n> \t\t\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n> \t\t\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n\nLikewise, good.\n\n[...]\n> --- a/t/t7415-submodule-names.sh\n> +++ b/t/t7415-submodule-names.sh\n> @@ -148,13 +148,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> \t\t{\n> \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n> \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n> -\t\t} | git mktree &&\n> +\t\t} >bad-tree &&\n> +\t\ttree=$(git mktree \n> \t\t# Check not only that we fail, but that it is due to the\n> \t\t# symlink detector; this grep string comes from the config\n> \t\t# variable name and will not be translated.\n> \t\ttest_must_fail git fsck 2>output &&\n> -\t\ttest_i18ngrep gitmodulesSymlink output\n> +\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n\nMakes sense.\n\nBy the way, why does GETTEXT_POISON lose the gitmodulesSymlink\nkeyword? Is this just a limitation of GETTEXT_POISON losing\ninformation that's passed in with %s?\n\nWith the commit message tweak mentioned above,\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"406904","messageId":"20201005074627.GE1166820@google.com","threadId":"54345","inReplyTo":"20201005071935.GB2291074@coredump.intra.peff.net","subject":"Re: [PATCH 2/7] fsck_tree(): wrap some long lines","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T07:46:27Z","receivedAt":"2020-10-05T07:46:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Many calls to report() in fsck_tree() are kept on a single line and are\n> quite long. Most were pretty big to begin with, but have gotten even\n> longer over the years as we've added more parameters. Let's accept the\n> churn of wrapping them in order to conform to our usual line limits.\n>\n> Signed-off-by: Jeff King \n> ---\n> fsck.c | 48 ++++++++++++++++++++++++++++++++++++------------\n> 1 file changed, 36 insertions(+), 12 deletions(-)\n\n\"git show --word-diff\" and \"git show --check\" show nothing obvious\nwrong (which in particular means there's no functional change); eyeing\nthe patch, it looks good, too. Thanks.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"406905","messageId":"20201005075020.GF1166820@google.com","threadId":"54345","inReplyTo":"20201005071954.GC2291074@coredump.intra.peff.net","subject":"Re: [PATCH 3/7] t7415: rename to expand scope","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T07:50:20Z","receivedAt":"2020-10-05T07:50:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This script has already expanded beyond its original intent of \".. in\n> submodule names\" to include other malicious submodule bits. Let's update\n> the name and description to reflect that, as well as the fact that we'll\n> soon be adding similar tests for other meta-files (.gitattributes, etc).\n> We'll also renumber it to move it out of the group of submodule-specific\n> tests.\n>\n> Signed-off-by: Jeff King \n> ---\n> ...5-submodule-names.sh => t7450-bad-meta-files.sh} | 13 ++++++++++---\n> 1 file changed, 10 insertions(+), 3 deletions(-)\n> rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%)\n\nI've never heard of a \"meta file\" before, but I don't tend to discover\ntest scripts based on their filename anyway. :) Thanks for updating the\ntest_description.\n\nt7* is \"the porcelainish commands concerning the working tree\". Should\nthis go in t1* (basic commands concerning database) instead?\n\nt745* is unused number space so this at least won't hit any conflicts,\nso fwiw\n\nReviewed-by: Jonathan Nieder \n"},{"id":"406906","messageId":"20201005075311.GG1166820@google.com","threadId":"54345","inReplyTo":"20201005072015.GD2291074@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] t7450: test verify_path() handling of gitmodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T07:53:11Z","receivedAt":"2020-10-05T07:53:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n> 2018-05-04) made it impossible to load a symlink .gitmodules file into\n> the index. However, there are no tests of this behavior. Let's make sure\n> this case is covered. We can easily reuse the test setup created by\n> the matching b7b1fca175 (fsck: complain when .gitmodules is a symlink,\n> 2018-05-04).\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t7450-bad-meta-files.sh | 17 ++++++++++++++---\n> 1 file changed, 14 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t7450-bad-meta-files.sh b/t/t7450-bad-meta-files.sh\n> index 6b703b12bc..b73985157f 100755\n> --- a/t/t7450-bad-meta-files.sh\n> +++ b/t/t7450-bad-meta-files.sh\n> @@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n> \tgrep gitmodulesName output\n> '\n> \n> -test_expect_success 'fsck detects symlinked .gitmodules file' '\n> +test_expect_success 'create repo with symlinked .gitmodules file' '\n> \tgit init symlink &&\n> \t(\n> \t\tcd symlink &&\n> @@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> \t\t{\n> \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n> \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n> -\t\t} >bad-tree &&\n> -\t\ttree=$(git mktree +\t\t} >bad-tree\n> +\t) &&\n> +\ttree=$(git -C symlink mktree +'\n\nThis is super nitpicky, but: test scripts can be hard to maintain when\nthere's this kind of state carried from assertion to assertion without\nit being made obvious.\n\nCan this include \"setup\" or \"set up\" in the name to do that? E.g.\n\n\ttest_expect_success 'set up repo with symlinked .gitmodules file' '\n\t\t...\n\t'\n\nThanks,\nJonathan\n"},{"id":"406907","messageId":"20201005080353.GH1166820@google.com","threadId":"54345","inReplyTo":"20201005072102.GE2291074@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T08:03:53Z","receivedAt":"2020-10-05T08:03:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Dscho for NTFS savvy)\nJeff King wrote:\n\n> We have tests that cover various filesystem-specific spellings of\n> \".gitmodules\", because we need to reliably identify that path for some\n> security checks. These are from dc2d9ba318 (is_{hfs,ntfs}_dotgitmodules:\n> add tests, 2018-05-12), with the actual code coming from e7cb0b4455\n> (is_ntfs_dotgit: match other .git files, 2018-05-11) and 0fc333ba20\n> (is_hfs_dotgit: match other .git files, 2018-05-02).\n>\n> Those latter two commits also added similar matching functions for\n> .gitattributes and .gitignore. These ended up not being used in the\n> final series, and are currently dead code. But in preparation for them\n> being used, let's make sure they actually work by throwing a few basic\n> checks at them.\n>\n> I didn't bother with the whole battery of tests that we cover for\n> .gitmodules. These functions are all based on the same generic matcher,\n> so it's sufficient to test most of the corner cases just once.\n\nYeah, that's reasonable.\n\n> Note that the ntfs magic prefix names in the tests come from the\n> algorithm described in e7cb0b4455 (and are different for each file).\n\nDoesn't block this patch, but I'm curious: how hard would it be to make\na test with an NTFS prerequisite that makes sure we got the magic prefix\nright?\n\n> Signed-off-by: Jeff King \n> ---\n> t/helper/test-path-utils.c | 41 ++++++++++++++++++++++++++------------\n> t/t0060-path-utils.sh | 20 +++++++++++++++++++\n> 2 files changed, 48 insertions(+), 13 deletions(-)\n> \n> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\n> index 313a153209..9e253f8058 100644\n> --- a/t/helper/test-path-utils.c\n> +++ b/t/helper/test-path-utils.c\n> @@ -172,9 +172,22 @@ static struct test_data dirname_data[] = {\n> \t{ NULL, NULL }\n> };\n> \n> -static int is_dotgitmodules(const char *path)\n> +static int check_dotgitx(const char *x, const char **argv,\n> +\t\t\t int (*is_hfs)(const char *),\n> +\t\t\t int (*is_ntfs)(const char *))\n> {\n> -\treturn is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);\n> +\tint res = 0, expect = 1;\n> +\tfor (; *argv; argv++) {\n> +\t\tif (!strcmp(\"--not\", *argv))\n> +\t\t\texpect = !expect;\n> +\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n> +\t\t\t res = error(\"'%s' is %s.%s\", *argv,\n> +\t\t\t\t expect ? \"not \" : \"\", x);\n> +\t\telse\n> +\t\t\tfprintf(stderr, \"ok: '%s' is %s.%s\\n\",\n> +\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n\nmicronit: extra space on the \"res\" line.\n\nThis \"if\" cascade is a little hard to read, even though it does the\nright thing. Can we make it more explicit? E.g.\n\n\t\tif (!strcmp(\"--not\", *argv)) {\n\t\t\texpect = !expect;\n\t\t\tcontinue;\n\t\t}\n\n\t\tactual = is_hfs(*argv) || is_ntfs(*argv);\n\n\t\tfprintf(stderr, \"%s: '%s' is %s%s\",\n\t\t\texpect == actual ? \"ok\" : \"error\",\n\t\t\t*argv, actual ? \"\" : \"not \", x);\n\t\tif (expect != actual)\n\t\t\tres = -1;\n\nI think it's a little easier to read with either (a) the dot included\nin the 'x' parameter or (b) the entire \".git\" missing from the 'x'\nparameter.\n\n[...]\n> index 56db5c8aba..b2e3cf3f4c 100755\n> --- a/t/t0060-path-utils.sh\n> +++ b/t/t0060-path-utils.sh\n> @@ -468,6 +468,26 @@ test_expect_success 'match .gitmodules' '\n> \t\t.gitmodules,:\\$DATA\n> '\n> \n> +test_expect_success 'match .gitattributes' '\n> +\ttest-tool path-utils is_dotgitattributes \\\n> +\t\t.gitattributes \\\n> +\t\t.git${u200c}attributes \\\n> +\t\t.Gitattributes \\\n> +\t\t.gitattributeS \\\n> +\t\tGITATT~1 \\\n> +\t\tGI7D29~1\n> +'\n> +\n> +test_expect_success 'match .gitignore' '\n> +\ttest-tool path-utils is_dotgitignore \\\n> +\t\t.gitignore \\\n> +\t\t.git${u200c}ignore \\\n> +\t\t.Gitignore \\\n> +\t\t.gitignorE \\\n> +\t\tGITIGN~1 \\\n> +\t\tGI250A~1\n> +'\n> +\n> test_expect_success MINGW 'is_valid_path() on Windows' '\n> \ttest-tool path-utils is_valid_path \\\n> \t\twin32 \\\n\nWith whatever subset of the changes above makes sense,\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"406908","messageId":"20201005080930.GI1166820@google.com","threadId":"54345","inReplyTo":"20201005072406.GF2291074@coredump.intra.peff.net","subject":"Re: [PATCH 6/7] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T08:09:30Z","receivedAt":"2020-10-05T08:09:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> However, it's still a reasonable idea to forbid symlinks for these\n> files:\n>\n> - As noted, they can still be used to read out-of-repo files (which is\n> fairly restricted, but in some circumstances you can probe file\n> content by speculatively creating files and seeing if they get\n> ignored)\n>\n> - They don't currently behave well in all cases. We sometimes read\n> these files from the index, where we _don't_ follow symlinks (we'd\n> just treat the symlink target as the .gitignore or .gitattributes\n> content, which is actively wrong).\n>\n> This patch forbids symlinked versions of these files from entering the\n> index. We already have helpers for obscured forms of the names from\n> e7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11) and\n> 0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which\n> were done as part of the series touching .gitmodules.\n>\n> Signed-off-by: Jeff King \n> ---\n> I note that neither these new tests nor the existing .gitmodules ones\n> confirm that we catch the obscured ntfs/hfs forms in the actual code\n> paths (instead, we feed them to a synthetic test-tool helper in t0060).\n> I think that's OK, but if we wanted to be super-paranoid we could beef\n> up these tests with trickier names.\n\nI think being exhaustive wouldn't be worth it, but perhaps *one*\nexample (e.g., \".gitmodules \") would not be a terrible idea.\n\n> read-cache.c | 12 +++++++++---\n> t/t7450-bad-meta-files.sh | 29 +++++++++++++++++++++++++++++\n> 2 files changed, 38 insertions(+), 3 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\n[...]\n> --- a/t/t7450-bad-meta-files.sh\n> +++ b/t/t7450-bad-meta-files.sh\n> @@ -267,4 +267,33 @@ test_expect_success 'git dirs of sibling submodules must not be nested' '\n> \ttest_i18ngrep \"is inside git dir\" err\n> '\n> \n> +test_expect_success 'create repo with symlinked .gitattributes file' '\n> +\tgit init symlink-attr &&\n> +\ttarget=$(echo target | git -C symlink-attr hash-object -w --stdin) &&\n> +\ttree=$(\n> +\t\tprintf \"120000 blob $target\\t.gitattributes\\n\" |\n> +\t\tgit -C symlink-attr mktree\n> +\t)\n> +'\n> +\n> +test_expect_success 'refuse to load symlinked .gitattributes into index' '\n> +\ttest_must_fail git -C symlink-attr read-tree $tree 2>err &&\n> +\ttest_i18ngrep \"invalid path.*gitattributes\" err\n\nThis tests that it fails but doesn't test that it had no effect.\nWould that be straightforward to check as well (e.g. an \"ls-files -s\"\nbefore and after)?\n"},{"id":"406909","messageId":"20201005081233.GJ1166820@google.com","threadId":"54345","inReplyTo":"20201005072503.GG2291074@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] fsck: complain when .gitattributes or .gitignore is a symlink","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T08:12:33Z","receivedAt":"2020-10-05T08:12:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The previous commit made it impossible to have a symlinked\n> .gitattributes or .gitignore file via verify_path(). Let's add the same\n> check to fsck, which matches how we handle .gitmodules symlinks, via\n> b7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).\n>\n> Note that we won't add these to the existing gitmodules block. Its logic\n> is a bit more complicated, as we also check the content of non-symlink\n> instances we find. But for these new files, there is no content check;\n> we're just looking at the name and mode of the tree entry (and we can\n> avoid even the complicated name checks in the common case that the mode\n> doesn't indicate a symlink).\n\nOn the subject of where the check gets added, the old description said\n\n\tIt's easier to handle than .gitmodules, because we don't care\n\tabout checking the blob content. This is really just about\n\twhether the name and mode for the tree entry are valid.\n\nwhich I think was self-explanatory enough. The new text is a little\nmore confusing because I get lost in figuring out what the \"it\" in\n\"Its\" refers to.\n\n> Signed-off-by: Jeff King \n> ---\n> fsck.c | 15 +++++++++++++++\n> t/t7450-bad-meta-files.sh | 9 +++++++++\n> 2 files changed, 24 insertions(+)\n\nReviewed-by: Jonathan Nieder \nThanks for a pleasant read.\n"},{"id":"406910","messageId":"20201005082023.GA2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005074404.GD1166820@google.com","subject":"Re: [PATCH 1/7] fsck_tree(): fix shadowed variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:20:23Z","receivedAt":"2020-10-05T08:20:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 12:44:04AM -0700, Jonathan Nieder wrote:\n\n> > Our tests didn't catch this because they checked only that we found the\n> > expected fsck problem, not that it was attached to the correct object.\n> \n> Oh, goodness. Does this mean we should be similarly checking oid in\n> the rest of the fsck test scripts? (I'm not saying this patch should\n> do so, just curious about what you think on the subject.)\n\nI don't feel strongly either way. It would be a bigger change than you\nmight hope, I'd expect, because it requires collecting the oid of the\nbad object in the test script as we create it. So I'm not planning to\nwork on it, but if somebody else wants to, be my guest.\n\n> > Let's rename both variables in the function to avoid confusion. This\n> > makes the diff a little noisy (e.g., all of the report() calls outside\n> > the loop wee already correct but need touched), but makes sure we catch\n> \n> nit: s/wee/are/, s/need touched/need to be touched/\n\nFoiled by my last-minute editing. It was originally \"are\", but I meant\nto change it to \"were\".\n\n> > -static int fsck_tree(const struct object_id *oid,\n> > +static int fsck_tree(const struct object_id *tree_oid,\n> \n> optional: we could call it \"tree\".\n\nYeah, I started that way but wondered if it was confusing with a \"struct\ntree\" (which we don't have here; the point of the change that introduced\nthe shadowing was getting rid of that). Being longer doesn't hurt too\nmuch and is quite clear.\n\n> > @@ -664,11 +664,11 @@ static int fsck_tree(const struct object_id *oid,\n> > \twhile (desc.size) {\n> > \t\tunsigned short mode;\n> > \t\tconst char *name, *backslash;\n> > -\t\tconst struct object_id *oid;\n> > +\t\tconst struct object_id *entry_oid;\n> > \n> > -\t\toid = tree_entry_extract(&desc, &name, &mode);\n> > +\t\tentry_oid = tree_entry_extract(&desc, &name, &mode);\n> \n> optional: could call it \"child\".\n\nIMHO that's too vague. We have a child name, a child mode, etc.\n\nAnother option is to just refer to desc.entry.oid directly, but that's\nlonger and seemed a bit too intimate with how tree_entry_extract works.\n\n> not about this patch: Could report() notice when the oid doesn't match\n> the type passed in, to more easily catch this kind of mistake?\n\nHmm. It would incur an extra hash lookup, but since we expect to\nreport() infrequently, I don't think the cost would be too high. I\nsuspect we could even drop the \"type\" field and have report() figure it\nout from the oid, but that is working in the opposite direction of your\nsuggestion. ;)\n\nI'm not 100% sure we'd always be able to look up such a struct, though.\nThis code path also gets called from index-pack as it's checking objects\n(so likewise we might not even have something in the object database\nyet).\n\n> > \t\t# Check not only that we fail, but that it is due to the\n> > \t\t# symlink detector; this grep string comes from the config\n> > \t\t# variable name and will not be translated.\n> > \t\ttest_must_fail git fsck 2>output &&\n> > -\t\ttest_i18ngrep gitmodulesSymlink output\n> > +\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n> \n> Makes sense.\n> \n> By the way, why does GETTEXT_POISON lose the gitmodulesSymlink\n> keyword? Is this just a limitation of GETTEXT_POISON losing\n> information that's passed in with %s?\n\nYes, I think so. It comes from 674ba34038 (fsck: mark strings for\ntranslation, 2018-11-10) which is passing through our string. Arguably\nthat commit made the comment lines rather confusing and pointless.\n\nThough hmm. Looks like the \"tree %s: %s\" part is in the translated\nstring. So a translation _could_ change it, though I'd expect it to only\nchange the words and not the syntax. Maybe an RTL language would. I\ndunno.\n\n-Peff\n"},{"id":"406911","messageId":"20201005082448.GB2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005075020.GF1166820@google.com","subject":"Re: [PATCH 3/7] t7415: rename to expand scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:24:48Z","receivedAt":"2020-10-05T08:24:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 12:50:20AM -0700, Jonathan Nieder wrote:\n\n> > ...5-submodule-names.sh => t7450-bad-meta-files.sh} | 13 ++++++++++---\n> > 1 file changed, 10 insertions(+), 3 deletions(-)\n> > rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%)\n> \n> I've never heard of a \"meta file\" before, but I don't tend to discover\n> test scripts based on their filename anyway. :) Thanks for updating the\n> test_description.\n\nI couldn't think of a better name for \"files that start with .git\". I\nalmost called it \"dot-git\", but then I worried about confusion with the\nactual \".git\" directory.\n\n> t7* is \"the porcelainish commands concerning the working tree\". Should\n> this go in t1* (basic commands concerning database) instead?\n> \n> t745* is unused number space so this at least won't hit any conflicts,\n> so fwiw\n\nWe've generally tried to order tests so that basic functionality in some\narea comes before more advanced. So I tried to put these specialty\n.gitmodules tests after the basic submodule tests (and likewise after\nany attribute or gitignore tests).\n\nIn practice, I doubt it matters that much. We don't tend to run the test\nsuite serially in order these days anyway, so the notion that finding a\nbug in an early test might save you CPU time or time spent reading error\nmessages likely no longer applies.\n\n-Peff\n"},{"id":"406912","messageId":"20201005082952.GK1166820@google.com","threadId":"54345","inReplyTo":"20201005082023.GA2862927@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] fsck_tree(): fix shadowed variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T08:29:52Z","receivedAt":"2020-10-05T08:29:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Oct 05, 2020 at 12:44:04AM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> -\t\ttest_i18ngrep gitmodulesSymlink output\n>>> +\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n>>\n>> Makes sense.\n>>\n>> By the way, why does GETTEXT_POISON lose the gitmodulesSymlink\n>> keyword? Is this just a limitation of GETTEXT_POISON losing\n>> information that's passed in with %s?\n>\n> Yes, I think so. It comes from 674ba34038 (fsck: mark strings for\n> translation, 2018-11-10) which is passing through our string. Arguably\n> that commit made the comment lines rather confusing and pointless.\n>\n> Though hmm. Looks like the \"tree %s: %s\" part is in the translated\n> string. So a translation _could_ change it, though I'd expect it to only\n> change the words and not the syntax. Maybe an RTL language would. I\n> dunno.\n\nAh, right. I can use $n markers in the format string to reorder\nparameters, so we don't want scripts to rely on their order. In\nprinciple one can imagine GETTEXT_POISON printing a random permutation\nof its arguments but this kind of nondeterminism in tests would be\nharder to debug than the current state where the args aren't passed\nthrough at all.\n\nAnd after all, the main point of GETTEXT_POISON is to allow us to test\nscripted commands to make sure we are not translating any output that\nscripts would want to use. For that purpose, the current poisoning\ndoes what we need. (In other words, we don't encourage non-test\nscripts to rely on these lines at all, even to grep for\ngitmodulesSymlink.)\n\nThanks,\nJonathan\n"},{"id":"406913","messageId":"20201005083004.GC2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005075311.GG1166820@google.com","subject":"Re: [PATCH 4/7] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:30:04Z","receivedAt":"2020-10-05T08:30:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 12:53:11AM -0700, Jonathan Nieder wrote:\n\n> > @@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n> > \t\t{\n> > \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n> > \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n> > -\t\t} >bad-tree &&\n> > -\t\ttree=$(git mktree > +\t\t} >bad-tree\n> > +\t) &&\n> > +\ttree=$(git -C symlink mktree > +'\n> \n> This is super nitpicky, but: test scripts can be hard to maintain when\n> there's this kind of state carried from assertion to assertion without\n> it being made obvious.\n> \n> Can this include \"setup\" or \"set up\" in the name to do that? E.g.\n> \n> \ttest_expect_success 'set up repo with symlinked .gitmodules file' '\n> \t\t...\n> \t'\n\nHmph. I specifically _tried_ to do that by breaking it into a separate\ntest with the name \"create\" in it, which I thought was one of the\ncode-words for \"I'm doing stuff that will be used in another test\". But\nI guess there's no official rule on that. I dug up:\n\n https://lore.kernel.org/git/20130826173501.GS4110@google.com/\n\nbut I guess I mis-remembered \"create\" being present there.\n\n-Peff\n"},{"id":"406914","messageId":"20201005083441.GL1166820@google.com","threadId":"54345","inReplyTo":"20201005082448.GB2862927@coredump.intra.peff.net","subject":"Re: [PATCH 3/7] t7415: rename to expand scope","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T08:34:41Z","receivedAt":"2020-10-05T08:34:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Oct 05, 2020 at 12:50:20AM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%)\n>>\n>> I've never heard of a \"meta file\" before, but I don't tend to discover\n>> test scripts based on their filename anyway. :) Thanks for updating the\n>> test_description.\n>\n> I couldn't think of a better name for \"files that start with .git\". I\n> almost called it \"dot-git\", but then I worried about confusion with the\n> actual \".git\" directory.\n\nt7450-dot-gitfoo-files.sh seems clear to me.\n\n[...]\n> In practice, I doubt it matters that much. We don't tend to run the test\n> suite serially in order these days anyway, so the notion that finding a\n> bug in an early test might save you CPU time or time spent reading error\n> messages likely no longer applies.\n\nI see --- the point here is that because it's using e.g. \"git clone\n--recurse-submodules\", we want it to be later than the other clone\ntests?\n\nI think I'd like us to move away from having the numbers at all some\nday (since collisions are very common), but there's probably not much\nto discuss there until one of us comes up with a proposal that still\nmakes it easy to do things like \"skip all git-svn tests\". :)\n\nJonathan\n"},{"id":"406915","messageId":"20201005083814.GM1166820@google.com","threadId":"54345","inReplyTo":"20201005083004.GC2862927@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] t7450: test verify_path() handling of gitmodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-05T08:38:14Z","receivedAt":"2020-10-05T08:38:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Oct 05, 2020 at 12:53:11AM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> @@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n>>> \t\t{\n>>> \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n>>> \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n>>> -\t\t} >bad-tree &&\n>>> -\t\ttree=$(git mktree >> +\t\t} >bad-tree\n>>> +\t) &&\n>>> +\ttree=$(git -C symlink mktree >> +'\n>>\n>> This is super nitpicky, but: test scripts can be hard to maintain when\n>> there's this kind of state carried from assertion to assertion without\n>> it being made obvious.\n>>\n>> Can this include \"setup\" or \"set up\" in the name to do that? E.g.\n>>\n>> \ttest_expect_success 'set up repo with symlinked .gitmodules file' '\n>> \t\t...\n>> \t'\n>\n> Hmph. I specifically _tried_ to do that by breaking it into a separate\n> test with the name \"create\" in it, which I thought was one of the\n> code-words for \"I'm doing stuff that will be used in another test\". But\n> I guess there's no official rule on that. I dug up:\n>\n> https://lore.kernel.org/git/20130826173501.GS4110@google.com/\n>\n> but I guess I mis-remembered \"create\" being present there.\n\nI can try to find some time today to introduce a test_setup helper.\nHaving to figure out and rely on this kind of ad hoc convention is not\nsomething I really want to ask of patch authors and reviewers.\n\nThe reason I find \"set up\" clearer than \"create\" is that the latter is\nsomething I can easily imagine myself genuinely wanting to test. \"Set\nup for a later test\" is more explicit about what the commands are\nbeing run for.\n\nJonathan\n"},{"id":"406916","messageId":"20201005084051.GD2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005080353.GH1166820@google.com","subject":"Re: [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:40:51Z","receivedAt":"2020-10-05T08:40:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 01:03:53AM -0700, Jonathan Nieder wrote:\n\n> > Note that the ntfs magic prefix names in the tests come from the\n> > algorithm described in e7cb0b4455 (and are different for each file).\n> \n> Doesn't block this patch, but I'm curious: how hard would it be to make\n> a test with an NTFS prerequisite that makes sure we got the magic prefix\n> right?\n\nI suspect hard since Dscho punted on it in the original series. :) If I\nunderstand correctly, it would require having an NTFS filesystem, and\ngenerating 10,000+ files with a clashing prefix.\n\n> > +\tfor (; *argv; argv++) {\n> > +\t\tif (!strcmp(\"--not\", *argv))\n> > +\t\t\texpect = !expect;\n> > +\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n> > +\t\t\t res = error(\"'%s' is %s.%s\", *argv,\n> > +\t\t\t\t expect ? \"not \" : \"\", x);\n> > +\t\telse\n> > +\t\t\tfprintf(stderr, \"ok: '%s' is %s.%s\\n\",\n> > +\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n> \n> micronit: extra space on the \"res\" line.\n\nThanks, fixed.\n\n> This \"if\" cascade is a little hard to read, even though it does the\n> right thing. Can we make it more explicit? E.g.\n\nThis is directly moved from the existing code. I'd prefer to keep the\noverall structure intact to make that clear.\n\n> I think it's a little easier to read with either (a) the dot included\n> in the 'x' parameter or (b) the entire \".git\" missing from the 'x'\n> parameter.\n\nYeah, I agree that's worth doing. I took (b), as \"dotgitx\" implies that\n\"x\" is \"modules\", etc. I had originally planned to automatically turn\n\"gitmodules\" into \"is_ntfs_dotgitmodules\", too, but it required\nmacros and string-pasting. So I decided it was a bit too ugly. :)\n\n-Peff\n"},{"id":"406917","messageId":"20201005084946.GE2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005083441.GL1166820@google.com","subject":"Re: [PATCH 3/7] t7415: rename to expand scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:49:46Z","receivedAt":"2020-10-05T08:49:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 01:34:41AM -0700, Jonathan Nieder wrote:\n\n> > I couldn't think of a better name for \"files that start with .git\". I\n> > almost called it \"dot-git\", but then I worried about confusion with the\n> > actual \".git\" directory.\n> \n> t7450-dot-gitfoo-files.sh seems clear to me.\n\nHeh, that was actually one of the ones I thought of, but I worried that\n\"foo\" was too confusing (likewise, I almost called the test-tool\nfunction check_dotgitfoo(const char *foo)). I guess dotgitx would follow\nthe same pattern here.\n\n> > In practice, I doubt it matters that much. We don't tend to run the test\n> > suite serially in order these days anyway, so the notion that finding a\n> > bug in an early test might save you CPU time or time spent reading error\n> > messages likely no longer applies.\n> \n> I see --- the point here is that because it's using e.g. \"git clone\n> --recurse-submodules\", we want it to be later than the other clone\n> tests?\n> \n> I think I'd like us to move away from having the numbers at all some\n> day (since collisions are very common), but there's probably not much\n> to discuss there until one of us comes up with a proposal that still\n> makes it easy to do things like \"skip all git-svn tests\". :)\n\nI'd be happy to get away from numbers, too. They're a frequent pain when\ndealing with duplicates, or when we run out of space in a group (I have\nanother series to split t9001 into a few separate scripts, but I have to\nmove either it or the unrelated bits at t9002).\n\nAn obvious solution is providing some kind of name hierarchy. E.g.:\n\n t-svn-commit-diff.sh\n t-svn-dcommit-auto-props.sh\n t-svn-dcommit-clobber-series.sh\n t-svn-dcommit-concurrent.sh\n t-svn-dcommit-crlf.sh\n t-svn-dcommit-funky-renames.sh\n t-svn-dcommit-interactive.sh\n t-svn-dcommit-merge.sh\n t-svn-dcommit-new-file.sh\n t-svn-deep-rmdir.sh\n\nThen you could skip t-svn-*, or just t-svn-dcommit-*, or even\nt-svn-commit-diff.12.\n\nThe \"t-\" is ugly, but lets you distinguish test scripts from other shell\nscripts in the directory.\n\nWe could also use actual directories for the hierarchy. svn/dcommit/*,\netc. The t/ directory is rather big. It does make it a little more work\nto assemble the full set of tests (you have to use `find` rather than a\nglob).\n\n-Peff\n"},{"id":"406918","messageId":"20201005085326.GF2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005081233.GJ1166820@google.com","subject":"Re: [PATCH 7/7] fsck: complain when .gitattributes or .gitignore is a symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:53:26Z","receivedAt":"2020-10-05T08:53:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 01:12:33AM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > The previous commit made it impossible to have a symlinked\n> > .gitattributes or .gitignore file via verify_path(). Let's add the same\n> > check to fsck, which matches how we handle .gitmodules symlinks, via\n> > b7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).\n> >\n> > Note that we won't add these to the existing gitmodules block. Its logic\n> > is a bit more complicated, as we also check the content of non-symlink\n> > instances we find. But for these new files, there is no content check;\n> > we're just looking at the name and mode of the tree entry (and we can\n> > avoid even the complicated name checks in the common case that the mode\n> > doesn't indicate a symlink).\n> \n> On the subject of where the check gets added, the old description said\n> \n> \tIt's easier to handle than .gitmodules, because we don't care\n> \tabout checking the blob content. This is really just about\n> \twhether the name and mode for the tree entry are valid.\n> \n> which I think was self-explanatory enough. The new text is a little\n> more confusing because I get lost in figuring out what the \"it\" in\n> \"Its\" refers to.\n\nI rewrote it because I found your original text confusing. ;)\n\nI'll write \"The logic for gitmodules is a bit more complicated...\".\n\n-Peff\n"},{"id":"406922","messageId":"20201005085814.GG2862927@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005073202.GC1166820@google.com","subject":"Re: [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T08:58:14Z","receivedAt":"2020-10-05T08:58:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 12:32:02AM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > About 2 years ago as part of a security release we made it illegal to\n> > have a symlinked .gitmodules file (refusing it both in the index and via\n> > fsck). At the time we discussed (on the security list) outlawing\n> > symlinks for other .git files in the same way, but we decided not to do\n> > so as part of the security release, as it wasn't strictly necessary.\n> >\n> > We publicly revisited the topic in:\n> >\n> > https://lore.kernel.org/git/20190114230902.GG162110@google.com/\n> >\n> > but there were a few fixes needed, and it got forgotten. So here it is\n> > again, with those fixes:\n> \n> Oh! I'm excited --- at $DAYJOB we've been carrying that patch since\n> then; I'll be happy to drop it. :)\n\nPart of my ulterior motive is that we've been carrying a patch at GitHub\nto pass O_NOFOLLOW when opening in-tree attributes and ignore files\n(which we don't normally do on our servers, but do for things like\nGitHub Pages). But I think O_NOFOLLOW isn't perfectly portable (despite\nbeing in POSIX), and the patch is rather invasive.\n\nI also looked at one point into preventing just out-of-tree symlinks.\nThat helps with out-of-repo reads, but it doesn't change the fact that\nin-index reads of symlinks are broken. We _could_ change that with a lot\nof work, but I don't think anybody cares enough about the feature to do\nso.\n\n-Peff\n"},{"id":"406927","messageId":"20201005120738.GA2902357@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005080930.GI1166820@google.com","subject":"Re: [PATCH 6/7] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:07:38Z","receivedAt":"2020-10-05T12:07:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 01:09:30AM -0700, Jonathan Nieder wrote:\n\n> > I note that neither these new tests nor the existing .gitmodules ones\n> > confirm that we catch the obscured ntfs/hfs forms in the actual code\n> > paths (instead, we feed them to a synthetic test-tool helper in t0060).\n> > I think that's OK, but if we wanted to be super-paranoid we could beef\n> > up these tests with trickier names.\n> \n> I think being exhaustive wouldn't be worth it, but perhaps *one*\n> example (e.g., \".gitmodules \") would not be a terrible idea.\n\nIt wasn't _too_ bad to extract these tests into a function, at which\npoint it was easy to test one filename of each type. That will be in my\nv2.\n\n> > +test_expect_success 'refuse to load symlinked .gitattributes into index' '\n> > +\ttest_must_fail git -C symlink-attr read-tree $tree 2>err &&\n> > +\ttest_i18ngrep \"invalid path.*gitattributes\" err\n> \n> This tests that it fails but doesn't test that it had no effect.\n> Would that be straightforward to check as well (e.g. an \"ls-files -s\"\n> before and after)?\n\nYeah, \"ls-files\" should be empty afterwards (we've never added anything\nto the index during the creation steps). That was easy enough to add.\n\n-Peff\n"},{"id":"406928","messageId":"20201005121609.GA2907272@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005071751.GA2290770@coredump.intra.peff.net","subject":"[PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:09Z","receivedAt":"2020-10-05T12:16:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote:\n\n> About 2 years ago as part of a security release we made it illegal to\n> have a symlinked .gitmodules file (refusing it both in the index and via\n> fsck). At the time we discussed (on the security list) outlawing\n> symlinks for other .git files in the same way, but we decided not to do\n> so as part of the security release, as it wasn't strictly necessary.\n> \n> We publicly revisited the topic in:\n> \n> https://lore.kernel.org/git/20190114230902.GG162110@google.com/\n> \n> but there were a few fixes needed, and it got forgotten. So here it is\n> again, with those fixes:\n> [...]\n\nAnd here's a v2 incorporating feedback from Jonathan. There are no\nsubstantial changes in the code. Most of the fixes are cosmetic, but the\ntests are beefed up a bit, as well:\n\n - we now test that ntfs and hfs names are matched via fsck and\n verify_path() for all file types. The bulk of this is in a new patch\n 5, and the final patches are adjusted to use the new helper.\n\n - we confirm that read-tree doesn't write the forbidden entry into the\n index (in addition to seeing that it complains)\n\n - the test script name is now \"bad-dotgitx\" instead of the vague\n \"bad-meta-files\"\n\n - whitespace, typo-fixes, clarity, etc; the range diff is below\n\n [1/8]: fsck_tree(): fix shadowed variable\n [2/8]: fsck_tree(): wrap some long lines\n [3/8]: t7415: rename to expand scope\n [4/8]: t7450: test verify_path() handling of gitmodules\n [5/8]: t7450: test .gitmodules symlink matching against obscured names\n [6/8]: t0060: test obscured .gitattributes and .gitignore matching\n [7/8]: verify_path(): disallow symlinks in .gitattributes and .gitignore\n [8/8]: fsck: complain when .gitattributes or .gitignore is a symlink\n\n fsck.c | 79 +++++++++++----\n read-cache.c | 12 ++-\n t/helper/test-path-utils.c | 41 +++++---\n t/t0060-path-utils.sh | 20 ++++\n ...le-names.sh => t7450-bad-dotgitx-files.sh} | 99 +++++++++++++------\n 5 files changed, 187 insertions(+), 64 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-dotgitx-files.sh} (73%)\n\n1: d4c4b98188 ! 1: 78689a44ba fsck_tree(): fix shadowed variable\n @@ Commit message\n \n Let's rename both variables in the function to avoid confusion. This\n makes the diff a little noisy (e.g., all of the report() calls outside\n - the loop wee already correct but need touched), but makes sure we catch\n - all cases and will avoid similar confusion in the future.\n + the loop were already correct but need to be touched), but makes sure we\n + catch all cases and will avoid similar confusion in the future.\n +\n + Note that our test change removes the comment about translation. It was\n + arguably confusing since 674ba34038 (fsck: mark strings for translation,\n + 2018-11-10); we wouldn't translate gitmodulesSymlink, but it did get\n + removed by GETTEXT_POISON because that feature eats embedded\n + %s characters. But certainly after this patch, when we look for the\n + \"tree %s: %s\" format, we could get foiled by translation.\n \n Signed-off-by: Jeff King \n \n @@ t/t7415-submodule-names.sh: test_expect_success 'fsck detects symlinked .gitmodu\n +\t\ttree=$(git mktree output &&\n -\t\ttest_i18ngrep gitmodulesSymlink output\n +\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n2: 29d0d3af44 = 2: b1f7ec465c fsck_tree(): wrap some long lines\n3: 8679f0b2f2 ! 3: d26ef683fd t7415: rename to expand scope\n @@ Commit message\n \n Signed-off-by: Jeff King \n \n - ## t/t7415-submodule-names.sh => t/t7450-bad-meta-files.sh ##\n + ## t/t7415-submodule-names.sh => t/t7450-bad-dotgitx-files.sh ##\n @@\n #!/bin/sh\n \n4: 84e58f7f46 ! 4: 493a4c79a3 t7450: test verify_path() handling of gitmodules\n @@ Commit message\n \n Signed-off-by: Jeff King \n \n - ## t/t7450-bad-meta-files.sh ##\n -@@ t/t7450-bad-meta-files.sh: test_expect_success 'index-pack --strict works for non-repo pack' '\n + ## t/t7450-bad-dotgitx-files.sh ##\n +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n -test_expect_success 'fsck detects symlinked .gitmodules file' '\n -+test_expect_success 'create repo with symlinked .gitmodules file' '\n ++test_expect_success 'set up repo with symlinked .gitmodules file' '\n \tgit init symlink &&\n \t(\n \t\tcd symlink &&\n -@@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n @@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodul\n +\t\tcd symlink &&\n \n \t\t# Check not only that we fail, but that it is due to the\n - \t\t# symlink detector; this grep string comes from the config\n -@@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n + \t\t# symlink detector\n +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t)\n '\n \n -+test_expect_success 'refuse to load symlinked .gitmodule into index' '\n ++test_expect_success 'refuse to load symlinked .gitmodules into index' '\n +\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n -+\ttest_i18ngrep \"invalid path.*gitmodules\" err\n ++\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n ++\tgit -C symlink ls-files >out &&\n ++\ttest_must_be_empty out\n +'\n +\n test_expect_success 'fsck detects non-blob .gitmodules' '\n-: ---------- > 5: db5e78ff5b t7450: test .gitmodules symlink matching against obscured names\n5: e141e49a5b ! 6: b5962a75a4 t0060: test obscured .gitattributes and .gitignore matching\n @@ t/helper/test-path-utils.c: static struct test_data dirname_data[] = {\n +\t\tif (!strcmp(\"--not\", *argv))\n +\t\t\texpect = !expect;\n +\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n -+\t\t\t res = error(\"'%s' is %s.%s\", *argv,\n -+\t\t\t\t expect ? \"not \" : \"\", x);\n ++\t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n ++\t\t\t\t expect ? \"not \" : \"\", x);\n +\t\telse\n -+\t\t\tfprintf(stderr, \"ok: '%s' is %s.%s\\n\",\n ++\t\t\tfprintf(stderr, \"ok: '%s' is %s.git%s\\n\",\n +\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n +\t}\n +\treturn !!res;\n @@ t/helper/test-path-utils.c: int cmd__path_utils(int argc, const char **argv)\n -\t\t\t\tfprintf(stderr, \"ok: '%s' is %s.gitmodules\\n\",\n -\t\t\t\t\targv[i], expect ? \"\" : \"not \");\n -\t\treturn !!res;\n -+\t\treturn check_dotgitx(\"gitmodules\", argv + 2,\n ++\t\treturn check_dotgitx(\"modules\", argv + 2,\n +\t\t\t\t is_hfs_dotgitmodules,\n +\t\t\t\t is_ntfs_dotgitmodules);\n +\t}\n +\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitignore\")) {\n -+\t\treturn check_dotgitx(\"gitignore\", argv + 2,\n ++\t\treturn check_dotgitx(\"ignore\", argv + 2,\n +\t\t\t\t is_hfs_dotgitignore,\n +\t\t\t\t is_ntfs_dotgitignore);\n +\t}\n +\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitattributes\")) {\n -+\t\treturn check_dotgitx(\"gitattributes\", argv + 2,\n ++\t\treturn check_dotgitx(\"attributes\", argv + 2,\n +\t\t\t\t is_hfs_dotgitattributes,\n +\t\t\t\t is_ntfs_dotgitattributes);\n \t}\n6: d214bbd8ec ! 7: e4ec698a5b verify_path(): disallow symlinks in .gitattributes and .gitignore\n @@ Commit message\n 0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which\n were done as part of the series touching .gitmodules.\n \n + No tests yet, as we'll add them in a subsequent patch once we have fsck\n + support, too.\n +\n Signed-off-by: Jeff King \n \n ## read-cache.c ##\n @@ read-cache.c: int verify_path(const char *path, unsigned mode)\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\n -\n - ## t/t7450-bad-meta-files.sh ##\n -@@ t/t7450-bad-meta-files.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '\n - \ttest_i18ngrep \"is inside git dir\" err\n - '\n - \n -+test_expect_success 'create repo with symlinked .gitattributes file' '\n -+\tgit init symlink-attr &&\n -+\ttarget=$(echo target | git -C symlink-attr hash-object -w --stdin) &&\n -+\ttree=$(\n -+\t\tprintf \"120000 blob $target\\t.gitattributes\\n\" |\n -+\t\tgit -C symlink-attr mktree\n -+\t)\n -+'\n -+\n -+test_expect_success 'refuse to load symlinked .gitattributes into index' '\n -+\ttest_must_fail git -C symlink-attr read-tree $tree 2>err &&\n -+\ttest_i18ngrep \"invalid path.*gitattributes\" err\n -+'\n -+\n -+test_expect_success 'create repo with symlinked .gitignore file' '\n -+\tgit init symlink-ignore &&\n -+\ttarget=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&\n -+\ttree=$(\n -+\t\tprintf \"120000 blob $target\\t.gitignore\\n\" |\n -+\t\tgit -C symlink-ignore mktree\n -+\t)\n -+'\n -+\n -+test_expect_success 'refuse to load symlinked .gitignore into index' '\n -+\ttest_must_fail git -C symlink-ignore read-tree $tree 2>err &&\n -+\ttest_i18ngrep \"invalid path.*gitignore\" err\n -+'\n -+\n -+\n - test_done\n7: 49423d03b5 ! 8: 58c9ce0f3c fsck: complain when .gitattributes or .gitignore is a symlink\n @@ Commit message\n check to fsck, which matches how we handle .gitmodules symlinks, via\n b7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).\n \n - Note that we won't add these to the existing gitmodules block. Its logic\n - is a bit more complicated, as we also check the content of non-symlink\n - instances we find. But for these new files, there is no content check;\n - we're just looking at the name and mode of the tree entry (and we can\n - avoid even the complicated name checks in the common case that the mode\n - doesn't indicate a symlink).\n + Note that we won't add these to the existing gitmodules block. The logic\n + for gitmodules is a bit more complicated, as we also check the content\n + of non-symlink instances we find. But for these new files, there is no\n + content check; we're just looking at the name and mode of the tree entry\n + (and we can avoid even the complicated name checks in the common case\n + that the mode doesn't indicate a symlink).\n +\n + We can reuse the test helper function we defined for .gitmodules,\n + though (and this covers the verify_path() change from the previous\n + commit, as well).\n \n Signed-off-by: Jeff King \n \n @@ fsck.c: static int fsck_tree(const struct object_id *tree_oid,\n \t\t\twhile (backslash) {\n \t\t\t\tbackslash++;\n \n - ## t/t7450-bad-meta-files.sh ##\n -@@ t/t7450-bad-meta-files.sh: test_expect_success 'refuse to load symlinked .gitattributes into index' '\n - \ttest_i18ngrep \"invalid path.*gitattributes\" err\n - '\n + ## t/t7450-bad-dotgitx-files.sh ##\n +@@ t/t7450-bad-dotgitx-files.sh: check_forbidden_symlink gitmodules vanilla .gitmodules\n + check_forbidden_symlink gitmodules ntfs \".gitmodules .\"\n + check_forbidden_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n -+test_expect_success 'fsck detects symlinked .gitattributes file' '\n -+\ttest_must_fail git -C symlink-attr fsck 2>err &&\n -+\ttest_i18ngrep \"tree $tree: gitattributesSymlink\" err\n -+'\n ++check_forbidden_symlink gitattributes vanilla .gitattributes\n ++check_forbidden_symlink gitattributes ntfs \".gitattributes .\"\n ++check_forbidden_symlink gitattributes hfs \".${u200c}gitattributes\"\n +\n - test_expect_success 'create repo with symlinked .gitignore file' '\n - \tgit init symlink-ignore &&\n - \ttarget=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&\n -@@ t/t7450-bad-meta-files.sh: test_expect_success 'refuse to load symlinked .gitignore into index' '\n - \ttest_i18ngrep \"invalid path.*gitignore\" err\n - '\n - \n -+test_expect_success 'fsck detects symlinked .gitignore file' '\n -+\ttest_must_fail git -C symlink-ignore fsck 2>err &&\n -+\ttest_i18ngrep \"tree $tree: gitignoreSymlink\" err\n -+'\n - \n - test_done\n ++check_forbidden_symlink gitignore vanilla .gitignore\n ++check_forbidden_symlink gitignore ntfs \".gitignore .\"\n ++check_forbidden_symlink gitignore hfs \".${u200c}gitignore\"\n ++\n + test_expect_success 'fsck detects non-blob .gitmodules' '\n + \tgit init non-blob &&\n + \t(\n"},{"id":"406929","messageId":"20201005121627.GA2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 1/8] fsck_tree(): fix shadowed variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:27Z","receivedAt":"2020-10-05T12:16:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b2f2039c2b (fsck: accept an oid instead of a \"struct tree\" for\nfsck_tree(), 2019-10-18) introduced a new \"oid\" parameter to\nfsck_tree(), and we pass it to the report() function when we find\nproblems. However, that is shadowed within the tree-walking loop by the\nexisting \"oid\" variable which we use to store the oid of each tree\nentry. As a result, we may report the wrong oid for some problems we\ndetect within the loop (the entry oid, instead of the tree oid).\n\nOur tests didn't catch this because they checked only that we found the\nexpected fsck problem, not that it was attached to the correct object.\n\nLet's rename both variables in the function to avoid confusion. This\nmakes the diff a little noisy (e.g., all of the report() calls outside\nthe loop were already correct but need to be touched), but makes sure we\ncatch all cases and will avoid similar confusion in the future.\n\nNote that our test change removes the comment about translation. It was\narguably confusing since 674ba34038 (fsck: mark strings for translation,\n2018-11-10); we wouldn't translate gitmodulesSymlink, but it did get\nremoved by GETTEXT_POISON because that feature eats embedded\n%s characters. But certainly after this patch, when we look for the\n\"tree %s: %s\" format, we could get foiled by translation.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 40 +++++++++++++++++++-------------------\n t/t7415-submodule-names.sh | 8 ++++----\n 2 files changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f82e2fe9e3..46a108839f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -633,7 +633,7 @@ static int verify_ordered(unsigned mode1, const char *name1,\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(const struct object_id *oid,\n+static int fsck_tree(const struct object_id *tree_oid,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -654,7 +654,7 @@ static int fsck_tree(const struct object_id *oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -664,11 +664,11 @@ static int fsck_tree(const struct object_id *oid,\n \twhile (desc.size) {\n \t\tunsigned short mode;\n \t\tconst char *name, *backslash;\n-\t\tconst struct object_id *oid;\n+\t\tconst struct object_id *entry_oid;\n \n-\t\toid = tree_entry_extract(&desc, &name, &mode);\n+\t\tentry_oid = tree_entry_extract(&desc, &name, &mode);\n \n-\t\thas_null_sha1 |= is_null_oid(oid);\n+\t\thas_null_sha1 |= is_null_oid(entry_oid);\n \t\thas_full_path |= !!strchr(name, '/');\n \t\thas_empty_name |= !*name;\n \t\thas_dot |= !strcmp(name, \".\");\n@@ -678,10 +678,10 @@ static int fsck_tree(const struct object_id *oid,\n \n \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n \t\t\tif (!S_ISLNK(mode))\n-\t\t\t\toidset_insert(&gitmodules_found, oid);\n+\t\t\t\toidset_insert(&gitmodules_found, entry_oid);\n \t\t\telse\n \t\t\t\tretval += report(options,\n-\t\t\t\t\t\t oid, OBJ_TREE,\n+\t\t\t\t\t\t tree_oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n@@ -692,9 +692,9 @@ static int fsck_tree(const struct object_id *oid,\n \t\t\t\thas_dotgit |= is_ntfs_dotgit(backslash);\n \t\t\t\tif (is_ntfs_dotgitmodules(backslash)) {\n \t\t\t\t\tif (!S_ISLNK(mode))\n-\t\t\t\t\t\toidset_insert(&gitmodules_found, oid);\n+\t\t\t\t\t\toidset_insert(&gitmodules_found, entry_oid);\n \t\t\t\t\telse\n-\t\t\t\t\t\tretval += report(options, oid, OBJ_TREE,\n+\t\t\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t\t\t}\n@@ -703,7 +703,7 @@ static int fsck_tree(const struct object_id *oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -751,25 +751,25 @@ static int fsck_tree(const struct object_id *oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex f70368bc2e..d1781ef10c 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -148,13 +148,13 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} | git mktree &&\n+\t\t} >bad-tree &&\n+\t\ttree=$(git mktree output &&\n-\t\ttest_i18ngrep gitmodulesSymlink output\n+\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n \t)\n '\n \n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406930","messageId":"20201005121630.GB2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 2/8] fsck_tree(): wrap some long lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:30Z","receivedAt":"2020-10-05T12:16:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many calls to report() in fsck_tree() are kept on a single line and are\nquite long. Most were pretty big to begin with, but have gotten even\nlonger over the years as we've added more parameters. Let's accept the\nchurn of wrapping them in order to conform to our usual line limits.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 48 ++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 36 insertions(+), 12 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 46a108839f..024810139b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -654,7 +654,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tstruct name_stack df_dup_candidates = { NULL };\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -703,7 +705,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t FSCK_MSG_BAD_TREE,\n+\t\t\t\t\t \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -751,25 +755,45 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tname_stack_clear(&df_dup_candidates);\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_NULL_SHA1,\n+\t\t\t\t \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_FULL_PATHNAME,\n+\t\t\t\t \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_EMPTY_NAME,\n+\t\t\t\t \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOT,\n+\t\t\t\t \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTDOT,\n+\t\t\t\t \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_HAS_DOTGIT,\n+\t\t\t\t \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_ZERO_PADDED_FILEMODE,\n+\t\t\t\t \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_BAD_FILEMODE,\n+\t\t\t\t \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_DUPLICATE_ENTRIES,\n+\t\t\t\t \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n+\t\t\t\t \"not properly sorted\");\n \treturn retval;\n }\n \n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406931","messageId":"20201005121634.GC2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 3/8] t7415: rename to expand scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:34Z","receivedAt":"2020-10-05T12:16:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This script has already expanded beyond its original intent of \".. in\nsubmodule names\" to include other malicious submodule bits. Let's update\nthe name and description to reflect that, as well as the fact that we'll\nsoon be adding similar tests for other meta-files (.gitattributes, etc).\nWe'll also renumber it to move it out of the group of submodule-specific\ntests.\n\nSigned-off-by: Jeff King \n---\n ...ubmodule-names.sh => t7450-bad-dotgitx-files.sh} | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n rename t/{t7415-submodule-names.sh => t7450-bad-dotgitx-files.sh} (95%)\n\ndiff --git a/t/t7415-submodule-names.sh b/t/t7450-bad-dotgitx-files.sh\nsimilarity index 95%\nrename from t/t7415-submodule-names.sh\nrename to t/t7450-bad-dotgitx-files.sh\nindex d1781ef10c..fb28539948 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7450-bad-dotgitx-files.sh\n@@ -1,9 +1,16 @@\n #!/bin/sh\n \n-test_description='check handling of .. in submodule names\n+test_description='check forbidden or malicious patterns in .git* files\n \n-Exercise the name-checking function on a variety of names, and then give a\n-real-world setup that confirms we catch this in practice.\n+Such as:\n+\n+ - presence of .. in submodule names;\n+ Exercise the name-checking function on a variety of names, and then give a\n+ real-world setup that confirms we catch this in practice.\n+\n+ - nested submodule names\n+\n+ - symlinked .gitmodules, etc\n '\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-pack.sh\n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406932","messageId":"20201005121637.GD2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 4/8] t7450: test verify_path() handling of gitmodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:37Z","receivedAt":"2020-10-05T12:16:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n2018-05-04) made it impossible to load a symlink .gitmodules file into\nthe index. However, there are no tests of this behavior. Let's make sure\nthis case is covered. We can easily reuse the test setup created by\nthe matching b7b1fca175 (fsck: complain when .gitmodules is a symlink,\n2018-05-04).\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-dotgitx-files.sh | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7450-bad-dotgitx-files.sh b/t/t7450-bad-dotgitx-files.sh\nindex fb28539948..8bfd32b10a 100755\n--- a/t/t7450-bad-dotgitx-files.sh\n+++ b/t/t7450-bad-dotgitx-files.sh\n@@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'fsck detects symlinked .gitmodules file' '\n+test_expect_success 'set up repo with symlinked .gitmodules file' '\n \tgit init symlink &&\n \t(\n \t\tcd symlink &&\n@@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '\n \t\t{\n \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree &&\n-\t\ttree=$(git mktree bad-tree\n+\t) &&\n+\ttree=$(git -C symlink mktree err &&\n+\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n+\tgit -C symlink ls-files >out &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406933","messageId":"20201005121639.GE2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 5/8] t7450: test .gitmodules symlink matching against obscured names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:39Z","receivedAt":"2020-10-05T12:16:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t7450 we check that both verify_path() and fsck catch malformed\n.gitmodules entries in trees. However, we don't check that we catch\nfilesystem-equivalent forms of these (e.g., \".GITMOD~1\" on Windows).\nOur name-matching functions are exercised well in t0060, but there's\nnothing to test that we correctly call the matching functions from the\nactual fsck and verify_path() code.\n\nSo instead of testing just .gitmodules, let's repeat our tests for a few\nbasic cases. We don't need to be exhaustive here (t0060 handles that),\nbut just make sure we hit one name of each type.\n\nBesides pushing the tests into a function that takes the path as a\nparameter, we'll need to do a few things:\n\n - adjust the directory name to accommodate the tests running multiple\n times\n\n - set core.protecthfs for index checks. Fsck always protects all types\n by default, but we want to be able to exercise the HFS routines on\n every system. Note that core.protectntfs is already the default\n these days, but it doesn't hurt to explicitly label our need for it.\n\n - we'll also take the filename (\"gitmodules\") as a parameter. All\n calls use the same name for now, but a future patch will extend this\n to handle other .gitfoo files. Note that our fake-content symlink\n destination is somewhat .gitmodules specific. But it isn't necessary\n for other files (which don't do a content check). And it happens to\n be a valid attribute and ignore file anyway.\n\nSigned-off-by: Jeff King \n---\n t/t7450-bad-dotgitx-files.sh | 91 +++++++++++++++++++++---------------\n 1 file changed, 53 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t7450-bad-dotgitx-files.sh b/t/t7450-bad-dotgitx-files.sh\nindex 8bfd32b10a..0cd0f71c39 100755\n--- a/t/t7450-bad-dotgitx-files.sh\n+++ b/t/t7450-bad-dotgitx-files.sh\n@@ -139,44 +139,59 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n \tgrep gitmodulesName output\n '\n \n-test_expect_success 'set up repo with symlinked .gitmodules file' '\n-\tgit init symlink &&\n-\t(\n-\t\tcd symlink &&\n-\n-\t\t# Make the tree directly to avoid index restrictions.\n-\t\t#\n-\t\t# Because symlinks store the target as a blob, choose\n-\t\t# a pathname that could be parsed as a .gitmodules file\n-\t\t# to trick naive non-symlink-aware checking.\n-\t\ttricky=\"[foo]bar=true\" &&\n-\t\tcontent=$(git hash-object -w ../.gitmodules) &&\n-\t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n-\t\t{\n-\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n-\t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n-\t\t} >bad-tree\n-\t) &&\n-\ttree=$(git -C symlink mktree output &&\n-\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n-\t)\n-'\n-\n-test_expect_success 'refuse to load symlinked .gitmodules into index' '\n-\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n-\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n-\tgit -C symlink ls-files >out &&\n-\ttest_must_be_empty out\n-'\n+check_forbidden_symlink () {\n+\tname=$1\n+\ttype=$2\n+\tpath=$3\n+\tdir=symlink-$name-$type\n+\n+\ttest_expect_success \"set up repo with symlinked $name ($type)\" '\n+\t\tgit init $dir &&\n+\t\t(\n+\t\t\tcd $dir &&\n+\n+\t\t\t# Make the tree directly to avoid index restrictions.\n+\t\t\t#\n+\t\t\t# Because symlinks store the target as a blob, choose\n+\t\t\t# a pathname that could be parsed as a .gitmodules file\n+\t\t\t# to trick naive non-symlink-aware checking.\n+\t\t\ttricky=\"[foo]bar=true\" &&\n+\t\t\tcontent=$(git hash-object -w ../.gitmodules) &&\n+\t\t\ttarget=$(printf \"$tricky\" | git hash-object -w --stdin) &&\n+\t\t\t{\n+\t\t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n+\t\t\t\tprintf \"120000 blob $target\\t$path\\n\"\n+\t\t\t} >bad-tree\n+\t\t) &&\n+\t\ttree=$(git -C $dir mktree <$dir/bad-tree)\n+\t'\n+\n+\ttest_expect_success \"fsck detects symlinked $name ($type)\" '\n+\t\t(\n+\t\t\tcd $dir &&\n+\n+\t\t\t# Check not only that we fail, but that it is due to the\n+\t\t\t# symlink detector\n+\t\t\ttest_must_fail git fsck 2>output &&\n+\t\t\ttest_i18ngrep \"tree $tree: ${name}Symlink\" output\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"refuse to load symlinked $name into index ($type)\" '\n+\t\ttest_must_fail \\\n+\t\t\tgit -C $dir \\\n+\t\t\t -c core.protectntfs \\\n+\t\t\t -c core.protecthfs \\\n+\t\t\t read-tree $tree 2>err &&\n+\t\ttest_i18ngrep \"invalid path.*$name\" err &&\n+\t\tgit -C $dir ls-files -s >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+}\n+\n+check_forbidden_symlink gitmodules vanilla .gitmodules\n+check_forbidden_symlink gitmodules ntfs \".gitmodules .\"\n+check_forbidden_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406934","messageId":"20201005121645.GG2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:45Z","receivedAt":"2020-10-05T12:16:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules,\n2018-05-04) we made it impossible to load a .gitmodules file that's a\nsymlink into the index. The security reasons for doing so are described\nthere. We also discussed forbidding symlinks of other .git files as part\nof that fix, but the tradeoff was less compelling:\n\n 1. Unlike .gitmodules, the other files don't have content-level fsck\n checks. So an attacker using symlinks to evade those checks isn't a\n problem.\n\n 2. Unlike .gitmodules, Git will never write .gitignore or\n .gitattributes itself, making it much less likely to use them to\n write outside the repo. They could be used for out-of-repo reads,\n however.\n\n 3. The .gitmodules change was part of a critical bug-fix that was\n not publicly disclosed until it was released. Changing the other\n files was not needed for the minimal fix.\n\nHowever, it's still a reasonable idea to forbid symlinks for these\nfiles:\n\n - As noted, they can still be used to read out-of-repo files (which is\n fairly restricted, but in some circumstances you can probe file\n content by speculatively creating files and seeing if they get\n ignored)\n\n - They don't currently behave well in all cases. We sometimes read\n these files from the index, where we _don't_ follow symlinks (we'd\n just treat the symlink target as the .gitignore or .gitattributes\n content, which is actively wrong).\n\nThis patch forbids symlinked versions of these files from entering the\nindex. We already have helpers for obscured forms of the names from\ne7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11) and\n0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which\nwere done as part of the series touching .gitmodules.\n\nNo tests yet, as we'll add them in a subsequent patch once we have fsck\nsupport, too.\n\nSigned-off-by: Jeff King \n---\n read-cache.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex ecf6f68994..63aec6c35d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -947,7 +947,9 @@ static int verify_dotfile(const char *rest, unsigned mode)\n \t\t\treturn 0;\n \t\tif (S_ISLNK(mode)) {\n \t\t\trest += 3;\n-\t\t\tif (skip_iprefix(rest, \"modules\", &rest) &&\n+\t\t\tif ((skip_iprefix(rest, \"modules\", &rest) ||\n+\t\t\t skip_iprefix(rest, \"ignore\", &rest) ||\n+\t\t\t skip_iprefix(rest, \"attributes\", &rest)) &&\n \t\t\t (*rest == '\\0' || is_dir_sep(*rest)))\n \t\t\t\treturn 0;\n \t\t}\n@@ -980,7 +982,9 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t\tif (is_hfs_dotgit(path))\n \t\t\t\t\treturn 0;\n \t\t\t\tif (S_ISLNK(mode)) {\n-\t\t\t\t\tif (is_hfs_dotgitmodules(path))\n+\t\t\t\t\tif (is_hfs_dotgitmodules(path) ||\n+\t\t\t\t\t is_hfs_dotgitignore(path) ||\n+\t\t\t\t\t is_hfs_dotgitattributes(path))\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\n@@ -992,7 +996,9 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t\tif (is_ntfs_dotgit(path))\n \t\t\t\t\treturn 0;\n \t\t\t\tif (S_ISLNK(mode)) {\n-\t\t\t\t\tif (is_ntfs_dotgitmodules(path))\n+\t\t\t\t\tif (is_ntfs_dotgitmodules(path) ||\n+\t\t\t\t\t is_ntfs_dotgitignore(path) ||\n+\t\t\t\t\t is_ntfs_dotgitattributes(path))\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406935","messageId":"20201005121643.GF2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 6/8] t0060: test obscured .gitattributes and .gitignore matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:43Z","receivedAt":"2020-10-05T12:16:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have tests that cover various filesystem-specific spellings of\n\".gitmodules\", because we need to reliably identify that path for some\nsecurity checks. These are from dc2d9ba318 (is_{hfs,ntfs}_dotgitmodules:\nadd tests, 2018-05-12), with the actual code coming from e7cb0b4455\n(is_ntfs_dotgit: match other .git files, 2018-05-11) and 0fc333ba20\n(is_hfs_dotgit: match other .git files, 2018-05-02).\n\nThose latter two commits also added similar matching functions for\n.gitattributes and .gitignore. These ended up not being used in the\nfinal series, and are currently dead code. But in preparation for them\nbeing used, let's make sure they actually work by throwing a few basic\nchecks at them.\n\nI didn't bother with the whole battery of tests that we cover for\n.gitmodules. These functions are all based on the same generic matcher,\nso it's sufficient to test most of the corner cases just once.\n\nNote that the ntfs magic prefix names in the tests come from the\nalgorithm described in e7cb0b4455 (and are different for each file).\n\nSigned-off-by: Jeff King \n---\n t/helper/test-path-utils.c | 41 ++++++++++++++++++++++++++------------\n t/t0060-path-utils.sh | 20 +++++++++++++++++++\n 2 files changed, 48 insertions(+), 13 deletions(-)\n\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 313a153209..732372b97f 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -172,9 +172,22 @@ static struct test_data dirname_data[] = {\n \t{ NULL, NULL }\n };\n \n-static int is_dotgitmodules(const char *path)\n+static int check_dotgitx(const char *x, const char **argv,\n+\t\t\t int (*is_hfs)(const char *),\n+\t\t\t int (*is_ntfs)(const char *))\n {\n-\treturn is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);\n+\tint res = 0, expect = 1;\n+\tfor (; *argv; argv++) {\n+\t\tif (!strcmp(\"--not\", *argv))\n+\t\t\texpect = !expect;\n+\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n+\t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n+\t\t\t\t expect ? \"not \" : \"\", x);\n+\t\telse\n+\t\t\tfprintf(stderr, \"ok: '%s' is %s.git%s\\n\",\n+\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n+\t}\n+\treturn !!res;\n }\n \n static int cmp_by_st_size(const void *a, const void *b)\n@@ -382,17 +395,19 @@ int cmd__path_utils(int argc, const char **argv)\n \t\treturn test_function(dirname_data, posix_dirname, argv[1]);\n \n \tif (argc > 2 && !strcmp(argv[1], \"is_dotgitmodules\")) {\n-\t\tint res = 0, expect = 1, i;\n-\t\tfor (i = 2; i < argc; i++)\n-\t\t\tif (!strcmp(\"--not\", argv[i]))\n-\t\t\t\texpect = !expect;\n-\t\t\telse if (expect != is_dotgitmodules(argv[i]))\n-\t\t\t\tres = error(\"'%s' is %s.gitmodules\", argv[i],\n-\t\t\t\t\t expect ? \"not \" : \"\");\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"ok: '%s' is %s.gitmodules\\n\",\n-\t\t\t\t\targv[i], expect ? \"\" : \"not \");\n-\t\treturn !!res;\n+\t\treturn check_dotgitx(\"modules\", argv + 2,\n+\t\t\t\t is_hfs_dotgitmodules,\n+\t\t\t\t is_ntfs_dotgitmodules);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitignore\")) {\n+\t\treturn check_dotgitx(\"ignore\", argv + 2,\n+\t\t\t\t is_hfs_dotgitignore,\n+\t\t\t\t is_ntfs_dotgitignore);\n+\t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitattributes\")) {\n+\t\treturn check_dotgitx(\"attributes\", argv + 2,\n+\t\t\t\t is_hfs_dotgitattributes,\n+\t\t\t\t is_ntfs_dotgitattributes);\n \t}\n \n \tif (argc > 2 && !strcmp(argv[1], \"file-size\")) {\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 56db5c8aba..b2e3cf3f4c 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -468,6 +468,26 @@ test_expect_success 'match .gitmodules' '\n \t\t.gitmodules,:\\$DATA\n '\n \n+test_expect_success 'match .gitattributes' '\n+\ttest-tool path-utils is_dotgitattributes \\\n+\t\t.gitattributes \\\n+\t\t.git${u200c}attributes \\\n+\t\t.Gitattributes \\\n+\t\t.gitattributeS \\\n+\t\tGITATT~1 \\\n+\t\tGI7D29~1\n+'\n+\n+test_expect_success 'match .gitignore' '\n+\ttest-tool path-utils is_dotgitignore \\\n+\t\t.gitignore \\\n+\t\t.git${u200c}ignore \\\n+\t\t.Gitignore \\\n+\t\t.gitignorE \\\n+\t\tGITIGN~1 \\\n+\t\tGI250A~1\n+'\n+\n test_expect_success MINGW 'is_valid_path() on Windows' '\n \ttest-tool path-utils is_valid_path \\\n \t\twin32 \\\n-- \n2.28.0.1295.gf70bcb366f\n\n"},{"id":"406936","messageId":"20201005121648.GH2907394@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"[PATCH v2 8/8] fsck: complain when .gitattributes or .gitignore is a symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-05T12:16:48Z","receivedAt":"2020-10-05T12:17:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit made it impossible to have a symlinked\n.gitattributes or .gitignore file via verify_path(). Let's add the same\ncheck to fsck, which matches how we handle .gitmodules symlinks, via\nb7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).\n\nNote that we won't add these to the existing gitmodules block. The logic\nfor gitmodules is a bit more complicated, as we also check the content\nof non-symlink instances we find. But for these new files, there is no\ncontent check; we're just looking at the name and mode of the tree entry\n(and we can avoid even the complicated name checks in the common case\nthat the mode doesn't indicate a symlink).\n\nWe can reuse the test helper function we defined for .gitmodules,\nthough (and this covers the verify_path() change from the previous\ncommit, as well).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 15 +++++++++++++++\n t/t7450-bad-dotgitx-files.sh | 8 ++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 024810139b..fcd3f268b1 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -67,6 +67,8 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(GITMODULES_URL, ERROR) \\\n \tFUNC(GITMODULES_PATH, ERROR) \\\n \tFUNC(GITMODULES_UPDATE, ERROR) \\\n+\tFUNC(GITIGNORE_SYMLINK, ERROR) \\\n+\tFUNC(GITATTRIBUTES_SYMLINK, ERROR) \\\n \t/* warnings */ \\\n \tFUNC(BAD_FILEMODE, WARN) \\\n \tFUNC(EMPTY_NAME, WARN) \\\n@@ -688,6 +690,19 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n \n+\t\tif (S_ISLNK(mode)) {\n+\t\t\tif (is_hfs_dotgitignore(name) ||\n+\t\t\t is_ntfs_dotgitignore(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITIGNORE_SYMLINK,\n+\t\t\t\t\t\t \".gitignore is a symlink\");\n+\t\t\tif (is_hfs_dotgitattributes(name) ||\n+\t\t\t is_ntfs_dotgitattributes(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_GITATTRIBUTES_SYMLINK,\n+\t\t\t\t\t\t \".gitattributes is a symlink\");\n+\t\t}\n+\n \t\tif ((backslash = strchr(name, '\\\\'))) {\n \t\t\twhile (backslash) {\n \t\t\t\tbackslash++;\ndiff --git a/t/t7450-bad-dotgitx-files.sh b/t/t7450-bad-dotgitx-files.sh\nindex 0cd0f71c39..326b34e167 100755\n--- a/t/t7450-bad-dotgitx-files.sh\n+++ b/t/t7450-bad-dotgitx-files.sh\n@@ -193,6 +193,14 @@ check_forbidden_symlink gitmodules vanilla .gitmodules\n check_forbidden_symlink gitmodules ntfs \".gitmodules .\"\n check_forbidden_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n+check_forbidden_symlink gitattributes vanilla .gitattributes\n+check_forbidden_symlink gitattributes ntfs \".gitattributes .\"\n+check_forbidden_symlink gitattributes hfs \".${u200c}gitattributes\"\n+\n+check_forbidden_symlink gitignore vanilla .gitignore\n+check_forbidden_symlink gitignore ntfs \".gitignore .\"\n+check_forbidden_symlink gitignore hfs \".${u200c}gitignore\"\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\n-- \n2.28.0.1295.gf70bcb366f\n"},{"id":"406967","messageId":"nycvar.QRO.7.76.6.2010052306470.50@tvgsbejvaqbjf.bet","threadId":"54345","inReplyTo":"20201005084051.GD2862927@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-05T21:20:48Z","receivedAt":"2020-10-05T21:20:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Jonathan N,\n\nOn Mon, 5 Oct 2020, Jeff King wrote:\n\n> On Mon, Oct 05, 2020 at 01:03:53AM -0700, Jonathan Nieder wrote:\n>\n> > > Note that the ntfs magic prefix names in the tests come from the\n> > > algorithm described in e7cb0b4455 (and are different for each file).\n> >\n> > Doesn't block this patch, but I'm curious: how hard would it be to make\n> > a test with an NTFS prerequisite that makes sure we got the magic prefix\n> > right?\n>\n> I suspect hard since Dscho punted on it in the original series. :) If I\n> understand correctly, it would require having an NTFS filesystem, and\n> generating 10,000+ files with a clashing prefix.\n\nIt's not quite _as_ bad: you only need to generate 4 files with a clashing\nprefix and then the real one:\n\n-- snip --\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitattributes1\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitattributes2\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitattributes3\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitattributes4\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitattributes\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitignore1\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitignore2\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitignore3\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitignore4\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ touch .gitignore\n\nme@work MINGW64 ~/repros/ntfs-short-names\n$ cmd //c dir //x\n Volume in drive C is OSDisk\n Volume Serial Number is 5E6B-4E77\n\n Directory of C:\\Users\\me\\repros\\ntfs-short-names\n\n10/05/2020 11:11 PM .\n10/05/2020 11:11 PM ..\n10/05/2020 11:08 PM 0 GI7D29~1 .gitattributes\n10/05/2020 11:08 PM 0 GITATT~1 .gitattributes1\n10/05/2020 11:08 PM 0 GITATT~2 .gitattributes2\n10/05/2020 11:08 PM 0 GITATT~3 .gitattributes3\n10/05/2020 11:08 PM 0 GITATT~4 .gitattributes4\n10/05/2020 11:11 PM 0 GI250A~1 .gitignore\n10/05/2020 11:11 PM 0 GITIGN~1 .gitignore1\n10/05/2020 11:11 PM 0 GITIGN~2 .gitignore2\n10/05/2020 11:11 PM 0 GITIGN~3 .gitignore3\n10/05/2020 11:11 PM 0 GITIGN~4 .gitignore4\n 10 File(s) 0 bytes\n 2 Dir(s) 314,658,705,408 bytes free\n-- snap --\n\nBut I don't necessarily think that it would make sense to add that test:\nit adds churn _every_ time the regression test is run, and by deity, it\nsure takes way too long on Windows _already_, and the test would be for a\nregression _in the NTFS driver_.\n\nAt this stage, I also highly doubt that the algorithm will change ever\nagain (the last time it changed was several Windows versions ago, I want\nto say in Windows XP, but it could have been all the way back to NT).\n\nIn light of that, I'd say that the bang is rather small and the buck would\nbe not small at all, and would have to be paid by developers on Windows\nwho already pay a disproportionately high price when running the test\nsuite, so...\n\nCiao,\nDscho\n"},{"id":"406991","messageId":"20201006140158.GA6601@coredump.intra.peff.net","threadId":"54345","inReplyTo":"nycvar.QRO.7.76.6.2010052306470.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-06T14:01:58Z","receivedAt":"2020-10-06T14:02:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 05, 2020 at 11:20:48PM +0200, Johannes Schindelin wrote:\n\n> > > > Note that the ntfs magic prefix names in the tests come from the\n> > > > algorithm described in e7cb0b4455 (and are different for each file).\n> > >\n> > > Doesn't block this patch, but I'm curious: how hard would it be to make\n> > > a test with an NTFS prerequisite that makes sure we got the magic prefix\n> > > right?\n> >\n> > I suspect hard since Dscho punted on it in the original series. :) If I\n> > understand correctly, it would require having an NTFS filesystem, and\n> > generating 10,000+ files with a clashing prefix.\n> \n> It's not quite _as_ bad: you only need to generate 4 files with a clashing\n> prefix and then the real one:\n\nAh, that really isn't that bad, then. Still, I don't mind leaving this\nas-is under the notion that if the algorithm does change, it would\nlikely make it onto your radar anyway (or the radar of _anybody_ who\nwould raise the issue).\n\n-Peff\n"},{"id":"407005","messageId":"xmqq1rib5bp4.fsf@gitster.c.googlers.com","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-06T20:41:43Z","receivedAt":"2020-10-06T20:41:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote:\n>\n>> About 2 years ago as part of a security release we made it illegal to\n>> have a symlinked .gitmodules file (refusing it both in the index and via\n>> fsck). At the time we discussed (on the security list) outlawing\n>> symlinks for other .git files in the same way, but we decided not to do\n>> so as part of the security release, as it wasn't strictly necessary.\n>> \n>> We publicly revisited the topic in:\n>> \n>> https://lore.kernel.org/git/20190114230902.GG162110@google.com/\n>> \n>> but there were a few fixes needed, and it got forgotten. So here it is\n>> again, with those fixes:\n>> [...]\n>\n> And here's a v2 incorporating feedback from Jonathan. There are no\n> substantial changes in the code. Most of the fixes are cosmetic, but the\n> tests are beefed up a bit, as well:\n>\n> - we now test that ntfs and hfs names are matched via fsck and\n> verify_path() for all file types. The bulk of this is in a new patch\n> 5, and the final patches are adjusted to use the new helper.\n>\n> - we confirm that read-tree doesn't write the forbidden entry into the\n> index (in addition to seeing that it complains)\n>\n> - the test script name is now \"bad-dotgitx\" instead of the vague\n> \"bad-meta-files\"\n>\n> - whitespace, typo-fixes, clarity, etc; the range diff is below\n\nThanks for a pleasant read. Will queue.\n\n"},{"id":"408033","messageId":"6c0a0036-e217-a334-2a74-dd59a4592c1f@iee.email","threadId":"54345","inReplyTo":"20201005121609.GA2907272@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-10-20T23:19:25Z","receivedAt":"2020-10-20T23:19:33Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"A rather late comment,\n\nOn 05/10/2020 13:16, Jeff King wrote:\n> On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote:\n>\n>> About 2 years ago as part of a security release we made it illegal to\n>> have a symlinked .gitmodules file (refusing it both in the index and via\n>> fsck). At the time we discussed (on the security list) outlawing\n>> symlinks for other .git files in the same way, but we decided not to do\n>> so as part of the security release, as it wasn't strictly necessary.\n\nIs this something that should be recorded in the documentation, either as a\nsimple (sensible) limitation, or explicitly as a security related safety\nmeasure?\n\nI didn't see any changes to the .txt docs in the change list below.\n\nPhilip\n>>\n>> We publicly revisited the topic in:\n>>\n>> https://lore.kernel.org/git/20190114230902.GG162110@google.com/\n>>\n>> but there were a few fixes needed, and it got forgotten. So here it is\n>> again, with those fixes:\n>> [...]\n> And here's a v2 incorporating feedback from Jonathan. There are no\n> substantial changes in the code. Most of the fixes are cosmetic, but the\n> tests are beefed up a bit, as well:\n>\n> - we now test that ntfs and hfs names are matched via fsck and\n> verify_path() for all file types. The bulk of this is in a new patch\n> 5, and the final patches are adjusted to use the new helper.\n>\n> - we confirm that read-tree doesn't write the forbidden entry into the\n> index (in addition to seeing that it complains)\n>\n> - the test script name is now \"bad-dotgitx\" instead of the vague\n> \"bad-meta-files\"\n>\n> - whitespace, typo-fixes, clarity, etc; the range diff is below\n>\n> [1/8]: fsck_tree(): fix shadowed variable\n> [2/8]: fsck_tree(): wrap some long lines\n> [3/8]: t7415: rename to expand scope\n> [4/8]: t7450: test verify_path() handling of gitmodules\n> [5/8]: t7450: test .gitmodules symlink matching against obscured names\n> [6/8]: t0060: test obscured .gitattributes and .gitignore matching\n> [7/8]: verify_path(): disallow symlinks in .gitattributes and .gitignore\n> [8/8]: fsck: complain when .gitattributes or .gitignore is a symlink\n>\n> fsck.c | 79 +++++++++++----\n> read-cache.c | 12 ++-\n> t/helper/test-path-utils.c | 41 +++++---\n> t/t0060-path-utils.sh | 20 ++++\n> ...le-names.sh => t7450-bad-dotgitx-files.sh} | 99 +++++++++++++------\n> 5 files changed, 187 insertions(+), 64 deletions(-)\n> rename t/{t7415-submodule-names.sh => t7450-bad-dotgitx-files.sh} (73%)\n>\n> 1: d4c4b98188 ! 1: 78689a44ba fsck_tree(): fix shadowed variable\n> @@ Commit message\n> \n> Let's rename both variables in the function to avoid confusion. This\n> makes the diff a little noisy (e.g., all of the report() calls outside\n> - the loop wee already correct but need touched), but makes sure we catch\n> - all cases and will avoid similar confusion in the future.\n> + the loop were already correct but need to be touched), but makes sure we\n> + catch all cases and will avoid similar confusion in the future.\n> +\n> + Note that our test change removes the comment about translation. It was\n> + arguably confusing since 674ba34038 (fsck: mark strings for translation,\n> + 2018-11-10); we wouldn't translate gitmodulesSymlink, but it did get\n> + removed by GETTEXT_POISON because that feature eats embedded\n> + %s characters. But certainly after this patch, when we look for the\n> + \"tree %s: %s\" format, we could get foiled by translation.\n> \n> Signed-off-by: Jeff King \n> \n> @@ t/t7415-submodule-names.sh: test_expect_success 'fsck detects symlinked .gitmodu\n> +\t\ttree=$(git mktree \n> \t\t# Check not only that we fail, but that it is due to the\n> - \t\t# symlink detector; this grep string comes from the config\n> - \t\t# variable name and will not be translated.\n> +-\t\t# symlink detector; this grep string comes from the config\n> +-\t\t# variable name and will not be translated.\n> ++\t\t# symlink detector\n> \t\ttest_must_fail git fsck 2>output &&\n> -\t\ttest_i18ngrep gitmodulesSymlink output\n> +\t\ttest_i18ngrep \"tree $tree: gitmodulesSymlink\" output\n> 2: 29d0d3af44 = 2: b1f7ec465c fsck_tree(): wrap some long lines\n> 3: 8679f0b2f2 ! 3: d26ef683fd t7415: rename to expand scope\n> @@ Commit message\n> \n> Signed-off-by: Jeff King \n> \n> - ## t/t7415-submodule-names.sh => t/t7450-bad-meta-files.sh ##\n> + ## t/t7415-submodule-names.sh => t/t7450-bad-dotgitx-files.sh ##\n> @@\n> #!/bin/sh\n> \n> 4: 84e58f7f46 ! 4: 493a4c79a3 t7450: test verify_path() handling of gitmodules\n> @@ Commit message\n> \n> Signed-off-by: Jeff King \n> \n> - ## t/t7450-bad-meta-files.sh ##\n> -@@ t/t7450-bad-meta-files.sh: test_expect_success 'index-pack --strict works for non-repo pack' '\n> + ## t/t7450-bad-dotgitx-files.sh ##\n> +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'index-pack --strict works for non-repo pack' '\n> \tgrep gitmodulesName output\n> '\n> \n> -test_expect_success 'fsck detects symlinked .gitmodules file' '\n> -+test_expect_success 'create repo with symlinked .gitmodules file' '\n> ++test_expect_success 'set up repo with symlinked .gitmodules file' '\n> \tgit init symlink &&\n> \t(\n> \t\tcd symlink &&\n> -@@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n> +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n> \t\t{\n> \t\t\tprintf \"100644 blob $content\\t$tricky\\n\" &&\n> \t\t\tprintf \"120000 blob $target\\t.gitmodules\\n\"\n> @@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodul\n> +\t\tcd symlink &&\n> \n> \t\t# Check not only that we fail, but that it is due to the\n> - \t\t# symlink detector; this grep string comes from the config\n> -@@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n> + \t\t# symlink detector\n> +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '\n> \t)\n> '\n> \n> -+test_expect_success 'refuse to load symlinked .gitmodule into index' '\n> ++test_expect_success 'refuse to load symlinked .gitmodules into index' '\n> +\ttest_must_fail git -C symlink read-tree $tree 2>err &&\n> -+\ttest_i18ngrep \"invalid path.*gitmodules\" err\n> ++\ttest_i18ngrep \"invalid path.*gitmodules\" err &&\n> ++\tgit -C symlink ls-files >out &&\n> ++\ttest_must_be_empty out\n> +'\n> +\n> test_expect_success 'fsck detects non-blob .gitmodules' '\n> -: ---------- > 5: db5e78ff5b t7450: test .gitmodules symlink matching against obscured names\n> 5: e141e49a5b ! 6: b5962a75a4 t0060: test obscured .gitattributes and .gitignore matching\n> @@ t/helper/test-path-utils.c: static struct test_data dirname_data[] = {\n> +\t\tif (!strcmp(\"--not\", *argv))\n> +\t\t\texpect = !expect;\n> +\t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n> -+\t\t\t res = error(\"'%s' is %s.%s\", *argv,\n> -+\t\t\t\t expect ? \"not \" : \"\", x);\n> ++\t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n> ++\t\t\t\t expect ? \"not \" : \"\", x);\n> +\t\telse\n> -+\t\t\tfprintf(stderr, \"ok: '%s' is %s.%s\\n\",\n> ++\t\t\tfprintf(stderr, \"ok: '%s' is %s.git%s\\n\",\n> +\t\t\t\t*argv, expect ? \"\" : \"not \", x);\n> +\t}\n> +\treturn !!res;\n> @@ t/helper/test-path-utils.c: int cmd__path_utils(int argc, const char **argv)\n> -\t\t\t\tfprintf(stderr, \"ok: '%s' is %s.gitmodules\\n\",\n> -\t\t\t\t\targv[i], expect ? \"\" : \"not \");\n> -\t\treturn !!res;\n> -+\t\treturn check_dotgitx(\"gitmodules\", argv + 2,\n> ++\t\treturn check_dotgitx(\"modules\", argv + 2,\n> +\t\t\t\t is_hfs_dotgitmodules,\n> +\t\t\t\t is_ntfs_dotgitmodules);\n> +\t}\n> +\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitignore\")) {\n> -+\t\treturn check_dotgitx(\"gitignore\", argv + 2,\n> ++\t\treturn check_dotgitx(\"ignore\", argv + 2,\n> +\t\t\t\t is_hfs_dotgitignore,\n> +\t\t\t\t is_ntfs_dotgitignore);\n> +\t}\n> +\tif (argc > 2 && !strcmp(argv[1], \"is_dotgitattributes\")) {\n> -+\t\treturn check_dotgitx(\"gitattributes\", argv + 2,\n> ++\t\treturn check_dotgitx(\"attributes\", argv + 2,\n> +\t\t\t\t is_hfs_dotgitattributes,\n> +\t\t\t\t is_ntfs_dotgitattributes);\n> \t}\n> 6: d214bbd8ec ! 7: e4ec698a5b verify_path(): disallow symlinks in .gitattributes and .gitignore\n> @@ Commit message\n> 0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which\n> were done as part of the series touching .gitmodules.\n> \n> + No tests yet, as we'll add them in a subsequent patch once we have fsck\n> + support, too.\n> +\n> Signed-off-by: Jeff King \n> \n> ## read-cache.c ##\n> @@ read-cache.c: int verify_path(const char *path, unsigned mode)\n> \t\t\t\t\t\treturn 0;\n> \t\t\t\t}\n> \t\t\t}\n> -\n> - ## t/t7450-bad-meta-files.sh ##\n> -@@ t/t7450-bad-meta-files.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '\n> - \ttest_i18ngrep \"is inside git dir\" err\n> - '\n> - \n> -+test_expect_success 'create repo with symlinked .gitattributes file' '\n> -+\tgit init symlink-attr &&\n> -+\ttarget=$(echo target | git -C symlink-attr hash-object -w --stdin) &&\n> -+\ttree=$(\n> -+\t\tprintf \"120000 blob $target\\t.gitattributes\\n\" |\n> -+\t\tgit -C symlink-attr mktree\n> -+\t)\n> -+'\n> -+\n> -+test_expect_success 'refuse to load symlinked .gitattributes into index' '\n> -+\ttest_must_fail git -C symlink-attr read-tree $tree 2>err &&\n> -+\ttest_i18ngrep \"invalid path.*gitattributes\" err\n> -+'\n> -+\n> -+test_expect_success 'create repo with symlinked .gitignore file' '\n> -+\tgit init symlink-ignore &&\n> -+\ttarget=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&\n> -+\ttree=$(\n> -+\t\tprintf \"120000 blob $target\\t.gitignore\\n\" |\n> -+\t\tgit -C symlink-ignore mktree\n> -+\t)\n> -+'\n> -+\n> -+test_expect_success 'refuse to load symlinked .gitignore into index' '\n> -+\ttest_must_fail git -C symlink-ignore read-tree $tree 2>err &&\n> -+\ttest_i18ngrep \"invalid path.*gitignore\" err\n> -+'\n> -+\n> -+\n> - test_done\n> 7: 49423d03b5 ! 8: 58c9ce0f3c fsck: complain when .gitattributes or .gitignore is a symlink\n> @@ Commit message\n> check to fsck, which matches how we handle .gitmodules symlinks, via\n> b7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).\n> \n> - Note that we won't add these to the existing gitmodules block. Its logic\n> - is a bit more complicated, as we also check the content of non-symlink\n> - instances we find. But for these new files, there is no content check;\n> - we're just looking at the name and mode of the tree entry (and we can\n> - avoid even the complicated name checks in the common case that the mode\n> - doesn't indicate a symlink).\n> + Note that we won't add these to the existing gitmodules block. The logic\n> + for gitmodules is a bit more complicated, as we also check the content\n> + of non-symlink instances we find. But for these new files, there is no\n> + content check; we're just looking at the name and mode of the tree entry\n> + (and we can avoid even the complicated name checks in the common case\n> + that the mode doesn't indicate a symlink).\n> +\n> + We can reuse the test helper function we defined for .gitmodules,\n> + though (and this covers the verify_path() change from the previous\n> + commit, as well).\n> \n> Signed-off-by: Jeff King \n> \n> @@ fsck.c: static int fsck_tree(const struct object_id *tree_oid,\n> \t\t\twhile (backslash) {\n> \t\t\t\tbackslash++;\n> \n> - ## t/t7450-bad-meta-files.sh ##\n> -@@ t/t7450-bad-meta-files.sh: test_expect_success 'refuse to load symlinked .gitattributes into index' '\n> - \ttest_i18ngrep \"invalid path.*gitattributes\" err\n> - '\n> + ## t/t7450-bad-dotgitx-files.sh ##\n> +@@ t/t7450-bad-dotgitx-files.sh: check_forbidden_symlink gitmodules vanilla .gitmodules\n> + check_forbidden_symlink gitmodules ntfs \".gitmodules .\"\n> + check_forbidden_symlink gitmodules hfs \".${u200c}gitmodules\"\n> \n> -+test_expect_success 'fsck detects symlinked .gitattributes file' '\n> -+\ttest_must_fail git -C symlink-attr fsck 2>err &&\n> -+\ttest_i18ngrep \"tree $tree: gitattributesSymlink\" err\n> -+'\n> ++check_forbidden_symlink gitattributes vanilla .gitattributes\n> ++check_forbidden_symlink gitattributes ntfs \".gitattributes .\"\n> ++check_forbidden_symlink gitattributes hfs \".${u200c}gitattributes\"\n> +\n> - test_expect_success 'create repo with symlinked .gitignore file' '\n> - \tgit init symlink-ignore &&\n> - \ttarget=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&\n> -@@ t/t7450-bad-meta-files.sh: test_expect_success 'refuse to load symlinked .gitignore into index' '\n> - \ttest_i18ngrep \"invalid path.*gitignore\" err\n> - '\n> - \n> -+test_expect_success 'fsck detects symlinked .gitignore file' '\n> -+\ttest_must_fail git -C symlink-ignore fsck 2>err &&\n> -+\ttest_i18ngrep \"tree $tree: gitignoreSymlink\" err\n> -+'\n> - \n> - test_done\n> ++check_forbidden_symlink gitignore vanilla .gitignore\n> ++check_forbidden_symlink gitignore ntfs \".gitignore .\"\n> ++check_forbidden_symlink gitignore hfs \".${u200c}gitignore\"\n> ++\n> + test_expect_success 'fsck detects non-blob .gitmodules' '\n> + \tgit init non-blob &&\n> + \t(\n\n"},{"id":"408212","messageId":"20201023081711.GB4012156@coredump.intra.peff.net","threadId":"54345","inReplyTo":"6c0a0036-e217-a334-2a74-dd59a4592c1f@iee.email","subject":"[PATCH] documentation symlink restrictions for .git* files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T08:17:11Z","receivedAt":"2020-10-23T08:17:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 21, 2020 at 12:19:25AM +0100, Philip Oakley wrote:\n\n> On 05/10/2020 13:16, Jeff King wrote:\n> > On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote:\n> >\n> >> About 2 years ago as part of a security release we made it illegal to\n> >> have a symlinked .gitmodules file (refusing it both in the index and via\n> >> fsck). At the time we discussed (on the security list) outlawing\n> >> symlinks for other .git files in the same way, but we decided not to do\n> >> so as part of the security release, as it wasn't strictly necessary.\n> \n> Is this something that should be recorded in the documentation, either as a\n> simple (sensible) limitation, or explicitly as a security related safety\n> measure?\n> \n> I didn't see any changes to the .txt docs in the change list below.\n\nYeah, that's a good point.\n\nHow about this (on top of jk/symlinked-dotgitx-files)?\n\n-- >8 --\nSubject: [PATCH] documentation symlink restrictions for .git* files\n\nWe outlawed symbolic link versions of various .git files in 10ecfa7649\n(verify_path: disallow symlinks in .gitmodules, 2018-05-04) and\ndd4c2fe66b (verify_path(): disallow symlinks in .gitattributes and\n.gitignore, 2020-10-05). The reasons are discussed in detail there, but\nwe never adjusted the documentation to let users know.\n\nThis hasn't been a big deal since the point is that such setups were\nmildly broken and thought to be unusual anyway. But it certainly doesn't\nhurt to be clear and explicit about it.\n\nSuggested-by: Philip Oakley \nSigned-off-by: Jeff King \n---\n Documentation/gitattributes.txt | 7 +++++++\n Documentation/gitignore.txt | 5 +++++\n Documentation/gitmodules.txt | 8 ++++++++\n 3 files changed, 20 insertions(+)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 2d0a03715b..9a2ce4f1ea 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1241,6 +1241,13 @@ to:\n [attr]binary -diff -merge -text\n ------------\n \n+NOTES\n+-----\n+\n+Note that Git does not allow a `.gitattributes` file within the working\n+tree to be a symbolic link, and will refuse to check out such a tree\n+entry. This keeps behavior consistent when the file is accessed from\n+the index or a tree versus from the filesystem.\n \n EXAMPLES\n --------\ndiff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\nindex d47b1ae296..7e9a1d49d6 100644\n--- a/Documentation/gitignore.txt\n+++ b/Documentation/gitignore.txt\n@@ -149,6 +149,11 @@ not tracked by Git remain untracked.\n To stop tracking a file that is currently tracked, use\n 'git rm --cached'.\n \n+Note that Git does not allow a `.gitignore` file within the working tree\n+to be a symbolic link, and will refuse to check out such a tree entry.\n+This keeps behavior consistent when the file is accessed from the index\n+or a tree versus from the filesystem.\n+\n EXAMPLES\n --------\n \ndiff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt\nindex 539b4e1997..2b884be3c7 100644\n--- a/Documentation/gitmodules.txt\n+++ b/Documentation/gitmodules.txt\n@@ -98,6 +98,14 @@ submodule..shallow::\n \tshallow clone (with a history depth of 1) unless the user explicitly\n \tasks for a non-shallow clone.\n \n+NOTES\n+-----\n+\n+Note that Git does not allow the `.gitmodules` file within a working\n+tree to be a symbolic link, and will refuse to check out such a tree\n+entry. This keeps behavior consistent when the file is accessed from the\n+index or a tree versus from the filesystem, and helps Git reliably\n+enforce security checks of the file contents.\n \n EXAMPLES\n --------\n-- \n2.29.0.583.g8e3ac41d8f\n\n"},{"id":"408213","messageId":"20201023082741.GC4012156@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201023081711.GB4012156@coredump.intra.peff.net","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-23T08:27:41Z","receivedAt":"2020-10-23T08:27:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 23, 2020 at 04:17:11AM -0400, Jeff King wrote:\n\n> Subject: [PATCH] documentation symlink restrictions for .git* files\n\nOops, that should be \"document symlink\" of course.\n\n-Peff\n"},{"id":"408465","messageId":"8bb54778-507c-2e3f-d35a-5e32edb8bee7@iee.email","threadId":"54345","inReplyTo":"20201023081711.GB4012156@coredump.intra.peff.net","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-10-26T22:18:18Z","receivedAt":"2020-10-26T22:18:25Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 23/10/2020 09:17, Jeff King wrote:\n> On Wed, Oct 21, 2020 at 12:19:25AM +0100, Philip Oakley wrote:\n>\n>> On 05/10/2020 13:16, Jeff King wrote:\n>>> On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote:\n>>>\n>>>> About 2 years ago as part of a security release we made it illegal to\n>>>> have a symlinked .gitmodules file (refusing it both in the index and via\n>>>> fsck). At the time we discussed (on the security list) outlawing\n>>>> symlinks for other .git files in the same way, but we decided not to do\n>>>> so as part of the security release, as it wasn't strictly necessary.\n>> Is this something that should be recorded in the documentation, either as a\n>> simple (sensible) limitation, or explicitly as a security related safety\n>> measure?\n>>\n>> I didn't see any changes to the .txt docs in the change list below.\n> Yeah, that's a good point.\n>\n> How about this (on top of jk/symlinked-dotgitx-files)?\n>\n> -- >8 --\n> Subject: [PATCH] documentation symlink restrictions for .git* files\n>\n> We outlawed symbolic link versions of various .git files in 10ecfa7649\n> (verify_path: disallow symlinks in .gitmodules, 2018-05-04) and\n> dd4c2fe66b (verify_path(): disallow symlinks in .gitattributes and\n> .gitignore, 2020-10-05). The reasons are discussed in detail there, but\n> we never adjusted the documentation to let users know.\n>\n> This hasn't been a big deal since the point is that such setups were\n> mildly broken and thought to be unusual anyway. But it certainly doesn't\n> hurt to be clear and explicit about it.\n>\n> Suggested-by: Philip Oakley \n> Signed-off-by: Jeff King \n> ---\n> Documentation/gitattributes.txt | 7 +++++++\n> Documentation/gitignore.txt | 5 +++++\n> Documentation/gitmodules.txt | 8 ++++++++\n> 3 files changed, 20 insertions(+)\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 2d0a03715b..9a2ce4f1ea 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -1241,6 +1241,13 @@ to:\n> [attr]binary -diff -merge -text\n> ------------\n> \n> +NOTES\n> +-----\n> +\n> +Note that Git does not allow a `.gitattributes` file within the working\n> +tree to be a symbolic link, and will refuse to check out such a tree\n> +entry. This keeps behavior consistent when the file is accessed from\n> +the index or a tree versus from the filesystem.\n> \n> EXAMPLES\n> --------\n> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\n> index d47b1ae296..7e9a1d49d6 100644\n> --- a/Documentation/gitignore.txt\n> +++ b/Documentation/gitignore.txt\n> @@ -149,6 +149,11 @@ not tracked by Git remain untracked.\n> To stop tracking a file that is currently tracked, use\n> 'git rm --cached'.\n> \n> +Note that Git does not allow a `.gitignore` file within the working tree\n> +to be a symbolic link, and will refuse to check out such a tree entry.\n> +This keeps behavior consistent when the file is accessed from the index\n> +or a tree versus from the filesystem.\n> +\n> EXAMPLES\n> --------\n> \n> diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt\n> index 539b4e1997..2b884be3c7 100644\n> --- a/Documentation/gitmodules.txt\n> +++ b/Documentation/gitmodules.txt\n> @@ -98,6 +98,14 @@ submodule..shallow::\n> \tshallow clone (with a history depth of 1) unless the user explicitly\n> \tasks for a non-shallow clone.\n> \n> +NOTES\n> +-----\n> +\n> +Note that Git does not allow the `.gitmodules` file within a working\n> +tree to be a symbolic link, and will refuse to check out such a tree\n> +entry. This keeps behavior consistent when the file is accessed from the\n> +index or a tree versus from the filesystem, and helps Git reliably\n> +enforce security checks of the file contents.\n> \n> EXAMPLES\n> --------\nThe text looks good to me, with security point explicitly mentioned just\nfor .gitmodules file.\n\nHowever, is placing the Note so far down appropriate (.gitattributes and\n.gitignore), given that there is within the descriptions a discussion of\nthe priority order for finding those files?\n\nPhilip\n"},{"id":"408467","messageId":"20201026225300.GA23128@coredump.intra.peff.net","threadId":"54345","inReplyTo":"8bb54778-507c-2e3f-d35a-5e32edb8bee7@iee.email","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-26T22:53:00Z","receivedAt":"2020-10-26T22:53:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 10:18:18PM +0000, Philip Oakley wrote:\n\n> > +NOTES\n> > +-----\n> > +\n> > +Note that Git does not allow the `.gitmodules` file within a working\n> > +tree to be a symbolic link, and will refuse to check out such a tree\n> > +entry. This keeps behavior consistent when the file is accessed from the\n> > +index or a tree versus from the filesystem, and helps Git reliably\n> > +enforce security checks of the file contents.\n> > \n> > EXAMPLES\n> > --------\n> The text looks good to me, with security point explicitly mentioned just\n> for .gitmodules file.\n> \n> However, is placing the Note so far down appropriate (.gitattributes and\n> .gitignore), given that there is within the descriptions a discussion of\n> the priority order for finding those files?\n\nDefinitely a \"NOTES\" section should go in that spot, but possibly the\ntext should be in the \"DESCRIPTION\" section. I was worried about\ncluttering that early part with a detail that most people wouldn't care\ntoo much about.\n\nLooks like my patch is in 'next'; do you want to propose a patch moving\nit around on top?\n\n-Peff\n"},{"id":"408469","messageId":"xmqqo8kod0ms.fsf@gitster.c.googlers.com","threadId":"54345","inReplyTo":"20201026225300.GA23128@coredump.intra.peff.net","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-26T23:32:27Z","receivedAt":"2020-10-26T23:32:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Definitely a \"NOTES\" section should go in that spot, but possibly the\n> text should be in the \"DESCRIPTION\" section. I was worried about\n> cluttering that early part with a detail that most people wouldn't care\n> too much about.\n>\n> Looks like my patch is in 'next'; do you want to propose a patch moving\n> it around on top?\n\nIt probably is possible to tweak the introductory text this way\nwithout being unnecessarily loud and keep the NOTES section where it\nis.\n\nI personally found that the placement of new text was OK, but this\nmay be overly compensated by my tendency to discount things that we\nhave discussed recently, which our mind often consider (only for\nrelative a short moment) as more important than they are, relative\nto other things in the same document.\n\n Documentation/gitattributes.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt\nindex 2d0a03715b..fd2de7344a 100644\n--- i/Documentation/gitattributes.txt\n+++ w/Documentation/gitattributes.txt\n@@ -13,8 +13,8 @@ $GIT_DIR/info/attributes, .gitattributes\n DESCRIPTION\n -----------\n \n-A `gitattributes` file is a simple text file that gives\n-`attributes` to pathnames.\n+A `gitattributes` file is a simple text file (it cannot be a\n+symbolic link to anything) that gives `attributes` to pathnames.\n \n Each line in `gitattributes` file is of form:\n \n"},{"id":"408481","messageId":"20201027033518.GH2645313@google.com","threadId":"54345","inReplyTo":"20201005121645.GG2907394@coredump.intra.peff.net","subject":"Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-27T03:35:18Z","receivedAt":"2020-10-27T03:35:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> However, it's still a reasonable idea to forbid symlinks for these\n> files:\n>\n> - As noted, they can still be used to read out-of-repo files (which is\n> fairly restricted, but in some circumstances you can probe file\n> content by speculatively creating files and seeing if they get\n> ignored)\n>\n> - They don't currently behave well in all cases. We sometimes read\n> these files from the index, where we _don't_ follow symlinks (we'd\n> just treat the symlink target as the .gitignore or .gitattributes\n> content, which is actively wrong).\n>\n> This patch forbids symlinked versions of these files from entering the\n> index. We already have helpers for obscured forms of the names from\n> e7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11) and\n> 0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which\n> were done as part of the series touching .gitmodules.\n\nThanks again for this. Since this patch has been in \"next\", we've\ngotten a little experience of it at Google.\n\nWe've been running with the fsck check for a while (more than a year),\nbut not the verify_dotfile check. The verify_dotfile check didn't\ntrigger for anyone with .gitattributes, but it hit several people for\n.gitignore. Some examples that users have mentioned:\n\nBefore https://android-review.googlesource.com/c/platform/tools/test/connectivity/+/1462771/\nAndroid used a .gitignore symlink for two directories that had similar\ngitignore requirements. Diagnosing the error was confusing for them,\nespecially because the \"repo\" wrapper tool produced messages like\n\n\terror.GitError: Cannot initialize work tree for platform/tools/test/connectivity\n\nEventually someone manually ran\n\n\t$ git add --a\n\terror: invalid path 'acts_tests/.gitignore'\n\terror: unable to add 'acts_tests/.gitignore' to index\n\tfatal: adding files failed\n\nwhich helped them realize it was a git issue and helped me point them\nto the need to replace the symlink with a plain file.\n\nAs another example, a user working with the\nhttps://github.com/bakerstu/openmrn.git repository noticed \"git\ncheckout\" commands failing. In this user's case, the checkout failed\npart-way through, producing confusing behavior (\"git status\" showing\nentries missing from the index). When I tried to reproduce this, I\nwasn't able to clone the repository at all because it failed fsck;\nafter disabling transfer.fsckObjects, I still wasn't able to check out\nHEAD.\n\nObservations:\n\n- since some widely used repositories have .gitignore symlinks, I\n think we can't forbid it in fsck, alas\n\n- it would be useful to be able to check whether these symlinks would\n not escape the worktree, for a more targeted check. It might be\n nice to even respect these settings when they would not escape the\n worktree, but not necessarily\n\n- we could use a clearer error message than \"invalid path\". There's\n some room for improvement in \"git checkout\"'s error handling, too\n --- I think my ideal would be if the operation would fail entirely,\n with an advice message describing a checkout command that would\n succeed (But how do I checkout another commit while excluding some\n files? Should it suggest a sparse checkout?).\n\nThat's all I have time for today --- will revisit again tomorrow.\n\nThoughts?\n\nThanks,\nJonathan\n"},{"id":"408490","messageId":"20201027072648.GG3005508@coredump.intra.peff.net","threadId":"54345","inReplyTo":"xmqqo8kod0ms.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-27T07:26:48Z","receivedAt":"2020-10-27T07:26:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 04:32:27PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Definitely a \"NOTES\" section should go in that spot, but possibly the\n> > text should be in the \"DESCRIPTION\" section. I was worried about\n> > cluttering that early part with a detail that most people wouldn't care\n> > too much about.\n> >\n> > Looks like my patch is in 'next'; do you want to propose a patch moving\n> > it around on top?\n> \n> It probably is possible to tweak the introductory text this way\n> without being unnecessarily loud and keep the NOTES section where it\n> is.\n> [...]\n> DESCRIPTION\n> -----------\n> \n> -A `gitattributes` file is a simple text file that gives\n> -`attributes` to pathnames.\n> +A `gitattributes` file is a simple text file (it cannot be a\n> +symbolic link to anything) that gives `attributes` to pathnames.\n\nI worried that even a short mention like this would be distracting. Not\nbecause it's so long, but because it's right there in the very first\nsentence, and I really think this is a corner case that most people\nwould not even think about.\n\nSo it is helpful if you are looking for info on symlinks and these\nfiles, but probably clutter if you are looking for something else.\n\nI have to admit I don't feel all that strongly either way, though.\n\n-Peff\n"},{"id":"408495","messageId":"20201027075853.GH3005508@coredump.intra.peff.net","threadId":"54345","inReplyTo":"20201027033518.GH2645313@google.com","subject":"Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-27T07:58:53Z","receivedAt":"2020-10-27T07:58:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 08:35:18PM -0700, Jonathan Nieder wrote:\n\n> As another example, a user working with the\n> https://github.com/bakerstu/openmrn.git repository noticed \"git\n> checkout\" commands failing. In this user's case, the checkout failed\n> part-way through, producing confusing behavior (\"git status\" showing\n> entries missing from the index). When I tried to reproduce this, I\n> wasn't able to clone the repository at all because it failed fsck;\n> after disabling transfer.fsckObjects, I still wasn't able to check out\n> HEAD.\n\nI wouldn't be surprised if there are code paths which die() early\ninstead of checking out as much as they can. But I think fixing that is\nsomewhat orthogonal (here we're just adding new reasons that checking\nout may fail).\n\n> Observations:\n> \n> - since some widely used repositories have .gitignore symlinks, I\n> think we can't forbid it in fsck, alas\n\nI am a little puzzled here. You said you had the fsck checks for the\nlast year, so why did they just come up now? I guess nobody sets\ntransfer.fsckObjects, and because you were testing only with clients,\nyour server implementation didn't reject pushes?\n\nI agree it's annoying for them if they fail fsck, but it's not entirely\na show-stopper. There are config options for fine-tuning what you're\nwilling to enforce or ignore. But they of course are also annoying to\nuse, because every receiver of a transfer needs to set them (on GitHub,\nfor example, you have to email Support).\n\nSo I won't be too devastated to remove the symlink checks, or possibly\ndowngrade them to purely warnings (or \"info\"; the naming in fsck.c is\nconfusing, because the transfer operations take even warnings as fatal.\nI suspect we could do with some cleanup there).\n\n> - it would be useful to be able to check whether these symlinks would\n> not escape the worktree, for a more targeted check. It might be\n> nice to even respect these settings when they would not escape the\n> worktree, but not necessarily\n\nI actually wrote a patch several years ago for checking symlinks (not\njust these ones, but _any_ symlinks in the repo, but of course it would\nbe easy to limit it more). It's included at the end of this mail. It's\nbeen part of my daily build for many years, so I'm confident it doesn't\ncrash or have other bad behavior. But it's possible the logic for what\nit catches is faulty.\n\n> - we could use a clearer error message than \"invalid path\". \n\nThat part is tricky. The \"invalid path\" error comes from the caller of\nverify_path(), and we have no way to pass back an intelligent error\nthere. We can call error() ourselves, of course. That adds an extra line\nof output, but it's rare enough for verify_path() to fail that it's\nlikely OK. However, I would worry that some callers might be surprised\nby it producing output at all.\n\nAn alternative is letting the caller pass in a strbuf that we fill out\nwith an extra error string.\n\n> There's some room for improvement in \"git checkout\"'s error handling,\n> too --- I think my ideal would be if the operation would fail\n> entirely, with an advice message describing a checkout command that\n> would succeed (But how do I checkout another commit while excluding\n> some files? Should it suggest a sparse checkout?).\n\nI suspect it's too late for \"fail entirely\". We may have already written\nto the filesystem, and rolling back is difficult and error-prone. In\ngeneral I'd expect to checkout what we can, produce errors for the rest,\nand let the user work from there with \"git status\".\n\nBut I may be wrong. The problem is loading the value into the index, not\nwriting it to the filesystem. So perhaps the relevant code paths load\nthe index fully before writing out anything to the filesystem, and it's\neasy to rollback. But I imagine they are using unpack-trees' flag to\nupdate the filesystem, and I assume that checks out as it loads entries\n(but I didn't confirm).\n\n-Peff\n\nHere's the patch. The tests need the checkout-index fix from:\n\n https://lore.kernel.org/git/20201027073000.GA3651896@coredump.intra.peff.net/\n\nbut it should otherwise be stand-alone. I don't recall why I never sent\nit. One obvious downside is that it's difficult to have fsck checks for\nit, since the full path of an entry is not a property of a single tree\nobject.\n\n-- >8 --\nDate: Wed, 9 Nov 2016 23:24:09 -0500\nSubject: [PATCH] optionally block out-of-repo symlinks\n\n---\n apply.c | 2 +-\n cache.h | 3 +\n config.c | 5 ++\n entry.c | 2 +-\n environment.c | 1 +\n merge-recursive.c | 2 +-\n path.c | 73 ++++++++++++++++++\n t/t2031-checkout-symlink-external.sh | 107 +++++++++++++++++++++++++++\n 8 files changed, 192 insertions(+), 3 deletions(-)\n create mode 100755 t/t2031-checkout-symlink-external.sh\n\ndiff --git a/apply.c b/apply.c\nindex 76dba93c97..2ac940dc05 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4360,7 +4360,7 @@ static int try_create_file(struct apply_state *state, const char *path,\n \t\t/* Although buf:size is counted string, it also is NUL\n \t\t * terminated.\n \t\t */\n-\t\treturn !!symlink(buf, path);\n+\t\treturn !!safe_symlink(buf, path);\n \n \tfd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);\n \tif (fd < 0)\ndiff --git a/cache.h b/cache.h\nindex c0072d43b1..9b56e2327a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -959,6 +959,7 @@ extern int precomposed_unicode;\n extern int protect_hfs;\n extern int protect_ntfs;\n extern const char *core_fsmonitor;\n+extern int allow_external_symlinks;\n \n extern int core_apply_sparse_checkout;\n extern int core_sparse_checkout_cone;\n@@ -1979,4 +1980,6 @@ int print_sha1_ellipsis(void);\n /* Return 1 if the file is empty or does not exists, 0 otherwise. */\n int is_empty_or_missing_file(const char *filename);\n \n+int safe_symlink(const char *target, const char *linkpath);\n+\n #endif /* CACHE_H */\ndiff --git a/config.c b/config.c\nindex 2bdff4457b..6699881c6e 100644\n--- a/config.c\n+++ b/config.c\n@@ -1404,6 +1404,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.allowexternalsymlinks\")) {\n+\t\tallow_external_symlinks = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn platform_core_config(var, value, cb);\n }\ndiff --git a/entry.c b/entry.c\nindex a0532f1f00..f2d3456df3 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -291,7 +291,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tif (!has_symlinks || to_tempfile)\n \t\t\tgoto write_file_entry;\n \n-\t\tret = symlink(new_blob, path);\n+\t\tret = safe_symlink(new_blob, path);\n \t\tfree(new_blob);\n \t\tif (ret)\n \t\t\treturn error_errno(\"unable to create symlink %s\", path);\ndiff --git a/environment.c b/environment.c\nindex bb518c61cd..7c233e0e0e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -73,6 +73,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n+int allow_external_symlinks = 1;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d0214335a7..61208a8b43 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -986,7 +986,7 @@ static int update_file_flags(struct merge_options *opt,\n \t\t\tchar *lnk = xmemdupz(buf, size);\n \t\t\tsafe_create_leading_directories_const(path);\n \t\t\tunlink(path);\n-\t\t\tif (symlink(lnk, path))\n+\t\t\tif (safe_symlink(lnk, path))\n \t\t\t\tret = err(opt, _(\"failed to symlink '%s': %s\"),\n \t\t\t\t\t path, strerror(errno));\n \t\t\tfree(lnk);\ndiff --git a/path.c b/path.c\nindex 7b385e5eb2..251dd70ff5 100644\n--- a/path.c\n+++ b/path.c\n@@ -1498,6 +1498,79 @@ int looks_like_command_line_option(const char *str)\n \treturn str && str[0] == '-';\n }\n \n+static int symlink_leaves_repo(const char *target, const char *linkpath)\n+{\n+\t/*\n+\t * Absolute paths are always considered to leave the repository (even\n+\t * if they happen to point to the working tree path).\n+\t */\n+\tif (is_absolute_path(target))\n+\t\treturn 1;\n+\n+\t/*\n+\t * Allow relative paths that start with a sequence of \"../\",\n+\t * as long as they do not break out of the symlink's root.\n+\t * This loop will detect break-out cases and return; otherwise, at the\n+\t * end of the loop \"target\" will point to the first non-\"..\" component.\n+\t *\n+\t * We count the depth of linkpath by eating up directory components left\n+\t * to right. Technically the symlink would resolve right-to-left, but\n+\t * we don't care about the actual values, only the number.\n+\t */\n+\twhile (target[0] == '.') {\n+\t\tif (!target[1]) {\n+\t\t\t/* trailing \".\" -- ignore */\n+\t\t\ttarget++;\n+\t\t} else if (is_dir_sep(target[1])) {\n+\t\t\t/* \"./\" -- ignore */\n+\t\t\ttarget += 2;\n+\t\t} else if (target[1] == '.' &&\n+\t\t\t (!target[2] || is_dir_sep(target[2]))) {\n+\t\t\t/* \"..\" or \"../\" -- drop one from linkpath depth */\n+\t\t\twhile (!is_dir_sep(*linkpath)) {\n+\t\t\t\t/* end-of-string; target exceeded our depth */\n+\t\t\t\tif (!*linkpath)\n+\t\t\t\t\treturn 1;\n+\t\t\t\tlinkpath++;\n+\t\t\t}\n+\t\t\t/* skip final \"/\" */\n+\t\t\tlinkpath++;\n+\n+\t\t\t/* skip past \"..\" */\n+\t\t\ttarget += 2;\n+\t\t\t/* and \"/\" if present */\n+\t\t\tif (is_dir_sep(*target))\n+\t\t\t\ttarget++;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Now we have a path in \"target\" that only go down into the tree.\n+\t * Disallow any interior \"../\", like \"foo/../bar\". These might be\n+\t * OK, but we cannot know unless we know whether \"foo\" is itself a\n+\t * symlink. So err on the side of caution.\n+\t */\n+\twhile (*target) {\n+\t\tconst char *v;\n+\t\tif (skip_prefix(target, \"..\", &v) && (!*v || is_dir_sep(*v)))\n+\t\t\treturn 1;\n+\t\ttarget++;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+int safe_symlink(const char *target, const char *linkpath)\n+{\n+\tif (!allow_external_symlinks &&\n+\t symlink_leaves_repo(target, linkpath)) {\n+\t\terrno = EPERM;\n+\t\treturn -1;\n+\t}\n+\n+\treturn symlink(target, linkpath);\n+}\n+\n char *xdg_config_home(const char *filename)\n {\n \tconst char *home, *config_home;\ndiff --git a/t/t2031-checkout-symlink-external.sh b/t/t2031-checkout-symlink-external.sh\nnew file mode 100755\nindex 0000000000..c799f8158e\n--- /dev/null\n+++ b/t/t2031-checkout-symlink-external.sh\n@@ -0,0 +1,107 @@\n+#!/bin/sh\n+\n+test_description='detection and prevention of out-of-tree symlinks'\n+. ./test-lib.sh\n+\n+if ! test_have_prereq SYMLINKS\n+then\n+\tskip_all='skipping external symlink tests (missing SYMLINKS)'\n+\ttest_done\n+fi\n+\n+create_symlink() {\n+\tsymlink=$1\n+\ttarget=$2\n+\ttest_expect_success \"create symlink ($symlink)\" '\n+\t\tsha1=$(printf \"%s\" \"$target\" | git hash-object -w --stdin) &&\n+\t\tgit update-index --add --cacheinfo \"120000,$sha1,$symlink\"\n+\t'\n+}\n+\n+check_symlink () {\n+\tsymlink=$1\n+\tconfig=$2\n+\toutcome=$3\n+\texpect=$4\n+\n+\tif test \"$outcome\" = \"allow\"\n+\tthen\n+\t\tfail=\n+\t\t: ${expect:=test_cmp ../target}\n+\telse\n+\t\tfail=test_must_fail\n+\t\t: ${expect:=! cat}\n+\tfi\n+\n+\ttest_expect_success \" check symlink ($symlink, $config -> $outcome)\" \"\n+\t\trm -f $symlink &&\n+\t\t$fail git -c core.allowExternalSymlinks=$config \\\\\n+\t\t\tcheckout-index -- $symlink &&\n+\t\t$expect $symlink\n+\t\"\n+}\n+\n+# we want to try breaking out of the repository,\n+# so let's work inside a sub-repository, and break\n+# out to the top-level trash directory\n+test_expect_success 'set up repository' '\n+\techo content >target &&\n+\tgit init subrepo &&\n+\tcd subrepo &&\n+\ttest_commit base &&\n+\techo content >in-repo-target\n+'\n+\n+create_symlink in-repo in-repo-target\n+check_symlink in-repo false allow\n+\n+create_symlink subdir/in-repo ../in-repo-target\n+check_symlink subdir/in-repo false allow\n+\n+create_symlink absolute \"$TRASH_DIRECTORY/target\"\n+check_symlink absolute true allow\n+check_symlink absolute false forbid\n+\n+create_symlink relative \"../target\"\n+check_symlink relative true allow\n+check_symlink relative false forbid\n+\n+create_symlink curdir .\n+check_symlink curdir false allow test_path_is_dir\n+create_symlink sneaky curdir/../target\n+check_symlink sneaky true allow\n+check_symlink sneaky false forbid\n+\n+test_expect_success 'applying a patch checks symlink config' '\n+\tgit diff-index -p --cached HEAD -- relative >patch &&\n+\trm -f relative &&\n+\tgit -c core.allowExternalSymlinks=true apply writes:\n\n>> DESCRIPTION\n>> -----------\n>> \n>> -A `gitattributes` file is a simple text file that gives\n>> -`attributes` to pathnames.\n>> +A `gitattributes` file is a simple text file (it cannot be a\n>> +symbolic link to anything) that gives `attributes` to pathnames.\n>\n> I worried that even a short mention like this would be distracting. Not\n> because it's so long, but because it's right there in the very first\n> sentence, and I really think this is a corner case that most people\n> would not even think about.\n>\n> So it is helpful if you are looking for info on symlinks and these\n> files, but probably clutter if you are looking for something else.\n>\n> I have to admit I don't feel all that strongly either way, though.\n\nI don't, either, and as I said, I found that the placement of new\ntext was OK.\n\n\n"},{"id":"408531","messageId":"21e4bac1-b3ef-e28f-586d-fe17baf6b15e@iee.email","threadId":"54345","inReplyTo":"xmqqk0vbcxtb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-10-27T21:00:53Z","receivedAt":"2020-10-27T21:01:02Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 27/10/2020 18:45, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>>> DESCRIPTION\n>>> -----------\n>>> \n>>> -A `gitattributes` file is a simple text file that gives\n>>> -`attributes` to pathnames.\n>>> +A `gitattributes` file is a simple text file (it cannot be a\n>>> +symbolic link to anything) that gives `attributes` to pathnames.\n>> I worried that even a short mention like this would be distracting. Not\n>> because it's so long, but because it's right there in the very first\n>> sentence, and I really think this is a corner case that most people\n>> would not even think about.\n>>\n>> So it is helpful if you are looking for info on symlinks and these\n>> files, but probably clutter if you are looking for something else.\n>>\n>> I have to admit I don't feel all that strongly either way, though.\n> I don't, either, and as I said, I found that the placement of new\n> text was OK.\n>\n>\nI do think that the extra text above is the right thing to do.\n\nWe should be informing readers early about things that are expressly\nprohibited.\n\nLeaving just the note till nearly the end of the rather long\nattributes/ignore man pages makes it very hard to discover for\nfrustrated users, which would accidentally reinforce the idea of the\ndocs being poor (rather than being focussed).\n\nPhilip\n"},{"id":"408535","messageId":"xmqqv9ev9vnf.fsf@gitster.c.googlers.com","threadId":"54345","inReplyTo":"20201027075853.GH3005508@coredump.intra.peff.net","subject":"Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-27T22:00:36Z","receivedAt":"2020-10-27T22:00:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/environment.c b/environment.c\n> index bb518c61cd..7c233e0e0e 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -73,6 +73,7 @@ int merge_log_config = -1;\n> int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n> unsigned long pack_size_limit_cfg;\n> enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n> +int allow_external_symlinks = 1;\n\nOK, so by default it is not blocked...\n\n> +static int symlink_leaves_repo(const char *target, const char *linkpath)\n> +{\n> +\t/*\n> +\t * Absolute paths are always considered to leave the repository (even\n> +\t * if they happen to point to the working tree path).\n> +\t */\n> +\tif (is_absolute_path(target))\n> +\t\treturn 1;\n\nVery sensible.\n\n> +\t/*\n> +\t * Allow relative paths that start with a sequence of \"../\",\n> +\t * as long as they do not break out of the symlink's root.\n> +\t * This loop will detect break-out cases and return; otherwise, at the\n> +\t * end of the loop \"target\" will point to the first non-\"..\" component.\n> +\t *\n> +\t * We count the depth of linkpath by eating up directory components left\n> +\t * to right. Technically the symlink would resolve right-to-left, but\n> +\t * we don't care about the actual values, only the number.\n> +\t */\n> +\twhile (target[0] == '.') {\n> +\t\tif (!target[1]) {\n> +\t\t\t/* trailing \".\" -- ignore */\n> +\t\t\ttarget++;\n> +\t\t} else if (is_dir_sep(target[1])) {\n> +\t\t\t/* \"./\" -- ignore */\n> +\t\t\ttarget += 2;\n> +\t\t} else if (target[1] == '.' &&\n> +\t\t\t (!target[2] || is_dir_sep(target[2]))) {\n> +\t\t\t/* \"..\" or \"../\" -- drop one from linkpath depth */\n> +\t\t\twhile (!is_dir_sep(*linkpath)) {\n> +\t\t\t\t/* end-of-string; target exceeded our depth */\n> +\t\t\t\tif (!*linkpath)\n> +\t\t\t\t\treturn 1;\n> +\t\t\t\tlinkpath++;\n> +\t\t\t}\n> +\t\t\t/* skip final \"/\" */\n> +\t\t\tlinkpath++;\n> +\n> +\t\t\t/* skip past \"..\" */\n> +\t\t\ttarget += 2;\n> +\t\t\t/* and \"/\" if present */\n> +\t\t\tif (is_dir_sep(*target))\n> +\t\t\t\ttarget++;\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * Now we have a path in \"target\" that only go down into the tree.\n> +\t * Disallow any interior \"../\", like \"foo/../bar\". These might be\n> +\t * OK, but we cannot know unless we know whether \"foo\" is itself a\n> +\t * symlink. So err on the side of caution.\n> +\t */\n> +\twhile (*target) {\n> +\t\tconst char *v;\n> +\t\tif (skip_prefix(target, \"..\", &v) && (!*v || is_dir_sep(*v)))\n> +\t\t\treturn 1;\n> +\t\ttarget++;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int safe_symlink(const char *target, const char *linkpath)\n> +{\n> +\tif (!allow_external_symlinks &&\n> +\t symlink_leaves_repo(target, linkpath)) {\n> +\t\terrno = EPERM;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\treturn symlink(target, linkpath);\n> +}\n\nOK. This is only about blocking creation of new symbolic links that\ngoes outside the working tree. It obviously is a good thing to do.\n\nWe have some \"symlink safety\" in various parts of the system [*1*],\nand I wonder if we can somehow consolidate the support to a more\ncentral place.\n\nThanks.\n\n\n[Footnote]\n\n*1* For example, apply tries to be careful not to take the \"path\"\n recorded in the incoming patch blindly, and instead checks if\n any path component in it is a symbolic link before touching.\n Similarly, callers of has_symlink_leading_path() all try to be\n careful when the \"path\" they want to use to access a filesystem\n entity has a symbolic link in the middle on the filesystem.\n"},{"id":"408547","messageId":"20201027234309.GA1298045@google.com","threadId":"54345","inReplyTo":"20201027075853.GH3005508@coredump.intra.peff.net","subject":"Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-10-27T23:43:09Z","receivedAt":"2020-10-28T01:47:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Oct 26, 2020 at 08:35:18PM -0700, Jonathan Nieder wrote:\n\n>> Observations:\n>>\n>> - since some widely used repositories have .gitignore symlinks, I\n>> think we can't forbid it in fsck, alas\n>\n> I am a little puzzled here. You said you had the fsck checks for the\n> last year, so why did they just come up now? I guess nobody sets\n> transfer.fsckObjects, and because you were testing only with clients,\n> your server implementation didn't reject pushes?\n>\n> I agree it's annoying for them if they fail fsck, but it's not entirely\n> a show-stopper. There are config options for fine-tuning what you're\n> willing to enforce or ignore. But they of course are also annoying to\n> use, because every receiver of a transfer needs to set them (on GitHub,\n> for example, you have to email Support).\n\nYes. And to reiterate the point a little: the reason nobody sets\ntransfer.fsckObjects is that we haven't made it easy to distinguish\nbetween \"hard error, should never be overridden\" checks (like\nBAD_PARENT_SHA1), \"new tools shouldn't write these but they exist in\nimportant repos like perl.git and anything consuming Git repositories\nneeds to cope with them\" (like MISSING_SPACE_BEFORE_DATE from some\ncommits' concatenated authors), and so on.\n\n> So I won't be too devastated to remove the symlink checks, or possibly\n> downgrade them to purely warnings (or \"info\"; the naming in fsck.c is\n> confusing, because the transfer operations take even warnings as fatal.\n> I suspect we could do with some cleanup there).\n\nDowngrading the .gitignore check to warning sounds okay. .gitmodules\nwould still want to be an error, of course.\n\n>> - it would be useful to be able to check whether these symlinks would\n>> not escape the worktree, for a more targeted check. It might be\n>> nice to even respect these settings when they would not escape the\n>> worktree, but not necessarily\n>\n> I actually wrote a patch several years ago for checking symlinks (not\n> just these ones, but _any_ symlinks in the repo, but of course it would\n> be easy to limit it more). It's included at the end of this mail. It's\n> been part of my daily build for many years, so I'm confident it doesn't\n> crash or have other bad behavior. But it's possible the logic for what\n> it catches is faulty.\n[...]\n> Here's the patch.\n\nNice! I'll try to find time to experiment with this.\n\n[...]\n>> - we could use a clearer error message than \"invalid path\".\n>\n> That part is tricky. The \"invalid path\" error comes from the caller of\n> verify_path(), and we have no way to pass back an intelligent error\n> there. We can call error() ourselves, of course. That adds an extra line\n> of output, but it's rare enough for verify_path() to fail that it's\n> likely OK. However, I would worry that some callers might be surprised\n> by it producing output at all.\n>\n> An alternative is letting the caller pass in a strbuf that we fill out\n> with an extra error string.\n\nI think passing in a struct to govern error handling sounds nice.\nAlternatively, this might suggest that verify_path is not the right\nplace for the user-facing check: it's useful because it's exhaustive,\nbut it may be that we can also catch the same issue earlier and\nproduce a nicer diagnostic.\n\n>> There's some room for improvement in \"git checkout\"'s error handling,\n>> too --- I think my ideal would be if the operation would fail\n>> entirely, with an advice message describing a checkout command that\n>> would succeed (But how do I checkout another commit while excluding\n>> some files? Should it suggest a sparse checkout?).\n>\n> I suspect it's too late for \"fail entirely\". We may have already written\n> to the filesystem, and rolling back is difficult and error-prone. In\n> general I'd expect to checkout what we can, produce errors for the rest,\n> and let the user work from there with \"git status\".\n>\n> But I may be wrong. The problem is loading the value into the index, not\n> writing it to the filesystem. So perhaps the relevant code paths load\n> the index fully before writing out anything to the filesystem, and it's\n> easy to rollback. But I imagine they are using unpack-trees' flag to\n> update the filesystem, and I assume that checks out as it loads entries\n> (but I didn't confirm).\n\nRight, this would require taking two passes. I think we do already\nperform two passes (first deletions, then additions); I'll have to\nlook a little more closely to see whether this is straightforward to\ndo.\n\nI do still like the idea of not respecting .gitignore and\n.gitattributes symlinks, by the way. What the experiences upthread\ntell me is just that users need better facilities for repairing\nexisting repositories.\n\nThanks,\nJonathan\n"},{"id":"408551","messageId":"xmqqeeli9n2j.fsf@gitster.c.googlers.com","threadId":"54345","inReplyTo":"20201027234309.GA1298045@google.com","subject":"Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-28T19:18:12Z","receivedAt":"2020-10-28T21:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Yes. And to reiterate the point a little: the reason nobody sets\n> transfer.fsckObjects is that we haven't made it easy to distinguish\n> between \"hard error, should never be overridden\" checks (like\n> BAD_PARENT_SHA1), \"new tools shouldn't write these but they exist in\n> important repos like perl.git and anything consuming Git repositories\n> needs to cope with them\" (like MISSING_SPACE_BEFORE_DATE from some\n> commits' concatenated authors), and so on.\n\nHmph, don't we \"distinguish\" them by setting appropriate default\nlevels, though? Perhaps some classes of errors are set too strict?\n\n>> So I won't be too devastated to remove the symlink checks, or possibly\n>> downgrade them to purely warnings (or \"info\"; the naming in fsck.c is\n>> confusing, because the transfer operations take even warnings as fatal.\n>> I suspect we could do with some cleanup there).\n>\n> Downgrading the .gitignore check to warning sounds okay. .gitmodules\n> would still want to be an error, of course.\n\n.gitattributes (and any other .git we may have in the\nfuture), too.\n\nThanks.\n\n"},{"id":"408552","messageId":"xmqqimau9n93.fsf@gitster.c.googlers.com","threadId":"54345","inReplyTo":"21e4bac1-b3ef-e28f-586d-fe17baf6b15e@iee.email","subject":"Re: [PATCH] documentation symlink restrictions for .git* files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-28T19:14:16Z","receivedAt":"2020-10-28T21:34:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\n> On 27/10/2020 18:45, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>>> DESCRIPTION\n>>>> -----------\n>>>> \n>>>> -A `gitattributes` file is a simple text file that gives\n>>>> -`attributes` to pathnames.\n>>>> +A `gitattributes` file is a simple text file (it cannot be a\n>>>> +symbolic link to anything) that gives `attributes` to pathnames.\n>>> I worried that even a short mention like this would be distracting. Not\n>>> because it's so long, but because it's right there in the very first\n>>> sentence, and I really think this is a corner case that most people\n>>> would not even think about.\n>>>\n>>> So it is helpful if you are looking for info on symlinks and these\n>>> files, but probably clutter if you are looking for something else.\n>>>\n>>> I have to admit I don't feel all that strongly either way, though.\n>> I don't, either, and as I said, I found that the placement of new\n>> text was OK.\n>>\n>>\n> I do think that the extra text above is the right thing to do.\n>\n> We should be informing readers early about things that are expressly\n> prohibited.\n\nIt needs to be balanced, though. If it is so common a temptation\namong readers to make a symbolic link out of the tracked contents,\nperhaps it is worth telling them that they shouldn't do so, but I\njust do not get the feeling that it is the case.\n\n"},{"id":"408579","messageId":"20201028094105.GA175280@coredump.intra.peff.net","threadId":"54345","inReplyTo":"xmqqv9ev9vnf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-28T09:41:05Z","receivedAt":"2020-10-28T23:08:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 27, 2020 at 03:00:36PM -0700, Junio C Hamano wrote:\n\n> > +\t/*\n> > +\t * Now we have a path in \"target\" that only go down into the tree.\n> > +\t * Disallow any interior \"../\", like \"foo/../bar\". These might be\n> > +\t * OK, but we cannot know unless we know whether \"foo\" is itself a\n> > +\t * symlink. So err on the side of caution.\n> > +\t */\n> > +\twhile (*target) {\n> > +\t\tconst char *v;\n> > +\t\tif (skip_prefix(target, \"..\", &v) && (!*v || is_dir_sep(*v)))\n> > +\t\t\treturn 1;\n> > +\t\ttarget++;\n> > +\t}\n\nJust reading over the patch again, I suspect this is overly eager to\nfind \"..\" that is not bordered by a directory separator on the left-hand\nside (e.g., I think it will match \"foo../\").\n\n> > +int safe_symlink(const char *target, const char *linkpath)\n> > +{\n> > +\tif (!allow_external_symlinks &&\n> > +\t symlink_leaves_repo(target, linkpath)) {\n> > +\t\terrno = EPERM;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\treturn symlink(target, linkpath);\n> > +}\n> \n> OK. This is only about blocking creation of new symbolic links that\n> goes outside the working tree. It obviously is a good thing to do.\n> \n> We have some \"symlink safety\" in various parts of the system [*1*],\n> and I wonder if we can somehow consolidate the support to a more\n> central place.\n\nNote that it is overly conservative, as described in the comment quoted\nat the top of this email. It's possible that other code might be able to\nbe more accurate because it can see \"/foo/\" in the middle of a symlink\ntarget and actually _look_ at \"foo\". Does it exist, is it a symlink\nitself, etc.\n\nWhereas here we're taking a purely textual look at the target. We have\nto, I think, because we don't know if or when that \"foo\" will get\nupdated. And maybe that same restriction applies to other parts of the\nsystem, or maybe not.\n\nAt any rate, I don't really have plans to push this particular topic\nforward, at least not in the near term. I was mostly sharing it in case\nanybody found it useful. If somebody wants to run with it, please be my\nguest.\n\n-Peff\n"}]}