From: Johannes Schindelin Date: Fri, 16 Jan 2026 19:47:39 GMT Subject: Re: [PATCH v2 4/4] sideband: add options to allow more control sequences to be passed through Message-ID: In-Reply-To: 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 > > > > 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