Re: [PATCH v=2 1/1] files-backend: check symref name before update
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 5, 2025, 21:53 UTC
- Message-ID
- <xmqq347xrp5o.fsf@gitster.g>
- In-Reply-To
- <20251004144223.23436-2-hanyang.tony@bytedance.com>
Han Young <hanyang.tony@bytedance.com> writes:
Show 6 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.
This leaves the readers wondering why refname_is_safe(), which has no direct callers other than "git show-ref verify", is sufficient for the purpose of this particular validation. All other callers of refname_is_safe() seem to use it only as a sanity check combined with other criteria.
For example, refs.c::transaction_refname_valid() calls refname_is_safe() as a small part of its validation, together with check_refname_format(). It also refuses to touch anything that satisfies is_pseudo_ref().
Show 45 quoted lines
> Reported-by: Sigma <git@sigma-star.io>
> Signed-off-by: Han Young <hanyoung@protonmail.com>
> ---
> refs/files-backend.c | 10 ++++++++++
> t/t7102-reset.sh | 8 ++++++++
> 2 files changed, 18 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)) {
> + 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
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 0503a64d3..1dc314474 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -634,4 +634,12 @@ test_expect_success 'reset handles --end-of-options' '
> test_cmp expect actual
> '
>
> +test_expect_success 'reset should fail when HEAD is corrupt' '
> + head=$(cat .git/HEAD) &&
> + hex=$(git log -1 --format="%h") &&
> + echo "ref: refs/../foo" > .git/HEAD &&
> + test_must_fail git reset $hex &&
> + echo $head > .git/HEAD
> +'
> +
> test_done