[PATCH] send-email: add extra safetly in address sanitazion

[PATCH] send-email: add extra safetly in address sanitazion

[Felipe Contreras]

Currently bad addresses like 'Foo Bar <foo@bar.com>>' will just be sent
verbatim -- that's not good; we should either error out, or sanitize
them.

The following patch adds extra sanitazion so the following
transformations are performed:

  'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
  '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
  'foo@bar.com' -> 'foo@bar.com'
  '<foo@bar.com>' -> 'foo@bar.com'
  'Foo Bar' -> 'Foo Bar'
  'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
  '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
  '<foo@bar.com>>' -> 'foo@bar.com'

Basically, we try to check that the address is in the form of
"Name <email>", and if not, assume it's "email". According to commit
155197e[1], the "prhase" should not be empty, so if it is, remove the
<>. Extra characters after the first ">" are ignored.

[1] send-email: rfc822 forbids using <address@domain> without a non-empty "phrase"

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-send-email.perl |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ef30c55..19c600f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -889,15 +889,21 @@ sub is_rfc2047_quoted {
 # use the simplest quoting being able to handle the recipient
 sub sanitize_address {
 	my ($recipient) = @_;
-	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
+	my ($recipient_name, $recipient_addr);
+
+	if ($recipient =~ /^(.*?)\s*<(.*?)>/) {
+		($recipient_name, $recipient_addr) = ($1, $2);
+	} else {
+		$recipient_addr = $recipient;
+	}
 
 	if (not $recipient_name) {
-		return $recipient;
+		return $recipient_addr;
 	}
 
 	# if recipient_name is already quoted, do nothing
 	if (is_rfc2047_quoted($recipient_name)) {
-		return $recipient;
+		return "$recipient_name <$recipient_addr>";
 	}
 
 	# rfc2047 is needed if a non-ascii char is included
@@ -912,7 +918,7 @@ sub sanitize_address {
 		$recipient_name = qq["$recipient_name"];
 	}
 
-	return "$recipient_name $recipient_addr";
+	return "$recipient_name <$recipient_addr>";
 
 }
 
-- 
1.7.9

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Currently bad addresses like 'Foo Bar <foo@bar.com>>' will just be sent
> verbatim -- that's not good; we should either error out, or sanitize
> them.
>
> The following patch adds extra sanitazion so the following
> transformations are performed:
>
>   'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
>   '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
>   'foo@bar.com' -> 'foo@bar.com'
>   '<foo@bar.com>' -> 'foo@bar.com'
>   'Foo Bar' -> 'Foo Bar'

Am I the only one who stared at this for ten seconds, only to then
realize that there is no sanitizing whatsoever going on here?

>   'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>   '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>   '<foo@bar.com>>' -> 'foo@bar.com'

All of these are the same underlying issue.  Does your patch fix any
other malformed addresses, or just this particular type?

> Basically, we try to check that the address is in the form of
> "Name <email>", and if not, assume it's "email". According to commit
> 155197e[1], the "prhase" should not be empty, so if it is, remove the
                   ^^^^^^
"phrase"

>  git-send-email.perl |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)

Tests?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Felipe Contreras]

2012/2/5 Thomas Rast <trast@inf.ethz.ch>:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Currently bad addresses like 'Foo Bar <foo@bar.com>>' will just be sent
>> verbatim -- that's not good; we should either error out, or sanitize
>> them.
>>
>> The following patch adds extra sanitazion so the following
>> transformations are performed:
>>
>>   'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
>>   '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
>>   'foo@bar.com' -> 'foo@bar.com'
>>   '<foo@bar.com>' -> 'foo@bar.com'
>>   'Foo Bar' -> 'Foo Bar'
>
> Am I the only one who stared at this for ten seconds, only to then
> realize that there is no sanitizing whatsoever going on here?

There is: '<foo@bar.com>' -> 'foo@bar.com'

>>   'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>>   '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>>   '<foo@bar.com>>' -> 'foo@bar.com'
>
> All of these are the same underlying issue.  Does your patch fix any
> other malformed addresses, or just this particular type?

See above.

-- 
Felipe Contreras

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> 2012/2/5 Thomas Rast <trast@inf.ethz.ch>:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>   'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
>>>   '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
>>>   'foo@bar.com' -> 'foo@bar.com'
>>>   '<foo@bar.com>' -> 'foo@bar.com'
>>>   'Foo Bar' -> 'Foo Bar'
>>
>> Am I the only one who stared at this for ten seconds, only to then
>> realize that there is no sanitizing whatsoever going on here?
>
> There is: '<foo@bar.com>' -> 'foo@bar.com'

