Re: [PATCH v2 2/4] sideband: introduce an "escape hatch" to allow control characters
Hi Junio,
On Thu, 18 Dec 2025, Junio C Hamano wrote:
Show 21 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
> > new file mode 100644
> > index 0000000000..3fb5045cd7
> > --- /dev/null
> > +++ b/Documentation/config/sideband.txt
> > @@ -0,0 +1,5 @@
> > +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.
>
> Two thoughts.
>
> - Users may want to say "I trust this remote host" or "I trust this
> remote repository". For that, something similar to what we do to
> `http.variable` to allow `http.<url>.variable` to take precedence
> over `http.variable` would be necessary.
Good idea! What do you think about something like this?
-- snip --
diff --git a/http.c b/http.c
index d59e59f66b1..14b5a95586c 100644
--- a/http.c
+++ b/http.c
@@ -19,6 +19,7 @@
#include "string-list.h"
#include "object-file.h"
#include "object-store-ll.h"
+#include "sideband.h"
static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
static int trace_curl_data = 1;
@@ -566,6 +567,9 @@ static int http_options(const char *var, const char *value,
return 0;
}
+ if (!strcmp("http.sanitizesideband", var))
+ return sideband_allow_control_characters_config(var, value);
+
/* Fall back on the default ones */
return git_default_config(var, value, ctx, data);
}
diff --git a/sideband.c b/sideband.c
index 725e24db0db..178c1320cac 100644
--- a/sideband.c
+++ b/sideband.c
@@ -26,13 +26,14 @@ static struct keyword_entry keywords[] = {
};
static enum {
+ ALLOW_CONTROL_SEQUENCES_UNSET = -1,
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;
+} allow_control_characters = ALLOW_CONTROL_SEQUENCES_UNSET;
static inline int skip_prefix_in_csv(const char *value, const char *prefix,
const char **out)
@@ -44,8 +45,19 @@ static inline int skip_prefix_in_csv(const char *value, const char *prefix,
return 1;
}
-static void parse_allow_control_characters(const char *value)
+int sideband_allow_control_characters_config(const char *var, const char *value)
{
+ switch (git_parse_maybe_bool(value)) {
+ case 0:
+ allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+ return 0;
+ case 1:
+ allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS;
+ return 0;
+ default:
+ break;
+ }
+
allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
while (*value) {
if (skip_prefix_in_csv(value, "default", &value))
@@ -61,9 +73,9 @@ static void parse_allow_control_characters(const char *value)
else if (skip_prefix_in_csv(value, "false", &value))
allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
else
- warning(_("unrecognized value for `sideband."
- "allowControlCharacters`: '%s'"), value);
+ warning(_("unrecognized value for '%s': '%s'"), var, value);
}
+ return 0;
}
/* Returns a color setting (GIT_COLOR_NEVER, etc). */
@@ -79,20 +91,12 @@ static int use_sideband_colors(void)
if (use_sideband_colors_cached >= 0)
return use_sideband_colors_cached;
- 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 */
- else
- parse_allow_control_characters(value);
- break;
- default:
- break; /* not configured */
+ if (allow_control_characters == ALLOW_CONTROL_SEQUENCES_UNSET) {
+ if (!git_config_get_value("sideband.allowcontrolcharacters", &value))
+ sideband_allow_control_characters_config("sideband.allowcontrolcharacters", value);
+
+ if (allow_control_characters == ALLOW_CONTROL_SEQUENCES_UNSET)
+ allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
}
if (!git_config_get_string_tmp(key, &value))
diff --git a/sideband.h b/sideband.h
index 5a25331be55..e711ad0f4e0 100644
--- a/sideband.h
+++ b/sideband.h
@@ -30,4 +30,11 @@ int demultiplex_sideband(const char *me, int status,
void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
+/*
+ * Parse and set the sideband allow control characters configuration.
+ * The var parameter should be the key name (without section prefix).
+ * Returns 0 if the variable was recognized and handled, non-zero otherwise.
+ */
+int sideband_allow_control_characters_config(const char *var, const char *value);
+
#endif
-- snap --
If this is the direction you're thinking, I'll polish it and integrate it
into v3.
> - It may no longer matter but a remote repository that may send
> messages as strings encoded in ISO/IEC 2022 would need to set
> this, merely to make the messages human-readable. There may be
> other reasons the trusted repositories want to send "escape
> sequences".
If the remote side has no way to determine whether the client side is
connected to a terminal or not (which we have already established in this
thread), it has even less chance to determine which character encoding is
in use...
> It might even be a good idea to make the default setting of this
> variable "allow", except for the initial connections to repositories
> (i.e., "git clone $URL", and "git fetch/ls-remote $URL" with an
> explicit $URL without using a nickname recorded in our .git/config),
> as visiting a potentially malicious remote repository you are not
> familiar with may not be uncommon, and users may deserve protection
> over inconvenience.
>
> But once the user establishes a working relationship with a remote
> repository, would it be a lot more common to trust the contents
> there than be on the lookout that the repository may spew bad
> strings of bytes at your standard error stream, I have to wonder.
I am not so sure whether that would be desirable, for (at least :-) ) two
reasons:
- `git fetch` with an explicit URL is sometimes used outside clone
scenarios, and in some clone-type scenarios, `git clone` cannot be used
(e.g. to establish credentials or to determine the appropriate sparse
checkout based on information from the tip revision).
I know that it is a delicate balance to strike between convenience and
security. Yet I also know that users prefer easy-to-explain mental
models and this logic would be a bit hard to explain: Why disallow
something while cloning or fetching with an explicit URL while allowing
the very same thing in a subsequent fetch?
tl;dr I expect users to be much more okay with the strategy to disallow
all but very few ANSI sequences by default, with a message that tells
them what to do if they want to enable more (or all) control sequences.
- I do not see how the user can inspect what the remote side does, even
after an initial clone. Therefore users would not have any reasonable
chance to gain any confidence that the remote side isn't doing anything
malicious. To the contrary, remote servers could specifically "behave"
during a clone, and launch the attack only during a fetch (indicated by
"have" lines in the request).
tl;dr remote servers don't get more trustworthy just by successfully
serving clones.
Does that reasoning make sense to you?
Ciao,
Johannes
Johannes Schindelin <johannes.schindelin@gmx.de>