Re: [PATCH v2 3/4] sideband: do allow ANSI color sequences by default

> On Wed, Dec 17, 2025 at 02:23:41PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > The preceding two commits introduced special handling of the sideband
> > channel to neutralize ANSI escape sequences before sending the payload
> > to the terminal, and `sideband.allowControlCharacters` to override that
> > behavior.
> > 
> > However, as reported by brian m. carlson, some `pre-receive` hooks that
> > are actively used in practice want to color their messages and therefore
> > rely on the fact that Git passes them through to the terminal, even
> > though they have no way to determine whether the receiving side can
> > actually handle Escape sequences (think e.g. about the practice
> > recommended by Git that third-party applications wishing to use Git
> > functionality parse the output of Git commands).
> > 
> > In contrast to other ANSI escape sequences, it is highly unlikely that
> > coloring sequences can be essential tools in attack vectors that mislead
> > Git users e.g. by hiding crucial information.
> 
> The worst that they can do is to set up both fore- and background color
> to be the same so that text isn't visible. But I think that's an okay
> tradeoff.

> > Therefore we can have both: Continue to allow ANSI coloring sequences to
> > be passed to the terminal by default, and neutralize all other ANSI
> > Escape sequences.
> 
> Makes sense.
> 
> > diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
> > index 3fb5045cd7..e5b7383c7a 100644
> > --- a/Documentation/config/sideband.txt
> > +++ b/Documentation/config/sideband.txt
> > @@ -1,5 +1,17 @@
> >  sideband.allowControlCharacters::
> >  	By default, control characters that are delivered via the sideband
> > -	are masked, to prevent potentially unwanted ANSI escape sequences
> > -	from being sent to the terminal. Use this config setting to override
> > -	this behavior.
> > +	are masked, except ANSI color sequences. This prevents potentially
> > +	unwanted ANSI escape sequences from being sent to the terminal. Use
> > +	this config setting to override this behavior:
> > ++
> > +--
> > +	default::
> > +	color::
> > +		Allow ANSI color sequences, line feeds and horizontal tabs,
> > +		but mask all other control characters. This is the default.
> > +	false::
> > +		Mask all control characters other than line feeds and
> > +		horizontal tabs.
> > +	true::
> > +		Allow all control characters to be sent to the terminal.
> > +--
> 
> Nit: I think that our modern doc style requires the values to use
> backticks. E.g. "`default`::".

> > diff --git a/sideband.c b/sideband.c
> > index 997430f2ea..fb43008ab7 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -40,8 +45,26 @@ static int use_sideband_colors(void)
> >  	if (use_sideband_colors_cached >= 0)
> >  		return use_sideband_colors_cached;
> >  
> > -	git_config_get_bool("sideband.allowcontrolcharacters",
> > -			    &allow_control_characters);
> > +	switch (git_config_get_maybe_bool("sideband.allowcontrolcharacters", &i)) {
> > +	case 0: /* Boolean value */
> > +		allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS :
> > +			ALLOW_NO_CONTROL_CHARACTERS;
> > +		break;
> > +	case -1: /* non-Boolean value */
> > +		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
> > +					      &value))
> > +			; /* huh? `get_maybe_bool()` returned -1 */
> 
> This case is something that shouldn't happen in practice because we know
> that the config ought to exist. I guess it _could_ indicate a race
> condition, even though it's extremely unlikely to ever happen. So I was
> thinking about whether we want to `BUG()` here, but I guess just
> ignoring this is fine, as well.

> > @@ -70,9 +93,41 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
> >  		list_config_item(list, prefix, keywords[i].keyword);
> >  }
> >  
> > +static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n)
> > +{
> > +	int i;
> > +
> > +	/*
> > +	 * Valid ANSI color sequences are of the form
> > +	 *
> > +	 * ESC [ [<n> [; <n>]*] m
> > +	 *
> > +	 * These are part of the Select Graphic Rendition sequences which
> > +	 * contain more than just color sequences, for more details see
> > +	 * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
> > +	 */
> > +
> > +	if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
> > +	    n < 3 || src[0] != '\x1b' || src[1] != '[')
> > +		return 0;
> 
> This would break in case `allow_control_characters` allows _all_ ANSI
> sequences. But that doesn't matter right now because the function is
> only called via `strbuf_add_sanitized()` when we're sanitizing at least
> some characters.
> 
> Might be worth though to add a call to `BUG()` in case we see an
> unsupported value for `allow_control_characters`.

> 
> > +	for (i = 2; i < n; i++) {
> > +		if (src[i] == 'm') {
> > +			strbuf_add(dest, src, i + 1);
> > +			return i;
> > +		}
> > +		if (!isdigit(src[i]) && src[i] != ';')
> > +			break;
> > +	}
> 
> Okay, so this loop scans until we find the final "m" character that
> terminates the sequence. Looks good to me.