{"thread":{"id":"61816","subject":"[PATCH 0/2] safe.directory clean-up","startedAt":"2024-07-20T22:09:23Z","lastAt":"2024-08-15T14:43:08Z","messageCount":39,"participants":["Junio C Hamano","Phillip Wood","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"499010","messageId":"20240720220915.2933266-1-gitster@pobox.com","threadId":"61816","inReplyTo":null,"subject":"[PATCH 0/2] safe.directory clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-20T22:09:11Z","receivedAt":"2024-07-20T22:09:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Recently we discussed what we should do when either the path\nconfigured in the safe.directory configuration or coming from\nthe caller of ensure_valid_ownership() function as a result of\nrepository discovery is not normalized and textual equality check is\nnot sufficient. See the thread the contains\n\n https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/\n\nHere are two patches (yes, two) that implements the comparison\nbetween normalized path and configuration value.\n\nImagine that you have a repository at /mnt/disk4/repos/frotz\ndirectory but in order to make it simpler to manage and use, you\nhave your users use /projects/frotz to access the repository. A\nsymlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory\nallows you to do so.\n\n - The first patch normalizes the path to the directory that we\n suspect is a usable repository, before comparing it with the\n safe.directory configuration variable. The safe.directory may\n say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to\n the repository for the users may be /mnt/disk4/repos/frotz or\n /projects/frotz, depending on where they come from and what their\n $PWD makes getcwd() to say.\n\n - The second patch normalizes the value of the safe.directory\n variable. This allows safe.directory to say /projects/frotz\n or /projects/* and have them match /mnt/disk4/repos/frotz (which\n is how the first patch normalizes the repository path to).\n\n - The third patch is a preliminary clean-up that would be needed if\n we wanted to use the fourth patch.\n\n - The fourth patch would become relevant if we were to call\n ensure_valid_ownership() on many directories in a single process.\n We grab safe.directory values and normalize them just once before\n using in ensure_valid_ownership() to optimize away repeated\n normalization.\n\nIt turns out that nobody calls ensure_valid_ownership() on many\ndifferent directories even in the repository discovery loop, which\nmeans the fourth patch is not needed, which in turn means the third\npatch that is a preliminary clean-up is also not necessary.\n\nJunio C Hamano (4):\n safe.directory: normalize the checked path\n safe.directory: normalize the configured path\n setup: allow centralized clean-up when leaving setup_git_directory_gently_1()\n setup: cache normalized safe.directory configuration\n\n setup.c | 141 +++++++++++++++++++++++++++++++-------\n t/t0033-safe-directory.sh | 90 ++++++++++++++++++++++++\n 2 files changed, 205 insertions(+), 26 deletions(-)\n\n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499011","messageId":"20240720220915.2933266-2-gitster@pobox.com","threadId":"61816","inReplyTo":"20240720220915.2933266-1-gitster@pobox.com","subject":"[PATCH 1/2] safe.directory: normalize the checked path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-20T22:09:12Z","receivedAt":"2024-07-20T22:09:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\".\n\nA request to check if /home/u/repo is safe would be rejected if the\nsafe.directory configuration allows /home/u/repository/ but not its\nalias /home/u/repo/. Normalize the path being checked before\ncomparing with safe.directory value(s).\n\nSuggested-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n setup.c | 16 ++++++++++----\n t/t0033-safe-directory.sh | 45 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex d458edcc02..45bbbe329f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n }\n \n struct safe_directory_data {\n-\tconst char *path;\n+\tchar *path;\n \tint is_safe;\n };\n \n@@ -1263,9 +1263,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t\t\t\t const char *worktree, const char *gitdir,\n \t\t\t\t struct strbuf *report)\n {\n-\tstruct safe_directory_data data = {\n-\t\t.path = worktree ? worktree : gitdir\n-\t};\n+\tstruct safe_directory_data data = { 0 };\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n \t (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&\n@@ -1273,6 +1271,15 @@ static int ensure_valid_ownership(const char *gitfile,\n \t (!gitdir || is_path_owned_by_current_user(gitdir, report)))\n \t\treturn 1;\n \n+\t/*\n+\t * normalize the data.path for comparison with normalized paths\n+\t * that come from the configuration file. The path is unsafe\n+\t * if it cannot be normalized.\n+\t */\n+\tdata.path = real_pathdup(worktree ? worktree : gitdir, 0);\n+\tif (!data.path)\n+\t\treturn 0;\n+\n \t/*\n \t * data.path is the \"path\" that identifies the repository and it is\n \t * constant regardless of what failed above. data.is_safe should be\n@@ -1280,6 +1287,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t */\n \tgit_protected_config(safe_directory_cb, &data);\n \n+\tfree(data.path);\n \treturn data.is_safe;\n }\n \ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 5fe61f1291..3e487e7f4b 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -119,4 +119,49 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '\n \ttest_path_is_dir target\n '\n \n+test_expect_success SYMLINKS 'checked paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository\"\n+\t) &&\n+\tgit -C repository/ for-each-ref >/dev/null &&\n+\tgit -C repo/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'checked leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref >/dev/null &&\n+\tgit -C repo/s for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499012","messageId":"20240720220915.2933266-3-gitster@pobox.com","threadId":"61816","inReplyTo":"20240720220915.2933266-1-gitster@pobox.com","subject":"[PATCH 2/2] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-20T22:09:13Z","receivedAt":"2024-07-20T22:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\"\n\nA request to check if /home/u/repository is safe would be rejected\nif the safe.directory configuration allows /home/u/repo/ but not its\nalias /home/u/repository/. Normalize the paths configured for the\nsafe.directory configuration variable before comparing them with the\npath being checked.\n\nSuggested-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n setup.c | 12 +++++++++++\n t/t0033-safe-directory.sh | 45 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 45bbbe329f..29304d7452 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1236,6 +1236,16 @@ static int safe_directory_cb(const char *key, const char *value,\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n \t\t\tconst char *check = allowed ? allowed : value;\n+\t\t\tchar *to_free = real_pathdup(check, 0);\n+\n+\t\t\tif (!to_free) {\n+\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n+\t\t\t\t\tcheck);\n+\t\t\t\tgoto next;\n+\t\t\t} else {\n+\t\t\t\tcheck = to_free;\n+\t\t\t}\n+\n \t\t\tif (ends_with(check, \"/*\")) {\n \t\t\t\tsize_t len = strlen(check);\n \t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n@@ -1243,7 +1253,9 @@ static int safe_directory_cb(const char *key, const char *value,\n \t\t\t} else if (!fspathcmp(data->path, check)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n+\t\t\tfree(to_free);\n \t\t}\n+\tnext:\n \t\tif (allowed != value)\n \t\t\tfree(allowed);\n \t}\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 3e487e7f4b..17d45d481e 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -164,4 +164,49 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '\n \tgit -C repo/s for-each-ref\n '\n \n+test_expect_success SYMLINKS 'configured paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo\"\n+\t) &&\n+\tgit -C repository/ for-each-ref >/dev/null &&\n+\tgit -C repo/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'configured leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref >/dev/null &&\n+\tgit -C repo/s for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499013","messageId":"20240720220915.2933266-4-gitster@pobox.com","threadId":"61816","inReplyTo":"20240720220915.2933266-1-gitster@pobox.com","subject":"[PATCH 3/2] setup: use a single return path in setup_git_directory*()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-20T22:09:14Z","receivedAt":"2024-07-20T22:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[Do not use. For illustration purposes only]\n\nInstead of sprinkling \"return\" all over the place, use the \"assign\nto the result variable and then jump to the single label set up to\nleave the function\" pattern, so that we can clean up any extra\nresource allocated before returning at a single place.\n\nNo functional change is intended with this step, but it will be used\nsoon.\n\nSigned-off-by: Junio C Hamano \n---\n setup.c | 50 ++++++++++++++++++++++++++++++++------------------\n 1 file changed, 32 insertions(+), 18 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 29304d7452..29e23a905c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1416,6 +1416,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \tint ceil_offset = -1, min_offset = offset_1st_component(dir->buf);\n \tdev_t current_device = 0;\n \tint one_filesystem = 1;\n+\tenum discovery_result result;\n \n \t/*\n \t * If GIT_DIR is set explicitly, we're not going\n@@ -1425,7 +1426,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \tgitdirenv = getenv(GIT_DIR_ENVIRONMENT);\n \tif (gitdirenv) {\n \t\tstrbuf_addstr(gitdir, gitdirenv);\n-\t\treturn GIT_DIR_EXPLICIT;\n+\t\tresult = GIT_DIR_EXPLICIT;\n+\t\tgoto cleanup_and_return;\n \t}\n \n \tif (env_ceiling_dirs) {\n@@ -1479,8 +1481,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\tgitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;\n \t\t\t\t\tgitdir_path = xstrdup(dir->buf);\n \t\t\t\t}\n-\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)\n-\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {\n+\t\t\t\tresult = GIT_DIR_INVALID_GITFILE;\n+\t\t\t\tgoto cleanup_and_return;\n+\t\t\t}\n \t\t} else\n \t\t\tgitfile = xstrdup(dir->buf);\n \t\t/*\n@@ -1491,16 +1495,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t */\n \t\tstrbuf_setlen(dir, offset);\n \t\tif (gitdirenv) {\n-\t\t\tenum discovery_result ret;\n \t\t\tconst char *gitdir_candidate =\n \t\t\t\tgitdir_path ? gitdir_path : gitdirenv;\n \n \t\t\tif (ensure_valid_ownership(gitfile, dir->buf,\n \t\t\t\t\t\t gitdir_candidate, report)) {\n \t\t\t\tstrbuf_addstr(gitdir, gitdirenv);\n-\t\t\t\tret = GIT_DIR_DISCOVERED;\n+\t\t\t\tresult = GIT_DIR_DISCOVERED;\n \t\t\t} else\n-\t\t\t\tret = GIT_DIR_INVALID_OWNERSHIP;\n+\t\t\t\tresult = GIT_DIR_INVALID_OWNERSHIP;\n \n \t\t\t/*\n \t\t\t * Earlier, during discovery, we might have allocated\n@@ -1514,8 +1517,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t */\n \t\t\tfree(gitdir_path);\n \t\t\tfree(gitfile);\n-\n-\t\t\treturn ret;\n+\t\t\tgoto cleanup_and_return;\n \t\t}\n \n \t\tif (is_git_directory(dir->buf)) {\n@@ -1523,25 +1525,37 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n \t\t\t !is_implicit_bare_repo(dir->buf))\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n-\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n-\t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n-\t\t\tstrbuf_addstr(gitdir, \".\");\n-\t\t\treturn GIT_DIR_BARE;\n+\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) {\n+\t\t\t\tresult = GIT_DIR_INVALID_OWNERSHIP;\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addstr(gitdir, \".\");\n+\t\t\t\tresult = GIT_DIR_BARE;\n+\t\t\t}\n+\t\t\tgoto cleanup_and_return;\n \t\t}\n \n-\t\tif (offset <= min_offset)\n-\t\t\treturn GIT_DIR_HIT_CEILING;\n+\t\tif (offset <= min_offset) {\n+\t\t\tresult = GIT_DIR_HIT_CEILING;\n+\t\t\tgoto cleanup_and_return;\n+\t\t}\n \n \t\twhile (--offset > ceil_offset && !is_dir_sep(dir->buf[offset]))\n \t\t\t; /* continue */\n-\t\tif (offset <= ceil_offset)\n-\t\t\treturn GIT_DIR_HIT_CEILING;\n+\t\tif (offset <= ceil_offset) {\n+\t\t\tresult = GIT_DIR_HIT_CEILING;\n+\t\t\tgoto cleanup_and_return;\n+\t\t}\n \n \t\tstrbuf_setlen(dir, offset > min_offset ? offset : min_offset);\n \t\tif (one_filesystem &&\n-\t\t current_device != get_device_or_die(dir->buf, NULL, offset))\n-\t\t\treturn GIT_DIR_HIT_MOUNT_POINT;\n+\t\t current_device != get_device_or_die(dir->buf, NULL, offset)) {\n+\t\t\tresult = GIT_DIR_HIT_MOUNT_POINT;\n+\t\t\tgoto cleanup_and_return;\n+\t\t}\n \t}\n+\n+cleanup_and_return:\n+\treturn result;\n }\n \n enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499014","messageId":"20240720220915.2933266-5-gitster@pobox.com","threadId":"61816","inReplyTo":"20240720220915.2933266-1-gitster@pobox.com","subject":"[PATCH 4/2] setup: cache normalized safe.directory configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-20T22:09:15Z","receivedAt":"2024-07-20T22:09:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[Do not use. For illustration purposes only]\n\nThe current check performed in ensure_valid_ownership() reads each\nsafe.directory configuration item and normalizes it before checking\nagainst the path to the repository.\n\nThis is OK as long as we are checking just a single directory, like\nin die_upon_dubious_ownership() and setup_git_directory_gently_1().\nBut let's pretend that the latter calls ensure_valid_ownership()\nmany times in the loop, and demonstrate how we would avoid having to\nnormalize the same safe.directory configuration item over and over.\n\nSigned-off-by: Junio C Hamano \n---\n setup.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 67 insertions(+), 12 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 29e23a905c..75bcce0368 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -20,6 +20,7 @@\n #include \"trace2.h\"\n #include \"worktree.h\"\n #include \"exec-cmd.h\"\n+#include \"strvec.h\"\n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n@@ -1217,6 +1218,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n struct safe_directory_data {\n \tchar *path;\n \tint is_safe;\n+\tint prenormalized;\n };\n \n static int safe_directory_cb(const char *key, const char *value,\n@@ -1236,14 +1238,20 @@ static int safe_directory_cb(const char *key, const char *value,\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n \t\t\tconst char *check = allowed ? allowed : value;\n-\t\t\tchar *to_free = real_pathdup(check, 0);\n-\n-\t\t\tif (!to_free) {\n-\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n-\t\t\t\t\tcheck);\n-\t\t\t\tgoto next;\n-\t\t\t} else {\n+\t\t\tchar *to_free = NULL;\n+\n+\t\t\tif (!data->prenormalized) {\n+\t\t\t\tto_free = real_pathdup(check, 0);\n+\t\t\t\tif (!to_free) {\n+\t\t\t\t\twarning(_(\"safe.directory '%s' \"\n+\t\t\t\t\t\t \"cannot be normalized\"),\n+\t\t\t\t\t\tcheck);\n+\t\t\t\t\tgoto next;\n+\t\t\t\t}\n \t\t\t\tcheck = to_free;\n+\t\t\t} else {\n+\t\t\t\tto_free = NULL;\n+\t\t\t\tcheck = value;\n \t\t\t}\n \n \t\t\tif (ends_with(check, \"/*\")) {\n@@ -1263,6 +1271,39 @@ static int safe_directory_cb(const char *key, const char *value,\n \treturn 0;\n }\n \n+static int prenorm_cb(const char *key, const char *value,\n+\t\t const struct config_context *ctx UNUSED, void *v_)\n+{\n+\tstruct strvec *v = v_;\n+\n+\tif (strcmp(key, \"safe.directory\"))\n+\t\treturn 0;\n+\tif (!value || !*value) {\n+\t\tstrvec_clear(v);\n+\t} else if (!strcmp(value, \"*\")) {\n+\t\tstrvec_push(v, value);\n+\t} else {\n+\t\tchar *allowed = NULL;\n+\t\tif (!git_config_pathname(&allowed, key, value)) {\n+\t\t\tconst char *ccheck = allowed ? allowed : value;\n+\t\t\tchar *check = real_pathdup(ccheck, 0);\n+\t\t\tif (check)\n+\t\t\t\tstrvec_push_nodup(v, check);\n+\t\t\telse\n+\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n+\t\t\t\t\tccheck);\n+\t\t}\n+\t\tif (allowed != value)\n+\t\t\tfree(allowed);\n+\t}\n+\treturn 0;\n+}\n+\n+static void prenormalize_safe_directory(struct strvec *v)\n+{\n+\tgit_protected_config(prenorm_cb, v);\n+}\n+\n /*\n * Check if a repository is safe, by verifying the ownership of the\n * worktree (if any), the git directory, and the gitfile (if any).\n@@ -1273,6 +1314,7 @@ static int safe_directory_cb(const char *key, const char *value,\n */\n static int ensure_valid_ownership(const char *gitfile,\n \t\t\t\t const char *worktree, const char *gitdir,\n+\t\t\t\t struct strvec *safe_cache,\n \t\t\t\t struct strbuf *report)\n {\n \tstruct safe_directory_data data = { 0 };\n@@ -1297,8 +1339,15 @@ static int ensure_valid_ownership(const char *gitfile,\n \t * constant regardless of what failed above. data.is_safe should be\n \t * initialized to false, and might be changed by the callback.\n \t */\n-\tgit_protected_config(safe_directory_cb, &data);\n-\n+\tif (!safe_cache) {\n+\t\tgit_protected_config(safe_directory_cb, &data);\n+\t} else {\n+\t\tdata.prenormalized = 1;\n+\t\tfor (size_t i = 0; i < safe_cache->nr; i++) {\n+\t\t\tsafe_directory_cb(\"safe.directory\", safe_cache->v[i],\n+\t\t\t\t\t NULL, &data);\n+\t\t}\n+\t}\n \tfree(data.path);\n \treturn data.is_safe;\n }\n@@ -1309,7 +1358,7 @@ void die_upon_dubious_ownership(const char *gitfile, const char *worktree,\n \tstruct strbuf report = STRBUF_INIT, quoted = STRBUF_INIT;\n \tconst char *path;\n \n-\tif (ensure_valid_ownership(gitfile, worktree, gitdir, &report))\n+\tif (ensure_valid_ownership(gitfile, worktree, gitdir, NULL, &report))\n \t\treturn;\n \n \tstrbuf_complete(&report, '\\n');\n@@ -1416,6 +1465,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \tint ceil_offset = -1, min_offset = offset_1st_component(dir->buf);\n \tdev_t current_device = 0;\n \tint one_filesystem = 1;\n+\tstruct strvec safe_cache = STRVEC_INIT;\n \tenum discovery_result result;\n \n \t/*\n@@ -1463,6 +1513,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \tone_filesystem = !git_env_bool(\"GIT_DISCOVERY_ACROSS_FILESYSTEM\", 0);\n \tif (one_filesystem)\n \t\tcurrent_device = get_device_or_die(dir->buf, NULL, 0);\n+\n+\tprenormalize_safe_directory(&safe_cache);\n \tfor (;;) {\n \t\tint offset = dir->len, error_code = 0;\n \t\tchar *gitdir_path = NULL;\n@@ -1499,7 +1551,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\tgitdir_path ? gitdir_path : gitdirenv;\n \n \t\t\tif (ensure_valid_ownership(gitfile, dir->buf,\n-\t\t\t\t\t\t gitdir_candidate, report)) {\n+\t\t\t\t\t\t gitdir_candidate,\n+\t\t\t\t\t\t &safe_cache, report)) {\n \t\t\t\tstrbuf_addstr(gitdir, gitdirenv);\n \t\t\t\tresult = GIT_DIR_DISCOVERED;\n \t\t\t} else\n@@ -1525,7 +1578,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n \t\t\t !is_implicit_bare_repo(dir->buf))\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n-\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) {\n+\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf,\n+\t\t\t\t\t\t &safe_cache, report)) {\n \t\t\t\tresult = GIT_DIR_INVALID_OWNERSHIP;\n \t\t\t} else {\n \t\t\t\tstrbuf_addstr(gitdir, \".\");\n@@ -1555,6 +1609,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t}\n \n cleanup_and_return:\n+\tstrvec_clear(&safe_cache);\n \treturn result;\n }\n \n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499115","messageId":"20240723021900.388020-1-gitster@pobox.com","threadId":"61816","inReplyTo":"20240720220915.2933266-1-gitster@pobox.com","subject":"[PATCH v2 0/3] safe.directory clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T02:18:57Z","receivedAt":"2024-07-23T02:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Changes since v1:\n\n - two \"extra\" patches for optimization that are unnecessary have\n been dropped.\n - a patch to add test for safe.directory=\".\" use case has been\n added. \n\nRecently we discussed what we should do when either the path\nconfigured in the safe.directory configuration or coming from\nthe caller of ensure_valid_ownership() function as a result of\nrepository discovery is not normalized and textual equality check is\nnot sufficient. See the thread the contains\n\n https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/\n\nHere are three patches that implement the comparison between\nnormalized path and configuration value.\n\nImagine that you have a repository at /mnt/disk4/repos/frotz\ndirectory but in order to make it simpler to manage and use, you\nhave your users use /projects/frotz to access the repository. A\nsymlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory\nallows you to do so.\n\n - The first patch normalizes the path to the directory that we\n suspect is a usable repository, before comparing it with the\n safe.directory configuration variable. The safe.directory may\n say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to\n the repository for the users may be /mnt/disk4/repos/frotz or\n /projects/frotz, depending on where they come from and what their\n $PWD makes getcwd() to say.\n\n - The second patch normalizes the value of the safe.directory\n variable. This allows safe.directory to say /projects/frotz\n or /projects/* and have them match /mnt/disk4/repos/frotz (which\n is how the first patch normalizes the repository path to).\n\n - The third patch only adds a test to illustrate what happens when\n the safe.directory configuration is set to \".\", which was a\n plausible setting for those who run \"git daemon\". The\n normalization done by the first two patches is sufficient to make\n this just work.\n\nJunio C Hamano (3):\n safe.directory: normalize the checked path\n safe.directory: normalize the configured path\n safe.directory: setting safe.directory=\".\" allows the \"current\"\n directory\n\n setup.c | 28 ++++++--\n t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 170 insertions(+), 4 deletions(-)\n\nRange-diff against v1:\n1: 20e1160946 ! 1: 86d5c83ee7 safe.directory: normalize the checked path\n @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce\n +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n +\t\tgit config --global safe.directory \"$(pwd)/repository\"\n +\t) &&\n -+\tgit -C repository/ for-each-ref >/dev/null &&\n -+\tgit -C repo/ for-each-ref\n ++\tgit -C repository for-each-ref &&\n ++\tgit -C repository/ for-each-ref &&\n ++\tgit -C repo for-each-ref &&\n ++\tgit -C repo/ for-each-ref &&\n ++\ttest_must_fail git -C repository/.git for-each-ref &&\n ++\ttest_must_fail git -C repository/.git/ for-each-ref &&\n ++\ttest_must_fail git -C repo/.git for-each-ref &&\n ++\ttest_must_fail git -C repo/.git/ for-each-ref\n +'\n +\n +test_expect_success SYMLINKS 'checked leading paths are normalized' '\n @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce\n +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n +\t\tgit config --global safe.directory \"$(pwd)/repository/*\"\n +\t) &&\n -+\tgit -C repository/s for-each-ref >/dev/null &&\n -+\tgit -C repo/s for-each-ref\n ++\tgit -C repository/s for-each-ref &&\n ++\tgit -C repository/s/ for-each-ref &&\n ++\tgit -C repo/s for-each-ref &&\n ++\tgit -C repo/s/ for-each-ref &&\n ++\tgit -C repository/s/.git for-each-ref &&\n ++\tgit -C repository/s/.git/ for-each-ref &&\n ++\tgit -C repo/s/.git for-each-ref &&\n ++\tgit -C repo/s/.git/ for-each-ref\n +'\n +\n test_done\n2: 06de9038b7 ! 2: c4998076da safe.directory: normalize the configured path\n @@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n \n ## t/t0033-safe-directory.sh ##\n @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' '\n - \tgit -C repo/s for-each-ref\n + \tgit -C repo/s/.git/ for-each-ref\n '\n \n +test_expect_success SYMLINKS 'configured paths are normalized' '\n @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a\n +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n +\t\tgit config --global safe.directory \"$(pwd)/repo\"\n +\t) &&\n -+\tgit -C repository/ for-each-ref >/dev/null &&\n -+\tgit -C repo/ for-each-ref\n ++\tgit -C repository for-each-ref &&\n ++\tgit -C repository/ for-each-ref &&\n ++\tgit -C repo for-each-ref &&\n ++\tgit -C repo/ for-each-ref &&\n ++\ttest_must_fail git -C repository/.git for-each-ref &&\n ++\ttest_must_fail git -C repository/.git/ for-each-ref &&\n ++\ttest_must_fail git -C repo/.git for-each-ref &&\n ++\ttest_must_fail git -C repo/.git/ for-each-ref\n +'\n +\n +test_expect_success SYMLINKS 'configured leading paths are normalized' '\n @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a\n +\tgit init repository/s &&\n +\tln -s repository repo &&\n +\t(\n -+\t\tcd repository &&\n ++\t\tcd repository/s &&\n +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n +\t\ttest_commit sample\n +\t) &&\n @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a\n +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n +\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n +\t) &&\n -+\tgit -C repository/s for-each-ref >/dev/null &&\n -+\tgit -C repo/s for-each-ref\n ++\tgit -C repository/s for-each-ref &&\n ++\tgit -C repository/s/ for-each-ref &&\n ++\tgit -C repository/s/.git for-each-ref &&\n ++\tgit -C repository/s/.git/ for-each-ref &&\n ++\tgit -C repo/s for-each-ref &&\n ++\tgit -C repo/s/ for-each-ref &&\n ++\tgit -C repo/s/.git for-each-ref &&\n ++\tgit -C repo/s/.git/ for-each-ref\n +'\n +\n test_done\n-: ---------- > 3: ffc3f7364e safe.directory: setting safe.directory=\".\" allows the \"current\" directory\n-- \n2.46.0-rc1-52-gda884b23f2\n\n"},{"id":"499116","messageId":"20240723021900.388020-2-gitster@pobox.com","threadId":"61816","inReplyTo":"20240723021900.388020-1-gitster@pobox.com","subject":"[PATCH v2 1/3] safe.directory: normalize the checked path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T02:18:58Z","receivedAt":"2024-07-23T02:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\".\n\nA request to check if /home/u/repo is safe would be rejected if the\nsafe.directory configuration allows /home/u/repository/ but not its\nalias /home/u/repo/. Normalize the path being checked before\ncomparing with safe.directory value(s).\n\nSuggested-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n setup.c | 16 ++++++++---\n t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex d458edcc02..45bbbe329f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n }\n \n struct safe_directory_data {\n-\tconst char *path;\n+\tchar *path;\n \tint is_safe;\n };\n \n@@ -1263,9 +1263,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t\t\t\t const char *worktree, const char *gitdir,\n \t\t\t\t struct strbuf *report)\n {\n-\tstruct safe_directory_data data = {\n-\t\t.path = worktree ? worktree : gitdir\n-\t};\n+\tstruct safe_directory_data data = { 0 };\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n \t (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&\n@@ -1273,6 +1271,15 @@ static int ensure_valid_ownership(const char *gitfile,\n \t (!gitdir || is_path_owned_by_current_user(gitdir, report)))\n \t\treturn 1;\n \n+\t/*\n+\t * normalize the data.path for comparison with normalized paths\n+\t * that come from the configuration file. The path is unsafe\n+\t * if it cannot be normalized.\n+\t */\n+\tdata.path = real_pathdup(worktree ? worktree : gitdir, 0);\n+\tif (!data.path)\n+\t\treturn 0;\n+\n \t/*\n \t * data.path is the \"path\" that identifies the repository and it is\n \t * constant regardless of what failed above. data.is_safe should be\n@@ -1280,6 +1287,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t */\n \tgit_protected_config(safe_directory_cb, &data);\n \n+\tfree(data.path);\n \treturn data.is_safe;\n }\n \ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 5fe61f1291..07ac0f9a01 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -119,4 +119,61 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '\n \ttest_path_is_dir target\n '\n \n+test_expect_success SYMLINKS 'checked paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repo for-each-ref &&\n+\tgit -C repo/ for-each-ref &&\n+\ttest_must_fail git -C repository/.git for-each-ref &&\n+\ttest_must_fail git -C repository/.git/ for-each-ref &&\n+\ttest_must_fail git -C repo/.git for-each-ref &&\n+\ttest_must_fail git -C repo/.git/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'checked leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref &&\n+\tgit -C repository/s/ for-each-ref &&\n+\tgit -C repo/s for-each-ref &&\n+\tgit -C repo/s/ for-each-ref &&\n+\tgit -C repository/s/.git for-each-ref &&\n+\tgit -C repository/s/.git/ for-each-ref &&\n+\tgit -C repo/s/.git for-each-ref &&\n+\tgit -C repo/s/.git/ for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-rc1-52-gda884b23f2\n\n"},{"id":"499117","messageId":"20240723021900.388020-3-gitster@pobox.com","threadId":"61816","inReplyTo":"20240723021900.388020-1-gitster@pobox.com","subject":"[PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T02:18:59Z","receivedAt":"2024-07-23T02:19:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\"\n\nA request to check if /home/u/repository is safe would be rejected\nif the safe.directory configuration allows /home/u/repo/ but not its\nalias /home/u/repository/. Normalize the paths configured for the\nsafe.directory configuration variable before comparing them with the\npath being checked.\n\nSuggested-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n setup.c | 12 +++++++++\n t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 45bbbe329f..29304d7452 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1236,6 +1236,16 @@ static int safe_directory_cb(const char *key, const char *value,\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n \t\t\tconst char *check = allowed ? allowed : value;\n+\t\t\tchar *to_free = real_pathdup(check, 0);\n+\n+\t\t\tif (!to_free) {\n+\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n+\t\t\t\t\tcheck);\n+\t\t\t\tgoto next;\n+\t\t\t} else {\n+\t\t\t\tcheck = to_free;\n+\t\t\t}\n+\n \t\t\tif (ends_with(check, \"/*\")) {\n \t\t\t\tsize_t len = strlen(check);\n \t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n@@ -1243,7 +1253,9 @@ static int safe_directory_cb(const char *key, const char *value,\n \t\t\t} else if (!fspathcmp(data->path, check)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n+\t\t\tfree(to_free);\n \t\t}\n+\tnext:\n \t\tif (allowed != value)\n \t\t\tfree(allowed);\n \t}\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 07ac0f9a01..ea74657255 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '\n \tgit -C repo/s/.git/ for-each-ref\n '\n \n+test_expect_success SYMLINKS 'configured paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repo for-each-ref &&\n+\tgit -C repo/ for-each-ref &&\n+\ttest_must_fail git -C repository/.git for-each-ref &&\n+\ttest_must_fail git -C repository/.git/ for-each-ref &&\n+\ttest_must_fail git -C repo/.git for-each-ref &&\n+\ttest_must_fail git -C repo/.git/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'configured leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref &&\n+\tgit -C repository/s/ for-each-ref &&\n+\tgit -C repository/s/.git for-each-ref &&\n+\tgit -C repository/s/.git/ for-each-ref &&\n+\tgit -C repo/s for-each-ref &&\n+\tgit -C repo/s/ for-each-ref &&\n+\tgit -C repo/s/.git for-each-ref &&\n+\tgit -C repo/s/.git/ for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-rc1-52-gda884b23f2\n\n"},{"id":"499118","messageId":"20240723021900.388020-4-gitster@pobox.com","threadId":"61816","inReplyTo":"20240723021900.388020-1-gitster@pobox.com","subject":"[PATCH v2 3/3] safe.directory: setting safe.directory=\".\" allows the \"current\" directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T02:19:00Z","receivedAt":"2024-07-23T02:19:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"git daemon\" enters a repository, it chdir's to the requested\nrepository and then uses \".\" (the curent directory) to consult the\n\"is this repository considered safe?\" when it is not owned by the\nsame owner as the process.\n\nMake sure this access will be allowed by setting safe.directory to\n\".\".\n\nSigned-off-by: Junio C Hamano \n---\n t/t0033-safe-directory.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex ea74657255..1eeb794194 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -233,4 +233,36 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '\n \tgit -C repo/s/.git/ for-each-ref\n '\n \n+test_expect_success 'safe.directory set to a dot' '\n+\ttest_when_finished \"rm -rf repository\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository/subdir &&\n+\tgit init repository &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \".\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repository/.git for-each-ref &&\n+\tgit -C repository/.git/ for-each-ref &&\n+\n+\t# what is allowed is repository/subdir but the repository\n+\t# path is repository.\n+\ttest_must_fail git -C repository/subdir for-each-ref &&\n+\n+\t# likewise, repository .git/refs is allowed with \".\" but\n+\t# repository/.git that is accessed is not allowed.\n+\ttest_must_fail git -C repository/.git/refs for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-rc1-52-gda884b23f2\n\n"},{"id":"499326","messageId":"5332f244-7476-492a-a797-2ef7ba73f490@gmail.com","threadId":"61816","inReplyTo":"20240723021900.388020-3-gitster@pobox.com","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-25T09:45:37Z","receivedAt":"2024-07-25T09:45:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 23/07/2024 03:18, Junio C Hamano wrote:\n> The pathname of a repository comes from getcwd() and it could be a\n> path aliased via symbolic links, e.g., the real directory may be\n> /home/u/repository but a symbolic link /home/u/repo may point at it,\n> and the clone request may come as \"git clone file:///home/u/repo/\"\n> \n> A request to check if /home/u/repository is safe would be rejected\n> if the safe.directory configuration allows /home/u/repo/ but not its\n> alias /home/u/repository/. Normalize the paths configured for the\n> safe.directory configuration variable before comparing them with the\n> path being checked.\n\nI think this is sensible if the key is an absolute path, if the key is a \nrelative path I think we should ignore it as it is not clear which \ndirectory the user meant.\n\nBest Wishes\n\nPhillip\n\n> Suggested-by: Phillip Wood \n> Signed-off-by: Junio C Hamano \n> ---\n> setup.c | 12 +++++++++\n> t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 69 insertions(+)\n> \n> diff --git a/setup.c b/setup.c\n> index 45bbbe329f..29304d7452 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1236,6 +1236,16 @@ static int safe_directory_cb(const char *key, const char *value,\n> \n> \t\tif (!git_config_pathname(&allowed, key, value)) {\n> \t\t\tconst char *check = allowed ? allowed : value;\n> +\t\t\tchar *to_free = real_pathdup(check, 0);\n> +\n> +\t\t\tif (!to_free) {\n> +\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n> +\t\t\t\t\tcheck);\n> +\t\t\t\tgoto next;\n> +\t\t\t} else {\n> +\t\t\t\tcheck = to_free;\n> +\t\t\t}\n> +\n> \t\t\tif (ends_with(check, \"/*\")) {\n> \t\t\t\tsize_t len = strlen(check);\n> \t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n> @@ -1243,7 +1253,9 @@ static int safe_directory_cb(const char *key, const char *value,\n> \t\t\t} else if (!fspathcmp(data->path, check)) {\n> \t\t\t\tdata->is_safe = 1;\n> \t\t\t}\n> +\t\t\tfree(to_free);\n> \t\t}\n> +\tnext:\n> \t\tif (allowed != value)\n> \t\t\tfree(allowed);\n> \t}\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index 07ac0f9a01..ea74657255 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '\n> \tgit -C repo/s/.git/ for-each-ref\n> '\n> \n> +test_expect_success SYMLINKS 'configured paths are normalized' '\n> +\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n> +\t(\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global --unset-all safe.directory\n> +\t) &&\n> +\tgit init repository &&\n> +\tln -s repository repo &&\n> +\t(\n> +\t\tcd repository &&\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\ttest_commit sample\n> +\t) &&\n> +\n> +\t(\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \"$(pwd)/repo\"\n> +\t) &&\n> +\tgit -C repository for-each-ref &&\n> +\tgit -C repository/ for-each-ref &&\n> +\tgit -C repo for-each-ref &&\n> +\tgit -C repo/ for-each-ref &&\n> +\ttest_must_fail git -C repository/.git for-each-ref &&\n> +\ttest_must_fail git -C repository/.git/ for-each-ref &&\n> +\ttest_must_fail git -C repo/.git for-each-ref &&\n> +\ttest_must_fail git -C repo/.git/ for-each-ref\n> +'\n> +\n> +test_expect_success SYMLINKS 'configured leading paths are normalized' '\n> +\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n> +\t(\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global --unset-all safe.directory\n> +\t) &&\n> +\tmkdir -p repository &&\n> +\tgit init repository/s &&\n> +\tln -s repository repo &&\n> +\t(\n> +\t\tcd repository/s &&\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\ttest_commit sample\n> +\t) &&\n> +\n> +\t(\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n> +\t) &&\n> +\tgit -C repository/s for-each-ref &&\n> +\tgit -C repository/s/ for-each-ref &&\n> +\tgit -C repository/s/.git for-each-ref &&\n> +\tgit -C repository/s/.git/ for-each-ref &&\n> +\tgit -C repo/s for-each-ref &&\n> +\tgit -C repo/s/ for-each-ref &&\n> +\tgit -C repo/s/.git for-each-ref &&\n> +\tgit -C repo/s/.git/ for-each-ref\n> +'\n> +\n> test_done\n"},{"id":"499327","messageId":"ecfd53a7-645b-4e8b-a1e7-17ee5c782ad4@gmail.com","threadId":"61816","inReplyTo":"20240723021900.388020-4-gitster@pobox.com","subject":"Re: [PATCH v2 3/3] safe.directory: setting safe.directory=\".\" allows the \"current\" directory","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-25T09:45:41Z","receivedAt":"2024-07-25T09:45:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 23/07/2024 03:19, Junio C Hamano wrote:\n> When \"git daemon\" enters a repository, it chdir's to the requested\n> repository and then uses \".\" (the curent directory) to consult the\n> \"is this repository considered safe?\" when it is not owned by the\n> same owner as the process.\n> \n> Make sure this access will be allowed by setting safe.directory to\n> \".\".\n\nSetting safe.directory to \".\" should be unnecessary after the previous \ntwo patches. It might be better to test that \"git daemon\" works without \nsafe.directory containing \".\" instead.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Junio C Hamano \n> ---\n> t/t0033-safe-directory.sh | 32 ++++++++++++++++++++++++++++++++\n> 1 file changed, 32 insertions(+)\n> \n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index ea74657255..1eeb794194 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -233,4 +233,36 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '\n> \tgit -C repo/s/.git/ for-each-ref\n> '\n> \n> +test_expect_success 'safe.directory set to a dot' '\n> +\ttest_when_finished \"rm -rf repository\" &&\n> +\t(\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global --unset-all safe.directory\n> +\t) &&\n> +\tmkdir -p repository/subdir &&\n> +\tgit init repository &&\n> +\t(\n> +\t\tcd repository &&\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\ttest_commit sample\n> +\t) &&\n> +\n> +\t(\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \".\"\n> +\t) &&\n> +\tgit -C repository for-each-ref &&\n> +\tgit -C repository/ for-each-ref &&\n> +\tgit -C repository/.git for-each-ref &&\n> +\tgit -C repository/.git/ for-each-ref &&\n> +\n> +\t# what is allowed is repository/subdir but the repository\n> +\t# path is repository.\n> +\ttest_must_fail git -C repository/subdir for-each-ref &&\n> +\n> +\t# likewise, repository .git/refs is allowed with \".\" but\n> +\t# repository/.git that is accessed is not allowed.\n> +\ttest_must_fail git -C repository/.git/refs for-each-ref\n> +'\n> +\n> test_done\n"},{"id":"499328","messageId":"d9d1115a-d530-479a-acf8-189713632adf@gmail.com","threadId":"61816","inReplyTo":"20240723021900.388020-1-gitster@pobox.com","subject":"Re: [PATCH v2 0/3] safe.directory clean-up","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-25T09:45:46Z","receivedAt":"2024-07-25T09:45:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nThanks for picking this up, I think this looks like a good approach \napart from resolving relative entries in safe.directory which is \nintrinsically unsafe as we don't know which directory the user wants to \nconsider safe. With these changes it should not be necessary to add \".\" \nto safe.directory to get \"git daemon\" to work and all the other code \npaths use an absolute path from getcwd() so I don't think there is any \nneed to support relative directories.\n\nI'll be off the list for the next couple of weeks\n\nBest Wishes\n\nPhillip\n\nOn 23/07/2024 03:18, Junio C Hamano wrote:\n> Changes since v1:\n> \n> - two \"extra\" patches for optimization that are unnecessary have\n> been dropped.\n> - a patch to add test for safe.directory=\".\" use case has been\n> added.\n> \n> Recently we discussed what we should do when either the path\n> configured in the safe.directory configuration or coming from\n> the caller of ensure_valid_ownership() function as a result of\n> repository discovery is not normalized and textual equality check is\n> not sufficient. See the thread the contains\n> \n> https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/\n> \n> Here are three patches that implement the comparison between\n> normalized path and configuration value.\n> \n> Imagine that you have a repository at /mnt/disk4/repos/frotz\n> directory but in order to make it simpler to manage and use, you\n> have your users use /projects/frotz to access the repository. A\n> symlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory\n> allows you to do so.\n> \n> - The first patch normalizes the path to the directory that we\n> suspect is a usable repository, before comparing it with the\n> safe.directory configuration variable. The safe.directory may\n> say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to\n> the repository for the users may be /mnt/disk4/repos/frotz or\n> /projects/frotz, depending on where they come from and what their\n> $PWD makes getcwd() to say.\n> \n> - The second patch normalizes the value of the safe.directory\n> variable. This allows safe.directory to say /projects/frotz\n> or /projects/* and have them match /mnt/disk4/repos/frotz (which\n> is how the first patch normalizes the repository path to).\n> \n> - The third patch only adds a test to illustrate what happens when\n> the safe.directory configuration is set to \".\", which was a\n> plausible setting for those who run \"git daemon\". The\n> normalization done by the first two patches is sufficient to make\n> this just work.\n> \n> Junio C Hamano (3):\n> safe.directory: normalize the checked path\n> safe.directory: normalize the configured path\n> safe.directory: setting safe.directory=\".\" allows the \"current\"\n> directory\n> \n> setup.c | 28 ++++++--\n> t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 170 insertions(+), 4 deletions(-)\n> \n> Range-diff against v1:\n> 1: 20e1160946 ! 1: 86d5c83ee7 safe.directory: normalize the checked path\n> @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \"$(pwd)/repository\"\n> +\t) &&\n> -+\tgit -C repository/ for-each-ref >/dev/null &&\n> -+\tgit -C repo/ for-each-ref\n> ++\tgit -C repository for-each-ref &&\n> ++\tgit -C repository/ for-each-ref &&\n> ++\tgit -C repo for-each-ref &&\n> ++\tgit -C repo/ for-each-ref &&\n> ++\ttest_must_fail git -C repository/.git for-each-ref &&\n> ++\ttest_must_fail git -C repository/.git/ for-each-ref &&\n> ++\ttest_must_fail git -C repo/.git for-each-ref &&\n> ++\ttest_must_fail git -C repo/.git/ for-each-ref\n> +'\n> +\n> +test_expect_success SYMLINKS 'checked leading paths are normalized' '\n> @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \"$(pwd)/repository/*\"\n> +\t) &&\n> -+\tgit -C repository/s for-each-ref >/dev/null &&\n> -+\tgit -C repo/s for-each-ref\n> ++\tgit -C repository/s for-each-ref &&\n> ++\tgit -C repository/s/ for-each-ref &&\n> ++\tgit -C repo/s for-each-ref &&\n> ++\tgit -C repo/s/ for-each-ref &&\n> ++\tgit -C repository/s/.git for-each-ref &&\n> ++\tgit -C repository/s/.git/ for-each-ref &&\n> ++\tgit -C repo/s/.git for-each-ref &&\n> ++\tgit -C repo/s/.git/ for-each-ref\n> +'\n> +\n> test_done\n> 2: 06de9038b7 ! 2: c4998076da safe.directory: normalize the configured path\n> @@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n> \n> ## t/t0033-safe-directory.sh ##\n> @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' '\n> - \tgit -C repo/s for-each-ref\n> + \tgit -C repo/s/.git/ for-each-ref\n> '\n> \n> +test_expect_success SYMLINKS 'configured paths are normalized' '\n> @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \"$(pwd)/repo\"\n> +\t) &&\n> -+\tgit -C repository/ for-each-ref >/dev/null &&\n> -+\tgit -C repo/ for-each-ref\n> ++\tgit -C repository for-each-ref &&\n> ++\tgit -C repository/ for-each-ref &&\n> ++\tgit -C repo for-each-ref &&\n> ++\tgit -C repo/ for-each-ref &&\n> ++\ttest_must_fail git -C repository/.git for-each-ref &&\n> ++\ttest_must_fail git -C repository/.git/ for-each-ref &&\n> ++\ttest_must_fail git -C repo/.git for-each-ref &&\n> ++\ttest_must_fail git -C repo/.git/ for-each-ref\n> +'\n> +\n> +test_expect_success SYMLINKS 'configured leading paths are normalized' '\n> @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a\n> +\tgit init repository/s &&\n> +\tln -s repository repo &&\n> +\t(\n> -+\t\tcd repository &&\n> ++\t\tcd repository/s &&\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\ttest_commit sample\n> +\t) &&\n> @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a\n> +\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n> +\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n> +\t) &&\n> -+\tgit -C repository/s for-each-ref >/dev/null &&\n> -+\tgit -C repo/s for-each-ref\n> ++\tgit -C repository/s for-each-ref &&\n> ++\tgit -C repository/s/ for-each-ref &&\n> ++\tgit -C repository/s/.git for-each-ref &&\n> ++\tgit -C repository/s/.git/ for-each-ref &&\n> ++\tgit -C repo/s for-each-ref &&\n> ++\tgit -C repo/s/ for-each-ref &&\n> ++\tgit -C repo/s/.git for-each-ref &&\n> ++\tgit -C repo/s/.git/ for-each-ref\n> +'\n> +\n> test_done\n> -: ---------- > 3: ffc3f7364e safe.directory: setting safe.directory=\".\" allows the \"current\" directory\n"},{"id":"499355","messageId":"xmqqbk2ljvty.fsf@gitster.g","threadId":"61816","inReplyTo":"5332f244-7476-492a-a797-2ef7ba73f490@gmail.com","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T16:11:05Z","receivedAt":"2024-07-25T16:11:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I think this is sensible if the key is an absolute path, if the key is\n> a relative path I think we should ignore it as it is not clear which\n> directory the user meant.\n\nThe only thing that worries me in that proposal is that doing so\nwould break a configuration that used to work. I'd rather leave\nthe tightening of it to future work with its own justification.\n\nThanks.\n\n"},{"id":"499356","messageId":"xmqq7cd9jvsd.fsf@gitster.g","threadId":"61816","inReplyTo":"ecfd53a7-645b-4e8b-a1e7-17ee5c782ad4@gmail.com","subject":"Re: [PATCH v2 3/3] safe.directory: setting safe.directory=\".\" allows the \"current\" directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T16:12:02Z","receivedAt":"2024-07-25T16:12:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Junio\n>\n> On 23/07/2024 03:19, Junio C Hamano wrote:\n>> When \"git daemon\" enters a repository, it chdir's to the requested\n>> repository and then uses \".\" (the curent directory) to consult the\n>> \"is this repository considered safe?\" when it is not owned by the\n>> same owner as the process.\n>> Make sure this access will be allowed by setting safe.directory to\n>> \".\".\n>\n> Setting safe.directory to \".\" should be unnecessary after the previous\n> two patches. It might be better to test that \"git daemon\" works\n> without safe.directory containing \".\" instead.\n\nHmph. Even without the two previous steps, it was a working\nworkaround, wasn't it?\n"},{"id":"499357","messageId":"xmqq34nxjvos.fsf@gitster.g","threadId":"61816","inReplyTo":"d9d1115a-d530-479a-acf8-189713632adf@gmail.com","subject":"Re: [PATCH v2 0/3] safe.directory clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T16:14:11Z","receivedAt":"2024-07-25T16:14:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for picking this up, I think this looks like a good approach\n> apart from resolving relative entries in safe.directory which is\n> intrinsically unsafe as we don't know which directory the user wants\n> to consider safe. With these changes it should not be necessary to add\n> \".\" to safe.directory to get \"git daemon\" to work and all the other\n> code paths use an absolute path from getcwd() so I don't think there\n> is any need to support relative directories.\n\nI agree that we could limit to absolute, but that would mean we\nwould be breaking a configuration that used to work. I do not want\nto mix that into this topic.\n\n> I'll be off the list for the next couple of weeks\n\nI'd imagine it would be for a block of fun time? Enjoy.\n\nIn any case, this topic won't be moving in the coming few weeks\nanyway ;-)\n\n"},{"id":"499389","messageId":"20240726050253.GC642208@coredump.intra.peff.net","threadId":"61816","inReplyTo":"20240723021900.388020-3-gitster@pobox.com","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-26T05:02:53Z","receivedAt":"2024-07-26T05:02:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 22, 2024 at 07:18:59PM -0700, Junio C Hamano wrote:\n\n> The pathname of a repository comes from getcwd() and it could be a\n> path aliased via symbolic links, e.g., the real directory may be\n> /home/u/repository but a symbolic link /home/u/repo may point at it,\n> and the clone request may come as \"git clone file:///home/u/repo/\"\n> \n> A request to check if /home/u/repository is safe would be rejected\n> if the safe.directory configuration allows /home/u/repo/ but not its\n> alias /home/u/repository/. Normalize the paths configured for the\n> safe.directory configuration variable before comparing them with the\n> path being checked.\n\nThis may be a dumb question, but... will this always work, if the config\noption is not necessarily an exact path, and might have globbing\ncharacters in it?\n\nWe don't currently treat it like a wildmatch glob, but:\n\n 1. We do allow \"/*\" on the end. Should we strip that off so it doesn't\n confuse the realpath lookup?\n\n 2. This is shutting the door on using wildmatch or similar in the\n future. Is that OK?\n\n> @@ -1236,6 +1236,16 @@ static int safe_directory_cb(const char *key, const char *value,\n> \n> \t\tif (!git_config_pathname(&allowed, key, value)) {\n> \t\t\tconst char *check = allowed ? allowed : value;\n> +\t\t\tchar *to_free = real_pathdup(check, 0);\n> +\n> +\t\t\tif (!to_free) {\n> +\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n> +\t\t\t\t\tcheck);\n> +\t\t\t\tgoto next;\n> +\t\t\t} else {\n> +\t\t\t\tcheck = to_free;\n> +\t\t\t}\n\nI'm not sure about this warning. I don't know how people use this config\nin the real world, but it seems plausible to me they might have:\n\n [safe]\n directory = /home/me/this/always/exists\n directory = /home/me/this/might/not/exist\n\nperhaps because they're using a generic config file across multiple\nmachines. And then they'd get:\n\n warning: safe.directory '/home/me/this/might/not/exist' cannot be normalized\n\non every invocation (at least ones that need to consult safe.directory).\nShould we be quiet there, and maybe fall back to using the\nnon-normalized path (though I guess if we could not normalize it, that\nprobably means it could never match anyway)?\n\n-Peff\n"},{"id":"499445","messageId":"xmqqikwsyz6d.fsf@gitster.g","threadId":"61816","inReplyTo":"20240726050253.GC642208@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T15:02:02Z","receivedAt":"2024-07-26T15:02:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This may be a dumb question, but... will this always work, if the config\n> option is not necessarily an exact path, and might have globbing\n> characters in it?\n>\n> We don't currently treat it like a wildmatch glob, but:\n>\n> 1. We do allow \"/*\" on the end. Should we strip that off so it doesn't\n> confuse the realpath lookup?\n\nThis one I wondered, too, but the test seems to show that it is OK ;-)\n\n> 2. This is shutting the door on using wildmatch or similar in the\n> future. Is that OK?\n\nI am inclined to keep this part and any other logic that are meant\nto address \"security\" things simple and stupid. So in that sense,\nI am not so worried that it would be hard to retrofit wildmatch to\nthis codepath.\n\n> Should we be quiet there, and maybe fall back to using the\n> non-normalized path (though I guess if we could not normalize it, that\n> probably means it could never match anyway)?\n\nThe only reason I did the warning was because it makes me feel a bit\nuneasy to have a configuration item that either gets ignored or\ntriggers a fallback behaviour and not to tell users about it. Other\nthan that, I have no strong preference.\n\nUnfortunately this is not a very good fit for the advise mechanism,\nas we do not even have a repository yet.\n"},{"id":"499493","messageId":"20240727220524.GA1263829@coredump.intra.peff.net","threadId":"61816","inReplyTo":"xmqqikwsyz6d.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-27T22:05:24Z","receivedAt":"2024-07-27T22:05:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2024 at 08:02:02AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > This may be a dumb question, but... will this always work, if the config\n> > option is not necessarily an exact path, and might have globbing\n> > characters in it?\n> >\n> > We don't currently treat it like a wildmatch glob, but:\n> >\n> > 1. We do allow \"/*\" on the end. Should we strip that off so it doesn't\n> > confuse the realpath lookup?\n> \n> This one I wondered, too, but the test seems to show that it is OK ;-)\n\nHmm. Would it depend on what's in the filesystem, though? That is, if I\nhad a file named \"*\", would it get confused?\n\nThat is unlikely in normal use, of course, but can we do something\ntricky with it? I think probably no? Accessing \"/path/to/*\" implies that\nthe user meant to allow all of \"/path/to\", which would include that. So\nthe most an attacker could do is _disable_ safe.directory for something\nthat should have it on.\n\n> > 2. This is shutting the door on using wildmatch or similar in the\n> > future. Is that OK?\n> \n> I am inclined to keep this part and any other logic that are meant\n> to address \"security\" things simple and stupid. So in that sense,\n> I am not so worried that it would be hard to retrofit wildmatch to\n> this codepath.\n\nThat's my general instinct, too. But I wanted to make sure we were\nexplicit about the choice.\n\n> > Should we be quiet there, and maybe fall back to using the\n> > non-normalized path (though I guess if we could not normalize it, that\n> > probably means it could never match anyway)?\n> \n> The only reason I did the warning was because it makes me feel a bit\n> uneasy to have a configuration item that either gets ignored or\n> triggers a fallback behaviour and not to tell users about it. Other\n> than that, I have no strong preference.\n\nYes, that was my first thought, too, but I noticed the bogus messages\nwhile playing with it. I _think_ if it triggers it means that it points\nto something that doesn't exist (or you don't have permission to read),\nin which case it would by definition not have matched. So silently\nignoring might not be so bad.\n\n-Peff\n"},{"id":"499539","messageId":"20240730011004.4030246-1-gitster@pobox.com","threadId":"61816","inReplyTo":"20240723021900.388020-1-gitster@pobox.com","subject":"[PATCH v3 0/3] safe.directory clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:10:01Z","receivedAt":"2024-07-30T01:10:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Changes since v2:\n\n - It is no longer an error if a path listed on the safe.directory\n configuration variable does not exist, as suggested by Peff\n (cf. <20240726050253.GC642208@coredump.intra.peff.net>).\n\n - A path listed on the safe.directory that is not an absolute path is\n ignored, as it makes little sense, as suggested by Phillip\n (cf. <5332f244-7476-492a-a797-2ef7ba73f490@gmail.com>), except for\n \".\", which was once advertised as a valid workaround on the list.\n (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n\nRecently we discussed what we should do when either the path\nconfigured in the safe.directory configuration variable or coming\nfrom the caller of ensure_valid_ownership() function as a result of\nrepository discovery is not normalized and textual equality check is\nnot sufficient. See the thread the contains\n\n https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/\n\nHere are three patches that implement the comparison between\nnormalized path and configuration value.\n\nImagine that you have a repository at /mnt/disk4/repos/frotz\ndirectory but in order to make it simpler to manage and use, you\nhave your users use /projects/frotz to access the repository. A\nsymlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory\nallows you to do so.\n\n - The first patch normalizes the path to the directory that we\n suspect is a usable repository, before comparing it with the\n safe.directory configuration variable. The safe.directory may\n say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to\n the repository for the users may be /mnt/disk4/repos/frotz or\n /projects/frotz, depending on where they come from and what their\n $PWD makes getcwd() to say.\n\n - The second patch normalizes the value of the safe.directory\n variable. This allows safe.directory to say /projects/frotz or\n /projects/* and have them match /mnt/disk4/repos/frotz (which is\n how the first patch normalizes the repository path to). Note\n that non-absolute paths in safe.directory are ignored, as they\n make little sense, except for \".\".\n\n - The third patch only adds a test to illustrate what happens when\n the safe.directory configuration is set to \".\", which was\n advertised as a viable workaround for those who run \"git daemon\".\n\nJunio C Hamano (3):\n safe.directory: normalize the checked path\n safe.directory: normalize the configured path\n safe.directory: setting safe.directory=\".\" allows the \"current\"\n directory\n\n setup.c | 53 ++++++++++--\n t/t0033-safe-directory.sh | 178 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 223 insertions(+), 8 deletions(-)\n\nRange-diff against v2:\n1: 86d5c83ee7 = 1: 0e298f20ae safe.directory: normalize the checked path\n2: c4998076da ! 2: 68ada68936 safe.directory: normalize the configured path\n @@ Commit message\n safe.directory configuration variable before comparing them with the\n path being checked.\n \n + Two and a half things to note, compared to the previous step to\n + normalize the actual path of the suspected repository, are:\n +\n + - A configured safe.directory may be coming from .gitignore in the\n + home directory that may be shared across machines. The path\n + meant to match with an entry may not necessarily exist on all of\n + such machines, so not being able to convert them to real path on\n + this machine is *not* a condition that is worthy of warning.\n + Hence, we ignore a path that cannot be converted to a real path.\n +\n + - A configured safe.directory is essentially a random string that\n + user throws at us, written completely unrelated to the directory\n + the current process happens to be in. Hence it makes little\n + sense to give a non-absolute path. Hence we ignore any\n + non-absolute paths, except for \".\".\n +\n + - The safe.directory set to \".\" was once advertised on the list as\n + a valid workaround for the regression caused by the overly tight\n + safe.directory check introduced in 2.45.1; we treat it to mean\n + \"if we are at the top level of a repository, it is OK\".\n + (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n +\n Suggested-by: Phillip Wood \n + Helped-by: Jeff King \n Signed-off-by: Junio C Hamano \n \n ## setup.c ##\n @@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n \t\t\tconst char *check = allowed ? allowed : value;\n -+\t\t\tchar *to_free = real_pathdup(check, 0);\n +-\t\t\tif (ends_with(check, \"/*\")) {\n +-\t\t\t\tsize_t len = strlen(check);\n +-\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n ++\t\t\tchar *to_free = NULL;\n +\n -+\t\t\tif (!to_free) {\n -+\t\t\t\twarning(_(\"safe.directory '%s' cannot be normalized\"),\n ++\t\t\t/*\n ++\t\t\t * Setting safe.directory to a non-absolute path\n ++\t\t\t * makes little sense---it won't be relative to\n ++\t\t\t * the configuration file the item is defined in.\n ++\t\t\t * Except for \".\", which means \"if we are at the top\n ++\t\t\t * level of a repository, then it is OK\", which is\n ++\t\t\t * slightly tighter than \"*\" that allows discovery.\n ++\t\t\t */\n ++\t\t\tif (!is_absolute_path(check) && strcmp(check, \".\")) {\n ++\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n +\t\t\t\t\tcheck);\n +\t\t\t\tgoto next;\n -+\t\t\t} else {\n -+\t\t\t\tcheck = to_free;\n +\t\t\t}\n +\n - \t\t\tif (ends_with(check, \"/*\")) {\n - \t\t\t\tsize_t len = strlen(check);\n - \t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n -@@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n - \t\t\t} else if (!fspathcmp(data->path, check)) {\n ++\t\t\t/*\n ++\t\t\t * A .gitconfig in $HOME may be shared across\n ++\t\t\t * different machines and safe.directory entries\n ++\t\t\t * may or may not exist as paths on all of these\n ++\t\t\t * machines. In other words, it is not a warning\n ++\t\t\t * worthy event when there is no such path on this\n ++\t\t\t * machine---the entry may be useful elsewhere.\n ++\t\t\t */\n ++\t\t\tto_free = real_pathdup(check, 0);\n ++\t\t\tif (!to_free)\n ++\t\t\t\tgoto next;\n ++\t\t\tif (ends_with(to_free, \"/*\")) {\n ++\t\t\t\tsize_t len = strlen(to_free);\n ++\t\t\t\tif (!fspathncmp(to_free, data->path, len - 1))\n + \t\t\t\t\tdata->is_safe = 1;\n +-\t\t\t} else if (!fspathcmp(data->path, check)) {\n ++\t\t\t} else if (!fspathcmp(data->path, to_free)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n +\t\t\tfree(to_free);\n3: ffc3f7364e ! 3: 9d26940ba4 safe.directory: setting safe.directory=\".\" allows the \"current\" directory\n @@ Commit message\n same owner as the process.\n \n Make sure this access will be allowed by setting safe.directory to\n - \".\".\n + \".\", as that was once advertised on the list as a valid workaround\n + to the overly tight safe.directory settings introduced by 2.45.1\n + (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n +\n + Also add simlar test to show what happens in the same setting if the\n + safe.directory is set to \"*\" instead of \".\"; in short, \".\" is a bit\n + tighter (as it is custom designed for git-daemon situation) than\n + \"anything goes\" settings given by \"*\".\n \n Signed-off-by: Junio C Hamano \n \n @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'configured leading path\n +\tgit -C repository/.git for-each-ref &&\n +\tgit -C repository/.git/ for-each-ref &&\n +\n -+\t# what is allowed is repository/subdir but the repository\n ++\t# What is allowed is repository/subdir but the repository\n +\t# path is repository.\n +\ttest_must_fail git -C repository/subdir for-each-ref &&\n +\n -+\t# likewise, repository .git/refs is allowed with \".\" but\n ++\t# Likewise, repository .git/refs is allowed with \".\" but\n +\t# repository/.git that is accessed is not allowed.\n +\ttest_must_fail git -C repository/.git/refs for-each-ref\n +'\n ++\n ++test_expect_success 'safe.directory set to asterisk' '\n ++\ttest_when_finished \"rm -rf repository\" &&\n ++\t(\n ++\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n ++\t\tgit config --global --unset-all safe.directory\n ++\t) &&\n ++\tmkdir -p repository/subdir &&\n ++\tgit init repository &&\n ++\t(\n ++\t\tcd repository &&\n ++\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n ++\t\ttest_commit sample\n ++\t) &&\n ++\n ++\t(\n ++\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n ++\t\tgit config --global safe.directory \"*\"\n ++\t) &&\n ++\t# these are trivial\n ++\tgit -C repository for-each-ref &&\n ++\tgit -C repository/ for-each-ref &&\n ++\tgit -C repository/.git for-each-ref &&\n ++\tgit -C repository/.git/ for-each-ref &&\n ++\n ++\t# With \"*\", everything is allowed, and the repository is\n ++\t# discovered, which is different behaviour from \".\" above.\n ++\tgit -C repository/subdir for-each-ref &&\n ++\n ++\t# Likewise.\n ++\tgit -C repository/.git/refs for-each-ref\n ++'\n +\n test_done\n-- \n2.46.0-71-g1aa693ace8\n\n"},{"id":"499540","messageId":"20240730011004.4030246-2-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730011004.4030246-1-gitster@pobox.com","subject":"[PATCH v3 1/3] safe.directory: normalize the checked path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:10:02Z","receivedAt":"2024-07-30T01:10:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\".\n\nA request to check if /home/u/repo is safe would be rejected if the\nsafe.directory configuration allows /home/u/repository/ but not its\nalias /home/u/repo/. Normalize the path being checked before\ncomparing with safe.directory value(s).\n\nSuggested-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n setup.c | 16 ++++++++---\n t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex d458edcc02..45bbbe329f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n }\n \n struct safe_directory_data {\n-\tconst char *path;\n+\tchar *path;\n \tint is_safe;\n };\n \n@@ -1263,9 +1263,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t\t\t\t const char *worktree, const char *gitdir,\n \t\t\t\t struct strbuf *report)\n {\n-\tstruct safe_directory_data data = {\n-\t\t.path = worktree ? worktree : gitdir\n-\t};\n+\tstruct safe_directory_data data = { 0 };\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n \t (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&\n@@ -1273,6 +1271,15 @@ static int ensure_valid_ownership(const char *gitfile,\n \t (!gitdir || is_path_owned_by_current_user(gitdir, report)))\n \t\treturn 1;\n \n+\t/*\n+\t * normalize the data.path for comparison with normalized paths\n+\t * that come from the configuration file. The path is unsafe\n+\t * if it cannot be normalized.\n+\t */\n+\tdata.path = real_pathdup(worktree ? worktree : gitdir, 0);\n+\tif (!data.path)\n+\t\treturn 0;\n+\n \t/*\n \t * data.path is the \"path\" that identifies the repository and it is\n \t * constant regardless of what failed above. data.is_safe should be\n@@ -1280,6 +1287,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t */\n \tgit_protected_config(safe_directory_cb, &data);\n \n+\tfree(data.path);\n \treturn data.is_safe;\n }\n \ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 5fe61f1291..07ac0f9a01 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -119,4 +119,61 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '\n \ttest_path_is_dir target\n '\n \n+test_expect_success SYMLINKS 'checked paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repo for-each-ref &&\n+\tgit -C repo/ for-each-ref &&\n+\ttest_must_fail git -C repository/.git for-each-ref &&\n+\ttest_must_fail git -C repository/.git/ for-each-ref &&\n+\ttest_must_fail git -C repo/.git for-each-ref &&\n+\ttest_must_fail git -C repo/.git/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'checked leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref &&\n+\tgit -C repository/s/ for-each-ref &&\n+\tgit -C repo/s for-each-ref &&\n+\tgit -C repo/s/ for-each-ref &&\n+\tgit -C repository/s/.git for-each-ref &&\n+\tgit -C repository/s/.git/ for-each-ref &&\n+\tgit -C repo/s/.git for-each-ref &&\n+\tgit -C repo/s/.git/ for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-71-g1aa693ace8\n\n"},{"id":"499541","messageId":"20240730011004.4030246-3-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730011004.4030246-1-gitster@pobox.com","subject":"[PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:10:03Z","receivedAt":"2024-07-30T01:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\"\n\nA request to check if /home/u/repository is safe would be rejected\nif the safe.directory configuration allows /home/u/repo/ but not its\nalias /home/u/repository/. Normalize the paths configured for the\nsafe.directory configuration variable before comparing them with the\npath being checked.\n\nTwo and a half things to note, compared to the previous step to\nnormalize the actual path of the suspected repository, are:\n\n - A configured safe.directory may be coming from .gitignore in the\n home directory that may be shared across machines. The path\n meant to match with an entry may not necessarily exist on all of\n such machines, so not being able to convert them to real path on\n this machine is *not* a condition that is worthy of warning.\n Hence, we ignore a path that cannot be converted to a real path.\n\n - A configured safe.directory is essentially a random string that\n user throws at us, written completely unrelated to the directory\n the current process happens to be in. Hence it makes little\n sense to give a non-absolute path. Hence we ignore any\n non-absolute paths, except for \".\".\n\n - The safe.directory set to \".\" was once advertised on the list as\n a valid workaround for the regression caused by the overly tight\n safe.directory check introduced in 2.45.1; we treat it to mean\n \"if we are at the top level of a repository, it is OK\".\n (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n\nSuggested-by: Phillip Wood \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n setup.c | 37 ++++++++++++++++++++++---\n t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 90 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 45bbbe329f..6012f3f011 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n \t\t\tconst char *check = allowed ? allowed : value;\n-\t\t\tif (ends_with(check, \"/*\")) {\n-\t\t\t\tsize_t len = strlen(check);\n-\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n+\t\t\tchar *to_free = NULL;\n+\n+\t\t\t/*\n+\t\t\t * Setting safe.directory to a non-absolute path\n+\t\t\t * makes little sense---it won't be relative to\n+\t\t\t * the configuration file the item is defined in.\n+\t\t\t * Except for \".\", which means \"if we are at the top\n+\t\t\t * level of a repository, then it is OK\", which is\n+\t\t\t * slightly tighter than \"*\" that allows discovery.\n+\t\t\t */\n+\t\t\tif (!is_absolute_path(check) && strcmp(check, \".\")) {\n+\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n+\t\t\t\t\tcheck);\n+\t\t\t\tgoto next;\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * A .gitconfig in $HOME may be shared across\n+\t\t\t * different machines and safe.directory entries\n+\t\t\t * may or may not exist as paths on all of these\n+\t\t\t * machines. In other words, it is not a warning\n+\t\t\t * worthy event when there is no such path on this\n+\t\t\t * machine---the entry may be useful elsewhere.\n+\t\t\t */\n+\t\t\tto_free = real_pathdup(check, 0);\n+\t\t\tif (!to_free)\n+\t\t\t\tgoto next;\n+\t\t\tif (ends_with(to_free, \"/*\")) {\n+\t\t\t\tsize_t len = strlen(to_free);\n+\t\t\t\tif (!fspathncmp(to_free, data->path, len - 1))\n \t\t\t\t\tdata->is_safe = 1;\n-\t\t\t} else if (!fspathcmp(data->path, check)) {\n+\t\t\t} else if (!fspathcmp(data->path, to_free)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n+\t\t\tfree(to_free);\n \t\t}\n+\tnext:\n \t\tif (allowed != value)\n \t\t\tfree(allowed);\n \t}\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 07ac0f9a01..ea74657255 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '\n \tgit -C repo/s/.git/ for-each-ref\n '\n \n+test_expect_success SYMLINKS 'configured paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repo for-each-ref &&\n+\tgit -C repo/ for-each-ref &&\n+\ttest_must_fail git -C repository/.git for-each-ref &&\n+\ttest_must_fail git -C repository/.git/ for-each-ref &&\n+\ttest_must_fail git -C repo/.git for-each-ref &&\n+\ttest_must_fail git -C repo/.git/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'configured leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref &&\n+\tgit -C repository/s/ for-each-ref &&\n+\tgit -C repository/s/.git for-each-ref &&\n+\tgit -C repository/s/.git/ for-each-ref &&\n+\tgit -C repo/s for-each-ref &&\n+\tgit -C repo/s/ for-each-ref &&\n+\tgit -C repo/s/.git for-each-ref &&\n+\tgit -C repo/s/.git/ for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-71-g1aa693ace8\n\n"},{"id":"499542","messageId":"20240730011004.4030246-4-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730011004.4030246-1-gitster@pobox.com","subject":"[PATCH v3 3/3] safe.directory: setting safe.directory=\".\" allows the \"current\" directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T01:10:04Z","receivedAt":"2024-07-30T01:10:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"git daemon\" enters a repository, it chdir's to the requested\nrepository and then uses \".\" (the curent directory) to consult the\n\"is this repository considered safe?\" when it is not owned by the\nsame owner as the process.\n\nMake sure this access will be allowed by setting safe.directory to\n\".\", as that was once advertised on the list as a valid workaround\nto the overly tight safe.directory settings introduced by 2.45.1\n(cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n\nAlso add simlar test to show what happens in the same setting if the\nsafe.directory is set to \"*\" instead of \".\"; in short, \".\" is a bit\ntighter (as it is custom designed for git-daemon situation) than\n\"anything goes\" settings given by \"*\".\n\nSigned-off-by: Junio C Hamano \n---\n t/t0033-safe-directory.sh | 64 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex ea74657255..e97a84764f 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -233,4 +233,68 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '\n \tgit -C repo/s/.git/ for-each-ref\n '\n \n+test_expect_success 'safe.directory set to a dot' '\n+\ttest_when_finished \"rm -rf repository\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository/subdir &&\n+\tgit init repository &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \".\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repository/.git for-each-ref &&\n+\tgit -C repository/.git/ for-each-ref &&\n+\n+\t# What is allowed is repository/subdir but the repository\n+\t# path is repository.\n+\ttest_must_fail git -C repository/subdir for-each-ref &&\n+\n+\t# Likewise, repository .git/refs is allowed with \".\" but\n+\t# repository/.git that is accessed is not allowed.\n+\ttest_must_fail git -C repository/.git/refs for-each-ref\n+'\n+\n+test_expect_success 'safe.directory set to asterisk' '\n+\ttest_when_finished \"rm -rf repository\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository/subdir &&\n+\tgit init repository &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"*\"\n+\t) &&\n+\t# these are trivial\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repository/.git for-each-ref &&\n+\tgit -C repository/.git/ for-each-ref &&\n+\n+\t# With \"*\", everything is allowed, and the repository is\n+\t# discovered, which is different behaviour from \".\" above.\n+\tgit -C repository/subdir for-each-ref &&\n+\n+\t# Likewise.\n+\tgit -C repository/.git/refs for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-71-g1aa693ace8\n\n"},{"id":"499575","messageId":"20240730073119.GA562212@coredump.intra.peff.net","threadId":"61816","inReplyTo":"20240730011004.4030246-3-gitster@pobox.com","subject":"Re: [PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-30T07:31:19Z","receivedAt":"2024-07-30T07:31:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:\n\n> Two and a half things to note, compared to the previous step to\n> normalize the actual path of the suspected repository, are:\n> \n> - A configured safe.directory may be coming from .gitignore in the\n> home directory that may be shared across machines. The path\n> meant to match with an entry may not necessarily exist on all of\n> such machines, so not being able to convert them to real path on\n> this machine is *not* a condition that is worthy of warning.\n> Hence, we ignore a path that cannot be converted to a real path.\n\nThanks, I think this addresses my concern. One small thing I noticed in\nthe patch:\n\n> @@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,\n> \n> \t\tif (!git_config_pathname(&allowed, key, value)) {\n> \t\t\tconst char *check = allowed ? allowed : value;\n> -\t\t\tif (ends_with(check, \"/*\")) {\n> -\t\t\t\tsize_t len = strlen(check);\n> -\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n> +\t\t\tchar *to_free = NULL;\n> +\n> +\t\t\t/*\n> +\t\t\t * Setting safe.directory to a non-absolute path\n> +\t\t\t * makes little sense---it won't be relative to\n> +\t\t\t * the configuration file the item is defined in.\n> +\t\t\t * Except for \".\", which means \"if we are at the top\n> +\t\t\t * level of a repository, then it is OK\", which is\n> +\t\t\t * slightly tighter than \"*\" that allows discovery.\n> +\t\t\t */\n> +\t\t\tif (!is_absolute_path(check) && strcmp(check, \".\")) {\n> +\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n> +\t\t\t\t\tcheck);\n> +\t\t\t\tgoto next;\n> +\t\t\t}\n\nThis is_absolute_path() check is redundant, isn't it? If we are checking\nfor a literal \".\", then we know the patch must be non-absolute.\n\n-Peff\n"},{"id":"499577","messageId":"20240730074307.GB562212@coredump.intra.peff.net","threadId":"61816","inReplyTo":"20240730011004.4030246-3-gitster@pobox.com","subject":"Re: [PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-30T07:43:07Z","receivedAt":"2024-07-30T07:43:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:\n\n> @@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,\n> \n> \t\tif (!git_config_pathname(&allowed, key, value)) {\n> \t\t\tconst char *check = allowed ? allowed : value;\n> -\t\t\tif (ends_with(check, \"/*\")) {\n> -\t\t\t\tsize_t len = strlen(check);\n> -\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n\nBTW, one oddity I noticed in the existing code:\n\nUnder what circumstances will \"allowed\" be NULL in that ternary? I think\nif git_config_pathname() returns non-zero, then we called\ninterpolate_path(). It can return NULL, but in that case\ngit_config_pathname() will die(). We might change that later, but then\nI'd expect it to return non-zero. So I suspect the whole \"check\"\nvariable could just be dropped in favor of using \"allowed\".\n\nObviously not new in your patch, but maybe worth fixing while in the\narea? I think it comes from an evil merge in b8bdb2f283 (Merge branch\n'jc/safe-directory-leading-path', 2024-06-12).\n\n-Peff\n"},{"id":"499621","messageId":"xmqq4j86g948.fsf@gitster.g","threadId":"61816","inReplyTo":"20240730073119.GA562212@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T16:03:35Z","receivedAt":"2024-07-30T16:03:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\t\t\tif (!is_absolute_path(check) && strcmp(check, \".\")) {\n>> +\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n>> +\t\t\t\t\tcheck);\n>> +\t\t\t\tgoto next;\n>> +\t\t\t}\n>\n> This is_absolute_path() check is redundant, isn't it? If we are checking\n> for a literal \".\", then we know the path must be non-absolute.\n\nWhat I meant was \"If it is not absolute, that is an error, but if\nthe thing is a dot, that is allowed as an exception\".\n\nIs the lack of \"!\" confusing, I wonder? We could rewrite it to\nmake it more explicit:\n\n\tif (is_absolute_path(check) || !strcmp(check, \".\")) {\n\t\t; /* OK */\n\t} else {\n\t\twarning(_(\"not absolute %s\"), check);\n\t\tgoto next;\n\t}\n\nMy earlier draft for v3 had the check for dot a lot earlier in the\nfunction, i.e.\n\n\t-\t} else if (!strcmp(value, \"*\")) {\n\t+\t} else if (!strcmp(value, \"*\") || !strcmp(value, \".\")) {\n\t\t\tdata->is_safe = 1;\n\nand this part said \"If not absolute, that is an error\" without\nanything about dot.\n\nBut then I changed my mind and made it unsafe to do this:\n\n\tcd .git/refs && git -c safe.directory=. foo\n\nas safe.directory=. means \"A repository at the current directory of\nthe process is allowed\" and the repository in this case is not at \".\"\nbut at \"..\", meaning \".\" is a lot stricter than \"*\".\n\n\n"},{"id":"499624","messageId":"xmqqv80metou.fsf@gitster.g","threadId":"61816","inReplyTo":"20240730074307.GB562212@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T16:22:09Z","receivedAt":"2024-07-30T16:22:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:\n>\n>> @@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,\n>> \n>> \t\tif (!git_config_pathname(&allowed, key, value)) {\n>> \t\t\tconst char *check = allowed ? allowed : value;\n>> -\t\t\tif (ends_with(check, \"/*\")) {\n>> -\t\t\t\tsize_t len = strlen(check);\n>> -\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n>\n> BTW, one oddity I noticed in the existing code:\n>\n> Under what circumstances will \"allowed\" be NULL in that ternary? I think\n> if git_config_pathname() returns non-zero, then we called\n> interpolate_path(). It can return NULL, but in that case\n> git_config_pathname() will die(). We might change that later, but then\n> I'd expect it to return non-zero. So I suspect the whole \"check\"\n> variable could just be dropped in favor of using \"allowed\".\n>\n> Obviously not new in your patch, but maybe worth fixing while in the\n> area? I think it comes from an evil merge in b8bdb2f283 (Merge branch\n> 'jc/safe-directory-leading-path', 2024-06-12).\n\nI think it deserves to be a separate change, probably a preliminary\nclean-up, as it predates that by a few years, and goes back to the\ninitial introduction of the safe.directory feature. The merge you\nfound had this bit:\n\ndiff --cc setup.c\nindex e47946d0e7,4c5de0960b..e112545f71\n--- a/setup.c\n+++ b/setup.c\n@@@ -1230,13 -1176,21 +1230,20 @@@ static int safe_directory_cb(const cha\n \t} else if (!strcmp(value, \"*\")) {\n \t\tdata->is_safe = 1;\n \t} else {\n- \t\tchar *interpolated = NULL;\n- \n- \t\tif (!git_config_pathname(&interpolated, key, value) &&\n- \t\t !fspathcmp(data->path, interpolated ? interpolated : value))\n- \t\t\tdata->is_safe = 1;\n- \n- \t\tfree(interpolated);\n ...\n\n\nNotice that in the original, the code is prepared for the case where\ninterpolated is NULL when fspathcmp() needs to use it or value,\nwhich is when git_config_pathname() returned 0/success.\n\nIt came from 8959555c (setup_git_directory(): add an owner check for\nthe top-level directory, 2022-03-02) that introduced the\nsafe.directory feature:\n\n+\t\tif (!git_config_pathname(&interpolated, key, value) &&\n+\t\t !fspathcmp(data->path, interpolated ? interpolated : value))\n+\t\t\tdata->is_safe = 1;\n\nwhere it shared the same assumption.\n"},{"id":"499630","messageId":"xmqq5xsmdarv.fsf_-_@gitster.g","threadId":"61816","inReplyTo":"xmqqv80metou.fsf@gitster.g","subject":"safe.directory: preliminary clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T17:56:04Z","receivedAt":"2024-07-30T17:56:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The paths given in the safe.directory configuration variable are\nallowed to contain \"~user\" (which interpolates to user's home\ndirectory) and \"%(prefix)\" (which interpolates to the installation\nlocation in RUNTIME_PREFIX-enabled builds, and a call to the\ngit_config_pathname() function is tasked to obtain a copy of the\npath with these constructs interpolated.\n\nThe function, when it succeeds, always yields an allocated string in\nthe location given as the out-parameter; even when there is nothing\nto interpolate in the original, a literal copy is made. The code\npath that contains this caller somehow made two contradicting and\nincorrect assumptions of the behaviour when there is no need for\ninterpolation, and was written with extra defensiveness against\ntwo phantom risks that do not exist.\n\nOne wrong assumption was that the function might yield NULL when\nthere is no interpolation. This led to the use of an extra \"check\"\nvariable, conditionally holding either the interpolated or the\noriginal string. The assumption was with us since 8959555c\n(setup_git_directory(): add an owner check for the top-level\ndirectory, 2022-03-02) originally introduced the safe.directory\nfeature.\n\nAnother wrong assumption was that the function might yield the same\npointer as the input when there is no interpolation. This led to a\nconditional free'ing of the interpolated copy, that the conditional\nnever skipped, as we always received an allocated string.\n\nSimplify the code by removing the extra defensiveness.\n\nSigned-off-by: Junio C Hamano \n---\n setup.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git c/setup.c w/setup.c\nindex d458edcc02..3177d010d1 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -1235,17 +1235,15 @@ static int safe_directory_cb(const char *key, const char *value,\n \t\tchar *allowed = NULL;\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n-\t\t\tconst char *check = allowed ? allowed : value;\n-\t\t\tif (ends_with(check, \"/*\")) {\n-\t\t\t\tsize_t len = strlen(check);\n-\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n+\t\t\tif (ends_with(allowed, \"/*\")) {\n+\t\t\t\tsize_t len = strlen(allowed);\n+\t\t\t\tif (!fspathncmp(allowed, data->path, len - 1))\n \t\t\t\t\tdata->is_safe = 1;\n-\t\t\t} else if (!fspathcmp(data->path, check)) {\n+\t\t\t} else if (!fspathcmp(data->path, allowed)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n-\t\t}\n-\t\tif (allowed != value)\n \t\t\tfree(allowed);\n+\t\t}\n \t}\n \n \treturn 0;\n"},{"id":"499632","messageId":"20240730184352.2503276-1-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730011004.4030246-1-gitster@pobox.com","subject":"[PATCH v4 0/4] safe.directory clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T18:43:48Z","receivedAt":"2024-07-30T18:44:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" Changes since v3:\n\n - A preliminary patch to fix overly defensive (mis)uses of the\n result from git_config_pathname() API call has been added.\n\n - \"to_free\" variable that did not have the corresponding underlying\n variable that may or may not be allocated has been renamed.\n\nRecently we discussed what we should do when either the path\nconfigured in the safe.directory configuration variable or coming\nfrom the caller of ensure_valid_ownership() function as a result of\nrepository discovery is not normalized and textual equality check is\nnot sufficient. See the thread the contains\n\n https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/\n\nHere are three patches that implement the comparison between\nnormalized path and configuration value.\n\nImagine that you have a repository at /mnt/disk4/repos/frotz\ndirectory but in order to make it simpler to manage and use, you\nhave your users use /projects/frotz to access the repository. A\nsymlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory\nallows you to do so.\n\n - The first patch normalizes the path to the directory that we\n suspect is a usable repository, before comparing it with the\n safe.directory configuration variable. The safe.directory may\n say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to\n the repository for the users may be /mnt/disk4/repos/frotz or\n /projects/frotz, depending on where they come from and what their\n $PWD makes getcwd() to say.\n\n - The second patch normalizes the value of the safe.directory\n variable. This allows safe.directory to say /projects/frotz or\n /projects/* and have them match /mnt/disk4/repos/frotz (which is\n how the first patch normalizes the repository path to). Note\n that non-absolute paths in safe.directory are ignored, as they\n make little sense, except for \".\".\n\n - The third patch only adds a test to illustrate what happens when\n the safe.directory configuration is set to \".\", which was\n advertised as a viable workaround for those who run \"git daemon\".\n\nJunio C Hamano (4):\n safe.directory: preliminary clean-up\n safe.directory: normalize the checked path\n safe.directory: normalize the configured path\n safe.directory: setting safe.directory=\".\" allows the \"current\" directory\n\n setup.c | 58 ++++++++++---\n t/t0033-safe-directory.sh | 178 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 225 insertions(+), 11 deletions(-)\n\nRange-diff against v3:\n-: ---------- > 1: baed16ada3 safe.directory: preliminary clean-up\n1: 7da381eef6 = 2: 2ac2407c22 safe.directory: normalize the checked path\n2: 1e872df885 ! 3: 747120dcd7 safe.directory: normalize the configured path\n @@ Commit message\n \n ## setup.c ##\n @@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n + \t\tchar *allowed = NULL;\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n - \t\t\tconst char *check = allowed ? allowed : value;\n --\t\t\tif (ends_with(check, \"/*\")) {\n --\t\t\t\tsize_t len = strlen(check);\n --\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n -+\t\t\tchar *to_free = NULL;\n +-\t\t\tif (ends_with(allowed, \"/*\")) {\n +-\t\t\t\tsize_t len = strlen(allowed);\n +-\t\t\t\tif (!fspathncmp(allowed, data->path, len - 1))\n ++\t\t\tchar *normalized = NULL;\n +\n +\t\t\t/*\n +\t\t\t * Setting safe.directory to a non-absolute path\n @@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n +\t\t\t * level of a repository, then it is OK\", which is\n +\t\t\t * slightly tighter than \"*\" that allows discovery.\n +\t\t\t */\n -+\t\t\tif (!is_absolute_path(check) && strcmp(check, \".\")) {\n ++\t\t\tif (!is_absolute_path(allowed) && strcmp(allowed, \".\")) {\n +\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n -+\t\t\t\t\tcheck);\n ++\t\t\t\t\tallowed);\n +\t\t\t\tgoto next;\n +\t\t\t}\n +\n @@ setup.c: static int safe_directory_cb(const char *key, const char *value,\n +\t\t\t * worthy event when there is no such path on this\n +\t\t\t * machine---the entry may be useful elsewhere.\n +\t\t\t */\n -+\t\t\tto_free = real_pathdup(check, 0);\n -+\t\t\tif (!to_free)\n ++\t\t\tnormalized = real_pathdup(allowed, 0);\n ++\t\t\tif (!normalized)\n +\t\t\t\tgoto next;\n -+\t\t\tif (ends_with(to_free, \"/*\")) {\n -+\t\t\t\tsize_t len = strlen(to_free);\n -+\t\t\t\tif (!fspathncmp(to_free, data->path, len - 1))\n ++\n ++\t\t\tif (ends_with(normalized, \"/*\")) {\n ++\t\t\t\tsize_t len = strlen(normalized);\n ++\t\t\t\tif (!fspathncmp(normalized, data->path, len - 1))\n \t\t\t\t\tdata->is_safe = 1;\n --\t\t\t} else if (!fspathcmp(data->path, check)) {\n -+\t\t\t} else if (!fspathcmp(data->path, to_free)) {\n +-\t\t\t} else if (!fspathcmp(data->path, allowed)) {\n ++\t\t\t} else if (!fspathcmp(data->path, normalized)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n -+\t\t\tfree(to_free);\n - \t\t}\n -+\tnext:\n - \t\tif (allowed != value)\n ++\t\tnext:\n ++\t\t\tfree(normalized);\n \t\t\tfree(allowed);\n + \t\t}\n \t}\n \n ## t/t0033-safe-directory.sh ##\n3: 53605f4e20 = 4: 5d0cd13e34 safe.directory: setting safe.directory=\".\" allows the \"current\" directory\n-- \n2.46.0-77-g633c50689c\n\n"},{"id":"499633","messageId":"20240730184352.2503276-3-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730184352.2503276-1-gitster@pobox.com","subject":"[PATCH v4 2/4] safe.directory: normalize the checked path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T18:43:50Z","receivedAt":"2024-07-30T18:44:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\".\n\nA request to check if /home/u/repo is safe would be rejected if the\nsafe.directory configuration allows /home/u/repository/ but not its\nalias /home/u/repo/. Normalize the path being checked before\ncomparing with safe.directory value(s).\n\nSuggested-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n setup.c | 16 ++++++++---\n t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3177d010d1..54cce7219b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,\n }\n \n struct safe_directory_data {\n-\tconst char *path;\n+\tchar *path;\n \tint is_safe;\n };\n \n@@ -1261,9 +1261,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t\t\t\t const char *worktree, const char *gitdir,\n \t\t\t\t struct strbuf *report)\n {\n-\tstruct safe_directory_data data = {\n-\t\t.path = worktree ? worktree : gitdir\n-\t};\n+\tstruct safe_directory_data data = { 0 };\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n \t (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&\n@@ -1271,6 +1269,15 @@ static int ensure_valid_ownership(const char *gitfile,\n \t (!gitdir || is_path_owned_by_current_user(gitdir, report)))\n \t\treturn 1;\n \n+\t/*\n+\t * normalize the data.path for comparison with normalized paths\n+\t * that come from the configuration file. The path is unsafe\n+\t * if it cannot be normalized.\n+\t */\n+\tdata.path = real_pathdup(worktree ? worktree : gitdir, 0);\n+\tif (!data.path)\n+\t\treturn 0;\n+\n \t/*\n \t * data.path is the \"path\" that identifies the repository and it is\n \t * constant regardless of what failed above. data.is_safe should be\n@@ -1278,6 +1285,7 @@ static int ensure_valid_ownership(const char *gitfile,\n \t */\n \tgit_protected_config(safe_directory_cb, &data);\n \n+\tfree(data.path);\n \treturn data.is_safe;\n }\n \ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 5fe61f1291..07ac0f9a01 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -119,4 +119,61 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '\n \ttest_path_is_dir target\n '\n \n+test_expect_success SYMLINKS 'checked paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repo for-each-ref &&\n+\tgit -C repo/ for-each-ref &&\n+\ttest_must_fail git -C repository/.git for-each-ref &&\n+\ttest_must_fail git -C repository/.git/ for-each-ref &&\n+\ttest_must_fail git -C repo/.git for-each-ref &&\n+\ttest_must_fail git -C repo/.git/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'checked leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repository/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref &&\n+\tgit -C repository/s/ for-each-ref &&\n+\tgit -C repo/s for-each-ref &&\n+\tgit -C repo/s/ for-each-ref &&\n+\tgit -C repository/s/.git for-each-ref &&\n+\tgit -C repository/s/.git/ for-each-ref &&\n+\tgit -C repo/s/.git for-each-ref &&\n+\tgit -C repo/s/.git/ for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-77-g633c50689c\n\n"},{"id":"499634","messageId":"20240730184352.2503276-2-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730184352.2503276-1-gitster@pobox.com","subject":"[PATCH v4 1/4] safe.directory: preliminary clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T18:43:49Z","receivedAt":"2024-07-30T18:44:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The paths given in the safe.directory configuration variable are\nallowed to contain \"~user\" (which interpolates to user's home\ndirectory) and \"%(prefix)\" (which interpolates to the installation\nlocation in RUNTIME_PREFIX-enabled builds, and a call to the\ngit_config_pathname() function is tasked to obtain a copy of the\npath with these constructs interpolated.\n\nThe function, when it succeeds, always yields an allocated string in\nthe location given as the out-parameter; even when there is nothing\nto interpolate in the original, a literal copy is made. The code\npath that contains this caller somehow made two contradicting and\nincorrect assumptions of the behaviour when there is no need for\ninterpolation, and was written with extra defensiveness against\ntwo phantom risks that do not exist.\n\nOne wrong assumption was that the function might yield NULL when\nthere is no interpolation. This led to the use of an extra \"check\"\nvariable, conditionally holding either the interpolated or the\noriginal string. The assumption was with us since 8959555c\n(setup_git_directory(): add an owner check for the top-level\ndirectory, 2022-03-02) originally introduced the safe.directory\nfeature.\n\nAnother wrong assumption was that the function might yield the same\npointer as the input when there is no interpolation. This led to a\nconditional free'ing of the interpolated copy, that the conditional\nnever skipped, as we always received an allocated string.\n\nSimplify the code by removing the extra defensiveness.\n\nSigned-off-by: Junio C Hamano \n---\n setup.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex d458edcc02..3177d010d1 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1235,17 +1235,15 @@ static int safe_directory_cb(const char *key, const char *value,\n \t\tchar *allowed = NULL;\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n-\t\t\tconst char *check = allowed ? allowed : value;\n-\t\t\tif (ends_with(check, \"/*\")) {\n-\t\t\t\tsize_t len = strlen(check);\n-\t\t\t\tif (!fspathncmp(check, data->path, len - 1))\n+\t\t\tif (ends_with(allowed, \"/*\")) {\n+\t\t\t\tsize_t len = strlen(allowed);\n+\t\t\t\tif (!fspathncmp(allowed, data->path, len - 1))\n \t\t\t\t\tdata->is_safe = 1;\n-\t\t\t} else if (!fspathcmp(data->path, check)) {\n+\t\t\t} else if (!fspathcmp(data->path, allowed)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n-\t\t}\n-\t\tif (allowed != value)\n \t\t\tfree(allowed);\n+\t\t}\n \t}\n \n \treturn 0;\n-- \n2.46.0-77-g633c50689c\n\n"},{"id":"499635","messageId":"20240730184352.2503276-5-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730184352.2503276-1-gitster@pobox.com","subject":"[PATCH v4 4/4] safe.directory: setting safe.directory=\".\" allows the \"current\" directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T18:43:52Z","receivedAt":"2024-07-30T18:44:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"git daemon\" enters a repository, it chdir's to the requested\nrepository and then uses \".\" (the curent directory) to consult the\n\"is this repository considered safe?\" when it is not owned by the\nsame owner as the process.\n\nMake sure this access will be allowed by setting safe.directory to\n\".\", as that was once advertised on the list as a valid workaround\nto the overly tight safe.directory settings introduced by 2.45.1\n(cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n\nAlso add simlar test to show what happens in the same setting if the\nsafe.directory is set to \"*\" instead of \".\"; in short, \".\" is a bit\ntighter (as it is custom designed for git-daemon situation) than\n\"anything goes\" settings given by \"*\".\n\nSigned-off-by: Junio C Hamano \n---\n t/t0033-safe-directory.sh | 64 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex ea74657255..e97a84764f 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -233,4 +233,68 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '\n \tgit -C repo/s/.git/ for-each-ref\n '\n \n+test_expect_success 'safe.directory set to a dot' '\n+\ttest_when_finished \"rm -rf repository\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository/subdir &&\n+\tgit init repository &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \".\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repository/.git for-each-ref &&\n+\tgit -C repository/.git/ for-each-ref &&\n+\n+\t# What is allowed is repository/subdir but the repository\n+\t# path is repository.\n+\ttest_must_fail git -C repository/subdir for-each-ref &&\n+\n+\t# Likewise, repository .git/refs is allowed with \".\" but\n+\t# repository/.git that is accessed is not allowed.\n+\ttest_must_fail git -C repository/.git/refs for-each-ref\n+'\n+\n+test_expect_success 'safe.directory set to asterisk' '\n+\ttest_when_finished \"rm -rf repository\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository/subdir &&\n+\tgit init repository &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"*\"\n+\t) &&\n+\t# these are trivial\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repository/.git for-each-ref &&\n+\tgit -C repository/.git/ for-each-ref &&\n+\n+\t# With \"*\", everything is allowed, and the repository is\n+\t# discovered, which is different behaviour from \".\" above.\n+\tgit -C repository/subdir for-each-ref &&\n+\n+\t# Likewise.\n+\tgit -C repository/.git/refs for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-77-g633c50689c\n\n"},{"id":"499636","messageId":"20240730184352.2503276-4-gitster@pobox.com","threadId":"61816","inReplyTo":"20240730184352.2503276-1-gitster@pobox.com","subject":"[PATCH v4 3/4] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T18:43:51Z","receivedAt":"2024-07-30T18:44:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pathname of a repository comes from getcwd() and it could be a\npath aliased via symbolic links, e.g., the real directory may be\n/home/u/repository but a symbolic link /home/u/repo may point at it,\nand the clone request may come as \"git clone file:///home/u/repo/\"\n\nA request to check if /home/u/repository is safe would be rejected\nif the safe.directory configuration allows /home/u/repo/ but not its\nalias /home/u/repository/. Normalize the paths configured for the\nsafe.directory configuration variable before comparing them with the\npath being checked.\n\nTwo and a half things to note, compared to the previous step to\nnormalize the actual path of the suspected repository, are:\n\n - A configured safe.directory may be coming from .gitignore in the\n home directory that may be shared across machines. The path\n meant to match with an entry may not necessarily exist on all of\n such machines, so not being able to convert them to real path on\n this machine is *not* a condition that is worthy of warning.\n Hence, we ignore a path that cannot be converted to a real path.\n\n - A configured safe.directory is essentially a random string that\n user throws at us, written completely unrelated to the directory\n the current process happens to be in. Hence it makes little\n sense to give a non-absolute path. Hence we ignore any\n non-absolute paths, except for \".\".\n\n - The safe.directory set to \".\" was once advertised on the list as\n a valid workaround for the regression caused by the overly tight\n safe.directory check introduced in 2.45.1; we treat it to mean\n \"if we are at the top level of a repository, it is OK\".\n (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).\n\nSuggested-by: Phillip Wood \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n setup.c | 38 +++++++++++++++++++++++---\n t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 91 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 54cce7219b..5f81d9fac0 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1235,13 +1235,43 @@ static int safe_directory_cb(const char *key, const char *value,\n \t\tchar *allowed = NULL;\n \n \t\tif (!git_config_pathname(&allowed, key, value)) {\n-\t\t\tif (ends_with(allowed, \"/*\")) {\n-\t\t\t\tsize_t len = strlen(allowed);\n-\t\t\t\tif (!fspathncmp(allowed, data->path, len - 1))\n+\t\t\tchar *normalized = NULL;\n+\n+\t\t\t/*\n+\t\t\t * Setting safe.directory to a non-absolute path\n+\t\t\t * makes little sense---it won't be relative to\n+\t\t\t * the configuration file the item is defined in.\n+\t\t\t * Except for \".\", which means \"if we are at the top\n+\t\t\t * level of a repository, then it is OK\", which is\n+\t\t\t * slightly tighter than \"*\" that allows discovery.\n+\t\t\t */\n+\t\t\tif (!is_absolute_path(allowed) && strcmp(allowed, \".\")) {\n+\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n+\t\t\t\t\tallowed);\n+\t\t\t\tgoto next;\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * A .gitconfig in $HOME may be shared across\n+\t\t\t * different machines and safe.directory entries\n+\t\t\t * may or may not exist as paths on all of these\n+\t\t\t * machines. In other words, it is not a warning\n+\t\t\t * worthy event when there is no such path on this\n+\t\t\t * machine---the entry may be useful elsewhere.\n+\t\t\t */\n+\t\t\tnormalized = real_pathdup(allowed, 0);\n+\t\t\tif (!normalized)\n+\t\t\t\tgoto next;\n+\n+\t\t\tif (ends_with(normalized, \"/*\")) {\n+\t\t\t\tsize_t len = strlen(normalized);\n+\t\t\t\tif (!fspathncmp(normalized, data->path, len - 1))\n \t\t\t\t\tdata->is_safe = 1;\n-\t\t\t} else if (!fspathcmp(data->path, allowed)) {\n+\t\t\t} else if (!fspathcmp(data->path, normalized)) {\n \t\t\t\tdata->is_safe = 1;\n \t\t\t}\n+\t\tnext:\n+\t\t\tfree(normalized);\n \t\t\tfree(allowed);\n \t\t}\n \t}\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 07ac0f9a01..ea74657255 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '\n \tgit -C repo/s/.git/ for-each-ref\n '\n \n+test_expect_success SYMLINKS 'configured paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tgit init repository &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo\"\n+\t) &&\n+\tgit -C repository for-each-ref &&\n+\tgit -C repository/ for-each-ref &&\n+\tgit -C repo for-each-ref &&\n+\tgit -C repo/ for-each-ref &&\n+\ttest_must_fail git -C repository/.git for-each-ref &&\n+\ttest_must_fail git -C repository/.git/ for-each-ref &&\n+\ttest_must_fail git -C repo/.git for-each-ref &&\n+\ttest_must_fail git -C repo/.git/ for-each-ref\n+'\n+\n+test_expect_success SYMLINKS 'configured leading paths are normalized' '\n+\ttest_when_finished \"rm -rf repository; rm -f repo\" &&\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global --unset-all safe.directory\n+\t) &&\n+\tmkdir -p repository &&\n+\tgit init repository/s &&\n+\tln -s repository repo &&\n+\t(\n+\t\tcd repository/s &&\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\ttest_commit sample\n+\t) &&\n+\n+\t(\n+\t\tsane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&\n+\t\tgit config --global safe.directory \"$(pwd)/repo/*\"\n+\t) &&\n+\tgit -C repository/s for-each-ref &&\n+\tgit -C repository/s/ for-each-ref &&\n+\tgit -C repository/s/.git for-each-ref &&\n+\tgit -C repository/s/.git/ for-each-ref &&\n+\tgit -C repo/s for-each-ref &&\n+\tgit -C repo/s/ for-each-ref &&\n+\tgit -C repo/s/.git for-each-ref &&\n+\tgit -C repo/s/.git/ for-each-ref\n+'\n+\n test_done\n-- \n2.46.0-77-g633c50689c\n\n"},{"id":"499639","messageId":"20240730200838.GA567852@coredump.intra.peff.net","threadId":"61816","inReplyTo":"xmqq4j86g948.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-30T20:08:38Z","receivedAt":"2024-07-30T20:08:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2024 at 09:03:35AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> +\t\t\tif (!is_absolute_path(check) && strcmp(check, \".\")) {\n> >> +\t\t\t\twarning(_(\"safe.directory '%s' not absolute\"),\n> >> +\t\t\t\t\tcheck);\n> >> +\t\t\t\tgoto next;\n> >> +\t\t\t}\n> >\n> > This is_absolute_path() check is redundant, isn't it? If we are checking\n> > for a literal \".\", then we know the path must be non-absolute.\n> \n> What I meant was \"If it is not absolute, that is an error, but if\n> the thing is a dot, that is allowed as an exception\".\n> \n> Is the lack of \"!\" confusing, I wonder? We could rewrite it to\n> make it more explicit:\n\nOh, right, I totally misread it. You'd think after 25+ years of writing\nC that I would be able to get the strcmp() return value right in my\nhead... :)\n\nSo yeah, it is doing the right thing.\n\n> \tif (is_absolute_path(check) || !strcmp(check, \".\")) {\n> \t\t; /* OK */\n> \t} else {\n> \t\twarning(_(\"not absolute %s\"), check);\n> \t\tgoto next;\n> \t}\n\nHmm. Yeah, that probably would have softened my confusion, but it's also\nkind of hard to read. I think what you wrote originally is just fine,\nand I just mis-read it.\n\n> My earlier draft for v3 had the check for dot a lot earlier in the\n> function, i.e.\n> \n> \t-\t} else if (!strcmp(value, \"*\")) {\n> \t+\t} else if (!strcmp(value, \"*\") || !strcmp(value, \".\")) {\n> \t\t\tdata->is_safe = 1;\n> \n> and this part said \"If not absolute, that is an error\" without\n> anything about dot.\n> \n> But then I changed my mind and made it unsafe to do this:\n> \n> \tcd .git/refs && git -c safe.directory=. foo\n> \n> as safe.directory=. means \"A repository at the current directory of\n> the process is allowed\" and the repository in this case is not at \".\"\n> but at \"..\", meaning \".\" is a lot stricter than \"*\".\n\nI could see arguments in either direction, and I don't have a strong\nopinion between them.\n\n-Peff\n"},{"id":"499640","messageId":"20240730201017.GB567852@coredump.intra.peff.net","threadId":"61816","inReplyTo":"xmqqv80metou.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] safe.directory: normalize the configured path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-30T20:10:17Z","receivedAt":"2024-07-30T20:10:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2024 at 09:22:09AM -0700, Junio C Hamano wrote:\n\n> > Obviously not new in your patch, but maybe worth fixing while in the\n> > area? I think it comes from an evil merge in b8bdb2f283 (Merge branch\n> > 'jc/safe-directory-leading-path', 2024-06-12).\n> \n> I think it deserves to be a separate change, probably a preliminary\n> clean-up, as it predates that by a few years, and goes back to the\n> initial introduction of the safe.directory feature. The merge you\n> found had this bit:\n\nAh, yeah, I was busy looking at the assignments and didn't notice the\nternary in the function call. So yeah, it comes from that earlier\ncommit, but I think it was equally useless there.\n\nAnd yes, fixing it definitely should be a separate commit.\n\n-Peff\n"},{"id":"499641","messageId":"20240730201347.GC567852@coredump.intra.peff.net","threadId":"61816","inReplyTo":"xmqq5xsmdarv.fsf_-_@gitster.g","subject":"Re: safe.directory: preliminary clean-up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-30T20:13:47Z","receivedAt":"2024-07-30T20:13:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2024 at 10:56:04AM -0700, Junio C Hamano wrote:\n\n> One wrong assumption was that the function might yield NULL when\n> there is no interpolation. This led to the use of an extra \"check\"\n> variable, conditionally holding either the interpolated or the\n> original string. The assumption was with us since 8959555c\n> (setup_git_directory(): add an owner check for the top-level\n> directory, 2022-03-02) originally introduced the safe.directory\n> feature.\n> \n> Another wrong assumption was that the function might yield the same\n> pointer as the input when there is no interpolation. This led to a\n> conditional free'ing of the interpolated copy, that the conditional\n> never skipped, as we always received an allocated string.\n\nYup. Good eyes on seeing that second one. Both look correct to me from\ndouble-checking git_config_pathname(), and the patch itself looks good.\n\n-Peff\n"},{"id":"500891","messageId":"5e4906be-2cbe-44b5-b490-593ee5a42b95@gmail.com","threadId":"61816","inReplyTo":"xmqqbk2ljvty.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-08-14T13:20:41Z","receivedAt":"2024-08-14T13:20:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 25/07/2024 17:11, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> I think this is sensible if the key is an absolute path, if the key is\n>> a relative path I think we should ignore it as it is not clear which\n>> directory the user meant.\n> \n> The only thing that worries me in that proposal is that doing so\n> would break a configuration that used to work. I'd rather leave\n> the tightening of it to future work with its own justification.\n\nAs far as I know the only caller that tried to match a relative \ndirectory was enter_repo(), all of the other code paths pass an absolute \npath from getcwd(). Before this series git daemon required the relative \ndirectory \".\" to be specified in addition to the absolute path so that \nwill not be broken by removing support for relative directories. Are \nthere other users of enter_repo() that still rely on being able to match \nrelative paths?\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n> \n"},{"id":"500922","messageId":"xmqqh6bnyqjp.fsf@gitster.g","threadId":"61816","inReplyTo":"5e4906be-2cbe-44b5-b490-593ee5a42b95@gmail.com","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-14T17:15:06Z","receivedAt":"2024-08-14T17:15:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 25/07/2024 17:11, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>> \n>>> I think this is sensible if the key is an absolute path, if the key is\n>>> a relative path I think we should ignore it as it is not clear which\n>>> directory the user meant.\n>> The only thing that worries me in that proposal is that doing so\n>> would break a configuration that used to work. I'd rather leave\n>> the tightening of it to future work with its own justification.\n>\n> As far as I know the only caller that tried to match a relative\n> directory was enter_repo(), all of the other code paths pass an\n> absolute path from getcwd(). Before this series git daemon required\n> the relative directory \".\" to be specified in addition to the absolute\n> path so that will not be broken by removing support for relative\n> directories. Are there other users of enter_repo() that still rely on\n> being able to match relative paths?\n\nOffhand I do not know of any, but no guarantees. I am tempted to\nleave it as a prerequisite task for those who want to tighten this\ncodepath further.\n\n"},{"id":"500967","messageId":"0882a098-eb07-41b1-9527-04bdb77fa0d4@gmail.com","threadId":"61816","inReplyTo":"xmqqh6bnyqjp.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-08-15T09:51:54Z","receivedAt":"2024-08-15T09:51:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/08/2024 18:15, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 25/07/2024 17:11, Junio C Hamano wrote:\n>>> Phillip Wood writes:\n>>>\n>>>> I think this is sensible if the key is an absolute path, if the key is\n>>>> a relative path I think we should ignore it as it is not clear which\n>>>> directory the user meant.\n>>> The only thing that worries me in that proposal is that doing so\n>>> would break a configuration that used to work. I'd rather leave\n>>> the tightening of it to future work with its own justification.\n>>\n>> As far as I know the only caller that tried to match a relative\n>> directory was enter_repo(), all of the other code paths pass an\n>> absolute path from getcwd(). Before this series git daemon required\n>> the relative directory \".\" to be specified in addition to the absolute\n>> path so that will not be broken by removing support for relative\n>> directories. Are there other users of enter_repo() that still rely on\n>> being able to match relative paths?\n> \n> Offhand I do not know of any, but no guarantees. I am tempted to\n> leave it as a prerequisite task for those who want to tighten this\n> codepath further.\n\nFair enough. I will note though that this change makes it much more \nlikely that \"safe.directory=.\" will match as it now behaves like \"*\". \nPreviously it only matched when we tried to match it against \".\" whereas \nnow it matches any directory.\n\nBest Wishes\n\nPhillip\n"},{"id":"501012","messageId":"xmqqo75tvocm.fsf@gitster.g","threadId":"61816","inReplyTo":"0882a098-eb07-41b1-9527-04bdb77fa0d4@gmail.com","subject":"Re: [PATCH v2 2/3] safe.directory: normalize the configured path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-15T14:43:05Z","receivedAt":"2024-08-15T14:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Fair enough. I will note though that this change makes it much more\n> likely that \"safe.directory=.\" will match as it now behaves like\n> \"*\". Previously it only matched when we tried to match it against \".\"\n> whereas now it matches any directory.\n\nUnless you are commenting on my earlier unpublished draft, I doubt\nthat it is the case. \n\nIf I did the code analysis correctly when I responded to Peff in\nhttps://lore.kernel.org/git/xmqq4j86g948.fsf@gitster.g/, that is.\n\nThere are some negative tests added by the series to verify that dot\nis really \"current directory\" and not \"any directory\" which is what\n'*' is for (and as I always tell my contributors, we should really\nget in the habit of writing more negative tests, exactly for things\nlike this where we do not want a \"feature\" to trigger in cases we do\nnot want them to).\n\nAhh, if your e-mail header is correct, you are commenting on the\nimplementation in v2, which did not care if the input were absolute\nor not? If so, then your confusion is somewhat understandable.\n\nThe things changed before the final edition v4 happened and it is\nslightly tigher than the version you are commenting on. We refuse\nto match any non-absolute path, except for \".\", since v3. But I\nthink even with the looser implementation of v2, the negative test\nwould have made sure that \".\" was really \"current and not any\".\n\nThanks.\n"}]}