Re: [PATCH 1/1] files-backend: check symref name before update

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2025, 19:22 UTC
Message-ID: <xmqqv7ky1l70.fsf@gitster.g>
In-Reply-To: <20251001150805.9652-2-hanyang.tony@bytedance.com>

Han Young <hanyang.tony@bytedance.com> writes:

Show 25 quoted lines

> From: Han Young <hanyoung@protonmail.com>
>
> In the ref files backend, the symbolic reference name is not checked
> before an update. This could cause reference and lock files to be created
> outside the refs/ directory. Validate the reference before adding it to
> the ref update transaction.
>
> Reported-by: Sigma <git@sigma-star.io>
> Signed-off-by: Han Young <hanyoung@protonmail.com>
> ---
>  refs/files-backend.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index bc3347d18..d47a8c392 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
>  	struct ref_update *new_update;
>  	unsigned int new_flags;
>  
> +	/*
> +	 * Check the referent is valid before adding it to the transaction.
> +	 */
> +	if (!refname_is_safe(referent)) {

Shouldn't this new condition share the logic with what is done by fsck? IOW, after doing this

  $ echo ref: refs/../HEAD > .git/HEAD

"git fsck" or "git refs verify" should barf (if not, we should make them barf), and this code should use the same logic to notice that the target of the symbolic ref is bogus.

Show 9 quoted lines

> +		strbuf_addf(err,
> +			    "reference '%s' appears to be broken",
> +			    update->refname);
> +		return -1;
> +	}
> +
>  	/*
>  	 * First make sure that referent is not already in the
>  	 * transaction. This check is O(lg N) in the transaction

Can we also have some tests?

Thanks.