From: Johannes Schindelin Date: Fri, 16 Jan 2026 19:38:22 GMT Subject: Re: [PATCH v2 3/4] sideband: do allow ANSI color sequences by default Message-ID: <53ed8f19-7084-b9ab-ca4c-cd558b75c1fc@gmx.de> In-Reply-To: Hi Patrick, On Fri, 9 Jan 2026, Patrick Steinhardt wrote: > On Wed, Dec 17, 2025 at 02:23:41PM +0000, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin > > > > 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. Indeed. The major concern here is to hide the fact from the user that Git already exited and that what they see in their terminal is not actually Git asking them to input something. Technically, this would be possible by setting the text to "invisible" (which would be a fine thing when pretending to ask for a password, anyway). But without the ability to move the cursor, attackers will have a much harder time to cover their tracks. > > 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`::". Will change. > > 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. I don't think that we can even get into a race condition because the config is cached after it is read. > > @@ -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 [ [ [; ]*] 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`. Later patches change the logic, though, to make `allow_control_characters` a bit field. So maybe it can be left as-is here? > > > + 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. Thank you for your review! Johannes