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

From: Jeff King <peff@peff.net>
Date: Jul 26, 2024, 05:02 UTC
Message-ID: <20240726050253.GC642208@coredump.intra.peff.net>
In-Reply-To: <20240723021900.388020-3-gitster@pobox.com>

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

Show 10 quoted lines

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

Show 13 quoted lines

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