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

Message

Junio C Hamano July 30, 2024, 1:10 a.m. UTC

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. <[e-mail]>).

 - 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. <[e-mail]>), except for
   ".", which was once advertised as a valid workaround on the list.
   (cf. <[e-mail]>).

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/[e-mail]/

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. <[e-mail]>).
    +
         Suggested-by: Phillip Wood <[e-mail]>
    +    Helped-by: Jeff King <[e-mail]>
         Signed-off-by: Junio C Hamano <[e-mail]>
     
      ## 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. <[e-mail]>).
    +
    +    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 <[e-mail]>
     
    @@ 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