Re: [PATCH v2 3/4] sideband: do allow ANSI color sequences by default
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 9, 2026, 12:38 UTC
- 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:
Show 18 quoted lines
> 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.
Show 25 quoted lines
> 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`::".
Show 19 quoted lines
> 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.
Show 21 quoted lines
> @@ -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`.
Show 8 quoted lines
> + 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