{"thread":{"id":"60708","subject":"[PATCH 0/3] Strengthen fsck checks for submodule URLs","startedAt":"2024-01-09T17:53:40Z","lastAt":"2024-11-14T19:11:34Z","messageCount":31,"participants":["Victoria Dye via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Jeff King","Victoria Dye","Neil Mayhew"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"486451","messageId":"pull.1635.git.1704822817.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":null,"subject":"[PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-09T17:53:34Z","receivedAt":"2024-01-09T17:53:40Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"While testing 'git fsck' checks on .gitmodules URLs, I noticed that some\ninvalid URLs were passing the checks. Digging into it a bit more, the issue\nturned out to be that 'credential_from_url_gently()' parses certain URLs\n(like \"http://example.com:something/deeper/path\") incorrectly, in a way that\nappeared to return a valid result.\n\nFortunately, these URLs are rejected in fetches/clones/pushes anyway because\n'url_normalize()' (called in 'validate_remote_url()') correctly identifies\nthem as invalid. So, to bring 'git fsck' in line with other (stronger)\nvalidation done on remote URLs, this series replaces the\n'credential_from_url_gently()' check with one that uses 'url_normalize()'.\n\n * Patch 1 moves 'check_submodule_url()' to a public location so that it can\n be used outside of 'fsck.c'.\n * Patch 2 adds a 'check-url' mode to 'test-tool submodule', calling the\n now-public 'check_submodule_url()' method on a given URL, and adds a new\n test checking a list of valid and invalid submodule URLs.\n * Patch 3 replaces the 'credential_from_url_gently()' check with\n 'url_normalize()' followed by 'url_decode()' and an explicit check for\n newlines (to preserve the newline handling added in 07259e74ec1 (fsck:\n detect gitmodules URLs with embedded newlines, 2020-03-11)).\n\nThanks!\n\n * Victoria\n\nVictoria Dye (3):\n submodule-config.h: move check_submodule_url\n t7450: test submodule urls\n submodule-config.c: strengthen URL fsck check\n\n fsck.c | 133 ----------------------------------\n submodule-config.c | 140 ++++++++++++++++++++++++++++++++++++\n submodule-config.h | 3 +\n t/helper/test-submodule.c | 31 ++++++--\n t/t7450-bad-git-dotfiles.sh | 26 +++++++\n 5 files changed, 196 insertions(+), 137 deletions(-)\n\n\nbase-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1635%2Fvdye%2Fvdye%2Fstrengthen-fsck-url-checks-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1635/vdye/vdye/strengthen-fsck-url-checks-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1635\n-- \ngitgitgadget\n"},{"id":"486452","messageId":"588de3022d7703cfacfca3362655531a56ea161e.1704822817.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.git.1704822817.gitgitgadget@gmail.com","subject":"[PATCH 1/3] submodule-config.h: move check_submodule_url","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-09T17:53:35Z","receivedAt":"2024-01-09T17:53:41Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nMove 'check_submodule_url' out of 'fsck.c' and into 'submodule-config.h' as\na public method, similar to 'check_submodule_name'. With the function now\naccessible outside of 'fsck', it can be used in a later commit to extend\n'test-tool submodule' to check the validity of submodule URLs as it does\nwith names in the 'check-name' subcommand.\n\nOther than its location, no changes are made to 'check_submodule_url' in\nthis patch.\n\nSigned-off-by: Victoria Dye \n---\n fsck.c | 133 --------------------------------------------\n submodule-config.c | 134 +++++++++++++++++++++++++++++++++++++++++++++\n submodule-config.h | 3 +\n 3 files changed, 137 insertions(+), 133 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 1ad02fcdfab..8ded0a473a4 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -20,7 +20,6 @@\n #include \"packfile.h\"\n #include \"submodule-config.h\"\n #include \"config.h\"\n-#include \"credential.h\"\n #include \"help.h\"\n \n static ssize_t max_tree_entry_len = 4096;\n@@ -1047,138 +1046,6 @@ done:\n \treturn ret;\n }\n \n-static int starts_with_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n-static int starts_with_dot_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n-static int submodule_url_is_relative(const char *url)\n-{\n-\treturn starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);\n-}\n-\n-/*\n- * Count directory components that a relative submodule URL should chop\n- * from the remote_url it is to be resolved against.\n- *\n- * In other words, this counts \"../\" components at the start of a\n- * submodule URL.\n- *\n- * Returns the number of directory components to chop and writes a\n- * pointer to the next character of url after all leading \"./\" and\n- * \"../\" components to out.\n- */\n-static int count_leading_dotdots(const char *url, const char **out)\n-{\n-\tint result = 0;\n-\twhile (1) {\n-\t\tif (starts_with_dot_dot_slash(url)) {\n-\t\t\tresult++;\n-\t\t\turl += strlen(\"../\");\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with_dot_slash(url)) {\n-\t\t\turl += strlen(\"./\");\n-\t\t\tcontinue;\n-\t\t}\n-\t\t*out = url;\n-\t\treturn result;\n-\t}\n-}\n-/*\n- * Check whether a transport is implemented by git-remote-curl.\n- *\n- * If it is, returns 1 and writes the URL that would be passed to\n- * git-remote-curl to the \"out\" parameter.\n- *\n- * Otherwise, returns 0 and leaves \"out\" untouched.\n- *\n- * Examples:\n- * http::https://example.com/repo.git -> 1, https://example.com/repo.git\n- * https://example.com/repo.git -> 1, https://example.com/repo.git\n- * git://example.com/repo.git -> 0\n- *\n- * This is for use in checking for previously exploitable bugs that\n- * required a submodule URL to be passed to git-remote-curl.\n- */\n-static int url_to_curl_url(const char *url, const char **out)\n-{\n-\t/*\n-\t * We don't need to check for case-aliases, \"http.exe\", and so\n-\t * on because in the default configuration, is_transport_allowed\n-\t * prevents URLs with those schemes from being cloned\n-\t * automatically.\n-\t */\n-\tif (skip_prefix(url, \"http::\", out) ||\n-\t skip_prefix(url, \"https::\", out) ||\n-\t skip_prefix(url, \"ftp::\", out) ||\n-\t skip_prefix(url, \"ftps::\", out))\n-\t\treturn 1;\n-\tif (starts_with(url, \"http://\") ||\n-\t starts_with(url, \"https://\") ||\n-\t starts_with(url, \"ftp://\") ||\n-\t starts_with(url, \"ftps://\")) {\n-\t\t*out = url;\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n-static int check_submodule_url(const char *url)\n-{\n-\tconst char *curl_url;\n-\n-\tif (looks_like_command_line_option(url))\n-\t\treturn -1;\n-\n-\tif (submodule_url_is_relative(url) || starts_with(url, \"git://\")) {\n-\t\tchar *decoded;\n-\t\tconst char *next;\n-\t\tint has_nl;\n-\n-\t\t/*\n-\t\t * This could be appended to an http URL and url-decoded;\n-\t\t * check for malicious characters.\n-\t\t */\n-\t\tdecoded = url_decode(url);\n-\t\thas_nl = !!strchr(decoded, '\\n');\n-\n-\t\tfree(decoded);\n-\t\tif (has_nl)\n-\t\t\treturn -1;\n-\n-\t\t/*\n-\t\t * URLs which escape their root via \"../\" can overwrite\n-\t\t * the host field and previous components, resolving to\n-\t\t * URLs like https::example.com/submodule.git and\n-\t\t * https:///example.com/submodule.git that were\n-\t\t * susceptible to CVE-2020-11008.\n-\t\t */\n-\t\tif (count_leading_dotdots(url, &next) > 0 &&\n-\t\t (*next == ':' || *next == '/'))\n-\t\t\treturn -1;\n-\t}\n-\n-\telse if (url_to_curl_url(url, &curl_url)) {\n-\t\tstruct credential c = CREDENTIAL_INIT;\n-\t\tint ret = 0;\n-\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n-\t\t !*c.host)\n-\t\t\tret = -1;\n-\t\tcredential_clear(&c);\n-\t\treturn ret;\n-\t}\n-\n-\treturn 0;\n-}\n-\n struct fsck_gitmodules_data {\n \tconst struct object_id *oid;\n \tstruct fsck_options *options;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex f4dd482abc9..3b295e9f89c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -14,6 +14,8 @@\n #include \"parse-options.h\"\n #include \"thread-utils.h\"\n #include \"tree-walk.h\"\n+#include \"url.h\"\n+#include \"credential.h\"\n \n /*\n * submodule cache lookup structure\n@@ -228,6 +230,138 @@ in_component:\n \treturn 0;\n }\n \n+static int starts_with_dot_slash(const char *const path)\n+{\n+\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |\n+\t\t\t\tPATH_MATCH_XPLATFORM);\n+}\n+\n+static int starts_with_dot_dot_slash(const char *const path)\n+{\n+\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |\n+\t\t\t\tPATH_MATCH_XPLATFORM);\n+}\n+\n+static int submodule_url_is_relative(const char *url)\n+{\n+\treturn starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);\n+}\n+\n+/*\n+ * Count directory components that a relative submodule URL should chop\n+ * from the remote_url it is to be resolved against.\n+ *\n+ * In other words, this counts \"../\" components at the start of a\n+ * submodule URL.\n+ *\n+ * Returns the number of directory components to chop and writes a\n+ * pointer to the next character of url after all leading \"./\" and\n+ * \"../\" components to out.\n+ */\n+static int count_leading_dotdots(const char *url, const char **out)\n+{\n+\tint result = 0;\n+\twhile (1) {\n+\t\tif (starts_with_dot_dot_slash(url)) {\n+\t\t\tresult++;\n+\t\t\turl += strlen(\"../\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (starts_with_dot_slash(url)) {\n+\t\t\turl += strlen(\"./\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\t*out = url;\n+\t\treturn result;\n+\t}\n+}\n+/*\n+ * Check whether a transport is implemented by git-remote-curl.\n+ *\n+ * If it is, returns 1 and writes the URL that would be passed to\n+ * git-remote-curl to the \"out\" parameter.\n+ *\n+ * Otherwise, returns 0 and leaves \"out\" untouched.\n+ *\n+ * Examples:\n+ * http::https://example.com/repo.git -> 1, https://example.com/repo.git\n+ * https://example.com/repo.git -> 1, https://example.com/repo.git\n+ * git://example.com/repo.git -> 0\n+ *\n+ * This is for use in checking for previously exploitable bugs that\n+ * required a submodule URL to be passed to git-remote-curl.\n+ */\n+static int url_to_curl_url(const char *url, const char **out)\n+{\n+\t/*\n+\t * We don't need to check for case-aliases, \"http.exe\", and so\n+\t * on because in the default configuration, is_transport_allowed\n+\t * prevents URLs with those schemes from being cloned\n+\t * automatically.\n+\t */\n+\tif (skip_prefix(url, \"http::\", out) ||\n+\t skip_prefix(url, \"https::\", out) ||\n+\t skip_prefix(url, \"ftp::\", out) ||\n+\t skip_prefix(url, \"ftps::\", out))\n+\t\treturn 1;\n+\tif (starts_with(url, \"http://\") ||\n+\t starts_with(url, \"https://\") ||\n+\t starts_with(url, \"ftp://\") ||\n+\t starts_with(url, \"ftps://\")) {\n+\t\t*out = url;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+int check_submodule_url(const char *url)\n+{\n+\tconst char *curl_url;\n+\n+\tif (looks_like_command_line_option(url))\n+\t\treturn -1;\n+\n+\tif (submodule_url_is_relative(url) || starts_with(url, \"git://\")) {\n+\t\tchar *decoded;\n+\t\tconst char *next;\n+\t\tint has_nl;\n+\n+\t\t/*\n+\t\t * This could be appended to an http URL and url-decoded;\n+\t\t * check for malicious characters.\n+\t\t */\n+\t\tdecoded = url_decode(url);\n+\t\thas_nl = !!strchr(decoded, '\\n');\n+\n+\t\tfree(decoded);\n+\t\tif (has_nl)\n+\t\t\treturn -1;\n+\n+\t\t/*\n+\t\t * URLs which escape their root via \"../\" can overwrite\n+\t\t * the host field and previous components, resolving to\n+\t\t * URLs like https::example.com/submodule.git and\n+\t\t * https:///example.com/submodule.git that were\n+\t\t * susceptible to CVE-2020-11008.\n+\t\t */\n+\t\tif (count_leading_dotdots(url, &next) > 0 &&\n+\t\t (*next == ':' || *next == '/'))\n+\t\t\treturn -1;\n+\t}\n+\n+\telse if (url_to_curl_url(url, &curl_url)) {\n+\t\tstruct credential c = CREDENTIAL_INIT;\n+\t\tint ret = 0;\n+\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n+\t\t !*c.host)\n+\t\t\tret = -1;\n+\t\tcredential_clear(&c);\n+\t\treturn ret;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int name_and_item_from_var(const char *var, struct strbuf *name,\n \t\t\t\t struct strbuf *item)\n {\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 958f320ac6c..b6133af71b0 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -89,6 +89,9 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value);\n */\n int check_submodule_name(const char *name);\n \n+/* Returns 0 if the URL valid per RFC3986 and -1 otherwise. */\n+int check_submodule_url(const char *url);\n+\n /*\n * Note: these helper functions exist solely to maintain backward\n * compatibility with 'fetch' and 'update_clone' storing configuration in\n-- \ngitgitgadget\n\n"},{"id":"486454","messageId":"cf7848edffca27931aad02c0652adf2715320d35.1704822817.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.git.1704822817.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t7450: test submodule urls","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-09T17:53:36Z","receivedAt":"2024-01-09T17:53:42Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a test to 't7450-bad-git-dotfiles.sh' to check the validity of different\nsubmodule URLs. To test this directly (without setting up test repositories\n& submodules), add a 'check-url' subcommand to 'test-tool submodule' that\ncalls 'check_submodule_url' in the same way that 'check-name' calls\n'check_submodule_name'.\n\nMark the test with 'test_expect_failure' because, as it stands,\n'check_submodule_url' marks certain invalid URLs valid. Specifically, the\ninvalid URL \"http://example.com:test/foo.git\" is incorrectly marked valid in\nthe test.\n\nSigned-off-by: Victoria Dye \n---\n t/helper/test-submodule.c | 31 +++++++++++++++++++++++++++----\n t/t7450-bad-git-dotfiles.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex 50c154d0370..da89d265f0f 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -15,6 +15,13 @@ static const char *submodule_check_name_usage[] = {\n \tNULL\n };\n \n+#define TEST_TOOL_CHECK_URL_USAGE \\\n+\t\"test-tool submodule check-url \"\n+static const char *submodule_check_url_usage[] = {\n+\tTEST_TOOL_CHECK_URL_USAGE,\n+\tNULL\n+};\n+\n #define TEST_TOOL_IS_ACTIVE_USAGE \\\n \t\"test-tool submodule is-active \"\n static const char *submodule_is_active_usage[] = {\n@@ -36,22 +43,24 @@ static const char *submodule_usage[] = {\n \tNULL\n };\n \n+typedef int (*check_fn_t)(const char *);\n+\n /*\n * Exit non-zero if any of the submodule names given on the command line is\n * invalid. If no names are given, filter stdin to print only valid names\n * (which is primarily intended for testing).\n */\n-static int check_name(int argc, const char **argv)\n+static int check_submodule(int argc, const char **argv, check_fn_t check_fn)\n {\n \tif (argc > 1) {\n \t\twhile (*++argv) {\n-\t\t\tif (check_submodule_name(*argv) < 0)\n+\t\t\tif (check_fn(*argv) < 0)\n \t\t\t\treturn 1;\n \t\t}\n \t} else {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\t\tif (!check_submodule_name(buf.buf))\n+\t\t\tif (!check_fn(buf.buf))\n \t\t\t\tprintf(\"%s\\n\", buf.buf);\n \t\t}\n \t\tstrbuf_release(&buf);\n@@ -69,7 +78,20 @@ static int cmd__submodule_check_name(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(submodule_check_name_usage, options);\n \n-\treturn check_name(argc, argv);\n+\treturn check_submodule(argc, argv, check_submodule_name);\n+}\n+\n+static int cmd__submodule_check_url(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t submodule_check_url_usage, 0);\n+\tif (argc)\n+\t\tusage_with_options(submodule_check_url_usage, options);\n+\n+\treturn check_submodule(argc, argv, check_submodule_url);\n }\n \n static int cmd__submodule_is_active(int argc, const char **argv)\n@@ -195,6 +217,7 @@ static int cmd__submodule_config_writeable(int argc, const char **argv UNUSED)\n \n static struct test_cmd cmds[] = {\n \t{ \"check-name\", cmd__submodule_check_name },\n+\t{ \"check-url\", cmd__submodule_check_url },\n \t{ \"is-active\", cmd__submodule_is_active },\n \t{ \"resolve-relative-url\", cmd__submodule_resolve_relative_url},\n \t{ \"config-list\", cmd__submodule_config_list },\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 35a31acd4d7..0dbf13724f4 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -45,6 +45,32 @@ test_expect_success 'check names' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'check urls' '\n+\tcat >expect <<-\\EOF &&\n+\t./bar/baz/foo.git\n+\thttps://example.com/foo.git\n+\thttp://example.com:80/deeper/foo.git\n+\tEOF\n+\n+\ttest-tool submodule check-url >actual <<-\\EOF &&\n+\t./bar/baz/foo.git\n+\thttps://example.com/foo.git\n+\thttp://example.com:80/deeper/foo.git\n+\t-a./foo\n+\t../../..//test/foo.git\n+\t../../../../../:localhost:8080/foo.git\n+\t..\\../.\\../:example.com/foo.git\n+\t./%0ahost=example.com/foo.git\n+\thttps://one.example.com/evil?%0ahost=two.example.com\n+\thttps:///example.com/foo.git\n+\thttp://example.com:test/foo.git\n+\thttps::example.com/foo.git\n+\thttp:::example.com/foo.git\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create innocent subrepo' '\n \tgit init innocent &&\n \tgit -C innocent commit --allow-empty -m foo\n-- \ngitgitgadget\n\n"},{"id":"486453","messageId":"893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.git.1704822817.gitgitgadget@gmail.com","subject":"[PATCH 3/3] submodule-config.c: strengthen URL fsck check","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-09T17:53:37Z","receivedAt":"2024-01-09T17:53:43Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the validation of \"curl URL\" submodule URLs (i.e. those that specify\nan \"http[s]\" or \"ftp[s]\" protocol) in 'check_submodule_url()' to catch more\ninvalid URLs. The existing validation using 'credential_from_url_gently()'\nparses certain URLs incorrectly, leading to invalid submodule URLs passing\n'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote\nURLs in 'remote_get()' - correctly identifies the invalid URLs missed by\n'credential_from_url_gently()'.\n\nTo catch more invalid cases, replace 'credential_from_url_gently()' with\n'url_normalize()' followed by a 'url_decode()' and a check for newlines\n(mirroring 'check_url_component()' in the 'credential_from_url_gently()'\nvalidation).\n\nSigned-off-by: Victoria Dye \n---\n submodule-config.c | 16 +++++++++++-----\n t/t7450-bad-git-dotfiles.sh | 2 +-\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 3b295e9f89c..54130f6a385 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -15,7 +15,7 @@\n #include \"thread-utils.h\"\n #include \"tree-walk.h\"\n #include \"url.h\"\n-#include \"credential.h\"\n+#include \"urlmatch.h\"\n \n /*\n * submodule cache lookup structure\n@@ -350,12 +350,18 @@ int check_submodule_url(const char *url)\n \t}\n \n \telse if (url_to_curl_url(url, &curl_url)) {\n-\t\tstruct credential c = CREDENTIAL_INIT;\n \t\tint ret = 0;\n-\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n-\t\t !*c.host)\n+\t\tchar *normalized = url_normalize(curl_url, NULL);\n+\t\tif (normalized) {\n+\t\t\tchar *decoded = url_decode(normalized);\n+\t\t\tif (strchr(decoded, '\\n'))\n+\t\t\t\tret = -1;\n+\t\t\tfree(normalized);\n+\t\t\tfree(decoded);\n+\t\t} else {\n \t\t\tret = -1;\n-\t\tcredential_clear(&c);\n+\t\t}\n+\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 0dbf13724f4..46d4fb0354b 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -45,7 +45,7 @@ test_expect_success 'check names' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'check urls' '\n+test_expect_success 'check urls' '\n \tcat >expect <<-\\EOF &&\n \t./bar/baz/foo.git\n \thttps://example.com/foo.git\n-- \ngitgitgadget\n"},{"id":"486473","messageId":"xmqqttnmfarm.fsf@gitster.g","threadId":"60708","inReplyTo":"cf7848edffca27931aad02c0652adf2715320d35.1704822817.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t7450: test submodule urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T21:38:05Z","receivedAt":"2024-01-09T21:38:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> +#define TEST_TOOL_CHECK_URL_USAGE \\\n> +\t\"test-tool submodule check-url \"\n> +static const char *submodule_check_url_usage[] = {\n> +\tTEST_TOOL_CHECK_URL_USAGE,\n> +\tNULL\n> +};\n\nGranted, the entry that follows this new one already uses the same\npattern, but TEST_TOOL_CHECK_URL_USAGE being used only once here and\nnowhere else, with its name almost as long as the value it expands to,\nI found it unnecessarily verbose and confusing.\n\n> #define TEST_TOOL_IS_ACTIVE_USAGE \\\n> \t\"test-tool submodule is-active \"\n> static const char *submodule_is_active_usage[] = {\n\n\n> +typedef int (*check_fn_t)(const char *);\n> +\n> /*\n> * Exit non-zero if any of the submodule names given on the command line is\n> * invalid. If no names are given, filter stdin to print only valid names\n> * (which is primarily intended for testing).\n> */\n\nOK. As long as each of the input lines are unique, we can use the\nusual \"does the actual output match the expected?\" to test many of\nthem at once, and notice if there is an extra one in the output that\nshouldn't have been emitted, or there is a missing one that should\nhave.\n\n> -static int check_name(int argc, const char **argv)\n> +static int check_submodule(int argc, const char **argv, check_fn_t check_fn)\n> {\n> \tif (argc > 1) {\n> \t\twhile (*++argv) {\n> -\t\t\tif (check_submodule_name(*argv) < 0)\n> +\t\t\tif (check_fn(*argv) < 0)\n\nQuite nice way to reuse what we already have, thanks to [1/3].\n"},{"id":"486474","messageId":"xmqqplyaf9vp.fsf@gitster.g","threadId":"60708","inReplyTo":"893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T21:57:14Z","receivedAt":"2024-01-09T21:57:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Update the validation of \"curl URL\" submodule URLs (i.e. those that specify\n> an \"http[s]\" or \"ftp[s]\" protocol) in 'check_submodule_url()' to catch more\n> invalid URLs. The existing validation using 'credential_from_url_gently()'\n> parses certain URLs incorrectly, leading to invalid submodule URLs passing\n> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote\n> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by\n> 'credential_from_url_gently()'.\n>\n> To catch more invalid cases, replace 'credential_from_url_gently()' with\n> 'url_normalize()' followed by a 'url_decode()' and a check for newlines\n> (mirroring 'check_url_component()' in the 'credential_from_url_gently()'\n> validation).\n\nThanks. Left hand and right hand checking the same thing in\ndifferent ways and coming up with different result is never a happy\nsituation. Making sure we consistently use the same definition of\nwhat the valid URLs are is a very welcome thing to do, of course.\n\n> -test_expect_failure 'check urls' '\n> +test_expect_success 'check urls' '\n> \tcat >expect <<-\\EOF &&\n> \t./bar/baz/foo.git\n> \thttps://example.com/foo.git\n\nIt is a bit unfortunate that from here we cannot tell which bogus\nURLs in this test that were incorrectly accepted are now rejected.\n\nAmong the many bogus URLs in the input, we used to allow\n\n http://example.com:test/foo.git\n\n(we do not accept non-numeric representation of port numbers, so\nhttp://example.com:http/foo.git would also be rejected), but with\nthis change, it is now rejected. All the other bogus ones are\nrejected just as before this change.\n\nWill queue. Thanks.\n\n"},{"id":"486487","messageId":"ZZ46MrjSocJl-kpU@tanuki","threadId":"60708","inReplyTo":"893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T06:33:22Z","receivedAt":"2024-01-10T06:33:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 09, 2024 at 05:53:37PM +0000, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Update the validation of \"curl URL\" submodule URLs (i.e. those that specify\n> an \"http[s]\" or \"ftp[s]\" protocol) in 'check_submodule_url()' to catch more\n> invalid URLs. The existing validation using 'credential_from_url_gently()'\n> parses certain URLs incorrectly, leading to invalid submodule URLs passing\n> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote\n> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by\n> 'credential_from_url_gently()'.\n\nOkay, so we retain the wrong behavior of `credential_from_url_gently()`,\nright? I wonder whether this can be abused in any way, doubly so because\nthe function gets invoked with untrusted input from the remote server\nwhen we handle redirects in `http_request_reauth()`. But the redirect\nURL we end up passing to `credential_from_url_gently()` would have to\ncontain a non-numeric port, and curl seemingly does not know to handle\nthose either.\n\nOther callsites include fsck (which you're fixing) and the credential\nstore (which is entirely user-controlled). It would be great regardless\nto fix the underlying bug in `credential_from_url_gently()` eventually\nthough. But I do not think that this has to be part this patch series\nhere, which is a strict improvement.\n\nThanks!\n\nPatrick\n"},{"id":"486514","messageId":"20240110102338.GA16674@coredump.intra.peff.net","threadId":"60708","inReplyTo":"pull.1635.git.1704822817.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-10T10:23:38Z","receivedAt":"2024-01-10T10:23:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 09, 2024 at 05:53:34PM +0000, Victoria Dye via GitGitGadget wrote:\n\n> While testing 'git fsck' checks on .gitmodules URLs, I noticed that some\n> invalid URLs were passing the checks. Digging into it a bit more, the issue\n> turned out to be that 'credential_from_url_gently()' parses certain URLs\n> (like \"http://example.com:something/deeper/path\") incorrectly, in a way that\n> appeared to return a valid result.\n\nI don't think that checks was ever intended to be an overall URL-quality\ncheck. The reason we used the credential code in the fsck check is that\nwe were checking for URLs which triggered a specific credential-related\nvulnerability.\n\nI don't mind tightening things further as long as:\n\n 1. We are not allowing any cases that the credential code would have\n forbidden (i.e., something that might let the vulnerability slip\n through, since ultimately it is the credential code which will need\n to be protected). You ported over the newline check, which is the\n main thing. It's possible that there is some difference between the\n two parsers that may allow an invalid input to create a newline for\n one but not the other, but having now looked over the code, I don't\n think so.\n\n And I think one could argue that the security-importance of the\n fsck check has mostly run its course. The real fix was in the\n credential code itself, and the matching fsck change was mostly\n about protecting downstream clients until they were upgraded. Now\n that it's been several years, there's not as much value there.\n\n 2. It is not making it harder for users to work with repositories that\n may contain malformed URLs that _aren't_ vulnerabilities. It sounds\n like the specific cases you found already don't work at all with\n Git, so presumably nobody is using them. By making it an fsck\n check, though, any mistakes that are embedded in history (even if\n they are now corrected) will make it a pain to use the repository\n with sites that enable transfer.fsckObjects.\n\n My gut feeling is that this is probably OK in practice. If it does\n cause pain, we might consider loosening the fsck.gitmodulesUrl\n severity (under the notion from above that it is no longer a\n critical security check). But if it doesn't cause real-world pain,\n being pickier is probably better (it may save us from a\n vulnerability down the road).\n\n-Peff\n"},{"id":"486515","messageId":"20240110103812.GB16674@coredump.intra.peff.net","threadId":"60708","inReplyTo":"cf7848edffca27931aad02c0652adf2715320d35.1704822817.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t7450: test submodule urls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-10T10:38:12Z","receivedAt":"2024-01-10T10:38:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 09, 2024 at 05:53:36PM +0000, Victoria Dye via GitGitGadget wrote:\n\n> +#define TEST_TOOL_CHECK_URL_USAGE \\\n> +\t\"test-tool submodule check-url \"\n\nI don't think this command-line \"\" mode works at all. Your\nunderlying function can handle either stdin or arguments:\n\n> -static int check_name(int argc, const char **argv)\n> +static int check_submodule(int argc, const char **argv, check_fn_t check_fn)\n> {\n> \tif (argc > 1) {\n> \t\twhile (*++argv) {\n> -\t\t\tif (check_submodule_name(*argv) < 0)\n> +\t\t\tif (check_fn(*argv) < 0)\n> \t\t\t\treturn 1;\n> \t\t}\n> \t} else {\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n> -\t\t\tif (!check_submodule_name(buf.buf))\n> +\t\t\tif (!check_fn(buf.buf))\n> \t\t\t\tprintf(\"%s\\n\", buf.buf);\n> \t\t}\n> \t\tstrbuf_release(&buf);\n\n...but the new caller rejects them before we get there:\n\n> +static int cmd__submodule_check_url(int argc, const char **argv)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\targc = parse_options(argc, argv, \"test-tools\", options,\n> +\t\t\t submodule_check_url_usage, 0);\n> +\tif (argc)\n> +\t\tusage_with_options(submodule_check_url_usage, options);\n> +\n> +\treturn check_submodule(argc, argv, check_submodule_url);\n> }\n\nSo you'd want at least:\n\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex da89d265f0..6b964c88ab 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -88,8 +88,6 @@ static int cmd__submodule_check_url(int argc, const char **argv)\n \t};\n \targc = parse_options(argc, argv, \"test-tools\", options,\n \t\t\t submodule_check_url_usage, 0);\n-\tif (argc)\n-\t\tusage_with_options(submodule_check_url_usage, options);\n \n \treturn check_submodule(argc, argv, check_submodule_url);\n }\n\nbut then that reveals another mismatch. In check_submodule() above we\nexpect argv[0] to be uninteresting (i.e., the name of the program), but\nparse_options() will already have thrown it away. So we silently fail to\ncheck the first option (which is especially bad since the only output is\nthe exit code, and thus the skipped one looks the same as one that\nvalidated correctly).\n\nAll of this is inherited from the existing check_name() code, which I\nthink has all of the same bugs. The test scripts all just use the stdin\nmode, so they don't notice. It's not too hard to fix, but maybe it's\nworth just ripping out the unreachable code.\n\n-Peff\n"},{"id":"486635","messageId":"d852ad72-32c4-4b0f-8f34-e8b38b7f71ad@github.com","threadId":"60708","inReplyTo":"20240110103812.GB16674@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] t7450: test submodule urls","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2024-01-11T16:54:47Z","receivedAt":"2024-01-11T16:54:50Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Jeff King wrote:\n> On Tue, Jan 09, 2024 at 05:53:36PM +0000, Victoria Dye via GitGitGadget wrote:\n> \n>> +#define TEST_TOOL_CHECK_URL_USAGE \\\n>> +\t\"test-tool submodule check-url \"\n> \n> I don't think this command-line \"\" mode works at all. Your\n> underlying function can handle either stdin or arguments:\n\n...\n\n> All of this is inherited from the existing check_name() code, which I\n> think has all of the same bugs. The test scripts all just use the stdin\n> mode, so they don't notice. It's not too hard to fix, but maybe it's\n> worth just ripping out the unreachable code.\n\nThanks for pointing out those issues, I think removing the command line\ninput mode is the way to go. The description of the 'check_name()' mentions\nthat the stdin mode was \"primarily intended for testing\". But as 85321a346b5\n(submodule--helper: move \"check-name\" to a test-tool, 2022-09-01) pointed\nout, 'check_name()' was never used outside of tests anyway, so whatever use\ncase was imagined for the command line mode never seemed to have existed. \n\nCombine that with the fact that the command line mode is so different from\nthe stdin mode (non-zero exit code for invalid names, prints nothing vs.\nzero exit code, prints valid names), there don't seem to be any real\ndownsides to removing the unused code.\n\n> \n> -Peff\n\n"},{"id":"486638","messageId":"a9afd237-e048-43eb-922a-2734e573a644@github.com","threadId":"60708","inReplyTo":"xmqqttnmfarm.fsf@gitster.g","subject":"Re: [PATCH 2/3] t7450: test submodule urls","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2024-01-11T17:23:04Z","receivedAt":"2024-01-11T17:23:07Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> +#define TEST_TOOL_CHECK_URL_USAGE \\\n>> +\t\"test-tool submodule check-url \"\n>> +static const char *submodule_check_url_usage[] = {\n>> +\tTEST_TOOL_CHECK_URL_USAGE,\n>> +\tNULL\n>> +};\n> \n> Granted, the entry that follows this new one already uses the same\n> pattern, but TEST_TOOL_CHECK_URL_USAGE being used only once here and\n> nowhere else, with its name almost as long as the value it expands to,\n> I found it unnecessarily verbose and confusing.\n\nThis is only used once because I missed the second place it should be used\n(in 'submodule_usage[]'). It's still somewhat verbose, but once I fix that\nit'll at least have the benefit of avoiding some duplication.\n\n"},{"id":"486686","messageId":"20240112065732.GC618729@coredump.intra.peff.net","threadId":"60708","inReplyTo":"d852ad72-32c4-4b0f-8f34-e8b38b7f71ad@github.com","subject":"Re: [PATCH 2/3] t7450: test submodule urls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-12T06:57:32Z","receivedAt":"2024-01-12T06:57:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2024 at 08:54:47AM -0800, Victoria Dye wrote:\n\n> > All of this is inherited from the existing check_name() code, which I\n> > think has all of the same bugs. The test scripts all just use the stdin\n> > mode, so they don't notice. It's not too hard to fix, but maybe it's\n> > worth just ripping out the unreachable code.\n> \n> Thanks for pointing out those issues, I think removing the command line\n> input mode is the way to go. The description of the 'check_name()' mentions\n> that the stdin mode was \"primarily intended for testing\". But as 85321a346b5\n> (submodule--helper: move \"check-name\" to a test-tool, 2022-09-01) pointed\n> out, 'check_name()' was never used outside of tests anyway, so whatever use\n> case was imagined for the command line mode never seemed to have existed. \n> \n> Combine that with the fact that the command line mode is so different from\n> the stdin mode (non-zero exit code for invalid names, prints nothing vs.\n> zero exit code, prints valid names), there don't seem to be any real\n> downsides to removing the unused code.\n\nThat sounds like a good plan to me. :)\n\n-Peff\n"},{"id":"486935","messageId":"08276422-3af8-40df-85dd-65ec4e891507@github.com","threadId":"60708","inReplyTo":"ZZ46MrjSocJl-kpU@tanuki","subject":"Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2024-01-17T21:19:30Z","receivedAt":"2024-01-17T21:19:32Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Patrick Steinhardt wrote:\n> On Tue, Jan 09, 2024 at 05:53:37PM +0000, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n>>\n>> Update the validation of \"curl URL\" submodule URLs (i.e. those that specify\n>> an \"http[s]\" or \"ftp[s]\" protocol) in 'check_submodule_url()' to catch more\n>> invalid URLs. The existing validation using 'credential_from_url_gently()'\n>> parses certain URLs incorrectly, leading to invalid submodule URLs passing\n>> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote\n>> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by\n>> 'credential_from_url_gently()'.\n> \n> Okay, so we retain the wrong behavior of `credential_from_url_gently()`,\n> right? I wonder whether this can be abused in any way, doubly so because\n> the function gets invoked with untrusted input from the remote server\n> when we handle redirects in `http_request_reauth()`. But the redirect\n> URL we end up passing to `credential_from_url_gently()` would have to\n> contain a non-numeric port, and curl seemingly does not know to handle\n> those either.\n\nCorrect, nothing about 'credential_from_url_gently()' changes here. As for\nwhether it could be abused - I don't *think* so, but I'm definitely not a\nsecurity expert. If it helps, here's a more detailed breakdown of the issue:\n\nIn 'credential_from_url_1()', suppose we have URL\n\"http://example.com:test/repo.git\". Stepping through the variables:\n\n- 'cp' is \"example.com:test/repo.git\"\n- 'at' is NULL\n- 'colon' is \":test/repo.git\"\n- 'slash' is \"/repo.git\"\n\nBecause 'at' is NULL, we set 'host = cp'. Later, because 'slash - host > 0',\nwe call 'url_decode_mem()' on \"example.com:test\" (which, in this case,\ndoesn't change anything) and the result 'host' to \"example.com:test\".\n\nThe issue for the fsck check is that 'credential_from_url_gently()' doesn't\nvalidate the hostname it extracts (e.g. whether ':' precedes a valid port,\nor if the hostname contains a '%'-escaped sequence). I don't *think* that\ncould be abused since, like you said, cURL should just reject the invalid\nURL altogether.\n\n> \n> Other callsites include fsck (which you're fixing) and the credential\n> store (which is entirely user-controlled). It would be great regardless\n> to fix the underlying bug in `credential_from_url_gently()` eventually\n> though. But I do not think that this has to be part this patch series\n> here, which is a strict improvement.\n\nAgreed! I think normalizing the URL before trying to extract the credentials\nmay be all that's needed to avoid surprise URL errors, but that probably\nwarrants a separate patch submission (with appropriately thorough testing).\n\n> \n> Thanks!\n> \n> Patrick\n\n"},{"id":"486950","messageId":"pull.1635.v2.git.1705542918.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.git.1704822817.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Strengthen fsck checks for submodule URLs","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T01:55:14Z","receivedAt":"2024-01-18T01:55:22Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"While testing 'git fsck' checks on .gitmodules URLs, I noticed that some\ninvalid URLs were passing the checks. Digging into it a bit more, the issue\nturned out to be that 'credential_from_url_gently()' parses certain URLs\n(like \"http://example.com:something/deeper/path\") incorrectly, in a way that\nappeared to return a valid result.\n\nFortunately, these URLs are rejected in fetches/clones/pushes anyway because\n'url_normalize()' (called in 'validate_remote_url()') correctly identifies\nthem as invalid. So, to bring 'git fsck' in line with other (stronger)\nvalidation done on remote URLs, this series replaces the\n'credential_from_url_gently()' check with one that uses 'url_normalize()'.\n\n * Patch 1 moves 'check_submodule_url()' to a public location so that it can\n be used outside of 'fsck.c'.\n * Patch 2 removes the obsolete/never-used code in 'test-tool submodule\n check-name' handling names provided on the command line.\n * Patch 3 adds a 'check-url' mode to 'test-tool submodule', calling the\n now-public 'check_submodule_url()' method on a given URL, and adds new\n tests checking valid and invalid submodule URLs.\n * Patch 4 replaces the 'credential_from_url_gently()' check with\n 'url_normalize()' followed by 'url_decode()' and an explicit check for\n newlines (to preserve the newline handling added in 07259e74ec1 (fsck:\n detect gitmodules URLs with embedded newlines, 2020-03-11)).\n\n\nChanges since V1\n================\n\n * Added 'TEST_TOOL_CHECK_URL_USAGE' to 'submodule_usage'.\n * Removed unused/unreachable code related to command line inputs in\n 'test-tool submodule check-name' and 'test-tool submodule check-url'.\n * Split the new t7450 test case into two tests (the first contains URLs\n that are validated successfully, the second demonstrates a URL\n incorrectly marked valid) to clearly show which pattern is handled\n improperly. The tests are merged in the final patch once the validation\n is corrected.\n\nThanks!\n\n * Victoria\n\nVictoria Dye (4):\n submodule-config.h: move check_submodule_url\n test-submodule: remove command line handling for check-name\n t7450: test submodule urls\n submodule-config.c: strengthen URL fsck check\n\n fsck.c | 133 ----------------------------------\n submodule-config.c | 140 ++++++++++++++++++++++++++++++++++++\n submodule-config.h | 3 +\n t/helper/test-submodule.c | 52 +++++++++-----\n t/t7450-bad-git-dotfiles.sh | 26 +++++++\n 5 files changed, 203 insertions(+), 151 deletions(-)\n\n\nbase-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1635%2Fvdye%2Fvdye%2Fstrengthen-fsck-url-checks-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1635/vdye/vdye/strengthen-fsck-url-checks-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1635\n\nRange-diff vs v1:\n\n 1: 588de3022d7 = 1: ce1de0406ef submodule-config.h: move check_submodule_url\n -: ----------- > 2: 14e8834c38b test-submodule: remove command line handling for check-name\n 2: cf7848edffc ! 3: b6843a58389 t7450: test submodule urls\n @@ Metadata\n ## Commit message ##\n t7450: test submodule urls\n \n - Add a test to 't7450-bad-git-dotfiles.sh' to check the validity of different\n - submodule URLs. To test this directly (without setting up test repositories\n - & submodules), add a 'check-url' subcommand to 'test-tool submodule' that\n - calls 'check_submodule_url' in the same way that 'check-name' calls\n - 'check_submodule_name'.\n + Add tests to 't7450-bad-git-dotfiles.sh' to check the validity of different\n + submodule URLs. To verify this directly (without setting up test\n + repositories & submodules), add a 'check-url' subcommand to 'test-tool\n + submodule' that calls 'check_submodule_url' in the same way that\n + 'check-name' calls 'check_submodule_name'.\n \n - Mark the test with 'test_expect_failure' because, as it stands,\n - 'check_submodule_url' marks certain invalid URLs valid. Specifically, the\n - invalid URL \"http://example.com:test/foo.git\" is incorrectly marked valid in\n - the test.\n + Add two tests to separately address cases where the URL check correctly\n + filters out invalid URLs and cases where the check misses invalid URLs. Mark\n + the latter (\"url check misses invalid cases\") with 'test_expect_failure' to\n + indicate that this not the undesired behavior.\n \n Signed-off-by: Victoria Dye \n \n @@ t/helper/test-submodule.c: static const char *submodule_check_name_usage[] = {\n };\n \n +#define TEST_TOOL_CHECK_URL_USAGE \\\n -+\t\"test-tool submodule check-url \"\n ++\t\"test-tool submodule check-url\"\n +static const char *submodule_check_url_usage[] = {\n +\tTEST_TOOL_CHECK_URL_USAGE,\n +\tNULL\n @@ t/helper/test-submodule.c: static const char *submodule_check_name_usage[] = {\n #define TEST_TOOL_IS_ACTIVE_USAGE \\\n \t\"test-tool submodule is-active \"\n static const char *submodule_is_active_usage[] = {\n -@@ t/helper/test-submodule.c: static const char *submodule_usage[] = {\n +@@ t/helper/test-submodule.c: static const char *submodule_resolve_relative_url_usage[] = {\n + \n + static const char *submodule_usage[] = {\n + \tTEST_TOOL_CHECK_NAME_USAGE,\n ++\tTEST_TOOL_CHECK_URL_USAGE,\n + \tTEST_TOOL_IS_ACTIVE_USAGE,\n + \tTEST_TOOL_RESOLVE_RELATIVE_URL_USAGE,\n \tNULL\n };\n \n +-/* Filter stdin to print only valid names. */\n +-static int check_name(void)\n +typedef int (*check_fn_t)(const char *);\n +\n - /*\n - * Exit non-zero if any of the submodule names given on the command line is\n - * invalid. If no names are given, filter stdin to print only valid names\n - * (which is primarily intended for testing).\n - */\n --static int check_name(int argc, const char **argv)\n -+static int check_submodule(int argc, const char **argv, check_fn_t check_fn)\n ++/*\n ++ * Apply 'check_fn' to each line of stdin, printing values that pass the check\n ++ * to stdout.\n ++ */\n ++static int check_submodule(check_fn_t check_fn)\n {\n - \tif (argc > 1) {\n - \t\twhile (*++argv) {\n --\t\t\tif (check_submodule_name(*argv) < 0)\n -+\t\t\tif (check_fn(*argv) < 0)\n - \t\t\t\treturn 1;\n - \t\t}\n - \t} else {\n - \t\tstruct strbuf buf = STRBUF_INIT;\n - \t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n --\t\t\tif (!check_submodule_name(buf.buf))\n -+\t\t\tif (!check_fn(buf.buf))\n - \t\t\t\tprintf(\"%s\\n\", buf.buf);\n - \t\t}\n - \t\tstrbuf_release(&buf);\n + \tstruct strbuf buf = STRBUF_INIT;\n + \twhile (strbuf_getline(&buf, stdin) != EOF) {\n +-\t\tif (!check_submodule_name(buf.buf))\n ++\t\tif (!check_fn(buf.buf))\n + \t\t\tprintf(\"%s\\n\", buf.buf);\n + \t}\n + \tstrbuf_release(&buf);\n @@ t/helper/test-submodule.c: static int cmd__submodule_check_name(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(submodule_check_name_usage, options);\n \n --\treturn check_name(argc, argv);\n -+\treturn check_submodule(argc, argv, check_submodule_name);\n +-\treturn check_name();\n ++\treturn check_submodule(check_submodule_name);\n +}\n +\n +static int cmd__submodule_check_url(int argc, const char **argv)\n @@ t/helper/test-submodule.c: static int cmd__submodule_check_name(int argc, const\n +\tif (argc)\n +\t\tusage_with_options(submodule_check_url_usage, options);\n +\n -+\treturn check_submodule(argc, argv, check_submodule_url);\n ++\treturn check_submodule(check_submodule_url);\n }\n \n static int cmd__submodule_is_active(int argc, const char **argv)\n @@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check names' '\n \ttest_cmp expect actual\n '\n \n -+test_expect_failure 'check urls' '\n ++test_expect_success 'check urls' '\n +\tcat >expect <<-\\EOF &&\n +\t./bar/baz/foo.git\n +\thttps://example.com/foo.git\n @@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check names' '\n +\t./%0ahost=example.com/foo.git\n +\thttps://one.example.com/evil?%0ahost=two.example.com\n +\thttps:///example.com/foo.git\n -+\thttp://example.com:test/foo.git\n +\thttps::example.com/foo.git\n +\thttp:::example.com/foo.git\n +\tEOF\n +\n +\ttest_cmp expect actual\n +'\n ++\n ++# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if\n ++# a user attempts to clone it), but the fsck check passes.\n ++test_expect_failure 'url check misses invalid cases' '\n ++\ttest-tool submodule check-url >actual <<-\\EOF &&\n ++\thttp://example.com:test/foo.git\n ++\tEOF\n ++\n ++\ttest_must_be_empty actual\n ++'\n +\n test_expect_success 'create innocent subrepo' '\n \tgit init innocent &&\n 3: 893071530d3 ! 4: b79b1a71780 submodule-config.c: strengthen URL fsck check\n @@ submodule-config.c: int check_submodule_url(const char *url)\n \n \n ## t/t7450-bad-git-dotfiles.sh ##\n -@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check names' '\n +@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check urls' '\n + \t./%0ahost=example.com/foo.git\n + \thttps://one.example.com/evil?%0ahost=two.example.com\n + \thttps:///example.com/foo.git\n ++\thttp://example.com:test/foo.git\n + \thttps::example.com/foo.git\n + \thttp:::example.com/foo.git\n + \tEOF\n +@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'check urls' '\n \ttest_cmp expect actual\n '\n \n --test_expect_failure 'check urls' '\n -+test_expect_success 'check urls' '\n - \tcat >expect <<-\\EOF &&\n - \t./bar/baz/foo.git\n - \thttps://example.com/foo.git\n +-# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if\n +-# a user attempts to clone it), but the fsck check passes.\n +-test_expect_failure 'url check misses invalid cases' '\n +-\ttest-tool submodule check-url >actual <<-\\EOF &&\n +-\thttp://example.com:test/foo.git\n +-\tEOF\n +-\n +-\ttest_must_be_empty actual\n +-'\n +-\n + test_expect_success 'create innocent subrepo' '\n + \tgit init innocent &&\n + \tgit -C innocent commit --allow-empty -m foo\n\n-- \ngitgitgadget\n"},{"id":"486951","messageId":"ce1de0406ef782c80b5e9181af2b8df991452787.1705542918.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.v2.git.1705542918.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] submodule-config.h: move check_submodule_url","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T01:55:15Z","receivedAt":"2024-01-18T01:55:22Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nMove 'check_submodule_url' out of 'fsck.c' and into 'submodule-config.h' as\na public method, similar to 'check_submodule_name'. With the function now\naccessible outside of 'fsck', it can be used in a later commit to extend\n'test-tool submodule' to check the validity of submodule URLs as it does\nwith names in the 'check-name' subcommand.\n\nOther than its location, no changes are made to 'check_submodule_url' in\nthis patch.\n\nSigned-off-by: Victoria Dye \n---\n fsck.c | 133 --------------------------------------------\n submodule-config.c | 134 +++++++++++++++++++++++++++++++++++++++++++++\n submodule-config.h | 3 +\n 3 files changed, 137 insertions(+), 133 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 1ad02fcdfab..8ded0a473a4 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -20,7 +20,6 @@\n #include \"packfile.h\"\n #include \"submodule-config.h\"\n #include \"config.h\"\n-#include \"credential.h\"\n #include \"help.h\"\n \n static ssize_t max_tree_entry_len = 4096;\n@@ -1047,138 +1046,6 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,\n \treturn ret;\n }\n \n-static int starts_with_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n-static int starts_with_dot_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n-static int submodule_url_is_relative(const char *url)\n-{\n-\treturn starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);\n-}\n-\n-/*\n- * Count directory components that a relative submodule URL should chop\n- * from the remote_url it is to be resolved against.\n- *\n- * In other words, this counts \"../\" components at the start of a\n- * submodule URL.\n- *\n- * Returns the number of directory components to chop and writes a\n- * pointer to the next character of url after all leading \"./\" and\n- * \"../\" components to out.\n- */\n-static int count_leading_dotdots(const char *url, const char **out)\n-{\n-\tint result = 0;\n-\twhile (1) {\n-\t\tif (starts_with_dot_dot_slash(url)) {\n-\t\t\tresult++;\n-\t\t\turl += strlen(\"../\");\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with_dot_slash(url)) {\n-\t\t\turl += strlen(\"./\");\n-\t\t\tcontinue;\n-\t\t}\n-\t\t*out = url;\n-\t\treturn result;\n-\t}\n-}\n-/*\n- * Check whether a transport is implemented by git-remote-curl.\n- *\n- * If it is, returns 1 and writes the URL that would be passed to\n- * git-remote-curl to the \"out\" parameter.\n- *\n- * Otherwise, returns 0 and leaves \"out\" untouched.\n- *\n- * Examples:\n- * http::https://example.com/repo.git -> 1, https://example.com/repo.git\n- * https://example.com/repo.git -> 1, https://example.com/repo.git\n- * git://example.com/repo.git -> 0\n- *\n- * This is for use in checking for previously exploitable bugs that\n- * required a submodule URL to be passed to git-remote-curl.\n- */\n-static int url_to_curl_url(const char *url, const char **out)\n-{\n-\t/*\n-\t * We don't need to check for case-aliases, \"http.exe\", and so\n-\t * on because in the default configuration, is_transport_allowed\n-\t * prevents URLs with those schemes from being cloned\n-\t * automatically.\n-\t */\n-\tif (skip_prefix(url, \"http::\", out) ||\n-\t skip_prefix(url, \"https::\", out) ||\n-\t skip_prefix(url, \"ftp::\", out) ||\n-\t skip_prefix(url, \"ftps::\", out))\n-\t\treturn 1;\n-\tif (starts_with(url, \"http://\") ||\n-\t starts_with(url, \"https://\") ||\n-\t starts_with(url, \"ftp://\") ||\n-\t starts_with(url, \"ftps://\")) {\n-\t\t*out = url;\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n-static int check_submodule_url(const char *url)\n-{\n-\tconst char *curl_url;\n-\n-\tif (looks_like_command_line_option(url))\n-\t\treturn -1;\n-\n-\tif (submodule_url_is_relative(url) || starts_with(url, \"git://\")) {\n-\t\tchar *decoded;\n-\t\tconst char *next;\n-\t\tint has_nl;\n-\n-\t\t/*\n-\t\t * This could be appended to an http URL and url-decoded;\n-\t\t * check for malicious characters.\n-\t\t */\n-\t\tdecoded = url_decode(url);\n-\t\thas_nl = !!strchr(decoded, '\\n');\n-\n-\t\tfree(decoded);\n-\t\tif (has_nl)\n-\t\t\treturn -1;\n-\n-\t\t/*\n-\t\t * URLs which escape their root via \"../\" can overwrite\n-\t\t * the host field and previous components, resolving to\n-\t\t * URLs like https::example.com/submodule.git and\n-\t\t * https:///example.com/submodule.git that were\n-\t\t * susceptible to CVE-2020-11008.\n-\t\t */\n-\t\tif (count_leading_dotdots(url, &next) > 0 &&\n-\t\t (*next == ':' || *next == '/'))\n-\t\t\treturn -1;\n-\t}\n-\n-\telse if (url_to_curl_url(url, &curl_url)) {\n-\t\tstruct credential c = CREDENTIAL_INIT;\n-\t\tint ret = 0;\n-\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n-\t\t !*c.host)\n-\t\t\tret = -1;\n-\t\tcredential_clear(&c);\n-\t\treturn ret;\n-\t}\n-\n-\treturn 0;\n-}\n-\n struct fsck_gitmodules_data {\n \tconst struct object_id *oid;\n \tstruct fsck_options *options;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex f4dd482abc9..3b295e9f89c 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -14,6 +14,8 @@\n #include \"parse-options.h\"\n #include \"thread-utils.h\"\n #include \"tree-walk.h\"\n+#include \"url.h\"\n+#include \"credential.h\"\n \n /*\n * submodule cache lookup structure\n@@ -228,6 +230,138 @@ int check_submodule_name(const char *name)\n \treturn 0;\n }\n \n+static int starts_with_dot_slash(const char *const path)\n+{\n+\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |\n+\t\t\t\tPATH_MATCH_XPLATFORM);\n+}\n+\n+static int starts_with_dot_dot_slash(const char *const path)\n+{\n+\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |\n+\t\t\t\tPATH_MATCH_XPLATFORM);\n+}\n+\n+static int submodule_url_is_relative(const char *url)\n+{\n+\treturn starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);\n+}\n+\n+/*\n+ * Count directory components that a relative submodule URL should chop\n+ * from the remote_url it is to be resolved against.\n+ *\n+ * In other words, this counts \"../\" components at the start of a\n+ * submodule URL.\n+ *\n+ * Returns the number of directory components to chop and writes a\n+ * pointer to the next character of url after all leading \"./\" and\n+ * \"../\" components to out.\n+ */\n+static int count_leading_dotdots(const char *url, const char **out)\n+{\n+\tint result = 0;\n+\twhile (1) {\n+\t\tif (starts_with_dot_dot_slash(url)) {\n+\t\t\tresult++;\n+\t\t\turl += strlen(\"../\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (starts_with_dot_slash(url)) {\n+\t\t\turl += strlen(\"./\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\t*out = url;\n+\t\treturn result;\n+\t}\n+}\n+/*\n+ * Check whether a transport is implemented by git-remote-curl.\n+ *\n+ * If it is, returns 1 and writes the URL that would be passed to\n+ * git-remote-curl to the \"out\" parameter.\n+ *\n+ * Otherwise, returns 0 and leaves \"out\" untouched.\n+ *\n+ * Examples:\n+ * http::https://example.com/repo.git -> 1, https://example.com/repo.git\n+ * https://example.com/repo.git -> 1, https://example.com/repo.git\n+ * git://example.com/repo.git -> 0\n+ *\n+ * This is for use in checking for previously exploitable bugs that\n+ * required a submodule URL to be passed to git-remote-curl.\n+ */\n+static int url_to_curl_url(const char *url, const char **out)\n+{\n+\t/*\n+\t * We don't need to check for case-aliases, \"http.exe\", and so\n+\t * on because in the default configuration, is_transport_allowed\n+\t * prevents URLs with those schemes from being cloned\n+\t * automatically.\n+\t */\n+\tif (skip_prefix(url, \"http::\", out) ||\n+\t skip_prefix(url, \"https::\", out) ||\n+\t skip_prefix(url, \"ftp::\", out) ||\n+\t skip_prefix(url, \"ftps::\", out))\n+\t\treturn 1;\n+\tif (starts_with(url, \"http://\") ||\n+\t starts_with(url, \"https://\") ||\n+\t starts_with(url, \"ftp://\") ||\n+\t starts_with(url, \"ftps://\")) {\n+\t\t*out = url;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+int check_submodule_url(const char *url)\n+{\n+\tconst char *curl_url;\n+\n+\tif (looks_like_command_line_option(url))\n+\t\treturn -1;\n+\n+\tif (submodule_url_is_relative(url) || starts_with(url, \"git://\")) {\n+\t\tchar *decoded;\n+\t\tconst char *next;\n+\t\tint has_nl;\n+\n+\t\t/*\n+\t\t * This could be appended to an http URL and url-decoded;\n+\t\t * check for malicious characters.\n+\t\t */\n+\t\tdecoded = url_decode(url);\n+\t\thas_nl = !!strchr(decoded, '\\n');\n+\n+\t\tfree(decoded);\n+\t\tif (has_nl)\n+\t\t\treturn -1;\n+\n+\t\t/*\n+\t\t * URLs which escape their root via \"../\" can overwrite\n+\t\t * the host field and previous components, resolving to\n+\t\t * URLs like https::example.com/submodule.git and\n+\t\t * https:///example.com/submodule.git that were\n+\t\t * susceptible to CVE-2020-11008.\n+\t\t */\n+\t\tif (count_leading_dotdots(url, &next) > 0 &&\n+\t\t (*next == ':' || *next == '/'))\n+\t\t\treturn -1;\n+\t}\n+\n+\telse if (url_to_curl_url(url, &curl_url)) {\n+\t\tstruct credential c = CREDENTIAL_INIT;\n+\t\tint ret = 0;\n+\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n+\t\t !*c.host)\n+\t\t\tret = -1;\n+\t\tcredential_clear(&c);\n+\t\treturn ret;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int name_and_item_from_var(const char *var, struct strbuf *name,\n \t\t\t\t struct strbuf *item)\n {\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 958f320ac6c..b6133af71b0 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -89,6 +89,9 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value);\n */\n int check_submodule_name(const char *name);\n \n+/* Returns 0 if the URL valid per RFC3986 and -1 otherwise. */\n+int check_submodule_url(const char *url);\n+\n /*\n * Note: these helper functions exist solely to maintain backward\n * compatibility with 'fetch' and 'update_clone' storing configuration in\n-- \ngitgitgadget\n\n"},{"id":"486952","messageId":"14e8834c38bcddc21856772b09f6fa77fa924b48.1705542918.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.v2.git.1705542918.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] test-submodule: remove command line handling for check-name","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T01:55:16Z","receivedAt":"2024-01-18T01:55:23Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nThe 'check-name' subcommand to 'test-tool submodule' is documented as being\nable to take a command line argument ''. However, this does not work -\nand has never worked - because 'argc > 0' triggers the usage message in\n'cmd__submodule_check_name()'. To simplify the helper and avoid future\nconfusion around proper use of the subcommand, remove any references to\ncommand line arguments for 'check-name' in usage strings and handling in\n'check_name()'.\n\nHelped-by: Jeff King \nSigned-off-by: Victoria Dye \n---\n t/helper/test-submodule.c | 29 +++++++++--------------------\n 1 file changed, 9 insertions(+), 20 deletions(-)\n\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex 50c154d0370..9adbc8d1568 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -9,7 +9,7 @@\n #include \"submodule.h\"\n \n #define TEST_TOOL_CHECK_NAME_USAGE \\\n-\t\"test-tool submodule check-name \"\n+\t\"test-tool submodule check-name\"\n static const char *submodule_check_name_usage[] = {\n \tTEST_TOOL_CHECK_NAME_USAGE,\n \tNULL\n@@ -36,26 +36,15 @@ static const char *submodule_usage[] = {\n \tNULL\n };\n \n-/*\n- * Exit non-zero if any of the submodule names given on the command line is\n- * invalid. If no names are given, filter stdin to print only valid names\n- * (which is primarily intended for testing).\n- */\n-static int check_name(int argc, const char **argv)\n+/* Filter stdin to print only valid names. */\n+static int check_name(void)\n {\n-\tif (argc > 1) {\n-\t\twhile (*++argv) {\n-\t\t\tif (check_submodule_name(*argv) < 0)\n-\t\t\t\treturn 1;\n-\t\t}\n-\t} else {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\t\tif (!check_submodule_name(buf.buf))\n-\t\t\t\tprintf(\"%s\\n\", buf.buf);\n-\t\t}\n-\t\tstrbuf_release(&buf);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\twhile (strbuf_getline(&buf, stdin) != EOF) {\n+\t\tif (!check_submodule_name(buf.buf))\n+\t\t\tprintf(\"%s\\n\", buf.buf);\n \t}\n+\tstrbuf_release(&buf);\n \treturn 0;\n }\n \n@@ -69,7 +58,7 @@ static int cmd__submodule_check_name(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(submodule_check_name_usage, options);\n \n-\treturn check_name(argc, argv);\n+\treturn check_name();\n }\n \n static int cmd__submodule_is_active(int argc, const char **argv)\n-- \ngitgitgadget\n\n"},{"id":"486953","messageId":"b6843a58389170a45b5ef7809e0335a6425eadaa.1705542918.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.v2.git.1705542918.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] t7450: test submodule urls","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T01:55:17Z","receivedAt":"2024-01-18T01:55:24Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd tests to 't7450-bad-git-dotfiles.sh' to check the validity of different\nsubmodule URLs. To verify this directly (without setting up test\nrepositories & submodules), add a 'check-url' subcommand to 'test-tool\nsubmodule' that calls 'check_submodule_url' in the same way that\n'check-name' calls 'check_submodule_name'.\n\nAdd two tests to separately address cases where the URL check correctly\nfilters out invalid URLs and cases where the check misses invalid URLs. Mark\nthe latter (\"url check misses invalid cases\") with 'test_expect_failure' to\nindicate that this not the undesired behavior.\n\nSigned-off-by: Victoria Dye \n---\n t/helper/test-submodule.c | 35 +++++++++++++++++++++++++++++++----\n t/t7450-bad-git-dotfiles.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex 9adbc8d1568..7197969a081 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -15,6 +15,13 @@ static const char *submodule_check_name_usage[] = {\n \tNULL\n };\n \n+#define TEST_TOOL_CHECK_URL_USAGE \\\n+\t\"test-tool submodule check-url\"\n+static const char *submodule_check_url_usage[] = {\n+\tTEST_TOOL_CHECK_URL_USAGE,\n+\tNULL\n+};\n+\n #define TEST_TOOL_IS_ACTIVE_USAGE \\\n \t\"test-tool submodule is-active \"\n static const char *submodule_is_active_usage[] = {\n@@ -31,17 +38,23 @@ static const char *submodule_resolve_relative_url_usage[] = {\n \n static const char *submodule_usage[] = {\n \tTEST_TOOL_CHECK_NAME_USAGE,\n+\tTEST_TOOL_CHECK_URL_USAGE,\n \tTEST_TOOL_IS_ACTIVE_USAGE,\n \tTEST_TOOL_RESOLVE_RELATIVE_URL_USAGE,\n \tNULL\n };\n \n-/* Filter stdin to print only valid names. */\n-static int check_name(void)\n+typedef int (*check_fn_t)(const char *);\n+\n+/*\n+ * Apply 'check_fn' to each line of stdin, printing values that pass the check\n+ * to stdout.\n+ */\n+static int check_submodule(check_fn_t check_fn)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\tif (!check_submodule_name(buf.buf))\n+\t\tif (!check_fn(buf.buf))\n \t\t\tprintf(\"%s\\n\", buf.buf);\n \t}\n \tstrbuf_release(&buf);\n@@ -58,7 +71,20 @@ static int cmd__submodule_check_name(int argc, const char **argv)\n \tif (argc)\n \t\tusage_with_options(submodule_check_name_usage, options);\n \n-\treturn check_name();\n+\treturn check_submodule(check_submodule_name);\n+}\n+\n+static int cmd__submodule_check_url(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t submodule_check_url_usage, 0);\n+\tif (argc)\n+\t\tusage_with_options(submodule_check_url_usage, options);\n+\n+\treturn check_submodule(check_submodule_url);\n }\n \n static int cmd__submodule_is_active(int argc, const char **argv)\n@@ -184,6 +210,7 @@ static int cmd__submodule_config_writeable(int argc, const char **argv UNUSED)\n \n static struct test_cmd cmds[] = {\n \t{ \"check-name\", cmd__submodule_check_name },\n+\t{ \"check-url\", cmd__submodule_check_url },\n \t{ \"is-active\", cmd__submodule_is_active },\n \t{ \"resolve-relative-url\", cmd__submodule_resolve_relative_url},\n \t{ \"config-list\", cmd__submodule_config_list },\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 35a31acd4d7..c73b1c92ecc 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -45,6 +45,41 @@ test_expect_success 'check names' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check urls' '\n+\tcat >expect <<-\\EOF &&\n+\t./bar/baz/foo.git\n+\thttps://example.com/foo.git\n+\thttp://example.com:80/deeper/foo.git\n+\tEOF\n+\n+\ttest-tool submodule check-url >actual <<-\\EOF &&\n+\t./bar/baz/foo.git\n+\thttps://example.com/foo.git\n+\thttp://example.com:80/deeper/foo.git\n+\t-a./foo\n+\t../../..//test/foo.git\n+\t../../../../../:localhost:8080/foo.git\n+\t..\\../.\\../:example.com/foo.git\n+\t./%0ahost=example.com/foo.git\n+\thttps://one.example.com/evil?%0ahost=two.example.com\n+\thttps:///example.com/foo.git\n+\thttps::example.com/foo.git\n+\thttp:::example.com/foo.git\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n+# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if\n+# a user attempts to clone it), but the fsck check passes.\n+test_expect_failure 'url check misses invalid cases' '\n+\ttest-tool submodule check-url >actual <<-\\EOF &&\n+\thttp://example.com:test/foo.git\n+\tEOF\n+\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'create innocent subrepo' '\n \tgit init innocent &&\n \tgit -C innocent commit --allow-empty -m foo\n-- \ngitgitgadget\n\n"},{"id":"486954","messageId":"b79b1a7178076be1d1a80212dfabd3c37587d443.1705542918.git.gitgitgadget@gmail.com","threadId":"60708","inReplyTo":"pull.1635.v2.git.1705542918.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] submodule-config.c: strengthen URL fsck check","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-18T01:55:18Z","receivedAt":"2024-01-18T01:55:24Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the validation of \"curl URL\" submodule URLs (i.e. those that specify\nan \"http[s]\" or \"ftp[s]\" protocol) in 'check_submodule_url()' to catch more\ninvalid URLs. The existing validation using 'credential_from_url_gently()'\nparses certain URLs incorrectly, leading to invalid submodule URLs passing\n'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote\nURLs in 'remote_get()' - correctly identifies the invalid URLs missed by\n'credential_from_url_gently()'.\n\nTo catch more invalid cases, replace 'credential_from_url_gently()' with\n'url_normalize()' followed by a 'url_decode()' and a check for newlines\n(mirroring 'check_url_component()' in the 'credential_from_url_gently()'\nvalidation).\n\nSigned-off-by: Victoria Dye \n---\n submodule-config.c | 16 +++++++++++-----\n t/t7450-bad-git-dotfiles.sh | 11 +----------\n 2 files changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 3b295e9f89c..54130f6a385 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -15,7 +15,7 @@\n #include \"thread-utils.h\"\n #include \"tree-walk.h\"\n #include \"url.h\"\n-#include \"credential.h\"\n+#include \"urlmatch.h\"\n \n /*\n * submodule cache lookup structure\n@@ -350,12 +350,18 @@ int check_submodule_url(const char *url)\n \t}\n \n \telse if (url_to_curl_url(url, &curl_url)) {\n-\t\tstruct credential c = CREDENTIAL_INIT;\n \t\tint ret = 0;\n-\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n-\t\t !*c.host)\n+\t\tchar *normalized = url_normalize(curl_url, NULL);\n+\t\tif (normalized) {\n+\t\t\tchar *decoded = url_decode(normalized);\n+\t\t\tif (strchr(decoded, '\\n'))\n+\t\t\t\tret = -1;\n+\t\t\tfree(normalized);\n+\t\t\tfree(decoded);\n+\t\t} else {\n \t\t\tret = -1;\n-\t\tcredential_clear(&c);\n+\t\t}\n+\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex c73b1c92ecc..46d4fb0354b 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -63,6 +63,7 @@ test_expect_success 'check urls' '\n \t./%0ahost=example.com/foo.git\n \thttps://one.example.com/evil?%0ahost=two.example.com\n \thttps:///example.com/foo.git\n+\thttp://example.com:test/foo.git\n \thttps::example.com/foo.git\n \thttp:::example.com/foo.git\n \tEOF\n@@ -70,16 +71,6 @@ test_expect_success 'check urls' '\n \ttest_cmp expect actual\n '\n \n-# NEEDSWORK: the URL checked here is not valid (and will not work as a remote if\n-# a user attempts to clone it), but the fsck check passes.\n-test_expect_failure 'url check misses invalid cases' '\n-\ttest-tool submodule check-url >actual <<-\\EOF &&\n-\thttp://example.com:test/foo.git\n-\tEOF\n-\n-\ttest_must_be_empty actual\n-'\n-\n test_expect_success 'create innocent subrepo' '\n \tgit init innocent &&\n \tgit -C innocent commit --allow-empty -m foo\n-- \ngitgitgadget\n"},{"id":"486988","messageId":"xmqqmst2sdn0.fsf@gitster.g","threadId":"60708","inReplyTo":"pull.1635.v2.git.1705542918.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] Strengthen fsck checks for submodule URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T18:24:51Z","receivedAt":"2024-01-18T18:24:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> While testing 'git fsck' checks on .gitmodules URLs, I noticed that some\n> invalid URLs were passing the checks. Digging into it a bit more, the issue\n> turned out to be that 'credential_from_url_gently()' parses certain URLs\n> (like \"http://example.com:something/deeper/path\") incorrectly, in a way that\n> appeared to return a valid result.\n>\n> Fortunately, these URLs are rejected in fetches/clones/pushes anyway because\n> 'url_normalize()' (called in 'validate_remote_url()') correctly identifies\n> them as invalid. So, to bring 'git fsck' in line with other (stronger)\n> validation done on remote URLs, this series replaces the\n> 'credential_from_url_gently()' check with one that uses 'url_normalize()'.\n>\n> * Patch 1 moves 'check_submodule_url()' to a public location so that it can\n> be used outside of 'fsck.c'.\n> * Patch 2 removes the obsolete/never-used code in 'test-tool submodule\n> check-name' handling names provided on the command line.\n> * Patch 3 adds a 'check-url' mode to 'test-tool submodule', calling the\n> now-public 'check_submodule_url()' method on a given URL, and adds new\n> tests checking valid and invalid submodule URLs.\n> * Patch 4 replaces the 'credential_from_url_gently()' check with\n> 'url_normalize()' followed by 'url_decode()' and an explicit check for\n> newlines (to preserve the newline handling added in 07259e74ec1 (fsck:\n> detect gitmodules URLs with embedded newlines, 2020-03-11)).\n\nNicely done. I'll wait for a few days to see if anybody else has\nreaction but after reading the patches myself, my inclination is to\nsuggest merging it to 'next'.\n\nThanks.\n"},{"id":"487004","messageId":"xmqqplxype1b.fsf@gitster.g","threadId":"60708","inReplyTo":"14e8834c38bcddc21856772b09f6fa77fa924b48.1705542918.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] test-submodule: remove command line handling for check-name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-18T20:44:32Z","receivedAt":"2024-01-18T20:44:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> The 'check-name' subcommand to 'test-tool submodule' is documented as being\n> able to take a command line argument ''. However, this does not work -\n> and has never worked - because 'argc > 0' triggers the usage message in\n> 'cmd__submodule_check_name()'. To simplify the helper and avoid future\n> confusion around proper use of the subcommand, remove any references to\n> command line arguments for 'check-name' in usage strings and handling in\n> 'check_name()'.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Victoria Dye \n> ---\n> t/helper/test-submodule.c | 29 +++++++++--------------------\n> 1 file changed, 9 insertions(+), 20 deletions(-)\n\nExcellent, both of you.\n"},{"id":"487041","messageId":"ZaoRDniGoIBXmjVx@tanuki","threadId":"60708","inReplyTo":"b6843a58389170a45b5ef7809e0335a6425eadaa.1705542918.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] t7450: test submodule urls","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-19T06:05:02Z","receivedAt":"2024-01-19T06:05:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 18, 2024 at 01:55:17AM +0000, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Add tests to 't7450-bad-git-dotfiles.sh' to check the validity of different\n> submodule URLs. To verify this directly (without setting up test\n> repositories & submodules), add a 'check-url' subcommand to 'test-tool\n> submodule' that calls 'check_submodule_url' in the same way that\n> 'check-name' calls 'check_submodule_name'.\n> \n> Add two tests to separately address cases where the URL check correctly\n> filters out invalid URLs and cases where the check misses invalid URLs. Mark\n> the latter (\"url check misses invalid cases\") with 'test_expect_failure' to\n> indicate that this not the undesired behavior.\n\nNit: this should probably say \"to indicate that this is not the desired\nbehaviour.\" But given that the other patches in this series look good to\nme I don't think this warrants a reroll.\n\nThanks!\n\nPatrick\n"},{"id":"487081","messageId":"xmqqbk9hkx3p.fsf@gitster.g","threadId":"60708","inReplyTo":"ZaoRDniGoIBXmjVx@tanuki","subject":"Re: [PATCH v2 3/4] t7450: test submodule urls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-19T18:16:10Z","receivedAt":"2024-01-19T18:16:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Add two tests to separately address cases where the URL check correctly\n>> filters out invalid URLs and cases where the check misses invalid URLs. Mark\n>> the latter (\"url check misses invalid cases\") with 'test_expect_failure' to\n>> indicate that this not the undesired behavior.\n>\n> Nit: this should probably say \"to indicate that this is not the desired\n> behaviour.\" But given that the other patches in this series look good to\n> me I don't think this warrants a reroll.\n\nGood eyes.\n\nI'll rewrite that part to \"... to indicate that this is currently\nbroken, which will be fixed in the next step.\" before merging the\nseries to 'next'.\n\nThanks.\n"},{"id":"487120","messageId":"20240120005136.GB117170@coredump.intra.peff.net","threadId":"60708","inReplyTo":"xmqqmst2sdn0.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] Strengthen fsck checks for submodule URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-20T00:51:36Z","receivedAt":"2024-01-20T00:51:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 18, 2024 at 10:24:51AM -0800, Junio C Hamano wrote:\n\n> > * Patch 1 moves 'check_submodule_url()' to a public location so that it can\n> > be used outside of 'fsck.c'.\n> > * Patch 2 removes the obsolete/never-used code in 'test-tool submodule\n> > check-name' handling names provided on the command line.\n> > * Patch 3 adds a 'check-url' mode to 'test-tool submodule', calling the\n> > now-public 'check_submodule_url()' method on a given URL, and adds new\n> > tests checking valid and invalid submodule URLs.\n> > * Patch 4 replaces the 'credential_from_url_gently()' check with\n> > 'url_normalize()' followed by 'url_decode()' and an explicit check for\n> > newlines (to preserve the newline handling added in 07259e74ec1 (fsck:\n> > detect gitmodules URLs with embedded newlines, 2020-03-11)).\n> \n> Nicely done. I'll wait for a few days to see if anybody else has\n> reaction but after reading the patches myself, my inclination is to\n> suggest merging it to 'next'.\n\nIt all looks good to me to go to 'next'.\n\nAfter simplifying the input handling in patch 2, I probably would not\nhave bothered with the abstracted interface in patch 3 (and instead just\nrepeated the few lines of boilerplate, since there's so much already).\nMostly just because function pointers in C often make reading and\ndebugging more annoying. But I don't think it's a very big deal either\nway in this instance.\n\n-Peff\n"},{"id":"507231","messageId":"d9f53fe7-6570-4aea-894c-942e12e012c4@mayhew.name","threadId":"60708","inReplyTo":"20240110102338.GA16674@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Neil Mayhew","fromEmail":"neil@mayhew.name","sentAt":"2024-11-13T19:24:21Z","receivedAt":"2024-11-13T19:24:25Z","isPatch":true,"sender":{"key":"neil@mayhew.name","avatar":"https://gravatar.com/avatar/8590e81106da7bc57a7d85d70e3814898ad4cdace681a8d48cac665c3fd51150?d=mp&s=160"},"body":"On 10 Jan 24 03:23, Jeff King wrote:\n > By making it an fsck\n > check, though, any mistakes that are embedded in history (even if\n > they are now corrected) will make it a pain to use the repository\n > with sites that enable transfer.fsckObjects.\n >\n > My gut feeling is that this is probably OK in practice. If it does\n > cause pain, we might consider loosening the fsck.gitmodulesUrl\n > severity (under the notion from above that it is no longer a\n > critical security check). But if it doesn't cause real-world pain,\n > being pickier is probably better (it may save us from a\n > vulnerability down the road).\n\nThis pain is happening in \nhttps://github.com/IntersectMBO/cardano-ledger.git, a large open-source \nrepo. There was a bad edit to .gitmodules which was immediately \ncorrected by another commit. However, the bad commit is still in the \nhistory. It happened 6 years ago, so there's no possibility of us \nchanging the history. We just spent time investigating a bug report from \nsomeone who was unable to clone the repo, and eventually we discovered \nthat they had transfer.fsckObjects enabled. Even without this option, \nhowever, we still want people to be able to run fsck successfully on the \nrepo.\n\nIt's awkward that our repo now won't pass an fsck check and we have no \nway to correct that. I'd really like not to have to put a note in the \nREADME warning about this.\n\nIs there any possibility of \"loosening the fsck.gitmodulesUrl severity\", \nas Jeff suggested?\n\n"},{"id":"507232","messageId":"c9260270-c783-45d0-8842-304abc978870@mayhew.name","threadId":"60708","inReplyTo":"d9f53fe7-6570-4aea-894c-942e12e012c4@mayhew.name","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Neil Mayhew","fromEmail":"neil@mayhew.name","sentAt":"2024-11-13T19:44:44Z","receivedAt":"2024-11-13T19:44:47Z","isPatch":true,"sender":{"key":"neil@mayhew.name","avatar":"https://gravatar.com/avatar/8590e81106da7bc57a7d85d70e3814898ad4cdace681a8d48cac665c3fd51150?d=mp&s=160"},"body":"On 13 Nov 24 12:24, Neil Mayhew wrote:\n> This pain is happening in \n> https://github.com/IntersectMBO/cardano-ledger.git, a large \n> open-source repo.\n\nIn case it's helpful, here's the script I wrote to reproduce the problem \nminimally in a fresh repo:\n\nhttps://gist.github.com/neilmayhew/f01dd8f807ebc0bdd37c4db154eabf64\n\n"},{"id":"507235","messageId":"xmqq5xoqahbx.fsf@gitster.g","threadId":"60708","inReplyTo":"d9f53fe7-6570-4aea-894c-942e12e012c4@mayhew.name","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-13T22:40:02Z","receivedAt":"2024-11-13T22:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neil Mayhew writes:\n\n> ... immediately corrected by another commit. However, the bad commit is\n> still in the history. It happened 6 years ago, so there's no\n> possibility of us changing the history.\n\nI think fsck.skipList was meant to cover such a case. The idea is\nthat the blob object name of the bad .gitmodules file can be placed\non the list, and the rest of the \"bad commit\" and the whole history\ncan still be checked for consistency, without triggering the warning\n(or error) resulting from the offending .gitmodules file.\n\n> Is there any possibility of \"loosening the fsck.gitmodulesUrl\n> severity\", as Jeff suggested?\n\nIsn't the suggestion not about butchering the rest of the world but\nby locally configuring fsck.gitmodulesUrl down from error to\nwarning? I personally think excluding a single known-offending blob\nwithout doing such loosening is a much better idea in that it\nprevents *new* offending instances from getting into the repository,\nwhile allowing an existing benign and honest mistake to stay in your\nhistory. Loosening the severity of a class of check means you will\naccept *new* offending instances, which may very well be malicious,\nunlike the existing benign one you know about.\n\n"},{"id":"507239","messageId":"20241114001003.GA1140565@coredump.intra.peff.net","threadId":"60708","inReplyTo":"xmqq5xoqahbx.fsf@gitster.g","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-14T00:10:03Z","receivedAt":"2024-11-14T00:10:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2024 at 07:40:02AM +0900, Junio C Hamano wrote:\n\n> > Is there any possibility of \"loosening the fsck.gitmodulesUrl\n> > severity\", as Jeff suggested?\n> \n> Isn't the suggestion not about butchering the rest of the world but\n> by locally configuring fsck.gitmodulesUrl down from error to\n> warning? I personally think excluding a single known-offending blob\n> without doing such loosening is a much better idea in that it\n> prevents *new* offending instances from getting into the repository,\n> while allowing an existing benign and honest mistake to stay in your\n> history. Loosening the severity of a class of check means you will\n> accept *new* offending instances, which may very well be malicious,\n> unlike the existing benign one you know about.\n\nThe trouble with configuring fsck.gitmodulesUrl yourself (or using\nskipList, which I agree is better if you can do it) is that it only\nhelps your local repo, and not:\n\n 1. Hosting sites which may need special work-arounds to let you push up\n to them, since they are using receive.fsckObjects.\n\n In theory this is a good thing, because it prevents dumb mistakes\n from getting distributed in the first place. But it also is a pain\n for projects with established history.\n\n 2. All of the people who are going to clone your repo, who might need\n to follow special instructions.\n\n The only reason this hasn't been a huge pain in practice is that\n almost nobody turns on transfer.fsckObjects in the first place. In\n theory the people who do turn it on know enough to examine the\n objects themselves and decide if it's OK. I don't know how true that\n is in practice, though (and certainly it would be nice to turn this\n feature on by default, but I do worry about people getting caught up\n in exactly these kind of historical messes).\n\nWe did add the gitmoduleUrl check to help with malicious URLs. But it\nwas always an extra layer of defense over the real fix, which was in the\ncredential code. It's _possible_ that a newly discovered vulnerability\nwill be protected by the existing fsck check, but I'm a little skeptical\nabout its security value at this point (especially because hardly\nanybody runs it locally, and protection on the hosting sites isn't that\nhard to work around).\n\nSo if it's causing people real pain in practice, I think there could be\nan argument for downgrading the check to a warning. I don't have a\nstrong feeling that we _should_ do that, only that I don't personally\nreject it immediately as an option.\n\n-Peff\n"},{"id":"507241","messageId":"e039fc2b-ba2b-4845-9a2e-58edcc003d87@mayhew.name","threadId":"60708","inReplyTo":"xmqq5xoqahbx.fsf@gitster.g","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Neil Mayhew","fromEmail":"neil@mayhew.name","sentAt":"2024-11-14T00:27:36Z","receivedAt":"2024-11-14T00:27:39Z","isPatch":true,"sender":{"key":"neil@mayhew.name","avatar":"https://gravatar.com/avatar/8590e81106da7bc57a7d85d70e3814898ad4cdace681a8d48cac665c3fd51150?d=mp&s=160"},"body":"On 13 Nov 24 15:40, Junio C Hamano wrote:\n\n > Neil Mayhew writes:\n >\n >> ... immediately corrected by another commit. However, the bad commit is\n >> still in the history. It happened 6 years ago, so there's no\n >> possibility of us changing the history.\n >\n > I think fsck.skipList was meant to cover such a case.  The idea is\n > that the blob object name of the bad .gitmodules file can be placed\n > on the list, and the rest of the \"bad commit\" and the whole history\n > can still be checked for consistency, without triggering the warning\n > (or error) resulting from the offending .gitmodules file.\n\nThank you. This explanation is very helpful. I hadn't noticed the \nexistence of this config option, and using it does indeed enable fsck to \npass on our repo.\n\n >> Is there any possibility of \"loosening the fsck.gitmodulesUrl\n >> severity\", as Jeff suggested?\n >\n > Isn't the suggestion not about butchering the rest of the world but\n > by locally configuring fsck.gitmodulesUrl down from error to\n > warning?\n\nAgain, thank you for the helpful explanation. I hadn't fully understood \nwhat was being suggested.\n\n > I personally think excluding a single known-offending blob\n > without doing such loosening is a much better idea in that it\n > prevents *new* offending instances from getting into the repository,\n > while allowing an existing benign and honest mistake to stay in your\n > history.  Loosening the severity of a class of check means you will\n > accept *new* offending instances, which may very well be malicious,\n > unlike the existing benign one you know about.\n\nI agree that skipping a single object is better than loosening.\n\nHowever, there is a bit of a catch-22 situation here. The problem in our \nrepo was reported to us by someone who was unable to clone, because they \nhave transfer.fsckObjects set globally (and initially didn't realise \nthat was where the error message was coming from). Someone in that \nsituation could in addition set fetch.fsck.gitmodulesUrl=warn globally, \nbut as you say that's too all-encompassing and it would be better to \nskip the specific object. Unfortunately, this is difficult to do when \ncloning, because the configuration value doesn't come automatically with \nthe repo. A local skiplist configuration value can't exist until after \nthe repo has been cloned, and adding the object name to a global \nskiplist doesn't seem appropriate. It's possible to add the option to \nthe clone command line (with -c) but that's starting to get quite messy \nand in any case many people wouldn't see the instruction even if we put \nit in our README.\n\nI don't think there's a perfect solution here. It seems like the best we \ncan do is to put the object name in a .git-fsck-skiplist file at the top \nlevel of the repo, and add a section at the end of the README telling \npeople to configure it for all three *fsck.skipList values if they want \nto use git fsck explicitly, or implicitly via other configuration \nvalues. We would also need to mention using -c when cloning initially. \nThis information will probably be intimidating for most people, and I \nwish we didn't have to include it, but hopefully we can make it clear \nthat most people won't need it.\n"},{"id":"507242","messageId":"c2f97b19-19e6-485d-91c8-24c261aedebe@mayhew.name","threadId":"60708","inReplyTo":"20241114001003.GA1140565@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Neil Mayhew","fromEmail":"neil@mayhew.name","sentAt":"2024-11-14T00:51:46Z","receivedAt":"2024-11-14T00:51:49Z","isPatch":true,"sender":{"key":"neil@mayhew.name","avatar":"https://gravatar.com/avatar/8590e81106da7bc57a7d85d70e3814898ad4cdace681a8d48cac665c3fd51150?d=mp&s=160"},"body":"On 13 Nov 24 17:10, Jeff King wrote:\n\nMy previous message crossed with Jeff's, and he already addressed most \nof what I was saying.\n\n >  2. All of the people who are going to clone your repo, who might need\n >     to follow special instructions.\n >\n >     The only reason this hasn't been a huge pain in practice is that\n >     almost nobody turns on transfer.fsckObjects in the first place. In\n >     theory the people who do turn it on know enough to examine the\n >     objects themselves and decide if it's OK. I don't know how true that\n >     is in practice, though (and certainly it would be nice to turn this\n >     feature on by default, but I do worry about people getting caught up\n >     in exactly these kind of historical messes).\n\nThis is what happened in our situation. The person had \ntransfer.fsckObjects enabled but didn't realize that this was the cause \nof the error. They assumed that the repo's *current* submodule \nconfiguration was corrupt and was somehow causing the clone to fail even \nthough they tried explicitly turning off submodule recursion.\n\n > We did add the gitmoduleUrl check to help with malicious URLs. But it\n > was always an extra layer of defense over the real fix, which was in the\n > credential code. It's _possible_ that a newly discovered vulnerability\n > will be protected by the existing fsck check, but I'm a little skeptical\n > about its security value at this point (especially because hardly\n > anybody runs it locally, and protection on the hosting sites isn't that\n > hard to work around).\n\nI also think it's surprising to have fsck check the *content* of blobs \nrather than just the relationships between them, and to give a blob \nnamed .gitmodules special treatment. It goes against the philosophy of \n\"do one thing well\". I feel that there should be a separate tool for \nchecking repos for security vulnerabilities, and it could be given \nadditional capabilities (such as checking the configuration as well as \nthe objects).\n\n > So if it's causing people real pain in practice, I think there could be\n > an argument for downgrading the check to a warning. I don't have a\n > strong feeling that we _should_ do that, only that I don't personally\n > reject it immediately as an option.\n\nPerhaps there could be some additional warnings in the documentation for \ntransfer.fsckObjects to make people aware of the potential costs of \nusing it, particularly the existence of legacy issues in established \nrepos that would prevent cloning unless some of the fsck. values \nare set to warn. The documentation currently just says \"see \nfsck.\" and in my case, despite being fairly familiar with git, \nthat didn't give me enough to go on while investigating this.\n\nIt might also help to give some guidance on how to track down the object \nname(s) that fsck lists, for example by using git log --raw --all \n--find-object=. This would help a user to make a more informed \ndecision on how to handle such a situation if it arises. In our case, \nonce we did this it was quickly obvious that the problem was a \nhistorical error that had since been fixed rather than a current problem.\n"},{"id":"507249","messageId":"xmqqzfm27dze.fsf@gitster.g","threadId":"60708","inReplyTo":"20241114001003.GA1140565@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-14T02:20:37Z","receivedAt":"2024-11-14T02:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... I'm a little skeptical\n> about its security value at this point (especially because hardly\n> anybody runs it locally, and protection on the hosting sites isn't that\n> hard to work around).\n>\n> So if it's causing people real pain in practice, I think there could be\n> an argument for downgrading the check to a warning. I don't have a\n> strong feeling that we _should_ do that, only that I don't personally\n> reject it immediately as an option.\n\nOh, I see. I do not think I have strong objection, either.\n\nThanks.\n"},{"id":"507295","messageId":"9a19a763-e4b3-4e5b-b933-d50307aa0af2@mayhew.name","threadId":"60708","inReplyTo":"xmqqzfm27dze.fsf@gitster.g","subject":"Re: [PATCH 0/3] Strengthen fsck checks for submodule URLs","fromName":"Neil Mayhew","fromEmail":"neil@mayhew.name","sentAt":"2024-11-14T19:11:31Z","receivedAt":"2024-11-14T19:11:34Z","isPatch":true,"sender":{"key":"neil@mayhew.name","avatar":"https://gravatar.com/avatar/8590e81106da7bc57a7d85d70e3814898ad4cdace681a8d48cac665c3fd51150?d=mp&s=160"},"body":"On 13 Nov 24 19:20, Junio C Hamano wrote:\n\n > Jeff King writes:\n >\n >> So if it's causing people real pain in practice, I think there could be\n >> an argument for downgrading the check to a warning. I don't have a\n >> strong feeling that we _should_ do that, only that I don't personally\n >> reject it immediately as an option.\n >\n > Oh, I see.  I do not think I have strong objection, either.\n\nSo if I was to submit a patch making this downgrade, is there a good \nchance it would be accepted?\n"}]}