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@xxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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
