Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize sideband channel messages #1853

Open
wants to merge 3 commits into
base: maint-2.47
Choose a base branch
from
+124 −2
Open

Sanitize sideband channel messages #1853

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7fb7a3
sideband: mask control characters
dscho Nov 6, 2024
14c612c
sideband: introduce an "escape hatch" to allow control characters
dscho Nov 6, 2024
a26c4ed
sideband: do allow ANSI color sequences by default
dscho Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions sideband.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
list_config_item(list, prefix, keywords[i].keyword);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Dscho

Just a couple of small comments

On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> > +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
> +{
> +	strbuf_grow(dest, n);
> +	for (; n && *src; src++, n--) {
> +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')

Isn't it a bug to pass '\n' to maybe_colorize_sideband() ?

> +			strbuf_addch(dest, *src);
> +		else {
> +			strbuf_addch(dest, '^');
> +			strbuf_addch(dest, 0x40 + *src);

This will escape DEL ('\x7f') as "^\xbf" which is invalid in utf-8 locales. Perhaps we could use "^?" for that instead.

> +test_expect_success 'disallow (color) control sequences in sideband' '
> +	write_script .git/color-me-surprised <<-\EOF &&
> +	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
> +	exec "$@"
> +	EOF
> +	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
> +	test_commit need-at-least-one-commit &&
> +	git clone --no-local . throw-away 2>stderr &&
> +	test_decode_color <stderr >decoded &&
> +	test_grep ! RED decoded

I'd be happier if we used test_cmp() here so that we check that the sanitized version matches what we expect and the test does not pass if there a typo in the script above stops it from writing the SGR code for red.

Best Wishes

Phillip

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Andreas Schwab wrote (reply to this):

On Jan 14 2025, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/sideband.c b/sideband.c
> index 02805573fab..c0b1cb044a3 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -65,6 +65,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
>  		list_config_item(list, prefix, keywords[i].keyword);
>  }
>  
> +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
> +{
> +	strbuf_grow(dest, n);
> +	for (; n && *src; src++, n--) {
> +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')

The argument of iscntrl needs to be converted to unsigned char.

-- 
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Andreas Schwab <[email protected]> writes:

> On Jan 14 2025, Johannes Schindelin via GitGitGadget wrote:
>
>> diff --git a/sideband.c b/sideband.c
>> index 02805573fab..c0b1cb044a3 100644
>> --- a/sideband.c
>> +++ b/sideband.c
>> @@ -65,6 +65,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
>>  		list_config_item(list, prefix, keywords[i].keyword);
>>  }
>>  
>> +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
>> +{
>> +	strbuf_grow(dest, n);
>> +	for (; n && *src; src++, n--) {
>> +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
>
> The argument of iscntrl needs to be converted to unsigned char.

If this were system-provided one, you are absolutely correct.

But I think this comes from 

sane-ctype.h:15:#undef iscntrl
sane-ctype.h:40:#define iscntrl(x) (sane_istest(x,GIT_CNTRL))

and sane_istest() does the casting to uchar for us, so this may be
OK (even if it may be a bit misleading).

}

static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
{
strbuf_grow(dest, n);
for (; n && *src; src++, n--) {
if (!iscntrl(*src) || *src == '\t' || *src == '\n')
strbuf_addch(dest, *src);
else {
strbuf_addch(dest, '^');
strbuf_addch(dest, 0x40 + *src);
}
}
}

/*
* Optionally highlight one keyword in remote output if it appears at the start
* of the line. This should be called for a single line only, which is
Expand All @@ -80,7 +93,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
int i;

if (!want_color_stderr(use_sideband_colors())) {
strbuf_add(dest, src, n);
strbuf_add_sanitized(dest, src, n);
return;
}

Expand Down Expand Up @@ -113,7 +126,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
}
}

strbuf_add(dest, src, n);
strbuf_add_sanitized(dest, src, n);
}


Expand Down
12 changes: 12 additions & 0 deletions t/t5409-colorize-remote-messages.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,16 @@ test_expect_success 'fallback to color.ui' '
grep "<BOLD;RED>error<RESET>: error" decoded
'

test_expect_success 'disallow (color) control sequences in sideband' '
write_script .git/color-me-surprised <<-\EOF &&
printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
exec "$@"
EOF
test_config_global uploadPack.packObjectshook ./color-me-surprised &&
test_commit need-at-least-one-commit &&
git clone --no-local . throw-away 2>stderr &&
test_decode_color <stderr >decoded &&
test_grep ! RED decoded
'

test_done