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

From: Jeff King <peff@peff.net>
Date: Oct 6, 2025, 00:46 UTC
Message-ID: <20251006004639.GA1462753@coredump.intra.peff.net>
In-Reply-To: <xmqq347xrp5o.fsf@gitster.g>

On Sun, Oct 05, 2025 at 02:53:39PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Han Young <hanyang.tony@bytedance.com> writes:
> 
> > 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().

Yes, if we wanted to add a check here, it should be doing the usual check for a syntactically valid refname and falling back to refname_is_safe() only for deletions.

But I'm not sure if this check is that valuable. We are in split_symref_update(), which takes an update to some symref and splits it into an update to that symref's reflog and a real update to the underlying target ref. So we are not checking input to the transaction here, but the existing state of the symref on disk. And in theory we should have checked that target already when we wrote it.

Do we want to check it again? I dunno. I can see an argument for being overly paranoid (garbage snuck in somehow, but we prefer not to act on it). We do already check sanity within resolve_ref_unsafe(), for example.

I do think there are also some gaps in our symref target checks (as well as a few other spots). I have a series to fix those that just needs a little bit of polishing, and hopefully can send out this coming week.

Show 15 quoted lines

> > 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
> > +'

This test won't work against the reftable backend.

-Peff