From 010fd39c3e230bc529e3c5ac7ec9eb36a7e8ab77 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 2 Oct 2026 10:23:18 +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. The latter is fine because the convention around promisor_remote_init() is that whoever needs to access the promisor remote information is expected to initialize it beforehand, and not that it should be initialized once at the very beginning before doing random things on promisor remotes. So moving its call site into lazy_fetch_objects(), which is the only code that needs the promisor remotes here, follows that convention. Nothing downstream of it, like is_promisor_object(), needs it when lazy fetching is disabled. While at it, let's document try_promisor_remotes() and the new lazy_fetch_objects() function, especially how their `remaining_oids`, `remaining_nr` and `to_free` arguments are used, as the ownership rules around `to_free` are easy to get wrong. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- promisor-remote.c | 83 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/promisor-remote.c b/promisor-remote.c index 43505d1e1a..91245fe9a8 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,9 +261,27 @@ static int remove_fetched_oids(struct repository *repo, return remaining_nr; } +/* + * Fetch the remaining objects (given in '*remaining_oids', which + * contains '*remaining_nr' object ids) from the known promisor + * remotes. If 'accepted_only' is true, ignore promisor remotes with + * their 'accepted' member unset. + * + * When a fetch from a remote fails, the objects that are still + * missing are computed, and '*remaining_oids' and '*remaining_nr' are + * updated accordingly before trying the next remote. In that case + * '*remaining_oids' points to a new array that this function + * allocated, and '*to_free' is set to 1 to tell the caller that it + * owns that array and should free it. '*to_free' should be 0 on the + * first call. + * + * Return 1 when all the requested objects have been fetched, 0 + * otherwise. + */ static int try_promisor_remotes(struct repository *repo, struct object_id **remaining_oids, - int *remaining_nr, int *to_free, + int *remaining_nr, + int *to_free, bool accepted_only) { struct promisor_remote *r = repo->promisor_remote_config->promisors; @@ -295,6 +304,38 @@ static int try_promisor_remotes(struct repository *repo, return 0; } +/* + * Lazily fetch the objects given in '*remaining_oids' from the + * promisor remotes, trying the accepted ones first. See + * try_promisor_remotes() above for how '*remaining_oids', + * '*remaining_nr' and '*to_free' are used. + * + * Return 1 when all the requested objects have been fetched, 0 + * otherwise. + */ +static int 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 0; + } + + promisor_remote_init(repo); + + /* Try accepted remotes first (those the server told us to use) */ + return try_promisor_remotes(repo, remaining_oids, remaining_nr, + to_free, true) || + try_promisor_remotes(repo, remaining_oids, remaining_nr, + to_free, false); +} + void promisor_remote_get_direct(struct repository *repo, const struct object_id *oids, int oid_nr) @@ -302,28 +343,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 96715284f4c2d08fd8eaf1ca2b4344e9e612b46d Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 2 Oct 2026 10:23:19 +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, by passing it a `bool (*allow_path)(const char *path, void *cbdata)` function that decides if a path is acceptable by the caller, and let's add a NEEDSWORK comment about it silently ignoring missing, possibly misspelled, paths. To further simplify how to reuse that new helper, and avoid duplicating the config-value handling in a future commit, let's also introduce a path_allowlist_config_apply() helper. As the new path_allowlist_apply() function reports its result through a `bool *matches` argument, let's also change the `int is_safe` member of `struct safe_directory_data` to a `bool`, so that its address can be passed as that argument. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- setup.c | 142 +++++++++++++++++++++++++++++++++++++------------------- setup.h | 50 ++++++++++++++++++++ 2 files changed, 143 insertions(+), 49 deletions(-) diff --git a/setup.c b/setup.c index dfe05d9a03..4deaf4d3f1 100644 --- a/setup.c +++ b/setup.c @@ -1338,67 +1338,111 @@ static int canonicalize_ceiling_entry(struct string_list_item *item, } } +void path_allowlist_apply(const char *allowed, const char *target_path, + bool *matches, + bool (*allow_path)(const char *path, void *cbdata), + void *allow_path_cbdata) +{ + char *normalized = NULL; + + if (!allowed || !*allowed) { + *matches = false; + return; + } + + if (!strcmp(allowed, "*")) { + *matches = true; + return; + } + + if (!allow_path(allowed, allow_path_cbdata)) + return; + + /* + * 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. + * + * NEEDSWORK: this also silently ignores misspelled paths. We + * may want to warn about a missing path unless it is marked + * as allowed to be missing, e.g., with an ":(optional)" + * prefix like pathname-typed configuration values, and hint + * about that prefix in the warning. + */ + normalized = real_pathdup(allowed, 0); + if (!normalized) + return; + + if (ends_with(normalized, "/*")) { + size_t len = strlen(normalized); + if (!fspathncmp(normalized, target_path, len - 1)) + *matches = true; + } else if (!fspathcmp(target_path, normalized)) { + *matches = true; + } + + free(normalized); +} + +void path_allowlist_config_apply(const char *key, const char *value, + const char *target_path, bool *matches, + bool (*allow_path)(const char *path, void *cbdata), + void *allow_path_cbdata) +{ + char *allowed = NULL; + + if (!value || !*value || !strcmp(value, "*")) { + path_allowlist_apply(value, target_path, matches, + allow_path, allow_path_cbdata); + return; + } + + if (git_config_pathname(&allowed, key, value) || !allowed) + return; + + path_allowlist_apply(allowed, target_path, matches, + allow_path, allow_path_cbdata); + + free(allowed); +} + +/* + * 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. + */ +static bool allow_safe_dir(const char *path, void *cbdata_) +{ + struct path_allowlist_cb_data *cbdata = cbdata_; + + if (is_absolute_path(path) || !strcmp(path, ".")) + return true; + + warning(_("%s '%s' not absolute"), cbdata->key, path); + return false; +} + struct safe_directory_data { char *path; - int is_safe; + bool is_safe; }; static int safe_directory_cb(const char *key, const char *value, const struct config_context *ctx UNUSED, void *d) { struct safe_directory_data *data = d; + struct path_allowlist_cb_data cbdata = { .key = key }; 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_config_apply(key, value, data->path, &data->is_safe, + allow_safe_dir, &cbdata); return 0; } diff --git a/setup.h b/setup.h index 763fd384e8..6b84fbe507 100644 --- a/setup.h +++ b/setup.h @@ -304,4 +304,54 @@ struct startup_info { extern struct startup_info *startup_info; extern const char *tmp_original_cwd; +/* Path allowlist */ + +struct path_allowlist_cb_data { + const char *key; +}; + +/* + * Check the allowlist entry in `allowed` against `target_path`, + * updating `*matches` accordingly. + * + * `allowed` is a single entry of an allowlist of paths, typically one + * value of a multi-valued config variable, already expanded by + * git_config_pathname(). `target_path` is the (normalized) path being + * tested. `*matches` is updated in place: + * + * - an empty `allowed` resets it to 'false' (so a later, more + * specific config scope can clear entries from a broader one), + * - "*" sets it to 'true' (allow everything), + * - "" sets it to 'true' if equals `target_path`, + * - "" + "/" + "*" sets it to 'true' if is a leading + * directory of `target_path`, + * - anything else leaves `*matches` unchanged. + * + * `allow_path` is called with `allowed` and `allow_path_cbdata`, and + * should return 'true' if the entry is acceptable to the caller. It + * lets each caller decide which paths it is willing to consider, and + * whether to warn about the ones it rejects. Returning 'false' leaves + * `*matches` unchanged. + * + * Callers are expected to invoke this once per allowlist entry, + * typically from a protected-config callback, so that untrusted + * repository config cannot influence the decision. + */ +void path_allowlist_apply(const char *allowed, const char *target_path, + bool *matches, + bool (*allow_path)(const char *path, void *cbdata), + void *allow_path_cbdata); + +/* + * Apply one value of a multi-valued config variable holding an + * allowlist of paths, expanding it with git_config_pathname() before + * checking it against `target_path`. Empty and "*" values are passed + * through without expansion, as interpolating them is not + * meaningful. See path_allowlist_apply(). + */ +void path_allowlist_config_apply(const char *key, const char *value, + const char *target_path, bool *matches, + bool (*allow_path)(const char *path, void *cbdata), + void *allow_path_cbdata); + #endif /* SETUP_H */ From 42896803342a649df195c95af127627a03d534f0 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 2 Oct 2026 10:23:20 +0200 Subject: [PATCH 3/5] upload-pack: read uploadpack.lazyFetchTrusted Previous commits created and prepared the path_allowlist_apply() and path_allowlist_config_apply() functions, but used them only for the "safe.directory" configuration variable. Let's reuse these functions 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. As path_allowlist_config_apply() lets each caller decide which paths it is willing to accept using a callback, let's pass it a new allow_trusted_path() callback. Unlike the "safe.directory" callback, it accepts only absolute paths, and not ".", as `upload-pack` always serves a repository given by an absolute path, so there is no "current repository" for "." to refer to. Note that a served repository is identified by its git directory, and not by its worktree. This is because `upload-pack` uses enter_repo() instead of the usual repository discovery, so it never learns about a worktree and `r->worktree` is always NULL there. In practice this means that a non-bare repository served as "/srv/repo" has to be allowlisted as "/srv/repo/.git" (or as the directory its ".git" file points to, if it has a ".git" file instead of a ".git" directory). The new upload_pack_lazy_fetch_trusted() 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 | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ upload-pack.h | 3 +++ 2 files changed, 62 insertions(+) diff --git a/upload-pack.c b/upload-pack.c index 22573ad365..a300870fa9 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) @@ -1343,6 +1345,63 @@ static int upload_pack_config(const char *var, const char *value, return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs); } +/* + * Only absolute paths make sense here. Unlike 'safe.directory', "." + * is not accepted, as the served repository is always identified by + * an absolute path. + */ +static bool allow_trusted_path(const char *path, void *cbdata_) +{ + struct path_allowlist_cb_data *cbdata = cbdata_; + + if (is_absolute_path(path)) + return true; + + warning(_("%s '%s' not absolute"), cbdata->key, path); + return false; +} + +struct lazy_fetch_trusted { + char *repo_path; + bool trusted; +}; + +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; + struct path_allowlist_cb_data cbdata = { .key = var }; + + if (strcmp("uploadpack.lazyfetchtrusted", var)) + return 0; + + path_allowlist_config_apply(var, value, data->repo_path, &data->trusted, + allow_trusted_path, &cbdata); + + return 0; +} + +bool upload_pack_lazy_fetch_trusted(struct repository *r) +{ + struct lazy_fetch_trusted data = { 0 }; + + /* + * A served repository is identified by its git directory, as + * `upload-pack` uses enter_repo() instead of the usual repository + * discovery, so its worktree, if any, is never known here. + */ + data.repo_path = real_pathdup(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 4360734e907c9c92e72217a8ba9bdc1989f3c3a5 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 2 Oct 2026 10:23:21 +0200 Subject: [PATCH 4/5] promisor-remote: prevent infinite recursion when lazy fetching If a repository R is configured to lazy fetch from a promisor remote P which is also configured to in turn lazy fetch from R, there is an infinite recursion: R asks P for a missing object, P asks R for it, and so on. The simplest case of this is a repository configured as its own promisor remote. This is not reachable when serving a repository by default, as `upload-pack` sets `GIT_NO_LAZY_FETCH` to 1, which makes the nested `upload-pack` refuse to lazily fetch. A following commit will let server operators allow lazy fetching for repositories they trust though, and as `GIT_NO_LAZY_FETCH` is then set to 0 and passed down to child processes, nothing stops the recursion anymore. It does not recurse forever in practice, but only because each level adds one more variable to the environment of the child process, so after a while `exec()` fails with: fatal: cannot exec 'git-upload-pack ...': Argument list too long fatal: unable to fork To avoid this pathological case altogether, let's use a new `GIT_INTERNAL_LAZY_FETCH_DEPTH` to count the recursion depth, and let's check that it doesn't exceed a MAX_LAZY_FETCH_DEPTH limit (set to 5 for now). Note that some nesting is legitimate: when `git fetch` runs `index-pack`, it can lazily fetch REF_DELTA bases that are missing locally, so the limit should not be 1. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- environment.h | 8 ++++++++ promisor-remote.c | 26 ++++++++++++++++++++++---- t/t0410-partial-clone.sh | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 4 deletions(-) diff --git a/environment.h b/environment.h index e7ec5b0437..f2833be9fe 100644 --- a/environment.h +++ b/environment.h @@ -52,6 +52,14 @@ */ #define GIT_ADVICE_ENVIRONMENT "GIT_ADVICE" +/* + * Environment variable used to detect that a lazy fetch is already in + * progress in a parent process, to prevent infinite recursion when a + * promisor remote resolves back to the repository being served. + * This is an internal variable that should not be set by the user. + */ +#define LAZY_FETCH_DEPTH_ENVIRONMENT "GIT_INTERNAL_LAZY_FETCH_DEPTH" + /* * Environment variable used in handshaking the wire protocol. * Contains a colon ':' separated list of keys with optional values diff --git a/promisor-remote.c b/promisor-remote.c index 91245fe9a8..316d9950aa 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -24,7 +24,7 @@ struct promisor_remote_config { static int fetch_objects(struct repository *repo, const char *remote_name, const struct object_id *oids, - int oid_nr) + int oid_nr, int depth) { struct child_process child = CHILD_PROCESS_INIT; int i; @@ -41,6 +41,7 @@ static int fetch_objects(struct repository *repo, "--filter=blob:none", "--stdin", NULL); if (!repo_config_get_bool(repo, "promisor.quiet", &quiet) && quiet) strvec_push(&child.args, "--quiet"); + strvec_pushf(&child.env, "%s=%d", LAZY_FETCH_DEPTH_ENVIRONMENT, depth + 1); if (start_command(&child)) die(_("promisor-remote: unable to fork off fetch subprocess")); child_in = xfdopen(child.in, "w"); @@ -282,6 +283,7 @@ static int try_promisor_remotes(struct repository *repo, struct object_id **remaining_oids, int *remaining_nr, int *to_free, + int depth, bool accepted_only) { struct promisor_remote *r = repo->promisor_remote_config->promisors; @@ -289,7 +291,8 @@ static int try_promisor_remotes(struct repository *repo, for (; r; r = r->next) { if (accepted_only != r->accepted) continue; - if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) { + if (fetch_objects(repo, r->name, + *remaining_oids, *remaining_nr, depth) < 0) { if (*remaining_nr == 1) continue; *remaining_nr = remove_fetched_oids(repo, remaining_oids, @@ -304,6 +307,8 @@ static int try_promisor_remotes(struct repository *repo, return 0; } +#define MAX_LAZY_FETCH_DEPTH 5 + /* * Lazily fetch the objects given in '*remaining_oids' from the * promisor remotes, trying the accepted ones first. See @@ -318,6 +323,8 @@ static int lazy_fetch_objects(struct repository *repo, int *remaining_nr, int *to_free) { + int depth = (int)git_env_ulong(LAZY_FETCH_DEPTH_ENVIRONMENT, 0); + if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) { static int warning_shown; if (!warning_shown) { @@ -327,13 +334,24 @@ static int lazy_fetch_objects(struct repository *repo, return 0; } + if (depth >= MAX_LAZY_FETCH_DEPTH) { + static int warning_shown; + if (!warning_shown) { + warning_shown = 1; + warning(_("too many nested lazy fetches (%d); " + "is a promisor remote pointing at the repository itself?"), + depth); + } + return 0; + } + promisor_remote_init(repo); /* Try accepted remotes first (those the server told us to use) */ return try_promisor_remotes(repo, remaining_oids, remaining_nr, - to_free, true) || + to_free, depth, true) || try_promisor_remotes(repo, remaining_oids, remaining_nr, - to_free, false); + to_free, depth, false); } void promisor_remote_get_direct(struct repository *repo, diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 788e9a1631..a54685e3c7 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -709,6 +709,39 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' ' test_grep ! "[?]$FILE_HASH" out ' +test_expect_success 'lazy-fetch does not recurse infinitely between two promisor remotes' ' + rm -rf full partial1.git partial2.git && + + # Create a repo with a blob + test_create_repo full && + test_config -C full uploadpack.allowfilter 1 && + test_config -C full uploadpack.allowanysha1inwant 1 && + test_commit -C full create-a-file file.txt && + FILE_HASH=$(git -C full rev-parse HEAD:file.txt) && + + # Create partial clone repos without blobs + git clone --filter=blob:none --bare "file://$(pwd)/full" partial1.git && + git clone --filter=blob:none --bare "file://$(pwd)/full" partial2.git && + test_config -C partial1.git uploadpack.allowfilter 1 && + test_config -C partial1.git uploadpack.allowanysha1inwant 1 && + test_config -C partial2.git uploadpack.allowfilter 1 && + test_config -C partial2.git uploadpack.allowanysha1inwant 1 && + + # Configure the partial repos as remotes of each other + git -C partial2.git remote set-url origin "file://$(pwd)/partial1.git" && + git -C partial1.git remote set-url origin "file://$(pwd)/partial2.git" && + + # Make sure lazy fetching fails + test_must_fail env GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 \ + git -C partial1.git cat-file -e "$FILE_HASH" 2>err && + test_grep "too many nested lazy fetches" err && + + # Make sure the recursion was bounded, i.e. that only + # MAX_LAZY_FETCH_DEPTH "git fetch" subprocesses were spawned + grep "run_command: GIT_INTERNAL_LAZY_FETCH_DEPTH" trace >fetches && + test_line_count = 5 fetches +' + test_expect_success 'push should not fetch new commit objects' ' rm -rf server client && test_create_repo server && From 85c7f5e7dac12e31444e9bab95e1af7fa74955eb Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 2 Oct 2026 10:23:22 +0200 Subject: [PATCH 5/5] builtin/upload-pack: don't disable lazy fetching 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. Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default, 2024-04-16), `upload-pack` sets `GIT_NO_LAZY_FETCH` to 1 itself, unconditionally, because by default it shouldn't trust the repositories it serves. Lazily fetching runs `git fetch`, which may execute arbitrary commands specified in the configuration and hooks of the served repo. The new "uploadpack.lazyFetchTrusted" protected config variable is not about overriding an environment variable. It's rather about teaching the code that automatically sets `GIT_NO_LAZY_FETCH` (because it had no way to know if the served repo could be trusted) to look at the new config variable to find out if a server operator actually vouched for that repo. Let's implement that, so we now have the following cases: - if `GIT_NO_LAZY_FETCH` is already set, we honor it and leave it alone, as it comes from the server operator, - otherwise, if the served repo is in the "uploadpack.lazyFetchTrusted" allowlist, we don't disable lazy fetching, - otherwise, we disable lazy fetching, as we used to. This 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. Note that what a server operator vouches for by listing a repo there is that the promisor remotes this repo is configured to lazily fetch from, as well as its configuration and hooks, are trustworthy. Whether a client trusts the repo it fetches from is a separate matter, and up to the client. As `pack-objects`, which performs the lazy fetch when serving a client, is a child process of `upload-pack`, not setting `GIT_NO_LAZY_FETCH` in `upload-pack` is enough for it to be allowed to lazily fetch, without any further plumbing. On the other hand, as we leave `GIT_NO_LAZY_FETCH` unset for a trusted repo instead of setting it to 0, the trust doesn't propagate: if a trusted repo lazily fetches from a promisor remote that is itself served by `upload-pack` on the same machine, that nested `upload-pack` decides for its own repo. This is unlike when a server operator sets `GIT_NO_LAZY_FETCH` to 0, as that is inherited by all child processes. Now that "uploadpack.lazyFetchTrusted" is actually doing something, let's document it (including this difference with `GIT_NO_LAZY_FETCH`), let's reference it from `GIT_NO_LAZY_FETCH`'s docs, and let's add tests for it. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/config/uploadpack.adoc | 57 +++++++ Documentation/git-upload-pack.adoc | 9 +- Documentation/git.adoc | 4 +- builtin/upload-pack.c | 19 ++- t/t5710-promisor-remote-capability.sh | 230 ++++++++++++++++++++++++++ 5 files changed, 316 insertions(+), 3 deletions(-) diff --git a/Documentation/config/uploadpack.adoc b/Documentation/config/uploadpack.adoc index 0e1dda944a..242a2c485a 100644 --- a/Documentation/config/uploadpack.adoc +++ b/Documentation/config/uploadpack.adoc @@ -86,3 +86,60 @@ 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:: + A multi-valued configuration variable, each value of which + specifies the absolute local path of a repository that + `upload-pack` is allowed to lazily fetch missing objects for. ++ +A repository is identified by its git directory, after following any +`.git` file and resolving symbolic links. That is the repository +itself if it is bare, the `.git` directory of a repository that has a +worktree, or the directory that a `.git` file points to, for example +when the repository was created with `--separate-git-dir` or for a +linked worktree (see linkgit:git-worktree[1]). So a non-bare +repository served as `/srv/repo` usually has to be allowlisted as +`/srv/repo/.git`. 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. ++ +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 similar to setting +`GIT_NO_LAZY_FETCH` to `0`, but only for the matching repositories: +unlike that environment variable, the trust is not inherited by child +processes. So if a trusted repository lazily fetches from a promisor +remote that is itself served by `upload-pack` on the same machine, +for example through a local path or a `file://` URL, lazy fetching is +allowed there only if that promisor remote is also listed here. 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. In particular, a trusted repository +should not be configured as its own promisor remote, as `upload-pack` +would then try to lazily fetch missing objects from the repository +itself, which is pointless. ++ +As this is a multi-valued setting, 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 an `uploadpack.lazyFetchTrusted` +entry with an empty value. ++ +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..9e3a3fe142 100644 --- a/Documentation/git-upload-pack.adoc +++ b/Documentation/git-upload-pack.adoc @@ -70,7 +70,14 @@ This is implemented by having `upload-pack` internally set the `GIT_NO_LAZY_FETCH` variable to `1`. If you want to override it (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`. +`0`. As it is an environment variable, it is also inherited by child +processes, including any `upload-pack` run to lazily fetch from a +promisor remote on the same machine. ++ +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..53e76deb23 100644 --- a/builtin/upload-pack.c +++ b/builtin/upload-pack.c @@ -46,7 +46,6 @@ int cmd_upload_pack(int argc, packet_trace_identity("upload-pack"); disable_replace_refs(); save_commit_buffer = 0; - xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 0); argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0); @@ -62,6 +61,24 @@ 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); + /* + * Lazily fetching while serving a client would run `git fetch`, + * which may execute arbitrary commands from the configuration + * and hooks of the served repo, so we disable it by default as + * we trust nobody. There are two ways for a server operator to + * allow it though: + * + * - if GIT_NO_LAZY_FETCH is already set, we leave it alone and + * honor whatever the operator put there, + * + * - otherwise, if the served repo is in the + * "uploadpack.lazyFetchTrusted" protected allowlist, we + * don't disable lazy fetching. + */ + if (!getenv(NO_LAZY_FETCH_ENVIRONMENT) && + !upload_pack_lazy_fetch_trusted(the_repository)) + xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 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..463e9e00b0 100755 --- a/t/t5710-promisor-remote-capability.sh +++ b/t/t5710-promisor-remote-capability.sh @@ -173,6 +173,236 @@ 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 "trusted repo as its own promisor remote does not recurse" ' + # No promisors are advertised + git -C server config promisor.advertise false && + test_when_finished "rm -rf client" && + + # Add itself as its own remote + git -C server remote add self "$TRASH_DIRECTORY_URL/server" && + git -C server config remote.self.promisor true && + test_when_finished "git -C server remote remove self" && + + # Make "self" the only promisor remote of the server, so that it + # cannot get the missing object from "lop". Note that + # "remote.lop.partialCloneFilter" also makes "lop" a promisor + # remote, so it has to be unset too. + git -C server config --unset remote.lop.promisor && + test_when_finished "git -C server config remote.lop.promisor true" && + lop_filter="$(git -C server config remote.lop.partialCloneFilter)" && + git -C server config --unset remote.lop.partialCloneFilter && + test_when_finished "git -C server config remote.lop.partialCloneFilter \"$lop_filter\"" && + + # Allow lazy fetching from itself + test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" && + + # Check that lazy fetching fails + test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err && + test_grep "too many nested lazy fetches" err && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + +test_expect_success "uploadpack.lazyFetchTrusted needs the git dir of a non-bare repo" ' + test_when_finished "rm -rf nonbare nonbare-pack-* client client2" && + + # Create a non-bare repo, without any worktree content, so that + # its largest object can be filtered out below + git init nonbare && + git -C nonbare remote add origin "$TRASH_DIRECTORY_URL/template" && + git -C nonbare fetch origin && + git -C nonbare update-ref HEAD FETCH_HEAD && + + git -C nonbare remote add lop "$TRASH_DIRECTORY_URL/lop" && + git -C nonbare config remote.lop.promisor true && + git -C nonbare config uploadpack.allowFilter true && + git -C nonbare config uploadpack.allowAnySHA1InWant true && + git -C nonbare config promisor.advertise false && + + # Repack everything, then repack without the largest object and + # create a promisor pack, like initialize_server() does + git -C nonbare -c repack.writebitmaps=false repack -a -d && + rm -f nonbare/.git/objects/pack/*.promisor && + git -C nonbare -c repack.writebitmaps=false repack -a -d \ + --filter=blob:limit=5k --filter-to="$(pwd)/nonbare-pack" && + promisor_file=$(ls nonbare/.git/objects/pack/*.pack | sed "s/\.pack/.promisor/") && + >"$promisor_file" && + check_missing_objects nonbare 1 "$oid" && + + # The worktree path does not identify the repo, so it is not + # trusted and the clone fails + test_config_global uploadpack.lazyFetchTrusted "$(pwd)/nonbare" && + test_must_fail git clone --no-local --filter="blob:limit=1k" \ + nonbare client 2>err && + test_grep "lazy fetching disabled" err && + check_missing_objects nonbare 1 "$oid" && + + # The git dir identifies the repo, so it is trusted and the + # clone succeeds + test_config_global uploadpack.lazyFetchTrusted "$(pwd)/nonbare/.git" && + git clone --no-local --filter="blob:limit=1k" nonbare client2 && + check_missing_objects nonbare 0 "" +' + +test_expect_success "uploadpack.lazyFetchTrusted needs the git dir a .git file points to" ' + test_when_finished "rm -rf sepwt sepgit sep-pack-* client client2" && + + # Create a non-bare repo with a ".git" file pointing to a + # separate git dir, without any worktree content, so that its + # largest object can be filtered out below + git init --separate-git-dir="$(pwd)/sepgit" sepwt && + test_path_is_file sepwt/.git && + git -C sepwt remote add origin "$TRASH_DIRECTORY_URL/template" && + git -C sepwt fetch origin && + git -C sepwt update-ref HEAD FETCH_HEAD && + + git -C sepwt remote add lop "$TRASH_DIRECTORY_URL/lop" && + git -C sepwt config remote.lop.promisor true && + git -C sepwt config uploadpack.allowFilter true && + git -C sepwt config uploadpack.allowAnySHA1InWant true && + git -C sepwt config promisor.advertise false && + + # Repack everything, then repack without the largest object and + # create a promisor pack, like initialize_server() does + git -C sepwt -c repack.writebitmaps=false repack -a -d && + rm -f sepgit/objects/pack/*.promisor && + git -C sepwt -c repack.writebitmaps=false repack -a -d \ + --filter=blob:limit=5k --filter-to="$(pwd)/sep-pack" && + promisor_file=$(ls sepgit/objects/pack/*.pack | sed "s/\.pack/.promisor/") && + >"$promisor_file" && + check_missing_objects sepwt 1 "$oid" && + + # The ".git" file does not identify the repo, so it is not + # trusted and the clone fails + test_config_global uploadpack.lazyFetchTrusted "$(pwd)/sepwt/.git" && + test_must_fail git clone --no-local --filter="blob:limit=1k" \ + sepwt client 2>err && + test_grep "lazy fetching disabled" err && + check_missing_objects sepwt 1 "$oid" && + + # The git dir the ".git" file points to identifies the repo, so + # it is trusted and the clone succeeds + test_config_global uploadpack.lazyFetchTrusted "$(pwd)/sepgit" && + git clone --no-local --filter="blob:limit=1k" sepwt client2 && + check_missing_objects sepwt 0 "" +' + +test_expect_success "uploadpack.lazyFetchTrusted trust does not propagate to promisor remotes" ' + # No promisors are advertised + git -C server config promisor.advertise false && + test_when_finished "rm -rf client lop2" && + + # Create "lop2", a partial clone that is also missing the + # largest object, and that can lazily fetch it from "lop" + test_config -C template uploadpack.allowFilter true && + git clone --bare --no-local --filter="blob:limit=5k" \ + "$TRASH_DIRECTORY_URL/template" lop2 && + git -C lop2 remote set-url origin "$TRASH_DIRECTORY_URL/lop" && + git -C lop2 config uploadpack.allowFilter true && + git -C lop2 config uploadpack.allowAnySHA1InWant true && + check_missing_objects lop2 1 "$oid" && + + # Make "lop2" the only promisor remote of the server. Note that + # "remote.lop.partialCloneFilter" also makes "lop" a promisor + # remote, so it has to be unset too. + git -C server remote add lop2 "$TRASH_DIRECTORY_URL/lop2" && + git -C server config remote.lop2.promisor true && + test_when_finished "git -C server remote remove lop2" && + git -C server config --unset remote.lop.promisor && + test_when_finished "git -C server config remote.lop.promisor true" && + lop_filter="$(git -C server config remote.lop.partialCloneFilter)" && + git -C server config --unset remote.lop.partialCloneFilter && + test_when_finished "git -C server config remote.lop.partialCloneFilter \"$lop_filter\"" && + + # Only the server is trusted, not "lop2", so the upload-pack + # serving "lop2" to the server refuses to lazily fetch from "lop" + test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" && + test_must_fail git clone --no-local --filter="blob:limit=5k" \ + server client 2>err && + test_grep "lazy fetching disabled" err && + check_missing_objects server 1 "$oid" && + check_missing_objects lop2 1 "$oid" && + + # Once "lop2" is also trusted, the clone succeeds + git config --global --add uploadpack.lazyFetchTrusted "$(pwd)/lop2" && + git clone --no-local --filter="blob:limit=5k" server client && + check_missing_objects server 0 "" && + + # Reinitialize server so that the largest object is missing again + initialize_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" &&