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

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

[Han Young]

From: Han Young <hanyoung@protonmail.com>

Following the discussing, I looked into the refs verify and fsck
commands. The refs verify command does not check the HEAD ref,
fsck checks the HEAD ref name and the commit object it points to.

I think we can move the fsck_head_link from fsck to refs. Then both
git fsck and git refs verify can use the same function to check the HEAD
ref.

However, there's no harm in adding a runtime check. 'git reset foo' will
happily create a ref file outside refs/ and return without error on an
invalid HEAD ref.

Change from v1:
* add a test in t7102-reset

Han Young (1):
  files-backend: check symref name before update

 refs/files-backend.c | 10 ++++++++++
 t/t7102-reset.sh     |  8 ++++++++
 2 files changed, 18 insertions(+)

-- 
2.51.0.373.g2c26b26d9

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

[Han Young]

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 ++++++++++
 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
-- 
2.51.0.373.g2c26b26d9

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

[Junio C Hamano]

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



> 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

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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