[PATCH 0/2] safe.directory clean-up

[PATCH 0/2] safe.directory clean-up

[Junio C Hamano]

Recently we discussed what we should do when either the path
configured in the safe.directory configuration 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@gmail.com/

Here are two patches (yes, two) that implements 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).

 - The third patch is a preliminary clean-up that would be needed if
   we wanted to use the fourth patch.

 - The fourth patch would become relevant if we were to call
   ensure_valid_ownership() on many directories in a single process.
   We grab safe.directory values and normalize them just once before
   using in ensure_valid_ownership() to optimize away repeated
   normalization.

It turns out that nobody calls ensure_valid_ownership() on many
different directories even in the repository discovery loop, which
means the fourth patch is not needed, which in turn means the third
patch that is a preliminary clean-up is also not necessary.

Junio C Hamano (4):
  safe.directory: normalize the checked path
  safe.directory: normalize the configured path
  setup: allow centralized clean-up when leaving setup_git_directory_gently_1()
  setup: cache normalized safe.directory configuration

 setup.c                   | 141 +++++++++++++++++++++++++++++++-------
 t/t0033-safe-directory.sh |  90 ++++++++++++++++++++++++
 2 files changed, 205 insertions(+), 26 deletions(-)

-- 
2.46.0-rc1-48-g0900f1888e

[PATCH 1/2] safe.directory: normalize the checked path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/".

A request to check if /home/u/repo is safe would be rejected if the
safe.directory configuration allows /home/u/repository/ but not its
alias /home/u/repo/.  Normalize the path being checked before
comparing with safe.directory value(s).

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 16 ++++++++++----
 t/t0033-safe-directory.sh | 45 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index d458edcc02..45bbbe329f 100644
--- a/setup.c
+++ b/setup.c
@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 }
 
 struct safe_directory_data {
-	const char *path;
+	char *path;
 	int is_safe;
 };
 
@@ -1263,9 +1263,7 @@ static int ensure_valid_ownership(const char *gitfile,
 				  const char *worktree, const char *gitdir,
 				  struct strbuf *report)
 {
-	struct safe_directory_data data = {
-		.path = worktree ? worktree : gitdir
-	};
+	struct safe_directory_data data = { 0 };
 
 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
 	    (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
@@ -1273,6 +1271,15 @@ static int ensure_valid_ownership(const char *gitfile,
 	    (!gitdir || is_path_owned_by_current_user(gitdir, report)))
 		return 1;
 
+	/*
+	 * normalize the data.path for comparison with normalized paths
+	 * that come from the configuration file.  The path is unsafe
+	 * if it cannot be normalized.
+	 */
+	data.path = real_pathdup(worktree ? worktree : gitdir, 0);
+	if (!data.path)
+		return 0;
+
 	/*
 	 * data.path is the "path" that identifies the repository and it is
 	 * constant regardless of what failed above. data.is_safe should be
@@ -1280,6 +1287,7 @@ static int ensure_valid_ownership(const char *gitfile,
 	 */
 	git_protected_config(safe_directory_cb, &data);
 
+	free(data.path);
 	return data.is_safe;
 }
 
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 5fe61f1291..3e487e7f4b 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -119,4 +119,49 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '
 	test_path_is_dir target
 '
 
+test_expect_success SYMLINKS 'checked paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository"
+	) &&
+	git -C repository/ for-each-ref >/dev/null &&
+	git -C repo/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'checked leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository/*"
+	) &&
+	git -C repository/s for-each-ref >/dev/null &&
+	git -C repo/s for-each-ref
+'
+
 test_done
-- 
2.46.0-rc1-48-g0900f1888e

[PATCH 2/2] safe.directory: normalize the configured path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/"

A request to check if /home/u/repository is safe would be rejected
if the safe.directory configuration allows /home/u/repo/ but not its
alias /home/u/repository/.  Normalize the paths configured for the
safe.directory configuration variable before comparing them with the
path being checked.

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 12 +++++++++++
 t/t0033-safe-directory.sh | 45 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/setup.c b/setup.c
index 45bbbe329f..29304d7452 100644
--- a/setup.c
+++ b/setup.c
@@ -1236,6 +1236,16 @@ 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 (!to_free) {
+				warning(_("safe.directory '%s' cannot be normalized"),
+					check);
+				goto next;
+			} else {
+				check = to_free;
+			}
+
 			if (ends_with(check, "/*")) {
 				size_t len = strlen(check);
 				if (!fspathncmp(check, data->path, len - 1))
@@ -1243,7 +1253,9 @@ static int safe_directory_cb(const char *key, const char *value,
 			} else if (!fspathcmp(data->path, check)) {
 				data->is_safe = 1;
 			}
+			free(to_free);
 		}
+	next:
 		if (allowed != value)
 			free(allowed);
 	}
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 3e487e7f4b..17d45d481e 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -164,4 +164,49 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '
 	git -C repo/s for-each-ref
 '
 
+test_expect_success SYMLINKS 'configured paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo"
+	) &&
+	git -C repository/ for-each-ref >/dev/null &&
+	git -C repo/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'configured leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo/*"
+	) &&
+	git -C repository/s for-each-ref >/dev/null &&
+	git -C repo/s for-each-ref
+'
+
 test_done
-- 
2.46.0-rc1-48-g0900f1888e

[PATCH 3/2] setup: use a single return path in setup_git_directory*()

[Junio C Hamano]

[Do not use. For illustration purposes only]

Instead of sprinkling "return" all over the place, use the "assign
to the result variable and then jump to the single label set up to
leave the function" pattern, so that we can clean up any extra
resource allocated before returning at a single place.

No functional change is intended with this step, but it will be used
soon.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c | 50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/setup.c b/setup.c
index 29304d7452..29e23a905c 100644
--- a/setup.c
+++ b/setup.c
@@ -1416,6 +1416,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	int ceil_offset = -1, min_offset = offset_1st_component(dir->buf);
 	dev_t current_device = 0;
 	int one_filesystem = 1;
+	enum discovery_result result;
 
 	/*
 	 * If GIT_DIR is set explicitly, we're not going
@@ -1425,7 +1426,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
 	if (gitdirenv) {
 		strbuf_addstr(gitdir, gitdirenv);
-		return GIT_DIR_EXPLICIT;
+		result = GIT_DIR_EXPLICIT;
+		goto cleanup_and_return;
 	}
 
 	if (env_ceiling_dirs) {
@@ -1479,8 +1481,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
 					gitdir_path = xstrdup(dir->buf);
 				}
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
-				return GIT_DIR_INVALID_GITFILE;
+			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
+				result = GIT_DIR_INVALID_GITFILE;
+				goto cleanup_and_return;
+			}
 		} else
 			gitfile = xstrdup(dir->buf);
 		/*
@@ -1491,16 +1495,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		 */
 		strbuf_setlen(dir, offset);
 		if (gitdirenv) {
-			enum discovery_result ret;
 			const char *gitdir_candidate =
 				gitdir_path ? gitdir_path : gitdirenv;
 
 			if (ensure_valid_ownership(gitfile, dir->buf,
 						   gitdir_candidate, report)) {
 				strbuf_addstr(gitdir, gitdirenv);
-				ret = GIT_DIR_DISCOVERED;
+				result = GIT_DIR_DISCOVERED;
 			} else
-				ret = GIT_DIR_INVALID_OWNERSHIP;
+				result = GIT_DIR_INVALID_OWNERSHIP;
 
 			/*
 			 * Earlier, during discovery, we might have allocated
@@ -1514,8 +1517,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 			 */
 			free(gitdir_path);
 			free(gitfile);
-
-			return ret;
+			goto cleanup_and_return;
 		}
 
 		if (is_git_directory(dir->buf)) {
@@ -1523,25 +1525,37 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
 			    !is_implicit_bare_repo(dir->buf))
 				return GIT_DIR_DISALLOWED_BARE;
-			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
-				return GIT_DIR_INVALID_OWNERSHIP;
-			strbuf_addstr(gitdir, ".");
-			return GIT_DIR_BARE;
+			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) {
+				result = GIT_DIR_INVALID_OWNERSHIP;
+			} else {
+				strbuf_addstr(gitdir, ".");
+				result = GIT_DIR_BARE;
+			}
+			goto cleanup_and_return;
 		}
 
-		if (offset <= min_offset)
-			return GIT_DIR_HIT_CEILING;
+		if (offset <= min_offset) {
+			result = GIT_DIR_HIT_CEILING;
+			goto cleanup_and_return;
+		}
 
 		while (--offset > ceil_offset && !is_dir_sep(dir->buf[offset]))
 			; /* continue */
-		if (offset <= ceil_offset)
-			return GIT_DIR_HIT_CEILING;
+		if (offset <= ceil_offset) {
+			result = GIT_DIR_HIT_CEILING;
+			goto cleanup_and_return;
+		}
 
 		strbuf_setlen(dir, offset > min_offset ?  offset : min_offset);
 		if (one_filesystem &&
-		    current_device != get_device_or_die(dir->buf, NULL, offset))
-			return GIT_DIR_HIT_MOUNT_POINT;
+		    current_device != get_device_or_die(dir->buf, NULL, offset)) {
+			result = GIT_DIR_HIT_MOUNT_POINT;
+			goto cleanup_and_return;
+		}
 	}
+
+cleanup_and_return:
+	return result;
 }
 
 enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
