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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 6, 2025, 15:52 UTC
Message-ID: <xmqqtt0cqb7g.fsf@gitster.g>
In-Reply-To: <20251006004639.GA1462753@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

>> 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.

Yes, it was Karthik, I think, who pointed out in the ealier round that the set-up procedure used to demonstrate the issue indicated that it was essentially a corrupt repository doing an unexpected thing, and I tend to agree. What you wrote in the previous paragraph matches the reason why I questioned "is this enough?"

> 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.

Thanks, looking forward to reading them.