Changes since v2: - It is no longer an error if a path listed on the safe.directory configuration variable does not exist, as suggested by Peff (cf. <20240726050253.GC642208@xxxxxxxxxxxxxxxxxxxxxxx>). - A path listed on the safe.directory that is not an absolute path is ignored, as it makes little sense, as suggested by Phillip (cf. <5332f244-7476-492a-a797-2ef7ba73f490@xxxxxxxxx>), except for ".", which was once advertised as a valid workaround on the list. (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@xxxxxxxxx>). Recently we discussed what we should do when either the path configured in the safe.directory configuration variable or coming from the caller of ensure_valid_ownership() function as a result of repository discovery is not normalized and textual equality check is not sufficient. See the thread the contains https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@xxxxxxxxx/ Here are three patches that implement the comparison between normalized path and configuration value. Imagine that you have a repository at /mnt/disk4/repos/frotz directory but in order to make it simpler to manage and use, you have your users use /projects/frotz to access the repository. A symlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory allows you to do so. - The first patch normalizes the path to the directory that we suspect is a usable repository, before comparing it with the safe.directory configuration variable. The safe.directory may say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to the repository for the users may be /mnt/disk4/repos/frotz or /projects/frotz, depending on where they come from and what their $PWD makes getcwd() to say. - The second patch normalizes the value of the safe.directory variable. This allows safe.directory to say /projects/frotz or /projects/* and have them match /mnt/disk4/repos/frotz (which is how the first patch normalizes the repository path to). Note that non-absolute paths in safe.directory are ignored, as they make little sense, except for ".". - The third patch only adds a test to illustrate what happens when the safe.directory configuration is set to ".", which was advertised as a viable workaround for those who run "git daemon". Junio C Hamano (3): safe.directory: normalize the checked path safe.directory: normalize the configured path safe.directory: setting safe.directory="." allows the "current" directory setup.c | 53 ++++++++++-- t/t0033-safe-directory.sh | 178 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 8 deletions(-) Range-diff against v2: 1: 86d5c83ee7 = 1: 0e298f20ae safe.directory: normalize the checked path 2: c4998076da ! 2: 68ada68936 safe.directory: normalize the configured path @@ Commit message safe.directory configuration variable before comparing them with the path being checked. + Two and a half things to note, compared to the previous step to + normalize the actual path of the suspected repository, are: + + - A configured safe.directory may be coming from .gitignore in the + home directory that may be shared across machines. The path + meant to match with an entry may not necessarily exist on all of + such machines, so not being able to convert them to real path on + this machine is *not* a condition that is worthy of warning. + Hence, we ignore a path that cannot be converted to a real path. + + - A configured safe.directory is essentially a random string that + user throws at us, written completely unrelated to the directory + the current process happens to be in. Hence it makes little + sense to give a non-absolute path. Hence we ignore any + non-absolute paths, except for ".". + + - The safe.directory set to "." was once advertised on the list as + a valid workaround for the regression caused by the overly tight + safe.directory check introduced in 2.45.1; we treat it to mean + "if we are at the top level of a repository, it is OK". + (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@xxxxxxxxx>). + Suggested-by: Phillip Wood <phillip.wood123@xxxxxxxxx> + Helped-by: Jeff King <peff@xxxxxxxx> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> ## setup.c ## @@ setup.c: static int safe_directory_cb(const char *key, const char *value, if (!git_config_pathname(&allowed, key, value)) { const char *check = allowed ? allowed : value; -+ char *to_free = real_pathdup(check, 0); +- if (ends_with(check, "/*")) { +- size_t len = strlen(check); +- if (!fspathncmp(check, data->path, len - 1)) ++ char *to_free = NULL; + -+ if (!to_free) { -+ warning(_("safe.directory '%s' cannot be normalized"), ++ /* ++ * Setting safe.directory to a non-absolute path ++ * makes little sense---it won't be relative to ++ * the configuration file the item is defined in. ++ * Except for ".", which means "if we are at the top ++ * level of a repository, then it is OK", which is ++ * slightly tighter than "*" that allows discovery. ++ */ ++ if (!is_absolute_path(check) && strcmp(check, ".")) { ++ warning(_("safe.directory '%s' not absolute"), + check); + goto next; -+ } else { -+ check = to_free; + } + - if (ends_with(check, "/*")) { - size_t len = strlen(check); - if (!fspathncmp(check, data->path, len - 1)) -@@ setup.c: static int safe_directory_cb(const char *key, const char *value, - } else if (!fspathcmp(data->path, check)) { ++ /* ++ * A .gitconfig in $HOME may be shared across ++ * different machines and safe.directory entries ++ * may or may not exist as paths on all of these ++ * machines. In other words, it is not a warning ++ * worthy event when there is no such path on this ++ * machine---the entry may be useful elsewhere. ++ */ ++ to_free = real_pathdup(check, 0); ++ if (!to_free) ++ goto next; ++ if (ends_with(to_free, "/*")) { ++ size_t len = strlen(to_free); ++ if (!fspathncmp(to_free, data->path, len - 1)) + data->is_safe = 1; +- } else if (!fspathcmp(data->path, check)) { ++ } else if (!fspathcmp(data->path, to_free)) { data->is_safe = 1; } + free(to_free); 3: ffc3f7364e ! 3: 9d26940ba4 safe.directory: setting safe.directory="." allows the "current" directory @@ Commit message same owner as the process. Make sure this access will be allowed by setting safe.directory to - ".". + ".", as that was once advertised on the list as a valid workaround + to the overly tight safe.directory settings introduced by 2.45.1 + (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@xxxxxxxxx>). + + Also add simlar test to show what happens in the same setting if the + safe.directory is set to "*" instead of "."; in short, "." is a bit + tighter (as it is custom designed for git-daemon situation) than + "anything goes" settings given by "*". Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'configured leading path + git -C repository/.git for-each-ref && + git -C repository/.git/ for-each-ref && + -+ # what is allowed is repository/subdir but the repository ++ # What is allowed is repository/subdir but the repository + # path is repository. + test_must_fail git -C repository/subdir for-each-ref && + -+ # likewise, repository .git/refs is allowed with "." but ++ # Likewise, repository .git/refs is allowed with "." but + # repository/.git that is accessed is not allowed. + test_must_fail git -C repository/.git/refs for-each-ref +' ++ ++test_expect_success 'safe.directory set to asterisk' ' ++ test_when_finished "rm -rf repository" && ++ ( ++ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && ++ git config --global --unset-all safe.directory ++ ) && ++ mkdir -p repository/subdir && ++ git init repository && ++ ( ++ cd repository && ++ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && ++ test_commit sample ++ ) && ++ ++ ( ++ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && ++ git config --global safe.directory "*" ++ ) && ++ # these are trivial ++ git -C repository for-each-ref && ++ git -C repository/ for-each-ref && ++ git -C repository/.git for-each-ref && ++ git -C repository/.git/ for-each-ref && ++ ++ # With "*", everything is allowed, and the repository is ++ # discovered, which is different behaviour from "." above. ++ git -C repository/subdir for-each-ref && ++ ++ # Likewise. ++ git -C repository/.git/refs for-each-ref ++' + test_done -- 2.46.0-71-g1aa693ace8