-- 
2.46.0-rc1-48-g0900f1888e

[PATCH 4/2] setup: cache normalized safe.directory configuration

[Junio C Hamano]

[Do not use. For illustration purposes only]

The current check performed in ensure_valid_ownership() reads each
safe.directory configuration item and normalizes it before checking
against the path to the repository.

This is OK as long as we are checking just a single directory, like
in die_upon_dubious_ownership() and setup_git_directory_gently_1().
But let's pretend that the latter calls ensure_valid_ownership()
many times in the loop, and demonstrate how we would avoid having to
normalize the same safe.directory configuration item over and over.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 12 deletions(-)

diff --git a/setup.c b/setup.c
index 29e23a905c..75bcce0368 100644
--- a/setup.c
+++ b/setup.c
@@ -20,6 +20,7 @@
 #include "trace2.h"
 #include "worktree.h"
 #include "exec-cmd.h"
+#include "strvec.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
@@ -1217,6 +1218,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 struct safe_directory_data {
 	char *path;
 	int is_safe;
+	int prenormalized;
 };
 
 static int safe_directory_cb(const char *key, const char *value,
@@ -1236,14 +1238,20 @@ 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 (!to_free) {
-				warning(_("safe.directory '%s' cannot be normalized"),
-					check);
-				goto next;
-			} else {
+			char *to_free = NULL;
+
+			if (!data->prenormalized) {
+				to_free = real_pathdup(check, 0);
+				if (!to_free) {
+					warning(_("safe.directory '%s' "
+						  "cannot be normalized"),
+						check);
+					goto next;
+				}
 				check = to_free;
+			} else {
+				to_free = NULL;
+				check = value;
 			}
 
 			if (ends_with(check, "/*")) {
@@ -1263,6 +1271,39 @@ static int safe_directory_cb(const char *key, const char *value,
 	return 0;
 }
 
+static int prenorm_cb(const char *key, const char *value,
+		      const struct config_context *ctx UNUSED, void *v_)
+{
+	struct strvec *v = v_;
+
+	if (strcmp(key, "safe.directory"))
+		return 0;
+	if (!value || !*value) {
+		strvec_clear(v);
+	} else if (!strcmp(value, "*")) {
+		strvec_push(v, value);
+	} else {
+		char *allowed = NULL;
+		if (!git_config_pathname(&allowed, key, value)) {
+			const char *ccheck = allowed ? allowed : value;
+			char *check = real_pathdup(ccheck, 0);
+			if (check)
+				strvec_push_nodup(v, check);
+			else
+				warning(_("safe.directory '%s' cannot be normalized"),
+					ccheck);
+		}
+		if (allowed != value)
+			free(allowed);
+	}
+	return 0;
+}
+
+static void prenormalize_safe_directory(struct strvec *v)
+{
+	git_protected_config(prenorm_cb, v);
+}
+
 /*
  * Check if a repository is safe, by verifying the ownership of the
  * worktree (if any), the git directory, and the gitfile (if any).
@@ -1273,6 +1314,7 @@ static int safe_directory_cb(const char *key, const char *value,
  */
 static int ensure_valid_ownership(const char *gitfile,
 				  const char *worktree, const char *gitdir,
+				  struct strvec *safe_cache,
 				  struct strbuf *report)
 {
 	struct safe_directory_data data = { 0 };
@@ -1297,8 +1339,15 @@ static int ensure_valid_ownership(const char *gitfile,
 	 * constant regardless of what failed above. data.is_safe should be
 	 * initialized to false, and might be changed by the callback.
 	 */
-	git_protected_config(safe_directory_cb, &data);
-
+	if (!safe_cache) {
+		git_protected_config(safe_directory_cb, &data);
+	} else {
+		data.prenormalized = 1;
+		for (size_t i = 0; i < safe_cache->nr; i++) {
+			safe_directory_cb("safe.directory", safe_cache->v[i],
+					  NULL, &data);
+		}
+	}
 	free(data.path);
 	return data.is_safe;
 }
@@ -1309,7 +1358,7 @@ void die_upon_dubious_ownership(const char *gitfile, const char *worktree,
 	struct strbuf report = STRBUF_INIT, quoted = STRBUF_INIT;
 	const char *path;
 
-	if (ensure_valid_ownership(gitfile, worktree, gitdir, &report))
+	if (ensure_valid_ownership(gitfile, worktree, gitdir, NULL, &report))
 		return;
 
 	strbuf_complete(&report, '\n');
@@ -1416,6 +1465,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	int ceil_offset = -1, min_offset = offset_1st_component(dir->buf);
 	dev_t current_device = 0;
 	int one_filesystem = 1;
+	struct strvec safe_cache = STRVEC_INIT;
 	enum discovery_result result;
 
 	/*
@@ -1463,6 +1513,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
 	if (one_filesystem)
 		current_device = get_device_or_die(dir->buf, NULL, 0);
+
+	prenormalize_safe_directory(&safe_cache);
 	for (;;) {
 		int offset = dir->len, error_code = 0;
 		char *gitdir_path = NULL;
@@ -1499,7 +1551,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 				gitdir_path ? gitdir_path : gitdirenv;
 
 			if (ensure_valid_ownership(gitfile, dir->buf,
-						   gitdir_candidate, report)) {
+						   gitdir_candidate,
+						   &safe_cache, report)) {
 				strbuf_addstr(gitdir, gitdirenv);
 				result = GIT_DIR_DISCOVERED;
 			} else
@@ -1525,7 +1578,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
 			    !is_implicit_bare_repo(dir->buf))
 				return GIT_DIR_DISALLOWED_BARE;
-			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) {
+			if (!ensure_valid_ownership(NULL, NULL, dir->buf,
+						    &safe_cache, report)) {
 				result = GIT_DIR_INVALID_OWNERSHIP;
 			} else {
 				strbuf_addstr(gitdir, ".");
@@ -1555,6 +1609,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	}
 
 cleanup_and_return:
+	strvec_clear(&safe_cache);
 	return result;
 }
 
-- 
2.46.0-rc1-48-g0900f1888e

[PATCH v2 0/3] safe.directory clean-up

[Junio C Hamano]

Changes since v1:

 - two "extra" patches for optimization that are unnecessary have
   been dropped.
 - a patch to add test for safe.directory="." use case has been
   added. 

Recently we discussed what we should do when either the path
configured in the safe.directory configuration 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@gmail.com/

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).

 - The third patch only adds a test to illustrate what happens when
   the safe.directory configuration is set to ".", which was a
   plausible setting for those who run "git daemon".  The
   normalization done by the first two patches is sufficient to make
   this just work.

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                   |  28 ++++++--
 t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 4 deletions(-)

Range-diff against v1:
1:  20e1160946 ! 1:  86d5c83ee7 safe.directory: normalize the checked path
    @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
     +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
     +		git config --global safe.directory "$(pwd)/repository"
     +	) &&
    -+	git -C repository/ for-each-ref >/dev/null &&
    -+	git -C repo/ for-each-ref
    ++	git -C repository for-each-ref &&
    ++	git -C repository/ for-each-ref &&
    ++	git -C repo for-each-ref &&
    ++	git -C repo/ for-each-ref &&
    ++	test_must_fail git -C repository/.git for-each-ref &&
    ++	test_must_fail git -C repository/.git/ for-each-ref &&
    ++	test_must_fail git -C repo/.git for-each-ref &&
    ++	test_must_fail git -C repo/.git/ for-each-ref
     +'
     +
     +test_expect_success SYMLINKS 'checked leading paths are normalized' '
    @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
     +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
     +		git config --global safe.directory "$(pwd)/repository/*"
     +	) &&
    -+	git -C repository/s for-each-ref >/dev/null &&
    -+	git -C repo/s for-each-ref
    ++	git -C repository/s for-each-ref &&
    ++	git -C repository/s/ for-each-ref &&
    ++	git -C repo/s for-each-ref &&
    ++	git -C repo/s/ for-each-ref &&
    ++	git -C repository/s/.git for-each-ref &&
    ++	git -C repository/s/.git/ for-each-ref &&
    ++	git -C repo/s/.git for-each-ref &&
    ++	git -C repo/s/.git/ for-each-ref
     +'
     +
      test_done
2:  06de9038b7 ! 2:  c4998076da safe.directory: normalize the configured path
    @@ setup.c: static int safe_directory_cb(const char *key, const char *value,
     
      ## t/t0033-safe-directory.sh ##
     @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' '
    - 	git -C repo/s for-each-ref
    + 	git -C repo/s/.git/ for-each-ref
      '
      
     +test_expect_success SYMLINKS 'configured paths are normalized' '
    @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
     +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
     +		git config --global safe.directory "$(pwd)/repo"
     +	) &&
    -+	git -C repository/ for-each-ref >/dev/null &&
    -+	git -C repo/ for-each-ref
    ++	git -C repository for-each-ref &&
    ++	git -C repository/ for-each-ref &&
    ++	git -C repo for-each-ref &&
    ++	git -C repo/ for-each-ref &&
    ++	test_must_fail git -C repository/.git for-each-ref &&
    ++	test_must_fail git -C repository/.git/ for-each-ref &&
    ++	test_must_fail git -C repo/.git for-each-ref &&
    ++	test_must_fail git -C repo/.git/ for-each-ref
     +'
     +
     +test_expect_success SYMLINKS 'configured leading paths are normalized' '
    @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
     +	git init repository/s &&
     +	ln -s repository repo &&
     +	(
    -+		cd repository &&
    ++		cd repository/s &&
     +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
     +		test_commit sample
     +	) &&
    @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
     +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
     +		git config --global safe.directory "$(pwd)/repo/*"
     +	) &&
    -+	git -C repository/s for-each-ref >/dev/null &&
    -+	git -C repo/s for-each-ref
    ++	git -C repository/s for-each-ref &&
    ++	git -C repository/s/ for-each-ref &&
    ++	git -C repository/s/.git for-each-ref &&
    ++	git -C repository/s/.git/ for-each-ref &&
    ++	git -C repo/s for-each-ref &&
    ++	git -C repo/s/ for-each-ref &&
    ++	git -C repo/s/.git for-each-ref &&
    ++	git -C repo/s/.git/ for-each-ref
     +'
     +
      test_done
-:  ---------- > 3:  ffc3f7364e safe.directory: setting safe.directory="." allows the "current" directory
-- 
2.46.0-rc1-52-gda884b23f2

[PATCH v2 1/3] safe.directory: normalize the checked path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/".

A request to check if /home/u/repo is safe would be rejected if the
safe.directory configuration allows /home/u/repository/ but not its
alias /home/u/repo/.  Normalize the path being checked before
comparing with safe.directory value(s).

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 16 ++++++++---
 t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index d458edcc02..45bbbe329f 100644
--- a/setup.c
+++ b/setup.c
@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 }
 
 struct safe_directory_data {
-	const char *path;
+	char *path;
 	int is_safe;
 };
 
@@ -1263,9 +1263,7 @@ static int ensure_valid_ownership(const char *gitfile,
 				  const char *worktree, const char *gitdir,
 				  struct strbuf *report)
 {
-	struct safe_directory_data data = {
-		.path = worktree ? worktree : gitdir
-	};
+	struct safe_directory_data data = { 0 };
 
 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
 	    (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
@@ -1273,6 +1271,15 @@ static int ensure_valid_ownership(const char *gitfile,
 	    (!gitdir || is_path_owned_by_current_user(gitdir, report)))
 		return 1;
 
+	/*
+	 * normalize the data.path for comparison with normalized paths
+	 * that come from the configuration file.  The path is unsafe
+	 * if it cannot be normalized.
+	 */
+	data.path = real_pathdup(worktree ? worktree : gitdir, 0);
+	if (!data.path)
+		return 0;
+
 	/*
 	 * data.path is the "path" that identifies the repository and it is
 	 * constant regardless of what failed above. data.is_safe should be
@@ -1280,6 +1287,7 @@ static int ensure_valid_ownership(const char *gitfile,
 	 */
 	git_protected_config(safe_directory_cb, &data);
 
+	free(data.path);
 	return data.is_safe;
 }
 
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 5fe61f1291..07ac0f9a01 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -119,4 +119,61 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '
 	test_path_is_dir target
 '
 
+test_expect_success SYMLINKS 'checked paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository"
+	) &&
+	git -C repository for-each-ref &&
+	git -C repository/ for-each-ref &&
+	git -C repo for-each-ref &&
+	git -C repo/ for-each-ref &&
+	test_must_fail git -C repository/.git for-each-ref &&
+	test_must_fail git -C repository/.git/ for-each-ref &&
+	test_must_fail git -C repo/.git for-each-ref &&
+	test_must_fail git -C repo/.git/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'checked leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository/*"
+	) &&
+	git -C repository/s for-each-ref &&
+	git -C repository/s/ for-each-ref &&
+	git -C repo/s for-each-ref &&
+	git -C repo/s/ for-each-ref &&
+	git -C repository/s/.git for-each-ref &&
+	git -C repository/s/.git/ for-each-ref &&
+	git -C repo/s/.git for-each-ref &&
+	git -C repo/s/.git/ for-each-ref
+'
+
 test_done
-- 
2.46.0-rc1-52-gda884b23f2

[PATCH v2 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/"

A request to check if /home/u/repository is safe would be rejected
if the safe.directory configuration allows /home/u/repo/ but not its
alias /home/u/repository/.  Normalize the paths configured for the
safe.directory configuration variable before comparing them with the
path being checked.

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 12 +++++++++
 t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/setup.c b/setup.c
index 45bbbe329f..29304d7452 100644
--- a/setup.c
+++ b/setup.c
@@ -1236,6 +1236,16 @@ 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 (!to_free) {
+				warning(_("safe.directory '%s' cannot be normalized"),
+					check);
+				goto next;
+			} else {
+				check = to_free;
+			}
+
 			if (ends_with(check, "/*")) {
 				size_t len = strlen(check);
 				if (!fspathncmp(check, data->path, len - 1))
@@ -1243,7 +1253,9 @@ static int safe_directory_cb(const char *key, const char *value,
 			} else if (!fspathcmp(data->path, check)) {
 				data->is_safe = 1;
 			}
+			free(to_free);
 		}
+	next:
 		if (allowed != value)
 			free(allowed);
 	}
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 07ac0f9a01..ea74657255 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '
 	git -C repo/s/.git/ for-each-ref
 '
 
+test_expect_success SYMLINKS 'configured paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo"
+	) &&
+	git -C repository for-each-ref &&
+	git -C repository/ for-each-ref &&
+	git -C repo for-each-ref &&
+	git -C repo/ for-each-ref &&
+	test_must_fail git -C repository/.git for-each-ref &&
+	test_must_fail git -C repository/.git/ for-each-ref &&
+	test_must_fail git -C repo/.git for-each-ref &&
+	test_must_fail git -C repo/.git/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'configured leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo/*"
+	) &&
+	git -C repository/s for-each-ref &&
+	git -C repository/s/ for-each-ref &&
+	git -C repository/s/.git for-each-ref &&
+	git -C repository/s/.git/ for-each-ref &&
+	git -C repo/s for-each-ref &&
+	git -C repo/s/ for-each-ref &&
+	git -C repo/s/.git for-each-ref &&
+	git -C repo/s/.git/ for-each-ref
+'
+
 test_done
