From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 09 Jan 2026 12:38:26 GMT
Subject: Re: [PATCH v2 3/4] sideband: do allow ANSI color sequences by default
Message-ID: <aWD2wpyOo0Tr34OD@pks.im>
In-Reply-To: <44585ba1f4223f053820d82f1513c2258e1e0059.1765981422.git.gitgitgadget@gmail.com>

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.

Patrick