Re: [PATCH v2 4/4] sideband: add options to allow more control sequences to be passed through
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 9, 2026, 12:38 UTC
- Message-ID
- <aWD2x154F5f-c3pL@pks.im>
- In-Reply-To
- <fe109cd3319a5e3a1d1982a53963a601bb62b81f.1765981422.git.gitgitgadget@gmail.com>
On Wed, Dec 17, 2025 at 02:23:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 15 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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.
Show 15 quoted lines
> 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;Show 26 quoted lines
> +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.
Show 5 quoted lines
> + 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.
Patrick