-- 
2.46.0-rc1-52-gda884b23f2

[PATCH v2 3/3] safe.directory: setting safe.directory="." allows the "current" directory

[Junio C Hamano]

When "git daemon" enters a repository, it chdir's to the requested
repository and then uses "." (the curent directory) to consult the
"is this repository considered safe?" when it is not owned by the
same owner as the process.

Make sure this access will be allowed by setting safe.directory to
".".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0033-safe-directory.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index ea74657255..1eeb794194 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -233,4 +233,36 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '
 	git -C repo/s/.git/ for-each-ref
 '
 
+test_expect_success 'safe.directory set to a dot' '
+	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 "."
+	) &&
+	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 &&
+
+	# 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
+	# repository/.git that is accessed is not allowed.
+	test_must_fail git -C repository/.git/refs for-each-ref
+'
+
 test_done
-- 
2.46.0-rc1-52-gda884b23f2

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Phillip Wood]

Hi Junio

On 23/07/2024 03:18, Junio C Hamano wrote:
> The pathname of a repository comes from getcwd() and it could be a
> path aliased via symbolic links, e.g., the real directory may be
> /home/u/repository but a symbolic link /home/u/repo may point at it,
> and the clone request may come as "git clone file:///home/u/repo/"
> 
> A request to check if /home/u/repository is safe would be rejected
> if the safe.directory configuration allows /home/u/repo/ but not its
> alias /home/u/repository/.  Normalize the paths configured for the
> safe.directory configuration variable before comparing them with the
> path being checked.

I think this is sensible if the key is an absolute path, if the key is a 
relative path I think we should ignore it as it is not clear which 
directory the user meant.

Best Wishes

Phillip

> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   setup.c                   | 12 +++++++++
>   t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 69 insertions(+)
> 
> diff --git a/setup.c b/setup.c
> index 45bbbe329f..29304d7452 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1236,6 +1236,16 @@ 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 (!to_free) {
> +				warning(_("safe.directory '%s' cannot be normalized"),
> +					check);
> +				goto next;
> +			} else {
> +				check = to_free;
> +			}
> +
>   			if (ends_with(check, "/*")) {
>   				size_t len = strlen(check);
>   				if (!fspathncmp(check, data->path, len - 1))
> @@ -1243,7 +1253,9 @@ static int safe_directory_cb(const char *key, const char *value,
>   			} else if (!fspathcmp(data->path, check)) {
>   				data->is_safe = 1;
>   			}
> +			free(to_free);
>   		}
> +	next:
>   		if (allowed != value)
>   			free(allowed);
>   	}
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 07ac0f9a01..ea74657255 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '
>   	git -C repo/s/.git/ for-each-ref
>   '
>   
> +test_expect_success SYMLINKS 'configured paths are normalized' '
> +	test_when_finished "rm -rf repository; rm -f repo" &&
> +	(
> +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		git config --global --unset-all safe.directory
> +	) &&
> +	git init repository &&
> +	ln -s repository repo &&
> +	(
> +		cd repository &&
> +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		test_commit sample
> +	) &&
> +
> +	(
> +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		git config --global safe.directory "$(pwd)/repo"
> +	) &&
> +	git -C repository for-each-ref &&
> +	git -C repository/ for-each-ref &&
> +	git -C repo for-each-ref &&
> +	git -C repo/ for-each-ref &&
> +	test_must_fail git -C repository/.git for-each-ref &&
> +	test_must_fail git -C repository/.git/ for-each-ref &&
> +	test_must_fail git -C repo/.git for-each-ref &&
> +	test_must_fail git -C repo/.git/ for-each-ref
> +'
> +
> +test_expect_success SYMLINKS 'configured leading paths are normalized' '
> +	test_when_finished "rm -rf repository; rm -f repo" &&
> +	(
> +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		git config --global --unset-all safe.directory
> +	) &&
> +	mkdir -p repository &&
> +	git init repository/s &&
> +	ln -s repository repo &&
> +	(
> +		cd repository/s &&
> +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		test_commit sample
> +	) &&
> +
> +	(
> +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		git config --global safe.directory "$(pwd)/repo/*"
> +	) &&
> +	git -C repository/s for-each-ref &&
> +	git -C repository/s/ for-each-ref &&
> +	git -C repository/s/.git for-each-ref &&
> +	git -C repository/s/.git/ for-each-ref &&
> +	git -C repo/s for-each-ref &&
> +	git -C repo/s/ for-each-ref &&
> +	git -C repo/s/.git for-each-ref &&
> +	git -C repo/s/.git/ for-each-ref
> +'
> +
>   test_done

