Hi Junio
Thanks for picking this up, I think this looks like a good approach
apart from resolving relative entries in safe.directory which is
intrinsically unsafe as we don't know which directory the user wants to
consider safe. With these changes it should not be necessary to add "."
to safe.directory to get "git daemon" to work and all the other code
paths use an absolute path from getcwd() so I don't think there is any
need to support relative directories.
I'll be off the list for the next couple of weeks
Best Wishes
Phillip
On 23/07/2024 03:18, Junio C Hamano wrote:
Changes since v1:
- two "extra" patches for optimization that are unnecessary have
been dropped.
- a patch to add test for safe.directory="." use case has been
added.
Recently we discussed what we should do when either the path
configured in the safe.directory configuration 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/6d5b75a6-639d-429b-bd37-232fc6f475af@xxxxxxxxx/
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).
- The third patch only adds a test to illustrate what happens when
the safe.directory configuration is set to ".", which was a
plausible setting for those who run "git daemon". The
normalization done by the first two patches is sufficient to make
this just work.
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 | 28 ++++++--
t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++
2 files changed, 170 insertions(+), 4 deletions(-)
Range-diff against v1:
1: 20e1160946 ! 1: 86d5c83ee7 safe.directory: normalize the checked path
@@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+ git config --global safe.directory "$(pwd)/repository"
+ ) &&
-+ git -C repository/ for-each-ref >/dev/null &&
-+ git -C repo/ for-each-ref
++ 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 'checked leading paths are normalized' '
@@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+ git config --global safe.directory "$(pwd)/repository/*"
+ ) &&
-+ git -C repository/s for-each-ref >/dev/null &&
-+ git -C repo/s for-each-ref
++ git -C repository/s for-each-ref &&
++ git -C repository/s/ for-each-ref &&
++ git -C repo/s for-each-ref &&
++ git -C repo/s/ for-each-ref &&
++ git -C repository/s/.git for-each-ref &&
++ git -C repository/s/.git/ for-each-ref &&
++ git -C repo/s/.git for-each-ref &&
++ git -C repo/s/.git/ for-each-ref
+'
+
test_done
2: 06de9038b7 ! 2: c4998076da safe.directory: normalize the configured path
@@ setup.c: static int safe_directory_cb(const char *key, const char *value,
## t/t0033-safe-directory.sh ##
@@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' '
- git -C repo/s for-each-ref
+ git -C repo/s/.git/ for-each-ref
'
+test_expect_success SYMLINKS 'configured paths are normalized' '
@@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+ git config --global safe.directory "$(pwd)/repo"
+ ) &&
-+ git -C repository/ for-each-ref >/dev/null &&
-+ git -C repo/ for-each-ref
++ 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' '
@@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
+ git init repository/s &&
+ ln -s repository repo &&
+ (
-+ cd repository &&
++ cd repository/s &&
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+ test_commit sample
+ ) &&
@@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
+ sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+ git config --global safe.directory "$(pwd)/repo/*"
+ ) &&
-+ git -C repository/s for-each-ref >/dev/null &&
-+ git -C repo/s for-each-ref
++ 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
-: ---------- > 3: ffc3f7364e safe.directory: setting safe.directory="." allows the "current" directory
