{"thread":{"id":"61709","subject":"[PATCH v4] git-send-email: Use sanitized address when reading mbox body","startedAt":"2024-07-01T09:03:29Z","lastAt":"2024-07-03T15:44:39Z","messageCount":4,"participants":["csókás","Junio C Hamano","Csókás Bence"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"497878","messageId":"20240701090115.56957-1-csokas.bence@prolan.hu","threadId":"61709","inReplyTo":null,"subject":"[PATCH v4] git-send-email: Use sanitized address when reading mbox body","fromName":"","fromEmail":"csókás","sentAt":"2024-07-01T09:01:16Z","receivedAt":"2024-07-01T09:03:29Z","isPatch":true,"sender":{"key":"csókás","avatar":null},"body":"Addresses that are mentioned on the trailers in the commit log\n('Signed-off-by: ' etc.) are added to @cc (unless suppressed),\npassed to the SMTP server. However, these hand-written\naddresses may be malformed (e.g. having unquoted commas and\nother punctuation marks in the display-name part).\n\nThe code was already calling `sanitize_address()` for suppression\npurposes, so we just have to use the result ($sc) for adding to @cc.\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 regular use with '+'-es in them).\n\nSigned-off-by: Csókás, Bence <csokas.bence@prolan.hu>\n---\n\nNotes:\n    Changes in v4:\n    * t9001: use ${SQ} instead of double quotes\n    * re-worded message again\n    Changes in v3:\n    * more testcases\n    * clarified wording in message\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 | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 45 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..64a4ab3736 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1299,6 +1299,49 @@ 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+!os@example.com!\n+!odd_?=mail@example.com!\n+!doug@example.com!\n+!codev@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+\tCc: Ronnie O${SQ}Sullivan <os@example.com>\n+\tReviewed-by: Füñný Nâmé <odd_?=mail@example.com>\n+\tReported-by: bugger on Jira\n+\tReported-by: Douglas Reporter <doug@example.com> [from Jira profile]\n+\tBugID: 12345\n+\tCo-developed-by: \"C. O. Developer\" <codev@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+\ttest_grep \"^(body) Adding cc: \\\"Person, One\\\" <one@example.com>\" actual-show-all-headers &&\n+\ttest_grep \"^(body) Adding cc: Ronnie O${SQ}Sullivan <os@example.com>\" actual-show-all-headers &&\n+\ttest_grep \"^(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+\ttest_grep \"^(body) Ignoring Reported-by .* bugger on Jira\" actual-show-all-headers &&\n+\ttest_grep \"^(body) Adding cc: Douglas Reporter <doug@example.com>\" actual-show-all-headers &&\n+\ttest_grep ! \"12345\" actual-show-all-headers &&\n+\ttest_grep \"^(body) Adding cc: \\\"C. O. Developer\\\" <codev@example.com>\" actual-show-all-headers &&\n+\ttest_grep \"^(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":"497913","messageId":"xmqqy16lvtix.fsf@gitster.g","threadId":"61709","inReplyTo":"20240701090115.56957-1-csokas.bence@prolan.hu","subject":"Re: [PATCH v4] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T18:47:34Z","receivedAt":"2024-07-01T18:47:40Z","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> Addresses that are mentioned on the trailers in the commit log\n> ('Signed-off-by: ' etc.) are added to @cc (unless suppressed),\n> passed to the SMTP server. However, these hand-written\n> addresses may be malformed (e.g. having unquoted commas and\n> other punctuation marks in the display-name part).\n>\n> The code was already calling `sanitize_address()` for suppression\n> purposes, so we just have to use the result ($sc) for adding to @cc.\n\nThere is a leap between the description of the status quo and your\nconclusion.  \"we just have to\" becomes valid only after explaining\nthat sanitize_address turns the address-looking string into valid\naddresses (and we do not want to send to malformed addresses--but\nthat goes without saying).\n\n> Also note that\n> `sanitize_address()` does not process the mailbox addresses,\n> so it is up to `sendmail` to handle special characters there\n> (e.g. there are mailboxes in regular use with '+'-es in them).\n\nI do not quote see the point of this final note.  mailboxes with\n'q'es in them are also in regular use, and singling out '+' does not\nmake much sense in the context of explaining this change.\n\n>     Changes in v4:\n>     * t9001: use ${SQ} instead of double quotes\n>     * re-worded message again\n\nOK.  The additional recipient address \n\n> +\tCo-developed-by: \"C. O. Developer\" <codev@example.com>\n\nto contrast with\n\n> +\tSigned-off-by: A. U. Thor <thor.au@example.com>\n\nis a nice touch.  We make sure that, with or without necessary\nquoting in the original, we produce the correct result ;-).\n\nLet's mark it for 'next' soonish, with proposed log message\nrewritten somewhat.\n\nThanks.\n\n----- >8 -----\ngit-send-email: use sanitized address when reading mbox body\n\nAddresses that are mentioned on the trailers in the commit log\nmessages (e.g., \"Reviewed-by\") are added to the \"Cc:\" list by \"git\nsend-email\".  These hand-written addresses, however, may be\nmalformed (e.g., having unquoted \".\" and other punctutation marks in\nthe display-name part) and can upset MTA.\n\nThe code does use the sanitize_address() helper on these\naddress-looking strings to turn them into valid addresses, but it is\nused only to see if the address should be suppressed.  The original\nstring taken from the message is added to the @cc list if the code\ndecides the address is not suppressed.\n\nBecause the addresses on trailer lines are hand-written and more\nlikely to contain malformed addresses, when adding to the @cc list,\nuse the result from sanitize_address, not the original.  Note that\nwe do not modify the behaviour for addresses taken from the e-mail\nheaders, as they are more likely to be machine generated and\nwell-formed.\n\nSigned-off-by: Csókás, Bence <csokas.bence@prolan.hu>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"498014","messageId":"dbdaf27a-4b11-44eb-ada7-9991628a47ce@prolan.hu","threadId":"61709","inReplyTo":"xmqqy16lvtix.fsf@gitster.g","subject":"Re: [PATCH v4] git-send-email: Use sanitized address when reading mbox body","fromName":"Csókás Bence","fromEmail":"csokas.bence@prolan.hu","sentAt":"2024-07-03T06:24:27Z","receivedAt":"2024-07-03T06:24:36Z","isPatch":true,"sender":{"key":"csokas.bence@prolan.hu","avatar":null},"body":"On 7/1/24 20:47, Junio C Hamano wrote:\n> Let's mark it for 'next' soonish, with proposed log message\n> rewritten somewhat.\n> \n> Thanks.\n> \n> ----- >8 -----\n\nAlright. Should I re-send it with this message then, or will you amend it?\n\nBence\n\n"},{"id":"498037","messageId":"xmqq5xtmfpk1.fsf@gitster.g","threadId":"61709","inReplyTo":"dbdaf27a-4b11-44eb-ada7-9991628a47ce@prolan.hu","subject":"Re: [PATCH v4] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T15:44:30Z","receivedAt":"2024-07-03T15:44:39Z","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> On 7/1/24 20:47, Junio C Hamano wrote:\n>> Let's mark it for 'next' soonish, with proposed log message\n>> rewritten somewhat.\n>> Thanks.\n>> ----- >8 -----\n>\n> Alright. Should I re-send it with this message then, or will you amend it?\n\nIf you are 100% happy with what you saw there, then just saying so\nwould be sufficient.  If not, you can update your commit with\nfurther improvements and send a v5 iteration.\n\nThanks.\n"}]}