Re: [PATCH v2 3/3] safe.directory: setting safe.directory="." allows the "current" directory

[Phillip Wood]

Hi Junio

On 23/07/2024 03:19, Junio C Hamano wrote:
> When "git daemon" enters a repository, it chdir's to the requested
> repository and then uses "." (the curent directory) to consult the
> "is this repository considered safe?" when it is not owned by the
> same owner as the process.
> 
> Make sure this access will be allowed by setting safe.directory to
> ".".

Setting safe.directory to "." should be unnecessary after the previous 
two patches. It might be better to test that "git daemon" works without 
safe.directory containing "." instead.

Best Wishes

Phillip

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   t/t0033-safe-directory.sh | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index ea74657255..1eeb794194 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -233,4 +233,36 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '
>   	git -C repo/s/.git/ for-each-ref
>   '
>   
> +test_expect_success 'safe.directory set to a dot' '
> +	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 "."
> +	) &&
> +	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 &&
> +
> +	# 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
> +	# repository/.git that is accessed is not allowed.
> +	test_must_fail git -C repository/.git/refs for-each-ref
> +'
> +
>   test_done

Re: [PATCH v2 0/3] safe.directory clean-up

[Phillip Wood]

Hi Junio

Thanks for picking this up, I think this looks like a good approach 
apart from resolving relative entries in safe.directory which is 
intrinsically unsafe as we don't know which directory the user wants to 
consider safe. With these changes it should not be necessary to add "." 
to safe.directory to get "git daemon" to work and all the other code 
paths use an absolute path from getcwd() so I don't think there is any 
need to support relative directories.

I'll be off the list for the next couple of weeks

Best Wishes

Phillip

On 23/07/2024 03:18, Junio C Hamano wrote:
> Changes since v1:
> 
>   - two "extra" patches for optimization that are unnecessary have
>     been dropped.
>   - a patch to add test for safe.directory="." use case has been
>     added.
> 
> Recently we discussed what we should do when either the path
> configured in the safe.directory configuration 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@gmail.com/
> 
> 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).
> 
>   - The third patch only adds a test to illustrate what happens when
>     the safe.directory configuration is set to ".", which was a
>     plausible setting for those who run "git daemon".  The
>     normalization done by the first two patches is sufficient to make
>     this just work.
> 
> 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                   |  28 ++++++--
>   t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 170 insertions(+), 4 deletions(-)
> 
> Range-diff against v1:
> 1:  20e1160946 ! 1:  86d5c83ee7 safe.directory: normalize the checked path
>      @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repository"
>       +	) &&
>      -+	git -C repository/ for-each-ref >/dev/null &&
>      -+	git -C repo/ for-each-ref
>      ++	git -C repository for-each-ref &&
>      ++	git -C repository/ for-each-ref &&
>      ++	git -C repo for-each-ref &&
>      ++	git -C repo/ for-each-ref &&
>      ++	test_must_fail git -C repository/.git for-each-ref &&
>      ++	test_must_fail git -C repository/.git/ for-each-ref &&
>      ++	test_must_fail git -C repo/.git for-each-ref &&
>      ++	test_must_fail git -C repo/.git/ for-each-ref
>       +'
>       +
>       +test_expect_success SYMLINKS 'checked leading paths are normalized' '
>      @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repository/*"
>       +	) &&
>      -+	git -C repository/s for-each-ref >/dev/null &&
>      -+	git -C repo/s for-each-ref
>      ++	git -C repository/s for-each-ref &&
>      ++	git -C repository/s/ for-each-ref &&
>      ++	git -C repo/s for-each-ref &&
>      ++	git -C repo/s/ for-each-ref &&
>      ++	git -C repository/s/.git for-each-ref &&
>      ++	git -C repository/s/.git/ for-each-ref &&
>      ++	git -C repo/s/.git for-each-ref &&
>      ++	git -C repo/s/.git/ for-each-ref
>       +'
>       +
>        test_done
> 2:  06de9038b7 ! 2:  c4998076da safe.directory: normalize the configured path
>      @@ setup.c: static int safe_directory_cb(const char *key, const char *value,
>       
>        ## t/t0033-safe-directory.sh ##
>       @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' '
>      - 	git -C repo/s for-each-ref
>      + 	git -C repo/s/.git/ for-each-ref
>        '
>        
>       +test_expect_success SYMLINKS 'configured paths are normalized' '
>      @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repo"
>       +	) &&
>      -+	git -C repository/ for-each-ref >/dev/null &&
>      -+	git -C repo/ for-each-ref
>      ++	git -C repository for-each-ref &&
>      ++	git -C repository/ for-each-ref &&
>      ++	git -C repo for-each-ref &&
>      ++	git -C repo/ for-each-ref &&
>      ++	test_must_fail git -C repository/.git for-each-ref &&
>      ++	test_must_fail git -C repository/.git/ for-each-ref &&
>      ++	test_must_fail git -C repo/.git for-each-ref &&
>      ++	test_must_fail git -C repo/.git/ for-each-ref
>       +'
>       +
>       +test_expect_success SYMLINKS 'configured leading paths are normalized' '
>      @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
>       +	git init repository/s &&
>       +	ln -s repository repo &&
>       +	(
>      -+		cd repository &&
>      ++		cd repository/s &&
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		test_commit sample
>       +	) &&
>      @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repo/*"
>       +	) &&
>      -+	git -C repository/s for-each-ref >/dev/null &&
>      -+	git -C repo/s for-each-ref
>      ++	git -C repository/s for-each-ref &&
>      ++	git -C repository/s/ for-each-ref &&
>      ++	git -C repository/s/.git for-each-ref &&
>      ++	git -C repository/s/.git/ for-each-ref &&
>      ++	git -C repo/s for-each-ref &&
>      ++	git -C repo/s/ for-each-ref &&
>      ++	git -C repo/s/.git for-each-ref &&
>      ++	git -C repo/s/.git/ for-each-ref
>       +'
>       +
>        test_done
> -:  ---------- > 3:  ffc3f7364e safe.directory: setting safe.directory="." allows the "current" directory

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I think this is sensible if the key is an absolute path, if the key is
> a relative path I think we should ignore it as it is not clear which
> directory the user meant.

The only thing that worries me in that proposal is that doing so
would break a configuration that used to work.  I'd rather leave
the tightening of it to future work with its own justification.

Thanks.

Re: [PATCH v2 3/3] safe.directory: setting safe.directory="." allows the "current" directory

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Junio
>
> On 23/07/2024 03:19, Junio C Hamano wrote:
>> When "git daemon" enters a repository, it chdir's to the requested
>> repository and then uses "." (the curent directory) to consult the
>> "is this repository considered safe?" when it is not owned by the
>> same owner as the process.
>> Make sure this access will be allowed by setting safe.directory to
>> ".".
>
> Setting safe.directory to "." should be unnecessary after the previous
> two patches. It might be better to test that "git daemon" works
> without safe.directory containing "." instead.

Hmph.  Even without the two previous steps, it was a working
workaround, wasn't it?

Re: [PATCH v2 0/3] safe.directory clean-up

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Thanks for picking this up, I think this looks like a good approach
> apart from resolving relative entries in safe.directory which is
> intrinsically unsafe as we don't know which directory the user wants
> to consider safe. With these changes it should not be necessary to add
> "." to safe.directory to get "git daemon" to work and all the other
> code paths use an absolute path from getcwd() so I don't think there
> is any need to support relative directories.

I agree that we could limit to absolute, but that would mean we
would be breaking a configuration that used to work.  I do not want
to mix that into this topic.

> I'll be off the list for the next couple of weeks

I'd imagine it would be for a block of fun time?  Enjoy.

In any case, this topic won't be moving in the coming few weeks
anyway ;-)

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Jeff King]

On Mon, Jul 22, 2024 at 07:18:59PM -0700, Junio C Hamano wrote:

> The pathname of a repository comes from getcwd() and it could be a
> path aliased via symbolic links, e.g., the real directory may be
> /home/u/repository but a symbolic link /home/u/repo may point at it,
> and the clone request may come as "git clone file:///home/u/repo/"
> 
> A request to check if /home/u/repository is safe would be rejected
> if the safe.directory configuration allows /home/u/repo/ but not its
> alias /home/u/repository/.  Normalize the paths configured for the
> safe.directory configuration variable before comparing them with the
> path being checked.

This may be a dumb question, but... will this always work, if the config
option is not necessarily an exact path, and might have globbing
characters in it?

We don't currently treat it like a wildmatch glob, but:

  1. We do allow "/*" on the end. Should we strip that off so it doesn't
     confuse the realpath lookup?

  2. This is shutting the door on using wildmatch or similar in the
     future. Is that OK?

> @@ -1236,6 +1236,16 @@ 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 (!to_free) {
> +				warning(_("safe.directory '%s' cannot be normalized"),
> +					check);
> +				goto next;
> +			} else {
> +				check = to_free;
> +			}

I'm not sure about this warning. I don't know how people use this config
in the real world, but it seems plausible to me they might have:

  [safe]
  directory = /home/me/this/always/exists
  directory = /home/me/this/might/not/exist

perhaps because they're using a generic config file across multiple
machines. And then they'd get:

  warning: safe.directory '/home/me/this/might/not/exist' cannot be normalized

on every invocation (at least ones that need to consult safe.directory).
Should we be quiet there, and maybe fall back to using the
non-normalized path (though I guess if we could not normalize it, that
probably means it could never match anyway)?

-Peff

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This may be a dumb question, but... will this always work, if the config
> option is not necessarily an exact path, and might have globbing
> characters in it?
>
> We don't currently treat it like a wildmatch glob, but:
>
>   1. We do allow "/*" on the end. Should we strip that off so it doesn't
>      confuse the realpath lookup?

This one I wondered, too, but the test seems to show that it is OK ;-)

>   2. This is shutting the door on using wildmatch or similar in the
>      future. Is that OK?

I am inclined to keep this part and any other logic that are meant
to address "security" things simple and stupid.  So in that sense,
I am not so worried that it would be hard to retrofit wildmatch to
this codepath.

> Should we be quiet there, and maybe fall back to using the
> non-normalized path (though I guess if we could not normalize it, that
> probably means it could never match anyway)?

The only reason I did the warning was because it makes me feel a bit
uneasy to have a configuration item that either gets ignored or
triggers a fallback behaviour and not to tell users about it.  Other
than that, I have no strong preference.

Unfortunately this is not a very good fit for the advise mechanism,
as we do not even have a repository yet.

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Jeff King]

On Fri, Jul 26, 2024 at 08:02:02AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This may be a dumb question, but... will this always work, if the config
> > option is not necessarily an exact path, and might have globbing
> > characters in it?
> >
> > We don't currently treat it like a wildmatch glob, but:
> >
> >   1. We do allow "/*" on the end. Should we strip that off so it doesn't
> >      confuse the realpath lookup?
> 
> This one I wondered, too, but the test seems to show that it is OK ;-)

Hmm. Would it depend on what's in the filesystem, though? That is, if I
had a file named "*", would it get confused?

That is unlikely in normal use, of course, but can we do something
tricky with it? I think probably no? Accessing "/path/to/*" implies that
the user meant to allow all of "/path/to", which would include that. So
the most an attacker could do is _disable_ safe.directory for something
that should have it on.

> >   2. This is shutting the door on using wildmatch or similar in the
> >      future. Is that OK?
> 
> I am inclined to keep this part and any other logic that are meant
> to address "security" things simple and stupid.  So in that sense,
> I am not so worried that it would be hard to retrofit wildmatch to
> this codepath.

That's my general instinct, too. But I wanted to make sure we were
explicit about the choice.

> > Should we be quiet there, and maybe fall back to using the
> > non-normalized path (though I guess if we could not normalize it, that
> > probably means it could never match anyway)?
> 
> The only reason I did the warning was because it makes me feel a bit
> uneasy to have a configuration item that either gets ignored or
> triggers a fallback behaviour and not to tell users about it.  Other
> than that, I have no strong preference.

Yes, that was my first thought, too, but I noticed the bogus messages
while playing with it. I _think_ if it triggers it means that it points
to something that doesn't exist (or you don't have permission to read),
in which case it would by definition not have matched. So silently
ignoring might not be so bad.

-Peff

[PATCH v3 0/3] safe.directory clean-up

[Junio C Hamano]

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@coredump.intra.peff.net>).

 - 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@gmail.com>), except for
   ".", which was once advertised as a valid workaround on the list.
   (cf. <834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com>).

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@gmail.com/

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@gmail.com>).
    +
         Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
    +    Helped-by: Jeff King <peff@peff.net>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## 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@gmail.com>).
    +
    +    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@pobox.com>
     
    @@ 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

[PATCH v3 1/3] safe.directory: normalize the checked path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/".

A request to check if /home/u/repo is safe would be rejected if the
safe.directory configuration allows /home/u/repository/ but not its
alias /home/u/repo/.  Normalize the path being checked before
comparing with safe.directory value(s).

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 16 ++++++++---
 t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index d458edcc02..45bbbe329f 100644
--- a/setup.c
+++ b/setup.c
@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 }
 
 struct safe_directory_data {
-	const char *path;
+	char *path;
 	int is_safe;
 };
 
@@ -1263,9 +1263,7 @@ static int ensure_valid_ownership(const char *gitfile,
 				  const char *worktree, const char *gitdir,
 				  struct strbuf *report)
 {
-	struct safe_directory_data data = {
-		.path = worktree ? worktree : gitdir
-	};
+	struct safe_directory_data data = { 0 };
 
 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
 	    (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
@@ -1273,6 +1271,15 @@ static int ensure_valid_ownership(const char *gitfile,
 	    (!gitdir || is_path_owned_by_current_user(gitdir, report)))
 		return 1;
 
+	/*
+	 * normalize the data.path for comparison with normalized paths
+	 * that come from the configuration file.  The path is unsafe
+	 * if it cannot be normalized.
+	 */
+	data.path = real_pathdup(worktree ? worktree : gitdir, 0);
+	if (!data.path)
+		return 0;
+
 	/*
 	 * data.path is the "path" that identifies the repository and it is
 	 * constant regardless of what failed above. data.is_safe should be
@@ -1280,6 +1287,7 @@ static int ensure_valid_ownership(const char *gitfile,
 	 */
 	git_protected_config(safe_directory_cb, &data);
 
