Re: [PATCH v2 4/4] sideband: add options to allow more control sequences to be passed through

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Patrick,

On Fri, 9 Jan 2026, Patrick Steinhardt wrote:

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

I agree that the feedback that elicited this patch did not specify any
concrete use case where this might be necessary. I basically implemented
this only to alleviate the reviewer feedback more than any real-world
issue.

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

I like that suggestion. Will change it.

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

I was thinking that 1) it keeps the implementation more consistent, and 2)
it would allow for an "oops, let's restart this" type of approach, saying
`color,erase,false,color`.

Might be over-engineered, but the alternative would have to take care of
special-casing `true` and `false` in the following warning (because they
_are_ recognized, they just wouldn't be recognized inside a
comma-separated list).

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

The code also was a lot more verbose. I know, because that's what it
looked like before I changed it. :-)

Ciao,
Johannes