threads / patch / 61685

v2git-send-email: Use sanitized address when reading mbox body

Subject: [PATCH v2] git-send-email: Use sanitized address when reading mbox body

## tl;dr

4 messages between Jun 26, 2024 and Jun 27, 2024. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

csókás· Jun 26, 2024, 13:24 UTC · lore

Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: ' etc. lines mess with git-send-email. In parsing the mbox headers, this is handled by calling `sanitize_address()`. This function is called when parsing the message body as well, but was only used for comparing it to $author. Now we add it to @cc too.

Note that sanitization is only done for the message body, as `git format-patch` already RFC 2047-encodes mbox headers, so those are generally trusted to be sane. Also note that `sanitize_address()` does not process the mailbox addresses, so it is up to `sendmail` to handle special characters there (e.g. there are mailboxes in rwgular use with '+'-es in them).

Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---
Notes:
    Changes in v2:
    * added testcase to t9001
    * added rationale behind trusting mbox headers and the address-parts
 git-send-email.perl   |  4 ++--
 t/t9001-send-email.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)
Show changes to 2 files +32 −2

git-send-email.perl, t/t9001-send-email.sh

diff --git a/git-send-email.perl b/git-send-email.perl
index f0be4b4560..72044e5ef3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1847,9 +1847,9 @@ sub pre_process_file {
 					$what, $_) unless $quiet;
 				next;
 			}
-			push @cc, $c;
+			push @cc, $sc;
 			printf(__("(body) Adding cc: %s from line '%s'\n"),
-				$c, $_) unless $quiet;
+				$sc, $_) unless $quiet;
 		}
 	}
 	close $fh;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 58699f8e4e..7e0b8ae57c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1299,6 +1299,36 @@ test_expect_success $PREREQ 'utf8 sender is not duplicated' '
 	test_line_count = 1 msgfrom
 '
 
+test_expect_success $PREREQ 'setup expect for cc list' "
+cat >expected-cc <<\EOF
+!recipient@example.com!
+!author@example.com!
+!one@example.com!
+!odd_?=mail@example.com!
+!thor.au@example.com!
+EOF
+"
+
+test_expect_success $PREREQ 'cc list is sanitized' '
+	clean_fake_sendmail &&
+	test_commit weird_cc_body &&
+	test_when_finished "git reset --hard HEAD^" &&
+	git commit --amend -F - <<-EOF &&
+	Test Cc: sanitization.
+
+	Cc: Person, One <one@example.com>
+	Reviewed-by: Füñný Nâmé <odd_?=mail@example.com>
+	Signed-off-by: A. U. Thor <thor.au@example.com>
+	EOF
+	git send-email -1 --to=recipient@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" >actual-show-all-headers &&
+	test_cmp expected-cc commandline1 &&
+	grep "^(body) Adding cc: \"Person, One\" <one@example.com>" actual-show-all-headers &&
+	grep "^(body) Adding cc: =?UTF-8?q?F=C3=BC=C3=B1n=C3=BD=20N=C3=A2m=C3=A9?="\
+" <odd_?=mail@example.com>" actual-show-all-headers &&
+	grep "^(body) Adding cc: \"A. U. Thor\" <thor.au@example.com>" actual-show-all-headers
+'
+
 test_expect_success $PREREQ 'sendemail.composeencoding works' '
 	clean_fake_sendmail &&
 	git config sendemail.composeencoding iso-8859-1 &&

-- 
2.34.1
Junio C Hamano· Jun 26, 2024, 17:28 UTC · re: csókás · lore

Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body

"Csókás, Bence" <csokas.bence@prolan.hu> writes:
Show 5 quoted lines
> Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: '
> etc. lines mess with git-send-email. In parsing the mbox headers,
> this is handled by calling `sanitize_address()`. This function
> is called when parsing the message body as well, but was only
> used for comparing it to $author. Now we add it to @cc too.

The above is misleading, though. We do use sanitize_address on addresses we find on e-mail headers, but the result is not used in @to or @cc, the addresses we use to actually send the message out.

Perhaps phrase it more like ...
    When we check addresses found on the mbox headers to see if we
    want to add them to Cc:, we use sanitize_address() function to
    normalize the addresses before passing them to the suppression
    mechanism, but we use the original addresses for the purpose of
    sending the message out.
    We use the same logic on the address-looking strings found on
    trailer lines that appear in the message body.  Sanitized
    addresses are used for Cc-suppression purposes, but the original
    addresses as written by the end-user are used as the mail
    destination.
    There are certain quoting rules for e-mail addresses, and unlike
    addresses on e-mail headers that are generated by format-patch,
    hand-written addresses on the trailer lines are more likely to
    violate them by mistake.
    When adding the address found on a trailer line in the message
    body, use the sanitized address for both sending the message
    out, as well as checking with Cc-suppression mechanism, to
    reduce the risk of malformed hand-written addresses to get the
    message rejected (but keep using the original addresses found on
    the e-mail headers in the input message for e-mail destination).