+	free(data.path);
 	return data.is_safe;
 }
 
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 5fe61f1291..07ac0f9a01 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -119,4 +119,61 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '
 	test_path_is_dir target
 '
 
+test_expect_success SYMLINKS 'checked paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository"
+	) &&
+	git -C repository for-each-ref &&
+	git -C repository/ for-each-ref &&
+	git -C repo for-each-ref &&
+	git -C repo/ for-each-ref &&
+	test_must_fail git -C repository/.git for-each-ref &&
+	test_must_fail git -C repository/.git/ for-each-ref &&
+	test_must_fail git -C repo/.git for-each-ref &&
+	test_must_fail git -C repo/.git/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'checked leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository/*"
+	) &&
+	git -C repository/s for-each-ref &&
+	git -C repository/s/ for-each-ref &&
+	git -C repo/s for-each-ref &&
+	git -C repo/s/ for-each-ref &&
+	git -C repository/s/.git for-each-ref &&
+	git -C repository/s/.git/ for-each-ref &&
+	git -C repo/s/.git for-each-ref &&
+	git -C repo/s/.git/ for-each-ref
+'
+
 test_done
-- 
2.46.0-71-g1aa693ace8

[PATCH v3 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/"

A request to check if /home/u/repository is safe would be rejected
if the safe.directory configuration allows /home/u/repo/ but not its
alias /home/u/repository/.  Normalize the paths configured for the
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@gmail.com>).

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 37 ++++++++++++++++++++++---
 t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 45bbbe329f..6012f3f011 100644
