{"thread":{"id":"61685","subject":"[PATCH v2] git-send-email: Use sanitized address when reading mbox body","startedAt":"2024-06-26T13:25:13Z","lastAt":"2024-06-27T15:09:08Z","messageCount":4,"participants":["csókás","Junio C Hamano","Csókás Bence"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"497694","messageId":"20240626132440.3762363-2-csokas.bence@prolan.hu","threadId":"61685","inReplyTo":null,"subject":"[PATCH v2] git-send-email: Use sanitized address when reading mbox body","fromName":"","fromEmail":"csókás","sentAt":"2024-06-26T13:24:41Z","receivedAt":"2024-06-26T13:25:13Z","isPatch":true,"sender":{"key":"csókás","avatar":null},"body":"Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: '\netc. lines mess with git-send-email. In parsing the mbox headers,\nthis is handled by calling `sanitize_address()`. This function\nis called when parsing the message body as well, but was only\nused for comparing it to $author. Now we add it to @cc too.\n\nNote that sanitization is only done for the message body, as\n`git format-patch` already RFC 2047-encodes mbox headers, so\nthose are generally trusted to be sane. Also note that\n`sanitize_address()` does not process the mailbox addresses,\nso it is up to `sendmail` to handle special characters there\n(e.g. there are mailboxes in rwgular use with '+'-es in them).\n\nSigned-off-by: Csókás, Bence <csokas.bence@prolan.hu>\n---\n\nNotes:\n    Changes in v2:\n    * added testcase to t9001\n    * added rationale behind trusting mbox headers and the address-parts\n\n git-send-email.perl   |  4 ++--\n t/t9001-send-email.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex f0be4b4560..72044e5ef3 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1847,9 +1847,9 @@ sub pre_process_file {\n \t\t\t\t\t$what, $_) unless $quiet;\n \t\t\t\tnext;\n \t\t\t}\n-\t\t\tpush @cc, $c;\n+\t\t\tpush @cc, $sc;\n \t\t\tprintf(__(\"(body) Adding cc: %s from line '%s'\\n\"),\n-\t\t\t\t$c, $_) unless $quiet;\n+\t\t\t\t$sc, $_) unless $quiet;\n \t\t}\n \t}\n \tclose $fh;\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 58699f8e4e..7e0b8ae57c 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1299,6 +1299,36 @@ test_expect_success $PREREQ 'utf8 sender is not duplicated' '\n \ttest_line_count = 1 msgfrom\n '\n \n+test_expect_success $PREREQ 'setup expect for cc list' \"\n+cat >expected-cc <<\\EOF\n+!recipient@example.com!\n+!author@example.com!\n+!one@example.com!\n+!odd_?=mail@example.com!\n+!thor.au@example.com!\n+EOF\n+\"\n+\n+test_expect_success $PREREQ 'cc list is sanitized' '\n+\tclean_fake_sendmail &&\n+\ttest_commit weird_cc_body &&\n+\ttest_when_finished \"git reset --hard HEAD^\" &&\n+\tgit commit --amend -F - <<-EOF &&\n+\tTest Cc: sanitization.\n+\n+\tCc: Person, One <one@example.com>\n+\tReviewed-by: Füñný Nâmé <odd_?=mail@example.com>\n+\tSigned-off-by: A. U. Thor <thor.au@example.com>\n+\tEOF\n+\tgit send-email -1 --to=recipient@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" >actual-show-all-headers &&\n+\ttest_cmp expected-cc commandline1 &&\n+\tgrep \"^(body) Adding cc: \\\"Person, One\\\" <one@example.com>\" actual-show-all-headers &&\n+\tgrep \"^(body) Adding cc: =?UTF-8?q?F=C3=BC=C3=B1n=C3=BD=20N=C3=A2m=C3=A9?=\"\\\n+\" <odd_?=mail@example.com>\" actual-show-all-headers &&\n+\tgrep \"^(body) Adding cc: \\\"A. U. Thor\\\" <thor.au@example.com>\" actual-show-all-headers\n+'\n+\n test_expect_success $PREREQ 'sendemail.composeencoding works' '\n \tclean_fake_sendmail &&\n \tgit config sendemail.composeencoding iso-8859-1 &&\n-- \n2.34.1\n\n\n"},{"id":"497709","messageId":"xmqqy16r1ulu.fsf@gitster.g","threadId":"61685","inReplyTo":"20240626132440.3762363-2-csokas.bence@prolan.hu","subject":"Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T17:28:45Z","receivedAt":"2024-06-26T17:28:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Csókás, Bence\" <csokas.bence@prolan.hu> writes:\n\n> Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: '\n> etc. lines mess with git-send-email. In parsing the mbox headers,\n> this is handled by calling `sanitize_address()`. This function\n> is called when parsing the message body as well, but was only\n> used for comparing it to $author. Now we add it to @cc too.\n\nThe above is misleading, though.  We do use sanitize_address on\naddresses we find on e-mail headers, but the result is not used\nin @to or @cc, the addresses we use to actually send the message\nout.\n\nPerhaps phrase it more like ...\n\n    When we check addresses found on the mbox headers to see if we\n    want to add them to Cc:, we use sanitize_address() function to\n    normalize the addresses before passing them to the suppression\n    mechanism, but we use the original addresses for the purpose of\n    sending the message out.\n\n    We use the same logic on the address-looking strings found on\n    trailer lines that appear in the message body.  Sanitized\n    addresses are used for Cc-suppression purposes, but the original\n    addresses as written by the end-user are used as the mail\n    destination.\n\n    There are certain quoting rules for e-mail addresses, and unlike\n    addresses on e-mail headers that are generated by format-patch,\n    hand-written addresses on the trailer lines are more likely to\n    violate them by mistake.\n\n    When adding the address found on a trailer line in the message\n    body, use the sanitized address for both sending the message\n    out, as well as checking with Cc-suppression mechanism, to\n    reduce the risk of malformed hand-written addresses to get the\n    message rejected (but keep using the original addresses found on\n    the e-mail headers in the input message for e-mail destination).\n\n... or something like that?\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 58699f8e4e..7e0b8ae57c 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n\nThis test uses mixture of test_grep and grep, so use of \"grep\" below\nis fine (but in the longer term we should make sure the tests use\nthe former for better debuggability).\n\nAs I always say, we should resist the temptation to write our tests\njust to demonstrate our shiny new toy.  In this case, the test is\ntoo focused to show that the system will give a best-effort output\nwhen fed invalid and/or malformed addresses, but it does not see\nwhat happens to a well formed addresses (ideally they are passed\nintact, but is that what happens with the new code?).  Perhaps add\none or two trailer lines with valid addresses (and non-address, like\n\"BugId: 143421\", that should not appear at all in the output) on\nthem?\n\nThanks.\n\n> +test_expect_success $PREREQ 'cc list is sanitized' '\n> +\tclean_fake_sendmail &&\n> +\ttest_commit weird_cc_body &&\n> +\ttest_when_finished \"git reset --hard HEAD^\" &&\n> +\tgit commit --amend -F - <<-EOF &&\n> +\tTest Cc: sanitization.\n> +\n> +\tCc: Person, One <one@example.com>\n> +\tReviewed-by: Füñný Nâmé <odd_?=mail@example.com>\n> +\tSigned-off-by: A. U. Thor <thor.au@example.com>\n> +\tEOF\n> +\tgit send-email -1 --to=recipient@example.com \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" >actual-show-all-headers &&\n> +\ttest_cmp expected-cc commandline1 &&\n> +\tgrep \"^(body) Adding cc: \\\"Person, One\\\" <one@example.com>\" actual-show-all-headers &&\n> +\tgrep \"^(body) Adding cc: =?UTF-8?q?F=C3=BC=C3=B1n=C3=BD=20N=C3=A2m=C3=A9?=\"\\\n> +\" <odd_?=mail@example.com>\" actual-show-all-headers &&\n> +\tgrep \"^(body) Adding cc: \\\"A. U. Thor\\\" <thor.au@example.com>\" actual-show-all-headers\n> +'\n\n>  test_expect_success $PREREQ 'sendemail.composeencoding works' '\n>  \tclean_fake_sendmail &&\n>  \tgit config sendemail.composeencoding iso-8859-1 &&\n"},{"id":"497742","messageId":"f245c5e8-b986-49f1-91fe-7e24593e8e85@prolan.hu","threadId":"61685","inReplyTo":"xmqqy16r1ulu.fsf@gitster.g","subject":"Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body","fromName":"Csókás Bence","fromEmail":"csokas.bence@prolan.hu","sentAt":"2024-06-27T08:37:06Z","receivedAt":"2024-06-27T08:43:43Z","isPatch":true,"sender":{"key":"csokas.bence@prolan.hu","avatar":null},"body":"On 6/26/24 19:28, Junio C Hamano wrote:\n>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n>> index 58699f8e4e..7e0b8ae57c 100755\n>> --- a/t/t9001-send-email.sh\n>> +++ b/t/t9001-send-email.sh\n> \n> This test uses mixture of test_grep and grep, so use of \"grep\" below\n> is fine (but in the longer term we should make sure the tests use\n> the former for better debuggability).\n\nOk, I'll use test_grep everywhere.\n\n> As I always say, we should resist the temptation to write our tests\n> just to demonstrate our shiny new toy.  In this case, the test is\n> too focused to show that the system will give a best-effort output\n> when fed invalid and/or malformed addresses, but it does not see\n> what happens to a well formed addresses (ideally they are passed\n> intact, but is that what happens with the new code?).  Perhaps add\n> one or two trailer lines with valid addresses (and non-address, like\n> \"BugId: 143421\", that should not appear at all in the output) on\n> them?\n\nValid addresses are already tested by former tests. I don't immediately \nsee any tests that would cover non-Cc, non-*-by and non-email-address \ntags, so I might add them; should they be a separate testcase or part of \nthis one? Or maybe even a separate patch?\n\nBence\n\n"},{"id":"497745","messageId":"xmqq34oywhgx.fsf@gitster.g","threadId":"61685","inReplyTo":"f245c5e8-b986-49f1-91fe-7e24593e8e85@prolan.hu","subject":"Re: [PATCH v2] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-27T15:09:02Z","receivedAt":"2024-06-27T15:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Csókás Bence <csokas.bence@prolan.hu> writes:\n\n> Valid addresses are already tested by former tests. I don't\n> immediately see any tests that would cover non-Cc, non-*-by and\n> non-email-address tags, so I might add them; should they be a separate\n> testcase or part of this one? Or maybe even a separate patch?\n\nThe tests you added has only malformed addresses on trailers, and\nadding a few trailers with valid addresses *and* string that does\nnot look like an address at all, would be a minimal fix that makes\nthe test complete.\n\nIt makes it clear that the point of the new test is about strings\nfound on trailers (that are dealt differently from addresses taken\nfrom other places) are handled appropriately, checked how they are\nshown to the MTA (with the comparison !address!  output from\nfake.sendmail script) and how their additions are logged in the\nverbose output (with the pattern match with \"^(body) Adding cc:\n...\").  Checking that totally irrelevant ones (e.g. bugId) are not\neven used as addresses, strings meant as addresses but malformed are\nused after correction, and well-formed addresses are used as-is,\nwould form a complete set of observable behaviour from this feature.\n\nThanks.\n\n"}]}