{"thread":{"id":"62493","subject":"[PATCH 0/1] Restore the ability to clone repositories owned by another user","startedAt":"2024-11-15T00:54:12Z","lastAt":"2025-03-31T21:54:06Z","messageCount":8,"participants":["brian m. carlson","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"507304","messageId":"20241115005404.3747302-1-sandals@crustytoothpaste.net","threadId":"62493","inReplyTo":null,"subject":"[PATCH 0/1] Restore the ability to clone repositories owned by another user","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-15T00:54:03Z","receivedAt":"2024-11-15T00:54:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"For a long time, we've told users that the only safe way to operate on\nan untrusted repository is to clone or fetch from it.  We've even\nmentioned this policy in a variety of places in our documentation.\n\nHowever, f4aa8c8bb1 (\"fetch/clone: detect dubious ownership of local\nrepositories\", 2024-04-10), this changed in an attempt to make things\nmore secure.  That broke a lot of user use cases, which have been\nreported to the list.\n\nBecause our security model hasn't changed and it's still safe to clone\nor fetch from an untrusted repository, let's revert a portion of that\nchange to allow us to clone and fetch from repositories owned by a\ndifferent user.  If a malicious repository were a problem for\nupload-pack, that would probably also be exploitable on major forges,\nand if it were a problem on the client side, then we'd also have a\nproblem with untrusted HTTPS remotes, so we're not really adding any\nsecurity risk here.\n\nThis matter was discussed extensively in the thread starting at\nhttps://lore.kernel.org/git/ZqUc8DJ1uKcHYlcy@imp.flyn.org/.\n\nNote that I haven't signed off on this patch because it's based on one\nfrom Junio and I haven't gotten his sign-off yet.  It's fine to add mine\nonce he's added his.\n\nbrian m. carlson (1):\n  Allow cloning from repositories owned by another user\n\n Documentation/git-clone.txt   |  9 +++++++++\n builtin/upload-pack.c         |  5 ++++-\n daemon.c                      |  6 ++++--\n path.c                        | 10 ++++++----\n path.h                        | 17 ++++++++++++++++-\n t/t0411-clone-from-partial.sh |  3 ---\n t/t5605-clone-local.sh        | 10 ++++++++++\n 7 files changed, 49 insertions(+), 11 deletions(-)\n\n"},{"id":"507305","messageId":"20241115005404.3747302-2-sandals@crustytoothpaste.net","threadId":"62493","inReplyTo":"20241115005404.3747302-1-sandals@crustytoothpaste.net","subject":"[PATCH 1/1] Allow cloning from repositories owned by another user","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-15T00:54:04Z","receivedAt":"2024-11-15T00:54:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Historically, Git has allowed users to clone from an untrusted\nrepository, and we have documented that this is safe to do so:\n\n    `upload-pack` tries to avoid any dangerous configuration options or\n    hooks from the repository it's serving, making it safe to clone an\n    untrusted directory and run commands on the resulting clone.\n\nHowever, this was broken by f4aa8c8bb1 (\"fetch/clone: detect dubious\nownership of local repositories\", 2024-04-10) in an attempt to make\nthings more secure.  That change resulted in a variety of problems when\ncloning locally and over SSH, but it did not change the stated security\nboundary.  Because the security boundary has not changed, it is safe to\nadjust part of the code that patch introduced.\n\nTo do that and restore the previous functionality, adjust enter_repo to\ntake two flags instead of one.\n\nThe two bits are\n\n - ENTER_REPO_STRICT: callers that require exact paths (as opposed\n   to allowing known suffixes like \".git\", \".git/.git\" to be\n   omitted) can set this bit.  Corresponds to the \"strict\" parameter\n   that the flags word replaces.\n\n - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without\n   ownership check can set this bit.\n\nThe former is --strict-paths option of \"git daemon\".  The latter is\nset only by upload-pack, which honors the claimed security boundary.\n\nNote that local clones across ownership boundaries require --no-local so\nthat upload-pack is used.  Document this fact in the manual page and\nprovide an example.\n\nThis patch was based on one written by Junio C Hamano.\n---\n Documentation/git-clone.txt   |  9 +++++++++\n builtin/upload-pack.c         |  5 ++++-\n daemon.c                      |  6 ++++--\n path.c                        | 10 ++++++----\n path.h                        | 17 ++++++++++++++++-\n t/t0411-clone-from-partial.sh |  3 ---\n t/t5605-clone-local.sh        | 10 ++++++++++\n 7 files changed, 49 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 7acb4cb176..de8d8f5893 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -63,6 +63,9 @@ symbolic link, the clone will fail. This is a security measure to\n prevent the unintentional copying of files by dereferencing the symbolic\n links.\n +\n+This option does not work with repositories owned by other users for security\n+reasons, and `--no-local` must be specified for the clone to succeed.\n++\n *NOTE*: this operation can race with concurrent modification to the\n source repository, similar to running `cp -r <src> <dst>` while modifying\n _<src>_.\n@@ -384,6 +387,12 @@ $ cd my-linux\n $ git clone --bare -l /home/proj/.git /pub/scm/proj.git\n ------------\n \n+* Clone a local repository from a different user:\n++\n+------------\n+$ git clone --no-local /home/otheruser/proj.git /pub/scm/proj.git\n+------------\n+\n CONFIGURATION\n -------------\n \ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 3b6c83fbce..dd63d6eadf 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -39,6 +39,7 @@ int cmd_upload_pack(int argc,\n \t\t\t    N_(\"interrupt transfer after <n> seconds of inactivity\")),\n \t\tOPT_END()\n \t};\n+\tunsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;\n \n \tpacket_trace_identity(\"upload-pack\");\n \tdisable_replace_refs();\n@@ -54,7 +55,9 @@ int cmd_upload_pack(int argc,\n \n \tdir = argv[0];\n \n-\tif (!enter_repo(dir, strict))\n+\tif (strict)\n+\t\tenter_repo_flags |= ENTER_REPO_STRICT;\n+\tif (!enter_repo(dir, enter_repo_flags))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n \tswitch (determine_protocol_version_server()) {\ndiff --git a/daemon.c b/daemon.c\nindex a40e435c63..756c3f0b02 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -152,6 +152,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \tsize_t rlen;\n \tconst char *path;\n \tconst char *dir;\n+\tunsigned enter_repo_flags;\n \n \tdir = directory;\n \n@@ -242,14 +243,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\tdir = rpath;\n \t}\n \n-\tpath = enter_repo(dir, strict_paths);\n+\tenter_repo_flags = strict_paths ? ENTER_REPO_STRICT : 0;\n+\tpath = enter_repo(dir, enter_repo_flags);\n \tif (!path && base_path && base_path_relaxed) {\n \t\t/*\n \t\t * if we fail and base_path_relaxed is enabled, try without\n \t\t * prefixing the base path\n \t\t */\n \t\tdir = directory;\n-\t\tpath = enter_repo(dir, strict_paths);\n+\t\tpath = enter_repo(dir, enter_repo_flags);\n \t}\n \n \tif (!path) {\ndiff --git a/path.c b/path.c\nindex 93491bab14..4dcf3c8d40 100644\n--- a/path.c\n+++ b/path.c\n@@ -684,7 +684,7 @@ char *interpolate_path(const char *path, int real_home)\n  * links.  User relative paths are also returned as they are given,\n  * except DWIM suffixing.\n  */\n-const char *enter_repo(const char *path, int strict)\n+const char *enter_repo(const char *path, unsigned flags)\n {\n \tstatic struct strbuf validated_path = STRBUF_INIT;\n \tstatic struct strbuf used_path = STRBUF_INIT;\n@@ -692,7 +692,7 @@ const char *enter_repo(const char *path, int strict)\n \tif (!path)\n \t\treturn NULL;\n \n-\tif (!strict) {\n+\tif (!(flags & ENTER_REPO_STRICT)) {\n \t\tstatic const char *suffix[] = {\n \t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n \t\t};\n@@ -736,7 +736,8 @@ const char *enter_repo(const char *path, int strict)\n \t\tif (!suffix[i])\n \t\t\treturn NULL;\n \t\tgitfile = read_gitfile(used_path.buf);\n-\t\tdie_upon_dubious_ownership(gitfile, NULL, used_path.buf);\n+\t\tif (!(flags & ENTER_REPO_ANY_OWNER_OK))\n+\t\t\tdie_upon_dubious_ownership(gitfile, NULL, used_path.buf);\n \t\tif (gitfile) {\n \t\t\tstrbuf_reset(&used_path);\n \t\t\tstrbuf_addstr(&used_path, gitfile);\n@@ -747,7 +748,8 @@ const char *enter_repo(const char *path, int strict)\n \t}\n \telse {\n \t\tconst char *gitfile = read_gitfile(path);\n-\t\tdie_upon_dubious_ownership(gitfile, NULL, path);\n+\t\tif (!(flags & ENTER_REPO_ANY_OWNER_OK))\n+\t\t\tdie_upon_dubious_ownership(gitfile, NULL, path);\n \t\tif (gitfile)\n \t\t\tpath = gitfile;\n \t\tif (chdir(path))\ndiff --git a/path.h b/path.h\nindex e91d19fff6..5f6c85e5f8 100644\n--- a/path.h\n+++ b/path.h\n@@ -156,7 +156,22 @@ int calc_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\n \n char *interpolate_path(const char *path, int real_home);\n-const char *enter_repo(const char *path, int strict);\n+\n+/* The bits are as follows:\n+ *\n+ * - ENTER_REPO_STRICT: callers that require exact paths (as opposed\n+ *   to allowing known suffixes like \".git\", \".git/.git\" to be\n+ *   omitted) can set this bit.\n+ *\n+ * - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without\n+ *   ownership check can set this bit.\n+ */\n+enum {\n+\tENTER_REPO_STRICT = (1<<0),\n+\tENTER_REPO_ANY_OWNER_OK = (1<<1),\n+};\n+\n+const char *enter_repo(const char *path, unsigned flags);\n const char *remove_leading_path(const char *in, const char *prefix);\n const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);\n int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);\ndiff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh\nindex 932bf2067d..2aff6261e4 100755\n--- a/t/t0411-clone-from-partial.sh\n+++ b/t/t0411-clone-from-partial.sh\n@@ -29,7 +29,6 @@ test_expect_success 'local clone must not fetch from promisor remote and execute\n \ttest_must_fail git clone \\\n \t\t--upload-pack=\"GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack\" \\\n \t\tevil clone1 2>err &&\n-\ttest_grep \"detected dubious ownership\" err &&\n \ttest_grep ! \"fake-upload-pack running\" err &&\n \ttest_path_is_missing script-executed\n '\n@@ -39,7 +38,6 @@ test_expect_success 'clone from file://... must not fetch from promisor remote a\n \ttest_must_fail git clone \\\n \t\t--upload-pack=\"GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack\" \\\n \t\t\"file://$(pwd)/evil\" clone2 2>err &&\n-\ttest_grep \"detected dubious ownership\" err &&\n \ttest_grep ! \"fake-upload-pack running\" err &&\n \ttest_path_is_missing script-executed\n '\n@@ -49,7 +47,6 @@ test_expect_success 'fetch from file://... must not fetch from promisor remote a\n \ttest_must_fail git fetch \\\n \t\t--upload-pack=\"GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack\" \\\n \t\t\"file://$(pwd)/evil\" 2>err &&\n-\ttest_grep \"detected dubious ownership\" err &&\n \ttest_grep ! \"fake-upload-pack running\" err &&\n \ttest_path_is_missing script-executed\n '\ndiff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\nindex d9a320abd2..286099390c 100755\n--- a/t/t5605-clone-local.sh\n+++ b/t/t5605-clone-local.sh\n@@ -154,6 +154,16 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '\n \t! repo_is_hardlinked force-nonlocal\n '\n \n+test_expect_success 'cloning a local path with --no-local from a different user succeeds' '\n+\tgit clone --upload-pack=\"GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack\" \\\n+\t\t--no-local a nonlocal-otheruser 2>err &&\n+\t! repo_is_hardlinked nonlocal-otheruser &&\n+\t# Verify that this is a git repository.\n+\tgit -C nonlocal-otheruser rev-parse --show-toplevel &&\n+\t! test_grep \"detected dubious ownership\" err\n+\n+'\n+\n test_expect_success 'cloning locally respects \"-u\" for fetching refs' '\n \ttest_must_fail git clone --bare -u false a should_not_work.git\n '\n"},{"id":"507307","messageId":"xmqqr07d47sn.fsf@gitster.g","threadId":"62493","inReplyTo":"20241115005404.3747302-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/1] Restore the ability to clone repositories owned by another user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-15T01:14:48Z","receivedAt":"2024-11-15T01:14:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> For a long time, we've told users that the only safe way to operate on\n> an untrusted repository is to clone or fetch from it.  We've even\n> mentioned this policy in a variety of places in our documentation.\n>\n> However, f4aa8c8bb1 (\"fetch/clone: detect dubious ownership of local\n> repositories\", 2024-04-10), this changed in an attempt to make things\n> more secure.  That broke a lot of user use cases, which have been\n> reported to the list.\n>\n> Because our security model hasn't changed and it's still safe to clone\n> or fetch from an untrusted repository, let's revert a portion of that\n> change to allow us to clone and fetch from repositories owned by a\n> different user.  If a malicious repository were a problem for\n> upload-pack, that would probably also be exploitable on major forges,\n> and if it were a problem on the client side, then we'd also have a\n> problem with untrusted HTTPS remotes, so we're not really adding any\n> security risk here.\n\nNice.  Better late than never.\n\n> This matter was discussed extensively in the thread starting at\n> https://lore.kernel.org/git/ZqUc8DJ1uKcHYlcy@imp.flyn.org/.\n\n> Note that I haven't signed off on this patch because it's based on one\n> from Junio and I haven't gotten his sign-off yet.  It's fine to add mine\n> once he's added his.\n\nYou can forge my sign-off on the old patch ;-)\n\nThe proposed commit log of the patch makes me wonder what should\nhappen when neither of the two bits is set.  Not strict, but we do\nnot allow ourselves to enter a random repo owned by a stranger.  It\nturns out that \"strict\" has nothing to do with this lifting of\nexcess ownership check, but the dwimming done by suffixing .git,\netc. to the given pathnames, so there is nothing strange going on.\n\nThanks.\n"},{"id":"507308","messageId":"ZzarpTaoooBOBohZ@tapette.crustytoothpaste.net","threadId":"62493","inReplyTo":"xmqqr07d47sn.fsf@gitster.g","subject":"Re: [PATCH 0/1] Restore the ability to clone repositories owned by another user","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-15T02:02:13Z","receivedAt":"2024-11-15T02:02:15Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-11-15 at 01:14:48, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > For a long time, we've told users that the only safe way to operate on\n> > an untrusted repository is to clone or fetch from it.  We've even\n> > mentioned this policy in a variety of places in our documentation.\n> >\n> > However, f4aa8c8bb1 (\"fetch/clone: detect dubious ownership of local\n> > repositories\", 2024-04-10), this changed in an attempt to make things\n> > more secure.  That broke a lot of user use cases, which have been\n> > reported to the list.\n> >\n> > Because our security model hasn't changed and it's still safe to clone\n> > or fetch from an untrusted repository, let's revert a portion of that\n> > change to allow us to clone and fetch from repositories owned by a\n> > different user.  If a malicious repository were a problem for\n> > upload-pack, that would probably also be exploitable on major forges,\n> > and if it were a problem on the client side, then we'd also have a\n> > problem with untrusted HTTPS remotes, so we're not really adding any\n> > security risk here.\n> \n> Nice.  Better late than never.\n\nYeah, I had intended to get to this sooner, but I got busy with other\nthings, and nobody got to it before me.  I had some time so I thought\nI'd send this out now so we can minimize the number of affected versions.\n\nI really appreciate you writing up the original patch for this; it was\nvery helpful and a great start.\n\n> > Note that I haven't signed off on this patch because it's based on one\n> > from Junio and I haven't gotten his sign-off yet.  It's fine to add mine\n> > once he's added his.\n> \n> You can forge my sign-off on the old patch ;-)\n\nGreat.  I suspect you'll probably pick this up as-is, but I've added\nboth our sign-offs in case we need a v2.  Let me know if you'd prefer me\nto send out the unmodified v2, and I can do that.\n\n> The proposed commit log of the patch makes me wonder what should\n> happen when neither of the two bits is set.  Not strict, but we do\n> not allow ourselves to enter a random repo owned by a stranger.  It\n> turns out that \"strict\" has nothing to do with this lifting of\n> excess ownership check, but the dwimming done by suffixing .git,\n> etc. to the given pathnames, so there is nothing strange going on.\n\nExactly.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"508153","messageId":"xmqqed2yzc5z.fsf@gitster.g","threadId":"62493","inReplyTo":"20241115005404.3747302-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/1] Restore the ability to clone repositories owned by another user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-26T07:28:40Z","receivedAt":"2024-11-26T07:28:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The breakage this fixes was originally meant as an extra \"security\"\nthing that turned out to have a larger blast radius than originally\nintended.  I was hoping that many many folks thank us for reversing\nthe course, but unfortunately we didn't hear much about this topic.\n\nUnless there are strong objections, I'll mark the topic for 'next'\nsoonish.\n\nThanks.\n"},{"id":"508302","messageId":"Z0in54Kk-5eBN_Uh@tapette.crustytoothpaste.net","threadId":"62493","inReplyTo":"xmqqed2yzc5z.fsf@gitster.g","subject":"Re: [PATCH 0/1] Restore the ability to clone repositories owned by another user","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-28T17:27:03Z","receivedAt":"2024-11-28T17:27:06Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-11-26 at 07:28:40, Junio C Hamano wrote:\n> The breakage this fixes was originally meant as an extra \"security\"\n> thing that turned out to have a larger blast radius than originally\n> intended.  I was hoping that many many folks thank us for reversing\n> the course, but unfortunately we didn't hear much about this topic.\n> \n> Unless there are strong objections, I'll mark the topic for 'next'\n> soonish.\n\nI think that will be fine, thanks.  I expect that a bunch of people who\nexperienced problems won't notice until this is in a released version,\nso we'll probably hear from them after the fact.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"515363","messageId":"Z+qVJPQ17urm06QN@szeder.dev","threadId":"62493","inReplyTo":"20241115005404.3747302-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/1] Allow cloning from repositories owned by another user","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-03-31T13:14:12Z","receivedAt":"2025-03-31T13:14:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Nov 15, 2024 at 12:54:04AM +0000, brian m. carlson wrote:\n> Historically, Git has allowed users to clone from an untrusted\n> repository, and we have documented that this is safe to do so:\n> \n>     `upload-pack` tries to avoid any dangerous configuration options or\n>     hooks from the repository it's serving, making it safe to clone an\n>     untrusted directory and run commands on the resulting clone.\n> \n> However, this was broken by f4aa8c8bb1 (\"fetch/clone: detect dubious\n> ownership of local repositories\", 2024-04-10) in an attempt to make\n> things more secure.  That change resulted in a variety of problems when\n> cloning locally and over SSH, but it did not change the stated security\n> boundary.  Because the security boundary has not changed, it is safe to\n> adjust part of the code that patch introduced.\n> \n> To do that and restore the previous functionality, adjust enter_repo to\n> take two flags instead of one.\n> \n> The two bits are\n> \n>  - ENTER_REPO_STRICT: callers that require exact paths (as opposed\n>    to allowing known suffixes like \".git\", \".git/.git\" to be\n>    omitted) can set this bit.  Corresponds to the \"strict\" parameter\n>    that the flags word replaces.\n> \n>  - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without\n>    ownership check can set this bit.\n> \n> The former is --strict-paths option of \"git daemon\".  The latter is\n> set only by upload-pack, which honors the claimed security boundary.\n> \n> Note that local clones across ownership boundaries require --no-local so\n> that upload-pack is used.  Document this fact in the manual page and\n> provide an example.\n> \n> This patch was based on one written by Junio C Hamano.\n\n\n> diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\n> index d9a320abd2..286099390c 100755\n> --- a/t/t5605-clone-local.sh\n> +++ b/t/t5605-clone-local.sh\n> @@ -154,6 +154,16 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '\n>  \t! repo_is_hardlinked force-nonlocal\n>  '\n>  \n> +test_expect_success 'cloning a local path with --no-local from a different user succeeds' '\n> +\tgit clone --upload-pack=\"GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack\" \\\n> +\t\t--no-local a nonlocal-otheruser 2>err &&\n> +\t! repo_is_hardlinked nonlocal-otheruser &&\n> +\t# Verify that this is a git repository.\n> +\tgit -C nonlocal-otheruser rev-parse --show-toplevel &&\n> +\t! test_grep \"detected dubious ownership\" err\n> +\n> +'\n\nThis test succeeds without checking everything it is supposed to:\n\n  + git clone --upload-pack=GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack --no-local a nonlocal-otheruser\n  + repo_is_hardlinked nonlocal-otheruser\n  + find nonlocal-otheruser/objects -type f -links 1\n  find: 'nonlocal-otheruser/objects': No such file or directory\n  + git -C nonlocal-otheruser rev-parse --show-toplevel\n  /home/szeder/src/git/t/trash directory.t5605-clone-local/nonlocal-otheruser\n  + test_grep detected dubious ownership err\n  error: 'grep detected dubious ownership err' didn't find a match in:\n  Cloning into 'nonlocal-otheruser'...\n  warning: remote HEAD refers to nonexistent ref, unable to checkout\n  ok 21 - cloning a local path with --no-local from a different user succeeds\n\nThe 'repo_is_hardlinked' helper function expects the path to the .git\ndirectory as parameter, but in this case it gets the top-level path of\nthe repository instead, which results in 'find' rightfully complaining\nabout the non-existing 'objects' directory.  Thanks to the &&-chain in\n'repo_is_hardlinked' this error then causes the helper function to\nreturn with non-zero error code, just what this test case expected.\n\nAll other tests using this helper function create bare clones, and\nthey all pass the right path as parameter.\n\nThe trivial fix would be to either use a bare clone in this test as\nwell, or to pass the right path to 'repo_is_hardlinked', i.e. the path\nto the '.git' directory.\n\nThe right fix, in my opinion, is to teach 'repo_is_hardlinked' a\nnegation parameter, so tests expecting the repo to be not hardlinked\ncould invoke it as 'repo_is_hardlinked ! <path>'.  An error from\n'find' within the helper function should then always result in an\nerror of the helper function, i.e. both with and without that '!'\nparameter.\n\nFurthermore, this test should use 'test_grep ! ...' instead of '!\ntest_grep ...', so we don't get that misleading error message in the\ntest's output.\n\nAnd there is a stray empty line at the end of the test as well.\n\n"},{"id":"515401","messageId":"Z-sO9gcWAi_xh0tI@tapette.crustytoothpaste.net","threadId":"62493","inReplyTo":"Z+qVJPQ17urm06QN@szeder.dev","subject":"Re: [PATCH 1/1] Allow cloning from repositories owned by another user","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-03-31T21:53:58Z","receivedAt":"2025-03-31T21:54:06Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-03-31 at 13:14:12, SZEDER Gábor wrote:\n> This test succeeds without checking everything it is supposed to:\n> \n>   + git clone --upload-pack=GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack --no-local a nonlocal-otheruser\n>   + repo_is_hardlinked nonlocal-otheruser\n>   + find nonlocal-otheruser/objects -type f -links 1\n>   find: 'nonlocal-otheruser/objects': No such file or directory\n>   + git -C nonlocal-otheruser rev-parse --show-toplevel\n>   /home/szeder/src/git/t/trash directory.t5605-clone-local/nonlocal-otheruser\n>   + test_grep detected dubious ownership err\n>   error: 'grep detected dubious ownership err' didn't find a match in:\n>   Cloning into 'nonlocal-otheruser'...\n>   warning: remote HEAD refers to nonexistent ref, unable to checkout\n>   ok 21 - cloning a local path with --no-local from a different user succeeds\n> \n> The 'repo_is_hardlinked' helper function expects the path to the .git\n> directory as parameter, but in this case it gets the top-level path of\n> the repository instead, which results in 'find' rightfully complaining\n> about the non-existing 'objects' directory.  Thanks to the &&-chain in\n> 'repo_is_hardlinked' this error then causes the helper function to\n> return with non-zero error code, just what this test case expected.\n> \n> All other tests using this helper function create bare clones, and\n> they all pass the right path as parameter.\n> \n> The trivial fix would be to either use a bare clone in this test as\n> well, or to pass the right path to 'repo_is_hardlinked', i.e. the path\n> to the '.git' directory.\n> \n> The right fix, in my opinion, is to teach 'repo_is_hardlinked' a\n> negation parameter, so tests expecting the repo to be not hardlinked\n> could invoke it as 'repo_is_hardlinked ! <path>'.  An error from\n> 'find' within the helper function should then always result in an\n> error of the helper function, i.e. both with and without that '!'\n> parameter.\n> \n> Furthermore, this test should use 'test_grep ! ...' instead of '!\n> test_grep ...', so we don't get that misleading error message in the\n> test's output.\n> \n> And there is a stray empty line at the end of the test as well.\n\nThanks for the report.  I've opted not to teach `repo_is_hardlinked` a\nnegation parameter, but I have fixed the three items you've mentioned.\nHere's a patch.\n\n-- >% --\nSubject: [PATCH] t5605: fix test for cloning from a different user\n\nThis test currently passes, but for the wrong reason.  The\nrepo_is_hardlinked function expects a .git directory or a bare\nrepository and currently fails because it cannot find the objects\ndirectory.\n\nOne solution is to use the --bare argument, but then --show-toplevel\nwon't work.  We could change that, but there's no need to, so just add\nthe missing .git directory.\n\nIn addition, use the built-in negation functionality of test_grep to\navoid mishandling real errors (such as a missing file) and, as a final\nfix, remove the extra newline.\n\nReported-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n t/t5605-clone-local.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\nindex 4605703496..2397f8fa61 100755\n--- a/t/t5605-clone-local.sh\n+++ b/t/t5605-clone-local.sh\n@@ -156,11 +156,10 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '\n test_expect_success 'cloning a local path with --no-local from a different user succeeds' '\n \tgit clone --upload-pack=\"GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack\" \\\n \t\t--no-local a nonlocal-otheruser 2>err &&\n-\t! repo_is_hardlinked nonlocal-otheruser &&\n+\t! repo_is_hardlinked nonlocal-otheruser/.git &&\n \t# Verify that this is a git repository.\n \tgit -C nonlocal-otheruser rev-parse --show-toplevel &&\n-\t! test_grep \"detected dubious ownership\" err\n-\n+\ttest_grep ! \"detected dubious ownership\" err\n '\n \n test_expect_success 'cloning locally respects \"-u\" for fetching refs' '\n-- \n2.49.0.395.g12beb8f557c\n-- >% --\n\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"}]}