[PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[Christian Couder]

Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
nothing" boolean that is set to 'true' by default when calling `git
upload-pack` for security reasons.

Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they
can safely use.

This series leverages that capability to implement a pragmatic middle
ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
fetching is allowed only when fetching from promisor remotes that are
both advertised by the server and accepted by the client.

Note that using an environment variable for this is probably not the
best from a usability perspective. An `upload-pack.allowLazyFetch`
configuration variable would likely be better.

Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
things currently work. It would be a much bigger and more invasive
change to implement `upload-pack.allowLazyFetch` in a way that is
compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
backward compatibility. Therefore, transitioning to a configuration
variable is left for future work.

High level overview of the patches
==================================

Patch 1/3: A refactor which separates the fetching logic from the
error handling and validation logic. This might also slightly increase
performance if there are several promisor remotes.

Patch 2/3: A preparatory commit that transitions `GIT_NO_LAZY_FETCH`
from a strict boolean check into an enum that can support multiple
states.

Patch 3/3: Introduces the 'fromAccepted' option, taking advantage of
the previous preparatory commits.

CI tests
========

They all pass, see:

https://github.com/chriscool/git/actions/runs/29078195030


Christian Couder (3):
  promisor-remote: factor out lazy_fetch_objects()
  promisor-remote: introduce enum allow_lazy_fetch
  promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH

 Documentation/git-upload-pack.adoc    |   5 ++
 Documentation/git.adoc                |   6 +-
 promisor-remote.c                     | 110 ++++++++++++++++++--------
 promisor-remote.h                     |  14 ++++
 setup.c                               |   5 +-
 t/t5710-promisor-remote-capability.sh |  49 ++++++++++++
 6 files changed, 154 insertions(+), 35 deletions(-)

-- 
2.55.0.125.g395cd2c8ec.dirty

[PATCH 1/3] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.

Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.

Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function. This will make it easier
to extend the lazy fetching logic in following commits.

This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:

  - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
    instead of once per promisor remote, and

  - promisor_remote_init() is no longer called when lazy fetching
    is disabled, which is fine as nothing downstream of it, like
    is_promisor_object(), needs it in that case.

While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
 	FILE *child_in;
 	int quiet;
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
-		static int warning_shown;
-		if (!warning_shown) {
-			warning_shown = 1;
-			warning(_("lazy fetching disabled; some objects may not be available"));
-		}
-		return -1;
-	}
-
 	child.git_cmd = 1;
 	child.in = -1;
 	if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
-static int try_promisor_remotes(struct repository *repo,
-				struct object_id **remaining_oids,
-				int *remaining_nr, int *to_free,
-				bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+				 struct object_id **remaining_oids,
+				 int *remaining_nr,
+				 int *to_free,
+				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
 
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
 				continue;
 			}
 		}
-		return 1; /* all fetched */
+		return true; /* all fetched */
 	}
-	return 0;
+	return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+			       struct object_id **remaining_oids,
+			       int *remaining_nr,
+			       int *to_free)
+{
+	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching disabled; some objects may not be available"));
+		}
+		return false;
+	}
+
+	promisor_remote_init(repo);
+
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				 to_free, true))
+		return true;
+
+	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				    to_free, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
-	int i;
 
 	if (oid_nr == 0)
 		return;
 
-	promisor_remote_init(repo);
-
-	/* Try accepted remotes first (those the server told us to use) */
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, true))
-		goto all_fetched;
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, false))
-		goto all_fetched;
-
-	for (i = 0; i < remaining_nr; i++) {
-		if (is_promisor_object(repo, &remaining_oids[i]))
-			die(_("could not fetch %s from promisor remote"),
-			    oid_to_hex(&remaining_oids[i]));
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+		for (int i = 0; i < remaining_nr; i++) {
+			if (is_promisor_object(repo, &remaining_oids[i]))
+				die(_("could not fetch %s from promisor remote"),
+				    oid_to_hex(&remaining_oids[i]));
+		}
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }
-- 
2.55.0.125.g395cd2c8ec.dirty

[PATCH 2/3] promisor-remote: introduce enum allow_lazy_fetch

[Christian Couder]

The `GIT_NO_LAZY_FETCH` environment variable is currently parsed as
a Boolean, using git_env_bool(), in both "setup.c" and
"promisor-remote.c".

In a following commit, we are going to allow a third value for this
variable, on top of 'true' and 'false'.

To prepare for that, let's introduce an `enum allow_lazy_fetch` with
the possible results of parsing the variable, along with a
parse_allow_lazy_fetch_env() function to parse it, and let's use them
everywhere the variable is parsed.

Note that, as before, an invalid value makes us die(), only the error
message changes from "bad boolean environment value ..." to "bad
environment value ...".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 24 +++++++++++++++++++++++-
 promisor-remote.h | 13 +++++++++++++
 setup.c           |  5 ++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 65496c69cf..56f57c5267 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -21,6 +21,26 @@ struct promisor_remote_config {
 	struct promisor_remote **promisors_tail;
 };
 
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
+{
+	const char *v = getenv(NO_LAZY_FETCH_ENVIRONMENT);
+	int val;
+
+	if (!v)
+		return LAZY_FETCH_ALL;
+
+	val = git_parse_maybe_bool(v);
+
+	if (!val)
+		return LAZY_FETCH_ALL;
+	if (val > 0)
+		return LAZY_FETCH_NONE;
+
+	die(_("bad environment value '%s' for '%s'; "
+	      "only 'false/0' and 'true/1' are valid"),
+	    v, NO_LAZY_FETCH_ENVIRONMENT);
+}
+
 static int fetch_objects(struct repository *repo,
 			 const char *remote_name,
 			 const struct object_id *oids,
@@ -299,7 +319,9 @@ static bool lazy_fetch_objects(struct repository *repo,
 			       int *remaining_nr,
 			       int *to_free)
 {
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+	enum allow_lazy_fetch lf = parse_allow_lazy_fetch_env();
+
+	if (lf == LAZY_FETCH_NONE) {
 		static int warning_shown;
 		if (!warning_shown) {
 			warning_shown = 1;
diff --git a/promisor-remote.h b/promisor-remote.h
index 301f5ac5cb..87fc24c9eb 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -25,6 +25,19 @@ void promisor_remote_clear(struct promisor_remote_config *config);
 struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);
 int repo_has_promisor_remote(struct repository *r);
 
+/* Enum for lazy fetching parsing */
+enum allow_lazy_fetch {
+	LAZY_FETCH_NONE    = 0,  /* No lazy fetching */
+	LAZY_FETCH_ALL           /* Lazy fetch from any promisor remotes */
+};
+
+/*
+ * Parse the NO_LAZY_FETCH_ENVIRONMENT env variable into an
+ * `enum allow_lazy_fetch`.
+ * If parsing fails, then die().
+ */
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void);
+
 /*
  * Fetches all requested objects from all promisor remotes, trying them one at
  * a time until all objects are fetched.
diff --git a/setup.c b/setup.c
index 0de56a074f..0a81d9f045 100644
--- a/setup.c
+++ b/setup.c
@@ -24,6 +24,7 @@
 #include "trace.h"
 #include "trace2.h"
 #include "worktree.h"
+#include "promisor-remote.h"
 
 enum allowed_bare_repo {
 	ALLOWED_BARE_REPO_EXPLICIT = 0,
@@ -1051,6 +1052,7 @@ static void setup_git_env_internal(struct repository *repo,
 	const char *replace_ref_base;
 	struct set_gitdir_args args = { NULL };
 	struct strvec to_free = STRVEC_INIT;
+	enum allow_lazy_fetch lf;
 
 	args.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);
 	args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
@@ -1072,7 +1074,8 @@ static void setup_git_env_internal(struct repository *repo,
 	if (shallow_file)
 		set_alternate_shallow_file(repo, shallow_file, 0);
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
+	lf = parse_allow_lazy_fetch_env();
+	if (lf == LAZY_FETCH_NONE)
 		fetch_if_missing = 0;
 }
 
-- 
2.55.0.125.g395cd2c8ec.dirty

[PATCH 3/3] promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH

[Christian Couder]

The `GIT_NO_LAZY_FETCH` environment variable can be set to 'true' or
'false' to enable or disable lazy fetching. By default it is set to
'true' when calling `git upload-pack` to avoid security issues, see
7b70e9efb1 (upload-pack: disable lazy-fetching by default, 2024-04-16).

Recently though, the "promisor-remote" capability was introduced into
protocol v2, which allows a server to advertise some promisor remotes
and clients to accept them or not.

When promisor remotes are advertised by the server and accepted by the
client, it means that they are quite trusted. So the security risks
which come from lazy fetching from them could be considered much more
acceptable.

Let's introduce a 'fromAccepted' option on top of 'true' and 'false'
for `GIT_NO_LAZY_FETCH` to allow lazy fetching only from accepted
promisor remotes.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-upload-pack.adoc    |  5 +++
 Documentation/git.adoc                |  6 ++--
 promisor-remote.c                     | 14 +++++++-
 promisor-remote.h                     |  1 +
 t/t5710-promisor-remote-capability.sh | 49 +++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..1c2ed9d7ba 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
 (because you are fetching from a partial clone, and you are sure
 you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
 `0`.
++
+`GIT_NO_LAZY_FETCH` can also be set to 'fromAccepted' which allows
+lazy fetching only from remotes that are advertised and accepted using
+the "promisor-remote" protocol v2 capability. See
+linkgit:gitprotocol-v2[5]. This is safer than setting it to `0`.
 
 SECURITY
 --------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..14a083bcdb 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -947,9 +947,9 @@ for full details.
 	pathspecs as case-insensitive.
 
 `GIT_NO_LAZY_FETCH`::
-	Setting this Boolean environment variable to true tells Git
-	not to lazily fetch missing objects from the promisor remote
-	on demand.
+	Setting this environment variable controls whether Git is
+	allowed to lazily fetch missing objects from a promisor remote
+	on demand. See linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep
diff --git a/promisor-remote.c b/promisor-remote.c
index 56f57c5267..c80319f966 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -35,9 +35,11 @@ enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
 		return LAZY_FETCH_ALL;
 	if (val > 0)
 		return LAZY_FETCH_NONE;
+	if (!strcasecmp(v, "fromAccepted"))
+		return LAZY_FETCH_ACCEPTED;
 
 	die(_("bad environment value '%s' for '%s'; "
-	      "only 'false/0' and 'true/1' are valid"),
+	      "only 'false/0', 'true/1' and 'fromAccepted' are valid"),
 	    v, NO_LAZY_FETCH_ENVIRONMENT);
 }
 
@@ -337,6 +339,16 @@ static bool lazy_fetch_objects(struct repository *repo,
 				 to_free, true))
 		return true;
 
+	if (lf == LAZY_FETCH_ACCEPTED) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching from accepted promisor remotes only; "
+				  "some objects may not be available"));
+		}
+		return false;
+	}
+
 	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
 				    to_free, false);
 }
diff --git a/promisor-remote.h b/promisor-remote.h
index 87fc24c9eb..0d05ff9d84 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -28,6 +28,7 @@ int repo_has_promisor_remote(struct repository *r);
 /* Enum for lazy fetching parsing */
 enum allow_lazy_fetch {
 	LAZY_FETCH_NONE    = 0,  /* No lazy fetching */
+	LAZY_FETCH_ACCEPTED,     /* Lazy fetching only from accepted promisor remotes */
 	LAZY_FETCH_ALL           /* Lazy fetch from any promisor remotes */
 };
 
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..1c61b100b9 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,55 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and accepted promisor remote" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	# Clone from server to create a client
+	GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+		-c promisor.acceptfromserver=All \
+		--no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and no accepted promisor remote" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	# Clone from server to create a client
+	# It should fail because the server cannot lazy fetch the missing blob
+	test_must_fail env GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+		-c promisor.acceptfromserver=None \
+		--no-local --filter="blob:limit=5k" server client 2>err &&
+
+	test_grep "lazy fetching from accepted promisor remotes only" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone failure with GIT_NO_LAZY_FETCH=bogus" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	test_must_fail env GIT_NO_LAZY_FETCH=bogus git clone -c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+		-c promisor.acceptfromserver=All \
+		--no-local --filter="blob:limit=5k" server client 2>err &&
+
+	test_grep "bad environment value" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.55.0.125.g395cd2c8ec.dirty

Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On 2026-07-10 at 08:51:34, Christian Couder wrote:
> Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> 2024-04-16), lazy fetching has been controlled by the
> `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> nothing" boolean that is set to 'true' by default when calling `git
> upload-pack` for security reasons.
> 
> Recently the "promisor-remote" capability was added to protocol v2,
> allowing servers and clients to agree on the promisor remotes they
> can safely use.
> 
> This series leverages that capability to implement a pragmatic middle
> ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> fetching is allowed only when fetching from promisor remotes that are
> both advertised by the server and accepted by the client.
> 
> Note that using an environment variable for this is probably not the
> best from a usability perspective. An `upload-pack.allowLazyFetch`
> configuration variable would likely be better.
> 
> Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> things currently work. It would be a much bigger and more invasive
> change to implement `upload-pack.allowLazyFetch` in a way that is
> compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> backward compatibility. Therefore, transitioning to a configuration
> variable is left for future work.

I don't think this is a good idea.  We get a lot of reports on the
security list involving various tooling that isn't within the scope of
our threat model.  This substantially increases the amount of code which
is now subject to that threat model and therefore our security
guarantees and I don't think we should do that as it stands, very
especially while so much of our network-facing code is written in C.

The fetch code by default reads lots of configuration information from
the repository, including remote settings and information and we really
want absolutely none of that code running in the context of an untrusted
repository.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[Christian Couder]

On Fri, Jul 10, 2026 at 9:50 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2026-07-10 at 08:51:34, Christian Couder wrote:
> > Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> > 2024-04-16), lazy fetching has been controlled by the
> > `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> > nothing" boolean that is set to 'true' by default when calling `git
> > upload-pack` for security reasons.
> >
> > Recently the "promisor-remote" capability was added to protocol v2,
> > allowing servers and clients to agree on the promisor remotes they
> > can safely use.
> >
> > This series leverages that capability to implement a pragmatic middle
> > ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> > fetching is allowed only when fetching from promisor remotes that are
> > both advertised by the server and accepted by the client.
> >
> > Note that using an environment variable for this is probably not the
> > best from a usability perspective. An `upload-pack.allowLazyFetch`
> > configuration variable would likely be better.
> >
> > Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> > things currently work. It would be a much bigger and more invasive
> > change to implement `upload-pack.allowLazyFetch` in a way that is
> > compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> > backward compatibility. Therefore, transitioning to a configuration
> > variable is left for future work.
>
> I don't think this is a good idea.  We get a lot of reports on the
> security list involving various tooling that isn't within the scope of
> our threat model.  This substantially increases the amount of code which
> is now subject to that threat model and therefore our security
> guarantees and I don't think we should do that as it stands, very
> especially while so much of our network-facing code is written in C.

This small series doesn't change any defaults, especially
GIT_NO_LAZY_FETCH is still set to 1 when calling `git upload-pack` by
default. And the new option is more restrictive than the
GIT_NO_LAZY_FETCH=0 option which already exists.

So I don't think it's fair to say that this _substantially increases_
the amount of code subject to some threat model.

I agree that client acceptance of some promisor remotes doesn't make
the served repo trusted. It's a real concern, but I think it's
addressable by different mechanisms. See below.

> The fetch code by default reads lots of configuration information from
> the repository, including remote settings and information and we really
> want absolutely none of that code running in the context of an untrusted
> repository.

When a promisor remote has been accepted, it means both the client and
the server trust it, so at least the promisor remote is not untrusted.

Now the main security issue on the server side is making sure the
served repo itself is also trusted. And I agree that the operator of
the server should decide and mark that trust, not the client.

I also agree that on GitLab/GitHub-style multi-tenant hosts most
repositories shouldn't be marked as trusted.

However note that:

- The operator of the server is the only actor which can set
GIT_NO_LAZY_FETCH on the server (where it matters).
- In the case of corporate/self-hosted repos, the operator also
controls the repos.
- Different features could be developed (in future work) to improve on
the current state:
    - a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
    - an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
    - operator-defined allow/deny rules, or maybe
    - some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.

I would be happy to hear opinions about those potential features or
any other ways to address the issue.

So I agree that this series doesn't fix all the problems on the server
side, but I think it's still valuable to be able to restrict lazy
fetching to accepted promisor remotes.

Also I definitely agree that the current series should have better
documentation about this, and I plan to improve on that in the v2 of
this series.

Thanks for your insightful comments.

[PATCH 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Christian Couder]

Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they can
safely use.

The more servers use promisor remotes, the more it is important to
properly control if they can lazy fetch when responding to a clone or
fetch request from the client.

For example, in the context of large object promisors (see
"Documentation/technical/large-object-promisors.adoc"), if a client
clones with a filter set to 100kB while the server has moved all of
the blobs >= 10kB to a promisor remote, the server will not be able to
provide blobs between 10kB and 100kB to the client, which will make
the clone fail.

Even if the `--filter=auto` option is available since ef2f1845ec
(fetch-pack: wire up and enable auto filter logic, 2026-02-16) it's
still a good idea to provide more control over lazy fetching on the
server side to server operators, as lazy fetching on the server side
could be useful in corporate environments.

Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is a boolean that is
set to 'true' by default when calling `git upload-pack` for security
reasons.

The main security issue on the server side is making sure the served
repo itself is also trusted, as lazily fetching runs `git fetch`,
which may execute arbitrary commands specified in the configuration
and hooks of the served repo. The operator of the server should decide
and mark that trust, not the served repo itself, nor the client.

This series introduces a new 'uploadpack.lazyFetchTrusted' protected
configuration variable similar to 'safe.directory' (see
"Documentation/config/safe.adoc") to mark trusted repos where lazy
fetching is allowed. As it is protected, this config variable will
only take effect if it is set in global or system scope, so only
server operators can control it.

Previous related work
=====================

A previous series called "Introduce a 'fromAccepted' option to
GIT_NO_LAZY_FETCH" [1] took a different approach as it wanted to make
it easier to allow lazy fetching from accepted promisor remotes. But
after brian replied that he didn't think it was a good idea, and after
thinking about this more, my opinion now is that some promisor remotes
being accepted or not is not really relevant to the issue.

In my reply to brian, I said:

"""
Different features could be developed (in future work) to improve on
the current state:
    - a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
    - an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
    - operator-defined allow/deny rules, or maybe
    - some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.
"""

So I decided to go with "an explicit way for operators to mark trusted
repos" and this series is an implementation of that.

Note that the feature developed in this series applies to protocol
v0/v1 as well as v2 while the previous one was only related to v2.

[1]: https://lore.kernel.org/git/CAP8UFD0_S9eg_w42tcNRnT9E2ntLr_eHLnzE4c2dSu67DzZoXg@mail.gmail.com/

Overview of the patches
=======================

  - Patch 1/5 is the only patch saved from the "Introduce a
    'fromAccepted' option to GIT_NO_LAZY_FETCH" series. It's not
    necessary for the rest of this series and its main feature to
    work, but I think it's a nice refactoring related to lazy
    fetching, so it might as well be part of this series. There is a
    small change in the commit message (to not mention following
    commits) compared to the version in the previous series.

  - Patches 2/5 and 3/5 extract and modify code used by the
    'safe.directory' config variable in a path_allowlist_apply()
    function, so that this function can be reused to process
    'uploadpack.lazyFetchTrusted' in the next patch.

  - Patch 4/5 actually uses path_allowlist_apply() from a new
    upload_pack_lazy_fetch_trusted() function to process
    'uploadpack.lazyFetchTrusted', but the result from that processing
    isn't actually used to have a practical effect.

  - Patch 5/5 wires up the new upload_pack_lazy_fetch_trusted()
    function to decide if lazy fetching can actually be enabled.

CI tests
========

They all pass, see:

https://github.com/chriscool/git/actions/runs/31171494296

Range diff with previous series
===============================

The range diff with the previous ("Introduce a 'fromAccepted' option
to GIT_NO_LAZY_FETCH") series is not very interesting as only the
first patch has been saved, but anyway here it is:

1:  8dd67ddaca ! 1:  b5b0836d19 promisor-remote: factor out lazy_fetch_objects()
    @@ Commit message
         that could not be fetched are promisor objects.
     
         Let's refactor the lazy fetching logic out of these two functions
    -    into a new lazy_fetch_objects() function. This will make it easier
    -    to extend the lazy fetching logic in following commits.
    +    into a new lazy_fetch_objects() function.
     
         This is a pure refactoring with no intended behavior change. Two
         things shift in ways that are observably equivalent though:
2:  314c61cbbe < -:  ---------- promisor-remote: introduce enum allow_lazy_fetch
3:  cb2f5447e2 < -:  ---------- promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
-:  ---------- > 2:  879e3a34e3 setup: extract path_allowlist_apply()
-:  ---------- > 3:  98431ab7b3 setup: add 'allow_dot' arg to path_allowlist_apply()
-:  ---------- > 4:  a46f4c1bb8 upload-pack: read uploadpack.lazyFetchTrusted
-:  ---------- > 5:  4063f233aa builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo


Christian Couder (5):
  promisor-remote: factor out lazy_fetch_objects()
  setup: extract path_allowlist_apply()
  setup: add 'allow_dot' arg to path_allowlist_apply()
  upload-pack: read uploadpack.lazyFetchTrusted
  builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo

 Documentation/config/uploadpack.adoc  |  42 ++++++++++
 Documentation/git-upload-pack.adoc    |   5 ++
 Documentation/git.adoc                |   4 +-
 builtin/upload-pack.c                 |  11 +++
 promisor-remote.c                     |  76 ++++++++++--------
 setup.c                               | 108 ++++++++++++++------------
 setup.h                               |  28 +++++++
 t/t5710-promisor-remote-capability.sh |  70 +++++++++++++++++
 upload-pack.c                         |  37 +++++++++
 upload-pack.h                         |   3 +
 10 files changed, 304 insertions(+), 80 deletions(-)

-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.

Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.

Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function.

This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:

  - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
    instead of once per promisor remote, and

  - promisor_remote_init() is no longer called when lazy fetching
    is disabled, which is fine as nothing downstream of it, like
    is_promisor_object(), needs it in that case.

While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
 	FILE *child_in;
 	int quiet;
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
-		static int warning_shown;
-		if (!warning_shown) {
-			warning_shown = 1;
-			warning(_("lazy fetching disabled; some objects may not be available"));
-		}
-		return -1;
-	}
-
 	child.git_cmd = 1;
 	child.in = -1;
 	if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
-static int try_promisor_remotes(struct repository *repo,
-				struct object_id **remaining_oids,
-				int *remaining_nr, int *to_free,
-				bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+				 struct object_id **remaining_oids,
+				 int *remaining_nr,
+				 int *to_free,
+				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
 
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
 				continue;
 			}
 		}
-		return 1; /* all fetched */
+		return true; /* all fetched */
 	}
-	return 0;
+	return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+			       struct object_id **remaining_oids,
+			       int *remaining_nr,
+			       int *to_free)
+{
+	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching disabled; some objects may not be available"));
+		}
+		return false;
+	}
+
+	promisor_remote_init(repo);
+
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				 to_free, true))
+		return true;
+
+	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				    to_free, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
-	int i;
 
 	if (oid_nr == 0)
 		return;
 
-	promisor_remote_init(repo);
-
-	/* Try accepted remotes first (those the server told us to use) */
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, true))
-		goto all_fetched;
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, false))
-		goto all_fetched;
-
-	for (i = 0; i < remaining_nr; i++) {
-		if (is_promisor_object(repo, &remaining_oids[i]))
-			die(_("could not fetch %s from promisor remote"),
-			    oid_to_hex(&remaining_oids[i]));
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+		for (int i = 0; i < remaining_nr; i++) {
+			if (is_promisor_object(repo, &remaining_oids[i]))
+				die(_("could not fetch %s from promisor remote"),
+				    oid_to_hex(&remaining_oids[i]));
+		}
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }
-- 
2.55.0.530.gdb3615d990.dirty

Re: [PATCH 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

On Fri, Aug 7, 2026 at 3:55 PM Christian Couder
<christian.couder@gmail.com> wrote:

[...]

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Sorry I just realized that there is the wrong sign-off email address
again. Will fix it in v2.

[PATCH 2/5] setup: extract path_allowlist_apply()

[Christian Couder]

In a following commit we are going to check whether a repository is
part of an allowlist specified in a config variable.

To prepare for that let's extract existing code from
safe_directory_cb() into a new path_allowlist_apply() helper that will
help with such checks.

While at it let's make the helper's code simpler and more generic.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 setup.c | 107 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 48 deletions(-)

diff --git a/setup.c b/setup.c
index 95909e9603..39dfa1cc5f 100644
--- a/setup.c
+++ b/setup.c
@@ -1339,6 +1339,64 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
+static void path_allowlist_apply(const char *key, const char *value,
+				 const char *target_path, int *is_match)
+{
+	char *allowed = NULL;
+	char *normalized = NULL;
+
+	if (!value || !*value) {
+		*is_match = 0;
+		return;
+	}
+
+	if (!strcmp(value, "*")) {
+		*is_match = 1;
+		return;
+	}
+
+	if (git_config_pathname(&allowed, key, value) || !allowed)
+		return;
+
+	/*
+	 * Setting the config variable to a non-absolute path makes
+	 * little sense---it won't be relative to the configuration
+	 * file the item is defined in.  Except for ".", which means
+	 * "if we are at the top level of a repository, then it is
+	 * OK", which is slightly tighter than "*" that allows
+	 * discovery.
+	 */
+	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
+		warning(_("%s '%s' not absolute"), key, allowed);
+		goto end;
+	}
+
+	/*
+	 * A .gitconfig in $HOME may be shared across different
+	 * machines and the config variable entries may or may not
+	 * exist as paths on all of these machines.  In other words,
+	 * it is not a warning worthy event when there is no such path
+	 * on this machine---the entry may be useful elsewhere.
+	 */
+	normalized = real_pathdup(allowed, 0);
+	if (!normalized)
+		goto end;
+
+	if (ends_with(normalized, "/*")) {
+		size_t len = strlen(normalized);
+		if (!fspathncmp(normalized, target_path, len - 1))
+			*is_match = 1;
+		goto end;
+	}
+
+	if (!fspathcmp(target_path, normalized))
+		*is_match = 1;
+
+end:
+	free(normalized);
+	free(allowed);
+}
+
 struct safe_directory_data {
 	char *path;
 	int is_safe;
@@ -1352,54 +1410,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value) {
-		data->is_safe = 0;
-	} else if (!strcmp(value, "*")) {
-		data->is_safe = 1;
-	} else {
-		char *allowed = NULL;
-
-		if (!git_config_pathname(&allowed, key, value) && allowed) {
-			char *normalized = NULL;
-
-			/*
-			 * Setting safe.directory to a non-absolute path
-			 * makes little sense---it won't be relative to
-			 * the configuration file the item is defined in.
-			 * Except for ".", which means "if we are at the top
-			 * level of a repository, then it is OK", which is
-			 * slightly tighter than "*" that allows discovery.
-			 */
-			if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
-				warning(_("safe.directory '%s' not absolute"),
-					allowed);
-				goto next;
-			}
-
-			/*
-			 * A .gitconfig in $HOME may be shared across
-			 * different machines and safe.directory entries
-			 * may or may not exist as paths on all of these
-			 * machines.  In other words, it is not a warning
-			 * worthy event when there is no such path on this
-			 * machine---the entry may be useful elsewhere.
-			 */
-			normalized = real_pathdup(allowed, 0);
-			if (!normalized)
-				goto next;
-
-			if (ends_with(normalized, "/*")) {
-				size_t len = strlen(normalized);
-				if (!fspathncmp(normalized, data->path, len - 1))
-					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, normalized)) {
-				data->is_safe = 1;
-			}
-		next:
-			free(normalized);
-			free(allowed);
-		}
-	}
+	path_allowlist_apply(key, value, data->path, &data->is_safe);
 
 	return 0;
 }
-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()

[Christian Couder]

A previous commit created path_allowlist_apply() with the goal of later
reusing that function. But when it will be reused in a following commit
this function will need to reject non-absolute paths including those
with a single dot that are currently accepted.

To prepare for reusing path_allowlist_apply(), let's add a
`bool allow_dot` argument to it, and let's export this function.

While at it let's document it properly in "setup.h".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 setup.c |  9 +++++----
 setup.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 39dfa1cc5f..a09e697e3a 100644
--- a/setup.c
+++ b/setup.c
@@ -1339,8 +1339,9 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
-static void path_allowlist_apply(const char *key, const char *value,
-				 const char *target_path, int *is_match)
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot)
 {
 	char *allowed = NULL;
 	char *normalized = NULL;
@@ -1366,7 +1367,7 @@ static void path_allowlist_apply(const char *key, const char *value,
 	 * OK", which is slightly tighter than "*" that allows
 	 * discovery.
 	 */
-	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
+	if (!is_absolute_path(allowed) && (!allow_dot || strcmp(allowed, "."))) {
 		warning(_("%s '%s' not absolute"), key, allowed);
 		goto end;
 	}
@@ -1410,7 +1411,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	path_allowlist_apply(key, value, data->path, &data->is_safe);
+	path_allowlist_apply(key, value, data->path, &data->is_safe, true);
 
 	return 0;
 }
diff --git a/setup.h b/setup.h
index 654f10e059..d4f8af5457 100644
--- a/setup.h
+++ b/setup.h
@@ -304,4 +304,32 @@ struct startup_info {
 extern struct startup_info *startup_info;
 extern const char *tmp_original_cwd;
 
+/*
+ * Apply the path allowlist in 'value' against 'target_path' setting
+ * '*is_match' accordingly.
+ *
+ * `value` is the value of a multi-valued config variable named `key`
+ * that holds an allowlist of paths. `target_path` is the (normalized)
+ * path being tested. `*is_match` is updated in place:
+ *
+ *   - an empty value resets it to 0 (so a later, more specific config
+ *     scope can clear entries from a broader one),
+ *   - "*" sets it to 1 (allow everything),
+ *   - "<path>" sets it to 1 if <path> equals `target_path`,
+ *   - "<path>" + "/" + "*" sets it to 1 if <path> is a leading
+ *     directory of `target_path`,
+ *   - any other (unmatching) value leaves `*is_match` unchanged.
+ *
+ * Non-absolute values are rejected with a warning, except "." when
+ * `allow_dot` is set (used by 'safe.directory' to mean "the top level
+ * of the current repository").
+ *
+ * Callers are expected to invoke this once per config value,
+ * typically from a protected-config callback, so that untrusted
+ * repository config cannot influence the decision.
+ */
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot);
+
 #endif /* SETUP_H */
-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 4/5] upload-pack: read uploadpack.lazyFetchTrusted

[Christian Couder]

Previous commits created and prepared the path_allowlist_apply()
function.

Let's reuse this function for a new "uploadpack.lazyFetchTrusted"
configuration variable.

It allows us to:

  - read an allowlist from that config variable,
  - check if the current repo is in that list, and
  - return the result from a new upload_pack_lazy_fetch_trusted()
    function.

The new function will be used in a following commit.

Note that the new config variable should be read only from protected
configuration files.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 upload-pack.c | 37 +++++++++++++++++++++++++++++++++++++
 upload-pack.h |  3 +++
 2 files changed, 40 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index a52856d869..29e700e43b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -34,6 +34,8 @@
 #include "json-writer.h"
 #include "strmap.h"
 #include "promisor-remote.h"
+#include "setup.h"
+#include "abspath.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)
@@ -1378,6 +1380,41 @@ static int upload_pack_config(const char *var, const char *value,
 	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
 }
 
+struct lazy_fetch_trusted {
+	int trusted;
+	char *repo_path;
+};
+
+static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
+						   const struct config_context *ctx UNUSED,
+						   void *cb_data)
+{
+	struct lazy_fetch_trusted *data = cb_data;
+
+	if (!strcmp("uploadpack.lazyfetchtrusted", var)) {
+		path_allowlist_apply(var, value, data->repo_path,
+				     &data->trusted, false);
+		return 0;
+	}
+
+	return 0;
+}
+
+bool upload_pack_lazy_fetch_trusted(struct repository *r)
+{
+	struct lazy_fetch_trusted data = { 0 };
+
+	data.repo_path = real_pathdup(r->worktree ? r->worktree : r->gitdir, 0);
+	if (!data.repo_path)
+		return false;
+
+	git_protected_config(upload_pack_protected_lazy_fetch_config, &data);
+
+	free(data.repo_path);
+
+	return !!data.trusted;
+}
+
 static int upload_pack_protected_config(const char *var, const char *value,
 					const struct config_context *ctx UNUSED,
 					void *cb_data)
diff --git a/upload-pack.h b/upload-pack.h
index d6ee25ea98..b2212992c3 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -12,4 +12,7 @@ struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
 
+/* Is this repo trusted for lazy fetching? */
+bool upload_pack_lazy_fetch_trusted(struct repository *r);
+
 #endif /* UPLOAD_PACK_H */
-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 5/5] builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo

[Christian Couder]

A previous commit added a new "uploadpack.lazyFetchTrusted" protected
config variable that can contain an allowlist of repos, as well as
functions to check if the current repo is in that list. But when the
current repo is in that list, we currently do nothing.

Let's instead set `GIT_NO_LAZY_FETCH` to `0`, which allows
`upload-pack` and its `pack-objects` child process to lazily fetch the
objects they need to serve a client, for example when the filter used
by the client and the one used by the server don't match.

This allows server operators to properly control lazy fetching. It is
their responsibility, not the client's, to decide if the served repo is
trusted, as the main security issue is that lazily fetching runs `git
fetch`, which may execute arbitrary commands specified in the
configuration and hooks of the served repo.

As `GIT_NO_LAZY_FETCH` is passed down to child processes through the
environment, this works for `pack-objects`, which performs the lazy
fetch when serving a client, without any further plumbing.

Now that "uploadpack.lazyFetchTrusted" is actually doing something,
let's document it and reference it from GIT_NO_LAZY_FETCH's docs.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/uploadpack.adoc  | 42 ++++++++++++++++
 Documentation/git-upload-pack.adoc    |  5 ++
 Documentation/git.adoc                |  4 +-
 builtin/upload-pack.c                 | 11 +++++
 t/t5710-promisor-remote-capability.sh | 70 +++++++++++++++++++++++++++
 5 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/uploadpack.adoc b/Documentation/config/uploadpack.adoc
index 0e1dda944a..e960879c16 100644
--- a/Documentation/config/uploadpack.adoc
+++ b/Documentation/config/uploadpack.adoc
@@ -86,3 +86,45 @@ uploadpack.allowRefInWant::
 	is intended for the benefit of load-balanced servers which may
 	not have the same view of what OIDs their refs point to due to
 	replication delay.
+
+uploadpack.lazyFetchTrusted::
+	These config entries specify repositories that `upload-pack` is
+	allowed to lazily fetch missing objects for. By default,
+	`upload-pack` refuses to lazily fetch (see the description of the
+	`GIT_NO_LAZY_FETCH` environment variable in
+	linkgit:git-upload-pack[1]), because doing so would run `git fetch`,
+	which may execute arbitrary commands specified in the configuration
+	and hooks of the served repository. Listing a repository here tells
+	`upload-pack` that it is trusted, so lazy fetching from the promisor
+	remotes configured in it is allowed. This is equivalent to setting
+	`GIT_NO_LAZY_FETCH` to `0` for the matching repositories. An
+	explicitly set `GIT_NO_LAZY_FETCH` takes precedence over this
+	setting.
++
+Note that this allows lazy fetching from any promisor remote
+configured in the served repository, not only from the promisor
+remotes that the client accepted using the "promisor-remote" protocol
+v2 capability (see linkgit:gitprotocol-v2[5]). The served repository
+is trusted as a whole, including its configuration, so the promisor
+remotes it configures are trusted too. It is the server operator's
+responsibility to make sure that the promisor remotes of a trusted
+repository are also trustworthy.
++
+This is a multi-valued setting, i.e. you can add more than one
+repository via `git config (--global|--system) --add`. To reset the
+list of trusted repositories (e.g. to override any such repositories
+specified in the system config), add a `uploadpack.lazyFetchTrusted`
+entry with an empty value.
++
+A repository is identified by its worktree, or its git directory for a bare
+repository, and the value must be an absolute path. Giving a path with `/*`
+appended to it will trust all repositories under the named directory. To trust
+all served repositories, set `uploadpack.lazyFetchTrusted` to the string `*`.
++
+The value of this setting is interpolated, i.e. `~/<path>` expands to a
+path relative to the home directory and `%(prefix)/<path>` expands to a
+path relative to Git's (runtime) prefix.
++
+Note that this configuration variable is only respected when it is specified
+in protected configuration (see <<SCOPES>>). This prevents untrusted
+repositories from tampering with this value.
diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..90c2ba1194 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
 (because you are fetching from a partial clone, and you are sure
 you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
 `0`.
++
+Instead of setting `GIT_NO_LAZY_FETCH` to `0` in the environment, a
+server operator can allow lazy fetching on a per-repository basis by
+listing trusted repositories in the `uploadpack.lazyFetchTrusted`
+configuration variable. See linkgit:git-config[1].
 
 SECURITY
 --------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..2e763d1f93 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -949,7 +949,9 @@ for full details.
 `GIT_NO_LAZY_FETCH`::
 	Setting this Boolean environment variable to true tells Git
 	not to lazily fetch missing objects from the promisor remote
-	on demand.
+	on demand. On the server side, the `uploadpack.lazyFetchTrusted`
+	configuration variable can control this per-repository. See
+	linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 32831fb879..8b531ca724 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -42,10 +42,13 @@ int cmd_upload_pack(int argc,
 		OPT_END()
 	};
 	unsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;
+	bool no_lazy_fetch_set;
 
 	packet_trace_identity("upload-pack");
 	disable_replace_refs();
 	save_commit_buffer = 0;
+
+	no_lazy_fetch_set = !!getenv(NO_LAZY_FETCH_ENVIRONMENT);
 	xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 0);
 
 	argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
@@ -62,6 +65,14 @@ int cmd_upload_pack(int argc,
 	if (!enter_repo(the_repository, dir, enter_repo_flags))
 		die("'%s' does not appear to be a git repository", dir);
 
+	/*
+	 * Relax the GIT_NO_LAZY_FETCH=1 default if the served repo is in
+	 * the "uploadpack.lazyFetchTrusted" protected allowlist and
+	 * GIT_NO_LAZY_FETCH was not already set explicitly.
+	 */
+	if (!no_lazy_fetch_set && upload_pack_lazy_fetch_trusted(the_repository))
+		xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "0", 1);
+
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
 		if (advertise_refs)
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..e6993f2761 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,76 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0
+	git clone --no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is not missing on the server
+	# This means the server lazy fetched it
+	check_missing_objects server 0 "" &&
+
+	# Reinitialize server so that the largest object is missing again
+	initialize_server 1 "$oid"
+'
+
+test_expect_success "clone without uploadpack.lazyFetchTrusted fails" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# Note: no uploadpack.lazyFetchTrusted config is set here, so
+	# the served repo is NOT trusted for lazy fetching.
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "uploadpack.lazyFetchTrusted is ignored in repo config" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching, but this is
+	# done in the repo config, not in protected config, so this is
+	# ignored.
+	test_config -C server uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "explicit GIT_NO_LAZY_FETCH overrides uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# But GIT_NO_LAZY_FETCH=1 disables lazy fetching, so clone fails
+	test_must_fail env GIT_NO_LAZY_FETCH=1 git clone --no-local \
+		--filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.55.0.530.gdb3615d990.dirty