--- a/setup.c
+++ b/setup.c
@@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,
 
 		if (!git_config_pathname(&allowed, key, value)) {
 			const char *check = allowed ? allowed : value;
-			if (ends_with(check, "/*")) {
-				size_t len = strlen(check);
-				if (!fspathncmp(check, data->path, len - 1))
+			char *to_free = NULL;
+
+			/*
+			 * 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;
+			}
+
+			/*
+			 * 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);
 		}
+	next:
 		if (allowed != value)
 			free(allowed);
 	}
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 07ac0f9a01..ea74657255 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '
 	git -C repo/s/.git/ for-each-ref
 '
 
+test_expect_success SYMLINKS 'configured paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo"
+	) &&
+	git -C repository for-each-ref &&
+	git -C repository/ for-each-ref &&
+	git -C repo for-each-ref &&
+	git -C repo/ for-each-ref &&
+	test_must_fail git -C repository/.git for-each-ref &&
+	test_must_fail git -C repository/.git/ for-each-ref &&
+	test_must_fail git -C repo/.git for-each-ref &&
+	test_must_fail git -C repo/.git/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'configured leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo/*"
+	) &&
+	git -C repository/s for-each-ref &&
+	git -C repository/s/ for-each-ref &&
+	git -C repository/s/.git for-each-ref &&
+	git -C repository/s/.git/ for-each-ref &&
+	git -C repo/s for-each-ref &&
+	git -C repo/s/ for-each-ref &&
+	git -C repo/s/.git for-each-ref &&
+	git -C repo/s/.git/ for-each-ref
+'
+
 test_done
-- 
2.46.0-71-g1aa693ace8

[PATCH v3 3/3] safe.directory: setting safe.directory="." allows the "current" directory

[Junio C Hamano]

When "git daemon" enters a repository, it chdir's to the requested
repository and then uses "." (the curent directory) to consult the
"is this repository considered safe?" when it is not owned by the
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@gmail.com>).

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@pobox.com>
---
 t/t0033-safe-directory.sh | 64 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index ea74657255..e97a84764f 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -233,4 +233,68 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '
 	git -C repo/s/.git/ for-each-ref
 '
 
+test_expect_success 'safe.directory set to a dot' '
+	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 "."
+	) &&
+	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 &&
+
+	# 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
+	# 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

Re: [PATCH v3 2/3] safe.directory: normalize the configured path

[Jeff King]

On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:

> 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.

Thanks, I think this addresses my concern. One small thing I noticed in
the patch:

> @@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,
>  
>  		if (!git_config_pathname(&allowed, key, value)) {
>  			const char *check = allowed ? allowed : value;
> -			if (ends_with(check, "/*")) {
> -				size_t len = strlen(check);
> -				if (!fspathncmp(check, data->path, len - 1))
> +			char *to_free = NULL;
> +
> +			/*
> +			 * 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;
> +			}

This is_absolute_path() check is redundant, isn't it? If we are checking
for a literal ".", then we know the patch must be non-absolute.

-Peff

Re: [PATCH v3 2/3] safe.directory: normalize the configured path

[Jeff King]

On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:

> @@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,
>  
>  		if (!git_config_pathname(&allowed, key, value)) {
>  			const char *check = allowed ? allowed : value;
> -			if (ends_with(check, "/*")) {
> -				size_t len = strlen(check);
> -				if (!fspathncmp(check, data->path, len - 1))

BTW, one oddity I noticed in the existing code:

Under what circumstances will "allowed" be NULL in that ternary? I think
if git_config_pathname() returns non-zero, then we called
interpolate_path(). It can return NULL, but in that case
git_config_pathname() will die(). We might change that later, but then
I'd expect it to return non-zero. So I suspect the whole "check"
variable could just be dropped in favor of using "allowed".

Obviously not new in your patch, but maybe worth fixing while in the
area? I think it comes from an evil merge in b8bdb2f283 (Merge branch
'jc/safe-directory-leading-path', 2024-06-12).

-Peff

Re: [PATCH v3 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +			if (!is_absolute_path(check) && strcmp(check, ".")) {
>> +				warning(_("safe.directory '%s' not absolute"),
>> +					check);
>> +				goto next;
>> +			}
>
> This is_absolute_path() check is redundant, isn't it? If we are checking
> for a literal ".", then we know the path must be non-absolute.

What I meant was "If it is not absolute, that is an error, but if
the thing is a dot, that is allowed as an exception".

Is the lack of "!" confusing, I wonder?   We could rewrite it to
make it more explicit:

	if (is_absolute_path(check) || !strcmp(check, ".")) {
		; /* OK */
	} else {
		warning(_("not absolute %s"), check);
		goto next;
	}

My earlier draft for v3 had the check for dot a lot earlier in the
function, i.e.

	-	} else if (!strcmp(value, "*")) {
	+	} else if (!strcmp(value, "*") || !strcmp(value, ".")) {
			data->is_safe = 1;

and this part said "If not absolute, that is an error" without
anything about dot.

But then I changed my mind and made it unsafe to do this:

	cd .git/refs && git -c safe.directory=. foo

as safe.directory=. means "A repository at the current directory of
the process is allowed" and the repository in this case is not at "."
but at "..", meaning "." is a lot stricter than "*".

Re: [PATCH v3 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:
>
>> @@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,
>>  
>>  		if (!git_config_pathname(&allowed, key, value)) {
>>  			const char *check = allowed ? allowed : value;
>> -			if (ends_with(check, "/*")) {
>> -				size_t len = strlen(check);
>> -				if (!fspathncmp(check, data->path, len - 1))
>
> BTW, one oddity I noticed in the existing code:
>
> Under what circumstances will "allowed" be NULL in that ternary? I think
> if git_config_pathname() returns non-zero, then we called
> interpolate_path(). It can return NULL, but in that case
> git_config_pathname() will die(). We might change that later, but then
> I'd expect it to return non-zero. So I suspect the whole "check"
> variable could just be dropped in favor of using "allowed".
>
> Obviously not new in your patch, but maybe worth fixing while in the
> area? I think it comes from an evil merge in b8bdb2f283 (Merge branch
> 'jc/safe-directory-leading-path', 2024-06-12).

I think it deserves to be a separate change, probably a preliminary
clean-up, as it predates that by a few years, and goes back to the
initial introduction of the safe.directory feature.  The merge you
found had this bit:

diff --cc setup.c
index e47946d0e7,4c5de0960b..e112545f71
--- a/setup.c
+++ b/setup.c
@@@ -1230,13 -1176,21 +1230,20 @@@ static int safe_directory_cb(const cha
  	} else if (!strcmp(value, "*")) {
  		data->is_safe = 1;
  	} else {
- 		char *interpolated = NULL;
- 
- 		if (!git_config_pathname(&interpolated, key, value) &&
- 		    !fspathcmp(data->path, interpolated ? interpolated : value))
- 			data->is_safe = 1;
- 
- 		free(interpolated);
 ...


Notice that in the original, the code is prepared for the case where
interpolated is NULL when fspathcmp() needs to use it or value,
which is when git_config_pathname() returned 0/success.

It came from 8959555c (setup_git_directory(): add an owner check for
the top-level directory, 2022-03-02) that introduced the
safe.directory feature:

+		if (!git_config_pathname(&interpolated, key, value) &&
+		    !fspathcmp(data->path, interpolated ? interpolated : value))
+			data->is_safe = 1;

where it shared the same assumption.

safe.directory: preliminary clean-up

[Junio C Hamano]

The paths given in the safe.directory configuration variable are
allowed to contain "~user" (which interpolates to user's home
directory) and "%(prefix)" (which interpolates to the installation
location in RUNTIME_PREFIX-enabled builds, and a call to the
git_config_pathname() function is tasked to obtain a copy of the
path with these constructs interpolated.

The function, when it succeeds, always yields an allocated string in
the location given as the out-parameter; even when there is nothing
to interpolate in the original, a literal copy is made.  The code
path that contains this caller somehow made two contradicting and
incorrect assumptions of the behaviour when there is no need for
interpolation, and was written with extra defensiveness against
two phantom risks that do not exist.

One wrong assumption was that the function might yield NULL when
there is no interpolation.  This led to the use of an extra "check"
variable, conditionally holding either the interpolated or the
original string.  The assumption was with us since 8959555c
(setup_git_directory(): add an owner check for the top-level
directory, 2022-03-02) originally introduced the safe.directory
feature.

Another wrong assumption was that the function might yield the same
pointer as the input when there is no interpolation.  This led to a
conditional free'ing of the interpolated copy, that the conditional
never skipped, as we always received an allocated string.

Simplify the code by removing the extra defensiveness.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git c/setup.c w/setup.c
index d458edcc02..3177d010d1 100644
--- c/setup.c
+++ w/setup.c
@@ -1235,17 +1235,15 @@ static int safe_directory_cb(const char *key, const char *value,
 		char *allowed = NULL;
 
 		if (!git_config_pathname(&allowed, key, value)) {
-			const char *check = allowed ? allowed : value;
-			if (ends_with(check, "/*")) {
-				size_t len = strlen(check);
-				if (!fspathncmp(check, data->path, len - 1))
+			if (ends_with(allowed, "/*")) {
+				size_t len = strlen(allowed);
+				if (!fspathncmp(allowed, data->path, len - 1))
 					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, check)) {
+			} else if (!fspathcmp(data->path, allowed)) {
 				data->is_safe = 1;
 			}
-		}
-		if (allowed != value)
 			free(allowed);
+		}
 	}
 
 	return 0;

[PATCH v4 0/4] safe.directory clean-up

[Junio C Hamano]

    Changes since v3:

     - A preliminary patch to fix overly defensive (mis)uses of the
       result from git_config_pathname() API call has been added.

     - "to_free" variable that did not have the corresponding underlying
       variable that may or may not be allocated has been renamed.

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@gmail.com/

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 (4):
  safe.directory: preliminary clean-up
  safe.directory: normalize the checked path
  safe.directory: normalize the configured path
  safe.directory: setting safe.directory="." allows the "current" directory

 setup.c                   |  58 ++++++++++---
 t/t0033-safe-directory.sh | 178 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 225 insertions(+), 11 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  baed16ada3 safe.directory: preliminary clean-up
1:  7da381eef6 = 2:  2ac2407c22 safe.directory: normalize the checked path
2:  1e872df885 ! 3:  747120dcd7 safe.directory: normalize the configured path
    @@ Commit message
     
      ## setup.c ##
     @@ setup.c: static int safe_directory_cb(const char *key, const char *value,
    + 		char *allowed = NULL;
      
      		if (!git_config_pathname(&allowed, key, value)) {
    - 			const char *check = allowed ? allowed : value;
    --			if (ends_with(check, "/*")) {
    --				size_t len = strlen(check);
    --				if (!fspathncmp(check, data->path, len - 1))
    -+			char *to_free = NULL;
    +-			if (ends_with(allowed, "/*")) {
    +-				size_t len = strlen(allowed);
    +-				if (!fspathncmp(allowed, data->path, len - 1))
    ++			char *normalized = NULL;
     +
     +			/*
     +			 * Setting safe.directory to a non-absolute path
    @@ setup.c: static int safe_directory_cb(const char *key, const char *value,
     +			 * level of a repository, then it is OK", which is
     +			 * slightly tighter than "*" that allows discovery.
     +			 */
    -+			if (!is_absolute_path(check) && strcmp(check, ".")) {
    ++			if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
     +				warning(_("safe.directory '%s' not absolute"),
    -+					check);
    ++					allowed);
     +				goto next;
     +			}
     +
    @@ setup.c: static int safe_directory_cb(const char *key, const char *value,
     +			 * 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)
    ++			normalized = real_pathdup(allowed, 0);
    ++			if (!normalized)
     +				goto next;
    -+			if (ends_with(to_free, "/*")) {
    -+				size_t len = strlen(to_free);
    -+				if (!fspathncmp(to_free, data->path, len - 1))
    ++
    ++			if (ends_with(normalized, "/*")) {
    ++				size_t len = strlen(normalized);
    ++				if (!fspathncmp(normalized, data->path, len - 1))
      					data->is_safe = 1;
    --			} else if (!fspathcmp(data->path, check)) {
    -+			} else if (!fspathcmp(data->path, to_free)) {
    +-			} else if (!fspathcmp(data->path, allowed)) {
    ++			} else if (!fspathcmp(data->path, normalized)) {
      				data->is_safe = 1;
      			}
    -+			free(to_free);
    - 		}
    -+	next:
    - 		if (allowed != value)
    ++		next:
    ++			free(normalized);
      			free(allowed);
    + 		}
      	}
     
      ## t/t0033-safe-directory.sh ##
3:  53605f4e20 = 4:  5d0cd13e34 safe.directory: setting safe.directory="." allows the "current" directory
-- 
2.46.0-77-g633c50689c

[PATCH v4 1/4] safe.directory: preliminary clean-up

[Junio C Hamano]

The paths given in the safe.directory configuration variable are
allowed to contain "~user" (which interpolates to user's home
directory) and "%(prefix)" (which interpolates to the installation
location in RUNTIME_PREFIX-enabled builds, and a call to the
git_config_pathname() function is tasked to obtain a copy of the
path with these constructs interpolated.

The function, when it succeeds, always yields an allocated string in
the location given as the out-parameter; even when there is nothing
to interpolate in the original, a literal copy is made.  The code
path that contains this caller somehow made two contradicting and
incorrect assumptions of the behaviour when there is no need for
interpolation, and was written with extra defensiveness against
two phantom risks that do not exist.

One wrong assumption was that the function might yield NULL when
there is no interpolation.  This led to the use of an extra "check"
variable, conditionally holding either the interpolated or the
original string.  The assumption was with us since 8959555c
(setup_git_directory(): add an owner check for the top-level
directory, 2022-03-02) originally introduced the safe.directory
feature.

Another wrong assumption was that the function might yield the same
pointer as the input when there is no interpolation.  This led to a
conditional free'ing of the interpolated copy, that the conditional
never skipped, as we always received an allocated string.

Simplify the code by removing the extra defensiveness.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/setup.c b/setup.c
index d458edcc02..3177d010d1 100644
--- a/setup.c
+++ b/setup.c
@@ -1235,17 +1235,15 @@ static int safe_directory_cb(const char *key, const char *value,
 		char *allowed = NULL;
 
 		if (!git_config_pathname(&allowed, key, value)) {
-			const char *check = allowed ? allowed : value;
-			if (ends_with(check, "/*")) {
-				size_t len = strlen(check);
-				if (!fspathncmp(check, data->path, len - 1))
+			if (ends_with(allowed, "/*")) {
+				size_t len = strlen(allowed);
+				if (!fspathncmp(allowed, data->path, len - 1))
 					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, check)) {
+			} else if (!fspathcmp(data->path, allowed)) {
 				data->is_safe = 1;
 			}
-		}
-		if (allowed != value)
 			free(allowed);
+		}
 	}
 
 	return 0;
-- 
2.46.0-77-g633c50689c

[PATCH v4 2/4] safe.directory: normalize the checked path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/".

A request to check if /home/u/repo is safe would be rejected if the
safe.directory configuration allows /home/u/repository/ but not its
alias /home/u/repo/.  Normalize the path being checked before
comparing with safe.directory value(s).

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 16 ++++++++---
 t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 3177d010d1..54cce7219b 100644
--- a/setup.c
+++ b/setup.c
@@ -1215,7 +1215,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 }
 
 struct safe_directory_data {
-	const char *path;
+	char *path;
 	int is_safe;
 };
 
@@ -1261,9 +1261,7 @@ static int ensure_valid_ownership(const char *gitfile,
 				  const char *worktree, const char *gitdir,
 				  struct strbuf *report)
 {
-	struct safe_directory_data data = {
-		.path = worktree ? worktree : gitdir
-	};
+	struct safe_directory_data data = { 0 };
 
 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
 	    (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
@@ -1271,6 +1269,15 @@ static int ensure_valid_ownership(const char *gitfile,
 	    (!gitdir || is_path_owned_by_current_user(gitdir, report)))
 		return 1;
 
+	/*
+	 * normalize the data.path for comparison with normalized paths
+	 * that come from the configuration file.  The path is unsafe
+	 * if it cannot be normalized.
+	 */
+	data.path = real_pathdup(worktree ? worktree : gitdir, 0);
+	if (!data.path)
+		return 0;
+
 	/*
 	 * data.path is the "path" that identifies the repository and it is
 	 * constant regardless of what failed above. data.is_safe should be
@@ -1278,6 +1285,7 @@ static int ensure_valid_ownership(const char *gitfile,
 	 */
 	git_protected_config(safe_directory_cb, &data);
 
+	free(data.path);
 	return data.is_safe;
 }
 
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 5fe61f1291..07ac0f9a01 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -119,4 +119,61 @@ test_expect_success 'local clone of unowned repo accepted in safe directory' '
 	test_path_is_dir target
 '
 
+test_expect_success SYMLINKS 'checked paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository"
+	) &&
+	git -C repository for-each-ref &&
+	git -C repository/ for-each-ref &&
+	git -C repo for-each-ref &&
+	git -C repo/ for-each-ref &&
+	test_must_fail git -C repository/.git for-each-ref &&
+	test_must_fail git -C repository/.git/ for-each-ref &&
+	test_must_fail git -C repo/.git for-each-ref &&
+	test_must_fail git -C repo/.git/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'checked leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repository/*"
+	) &&
+	git -C repository/s for-each-ref &&
+	git -C repository/s/ for-each-ref &&
+	git -C repo/s for-each-ref &&
+	git -C repo/s/ for-each-ref &&
+	git -C repository/s/.git for-each-ref &&
+	git -C repository/s/.git/ for-each-ref &&
+	git -C repo/s/.git for-each-ref &&
+	git -C repo/s/.git/ for-each-ref
+'
+
 test_done
-- 
2.46.0-77-g633c50689c

[PATCH v4 3/4] safe.directory: normalize the configured path

[Junio C Hamano]

The pathname of a repository comes from getcwd() and it could be a
path aliased via symbolic links, e.g., the real directory may be
/home/u/repository but a symbolic link /home/u/repo may point at it,
and the clone request may come as "git clone file:///home/u/repo/"

A request to check if /home/u/repository is safe would be rejected
if the safe.directory configuration allows /home/u/repo/ but not its
alias /home/u/repository/.  Normalize the paths configured for the
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@gmail.com>).

Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c                   | 38 +++++++++++++++++++++++---
 t/t0033-safe-directory.sh | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 54cce7219b..5f81d9fac0 100644
--- a/setup.c
+++ b/setup.c
@@ -1235,13 +1235,43 @@ static int safe_directory_cb(const char *key, const char *value,
 		char *allowed = NULL;
 
 		if (!git_config_pathname(&allowed, key, value)) {
-			if (ends_with(allowed, "/*")) {
-				size_t len = strlen(allowed);
-				if (!fspathncmp(allowed, data->path, len - 1))
+			char *normalized = NULL;
+
+			/*
+			 * 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(allowed) && strcmp(allowed, ".")) {
+				warning(_("safe.directory '%s' not absolute"),
+					allowed);
+				goto next;
+			}
+
+			/*
+			 * 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.
+			 */
+			normalized = real_pathdup(allowed, 0);
+			if (!normalized)
+				goto next;
+
+			if (ends_with(normalized, "/*")) {
+				size_t len = strlen(normalized);
+				if (!fspathncmp(normalized, data->path, len - 1))
 					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, allowed)) {
+			} else if (!fspathcmp(data->path, normalized)) {
 				data->is_safe = 1;
 			}
+		next:
+			free(normalized);
 			free(allowed);
 		}
 	}
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 07ac0f9a01..ea74657255 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -176,4 +176,61 @@ test_expect_success SYMLINKS 'checked leading paths are normalized' '
 	git -C repo/s/.git/ for-each-ref
 '
 
+test_expect_success SYMLINKS 'configured paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	git init repository &&
+	ln -s repository repo &&
+	(
+		cd repository &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo"
+	) &&
+	git -C repository for-each-ref &&
+	git -C repository/ for-each-ref &&
+	git -C repo for-each-ref &&
+	git -C repo/ for-each-ref &&
+	test_must_fail git -C repository/.git for-each-ref &&
+	test_must_fail git -C repository/.git/ for-each-ref &&
+	test_must_fail git -C repo/.git for-each-ref &&
+	test_must_fail git -C repo/.git/ for-each-ref
+'
+
+test_expect_success SYMLINKS 'configured leading paths are normalized' '
+	test_when_finished "rm -rf repository; rm -f repo" &&
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global --unset-all safe.directory
+	) &&
+	mkdir -p repository &&
+	git init repository/s &&
+	ln -s repository repo &&
+	(
+		cd repository/s &&
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		test_commit sample
+	) &&
+
+	(
+		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config --global safe.directory "$(pwd)/repo/*"
+	) &&
+	git -C repository/s for-each-ref &&
+	git -C repository/s/ for-each-ref &&
+	git -C repository/s/.git for-each-ref &&
+	git -C repository/s/.git/ for-each-ref &&
+	git -C repo/s for-each-ref &&
+	git -C repo/s/ for-each-ref &&
+	git -C repo/s/.git for-each-ref &&
+	git -C repo/s/.git/ for-each-ref
+'
+
 test_done
-- 
2.46.0-77-g633c50689c

[PATCH v4 4/4] safe.directory: setting safe.directory="." allows the "current" directory

[Junio C Hamano]

When "git daemon" enters a repository, it chdir's to the requested
repository and then uses "." (the curent directory) to consult the
"is this repository considered safe?" when it is not owned by the
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@gmail.com>).

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@pobox.com>
---
 t/t0033-safe-directory.sh | 64 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index ea74657255..e97a84764f 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -233,4 +233,68 @@ test_expect_success SYMLINKS 'configured leading paths are normalized' '
 	git -C repo/s/.git/ for-each-ref
 '
 
+test_expect_success 'safe.directory set to a dot' '
+	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 "."
+	) &&
+	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 &&
+
+	# 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
+	# 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-77-g633c50689c

