Re: [PATCH v2 2/3] safe.directory: normalize the configured path
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 25, 2024, 09:45 UTC
- Message-ID
- <5332f244-7476-492a-a797-2ef7ba73f490@gmail.com>
- In-Reply-To
- <20240723021900.388020-3-gitster@pobox.com>
Hi Junio
On 23/07/2024 03:18, 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.
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
Show 104 quoted lines
> 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