From fa35fef800cd7437e832d89769035878af4b9d29 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 7 Aug 2026 15:55:07 +0200 Subject: [PATCH 1/5] promisor-remote: factor out lazy_fetch_objects() 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 Signed-off-by: Junio C Hamano --- promisor-remote.c | 76 ++++++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index f34856d499..b5889e09f1 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -29,15 +29,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) @@ -268,10 +259,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; @@ -288,9 +284,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, @@ -300,28 +324,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); } From bc0897872d51e4d770d96ea4f18692b6415d2ac6 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 7 Aug 2026 15:55:08 +0200 Subject: [PATCH 2/5] setup: extract path_allowlist_apply() 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 Signed-off-by: Junio C Hamano --- setup.c | 107 +++++++++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/setup.c b/setup.c index b4652651df..734875c146 100644 --- a/setup.c +++ b/setup.c @@ -1329,6 +1329,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; @@ -1342,54 +1400,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; } From 062daeb81d63a29ee7ac5439a56bf906707317f8 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 7 Aug 2026 15:55:09 +0200 Subject: [PATCH 3/5] setup: add 'allow_dot' arg to path_allowlist_apply() 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 Signed-off-by: Junio C Hamano --- setup.c | 9 +++++---- setup.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/setup.c b/setup.c index 734875c146..3fc906939d 100644 --- a/setup.c +++ b/setup.c @@ -1329,8 +1329,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; @@ -1356,7 +1357,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; } @@ -1400,7 +1401,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 705d1d6ff7..3bb9688fb7 100644 --- a/setup.h +++ b/setup.h @@ -299,4 +299,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), + * - "" sets it to 1 if equals `target_path`, + * - "" + "/" + "*" sets it to 1 if 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 */ From 33afb07a348e4cab5b0d8d22314b3704c6f2e83b Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 7 Aug 2026 15:55:10 +0200 Subject: [PATCH 4/5] upload-pack: read uploadpack.lazyFetchTrusted 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 Signed-off-by: Junio C Hamano --- 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 */ From 1ac27eed51463c1de59393fec48f8f710e2abf4e Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 7 Aug 2026 15:55:11 +0200 Subject: [PATCH 5/5] builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo 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 Signed-off-by: Junio C Hamano --- 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. `~/` expands to a +path relative to the home directory and `%(prefix)/` 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 <>). 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 b404ad9f0a..fb9f155867 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" &&