Indeed.

I still feel cheated as a reader though, you showed me four examples of
no change but let me figure that on my own.

>>>   'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>>>   '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>>>   '<foo@bar.com>>' -> 'foo@bar.com'
>>
>> All of these are the same underlying issue.  Does your patch fix any
>> other malformed addresses, or just this particular type?
>
> See above.

Ok, I see I am falling into the same communication trap as Jonathan, so
let's be more explicit.

Your commit message first tells me you are going to sanitize something,
but starts out with examples of leaving the string unchanged.  Then it
continues with only the '>>' examples.

Today, and being someone who on average reads about half the mail that
comes through here, I know that this relates to the blame -e '>>' bug.
So today, I am wondering from the commit message why you narrowly focus
on that bug.  But you don't!  It's just that the commit message
insinuates it.

In a year, your reader (and bear in mind that this may very well be
yourself, at least if your memory is as good as mine) will wonder what
was so damn special about that '>>' string that it needs a specific fix
to send-email.

I see that you wrote in another thread:

> I have to write a peer-reviewed essay with an introduction for the
> people that are not familiar with the code in each of the patches

I'm not sure you meant it that literally, but the whole *point* is that
the message is for people who are not familiar with the code.  After
all, if I knew that your code did the right thing in the right way, I
would not be bothering with reading the message.  Today, I would just
send an Acked-by instead.  In a year, I'd scroll down for another
potential culprit for the bug I'm hunting.

What's especially striking me about your proposed messages of late: they
leave me with more open questions than I started with.  I tried to show
this above.  I'm not sure whether other contributors are better at
answering questions, or just better at not touching any topics that
might raise them.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Felipe Contreras]

On Sun, Feb 5, 2012 at 11:51 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 2012/2/5 Thomas Rast <trast@inf.ethz.ch>:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>   'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
>>>>   '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
>>>>   'foo@bar.com' -> 'foo@bar.com'
>>>>   '<foo@bar.com>' -> 'foo@bar.com'
>>>>   'Foo Bar' -> 'Foo Bar'
>>>
>>> Am I the only one who stared at this for ten seconds, only to then
>>> realize that there is no sanitizing whatsoever going on here?
>>
>> There is: '<foo@bar.com>' -> 'foo@bar.com'
>
> Indeed.
>
> I still feel cheated as a reader though, you showed me four examples of
> no change but let me figure that on my own.
>
>>>>   'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>>>>   '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>>>>   '<foo@bar.com>>' -> 'foo@bar.com'
>>>
>>> All of these are the same underlying issue.  Does your patch fix any
>>> other malformed addresses, or just this particular type?
>>
>> See above.
>
> Ok, I see I am falling into the same communication trap as Jonathan, so
> let's be more explicit.
>
> Your commit message first tells me you are going to sanitize something,
> but starts out with examples of leaving the string unchanged.  Then it
> continues with only the '>>' examples.

Which is why I added a paragraph to explain them. What is unclear about?

---
According to commit 155197e[1], the "prhase" should not be empty, so
if it is, remove the
<>. Extra characters after the first ">" are ignored.
---

> Today, and being someone who on average reads about half the mail that
> comes through here, I know that this relates to the blame -e '>>' bug.
> So today, I am wondering from the commit message why you narrowly focus
> on that bug.  But you don't!  It's just that the commit message
> insinuates it.

The summary explains the purpose of the patch "add extra safety in
address sanitation" (should fix those typos though).

> In a year, your reader (and bear in mind that this may very well be
> yourself, at least if your memory is as good as mine) will wonder what
> was so damn special about that '>>' string that it needs a specific fix
> to send-email.

It doesn't matter, could be "<foo@bar.com> err blop", or any number of
other malformed strings.

> I see that you wrote in another thread:
>
>> I have to write a peer-reviewed essay with an introduction for the
>> people that are not familiar with the code in each of the patches
>
> I'm not sure you meant it that literally, but the whole *point* is that
> the message is for people who are not familiar with the code.  After
> all, if I knew that your code did the right thing in the right way, I
> would not be bothering with reading the message.  Today, I would just
> send an Acked-by instead.  In a year, I'd scroll down for another
> potential culprit for the bug I'm hunting.

You are assuming too much. In this case, the code is clear and doesn't
need explaining. I am talking about other cases which in my mind are
akin to explaining what is $recipient, $recipient_name, and what does
sanitize_address does, and why the if case for is_rfc2047_quoted is
modified. IMO that's overkill.