Re: [PATCH v3 2/3] safe.directory: normalize the configured path

[Jeff King]

On Tue, Jul 30, 2024 at 09:03:35AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> +			if (!is_absolute_path(check) && strcmp(check, ".")) {
> >> +				warning(_("safe.directory '%s' not absolute"),
> >> +					check);
> >> +				goto next;
> >> +			}
> >
> > This is_absolute_path() check is redundant, isn't it? If we are checking
> > for a literal ".", then we know the path must be non-absolute.
> 
> What I meant was "If it is not absolute, that is an error, but if
> the thing is a dot, that is allowed as an exception".
> 
> Is the lack of "!" confusing, I wonder?   We could rewrite it to
> make it more explicit:

Oh, right, I totally misread it. You'd think after 25+ years of writing
C that I would be able to get the strcmp() return value right in my
head... :)

So yeah, it is doing the right thing.

> 	if (is_absolute_path(check) || !strcmp(check, ".")) {
> 		; /* OK */
> 	} else {
> 		warning(_("not absolute %s"), check);
> 		goto next;
> 	}

Hmm. Yeah, that probably would have softened my confusion, but it's also
kind of hard to read. I think what you wrote originally is just fine,
and I just mis-read it.

> My earlier draft for v3 had the check for dot a lot earlier in the
> function, i.e.
> 
> 	-	} else if (!strcmp(value, "*")) {
> 	+	} else if (!strcmp(value, "*") || !strcmp(value, ".")) {
> 			data->is_safe = 1;
> 
> and this part said "If not absolute, that is an error" without
> anything about dot.
> 
> But then I changed my mind and made it unsafe to do this:
> 
> 	cd .git/refs && git -c safe.directory=. foo
> 
> as safe.directory=. means "A repository at the current directory of
> the process is allowed" and the repository in this case is not at "."
> but at "..", meaning "." is a lot stricter than "*".

I could see arguments in either direction, and I don't have a strong
opinion between them.

-Peff

Re: [PATCH v3 2/3] safe.directory: normalize the configured path

[Jeff King]

On Tue, Jul 30, 2024 at 09:22:09AM -0700, Junio C Hamano wrote:

> > Obviously not new in your patch, but maybe worth fixing while in the
> > area? I think it comes from an evil merge in b8bdb2f283 (Merge branch
> > 'jc/safe-directory-leading-path', 2024-06-12).
> 
> I think it deserves to be a separate change, probably a preliminary
> clean-up, as it predates that by a few years, and goes back to the
> initial introduction of the safe.directory feature.  The merge you
> found had this bit:

Ah, yeah, I was busy looking at the assignments and didn't notice the
ternary in the function call. So yeah, it comes from that earlier
commit, but I think it was equally useless there.

And yes, fixing it definitely should be a separate commit.

-Peff

Re: safe.directory: preliminary clean-up

[Jeff King]

On Tue, Jul 30, 2024 at 10:56:04AM -0700, Junio C Hamano wrote:

> One wrong assumption was that the function might yield NULL when
> there is no interpolation.  This led to the use of an extra "check"
> variable, conditionally holding either the interpolated or the
> original string.  The assumption was with us since 8959555c
> (setup_git_directory(): add an owner check for the top-level
> directory, 2022-03-02) originally introduced the safe.directory
> feature.
> 
> Another wrong assumption was that the function might yield the same
> pointer as the input when there is no interpolation.  This led to a
> conditional free'ing of the interpolated copy, that the conditional
> never skipped, as we always received an allocated string.

Yup. Good eyes on seeing that second one. Both look correct to me from
double-checking git_config_pathname(), and the patch itself looks good.

-Peff

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Phillip Wood]