... or something like that?
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 58699f8e4e..7e0b8ae57c 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh

This test uses mixture of test_grep and grep, so use of "grep" below is fine (but in the longer term we should make sure the tests use the former for better debuggability).

As I always say, we should resist the temptation to write our tests just to demonstrate our shiny new toy. In this case, the test is too focused to show that the system will give a best-effort output when fed invalid and/or malformed addresses, but it does not see what happens to a well formed addresses (ideally they are passed intact, but is that what happens with the new code?). Perhaps add one or two trailer lines with valid addresses (and non-address, like "BugId: 143421", that should not appear at all in the output) on them?

Thanks.
Show 19 quoted lines
> +test_expect_success $PREREQ 'cc list is sanitized' '
> +	clean_fake_sendmail &&
> +	test_commit weird_cc_body &&
> +	test_when_finished "git reset --hard HEAD^" &&
> +	git commit --amend -F - <<-EOF &&
> +	Test Cc: sanitization.
> +
> +	Cc: Person, One <one@example.com>
> +	Reviewed-by: Füñný Nâmé <odd_?=mail@example.com>
> +	Signed-off-by: A. U. Thor <thor.au@example.com>
> +	EOF
> +	git send-email -1 --to=recipient@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" >actual-show-all-headers &&
> +	test_cmp expected-cc commandline1 &&
> +	grep "^(body) Adding cc: \"Person, One\" <one@example.com>" actual-show-all-headers &&
> +	grep "^(body) Adding cc: =?UTF-8?q?F=C3=BC=C3=B1n=C3=BD=20N=C3=A2m=C3=A9?="\
> +" <odd_?=mail@example.com>" actual-show-all-headers &&
> +	grep "^(body) Adding cc: \"A. U. Thor\" <thor.au@example.com>" actual-show-all-headers
> +'
>  test_expect_success $PREREQ 'sendemail.composeencoding works' '
>  	clean_fake_sendmail &&
>  	git config sendemail.composeencoding iso-8859-1 &&
Csókás Bence· Jun 27, 2024, 08:37 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body

On 6/26/24 19:28, Junio C Hamano wrote:
Show 8 quoted lines
>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
>> index 58699f8e4e..7e0b8ae57c 100755
>> --- a/t/t9001-send-email.sh
>> +++ b/t/t9001-send-email.sh
> 
> This test uses mixture of test_grep and grep, so use of "grep" below
> is fine (but in the longer term we should make sure the tests use
> the former for better debuggability).
Ok, I'll use test_grep everywhere.
Show 9 quoted lines
> As I always say, we should resist the temptation to write our tests
> just to demonstrate our shiny new toy.  In this case, the test is
> too focused to show that the system will give a best-effort output
> when fed invalid and/or malformed addresses, but it does not see
> what happens to a well formed addresses (ideally they are passed
> intact, but is that what happens with the new code?).  Perhaps add
> one or two trailer lines with valid addresses (and non-address, like
> "BugId: 143421", that should not appear at all in the output) on
> them?

Valid addresses are already tested by former tests. I don't immediately see any tests that would cover non-Cc, non-*-by and non-email-address tags, so I might add them; should they be a separate testcase or part of this one? Or maybe even a separate patch?

Bence
Junio C Hamano· Jun 27, 2024, 15:09 UTC · re: Csókás Bence · lore

Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body

Csókás Bence <csokas.bence@prolan.hu> writes:
> Valid addresses are already tested by former tests. I don't
> immediately see any tests that would cover non-Cc, non-*-by and
> non-email-address tags, so I might add them; should they be a separate
> testcase or part of this one? Or maybe even a separate patch?

The tests you added has only malformed addresses on trailers, and adding a few trailers with valid addresses *and* string that does not look like an address at all, would be a minimal fix that makes the test complete.

It makes it clear that the point of the new test is about strings found on trailers (that are dealt differently from addresses taken from other places) are handled appropriately, checked how they are shown to the MTA (with the comparison !address! output from fake.sendmail script) and how their additions are logged in the verbose output (with the pattern match with "^(body) Adding cc: ..."). Checking that totally irrelevant ones (e.g. bugId) are not even used as addresses, strings meant as addresses but malformed are used after correction, and well-formed addresses are used as-is, would form a complete set of observable behaviour from this feature.

Thanks.

← back to recent threads