From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 02 Oct 2025 09:54:54 GMT
Subject: Re: [PATCH 1/1] files-backend: check symref name before update
Message-ID: <CAOLa=ZTnHQbg9ocdA1omqER6CJH-w30G14-F2JAQMtueXENWew@mail.gmail.com>
In-Reply-To: <xmqqv7ky1l70.fsf@gitster.g>

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

[snip]