{"thread":{"id":"61665","subject":"[PATCH resend] git-send-email: Use sanitized address when reading mbox body","startedAt":"2024-06-21T09:28:34Z","lastAt":"2024-06-26T13:33:32Z","messageCount":6,"participants":["csókás","Junio C Hamano","Csókás Bence"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497473","messageId":"20240621092721.2980939-2-csokas.bence@prolan.hu","threadId":"61665","inReplyTo":null,"subject":"[PATCH resend] git-send-email: Use sanitized address when reading mbox body","fromName":"","fromEmail":"csókás","sentAt":"2024-06-21T09:27:22Z","receivedAt":"2024-06-21T09:28:34Z","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\nSigned-off-by: Csókás, Bence <csokas.bence@prolan.hu>\n---\n git-send-email.perl | 4 ++--\n 1 file changed, 2 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;\n-- \n2.34.1\n\n\n"},{"id":"497506","messageId":"xmqqr0cqmck5.fsf@gitster.g","threadId":"61665","inReplyTo":"20240621092721.2980939-2-csokas.bence@prolan.hu","subject":"Re: [PATCH resend] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T17:27:06Z","receivedAt":"2024-06-21T17:27:17Z","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>\n> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>\n> ---\n>  git-send-email.perl | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 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\nHmph, there is this piece of code before the block:\n\n\t\t\tif ($c !~ /.+@.+|<.+>/) {\n\t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n\t\t\t\t\t$what, $_) unless $quiet;\n\t\t\t\tnext;\n\t\t\t}\n\nThis is to reject strings that do not even look like an e-mail\naddress, but we called sanitize_address() call on $c way before this\ncheck.  I wonder if we should move this block way up, even before\nthe call to santize_address()?\n\nThat was a relatively unrelated tangent.\n\nIn the same function, there is this snippet about Cc: (I didn't\ncheck if the same issue is shared with other header fields):\n\n\t\t\telsif (/^Cc:\\s+(.*)$/i) {\n\t\t\t\tforeach my $addr (parse_address_line($1)) {\n\t\t\t\t\tmy $qaddr = unquote_rfc2047($addr);\n\t\t\t\t\tmy $saddr = sanitize_address($qaddr);\n\t\t\t\t\tif ($saddr eq $sender) {\n\t\t\t\t\t\tnext if ($suppress_cc{'self'});\n\t\t\t\t\t} else {\n\t\t\t\t\t\tnext if ($suppress_cc{'cc'});\n\t\t\t\t\t}\n\t\t\t\t\tprintf(__(\"(mbox) Adding cc: %s from line '%s'\\n\"),\n\t\t\t\t\t\t$addr, $_) unless $quiet;\n\t\t\t\t\tpush @cc, $addr;\n\t\t\t\t}\n\t\t\t}\n\nWe seem to use sanitized address only for the purpose of suppressing\nCc, and use the original address given in the input for e-mail\npurposes (@cc is the same variable as the patch under discussion\nsends the \"fixed\" address to, which holds the data to formulate the\nCc: header of the outgoing message, I presume?).  So in that way, we\nseem to be very consistent.\n\nPossibly we are being consistent in a broken way, but I am not yet\nconvinced that we are.\n\nIt looks to me that there are many other places that we try to be as\nfaithful to the original as possible.  In the same block as the one\nthat handles \"Cc:\" I quoted above, an address on \"From:\" is also\nsent intact into @cc and addresses on \"To:\" are handled the same\nway.\n\nThe patch under discussion singles out the addresses on the trailers\nin the message body and treat them differently from others, which I\nam not sure is what we want to do.\n"},{"id":"497558","messageId":"a55cc932-98b0-43cd-9dfa-ea21a4340bf6@prolan.hu","threadId":"61665","inReplyTo":"xmqqr0cqmck5.fsf@gitster.g","subject":"Re: [PATCH resend] git-send-email: Use sanitized address when reading mbox body","fromName":"Csókás Bence","fromEmail":"csokas.bence@prolan.hu","sentAt":"2024-06-24T07:37:51Z","receivedAt":"2024-06-24T07:38:00Z","isPatch":true,"sender":{"key":"csokas.bence@prolan.hu","avatar":null},"body":"Hi!\n\nOn 6/21/24 19:27, Junio C Hamano wrote:\n> Hmph, there is this piece of code before the block:\n> \n> \t\t\tif ($c !~ /.+@.+|<.+>/) {\n> \t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n> \t\t\t\t\t$what, $_) unless $quiet;\n> \t\t\t\tnext;\n> \t\t\t}\n> \n> This is to reject strings that do not even look like an e-mail\n> address, but we called sanitize_address() call on $c way before this\n> check.  I wonder if we should move this block way up, even before\n> the call to santize_address()?\n> \n> That was a relatively unrelated tangent.\n\nHm, maybe. Should I add this to the patch as well?\n\n> In the same function, there is this snippet about Cc: (I didn't\n> check if the same issue is shared with other header fields):\n\nYes, I've seen that. However, the mbox headers are more likely to \nconform to RFC 2047, or at least `git format-patch` generates correct \nmbox headers. (Before sending the original patch, I was playing around \nwith purposefully broken mbox files, and at least Thunderbird seems to \ncorrectly parse these non-conforming mbox headers in _most_ cases, but \nthat's probably just extra caution on Mozilla's side).\n\n> It looks to me that there are many other places that we try to be as\n> faithful to the original as possible.  In the same block as the one\n> that handles \"Cc:\" I quoted above, an address on \"From:\" is also\n> sent intact into @cc and addresses on \"To:\" are handled the same\n> way.\n> \n> The patch under discussion singles out the addresses on the trailers\n> in the message body and treat them differently from others, which I\n> am not sure is what we want to do.\n\nI think it is a reasonable assumption that the mbox headers will be \nconforming, whereas the message body is just freeform text and no such \nguarantees exist. But if we want to be paranoid about it, we could try \nand sanitize everything.\n\nBence\n\n"},{"id":"497573","messageId":"xmqqy16ue3vl.fsf@gitster.g","threadId":"61665","inReplyTo":"a55cc932-98b0-43cd-9dfa-ea21a4340bf6@prolan.hu","subject":"Re: [PATCH resend] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-24T15:50:54Z","receivedAt":"2024-06-24T15:51:05Z","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 6/21/24 19:27, Junio C Hamano wrote:\n>> Hmph, there is this piece of code before the block:\n>> \t\t\tif ($c !~ /.+@.+|<.+>/) {\n>> \t\t\t\tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n>> \t\t\t\t\t$what, $_) unless $quiet;\n>> \t\t\t\tnext;\n>> \t\t\t}\n>> This is to reject strings that do not even look like an e-mail\n>> address, but we called sanitize_address() call on $c way before this\n>> check.  I wonder if we should move this block way up, even before\n>> the call to santize_address()?\n>> That was a relatively unrelated tangent.\n>\n> Hm, maybe. Should I add this to the patch as well?\n\nNo, it is an unrelated tangent.  We try not to mix unrelated things\ninto a single patch.  Perhaps as a separate \"clean-up\" patch after\nthe dust from the main topic settles, or as a preliminary \"clean-up\"\nbefore the main topic, would be good (but see below).\n\n>> It looks to me that there are many other places that we try to be as\n>> faithful to the original as possible.  In the same block as the one\n>> that handles \"Cc:\" I quoted above, an address on \"From:\" is also\n>> sent intact into @cc and addresses on \"To:\" are handled the same\n>> way.\n>> The patch under discussion singles out the addresses on the trailers\n>> in the message body and treat them differently from others, which I\n>> am not sure is what we want to do.\n>\n> I think it is a reasonable assumption that the mbox headers will be\n> conforming, whereas the message body is just freeform text and no such\n> guarantees exist. But if we want to be paranoid about it, we could try\n> and sanitize everything.\n\nI actually do not think we want to be more paranoid.  I'd rather\ntrust what the user gave us, which was where my response came from.\n\nHaving said that, given that the \"Ignoring ...\" check we saw earlier\nin this message triggers only for trailers, it may be a sensible\nposition to take that mail headers are more trustworthy and a random\naddress-looking strings on the trailer lines should be checked more\ncarefully.  Because the \"Ignoring ...\" check is primarily to reject\nstrings that are not addresses (things like bug IDs, or just\npeople's names without e-mail addresses to credit for a new feature\nrequest) that we do not ever intend to actually Cc: the message to,\nanother sensible choice coming from that \"strings on trailers may\nnot even be addresses\" position may be not to add the $c to the Cc:\nlist, if $sc (the sanitized address) and $c (the original address)\nare different.  That is, \"the simple regexp check currently used to\ntrigger 'Ignoring ...' message thought that the string looked like\nan address, but when we ask sanitize_address, it turns out that it\nwas not, really.\"  And if we were to take that route, \"Ignoring ...\"\ncheck, which I called an unrelated tangent, would become very much\nrelated to the topic at hand, as it would mean the resulting logic\nwould look more like this:\n\n\tmy $sc = sanitize_address($c);\n\tif ($c !~ /.+@.+|<.+>/ || $sc ne $c) {\n        \tprintf(\"(body) Ignoring %s from line '%s'\\n\",\n\t\t       $what, $_) unless $quiet;\n\t\tnext;\n\t}\n\nIn any case, if we were to move forward with this topic (whether\n\"send to corrected $sc instead\" or \"$c is suspicious, do not add it\nto $cc\" is picked as the direction), the motivation behind the\ndesign decision to treat the address taken from a trailer line\ndifferently needs to be explained better, I think.  I had a chance\nto ask you directly on list, and you gave a good explanation to me\nin the message I am responding to, but you will not be around when\nfuture readers of \"git log\" want to ask the same question \"why are\nwe singling this out while we use the original address in all the\nother places?\".\n\nYour proposed commit log message is the place to help them.\n\nOh, before I forget, is this something we can test easily in t9001?\nWe would want to protect a new behaviour from accidental breakage,\nso adding a test or two would be a good thing.\n\nThanks.\n"},{"id":"497675","messageId":"202fc42e-c5be-4a4b-a135-302d8fbf6b2a@prolan.hu","threadId":"61665","inReplyTo":"xmqqy16ue3vl.fsf@gitster.g","subject":"Re: [PATCH resend] git-send-email: Use sanitized address when reading mbox body","fromName":"Csókás Bence","fromEmail":"csokas.bence@prolan.hu","sentAt":"2024-06-26T08:58:03Z","receivedAt":"2024-06-26T08:58:08Z","isPatch":true,"sender":{"key":"csokas.bence@prolan.hu","avatar":null},"body":"Hi!\n\nOn 6/24/24 17:50, Junio C Hamano wrote:\n> another sensible choice coming from that \"strings on trailers may\n> not even be addresses\" position may be not to add the $c to the Cc:\n> list, if $sc (the sanitized address) and $c (the original address)\n> are different.  That is, \"the simple regexp check currently used to\n> trigger 'Ignoring ...' message thought that the string looked like\n> an address, but when we ask sanitize_address, it turns out that it\n> was not, really.\"\n\nMaybe. Though we would need to run through the sanitized address through \n`unquote_rfc2047()` first. But I don't think it's necessary; if someone \nfeeds us an \"unsanitary\" address (for instance, there is whitespace \nbetween the angle brackets), we should try to make sense of it, and \nworst case, the SMTP server rejects it, as it does now.\n\n> In any case, if we were to move forward with this topic (whether\n> \"send to corrected $sc instead\" or \"$c is suspicious, do not add it\n> to $cc\" is picked as the direction), the motivation behind the\n> design decision to treat the address taken from a trailer line\n> differently needs to be explained better, I think. [...]\n> Your proposed commit log message is the place to help them.\n\nOkay. So in short, I should add justification for trusting mbox headers \nand not the message body, correct?\n\n> Oh, before I forget, is this something we can test easily in t9001?\n> We would want to protect a new behaviour from accidental breakage,\n> so adding a test or two would be a good thing.\n\nMaybe, I'll look into it.\n\nBence\n\n"},{"id":"497695","messageId":"xmqqzfr77rrv.fsf@gitster.g","threadId":"61665","inReplyTo":"202fc42e-c5be-4a4b-a135-302d8fbf6b2a@prolan.hu","subject":"Re: [PATCH resend] git-send-email: Use sanitized address when reading mbox body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T13:33:24Z","receivedAt":"2024-06-26T13:33:32Z","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>> In any case, if we were to move forward with this topic (whether\n>> \"send to corrected $sc instead\" or \"$c is suspicious, do not add it\n>> to $cc\" is picked as the direction), the motivation behind the\n>> design decision to treat the address taken from a trailer line\n>> differently needs to be explained better, I think. [...]\n>> Your proposed commit log message is the place to help them.\n>\n> Okay. So in short, I should add justification for trusting mbox\n> headers and not the message body, correct?\n\nWe want these to be explained for future readers:\n\n (1) we stuff the sanitized address to @cc in this particular place,\n     but we still let all other places copy the original taken from\n     the message to @to or @cc (as in the original code).\n\n (2) the reason behind the above difference is because we trust less\n     about the \"address looking\" strings that appear on the trailer\n     lines.\n\nSo, not just (2), but in order for the readers to understand why\nthey should care about (2), they need to be told (1) as well.\n\nThanks.\n"}]}