{"thread":{"id":"64246","subject":"[PATCH v=2 0/1] files-backend: check symref name before update","startedAt":"2025-10-04T14:42:35Z","lastAt":"2025-10-06T15:52:38Z","messageCount":5,"participants":["Han Young","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"527915","messageId":"20251004144223.23436-1-hanyang.tony@bytedance.com","threadId":"64246","inReplyTo":null,"subject":"[PATCH v=2 0/1] files-backend: check symref name before update","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-10-04T14:42:22Z","receivedAt":"2025-10-04T14:42:35Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"From: Han Young \n\nFollowing the discussing, I looked into the refs verify and fsck\ncommands. The refs verify command does not check the HEAD ref,\nfsck checks the HEAD ref name and the commit object it points to.\n\nI think we can move the fsck_head_link from fsck to refs. Then both\ngit fsck and git refs verify can use the same function to check the HEAD\nref.\n\nHowever, there's no harm in adding a runtime check. 'git reset foo' will\nhappily create a ref file outside refs/ and return without error on an\ninvalid HEAD ref.\n\nChange from v1:\n* add a test in t7102-reset\n\nHan Young (1):\n files-backend: check symref name before update\n\n refs/files-backend.c | 10 ++++++++++\n t/t7102-reset.sh | 8 ++++++++\n 2 files changed, 18 insertions(+)\n\n-- \n2.51.0.373.g2c26b26d9\n\n"},{"id":"527916","messageId":"20251004144223.23436-2-hanyang.tony@bytedance.com","threadId":"64246","inReplyTo":"20251004144223.23436-1-hanyang.tony@bytedance.com","subject":"[PATCH v=2 1/1] files-backend: check symref name before update","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-10-04T14:42:23Z","receivedAt":"2025-10-04T14:42:40Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"From: Han Young \n\nIn the ref files backend, the symbolic reference name is not checked\nbefore an update. This could cause reference and lock files to be created\noutside the refs/ directory. Validate the reference before adding it to\nthe ref update transaction.\n\nReported-by: Sigma \nSigned-off-by: Han Young \n---\n refs/files-backend.c | 10 ++++++++++\n t/t7102-reset.sh | 8 ++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bc3347d18..d47a8c392 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n \tstruct ref_update *new_update;\n \tunsigned int new_flags;\n \n+\t/*\n+\t * Check the referent is valid before adding it to the transaction.\n+\t */\n+\tif (!refname_is_safe(referent)) {\n+\t\tstrbuf_addf(err,\n+\t\t\t \"reference '%s' appears to be broken\",\n+\t\t\t update->refname);\n+\t\treturn -1;\n+\t}\n+\n \t/*\n \t * First make sure that referent is not already in the\n \t * transaction. This check is O(lg N) in the transaction\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 0503a64d3..1dc314474 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -634,4 +634,12 @@ test_expect_success 'reset handles --end-of-options' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reset should fail when HEAD is corrupt' '\n+\thead=$(cat .git/HEAD) &&\n+\thex=$(git log -1 --format=\"%h\") &&\n+\techo \"ref: refs/../foo\" > .git/HEAD &&\n+\ttest_must_fail git reset $hex &&\n+\techo $head > .git/HEAD\n+'\n+\n test_done\n-- \n2.51.0.373.g2c26b26d9\n\n"},{"id":"527956","messageId":"xmqq347xrp5o.fsf@gitster.g","threadId":"64246","inReplyTo":"20251004144223.23436-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH v=2 1/1] files-backend: check symref name before update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-05T21:53:39Z","receivedAt":"2025-10-05T21:53:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> From: Han Young \n>\n> In the ref files backend, the symbolic reference name is not checked\n> before an update. This could cause reference and lock files to be created\n> outside the refs/ directory. Validate the reference before adding it to\n> the ref update transaction.\n\nThis leaves the readers wondering why refname_is_safe(), which has\nno direct callers other than \"git show-ref verify\", is sufficient\nfor the purpose of this particular validation. All other callers of\nrefname_is_safe() seem to use it only as a sanity check combined\nwith other criteria.\n\nFor example, refs.c::transaction_refname_valid() calls\nrefname_is_safe() as a small part of its validation, together with\ncheck_refname_format(). It also refuses to touch anything that\nsatisfies is_pseudo_ref().\n\n\n\n> Reported-by: Sigma \n> Signed-off-by: Han Young \n> ---\n> refs/files-backend.c | 10 ++++++++++\n> t/t7102-reset.sh | 8 ++++++++\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index bc3347d18..d47a8c392 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n> \tstruct ref_update *new_update;\n> \tunsigned int new_flags;\n> \n> +\t/*\n> +\t * Check the referent is valid before adding it to the transaction.\n> +\t */\n> +\tif (!refname_is_safe(referent)) {\n> +\t\tstrbuf_addf(err,\n> +\t\t\t \"reference '%s' appears to be broken\",\n> +\t\t\t update->refname);\n> +\t\treturn -1;\n> +\t}\n> +\n> \t/*\n> \t * First make sure that referent is not already in the\n> \t * transaction. This check is O(lg N) in the transaction\n> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> index 0503a64d3..1dc314474 100755\n> --- a/t/t7102-reset.sh\n> +++ b/t/t7102-reset.sh\n> @@ -634,4 +634,12 @@ test_expect_success 'reset handles --end-of-options' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'reset should fail when HEAD is corrupt' '\n> +\thead=$(cat .git/HEAD) &&\n> +\thex=$(git log -1 --format=\"%h\") &&\n> +\techo \"ref: refs/../foo\" > .git/HEAD &&\n> +\ttest_must_fail git reset $hex &&\n> +\techo $head > .git/HEAD\n> +'\n> +\n> test_done\n"},{"id":"527957","messageId":"20251006004639.GA1462753@coredump.intra.peff.net","threadId":"64246","inReplyTo":"xmqq347xrp5o.fsf@gitster.g","subject":"Re: [PATCH v=2 1/1] files-backend: check symref name before update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-06T00:46:39Z","receivedAt":"2025-10-06T00:46:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 05, 2025 at 02:53:39PM -0700, Junio C Hamano wrote:\n\n> Han Young writes:\n> \n> > From: Han Young \n> >\n> > In the ref files backend, the symbolic reference name is not checked\n> > before an update. This could cause reference and lock files to be created\n> > outside the refs/ directory. Validate the reference before adding it to\n> > the ref update transaction.\n> \n> This leaves the readers wondering why refname_is_safe(), which has\n> no direct callers other than \"git show-ref verify\", is sufficient\n> for the purpose of this particular validation. All other callers of\n> refname_is_safe() seem to use it only as a sanity check combined\n> with other criteria.\n> \n> For example, refs.c::transaction_refname_valid() calls\n> refname_is_safe() as a small part of its validation, together with\n> check_refname_format(). It also refuses to touch anything that\n> satisfies is_pseudo_ref().\n\nYes, if we wanted to add a check here, it should be doing the usual\ncheck for a syntactically valid refname and falling back to\nrefname_is_safe() only for deletions.\n\nBut I'm not sure if this check is that valuable. We are in\nsplit_symref_update(), which takes an update to some symref and splits\nit into an update to that symref's reflog and a real update to the\nunderlying target ref. So we are not checking input to the transaction\nhere, but the existing state of the symref on disk. And in theory we\nshould have checked that target already when we wrote it.\n\nDo we want to check it again? I dunno. I can see an argument for being\noverly paranoid (garbage snuck in somehow, but we prefer not to act on\nit). We do already check sanity within resolve_ref_unsafe(), for\nexample.\n\nI do think there are also some gaps in our symref target checks (as well\nas a few other spots). I have a series to fix those that just needs a\nlittle bit of polishing, and hopefully can send out this coming week.\n\n> > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> > index 0503a64d3..1dc314474 100755\n> > --- a/t/t7102-reset.sh\n> > +++ b/t/t7102-reset.sh\n> > @@ -634,4 +634,12 @@ test_expect_success 'reset handles --end-of-options' '\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_success 'reset should fail when HEAD is corrupt' '\n> > +\thead=$(cat .git/HEAD) &&\n> > +\thex=$(git log -1 --format=\"%h\") &&\n> > +\techo \"ref: refs/../foo\" > .git/HEAD &&\n> > +\ttest_must_fail git reset $hex &&\n> > +\techo $head > .git/HEAD\n> > +'\n\nThis test won't work against the reftable backend.\n\n-Peff\n"},{"id":"527990","messageId":"xmqqtt0cqb7g.fsf@gitster.g","threadId":"64246","inReplyTo":"20251006004639.GA1462753@coredump.intra.peff.net","subject":"Re: [PATCH v=2 1/1] files-backend: check symref name before update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T15:52:35Z","receivedAt":"2025-10-06T15:52:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> This leaves the readers wondering why refname_is_safe(), which has\n>> no direct callers other than \"git show-ref verify\", is sufficient\n>> for the purpose of this particular validation. All other callers of\n>> refname_is_safe() seem to use it only as a sanity check combined\n>> with other criteria.\n>> \n>> For example, refs.c::transaction_refname_valid() calls\n>> refname_is_safe() as a small part of its validation, together with\n>> check_refname_format(). It also refuses to touch anything that\n>> satisfies is_pseudo_ref().\n>\n> Yes, if we wanted to add a check here, it should be doing the usual\n> check for a syntactically valid refname and falling back to\n> refname_is_safe() only for deletions.\n>\n> But I'm not sure if this check is that valuable. We are in\n> split_symref_update(), which takes an update to some symref and splits\n> it into an update to that symref's reflog and a real update to the\n> underlying target ref. So we are not checking input to the transaction\n> here, but the existing state of the symref on disk. And in theory we\n> should have checked that target already when we wrote it.\n\nYes, it was Karthik, I think, who pointed out in the ealier round\nthat the set-up procedure used to demonstrate the issue indicated\nthat it was essentially a corrupt repository doing an unexpected\nthing, and I tend to agree. What you wrote in the previous\nparagraph matches the reason why I questioned \"is this enough?\"\n\n> I do think there are also some gaps in our symref target checks (as well\n> as a few other spots). I have a series to fix those that just needs a\n> little bit of polishing, and hopefully can send out this coming week.\n\nThanks, looking forward to reading them.\n"}]}