[PATCH 0/2] use Email::Address::XS if available

[PATCH 0/2] use Email::Address::XS if available

[Eric Wong]

Email::Address::XS is loaded by default on modern Email::MIME
(which we depend on), so we might as well use it.  1/2 also
improves the pure-Perl fallback to match optimal ::XS behavior.

Eric Wong (2):
  address: use comment as name if no phrase available
  address: use Email::Address::XS if available

 MANIFEST                     |  1 +
 lib/PublicInbox/Address.pm   | 34 ++++++++++---------
 lib/PublicInbox/AddressPP.pm | 38 +++++++++++++++++++++
 t/address.t                  | 66 ++++++++++++++++++++++--------------
 4 files changed, 98 insertions(+), 41 deletions(-)
 create mode 100644 lib/PublicInbox/AddressPP.pm

[PATCH 1/2] address: use comment as name if no phrase available

[Eric Wong]

Some users will set their From: headers in the form of:
"<user@example.com> (A U Thor)", where their name is in
the parenthesized comment.  Use that instead of the
email address, if available.
---
 lib/PublicInbox/Address.pm | 30 ++++++++++++++++++++----------
 t/address.t                |  7 ++++---
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/Address.pm b/lib/PublicInbox/Address.pm
index 46aa0da7..a58d1eff 100644
--- a/lib/PublicInbox/Address.pm
+++ b/lib/PublicInbox/Address.pm
@@ -13,16 +13,26 @@ sub emails {
 }
 
 sub names {
-	map {
-		tr/\r\n\t/ /;
-		s/\s*<([^<]+)\z//;
-		my $e = $1;
-		s/\A['"\s]*//;
-		s/['"\s]*\z//;
-		$e = $_ =~ /\S/ ? $_ : $e;
-		$e =~ s/\@\S+\z//;
-		$e;
-	} split(/\@+[\w\.\-]+>?\s*(?:\(.*?\))?(?:,\s*|\z)/, $_[0]);
+	my @p = split(/<?([^@<>]+)\@[\w\.\-]+>?\s*(\(.*?\))?(?:,\s*|\z)/,
+			$_[0]);
+	my @ret;
+	for (my $i = 0; $i <= $#p;) {
+		my $phrase = $p[$i++];
+		$phrase =~ tr/\r\n\t / /s;
+		$phrase =~ s/\A['"\s]*//;
+		$phrase =~ s/['"\s]*\z//;
+		my $user = $p[$i++] // '';
+		my $comment = $p[$i++] // '';
+		if ($phrase =~ /\S/) {
+			$phrase =~ s/\@\S+\z//;
+			push @ret, $phrase;
+		} elsif ($comment =~ /\A\((.*?)\)\z/) {
+			push @ret, $1;
+		} else {
+			push @ret, $user;
+		}
+	}
+	@ret;
 }
 
 1;
diff --git a/t/address.t b/t/address.t
index bea45daa..2a287102 100644
--- a/t/address.t
+++ b/t/address.t
@@ -14,8 +14,9 @@ is_deeply(['user@example.com'],
 	'comment after domain accepted before >');
 
 my @names = PublicInbox::Address::names(
-	'User <e@e>, e@e, "John A. Doe" <j@d>, <x@x>');
-is_deeply(['User', 'e', 'John A. Doe', 'x'], \@names,
+	'User <e@e>, e@e, "John A. Doe" <j@d>, <x@x>, <y@x> (xyz), '.
+	'U Ser <u@x> (do not use)');
+is_deeply(\@names, ['User', 'e', 'John A. Doe', 'x', 'xyz', 'U Ser'],
 	'name extraction works as expected');
 
 @names = PublicInbox::Address::names('"user@example.com" <user@example.com>');
@@ -25,7 +26,7 @@ is_deeply(['user'], \@names, 'address-as-name extraction works as expected');
 {
 	my $backwards = 'u@example.com (John Q. Public)';
 	@names = PublicInbox::Address::names($backwards);
-	is_deeply(\@names, ['u'], 'backwards name OK');
+	is_deeply(\@names, ['John Q. Public'], 'backwards name OK');
 	my @emails = PublicInbox::Address::emails($backwards);
 	is_deeply(\@emails, ['u@example.com'], 'backwards emails OK');
 }

[PATCH 2/2] address: use Email::Address::XS if available

[Eric Wong]

Email::Address::XS is a dependency of modern versions of Email::MIME,
so it's likely loaded and installed on newer systems, already;
and capable of handling more corner-cases than our pure-Perl
fallback.

We still fallback to the imperfect-but-good-enough-in-practice
pure-Perl code while avoiding the non-XS Email::Address (which
was susceptible to DoS attacks (CVE-2015-7686)).  We just need
to keep "git fast-import" happy.
---
 MANIFEST                     |  1 +
 lib/PublicInbox/Address.pm   | 44 ++++++++++-------------
 lib/PublicInbox/AddressPP.pm | 38 ++++++++++++++++++++
 t/address.t                  | 67 ++++++++++++++++++++++--------------
 4 files changed, 98 insertions(+), 52 deletions(-)
 create mode 100644 lib/PublicInbox/AddressPP.pm

diff --git a/MANIFEST b/MANIFEST
index 044bbfef..3e760f89 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -81,6 +81,7 @@ examples/unsubscribe.milter
 examples/unsubscribe.psgi
 examples/varnish-4.vcl
 lib/PublicInbox/Address.pm
+lib/PublicInbox/AddressPP.pm
 lib/PublicInbox/Admin.pm
 lib/PublicInbox/AdminEdit.pm
 lib/PublicInbox/AltId.pm
diff --git a/lib/PublicInbox/Address.pm b/lib/PublicInbox/Address.pm
index a58d1eff..433b36eb 100644
--- a/lib/PublicInbox/Address.pm
+++ b/lib/PublicInbox/Address.pm
@@ -4,35 +4,27 @@ package PublicInbox::Address;
 use strict;
 use warnings;
 
-# very loose regexes, here.  We don't need RFC-compliance,
-# just enough to make thing sanely displayable and pass to git
+sub xs_emails { map { $_->address() } parse_email_addresses($_[0]) }
 
-sub emails {
-	($_[0] =~ /([\w\.\+=\?"\(\)\-!#\$%&'\*\/\^\`\|\{\}~]+\@[\w\.\-\(\)]+)
-		(?:\s[^>]*)?>?\s*(?:\(.*?\))?(?:,\s*|\z)/gx)
+sub xs_names {
+	map {
+		my $n = $_->name;
+		$n = $_->user if $n eq $_->address;
+		$n;
+	} parse_email_addresses($_[0]);
 }
 
-sub names {
-	my @p = split(/<?([^@<>]+)\@[\w\.\-]+>?\s*(\(.*?\))?(?:,\s*|\z)/,
-			$_[0]);
-	my @ret;
-	for (my $i = 0; $i <= $#p;) {
-		my $phrase = $p[$i++];
-		$phrase =~ tr/\r\n\t / /s;
-		$phrase =~ s/\A['"\s]*//;
-		$phrase =~ s/['"\s]*\z//;
-		my $user = $p[$i++] // '';
-		my $comment = $p[$i++] // '';
-		if ($phrase =~ /\S/) {
-			$phrase =~ s/\@\S+\z//;
-			push @ret, $phrase;
-		} elsif ($comment =~ /\A\((.*?)\)\z/) {
-			push @ret, $1;
-		} else {
-			push @ret, $user;
-		}
-	}
-	@ret;
+eval {
+	require Email::Address::XS;
+	Email::Address::XS->import(qw(parse_email_addresses));
+	*emails = \&xs_emails;
+	*names = \&xs_names;
+};
+
+if ($@) {
+	require PublicInbox::AddressPP;
+	*emails = \&PublicInbox::AddressPP::emails;
+	*names = \&PublicInbox::AddressPP::names;
 }
 
 1;
diff --git a/lib/PublicInbox/AddressPP.pm b/lib/PublicInbox/AddressPP.pm
new file mode 100644
index 00000000..cd7aedb9
--- /dev/null
+++ b/lib/PublicInbox/AddressPP.pm
@@ -0,0 +1,38 @@
+# Copyright (C) 2016-2019 all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+package PublicInbox::AddressPP;
+use strict;
+
+# very loose regexes, here.  We don't need RFC-compliance,
+# just enough to make thing sanely displayable and pass to git
+# We favor Email::Address::XS for conformance if available
+
+sub emails {
+	($_[0] =~ /([\w\.\+=\?"\(\)\-!#\$%&'\*\/\^\`\|\{\}~]+\@[\w\.\-\(\)]+)
+		(?:\s[^>]*)?>?\s*(?:\(.*?\))?(?:,\s*|\z)/gx)
+}
+
+sub names {
+	my @p = split(/<?([^@<>]+)\@[\w\.\-]+>?\s*(\(.*?\))?(?:,\s*|\z)/,
+			$_[0]);
+	my @ret;
+	for (my $i = 0; $i <= $#p;) {
+		my $phrase = $p[$i++];
+		$phrase =~ tr/\r\n\t / /s;
+		$phrase =~ s/\A['"\s]*//;
+		$phrase =~ s/['"\s]*\z//;
+		my $user = $p[$i++] // '';
+		my $comment = $p[$i++] // '';
+		if ($phrase =~ /\S/) {
+			$phrase =~ s/\@\S+\z//;
+			push @ret, $phrase;
+		} elsif ($comment =~ /\A\((.*?)\)\z/) {
+			push @ret, $1;
+		} else {
+			push @ret, $user;
+		}
+	}
+	@ret;
+}
+
+1;
diff --git a/t/address.t b/t/address.t
index 2a287102..e7c0d6a8 100644
--- a/t/address.t
+++ b/t/address.t
@@ -5,34 +5,49 @@ use warnings;
 use Test::More;
 use_ok 'PublicInbox::Address';
 
-is_deeply([qw(e@example.com e@example.org)],
-	[PublicInbox::Address::emails('User <e@example.com>, e@example.org')],
-	'address extraction works as expected');
-
-is_deeply(['user@example.com'],
-	[PublicInbox::Address::emails('<user@example.com (Comment)>')],
-	'comment after domain accepted before >');
-
-my @names = PublicInbox::Address::names(
-	'User <e@e>, e@e, "John A. Doe" <j@d>, <x@x>, <y@x> (xyz), '.
-	'U Ser <u@x> (do not use)');
-is_deeply(\@names, ['User', 'e', 'John A. Doe', 'x', 'xyz', 'U Ser'],
-	'name extraction works as expected');
-
-@names = PublicInbox::Address::names('"user@example.com" <user@example.com>');
-is_deeply(['user'], \@names, 'address-as-name extraction works as expected');
-
-
-{
-	my $backwards = 'u@example.com (John Q. Public)';
-	@names = PublicInbox::Address::names($backwards);
-	is_deeply(\@names, ['John Q. Public'], 'backwards name OK');
-	my @emails = PublicInbox::Address::emails($backwards);
-	is_deeply(\@emails, ['u@example.com'], 'backwards emails OK');
+sub test_pkg {
+	my ($pkg) = @_;
+	my $emails = \&{"${pkg}::emails"};
+	my $names = \&{"${pkg}::names"};
+
+	is_deeply([qw(e@example.com e@example.org)],
+		[$emails->('User <e@example.com>, e@example.org')],
+		'address extraction works as expected');
+
+	is_deeply(['user@example.com'],
+		[$emails->('<user@example.com (Comment)>')],
+		'comment after domain accepted before >');
+
+	my @names = $names->(
+		'User <e@e>, e@e, "John A. Doe" <j@d>, <x@x>, <y@x> (xyz), '.
+		'U Ser <u@x> (do not use)');
+	is_deeply(\@names, ['User', 'e', 'John A. Doe', 'x', 'xyz', 'U Ser'],
+		'name extraction works as expected');
+
+	@names = $names->('"user@example.com" <user@example.com>');
+	is_deeply(['user'], \@names,
+		'address-as-name extraction works as expected');
+
+	{
+		my $backwards = 'u@example.com (John Q. Public)';
+		@names = $names->($backwards);
+		is_deeply(\@names, ['John Q. Public'], 'backwards name OK');
+		my @emails = $emails->($backwards);
+		is_deeply(\@emails, ['u@example.com'], 'backwards emails OK');
+	}
+
+	@names = $names->('"Quote Unneeded" <user@example.com>');
+	is_deeply(['Quote Unneeded'], \@names, 'extra quotes dropped');
 }
 
+test_pkg('PublicInbox::Address');
 
-@names = PublicInbox::Address::names('"Quote Unneeded" <user@example.com>');
-is_deeply(['Quote Unneeded'], \@names, 'extra quotes dropped');
+SKIP: {
+	if ($INC{'PublicInbox/AddressPP.pm'}) {
+		skip 'Email::Address::XS missing', 8;
+	}
+	use_ok 'PublicInbox::AddressPP';
+	test_pkg('PublicInbox::AddressPP');
+}
 
 done_testing;

[PATCH 3/2] address: explicitly reject local-only addresses

[Eric Wong]

Apparently, neither our previous address parsing code nor
Email::Address::XS recognizes local, username-only addresses
in the form of <username> (without "@host").  Without
this change, Email::Address::XS->address would return
"undef", so we need to filter it out via "grep { defined }"

It seems the cases where users email each other on the same
machine is small and public-inbox won't be able to index
addresses for those cases...  Oh well :/
---
 lib/PublicInbox/Address.pm | 9 ++++++---
 t/address.t                | 5 +++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/Address.pm b/lib/PublicInbox/Address.pm
index 433b36eb..c23a5d62 100644
--- a/lib/PublicInbox/Address.pm
+++ b/lib/PublicInbox/Address.pm
@@ -4,12 +4,15 @@ package PublicInbox::Address;
 use strict;
 use warnings;
 
-sub xs_emails { map { $_->address() } parse_email_addresses($_[0]) }
+sub xs_emails {
+	grep { defined } map { $_->address() } parse_email_addresses($_[0])
+}
 
 sub xs_names {
-	map {
+	grep { defined } map {
 		my $n = $_->name;
-		$n = $_->user if $n eq $_->address;
+		my $addr = $_->address;
+		$n = $_->user if defined($addr) && $n eq $addr;
 		$n;
 	} parse_email_addresses($_[0]);
 }
diff --git a/t/address.t b/t/address.t
index e7c0d6a8..1f20702a 100644
--- a/t/address.t
+++ b/t/address.t
@@ -38,6 +38,11 @@ sub test_pkg {
 
 	@names = $names->('"Quote Unneeded" <user@example.com>');
 	is_deeply(['Quote Unneeded'], \@names, 'extra quotes dropped');
+
+	my @emails = $emails->('Local User <user>');
+	is_deeply([], \@emails , 'no address for local address');
+	@names = $emails->('Local User <user>');
+	is_deeply([], \@names, 'no address, no name');
 }
 
 test_pkg('PublicInbox::Address');