From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 09 Jan 2026 12:38:31 GMT
Subject: Re: [PATCH v2 4/4] sideband: add options to allow more control sequences to be passed through
Message-ID: <aWD2x154F5f-c3pL@pks.im>
In-Reply-To: <fe109cd3319a5e3a1d1982a53963a601bb62b81f.1765981422.git.gitgitgadget@gmail.com>

On Wed, Dec 17, 2025 at 02:23:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Even though control sequences that erase characters are quite juicy for
> attack scenarios, where attackers are eager to hide traces of suspicious
> activities, during the review of the side band sanitizing patch series
> concerns were raised that there might be some legimitate scenarios where
> Git server's `pre-receive` hooks use those sequences in a benign way.
> 
> Control sequences to move the cursor can likewise be used to hide tracks
> by overwriting characters, and have been equally pointed out as having
> legitimate users.
> 
> Let's add options to let users opt into passing through those ANSI
> Escape sequences: `sideband.allowControlCharacters` now supports also
> `cursor` and `erase`, and it parses the value as a comma-separated list.

Hm, okay. I don't really see much of a reason to allow these, but now
that the code exists already I don't see a reason why we should remove
those options again.

> diff --git a/sideband.c b/sideband.c
> index fb43008ab7..725e24db0d 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -28,9 +28,43 @@ static struct keyword_entry keywords[] = {
>  static enum {
>  	ALLOW_NO_CONTROL_CHARACTERS = 0,
>  	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
> +	ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1,
> +	ALLOW_ANSI_ERASE = 1<<2,
>  	ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
> -	ALLOW_ALL_CONTROL_CHARACTERS = 1<<1,
> -} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
> +	ALLOW_ALL_CONTROL_CHARACTERS = 1<<3,
> +} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;

Nit, not worth addressing on its own: readability would be helped a bit
if the assignments were all aligned.

        static enum {
                ALLOW_NO_CONTROL_CHARACTERS  = 0,
                ALLOW_ANSI_COLOR_SEQUENCES   = 1<<0,
                ALLOW_ANSI_CURSOR_MOVEMENTS  = 1<<1,
                ALLOW_ANSI_ERASE             = 1<<2,
                ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
                ALLOW_ALL_CONTROL_CHARACTERS = 1<<3,
        } allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;

> +static inline int skip_prefix_in_csv(const char *value, const char *prefix,
> +				     const char **out)
> +{
> +	if (!skip_prefix(value, prefix, &value) ||
> +	    (*value && *value != ','))
> +		return 0;
> +	*out = value + !!*value;
> +	return 1;
> +}
> +
> +static void parse_allow_control_characters(const char *value)
> +{
> +	allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
> +	while (*value) {
> +		if (skip_prefix_in_csv(value, "default", &value))
> +			allow_control_characters |= ALLOW_DEFAULT_ANSI_SEQUENCES;
> +		else if (skip_prefix_in_csv(value, "color", &value))
> +			allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES;
> +		else if (skip_prefix_in_csv(value, "cursor", &value))
> +			allow_control_characters |= ALLOW_ANSI_CURSOR_MOVEMENTS;
> +		else if (skip_prefix_in_csv(value, "erase", &value))
> +			allow_control_characters |= ALLOW_ANSI_ERASE;
> +		else if (skip_prefix_in_csv(value, "true", &value))
> +			allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS;
> +		else if (skip_prefix_in_csv(value, "false", &value))
> +			allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;

Does it really make sense to also handle "true" and "false" here? I
would expect that those values can only be passed standalone.

> +		else
> +			warning(_("unrecognized value for `sideband."
> +				  "allowControlCharacters`: '%s'"), value);
> +	}
> +}

This could be simplified if we used e.g. `string_list_split()`. But on
the other hand it avoids allocations, so that's a nice benefit.

Patrick