{"thread":{"id":"61691","subject":"[PATCH v3] git-send-email: Use sanitized address when reading mbox body","startedAt":"2024-06-28T08:51:25Z","lastAt":"2024-06-28T19:30:49Z","messageCount":2,"participants":["csókás","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"497801","messageId":"20240628085018.65076-2-csokas.bence@prolan.hu","threadId":"61691","inReplyTo":null,"subject":"[PATCH v3] git-send-email: Use sanitized address when reading mbox body","fromName":"","fromEmail":"csókás","sentAt":"2024-06-28T08:50:19Z","receivedAt":"2024-06-28T08:51:25Z","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. This is handled by calling\n`sanitize_address()` before adding addresses to @cc. This function\nwas already being called, but was only used for comparing it to\n$author for suppression purposes.\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 \n---\n\n[ sorry, I forgot the --notes: ]\n\nNotes:\n Changes in v2:\n * added testcase to t9001\n * added rationale behind trusting mbox headers and the address-parts\n Changes in v3:\n * more testcases\n * clarified wording in message\n\n git-send-email.perl | 4 ++--\n t/t9001-send-email.sh | 51 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 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..8bbbf20855 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -1299,6 +1299,57 @@ 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+!doug@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 \n+\tReviewed-by: Füñný Nâmé \n+\tReported-by: bugger on Jira\n+\tReported-by: Douglas Reporter [from Jira profile]\n+\tBugID: 12345\n+\tSigned-off-by: A. U. Thor \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\\\" \" 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+\" \" actual-show-all-headers &&\n+\ttest_grep \"^(body) Adding cc: Douglas Reporter \" actual-show-all-headers &&\n+\ttest_grep \"^(body) Adding cc: \\\"A. U. Thor\\\" \" actual-show-all-headers\n+'\n+\n+test_expect_success $PREREQ 'quotes are sanitized in cc list' \"\n+\tclean_fake_sendmail &&\n+\ttest_commit quote_in_cc_body &&\n+\ttest_when_finished \\\"git reset --hard HEAD^\\\" &&\n+\tgit commit --amend -F - <<-EOF &&\n+\tQuotation marks sanitization in Cc:.\n+\n+\tCc: P'erson, One \n+\tReported-by: \\\"Douglas 'Bug' Reporter\\\" \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_grep \\\"^(body) Adding cc: \\\\\\\"P'erson, One\\\\\\\" \\\" actual-show-all-headers &&\n+\ttest_grep \\\"^(body) Adding cc: \\\\\\\"Douglas 'Bug' Reporter\\\\\\\" \\\" 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":"497825","messageId":"xmqq1q4g3lwh.fsf@gitster.g","threadId":"61691","inReplyTo":"20240628085018.65076-2-csokas.bence@prolan.hu","subject":"Re: [PATCH v3] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-28T19:30:38Z","receivedAt":"2024-06-28T19:30:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Csókás, Bence\" writes:\n\n> Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: '\n> etc. lines mess with git-send-email.\n\nCalling these \"trailers\" would make it more obvious that you are\ntalking about parts of the log message (\"S-o-b\" would not be\nmistaken as an e-mail header, but you cannot complain if a reader\nmistook the mention of \"Cc\" as a reference to an e-mail header).\n\nMore importantly, \"mess with\" is true when potentially\naddress-looking strings from the trailer lines are added to @cc\nlist. So perhaps\n\n Addresses that are mentioned on the trailers in the commit log\n messages (e.g., \"Reviewed-by\") are added to the \"Cc:\" list\n (unless suppressed) by \"git send-email\". These hand-written\n addresses may be malformed (e.g., having unquoted \".\" and other\n punctutation marks in the display-name part).\n\n> This is handled by calling\n> `sanitize_address()` before adding addresses to @cc. This function\n> was already being called, but was only used for comparing it to\n> $author for suppression purposes.\n\nSo this is not telling the truth. This is *NOT* handled by calling\nthe helper function, as the result is *NOT* added to @cc. I would\nsuggest striking this paragraph (and the rest of the log message)\nout and rewrite, perhaps like so:\n\n The code does use sanitize_address on these address-looking\n strings to make them into valid addresses, but the result was\n used only to see if the address should be suppressed. The\n original string taken from the message was added to the @cc list.\n\n Because the addresses on trailer lines are hand-written and more\n likely to contain malformed addresses, when adding to the @cc\n list, use the result from sanitize_address, not the original.\n\n Note that we do not modify the behaviour for addresses taken\n from the e-mail headers, as they are more likely to be machine\n generated and well-formed.\n\nor something.\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 58699f8e4e..8bbbf20855 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -1299,6 +1299,57 @@ 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> +!doug@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 \n> +\tReviewed-by: Füñný Nâmé \n> +\tReported-by: bugger on Jira\n> +\tReported-by: Douglas Reporter [from Jira profile]\n> +\tBugID: 12345\n> +\tSigned-off-by: A. U. Thor \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\\\" \" 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> +\" \" actual-show-all-headers &&\n> +\ttest_grep \"^(body) Adding cc: Douglas Reporter \" actual-show-all-headers &&\n> +\ttest_grep \"^(body) Adding cc: \\\"A. U. Thor\\\" \" actual-show-all-headers\n> +'\n\nThe test does not check that \"bugger on Jira\" and \"12345\" are not\nused as recipients, but it should. Otherwise your effort to add the\nextra trailers to the test goes wasted.\n\nIt is a good choice to use Reported-by and BugID as two extra ones,\nby the way. The mechanisms that causes them omitted from the\nresulting message are different.\n\n> +\n> +test_expect_success $PREREQ 'quotes are sanitized in cc list' \"\n> +\tclean_fake_sendmail &&\n> +\ttest_commit quote_in_cc_body &&\n> +\ttest_when_finished \\\"git reset --hard HEAD^\\\" &&\n> +\tgit commit --amend -F - <<-EOF &&\n> +\tQuotation marks sanitization in Cc:.\n> +\n> +\tCc: P'erson, One \n> +\tReported-by: \\\"Douglas 'Bug' Reporter\\\" \n> +\tEOF\n\nQuoting the test body in a pair of double quotes is error prone. Do\nnot do it (and you do have a bug here that was caused by it).\n\nRather, write ${SQ} where you would write a single quote in your\ntest body, and enclose the test body in a pair of single quotes,\nlike everybody else does.\n\n> +\tgit send-email -1 --to=recipient@example.com \\\n> +\t\t--smtp-server=\\\"$(pwd)/fake.sendmail\\\" >actual-show-all-headers &&\n\nHere is the bug. This $(pwd) command substition is done *before*\nthe test runs---it is expanded while the shell computes the command\nline to run test_expect_success (as part of the test body string\nthat is quoted in a pair of double-quotes). So if your cwd has a\ncharacter that needs to be quoted inside double-quotes to be a valid\nvalue for the \"--smtp-server\" option, you end up breaking the shell\nquoting rules. In other words: what happens if your $(pwd) has a\ndouble-quote in it?\n\n> +\ttest_grep \\\"^(body) Adding cc: \\\\\\\"P'erson, One\\\\\\\" \\\" actual-show-all-headers &&\n> +\ttest_grep \\\"^(body) Adding cc: \\\\\\\"Douglas 'Bug' Reporter\\\\\\\" \\\" actual-show-all-headers\n> +\"\n> +\n\nSo, I do not see a strong reason why we want to have this as a\nseparate test. We can write and test addresses with single quotes\nin it as part of the previous test.\n\nA squashable change on top of your version is attached at the end.\n\n * Added a name with apostrophe in it on the Cc: line, and check\n that the address appears on the recipient list and also in the\n log.\n\n * Added checks for \"Reported-by: bugger on Jira\" that explicitly\n gets ignored in the log, and \"BugId:\" that is not even considered\n for Cc: (in other words, not appearing in the log at all).\n\nThanks.\n\n t/t9001-send-email.sh | 21 +++++----------------\n 1 file changed, 5 insertions(+), 16 deletions(-)\n\ndiff --git c/t/t9001-send-email.sh w/t/t9001-send-email.sh\nindex 658f3a72ae..64c56677cd 100755\n--- c/t/t9001-send-email.sh\n+++ w/t/t9001-send-email.sh\n@@ -1304,6 +1304,7 @@ 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 !thor.au@example.com!\n@@ -1318,6 +1319,7 @@ test_expect_success $PREREQ 'cc list is sanitized' '\n \tTest Cc: sanitization.\n \n \tCc: Person, One \n+\tCc: Ronnie O${SQ}Sullivan \n \tReviewed-by: Füñný Nâmé \n \tReported-by: bugger on Jira\n \tReported-by: Douglas Reporter [from Jira profile]\n@@ -1328,28 +1330,15 @@ test_expect_success $PREREQ 'cc list is sanitized' '\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\\\" \" actual-show-all-headers &&\n+\ttest_grep \"^(body) Adding cc: Ronnie O${SQ}Sullivan \" 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 \" \" 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 \" actual-show-all-headers &&\n+\ttest_grep ! \"12345\" actual-show-all-headers &&\n \ttest_grep \"^(body) Adding cc: \\\"A. U. Thor\\\" \" actual-show-all-headers\n '\n \n-test_expect_success $PREREQ 'quotes are sanitized in cc list' \"\n-\tclean_fake_sendmail &&\n-\ttest_commit quote_in_cc_body &&\n-\ttest_when_finished \\\"git reset --hard HEAD^\\\" &&\n-\tgit commit --amend -F - <<-EOF &&\n-\tQuotation marks sanitization in Cc:.\n-\n-\tCc: P'erson, One \n-\tReported-by: \\\"Douglas 'Bug' Reporter\\\" \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_grep \\\"^(body) Adding cc: \\\\\\\"P'erson, One\\\\\\\" \\\" actual-show-all-headers &&\n-\ttest_grep \\\"^(body) Adding cc: \\\\\\\"Douglas 'Bug' Reporter\\\\\\\" \\\" 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"}]}