{"thread":{"id":"62553","subject":"[PATCH] fast-import: disallow more path components","startedAt":"2024-11-27T20:47:47Z","lastAt":"2024-12-04T00:15:31Z","messageCount":11,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508244","messageId":"pull.1832.git.1732740464398.gitgitgadget@gmail.com","threadId":"62553","inReplyTo":null,"subject":"[PATCH] fast-import: disallow more path components","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-27T20:47:44Z","receivedAt":"2024-11-27T20:47:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nInstead of just disallowing '.' and '..', make use of verify_path() to\nensure that fast-import will disallow anything we wouldn't allow into\nthe index, such as anything under .git/, .gitmodules as a symlink, or\na dos drive prefix on Windows.\n\nSince a few fast-export and fast-import tests that tried to stress-test\nthe correct handling of quoting relied on filenames that fail\nis_valid_win32_path(), such as spaces or periods at the end of filenames\nor backslashes within the filename, turn off core.protectNTFS for those\ntests to ensure they keep passing.\n\nSigned-off-by: Elijah Newren \n---\n Disallow verify_path() failures from fast-import\n \n Since en/fast-import-path-sanitize has already made it to next, this\n commit is based on that. (See\n https://lore.kernel.org/git/pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com/\n for discussion of that series.)\n \n Changes relative to that commit: this fixes up the error message as\n suggested by Kristoffer, and makes the checks more encompassing as\n suggested by Patrick and Peff -- in particular, using verify_path() as\n suggested by Peff.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1832%2Fnewren%2Fdisallow-verify-path-fast-import-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1832/newren/disallow-verify-path-fast-import-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1832\n\n builtin/fast-import.c | 5 ++-\n t/t9300-fast-import.sh | 88 ++++++++++++++++++++++++++++++++++++++++--\n t/t9350-fast-export.sh | 2 +-\n 3 files changed, 88 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 3e7ec1f1198..bb4b769c7c3 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -13,6 +13,7 @@\n #include \"delta.h\"\n #include \"pack.h\"\n #include \"path.h\"\n+#include \"read-cache-ll.h\"\n #include \"refs.h\"\n #include \"csum-file.h\"\n #include \"quote.h\"\n@@ -1413,6 +1414,8 @@ static int tree_content_set(\n \t\tdie(\"Empty path component found in input\");\n \tif (!*slash1 && !S_ISDIR(mode) && subtree)\n \t\tdie(\"Non-directories cannot have subtrees\");\n+\tif (!verify_path(p, mode))\n+\t\tdie(\"invalid path '%s'\", p);\n \n \tif (!root->tree)\n \t\tload_tree(root);\n@@ -1468,8 +1471,6 @@ static int tree_content_set(\n \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n \te = new_tree_entry();\n \te->name = to_atom(p, n);\n-\tif (is_dot_or_dotdot(e->name->str_dat))\n-\t\tdie(\"path %s contains invalid component\", p);\n \te->versions[0].mode = 0;\n \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n \tt->entries[t->entry_count++] = e;\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 5a5127fffa7..e2b1db6bc2f 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -522,7 +522,7 @@ test_expect_success 'B: fail on invalid committer (5)' '\n \ttest_must_fail git fast-import input <<-INPUT_END &&\n \tblob\n \tmark :1\n@@ -542,6 +542,86 @@ test_expect_success 'B: fail on invalid file path' '\n \ttest_must_fail git fast-import input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <output &&\n \tcut -d\" \" -f1,2,5 output >actual &&\n \ttest_cmp expect actual\n@@ -3117,7 +3197,7 @@ test_path_eol_success () {\n \ttest_expect_success \"S: paths at EOL with $test must work\" '\n \t\ttest_when_finished \"git branch -D S-path-eol\" &&\n \n-\t\tgit fast-import --export-marks=marks.out <<-EOF >out 2>err &&\n+\t\tgit -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF >out 2>err &&\n \t\tblob\n \t\tmark :401\n \t\tdata <err &&\n+\t\tgit -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF 2>err &&\n \t\tblob\n \t\tmark :401\n \t\tdata <expect &&\n \t git init result &&\n \t cd result &&\n-\t git fast-import <../export.out &&\n+\t git -c core.protectNTFS=false fast-import <../export.out &&\n \t git rev-list HEAD >actual &&\n \t test_cmp ../expect actual\n \t)\n\nbase-commit: 4a2790a257b314ab59f6f2e25f3d7ca120219922\n-- \ngitgitgadget\n"},{"id":"508255","messageId":"xmqqldx4qkaf.fsf@gitster.g","threadId":"62553","inReplyTo":"pull.1832.git.1732740464398.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-import: disallow more path components","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-28T00:22:32Z","receivedAt":"2024-11-28T00:22:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Instead of just disallowing '.' and '..', make use of verify_path() to\n> ensure that fast-import will disallow anything we wouldn't allow into\n> the index, such as anything under .git/, .gitmodules as a symlink, or\n> a dos drive prefix on Windows.\n>\n> Since a few fast-export and fast-import tests that tried to stress-test\n> the correct handling of quoting relied on filenames that fail\n> is_valid_win32_path(), such as spaces or periods at the end of filenames\n> or backslashes within the filename, turn off core.protectNTFS for those\n> tests to ensure they keep passing.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> Disallow verify_path() failures from fast-import\n> \n> Since en/fast-import-path-sanitize has already made it to next, this\n> commit is based on that. (See\n> https://lore.kernel.org/git/pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com/\n> for discussion of that series.)\n> \n> Changes relative to that commit: this fixes up the error message as\n> suggested by Kristoffer, and makes the checks more encompassing as\n> suggested by Patrick and Peff -- in particular, using verify_path() as\n> suggested by Peff.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1832%2Fnewren%2Fdisallow-verify-path-fast-import-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1832/newren/disallow-verify-path-fast-import-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1832\n\nThanks, all. Looking good to me.\n\nWill queue.\n"},{"id":"508299","messageId":"20241128161232.GA11476@coredump.intra.peff.net","threadId":"62553","inReplyTo":"pull.1832.git.1732740464398.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-import: disallow more path components","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-28T16:12:32Z","receivedAt":"2024-11-28T16:12:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 27, 2024 at 08:47:44PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Instead of just disallowing '.' and '..', make use of verify_path() to\n> ensure that fast-import will disallow anything we wouldn't allow into\n> the index, such as anything under .git/, .gitmodules as a symlink, or\n> a dos drive prefix on Windows.\n> \n> Since a few fast-export and fast-import tests that tried to stress-test\n> the correct handling of quoting relied on filenames that fail\n> is_valid_win32_path(), such as spaces or periods at the end of filenames\n> or backslashes within the filename, turn off core.protectNTFS for those\n> tests to ensure they keep passing.\n\nGreat, thanks for following up on this. I think it will work as\nadvertised, though...\n\n> @@ -1413,6 +1414,8 @@ static int tree_content_set(\n> \t\tdie(\"Empty path component found in input\");\n> \tif (!*slash1 && !S_ISDIR(mode) && subtree)\n> \t\tdie(\"Non-directories cannot have subtrees\");\n> +\tif (!verify_path(p, mode))\n> +\t\tdie(\"invalid path '%s'\", p);\n\nThis spot is over-verifying, I think, because it's recursive. Given the\npath foo/bar/baz, it will see \"foo/bar/baz\", then \"bar/baz\", then \"baz\".\nBut just the first one should be sufficient to feed to verify_path().\n\nI'd have expected the check when we parse the path in file_change_m().\nThat would also require touching other callers, too. One option would be\nto put a non-recursive wrapper around tree_content_set(), and add the\ncheck there.\n\nBut looking at those other callers, I think it's really just\nfile_change_cr() that maters. The other call in do_change_note_fanout()\nis using a notes path that we've constructed ourselves (so it's not\nwrong to check, but probably pointless).\n\nThe patch below passes your tests (though perhaps it would be worth\nadding an explicit check of a copy/rename). Is it worth worrying about\nthe efficiency of the extra calls? I'm not sure, and I didn't measure.\nBut this just seems less surprising to me overall.\n\n(Both your patch and what I've shown below do not verify deletions from\nthe tree, but I think that's fine; such a name would not exist in the\ntree in the first place).\n\n-Peff\n\n---\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex bb4b769c7c..265d1b7d52 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1414,8 +1414,6 @@ static int tree_content_set(\n \t\tdie(\"Empty path component found in input\");\n \tif (!*slash1 && !S_ISDIR(mode) && subtree)\n \t\tdie(\"Non-directories cannot have subtrees\");\n-\tif (!verify_path(p, mode))\n-\t\tdie(\"invalid path '%s'\", p);\n \n \tif (!root->tree)\n \t\tload_tree(root);\n@@ -2417,6 +2415,9 @@ static void file_change_m(const char *p, struct branch *b)\n \t\ttree_content_replace(&b->branch_tree, &oid, mode, NULL);\n \t\treturn;\n \t}\n+\n+\tif (!verify_path(path.buf, mode))\n+\t\tdie(\"invalid path '%s'\", path.buf);\n \ttree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL);\n }\n \n@@ -2454,6 +2455,8 @@ static void file_change_cr(const char *p, struct branch *b, int rename)\n \t\t\tleaf.tree);\n \t\treturn;\n \t}\n+\tif (!verify_path(dest.buf, leaf.versions[1].mode))\n+\t\tdie(\"invalid path '%s'\", dest.buf);\n \ttree_content_set(&b->branch_tree, dest.buf,\n \t\t&leaf.versions[1].oid,\n \t\tleaf.versions[1].mode,\n"},{"id":"508368","messageId":"pull.1832.v2.git.1732928970059.gitgitgadget@gmail.com","threadId":"62553","inReplyTo":"pull.1832.git.1732740464398.gitgitgadget@gmail.com","subject":"[PATCH v2] fast-import: disallow more path components","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-30T01:09:29Z","receivedAt":"2024-11-30T01:09:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nInstead of just disallowing '.' and '..', make use of verify_path() to\nensure that fast-import will disallow anything we wouldn't allow into\nthe index, such as anything under .git/, .gitmodules as a symlink, or\na dos drive prefix on Windows.\n\nSince a few fast-export and fast-import tests that tried to stress-test\nthe correct handling of quoting relied on filenames that fail\nis_valid_win32_path(), such as spaces or periods at the end of filenames\nor backslashes within the filename, turn off core.protectNTFS for those\ntests to ensure they keep passing.\n\nHelped-by: Jeff King \nSigned-off-by: Elijah Newren \n---\n Disallow verify_path() failures from fast-import\n \n Since en/fast-import-path-sanitize has already made it to next, this\n commit is based on that. (See\n https://lore.kernel.org/git/pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com/\n for discussion of that series.)\n \n Changes relative to that commit: this fixes up the error message as\n suggested by Kristoffer, and makes the checks more encompassing as\n suggested by Patrick and Peff -- in particular, using verify_path() as\n suggested by Peff.\n \n Changes since v1:\n \n * Moved the check to a higher level, as suggested by Peff.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1832%2Fnewren%2Fdisallow-verify-path-fast-import-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1832/newren/disallow-verify-path-fast-import-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1832\n\nRange-diff vs v1:\n\n 1: 2c23d601112 ! 1: 787e9d71ae7 fast-import: disallow more path components\n @@ Commit message\n or backslashes within the filename, turn off core.protectNTFS for those\n tests to ensure they keep passing.\n \n + Helped-by: Jeff King \n Signed-off-by: Elijah Newren \n \n ## builtin/fast-import.c ##\n @@ builtin/fast-import.c\n #include \"refs.h\"\n #include \"csum-file.h\"\n #include \"quote.h\"\n -@@ builtin/fast-import.c: static int tree_content_set(\n - \t\tdie(\"Empty path component found in input\");\n - \tif (!*slash1 && !S_ISDIR(mode) && subtree)\n - \t\tdie(\"Non-directories cannot have subtrees\");\n -+\tif (!verify_path(p, mode))\n -+\t\tdie(\"invalid path '%s'\", p);\n - \n - \tif (!root->tree)\n - \t\tload_tree(root);\n @@ builtin/fast-import.c: static int tree_content_set(\n \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n \te = new_tree_entry();\n @@ builtin/fast-import.c: static int tree_content_set(\n \te->versions[0].mode = 0;\n \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n \tt->entries[t->entry_count++] = e;\n +@@ builtin/fast-import.c: static void file_change_m(const char *p, struct branch *b)\n + \t\ttree_content_replace(&b->branch_tree, &oid, mode, NULL);\n + \t\treturn;\n + \t}\n ++\n ++\tif (!verify_path(path.buf, mode))\n ++\t\tdie(\"invalid path '%s'\", path.buf);\n + \ttree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL);\n + }\n + \n +@@ builtin/fast-import.c: static void file_change_cr(const char *p, struct branch *b, int rename)\n + \t\t\tleaf.tree);\n + \t\treturn;\n + \t}\n ++\tif (!verify_path(dest.buf, leaf.versions[1].mode))\n ++\t\tdie(\"invalid path '%s'\", dest.buf);\n + \ttree_content_set(&b->branch_tree, dest.buf,\n + \t\t&leaf.versions[1].oid,\n + \t\tleaf.versions[1].mode,\n \n ## t/t9300-fast-import.sh ##\n @@ t/t9300-fast-import.sh: test_expect_success 'B: fail on invalid committer (5)' '\n\n\n builtin/fast-import.c | 8 +++-\n t/t9300-fast-import.sh | 88 ++++++++++++++++++++++++++++++++++++++++--\n t/t9350-fast-export.sh | 2 +-\n 3 files changed, 91 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 3e7ec1f1198..265d1b7d52b 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -13,6 +13,7 @@\n #include \"delta.h\"\n #include \"pack.h\"\n #include \"path.h\"\n+#include \"read-cache-ll.h\"\n #include \"refs.h\"\n #include \"csum-file.h\"\n #include \"quote.h\"\n@@ -1468,8 +1469,6 @@ static int tree_content_set(\n \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n \te = new_tree_entry();\n \te->name = to_atom(p, n);\n-\tif (is_dot_or_dotdot(e->name->str_dat))\n-\t\tdie(\"path %s contains invalid component\", p);\n \te->versions[0].mode = 0;\n \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n \tt->entries[t->entry_count++] = e;\n@@ -2416,6 +2415,9 @@ static void file_change_m(const char *p, struct branch *b)\n \t\ttree_content_replace(&b->branch_tree, &oid, mode, NULL);\n \t\treturn;\n \t}\n+\n+\tif (!verify_path(path.buf, mode))\n+\t\tdie(\"invalid path '%s'\", path.buf);\n \ttree_content_set(&b->branch_tree, path.buf, &oid, mode, NULL);\n }\n \n@@ -2453,6 +2455,8 @@ static void file_change_cr(const char *p, struct branch *b, int rename)\n \t\t\tleaf.tree);\n \t\treturn;\n \t}\n+\tif (!verify_path(dest.buf, leaf.versions[1].mode))\n+\t\tdie(\"invalid path '%s'\", dest.buf);\n \ttree_content_set(&b->branch_tree, dest.buf,\n \t\t&leaf.versions[1].oid,\n \t\tleaf.versions[1].mode,\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 5a5127fffa7..e2b1db6bc2f 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -522,7 +522,7 @@ test_expect_success 'B: fail on invalid committer (5)' '\n \ttest_must_fail git fast-import input <<-INPUT_END &&\n \tblob\n \tmark :1\n@@ -542,6 +542,86 @@ test_expect_success 'B: fail on invalid file path' '\n \ttest_must_fail git fast-import input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <output &&\n \tcut -d\" \" -f1,2,5 output >actual &&\n \ttest_cmp expect actual\n@@ -3117,7 +3197,7 @@ test_path_eol_success () {\n \ttest_expect_success \"S: paths at EOL with $test must work\" '\n \t\ttest_when_finished \"git branch -D S-path-eol\" &&\n \n-\t\tgit fast-import --export-marks=marks.out <<-EOF >out 2>err &&\n+\t\tgit -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF >out 2>err &&\n \t\tblob\n \t\tmark :401\n \t\tdata <err &&\n+\t\tgit -c core.protectNTFS=false fast-import --export-marks=marks.out <<-EOF 2>err &&\n \t\tblob\n \t\tmark :401\n \t\tdata <expect &&\n \t git init result &&\n \t cd result &&\n-\t git fast-import <../export.out &&\n+\t git -c core.protectNTFS=false fast-import <../export.out &&\n \t git rev-list HEAD >actual &&\n \t test_cmp ../expect actual\n \t)\n\nbase-commit: 4a2790a257b314ab59f6f2e25f3d7ca120219922\n-- \ngitgitgadget\n"},{"id":"508392","messageId":"20241201214014.GC145938@coredump.intra.peff.net","threadId":"62553","inReplyTo":"pull.1832.v2.git.1732928970059.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: disallow more path components","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-01T21:40:14Z","receivedAt":"2024-12-01T21:40:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 30, 2024 at 01:09:29AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Changes since v1:\n> \n> * Moved the check to a higher level, as suggested by Peff.\n\nThanks, the code change looks good. Is it worth tweaking one of the\ntests to do \"R innocent-path .git/evil\"? Otherwise I don't think there's\nany coverage of the file_change_cr() call at all.\n\n-Peff\n"},{"id":"508405","messageId":"xmqqa5delwsx.fsf@gitster.g","threadId":"62553","inReplyTo":"pull.1832.v2.git.1732928970059.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: disallow more path components","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-02T01:04:46Z","receivedAt":"2024-12-02T01:04:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Instead of just disallowing '.' and '..', make use of verify_path() to\n> ensure that fast-import will disallow anything we wouldn't allow into\n> the index, such as anything under .git/, .gitmodules as a symlink, or\n> a dos drive prefix on Windows.\n>\n> Since a few fast-export and fast-import tests that tried to stress-test\n> the correct handling of quoting relied on filenames that fail\n> is_valid_win32_path(), such as spaces or periods at the end of filenames\n> or backslashes within the filename, turn off core.protectNTFS for those\n> tests to ensure they keep passing.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Elijah Newren \n> ---\n> Disallow verify_path() failures from fast-import\n> \n> Since en/fast-import-path-sanitize has already made it to next, this\n> commit is based on that. (See\n> https://lore.kernel.org/git/pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com/\n> for discussion of that series.)\n\nAh, sorry and thanks.\n\nWill queue.\n"},{"id":"508520","messageId":"CABPp-BGP8zrSzxcacTzLY-EuYAQW5EwyDGNAMh196udGN18fmg@mail.gmail.com","threadId":"62553","inReplyTo":"20241201214014.GC145938@coredump.intra.peff.net","subject":"Re: [PATCH v2] fast-import: disallow more path components","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-12-03T08:01:51Z","receivedAt":"2024-12-03T08:02:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Dec 1, 2024 at 1:40 PM Jeff King wrote:\n>\n> On Sat, Nov 30, 2024 at 01:09:29AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > Changes since v1:\n> >\n> > * Moved the check to a higher level, as suggested by Peff.\n>\n> Thanks, the code change looks good. Is it worth tweaking one of the\n> tests to do \"R innocent-path .git/evil\"? Otherwise I don't think there's\n> any coverage of the file_change_cr() call at all.\n>\n> -Peff\n\nI would say yes, but since this patch too has made it to next and is\nmarked for master, I'm kinda tempted to just leave it as-is...\n"},{"id":"508558","messageId":"20241203210652.GA1413195@coredump.intra.peff.net","threadId":"62553","inReplyTo":"CABPp-BGP8zrSzxcacTzLY-EuYAQW5EwyDGNAMh196udGN18fmg@mail.gmail.com","subject":"[PATCH 2/1] t9300: test verification of renamed paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-03T21:06:52Z","receivedAt":"2024-12-03T21:07:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 03, 2024 at 12:01:51AM -0800, Elijah Newren wrote:\n\n> > On Sat, Nov 30, 2024 at 01:09:29AM +0000, Elijah Newren via GitGitGadget wrote:\n> >\n> > > Changes since v1:\n> > >\n> > > * Moved the check to a higher level, as suggested by Peff.\n> >\n> > Thanks, the code change looks good. Is it worth tweaking one of the\n> > tests to do \"R innocent-path .git/evil\"? Otherwise I don't think there's\n> > any coverage of the file_change_cr() call at all.\n> \n> I would say yes, but since this patch too has made it to next and is\n> marked for master, I'm kinda tempted to just leave it as-is...\n\nIs is tempting. :) I wrote this up, though, which can just go on top (of\nen/fast-import-verify-path).\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] t9300: test verification of renamed paths\n\nCommit da91a90c2f (fast-import: disallow more path components,\n2024-11-30) added two separate verify_path() calls (one for\nadded/modified files, and one for renames/copies). But our tests only\nexercise the first one. Let's protect ourselves against regressions by\ntweaking one of the tests to rename into the bad path. There are\nadjacent tests that will stay as additions, so now both calls are\ncovered.\n\nSigned-off-by: Jeff King \n---\n t/t9300-fast-import.sh | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex e2b1db6bc2..fd01a2353c 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' '\n \tcommit refs/heads/badpath\n \tcommitter Name $GIT_COMMITTER_DATE\n \tdata < $GIT_COMMITTER_DATE\n+\tdata < writes:\n\n> On Sun, Dec 1, 2024 at 1:40 PM Jeff King wrote:\n>>\n>> On Sat, Nov 30, 2024 at 01:09:29AM +0000, Elijah Newren via GitGitGadget wrote:\n>>\n>> > Changes since v1:\n>> >\n>> > * Moved the check to a higher level, as suggested by Peff.\n>>\n>> Thanks, the code change looks good. Is it worth tweaking one of the\n>> tests to do \"R innocent-path .git/evil\"? Otherwise I don't think there's\n>> any coverage of the file_change_cr() call at all.\n>>\n>> -Peff\n>\n> I would say yes, but since this patch too has made it to next and is\n> marked for master, I'm kinda tempted to just leave it as-is...\n\nIt is perfectly OK to have a follow-up patch that adds a test or two\n;-)\n"},{"id":"508577","messageId":"CABPp-BGFxss5m0OU-mj8XSxqMrrxu_NRb9f5qdb-88bjj3+Y9A@mail.gmail.com","threadId":"62553","inReplyTo":"20241203210652.GA1413195@coredump.intra.peff.net","subject":"Re: [PATCH 2/1] t9300: test verification of renamed paths","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-12-03T22:22:53Z","receivedAt":"2024-12-03T22:23:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Dec 3, 2024 at 1:07 PM Jeff King wrote:\n>\n> On Tue, Dec 03, 2024 at 12:01:51AM -0800, Elijah Newren wrote:\n>\n> > > On Sat, Nov 30, 2024 at 01:09:29AM +0000, Elijah Newren via GitGitGadget wrote:\n> > >\n> > > > Changes since v1:\n> > > >\n> > > > * Moved the check to a higher level, as suggested by Peff.\n> > >\n> > > Thanks, the code change looks good. Is it worth tweaking one of the\n> > > tests to do \"R innocent-path .git/evil\"? Otherwise I don't think there's\n> > > any coverage of the file_change_cr() call at all.\n> >\n> > I would say yes, but since this patch too has made it to next and is\n> > marked for master, I'm kinda tempted to just leave it as-is...\n>\n> Is is tempting. :) I wrote this up, though, which can just go on top (of\n> en/fast-import-verify-path).\n\nThanks!\n\n> -Peff\n>\n> -- >8 --\n> Subject: [PATCH] t9300: test verification of renamed paths\n>\n> Commit da91a90c2f (fast-import: disallow more path components,\n> 2024-11-30) added two separate verify_path() calls (one for\n> added/modified files, and one for renames/copies). But our tests only\n> exercise the first one. Let's protect ourselves against regressions by\n> tweaking one of the tests to rename into the bad path. There are\n> adjacent tests that will stay as additions, so now both calls are\n> covered.\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t9300-fast-import.sh | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index e2b1db6bc2..fd01a2353c 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' '\n> commit refs/heads/badpath\n> committer Name $GIT_COMMITTER_DATE\n> data < - Commit Message\n> + Good path\n> + COMMIT\n> + M 100644 :1 ok-path\n> +\n> + commit refs/heads/badpath\n> + committer Name $GIT_COMMITTER_DATE\n> + data < + Bad path\n> COMMIT\n> - M 100644 :1 ./invalid-path\n> + R ok-path ./invalid-path\n> INPUT_END\n>\n> test_when_finished \"git update-ref -d refs/heads/badpath\" &&\n> --\n> 2.47.1.707.g92f6f18526\n\nChange looks good to me.\n"},{"id":"508588","messageId":"xmqqed2o717j.fsf@gitster.g","threadId":"62553","inReplyTo":"20241203210652.GA1413195@coredump.intra.peff.net","subject":"Re: [PATCH 2/1] t9300: test verification of renamed paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T00:15:28Z","receivedAt":"2024-12-04T00:15:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I would say yes, but since this patch too has made it to next and is\n>> marked for master, I'm kinda tempted to just leave it as-is...\n>\n> Is is tempting. :) I wrote this up, though, which can just go on top (of\n> en/fast-import-verify-path).\n\nThanks, queued.\n\n> -- >8 --\n> Subject: [PATCH] t9300: test verification of renamed paths\n>\n> Commit da91a90c2f (fast-import: disallow more path components,\n> 2024-11-30) added two separate verify_path() calls (one for\n> added/modified files, and one for renames/copies). But our tests only\n> exercise the first one. Let's protect ourselves against regressions by\n> tweaking one of the tests to rename into the bad path. There are\n> adjacent tests that will stay as additions, so now both calls are\n> covered.\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t9300-fast-import.sh | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index e2b1db6bc2..fd01a2353c 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -553,9 +553,16 @@ test_expect_success 'B: fail on invalid file path of .' '\n> \tcommit refs/heads/badpath\n> \tcommitter Name $GIT_COMMITTER_DATE\n> \tdata < -\tCommit Message\n> +\tGood path\n> +\tCOMMIT\n> +\tM 100644 :1 ok-path\n> +\n> +\tcommit refs/heads/badpath\n> +\tcommitter Name $GIT_COMMITTER_DATE\n> +\tdata < +\tBad path\n> \tCOMMIT\n> -\tM 100644 :1 ./invalid-path\n> +\tR ok-path ./invalid-path\n> \tINPUT_END\n> \n> \ttest_when_finished \"git update-ref -d refs/heads/badpath\" &&\n"}]}