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

[Junio C Hamano <gitster@pobox.com>]

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