[PATCH] t5702: be more resilient to env quoting

[PATCH] t5702: be more resilient to env quoting

[Jeff King]

A few tests in t5702 check whether we specify the v2 protocol via the
GIT_PROTOCOL environment variable. They do this by redirecting the
output of "env" into a file and grepping it. But our exact pattern
depends on how env quotes its output. GNU coreutils 9.10 will output
this:

  GIT_PROTOCOL=version=2

but version 9.12 will output this:

  GIT_PROTOCOL='version=2'

It doesn't quote every variable, but the presence of "=" now triggers
it. We could loosen our regex to accommodate both versions, but there's
a simpler fix. We only care about the value of one variable, so we can
just print it using the shell invoked for the server-side command.

Note that this new formula is slightly less strict: it allows
GIT_PROTOCOL as an empty string in addition to it being missing. But
that's OK; they both mean the same thing to the child Git process.

Likewise, it's actually more strict when we do expect the v2 protocol:
it checks the contents of the whole GIT_PROTOCOL rather than a partial
grep. In practice it's the same for now (we only ever set protocol=2),
but I think in the long run being strict is better (if we do add
something else, the test will break and we can consider how to handle it
then).

Signed-off-by: Jeff King <peff@peff.net>
---
The new coreutils hit Debian unstable, so I ran into this doing my usual
daily "make test". I wondered what idiot did the overly-complicated
"env" invocation in the first place, but naturally it turns out it was
me.

 t/t5702-protocol-v2.sh | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 7e69ba7f84..a66adfbc3b 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -929,28 +929,35 @@ test_expect_success 'file:// --negotiate-only with protocol v0' '
 test_expect_success 'push with custom path does not request v2' '
 	rm -f env.trace &&
 	git -C client push \
-		--receive-pack="env >../env.trace; git-receive-pack" \
+		--receive-pack="
+			echo GIT_PROTOCOL=\$GIT_PROTOCOL >../env.trace
+			git-receive-pack" \
 		origin HEAD:refs/heads/custom-push-test &&
-	test_path_is_file env.trace &&
-	test_grep ! ^GIT_PROTOCOL env.trace
+	echo GIT_PROTOCOL= >expect &&
+	test_cmp expect env.trace
 '
 
 test_expect_success 'fetch with custom path does request v2' '
 	rm -f env.trace &&
 	git -C client fetch \
-		--upload-pack="env >../env.trace; git-upload-pack" \
+		--upload-pack="
+			echo GIT_PROTOCOL=\$GIT_PROTOCOL >../env.trace
+			git-upload-pack" \
 		origin HEAD &&
-	test_grep ^GIT_PROTOCOL=version=2 env.trace
+	echo GIT_PROTOCOL=version=2 >expect &&
+	test_cmp expect env.trace
 '
 
 test_expect_success 'archive with custom path does not request v2' '
 	rm -f env.trace &&
 	git -C client archive \
-		--exec="env >../env.trace; git-upload-archive" \
+		--exec="
+			echo GIT_PROTOCOL=\$GIT_PROTOCOL >../env.trace
+			git-upload-archive" \
 		--remote=origin \
 		HEAD >/dev/null &&
-	test_path_is_file env.trace &&
-	test_grep ! ^GIT_PROTOCOL env.trace
+	echo "GIT_PROTOCOL=" >expect &&
+	test_cmp expect env.trace
 '
 
 test_expect_success 'reject client packfile-uris if not advertised' '
-- 
2.56.0.613.g16c2199927

Re: [PATCH] t5702: be more resilient to env quoting

[Todd Zullinger]

Jeff King wrote:
> A few tests in t5702 check whether we specify the v2 protocol via the
> GIT_PROTOCOL environment variable. They do this by redirecting the
> output of "env" into a file and grepping it. But our exact pattern
> depends on how env quotes its output. GNU coreutils 9.10 will output
> this:
> 
>   GIT_PROTOCOL=version=2
> 
> but version 9.12 will output this:
> 
>   GIT_PROTOCOL='version=2'
> 
> It doesn't quote every variable, but the presence of "=" now triggers
> it. We could loosen our regex to accommodate both versions, but there's
> a simpler fix. We only care about the value of one variable, so we can
> just print it using the shell invoked for the server-side command.

Per the coreutils-9.12 NEWS¹, it now honors "the
QUOTING_STYLE environment variable, defaulting to
shell-escape style."

Not that I think we should keep the env parsing; as you note
there are some pros to replacing it anyway.  But if we want
to use env somewhere else, we can set QUOTING_STYLE=literal
to get the unquoted values too.

¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121

  I'd link to the main Savannah cgit or gitweb, but those
  are currently down, thanks to AI scraper bots. :(

> ---
> The new coreutils hit Debian unstable, so I ran into this doing my usual
> daily "make test". I wondered what idiot did the overly-complicated
> "env" invocation in the first place, but naturally it turns out it was
> me.

Haha.  I've had that experience as well.  More often than
I'd like. :)

I didn't properly review or test your changes, though they
do _look_ sane at a glance (albeit a quick Friday night
glance).

-- 
Todd

Re: [PATCH] t5702: be more resilient to env quoting

[Jeff King]

On Fri, Oct 30, 2026 at 10:03:18PM -0400, Todd Zullinger wrote:

> > It doesn't quote every variable, but the presence of "=" now triggers
> > it. We could loosen our regex to accommodate both versions, but there's
> > a simpler fix. We only care about the value of one variable, so we can
> > just print it using the shell invoked for the server-side command.
> 
> Per the coreutils-9.12 NEWS¹, it now honors "the
> QUOTING_STYLE environment variable, defaulting to
> shell-escape style."
> 
> Not that I think we should keep the env parsing; as you note
> there are some pros to replacing it anyway.  But if we want
> to use env somewhere else, we can set QUOTING_STYLE=literal
> to get the unquoted values too.

Heh, I started to wonder if there might be such an option, but then
realized we were probably better without it. But thank you for (as
usual) filling my brain with more Unix esoterica. You never know when it
will later come in handy. :)

> I didn't properly review or test your changes, though they
> do _look_ sane at a glance (albeit a quick Friday night
> glance).

Thanks. The thing I most worried about was messing up the quoting of "$"
in the embedded shell command, since otherwise we expand $GIT_PROTOCOL
in the outer layer. And as it would obviously be empty there, that would
give a false pass to the tests that are expecting it to be empty (but
you can see from the upload-pack one that we do get it right).

This is a case where using an "<<EOT" here-doc might make things easier,
but I stopped short.

-Peff

Re: [PATCH] t5702: be more resilient to env quoting

[Collin Funk]

Todd Zullinger <tmz@pobox.com> writes:

> Jeff King wrote:
>> A few tests in t5702 check whether we specify the v2 protocol via the
>> GIT_PROTOCOL environment variable. They do this by redirecting the
>> output of "env" into a file and grepping it. But our exact pattern
>> depends on how env quotes its output. GNU coreutils 9.10 will output
>> this:
>> 
>>   GIT_PROTOCOL=version=2
>> 
>> but version 9.12 will output this:
>> 
>>   GIT_PROTOCOL='version=2'
>> 
>> It doesn't quote every variable, but the presence of "=" now triggers
>> it. We could loosen our regex to accommodate both versions, but there's
>> a simpler fix. We only care about the value of one variable, so we can
>> just print it using the shell invoked for the server-side command.
>
> Per the coreutils-9.12 NEWS¹, it now honors "the
> QUOTING_STYLE environment variable, defaulting to
> shell-escape style."
>
> Not that I think we should keep the env parsing; as you note
> there are some pros to replacing it anyway.  But if we want
> to use env somewhere else, we can set QUOTING_STYLE=literal
> to get the unquoted values too.
>
> ¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121
>
>   I'd link to the main Savannah cgit or gitweb, but those
>   are currently down, thanks to AI scraper bots. :(

Nice catch on the NEWS entry. Sorry to you both for the extra work. I am
at least partly to blame for those changes.

In coreutils-9.12, we made 'env' and 'printenv' quote their output. This
was to make the output more robust in the case of whitespace, special
characters, and in particular, newlines in environment variables.
Specifically, any program that parses the output of 'env', without -0,
will certainly mistake environment variables with newlines in them as
multiple environment variables. See the following example of the output
before coreutils-9.12:

    $ env -i env --version | head -n 1
    env (GNU coreutils) 9.10
    $ env -i env VAR1=real$'\n'VAR2=fake
    VAR1=real
    VAR2=fake

Compared to the output of 9.12:

    $ env -i ~/.local/bin/env --version | head -n1
    env (GNU coreutils) 9.12.163-9e804
    $ env -i ~/.local/bin/env VAR1=real$'\n'VAR2=fake
    VAR1='real'$'\n''VAR2=fake'

In previous instances, we only quoted when emitting text to a terminal,
e.g., with the quoting performed by 'ls':

    $ touch a$'\n'b
    $ /bin/ls .
    'a'$'\n''b'
    $ /bin/ls . | cat
    a
    b

However, we saw benefit in not doing that in this case. One could use
'env' in a shell script, redirect it to a file, and eval it later, for
example. We thought this would be okay, since we did not find any
instances in Debian code search where this would have caused problems.

Evidently, that search was not thorough enough, and various places ran
into issues [1] [2]. That code, in some cases, does have the problem I
mentioned previously, where it could mistake parts of the values in
environment variables as separate variables. However, we obviously do
not want to break things that have worked forever, and the quoting was
causing more problems than the uncommon issue that it was trying to
prevent.

As a result, we changed the behavior to quote only when standard output
is a terminal. E.g., here is the output from my current build from the
master branch:

    $ env -i src/env VAR1=real$'\n'VAR2=fake
    VAR1='real'$'\n''VAR2=fake'
    $ env -i src/env VAR1=real$'\n'VAR2=fake | cat
    VAR1=real
    VAR2=fake

In all of the cases that I have seen where the 9.12 behavior caused
problems, quoting only when standard output is a terminal would have
avoided all of them.

Pádraig Brady and I reached out to various distributions that tend to
update quickly about that patch [3], in hopes of preventing their users
from needing to debug applications/libraries which they may or may not
even be aware use 'env' in a way that this change breaks. I had
forgotten to submit a Debian bug report, but reported it today to the
Debian maintainers, who have applied it now [4]. I'm too familiar with
Debian packaging, but it appears that 9.12-0.3 should be available with
the patch [5].

Note that some distributions may have also picked up the patch adding
the --quoting-style option [6] (I think Slackware did, not sure if
anyone else did). But that hasn't been in a release yet, so you probably
don't want to use it.

Sorry again for the extra work of having to investigate that behavior
change.

Collin

[1] https://github.com/coreutils/coreutils/issues/355
[2] https://bugs.kde.org/525805
[3] https://github.com/coreutils/coreutils/commit/782a1e5bc2090212273bb731dceee2cc2a071e54
[4] https://bugs.debian.org/1152651
[5] https://packages.debian.org/sid/coreutils

Re: [PATCH] t5702: be more resilient to env quoting

[Collin Funk]

Todd Zullinger <tmz@pobox.com> writes:

> ¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121
>
>   I'd link to the main Savannah cgit or gitweb, but those
>   are currently down, thanks to AI scraper bots. :(

I forgot to mention, but the only people with access to that GitHub are
people with commit access to GNU coreutils and/or gnulib, which contains
source code imported into coreutils and various other GNU projects and
then compiled as a static library.

So I wouldn't worry too much about linking or using it from a security
perspective. Though, of course, it would be better if the scrapers
weren't trying their hardest to kill Savannah and various other websites
that are used by free software projects. :'(

Collin

Re: [PATCH] t5702: be more resilient to env quoting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The new coreutils hit Debian unstable, so I ran into this doing my usual
> daily "make test". I wondered what idiot did the overly-complicated
> "env" invocation in the first place, but naturally it turns out it was
> me.

Namely, in eaa0fd6584 (git_connect(): fix corner cases in
downgrading v2 to v0, 2023-03-17).  The log message starts by
discussing dropping a "push" that asks for v2 to v0 because we can
only do v2 for fetches, but the three tests added by the patch
verify that fetch, push, and archive invoked via a custom path do
not request v2.

I can see that the patch correctly checks the value of '$GIT_PROTOCOL'
in the environment when these custom commands are invoked.  I would
have to read the log message for the "head-scratching notes" in
eaa0fd6584 if I wanted to remind myself why the original code wanted
to downgrade a custom upload-pack to v0 in the first place.  Luckily,
I do not have to do that to be convinced that this particular patch
is correct.

Re: [PATCH] t5702: be more resilient to env quoting

[Jeff King]

On Fri, Oct 30, 2026 at 08:12:06PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The new coreutils hit Debian unstable, so I ran into this doing my usual
> > daily "make test". I wondered what idiot did the overly-complicated
> > "env" invocation in the first place, but naturally it turns out it was
> > me.
> 
> Namely, in eaa0fd6584 (git_connect(): fix corner cases in
> downgrading v2 to v0, 2023-03-17).  The log message starts by
> discussing dropping a "push" that asks for v2 to v0 because we can
> only do v2 for fetches, but the three tests added by the patch
> verify that fetch, push, and archive invoked via a custom path do
> not request v2.

I think you're misreading one of the tests. The one for fetch checks
that we _do_ send version=2 in the $GIT_PROTOCOL field. The others
confirm that we don't (the idea was to cover all of the possible
operations).

> I can see that the patch correctly checks the value of '$GIT_PROTOCOL'
> in the environment when these custom commands are invoked.  I would
> have to read the log message for the "head-scratching notes" in
> eaa0fd6584 if I wanted to remind myself why the original code wanted
> to downgrade a custom upload-pack to v0 in the first place.  Luckily,
> I do not have to do that to be convinced that this particular patch
> is correct.

Yes, I don't think digging into the esoterica of eaa0fd6584 is worth
your time. But knowing that it is confirming upload-pack as v2 might
make it easier (and hopefully it is clear that this new patch retains
that behavior in its "expect").

-Peff