If you have some suggestion about how to improve the commit message, I
would be glad to listen to them, as in this case, I do believe the
changes merit some clear explanation. Not all patches do, though.

> What's especially striking me about your proposed messages of late: they
> leave me with more open questions than I started with.  I tried to show
> this above.  I'm not sure whether other contributors are better at
> answering questions, or just better at not touching any topics that
> might raise them.

Again, what is not clear about:

---
Basically, we try to check that the address is in the form of
"Name <email>", and if not, assume it's "email". According to commit
155197e[1], the "prhase" should not be empty, so if it is, remove the
<>. Extra characters after the first ">" are ignored.
---

To me that explains what the patch is trying to do: "add extra safety
in address sanitation".

Anyway, it seems people don't care if 'git send-email' attempts to
send random garbage regardless, so I'm not going to pursue this patch.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Anyway, it seems people don't care if 'git send-email' attempts to
> send random garbage regardless, so I'm not going to pursue this patch.

I actually think people _do_ care, and that is the _only_ reason you are
getting review comments.

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Am I the only one who stared at this for ten seconds, only to then
> realize that there is no sanitizing whatsoever going on here?
>
>>   'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>>   '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>>   '<foo@bar.com>>' -> 'foo@bar.com'
>
> All of these are the same underlying issue.  Does your patch fix any
> other malformed addresses, or just this particular type?

Just this particular type, as long as the code handles it correctly, would
be better than nothing.

On the recieving end in mailinfo, I think we also support

	gitster@pobox.com (Junio C Hamano)

although it does not seem to be used by many people these days (the only
one I can remember seeing on this list was merlyn), so we may want to be a
bit more consistent between sending and receiving end, though.

[PATCH] send-email: add extra safetly in address sanitazion

[Felipe Contreras]

Otherwise, 'git send-email' would be happy to do:

 % git send-email --to '<foo@bar.com>>'

And use '<foo@bar.com>>' in the headers.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ef30c55..b8bf014 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -889,7 +889,7 @@ sub is_rfc2047_quoted {
 # use the simplest quoting being able to handle the recipient
 sub sanitize_address {
 	my ($recipient) = @_;
-	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
+	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*?>)/);
 
 	if (not $recipient_name) {
 		return $recipient;
-- 
1.7.9

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Felipe Contreras]

On Sat, Feb 4, 2012 at 5:10 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Otherwise, 'git send-email' would be happy to do:
>
>  % git send-email --to '<foo@bar.com>>'
>
> And use '<foo@bar.com>>' in the headers.

Er, actually that's not correct: '<foo@bar.com>>' will remain the
same, but 'Foo <foo@bar.com>>' will be sanitized.

-- 
Felipe Contreras

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sat, Feb 4, 2012 at 5:10 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Otherwise, 'git send-email' would be happy to do:
>>
>>  % git send-email --to '<foo@bar.com>>'
>>
>> And use '<foo@bar.com>>' in the headers.
>
> Er, actually that's not correct: '<foo@bar.com>>' will remain the
> same, but 'Foo <foo@bar.com>>' will be sanitized.

I suspect that this "Er" is merely a sympotom of a larger issue in the
approach taken by this patch.  The code takes a potentially malformed
input, and applies a rewrite logic without telling the user what it is
doing.  If the rewrite logic is perfect, that may be OK, but if not, the
logic to rewrite may or may not trigger, or when it triggers it may or may
not produce a correct result, and it all depends on the nature of breakage
in the input.

Wouldn't a better approach to detect problem on the input side and reject
a wrong one by erroring out, so that the user has a chance to fix?

Re: [PATCH] send-email: add extra safetly in address sanitazion

[Felipe Contreras]

On Sun, Feb 5, 2012 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sat, Feb 4, 2012 at 5:10 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> Otherwise, 'git send-email' would be happy to do:
>>>
>>>  % git send-email --to '<foo@bar.com>>'
>>>
>>> And use '<foo@bar.com>>' in the headers.
>>
>> Er, actually that's not correct: '<foo@bar.com>>' will remain the
>> same, but 'Foo <foo@bar.com>>' will be sanitized.
>
> I suspect that this "Er" is merely a sympotom of a larger issue in the
> approach taken by this patch.  The code takes a potentially malformed
> input, and applies a rewrite logic without telling the user what it is
> doing.

That's what the function is doing already: sanitizing the address.

> Wouldn't a better approach to detect problem on the input side and reject
> a wrong one by erroring out, so that the user has a chance to fix?

Perhaps, but the code is not prepared for that. Anyway, feel free to
drop it, I am not interested in pursing this.

Cheers.

-- 
Felipe Contreras