Re: [PATCH 14/16] promisor-remote: trust known remotes matching acceptFromServerUrl

[Patrick Steinhardt <ps@pks.im>]

On Mon, Mar 23, 2026 at 09:05:17AM +0100, Christian Couder wrote:
> A previous commit introduced the `promisor.acceptFromServerUrl` config
> variable along with the machinery to parse and validate the URL glob
> patterns and optional remote name prefixes it contains. However, these
> URL patterns are not yet tied into the client's acceptance logic.
> 
> When a promisor remote is already configured locally, its fields (like
> authentication tokens) may occasionally need to be refreshed by the
> server. If `promisor.acceptFromServer` is set to the secure default
> ("None"), these updates are rejected, potentially causing future
> fetches to fail.

This only talks about already-configured remotes, and so I was naturally
wondering what about not-yet-configured remotes. But looking ahead a bit
shows that this is implemented in the next commit. Good.

> To enable such targeted updates for trusted URLs, let's use the URL
> patterns from `promisor.acceptFromServerUrl` as an additional URL
> based whitelist.
> 
> Concretely, let's check the advertised URLs against the URL glob
> patterns by introducing a new small helper function called
> url_matches_accept_list(), which iterates over the glob patterns and
> returns the first matching allowed_url entry (or NULL).
> 
> (Before matching, the advertised URL is passed through url_normalize()
> so that case variations in the scheme/host, percent-encoding tricks,
> and ".." path segments cannot bypass the whitelist.)
> 
> Let's then use this helper at the tail of should_accept_remote() so
> that, when `accept == ACCEPT_NONE`, a known remote whose URL matches
> the whitelist is still accepted.
> 
> To prepare for this new logic, let's also:
> 
>  - Add an 'accept_urls' parameter to should_accept_remote().
> 
>  - Replace the BUG() guard in the ACCEPT_KNOWN_URL case with an
>    explicit 'if (accept == ACCEPT_KNOWN_URL) return' and a new
>    BUG() guard in the ACCEPT_NONE case, so url_matches_accept_list()
>    is only called in the ACCEPT_NONE case.
> 
>  - Call accept_from_server_url() from filter_promisor_remote()
>    and relax its early return so that the function is entered when
>    `accept_urls` has entries even if `accept == ACCEPT_NONE`.
> 
> Let's then properly document `promisor.acceptFromServerUrl` in
> "promisor.adoc" as an additive security whitelist for known remotes,
> including the URL normalization behavior, and let's mention it in
> "gitprotocol-v2.adoc".

I feel like the description is steering a bit too strongly into the
direction of a step-by-step instruction of how to implement the change
rather than explaining what's done and why it's done this way.

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/config/promisor.adoc    | 46 +++++++++++++++++
>  Documentation/gitprotocol-v2.adoc     |  9 ++--
>  promisor-remote.c                     | 50 +++++++++++++++---
>  t/t5710-promisor-remote-capability.sh | 73 +++++++++++++++++++++++++++
>  4 files changed, 166 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
> index b0fa43b839..6f5442cd65 100644
> --- a/Documentation/config/promisor.adoc
> +++ b/Documentation/config/promisor.adoc
> @@ -51,6 +51,52 @@ promisor.acceptFromServer::
>  	to "fetch" and "clone" requests from the client. Name and URL
>  	comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
>  
> +promisor.acceptFromServerUrl::
> +	A glob pattern to specify which URLs advertised by a server
> +	are considered trusted by the client. This option acts as an
> +	additive security whitelist that works in conjunction with
> +	`promisor.acceptFromServer`.
> ++
> +This option can appear multiple times in config files. An advertised
> +URL will be accepted if it matches _ANY_ glob pattern specified by
> +this option in _ANY_ config file read by Git.
> ++
> +Be _VERY_ careful with these glob patterns, as it can be a big
> +security hole to allow any advertised remote to be auto-configured!
> +To minimize security risks, follow these guidelines:
> ++
> +1. Start with a secure protocol scheme, like `https://` or `ssh://`.
> ++
> +2. Only allow domain names or paths where you control and trust _ALL_
> +   the content. Be especially careful with shared hosting platforms
> +   like `github.com` or `gitlab.com`. A broad pattern like
> +   `https://gitlab.com/*` is dangerous because it trusts every
> +   repository on the entire platform. Always restrict such patterns to
> +   your specific organization or namespace (e.g.,
> +   `https://gitlab.com/your-org/*`).
> ++
> +3. Don't use globs (`*`) in the domain name. For example
> +   `https://cdn.example.com/*` is much safer than
> +   `https://*.example.com/*`, because the latter matches
> +   `https://evil-hacker.net/fake.example.com/repo`.
> ++
> +4. Make sure to have a `/` at the end of the domain name (or the end
> +   of specific directories). For example `https://cdn.example.com/*`
> +   is much safer than `https://cdn.example.com*`, because the latter
> +   matches `https://cdn.example.com.hacker.net/repo`.
> ++
> +Before matching, the advertised URL is normalized: the scheme and
> +host are lowercased, percent-encoded characters are decoded where
> +possible, and path segments like `..` are resolved.  Glob patterns
> +are matched against this normalized URL as-is, so patterns should
> +be written in normalized form (e.g., lowercase scheme and host).
> ++
> +Even if `promisor.acceptFromServer` is set to `None` (the default),
> +Git will still accept field updates (like tokens) for known remotes,
> +provided their URLs match a pattern in
> +`promisor.acceptFromServerUrl`. See linkgit:gitprotocol-v2[5] for
> +details on the protocol.

Given that there's a bunch to process here, would it make sense to give
users an example for how to do it properly?

I also wonder why we require a new config entry instead of extending
`promisor.acceptFromRemote` to have for example a new "url:https://..."
setting. Are there cases where you would ever want to use the new
URL-based schema with a different setting than "all"?

I guess the case with "none" is exactly that, where you may auto-update
configured remotes. But I wonder whether it would be more sensible to
split out behaviour of accepting and updating promisors into separate
configuration variables. These are ultimately different concerns, and
the interaction as layed out in this commit is somewhat non-obvious to
me.

Patrick