118 lines
4.8 KiB
Diff
118 lines
4.8 KiB
Diff
From cf8f6ce02a13f4d1979a53241afbee15a293fce9 Mon Sep 17 00:00:00 2001
|
|
From: Taylor Blau <me@ttaylorr.com>
|
|
Date: Tue, 24 Jan 2023 19:43:48 -0500
|
|
Subject: [PATCH] clone: delay picking a transport until after get_repo_path()
|
|
|
|
In the previous commit, t5619 demonstrates an issue where two calls to
|
|
`get_repo_path()` could trick Git into using its local clone mechanism
|
|
in conjunction with a non-local transport.
|
|
|
|
That sequence is:
|
|
|
|
- the starting state is that the local path https:/example.com/foo is a
|
|
symlink that points to ../../../.git/modules/foo. So it's dangling.
|
|
|
|
- get_repo_path() sees that no such path exists (because it's
|
|
dangling), and thus we do not canonicalize it into an absolute path
|
|
|
|
- because we're using --separate-git-dir, we create .git/modules/foo.
|
|
Now our symlink is no longer dangling!
|
|
|
|
- we pass the url to transport_get(), which sees it as an https URL.
|
|
|
|
- we call get_repo_path() again, on the url. This second call was
|
|
introduced by f38aa83f9a (use local cloning if insteadOf makes a
|
|
local URL, 2014-07-17). The idea is that we want to pull the url
|
|
fresh from the remote.c API, because it will apply any aliases.
|
|
|
|
And of course now it sees that there is a local file, which is a
|
|
mismatch with the transport we already selected.
|
|
|
|
The issue in the above sequence is calling `transport_get()` before
|
|
deciding whether or not the repository is indeed local, and not passing
|
|
in an absolute path if it is local.
|
|
|
|
This is reminiscent of a similar bug report in [1], where it was
|
|
suggested to perform the `insteadOf` lookup earlier. Taking that
|
|
approach may not be as straightforward, since the intent is to store the
|
|
original URL in the config, but to actually fetch from the insteadOf
|
|
one, so conflating the two early on is a non-starter.
|
|
|
|
Note: we pass the path returned by `get_repo_path(remote->url[0])`,
|
|
which should be the same as `repo_name` (aside from any `insteadOf`
|
|
rewrites).
|
|
|
|
We *could* pass `absolute_pathdup()` of the same argument, which
|
|
86521acaca (Bring local clone's origin URL in line with that of a remote
|
|
clone, 2008-09-01) indicates may differ depending on the presence of
|
|
".git/" for a non-bare repo. That matters for forming relative submodule
|
|
paths, but doesn't matter for the second call, since we're just feeding
|
|
it to the transport code, which is fine either way.
|
|
|
|
[1]: https://lore.kernel.org/git/CAMoD=Bi41mB3QRn3JdZL-FGHs4w3C2jGpnJB-CqSndO7FMtfzA@mail.gmail.com/
|
|
|
|
Signed-off-by: Jeff King <peff@peff.net>
|
|
Signed-off-by: Taylor Blau <me@ttaylorr.com>
|
|
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
---
|
|
builtin/clone.c | 8 ++++----
|
|
t/t5619-clone-local-ambiguous-transport.sh | 15 +++++++++++----
|
|
2 files changed, 15 insertions(+), 8 deletions(-)
|
|
|
|
diff --git a/builtin/clone.c b/builtin/clone.c
|
|
index e626073b1f..c042b2e256 100644
|
|
--- a/builtin/clone.c
|
|
+++ b/builtin/clone.c
|
|
@@ -1201,10 +1201,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
|
|
refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
|
|
branch_top.buf);
|
|
|
|
- transport = transport_get(remote, remote->url[0]);
|
|
- transport_set_verbosity(transport, option_verbosity, option_progress);
|
|
- transport->family = family;
|
|
-
|
|
path = get_repo_path(remote->url[0], &is_bundle);
|
|
is_local = option_local != 0 && path && !is_bundle;
|
|
if (is_local) {
|
|
@@ -1224,6 +1220,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
|
|
}
|
|
if (option_local > 0 && !is_local)
|
|
warning(_("--local is ignored"));
|
|
+
|
|
+ transport = transport_get(remote, path ? path : remote->url[0]);
|
|
+ transport_set_verbosity(transport, option_verbosity, option_progress);
|
|
+ transport->family = family;
|
|
transport->cloning = 1;
|
|
|
|
if (is_bundle) {
|
|
diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh
|
|
index 7ebd31a150..cce62bf78d 100755
|
|
--- a/t/t5619-clone-local-ambiguous-transport.sh
|
|
+++ b/t/t5619-clone-local-ambiguous-transport.sh
|
|
@@ -53,11 +53,18 @@ test_expect_success 'setup' '
|
|
git -C "$REPO" update-server-info
|
|
'
|
|
|
|
-test_expect_failure 'ambiguous transport does not lead to arbitrary file-inclusion' '
|
|
+test_expect_success 'ambiguous transport does not lead to arbitrary file-inclusion' '
|
|
git clone malicious clone &&
|
|
- git -C clone submodule update --init &&
|
|
-
|
|
- test_path_is_missing clone/.git/modules/sub/objects/secret
|
|
+ test_must_fail git -C clone submodule update --init 2>err &&
|
|
+
|
|
+ test_path_is_missing clone/.git/modules/sub/objects/secret &&
|
|
+ # We would actually expect "transport .file. not allowed" here,
|
|
+ # but due to quirks of the URL detection in Git, we mis-parse
|
|
+ # the absolute path as a bogus URL and die before that step.
|
|
+ #
|
|
+ # This works for now, and if we ever fix the URL detection, it
|
|
+ # is OK to change this to detect the transport error.
|
|
+ grep "protocol .* is not supported" err
|
|
'
|
|
|
|
test_done
|
|
--
|
|
2.27.0
|
|
|