# git-send-email.perl defect: address missing trailing > accepted 6 messages from 2009-10-20 to 2009-10-20. Participants: Joe Perches, Erik Faye-Lund, Junio C Hamano. Thread: https://gitlist.dev/t/21301 ## Joe Perches, 2009-10-20 22:12 Subject: git-send-email.perl defect: address missing trailing > accepted Message-ID: <1256076767.2029.59.camel@Joe-Laptop.home> URL: https://gitlist.dev/e/1256076767.2029.59.camel%40Joe-Laptop.home ``` I typo cut/pasted an invalid email address, neglecting to copy the trailing ">". was: "Name " Anyone have suggestions on how to get git-send-email.perl to notify and abort sending on more invalid address styles? ``` ## Erik Faye-Lund, 2009-10-20 22:29 Subject: Re: git-send-email.perl defect: address missing trailing > accepted Message-ID: <40aa078e0910201529m338ef3d1o4fa1a31c3dcc2a20@mail.gmail.com> URL: https://gitlist.dev/e/40aa078e0910201529m338ef3d1o4fa1a31c3dcc2a20%40mail.gmail.com In-Reply-To: <1256076767.2029.59.camel@Joe-Laptop.home> ``` On Wed, Oct 21, 2009 at 12:12 AM, Joe Perches wrote: > I typo cut/pasted an invalid email address, > neglecting to copy the trailing ">". > > was:    "Name needed: "Name " > > Anyone have suggestions on how to get > git-send-email.perl to notify and abort > sending on more invalid address styles? Something along these lines? Of course, the error message is, uhm, less than helpful :) --->8--- diff --git a/git-send-email.perl b/git-send-email.perl index f5ba4e7..83f5e80 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -787,6 +787,10 @@ sub is_rfc2047_quoted { sub sanitize_address { my ($recipient) = @_; + if ($recipient =~ m/.*<[^>]*$/) { + die "EEK!" + } + my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/); if (not $recipient_name) { -- Erik "kusma" Faye-Lund ``` ## Joe Perches, 2009-10-20 22:48 Subject: Re: git-send-email.perl defect: address missing trailing > accepted Message-ID: <1256078917.2029.77.camel@Joe-Laptop.home> URL: https://gitlist.dev/e/1256078917.2029.77.camel%40Joe-Laptop.home In-Reply-To: <40aa078e0910201529m338ef3d1o4fa1a31c3dcc2a20@mail.gmail.com> ``` On Wed, 2009-10-21 at 00:29 +0200, Erik Faye-Lund wrote: > On Wed, Oct 21, 2009 at 12:12 AM, Joe Perches wrote: > > I typo cut/pasted an invalid email address, > > neglecting to copy the trailing ">". > > was: "Name > needed: "Name " > > Anyone have suggestions on how to get > > git-send-email.perl to notify and abort > > sending on more invalid address styles? > > Something along these lines? Of course, the error message is, uhm, > less than helpful :) > > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -787,6 +787,10 @@ sub is_rfc2047_quoted { > sub sanitize_address > { > my ($recipient) = @_; > + if ($recipient =~ m/.*<[^>]*$/) { > + die "EEK!" > + } > + Maybe this? Seems to work. diff --git a/git-send-email.perl b/git-send-email.perl index a0279de..52ddd9e 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -374,15 +374,18 @@ my ($repoauthor, $repocommitter); # Verify the user input foreach my $entry (@to) { - die "Comma in --to entry: $entry'\n" unless $entry !~ m/,/; + die "Comma in --to entry: '$entry'\n" unless $entry !~ m/,/; + die "Invalid --to entry: '$entry'\n" unless $entry !~ m/.*<[^>]*$/; } foreach my $entry (@initial_cc) { - die "Comma in --cc entry: $entry'\n" unless $entry !~ m/,/; + die "Comma in --cc entry: '$entry'\n" unless $entry !~ m/,/; + die "Invalid --cc entry: '$entry'\n" unless $entry !~ m/.*<[^>]*$/; } foreach my $entry (@bcclist) { - die "Comma in --bcclist entry: $entry'\n" unless $entry !~ m/,/; + die "Comma in --bcclist entry: '$entry'\n" unless $entry !~ m/,/; + die "Invalid --bcclist entry: '$entry'\n" unless $entry !~ m/.*<[^>]*$/; } sub parse_address_line { ``` ## Erik Faye-Lund, 2009-10-20 22:56 Subject: Re: git-send-email.perl defect: address missing trailing > accepted Message-ID: <40aa078e0910201556h4757dbbdn853be4dd8aa920d0@mail.gmail.com> URL: https://gitlist.dev/e/40aa078e0910201556h4757dbbdn853be4dd8aa920d0%40mail.gmail.com In-Reply-To: <1256078917.2029.77.camel@Joe-Laptop.home> ``` On Wed, Oct 21, 2009 at 12:48 AM, Joe Perches wrote: >> Something along these lines? Of course, the error message is, uhm, >> less than helpful :) > > Maybe this?  Seems to work. > Didn't my version work for you? It worked for me. I find it a bit cleaner to make it a part of the address-sanitizion, since that needs to be performed for all addresses. I might miss something vital, though. I don't really speak perl all that well ;) -- Erik "kusma" Faye-Lund ``` ## Joe Perches, 2009-10-20 23:00 Subject: Re: git-send-email.perl defect: address missing trailing > accepted Message-ID: <1256079633.2029.81.camel@Joe-Laptop.home> URL: https://gitlist.dev/e/1256079633.2029.81.camel%40Joe-Laptop.home In-Reply-To: <40aa078e0910201556h4757dbbdn853be4dd8aa920d0@mail.gmail.com> ``` On Wed, 2009-10-21 at 00:56 +0200, Erik Faye-Lund wrote: > Didn't my version work for you? It worked for me. Hi Erik. It worked, but an unexplained die isn't great, so I put it where the other validations are done. It seems that the regex for address validation isn't very good and perhaps there could/should be a stronger validation done for each address entered. cheers, Joe ``` ## Junio C Hamano, 2009-10-20 23:28 Subject: Re: git-send-email.perl defect: address missing trailing > accepted Message-ID: <7vy6n5k5l0.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6n5k5l0.fsf%40alter.siamese.dyndns.org In-Reply-To: <1256079633.2029.81.camel@Joe-Laptop.home> ``` Joe Perches writes: > It seems that the regex for address validation > isn't very good and perhaps there could/should > be a stronger validation done for each address > entered. The existing ones are actually already harmful. It would trigger on a valid addresses like this, wouldn't it? To: "Hamano, Jun" It is worse than that. The "# Verify the user input" block is in a wrong place in the codepath. After @to goes through this bogus "verification" step, it then is given to expand_aliases(), which may expand to real addresses. And then they pass through sanitize_address() before getting used. Three implications that come from this wrong code structure are: (1) The stricter checks you added on top of the existing bogus verification step may prevent @to to reach expand_aliases() step, even if the tokens in @to may expand to correct addresses by this expand_aliases() step if they were allowed to reach here; (2) The result from expand_aliases() is never checked. (3) The result from sanitize_address() is not checked either. ```