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

Message

Junio C Hamano July 30, 2024, 6:43 p.m. UTC

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/[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 (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