Re: [PATCH 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Range diff with previous series
> ===============================
>
> The range diff with the previous ("Introduce a 'fromAccepted' option
> to GIT_NO_LAZY_FETCH") series is not very interesting as only the
> first patch has been saved, but anyway here it is:
>
> 1:  8dd67ddaca ! 1:  b5b0836d19 promisor-remote: factor out lazy_fetch_objects()
>     @@ Commit message
>          that could not be fetched are promisor objects.
>      
>          Let's refactor the lazy fetching logic out of these two functions
>     -    into a new lazy_fetch_objects() function. This will make it easier
>     -    to extend the lazy fetching logic in following commits.
>     +    into a new lazy_fetch_objects() function.
>      
>          This is a pure refactoring with no intended behavior change. Two
>          things shift in ways that are observably equivalent though:
> 2:  314c61cbbe < -:  ---------- promisor-remote: introduce enum allow_lazy_fetch
> 3:  cb2f5447e2 < -:  ---------- promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
> -:  ---------- > 2:  879e3a34e3 setup: extract path_allowlist_apply()
> -:  ---------- > 3:  98431ab7b3 setup: add 'allow_dot' arg to path_allowlist_apply()
> -:  ---------- > 4:  a46f4c1bb8 upload-pack: read uploadpack.lazyFetchTrusted
> -:  ---------- > 5:  4063f233aa builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
>
>
> Christian Couder (5):
>   promisor-remote: factor out lazy_fetch_objects()
>   setup: extract path_allowlist_apply()
>   setup: add 'allow_dot' arg to path_allowlist_apply()
>   upload-pack: read uploadpack.lazyFetchTrusted
>   builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
>
>  Documentation/config/uploadpack.adoc  |  42 ++++++++++
>  Documentation/git-upload-pack.adoc    |   5 ++
>  Documentation/git.adoc                |   4 +-
>  builtin/upload-pack.c                 |  11 +++
>  promisor-remote.c                     |  76 ++++++++++--------
>  setup.c                               | 108 ++++++++++++++------------
>  setup.h                               |  28 +++++++
>  t/t5710-promisor-remote-capability.sh |  70 +++++++++++++++++
>  upload-pack.c                         |  37 +++++++++
>  upload-pack.h                         |   3 +
>  10 files changed, 304 insertions(+), 80 deletions(-)

What's missing is the information on the base.  I tried applying
these patches to 'v2.55.0' and the recent tips of 'master':

    2c78326f81 The 11th batch
    5b2471720c The 10th batch
    a97fcc37c2 The 9th batch
    13c7afec21 The 8th batch
    9a0c4701dc The 7th batch
    5d2e770923 The 6th batch
    48bbf81c29 The 5th batch
    41365c2a9b The 4th batch for Git 2.56
    d35c5399e3 The 3rd batch for Git 2.56
    55526a1826 The 2nd batch for Git 2.56

but the series did not apply to any of them.

It turns out the reason has nothing to do with your choice of
base.  It is because the series structure is not understood by 'b4'.

The cover letter I am responding to is a reply to another series,
but the patches in this round are not marked as 'v2'.  This seems
to cause 'b4' to grab patches from both series and smash them
together, resulting in an inapplicable mess.  It seems you cannot
have your cake and eat it, too 😠.

Next time, please do not thread the two topics together unless you
are marking the newer iteration with a higher 'vN' number.

Thanks.