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

> On Thu, Oct 02, 2025 at 02:54:54AM -0700, Karthik Nayak wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > 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.
>> >>
>> >> 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.
>> >
>> 
>> Good point. I see that 'git fsck' does complain about this:
>> 
>>   $ git fsck
>>   Checking ref database: 100% (1/1), done.
>>   Checking object directories: 100% (256/256), done.
>>   error: invalid HEAD
>>   dangling commit ccd1771e44a18887197d3ee26ca37c2e892b9fb6
>>   dangling commit f99d68ea2c378218e2360dee4e24115c404f6a66
>> 
>> However 'git refs verify' doesn't...
>> 
>>   $ git refs verify --verbose
>>   Checking references consistency
>>   Checking refs/heads/master
>>   Checking packed-refs file .git/packed-refs
>> 
>> Okay, so this seems like because fsck also parses all references to mark
>> reachability and also parses 'HEAD' via `refs_resolve_ref_unsafe()`
>> which fails.
>> 
>> This symref checks and checking root refs is definitely something we
>> should consider adding to 'git refs verify'.
>
> Agreed! Overall, the goal is that all logic to verify references should
> be contained in `git refs verify`, so that git-fsck(1) only needs to
> shell out to that command to perform the full check.
>
> So if this logic isn't yet part of `git refs verify`, we should migrate
> it over.