{"thread":{"id":"66720","subject":"[PATCH] t5702: be more resilient to env quoting","startedAt":"2026-10-31T00:25:50Z","lastAt":"2026-11-04T06:32:01Z","messageCount":9,"participants":["Jeff King","Todd Zullinger","Junio C Hamano","Collin Funk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"556984","messageId":"20261031002550.GA626623@coredump.intra.peff.net","threadId":"66720","inReplyTo":null,"subject":"[PATCH] t5702: be more resilient to env quoting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-31T00:25:50Z","receivedAt":"2026-10-31T00:25:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A few tests in t5702 check whether we specify the v2 protocol via the\nGIT_PROTOCOL environment variable. They do this by redirecting the\noutput of \"env\" into a file and grepping it. But our exact pattern\ndepends on how env quotes its output. GNU coreutils 9.10 will output\nthis:\n\n GIT_PROTOCOL=version=2\n\nbut version 9.12 will output this:\n\n GIT_PROTOCOL='version=2'\n\nIt doesn't quote every variable, but the presence of \"=\" now triggers\nit. We could loosen our regex to accommodate both versions, but there's\na simpler fix. We only care about the value of one variable, so we can\njust print it using the shell invoked for the server-side command.\n\nNote that this new formula is slightly less strict: it allows\nGIT_PROTOCOL as an empty string in addition to it being missing. But\nthat's OK; they both mean the same thing to the child Git process.\n\nLikewise, it's actually more strict when we do expect the v2 protocol:\nit checks the contents of the whole GIT_PROTOCOL rather than a partial\ngrep. In practice it's the same for now (we only ever set protocol=2),\nbut I think in the long run being strict is better (if we do add\nsomething else, the test will break and we can consider how to handle it\nthen).\n\nSigned-off-by: Jeff King \n---\nThe new coreutils hit Debian unstable, so I ran into this doing my usual\ndaily \"make test\". I wondered what idiot did the overly-complicated\n\"env\" invocation in the first place, but naturally it turns out it was\nme.\n\n t/t5702-protocol-v2.sh | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 7e69ba7f84..a66adfbc3b 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -929,28 +929,35 @@ test_expect_success 'file:// --negotiate-only with protocol v0' '\n test_expect_success 'push with custom path does not request v2' '\n \trm -f env.trace &&\n \tgit -C client push \\\n-\t\t--receive-pack=\"env >../env.trace; git-receive-pack\" \\\n+\t\t--receive-pack=\"\n+\t\t\techo GIT_PROTOCOL=\\$GIT_PROTOCOL >../env.trace\n+\t\t\tgit-receive-pack\" \\\n \t\torigin HEAD:refs/heads/custom-push-test &&\n-\ttest_path_is_file env.trace &&\n-\ttest_grep ! ^GIT_PROTOCOL env.trace\n+\techo GIT_PROTOCOL= >expect &&\n+\ttest_cmp expect env.trace\n '\n \n test_expect_success 'fetch with custom path does request v2' '\n \trm -f env.trace &&\n \tgit -C client fetch \\\n-\t\t--upload-pack=\"env >../env.trace; git-upload-pack\" \\\n+\t\t--upload-pack=\"\n+\t\t\techo GIT_PROTOCOL=\\$GIT_PROTOCOL >../env.trace\n+\t\t\tgit-upload-pack\" \\\n \t\torigin HEAD &&\n-\ttest_grep ^GIT_PROTOCOL=version=2 env.trace\n+\techo GIT_PROTOCOL=version=2 >expect &&\n+\ttest_cmp expect env.trace\n '\n \n test_expect_success 'archive with custom path does not request v2' '\n \trm -f env.trace &&\n \tgit -C client archive \\\n-\t\t--exec=\"env >../env.trace; git-upload-archive\" \\\n+\t\t--exec=\"\n+\t\t\techo GIT_PROTOCOL=\\$GIT_PROTOCOL >../env.trace\n+\t\t\tgit-upload-archive\" \\\n \t\t--remote=origin \\\n \t\tHEAD >/dev/null &&\n-\ttest_path_is_file env.trace &&\n-\ttest_grep ! ^GIT_PROTOCOL env.trace\n+\techo \"GIT_PROTOCOL=\" >expect &&\n+\ttest_cmp expect env.trace\n '\n \n test_expect_success 'reject client packfile-uris if not advertised' '\n-- \n2.56.0.613.g16c2199927\n\n"},{"id":"556985","messageId":"20261031020318.tLesiElF@teonanacatl.net","threadId":"66720","inReplyTo":"20261031002550.GA626623@coredump.intra.peff.net","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2026-10-31T02:03:18Z","receivedAt":"2026-10-31T02:03:18Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> A few tests in t5702 check whether we specify the v2 protocol via the\n> GIT_PROTOCOL environment variable. They do this by redirecting the\n> output of \"env\" into a file and grepping it. But our exact pattern\n> depends on how env quotes its output. GNU coreutils 9.10 will output\n> this:\n> \n> GIT_PROTOCOL=version=2\n> \n> but version 9.12 will output this:\n> \n> GIT_PROTOCOL='version=2'\n> \n> It doesn't quote every variable, but the presence of \"=\" now triggers\n> it. We could loosen our regex to accommodate both versions, but there's\n> a simpler fix. We only care about the value of one variable, so we can\n> just print it using the shell invoked for the server-side command.\n\nPer the coreutils-9.12 NEWS�, it now honors \"the\nQUOTING_STYLE environment variable, defaulting to\nshell-escape style.\"\n\nNot that I think we should keep the env parsing; as you note\nthere are some pros to replacing it anyway. But if we want\nto use env somewhere else, we can set QUOTING_STYLE=literal\nto get the unquoted values too.\n\n� https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121\n\n I'd link to the main Savannah cgit or gitweb, but those\n are currently down, thanks to AI scraper bots. :(\n\n> ---\n> The new coreutils hit Debian unstable, so I ran into this doing my usual\n> daily \"make test\". I wondered what idiot did the overly-complicated\n> \"env\" invocation in the first place, but naturally it turns out it was\n> me.\n\nHaha. I've had that experience as well. More often than\nI'd like. :)\n\nI didn't properly review or test your changes, though they\ndo _look_ sane at a glance (albeit a quick Friday night\nglance).\n\n-- \nTodd\n\n"},{"id":"556987","messageId":"xmqqwlqytvg9.fsf@gitster.g","threadId":"66720","inReplyTo":"20261031002550.GA626623@coredump.intra.peff.net","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-31T03:12:06Z","receivedAt":"2026-10-31T03:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The new coreutils hit Debian unstable, so I ran into this doing my usual\n> daily \"make test\". I wondered what idiot did the overly-complicated\n> \"env\" invocation in the first place, but naturally it turns out it was\n> me.\n\nNamely, in eaa0fd6584 (git_connect(): fix corner cases in\ndowngrading v2 to v0, 2023-03-17). The log message starts by\ndiscussing dropping a \"push\" that asks for v2 to v0 because we can\nonly do v2 for fetches, but the three tests added by the patch\nverify that fetch, push, and archive invoked via a custom path do\nnot request v2.\n\nI can see that the patch correctly checks the value of '$GIT_PROTOCOL'\nin the environment when these custom commands are invoked. I would\nhave to read the log message for the \"head-scratching notes\" in\neaa0fd6584 if I wanted to remind myself why the original code wanted\nto downgrade a custom upload-pack to v0 in the first place. Luckily,\nI do not have to do that to be convinced that this particular patch\nis correct.\n\n"},{"id":"556988","messageId":"20261031050337.GA1202343@coredump.intra.peff.net","threadId":"66720","inReplyTo":"xmqqwlqytvg9.fsf@gitster.g","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-31T05:03:37Z","receivedAt":"2026-10-31T05:03:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2026 at 08:12:06PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The new coreutils hit Debian unstable, so I ran into this doing my usual\n> > daily \"make test\". I wondered what idiot did the overly-complicated\n> > \"env\" invocation in the first place, but naturally it turns out it was\n> > me.\n> \n> Namely, in eaa0fd6584 (git_connect(): fix corner cases in\n> downgrading v2 to v0, 2023-03-17). The log message starts by\n> discussing dropping a \"push\" that asks for v2 to v0 because we can\n> only do v2 for fetches, but the three tests added by the patch\n> verify that fetch, push, and archive invoked via a custom path do\n> not request v2.\n\nI think you're misreading one of the tests. The one for fetch checks\nthat we _do_ send version=2 in the $GIT_PROTOCOL field. The others\nconfirm that we don't (the idea was to cover all of the possible\noperations).\n\n> I can see that the patch correctly checks the value of '$GIT_PROTOCOL'\n> in the environment when these custom commands are invoked. I would\n> have to read the log message for the \"head-scratching notes\" in\n> eaa0fd6584 if I wanted to remind myself why the original code wanted\n> to downgrade a custom upload-pack to v0 in the first place. Luckily,\n> I do not have to do that to be convinced that this particular patch\n> is correct.\n\nYes, I don't think digging into the esoterica of eaa0fd6584 is worth\nyour time. But knowing that it is confirming upload-pack as v2 might\nmake it easier (and hopefully it is clear that this new patch retains\nthat behavior in its \"expect\").\n\n-Peff\n\n"},{"id":"556989","messageId":"20261031050638.GB1202343@coredump.intra.peff.net","threadId":"66720","inReplyTo":"20261031020318.tLesiElF@teonanacatl.net","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-31T05:06:38Z","receivedAt":"2026-10-31T05:06:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2026 at 10:03:18PM -0400, Todd Zullinger wrote:\n\n> > It doesn't quote every variable, but the presence of \"=\" now triggers\n> > it. We could loosen our regex to accommodate both versions, but there's\n> > a simpler fix. We only care about the value of one variable, so we can\n> > just print it using the shell invoked for the server-side command.\n> \n> Per the coreutils-9.12 NEWS¹, it now honors \"the\n> QUOTING_STYLE environment variable, defaulting to\n> shell-escape style.\"\n> \n> Not that I think we should keep the env parsing; as you note\n> there are some pros to replacing it anyway. But if we want\n> to use env somewhere else, we can set QUOTING_STYLE=literal\n> to get the unquoted values too.\n\nHeh, I started to wonder if there might be such an option, but then\nrealized we were probably better without it. But thank you for (as\nusual) filling my brain with more Unix esoterica. You never know when it\nwill later come in handy. :)\n\n> I didn't properly review or test your changes, though they\n> do _look_ sane at a glance (albeit a quick Friday night\n> glance).\n\nThanks. The thing I most worried about was messing up the quoting of \"$\"\nin the embedded shell command, since otherwise we expand $GIT_PROTOCOL\nin the outer layer. And as it would obviously be empty there, that would\ngive a false pass to the tests that are expecting it to be empty (but\nyou can see from the upload-pack one that we do get it right).\n\nThis is a case where using an \"< writes:\n\n> Jeff King wrote:\n>> A few tests in t5702 check whether we specify the v2 protocol via the\n>> GIT_PROTOCOL environment variable. They do this by redirecting the\n>> output of \"env\" into a file and grepping it. But our exact pattern\n>> depends on how env quotes its output. GNU coreutils 9.10 will output\n>> this:\n>> \n>> GIT_PROTOCOL=version=2\n>> \n>> but version 9.12 will output this:\n>> \n>> GIT_PROTOCOL='version=2'\n>> \n>> It doesn't quote every variable, but the presence of \"=\" now triggers\n>> it. We could loosen our regex to accommodate both versions, but there's\n>> a simpler fix. We only care about the value of one variable, so we can\n>> just print it using the shell invoked for the server-side command.\n>\n> Per the coreutils-9.12 NEWS¹, it now honors \"the\n> QUOTING_STYLE environment variable, defaulting to\n> shell-escape style.\"\n>\n> Not that I think we should keep the env parsing; as you note\n> there are some pros to replacing it anyway. But if we want\n> to use env somewhere else, we can set QUOTING_STYLE=literal\n> to get the unquoted values too.\n>\n> ¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121\n>\n> I'd link to the main Savannah cgit or gitweb, but those\n> are currently down, thanks to AI scraper bots. :(\n\nNice catch on the NEWS entry. Sorry to you both for the extra work. I am\nat least partly to blame for those changes.\n\nIn coreutils-9.12, we made 'env' and 'printenv' quote their output. This\nwas to make the output more robust in the case of whitespace, special\ncharacters, and in particular, newlines in environment variables.\nSpecifically, any program that parses the output of 'env', without -0,\nwill certainly mistake environment variables with newlines in them as\nmultiple environment variables. See the following example of the output\nbefore coreutils-9.12:\n\n $ env -i env --version | head -n 1\n env (GNU coreutils) 9.10\n $ env -i env VAR1=real$'\\n'VAR2=fake\n VAR1=real\n VAR2=fake\n\nCompared to the output of 9.12:\n\n $ env -i ~/.local/bin/env --version | head -n1\n env (GNU coreutils) 9.12.163-9e804\n $ env -i ~/.local/bin/env VAR1=real$'\\n'VAR2=fake\n VAR1='real'$'\\n''VAR2=fake'\n\nIn previous instances, we only quoted when emitting text to a terminal,\ne.g., with the quoting performed by 'ls':\n\n $ touch a$'\\n'b\n $ /bin/ls .\n 'a'$'\\n''b'\n $ /bin/ls . | cat\n a\n b\n\nHowever, we saw benefit in not doing that in this case. One could use\n'env' in a shell script, redirect it to a file, and eval it later, for\nexample. We thought this would be okay, since we did not find any\ninstances in Debian code search where this would have caused problems.\n\nEvidently, that search was not thorough enough, and various places ran\ninto issues [1] [2]. That code, in some cases, does have the problem I\nmentioned previously, where it could mistake parts of the values in\nenvironment variables as separate variables. However, we obviously do\nnot want to break things that have worked forever, and the quoting was\ncausing more problems than the uncommon issue that it was trying to\nprevent.\n\nAs a result, we changed the behavior to quote only when standard output\nis a terminal. E.g., here is the output from my current build from the\nmaster branch:\n\n $ env -i src/env VAR1=real$'\\n'VAR2=fake\n VAR1='real'$'\\n''VAR2=fake'\n $ env -i src/env VAR1=real$'\\n'VAR2=fake | cat\n VAR1=real\n VAR2=fake\n\nIn all of the cases that I have seen where the 9.12 behavior caused\nproblems, quoting only when standard output is a terminal would have\navoided all of them.\n\nPádraig Brady and I reached out to various distributions that tend to\nupdate quickly about that patch [3], in hopes of preventing their users\nfrom needing to debug applications/libraries which they may or may not\neven be aware use 'env' in a way that this change breaks. I had\nforgotten to submit a Debian bug report, but reported it today to the\nDebian maintainers, who have applied it now [4]. I'm too familiar with\nDebian packaging, but it appears that 9.12-0.3 should be available with\nthe patch [5].\n\nNote that some distributions may have also picked up the patch adding\nthe --quoting-style option [6] (I think Slackware did, not sure if\nanyone else did). But that hasn't been in a release yet, so you probably\ndon't want to use it.\n\nSorry again for the extra work of having to investigate that behavior\nchange.\n\nCollin\n\n[1] https://github.com/coreutils/coreutils/issues/355\n[2] https://bugs.kde.org/525805\n[3] https://github.com/coreutils/coreutils/commit/782a1e5bc2090212273bb731dceee2cc2a071e54\n[4] https://bugs.debian.org/1152651\n[5] https://packages.debian.org/sid/coreutils\n\n"},{"id":"556991","messageId":"87fqxm5tsp.fsf@gmail.com","threadId":"66720","inReplyTo":"20261031020318.tLesiElF@teonanacatl.net","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2026-10-31T05:21:42Z","receivedAt":"2026-10-31T05:21:42Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Todd Zullinger writes:\n\n> ¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121\n>\n> I'd link to the main Savannah cgit or gitweb, but those\n> are currently down, thanks to AI scraper bots. :(\n\nI forgot to mention, but the only people with access to that GitHub are\npeople with commit access to GNU coreutils and/or gnulib, which contains\nsource code imported into coreutils and various other GNU projects and\nthen compiled as a static library.\n\nSo I wouldn't worry too much about linking or using it from a security\nperspective. Though, of course, it would be better if the scrapers\nweren't trying their hardest to kill Savannah and various other websites\nthat are used by free software projects. :'(\n\nCollin\n\n"},{"id":"557091","messageId":"xmqqpkwoz2v6.fsf@gitster.g","threadId":"66720","inReplyTo":"20261031050337.GA1202343@coredump.intra.peff.net","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-11-01T20:57:33Z","receivedAt":"2026-11-01T20:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think you're misreading one of the tests. The one for fetch checks\n> that we _do_ send version=2 in the $GIT_PROTOCOL field. The others\n> confirm that we don't (the idea was to cover all of the possible\n> operations).\n\nYou're right.\n\nQueued.\n\nThanks.\n\n"},{"id":"557404","messageId":"20261104063201.GA1543013@coredump.intra.peff.net","threadId":"66720","inReplyTo":"87jymy5uek.fsf@gmail.com","subject":"Re: [PATCH] t5702: be more resilient to env quoting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-11-04T06:32:01Z","receivedAt":"2026-11-04T06:32:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2026 at 10:08:35PM -0700, Collin Funk wrote:\n\n> Nice catch on the NEWS entry. Sorry to you both for the extra work. I am\n> at least partly to blame for those changes.\n> \n> In coreutils-9.12, we made 'env' and 'printenv' quote their output. This\n> was to make the output more robust in the case of whitespace, special\n> characters, and in particular, newlines in environment variables.\n> Specifically, any program that parses the output of 'env', without -0,\n> will certainly mistake environment variables with newlines in them as\n> multiple environment variables. See the following example of the output\n> before coreutils-9.12:\n\nThough it created some extra work here, I think this is a good change.\nThere's another spot where we use \"env\" in the test suite that I was\nlooking at a month or two ago, and I was mildly terrified that it would\nallow variable injection (though probably not a big deal in practice for\nour purposes). So I'm happy to see the situation improving.\n\n> In all of the cases that I have seen where the 9.12 behavior caused\n> problems, quoting only when standard output is a terminal would have\n> avoided all of them.\n\nYeah, but then we lose the injection protection. :)\n\nWhat a complicated world we live in.\n\n> Note that some distributions may have also picked up the patch adding\n> the --quoting-style option [6] (I think Slackware did, not sure if\n> anyone else did). But that hasn't been in a release yet, so you probably\n> don't want to use it.\n\nYeah, I don't have it here yet. I do agree it's good to have, as it lets\nthe caller be careful when they know they're going to eval the result.\nThough of course it creates its own headaches, because we might not be\nusing coreutils env. :-/\n\n> Sorry again for the extra work of having to investigate that behavior\n> change.\n\nNo worries, and thanks for the explanation. I think the revised code in\ngit.git is better for not using \"env\" anyway.\n\n-Peff\n\n"}]}