{"thread":{"id":"29544","subject":"[PATCH] send-email: add extra safetly in address sanitazion","startedAt":"2012-02-04T16:32:42Z","lastAt":"2012-02-06T01:48:50Z","messageCount":7,"participants":["Felipe Contreras","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"183846","messageId":"1328373162-25188-1-git-send-email-felipe.contreras@gmail.com","threadId":"29544","inReplyTo":null,"subject":"[PATCH] send-email: add extra safetly in address sanitazion","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-04T16:32:42Z","receivedAt":"2012-02-04T16:32:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently bad addresses like 'Foo Bar >' will just be sent\nverbatim -- that's not good; we should either error out, or sanitize\nthem.\n\nThe following patch adds extra sanitazion so the following\ntransformations are performed:\n\n 'Foo Bar ' -> 'Foo Bar '\n '\"Foo Bar\" ' -> '\"Foo Bar\" '\n 'foo@bar.com' -> 'foo@bar.com'\n '' -> 'foo@bar.com'\n 'Foo Bar' -> 'Foo Bar'\n 'Foo Bar >' -> 'Foo Bar '\n '\"Foo Bar\" >' -> '\"Foo Bar\" '\n '>' -> 'foo@bar.com'\n\nBasically, we try to check that the address is in the form of\n\"Name \", and if not, assume it's \"email\". According to commit\n155197e[1], the \"prhase\" should not be empty, so if it is, remove the\n<>. Extra characters after the first \">\" are ignored.\n\n[1] send-email: rfc822 forbids using without a non-empty \"phrase\"\n\nSigned-off-by: Felipe Contreras \n---\n git-send-email.perl | 14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex ef30c55..19c600f 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -889,15 +889,21 @@ sub is_rfc2047_quoted {\n # use the simplest quoting being able to handle the recipient\n sub sanitize_address {\n \tmy ($recipient) = @_;\n-\tmy ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\\s*(<.*)/);\n+\tmy ($recipient_name, $recipient_addr);\n+\n+\tif ($recipient =~ /^(.*?)\\s*<(.*?)>/) {\n+\t\t($recipient_name, $recipient_addr) = ($1, $2);\n+\t} else {\n+\t\t$recipient_addr = $recipient;\n+\t}\n \n \tif (not $recipient_name) {\n-\t\treturn $recipient;\n+\t\treturn $recipient_addr;\n \t}\n \n \t# if recipient_name is already quoted, do nothing\n \tif (is_rfc2047_quoted($recipient_name)) {\n-\t\treturn $recipient;\n+\t\treturn \"$recipient_name <$recipient_addr>\";\n \t}\n \n \t# rfc2047 is needed if a non-ascii char is included\n@@ -912,7 +918,7 @@ sub sanitize_address {\n \t\t$recipient_name = qq[\"$recipient_name\"];\n \t}\n \n-\treturn \"$recipient_name $recipient_addr\";\n+\treturn \"$recipient_name <$recipient_addr>\";\n \n }\n \n-- \n1.7.9\n"},{"id":"183906","messageId":"87sjipxe5u.fsf@thomas.inf.ethz.ch","threadId":"29544","inReplyTo":"1328373162-25188-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] send-email: add extra safetly in address sanitazion","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-05T19:39:57Z","receivedAt":"2012-02-05T19:39:57Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> Currently bad addresses like 'Foo Bar >' will just be sent\n> verbatim -- that's not good; we should either error out, or sanitize\n> them.\n>\n> The following patch adds extra sanitazion so the following\n> transformations are performed:\n>\n> 'Foo Bar ' -> 'Foo Bar '\n> '\"Foo Bar\" ' -> '\"Foo Bar\" '\n> 'foo@bar.com' -> 'foo@bar.com'\n> '' -> 'foo@bar.com'\n> 'Foo Bar' -> 'Foo Bar'\n\nAm I the only one who stared at this for ten seconds, only to then\nrealize that there is no sanitizing whatsoever going on here?\n\n> 'Foo Bar >' -> 'Foo Bar '\n> '\"Foo Bar\" >' -> '\"Foo Bar\" '\n> '>' -> 'foo@bar.com'\n\nAll of these are the same underlying issue. Does your patch fix any\nother malformed addresses, or just this particular type?\n\n> Basically, we try to check that the address is in the form of\n> \"Name \", and if not, assume it's \"email\". According to commit\n> 155197e[1], the \"prhase\" should not be empty, so if it is, remove the\n ^^^^^^\n\"phrase\"\n\n> git-send-email.perl | 14 ++++++++++----\n> 1 files changed, 10 insertions(+), 4 deletions(-)\n\nTests?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"183915","messageId":"CAMP44s1wqmT4mavsXrEhB-OquOtQrYnnoSoX9G7X4wzFoMD29A@mail.gmail.com","threadId":"29544","inReplyTo":"87sjipxe5u.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] send-email: add extra safetly in address sanitazion","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-05T20:51:46Z","receivedAt":"2012-02-05T20:51:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"2012/2/5 Thomas Rast :\n> Felipe Contreras writes:\n>\n>> Currently bad addresses like 'Foo Bar >' will just be sent\n>> verbatim -- that's not good; we should either error out, or sanitize\n>> them.\n>>\n>> The following patch adds extra sanitazion so the following\n>> transformations are performed:\n>>\n>>   'Foo Bar ' -> 'Foo Bar '\n>>   '\"Foo Bar\" ' -> '\"Foo Bar\" '\n>>   'foo@bar.com' -> 'foo@bar.com'\n>>   '' -> 'foo@bar.com'\n>>   'Foo Bar' -> 'Foo Bar'\n>\n> Am I the only one who stared at this for ten seconds, only to then\n> realize that there is no sanitizing whatsoever going on here?\n\nThere is: '' -> 'foo@bar.com'\n\n>>   'Foo Bar >' -> 'Foo Bar '\n>>   '\"Foo Bar\" >' -> '\"Foo Bar\" '\n>>   '>' -> 'foo@bar.com'\n>\n> All of these are the same underlying issue.  Does your patch fix any\n> other malformed addresses, or just this particular type?\n\nSee above.\n\n-- \nFelipe Contreras\n"},{"id":"183926","messageId":"877h01rlsr.fsf@thomas.inf.ethz.ch","threadId":"29544","inReplyTo":"CAMP44s1wqmT4mavsXrEhB-OquOtQrYnnoSoX9G7X4wzFoMD29A@mail.gmail.com","subject":"Re: [PATCH] send-email: add extra safetly in address sanitazion","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-05T21:51:32Z","receivedAt":"2012-02-05T21:51:32Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> 2012/2/5 Thomas Rast :\n>> Felipe Contreras writes:\n>>>\n>>>   'Foo Bar ' -> 'Foo Bar '\n>>>   '\"Foo Bar\" ' -> '\"Foo Bar\" '\n>>>   'foo@bar.com' -> 'foo@bar.com'\n>>>   '' -> 'foo@bar.com'\n>>>   'Foo Bar' -> 'Foo Bar'\n>>\n>> Am I the only one who stared at this for ten seconds, only to then\n>> realize that there is no sanitizing whatsoever going on here?\n>\n> There is: '' -> 'foo@bar.com'\n\nIndeed.\n\nI still feel cheated as a reader though, you showed me four examples of\nno change but let me figure that on my own.\n\n>>>   'Foo Bar >' -> 'Foo Bar '\n>>>   '\"Foo Bar\" >' -> '\"Foo Bar\" '\n>>>   '>' -> 'foo@bar.com'\n>>\n>> All of these are the same underlying issue.  Does your patch fix any\n>> other malformed addresses, or just this particular type?\n>\n> See above.\n\nOk, I see I am falling into the same communication trap as Jonathan, so\nlet's be more explicit.\n\nYour commit message first tells me you are going to sanitize something,\nbut starts out with examples of leaving the string unchanged. Then it\ncontinues with only the '>>' examples.\n\nToday, and being someone who on average reads about half the mail that\ncomes through here, I know that this relates to the blame -e '>>' bug.\nSo today, I am wondering from the commit message why you narrowly focus\non that bug. But you don't! It's just that the commit message\ninsinuates it.\n\nIn a year, your reader (and bear in mind that this may very well be\nyourself, at least if your memory is as good as mine) will wonder what\nwas so damn special about that '>>' string that it needs a specific fix\nto send-email.\n\nI see that you wrote in another thread:\n\n> I have to write a peer-reviewed essay with an introduction for the\n> people that are not familiar with the code in each of the patches\n\nI'm not sure you meant it that literally, but the whole *point* is that\nthe message is for people who are not familiar with the code. After\nall, if I knew that your code did the right thing in the right way, I\nwould not be bothering with reading the message. Today, I would just\nsend an Acked-by instead. In a year, I'd scroll down for another\npotential culprit for the bug I'm hunting.\n\nWhat's especially striking me about your proposed messages of late: they\nleave me with more open questions than I started with. I tried to show\nthis above. I'm not sure whether other contributors are better at\nanswering questions, or just better at not touching any topics that\nmight raise them.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"183927","messageId":"7vbopdeyne.fsf@alter.siamese.dyndns.org","threadId":"29544","inReplyTo":"87sjipxe5u.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] send-email: add extra safetly in address sanitazion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-05T21:52:21Z","receivedAt":"2012-02-05T21:52:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Am I the only one who stared at this for ten seconds, only to then\n> realize that there is no sanitizing whatsoever going on here?\n>\n>> 'Foo Bar >' -> 'Foo Bar '\n>> '\"Foo Bar\" >' -> '\"Foo Bar\" '\n>> '>' -> 'foo@bar.com'\n>\n> All of these are the same underlying issue. Does your patch fix any\n> other malformed addresses, or just this particular type?\n\nJust this particular type, as long as the code handles it correctly, would\nbe better than nothing.\n\nOn the recieving end in mailinfo, I think we also support\n\n\tgitster@pobox.com (Junio C Hamano)\n\nalthough it does not seem to be used by many people these days (the only\none I can remember seeing on this list was merlyn), so we may want to be a\nbit more consistent between sending and receiving end, though.\n"},{"id":"183951","messageId":"CAMP44s30VmJasMLJxs-JFwksvPEPpG1LB3Gr_pA2+hpE1AnwXg@mail.gmail.com","threadId":"29544","inReplyTo":"877h01rlsr.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] send-email: add extra safetly in address sanitazion","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-06T01:27:50Z","receivedAt":"2012-02-06T01:27:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Feb 5, 2012 at 11:51 PM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> 2012/2/5 Thomas Rast :\n>>> Felipe Contreras writes:\n>>>>\n>>>>   'Foo Bar ' -> 'Foo Bar '\n>>>>   '\"Foo Bar\" ' -> '\"Foo Bar\" '\n>>>>   'foo@bar.com' -> 'foo@bar.com'\n>>>>   '' -> 'foo@bar.com'\n>>>>   'Foo Bar' -> 'Foo Bar'\n>>>\n>>> Am I the only one who stared at this for ten seconds, only to then\n>>> realize that there is no sanitizing whatsoever going on here?\n>>\n>> There is: '' -> 'foo@bar.com'\n>\n> Indeed.\n>\n> I still feel cheated as a reader though, you showed me four examples of\n> no change but let me figure that on my own.\n>\n>>>>   'Foo Bar >' -> 'Foo Bar '\n>>>>   '\"Foo Bar\" >' -> '\"Foo Bar\" '\n>>>>   '>' -> 'foo@bar.com'\n>>>\n>>> All of these are the same underlying issue.  Does your patch fix any\n>>> other malformed addresses, or just this particular type?\n>>\n>> See above.\n>\n> Ok, I see I am falling into the same communication trap as Jonathan, so\n> let's be more explicit.\n>\n> Your commit message first tells me you are going to sanitize something,\n> but starts out with examples of leaving the string unchanged.  Then it\n> continues with only the '>>' examples.\n\nWhich is why I added a paragraph to explain them. What is unclear about?\n\n---\nAccording to commit 155197e[1], the \"prhase\" should not be empty, so\nif it is, remove the\n<>. Extra characters after the first \">\" are ignored.\n---\n\n> Today, and being someone who on average reads about half the mail that\n> comes through here, I know that this relates to the blame -e '>>' bug.\n> So today, I am wondering from the commit message why you narrowly focus\n> on that bug.  But you don't!  It's just that the commit message\n> insinuates it.\n\nThe summary explains the purpose of the patch \"add extra safety in\naddress sanitation\" (should fix those typos though).\n\n> In a year, your reader (and bear in mind that this may very well be\n> yourself, at least if your memory is as good as mine) will wonder what\n> was so damn special about that '>>' string that it needs a specific fix\n> to send-email.\n\nIt doesn't matter, could be \" err blop\", or any number of\nother malformed strings.\n\n> I see that you wrote in another thread:\n>\n>> I have to write a peer-reviewed essay with an introduction for the\n>> people that are not familiar with the code in each of the patches\n>\n> I'm not sure you meant it that literally, but the whole *point* is that\n> the message is for people who are not familiar with the code.  After\n> all, if I knew that your code did the right thing in the right way, I\n> would not be bothering with reading the message.  Today, I would just\n> send an Acked-by instead.  In a year, I'd scroll down for another\n> potential culprit for the bug I'm hunting.\n\nYou are assuming too much. In this case, the code is clear and doesn't\nneed explaining. I am talking about other cases which in my mind are\nakin to explaining what is $recipient, $recipient_name, and what does\nsanitize_address does, and why the if case for is_rfc2047_quoted is\nmodified. IMO that's overkill.\n\nIf you have some suggestion about how to improve the commit message, I\nwould be glad to listen to them, as in this case, I do believe the\nchanges merit some clear explanation. Not all patches do, though.\n\n> What's especially striking me about your proposed messages of late: they\n> leave me with more open questions than I started with.  I tried to show\n> this above.  I'm not sure whether other contributors are better at\n> answering questions, or just better at not touching any topics that\n> might raise them.\n\nAgain, what is not clear about:\n\n---\nBasically, we try to check that the address is in the form of\n\"Name \", and if not, assume it's \"email\". According to commit\n155197e[1], the \"prhase\" should not be empty, so if it is, remove the\n<>. Extra characters after the first \">\" are ignored.\n---\n\nTo me that explains what the patch is trying to do: \"add extra safety\nin address sanitation\".\n\nAnyway, it seems people don't care if 'git send-email' attempts to\nsend random garbage regardless, so I'm not going to pursue this patch.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"183953","messageId":"7vy5sgbukd.fsf@alter.siamese.dyndns.org","threadId":"29544","inReplyTo":"CAMP44s30VmJasMLJxs-JFwksvPEPpG1LB3Gr_pA2+hpE1AnwXg@mail.gmail.com","subject":"Re: [PATCH] send-email: add extra safetly in address sanitazion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T01:48:50Z","receivedAt":"2012-02-06T01:48:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Anyway, it seems people don't care if 'git send-email' attempts to\n> send random garbage regardless, so I'm not going to pursue this patch.\n\nI actually think people _do_ care, and that is the _only_ reason you are\ngetting review comments.\n"}]}