Hi Junio

On 25/07/2024 17:11, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I think this is sensible if the key is an absolute path, if the key is
>> a relative path I think we should ignore it as it is not clear which
>> directory the user meant.
> 
> The only thing that worries me in that proposal is that doing so
> would break a configuration that used to work.  I'd rather leave
> the tightening of it to future work with its own justification.

As far as I know the only caller that tried to match a relative 
directory was enter_repo(), all of the other code paths pass an absolute 
path from getcwd(). Before this series git daemon required the relative 
directory "." to be specified in addition to the absolute path so that 
will not be broken by removing support for relative directories. Are 
there other users of enter_repo() that still rely on being able to match 
relative paths?

Best Wishes

Phillip

> Thanks.
> 
> 

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 25/07/2024 17:11, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> I think this is sensible if the key is an absolute path, if the key is
>>> a relative path I think we should ignore it as it is not clear which
>>> directory the user meant.
>> The only thing that worries me in that proposal is that doing so
>> would break a configuration that used to work.  I'd rather leave
>> the tightening of it to future work with its own justification.
>
> As far as I know the only caller that tried to match a relative
> directory was enter_repo(), all of the other code paths pass an
> absolute path from getcwd(). Before this series git daemon required
> the relative directory "." to be specified in addition to the absolute
> path so that will not be broken by removing support for relative
> directories. Are there other users of enter_repo() that still rely on
> being able to match relative paths?

Offhand I do not know of any, but no guarantees.  I am tempted to
leave it as a prerequisite task for those who want to tighten this
codepath further.

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Phillip Wood]

On 14/08/2024 18:15, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> On 25/07/2024 17:11, Junio C Hamano wrote:
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>
>>>> I think this is sensible if the key is an absolute path, if the key is
>>>> a relative path I think we should ignore it as it is not clear which
>>>> directory the user meant.
>>> The only thing that worries me in that proposal is that doing so
>>> would break a configuration that used to work.  I'd rather leave
>>> the tightening of it to future work with its own justification.
>>
>> As far as I know the only caller that tried to match a relative
>> directory was enter_repo(), all of the other code paths pass an
>> absolute path from getcwd(). Before this series git daemon required
>> the relative directory "." to be specified in addition to the absolute
>> path so that will not be broken by removing support for relative
>> directories. Are there other users of enter_repo() that still rely on
>> being able to match relative paths?
> 
> Offhand I do not know of any, but no guarantees.  I am tempted to
> leave it as a prerequisite task for those who want to tighten this
> codepath further.

Fair enough. I will note though that this change makes it much more 
likely that "safe.directory=." will match as it now behaves like "*". 
Previously it only matched when we tried to match it against "." whereas 
now it matches any directory.

Best Wishes

Phillip

Re: [PATCH v2 2/3] safe.directory: normalize the configured path

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Fair enough. I will note though that this change makes it much more
> likely that "safe.directory=." will match as it now behaves like
> "*". Previously it only matched when we tried to match it against "."
> whereas now it matches any directory.

Unless you are commenting on my earlier unpublished draft, I doubt
that it is the case.  

If I did the code analysis correctly when I responded to Peff in
https://lore.kernel.org/git/xmqq4j86g948.fsf@gitster.g/, that is.

There are some negative tests added by the series to verify that dot
is really "current directory" and not "any directory" which is what
'*' is for (and as I always tell my contributors, we should really
get in the habit of writing more negative tests, exactly for things
like this where we do not want a "feature" to trigger in cases we do
not want them to).

Ahh, if your e-mail header is correct, you are commenting on the
implementation in v2, which did not care if the input were absolute
or not?  If so, then your confusion is somewhat understandable.

The things changed before the final edition v4 happened and it is
slightly tigher than the version you are commenting on.  We refuse
to match any non-absolute path, except for ".", since v3.  But I
think even with the looser implementation of v2, the negative test
would have made sure that "." was really "current and not any".

Thanks.