jbrandt at bestpractical
Jul 26, 2012, 6:18 AM
Post #1 of 1
(97 views)
Permalink
|
|
rt branch, 4.0/warn-on-non-ASCII-email-headers, created. rt-4.0.6-251-g1ba0f1a
|
|
The branch, 4.0/warn-on-non-ASCII-email-headers has been created
at 1ba0f1a96e4fb7f01bd896ebd1b56d840c2e2116 (commit)
- Log -----------------------------------------------------------------
commit 220063a15cb1bcb25bee21aac709fa46869dd5a7
Author: Jim Brandt <jbrandt [at] bestpractical>
Date: Fri Jul 13 09:43:28 2012 -0400
Avoid warnings with undef $From
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 2594e99..43b501d 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -149,6 +149,9 @@ sub CheckForSuspiciousSender {
my ( $From, $junk ) = ParseSenderAddressFromHead($head);
+ # If unparseable (non-ASCII), $From can come back undef
+ return undef if not defined $From;
+
if ( ( $From =~ /^mailer-daemon\@/i )
or ( $From =~ /^postmaster\@/i )
or ( $From eq "" ))
commit 1ba0f1a96e4fb7f01bd896ebd1b56d840c2e2116
Author: Jim Brandt <jbrandt [at] bestpractical>
Date: Fri Jul 13 13:49:40 2012 -0400
Improve logging for failed sender parsing in email
Starting with version 1.893, Email::Address returns undef
if passed non-ASCII email addresses to parse.
Track such email parsing failures in ParseSenderAddressFromHead and
pass them back as an optional third parameter. This allows the
caller to report the error if desired. Failure to parse one message
will generate an error, but the function can still find and return
a valid address from another header. This function is
called from several places and returning the error rather than reporting
avoids multiple warnings from parsing the same emails.
ParseSenderAddressFromHead still reports a failure to find a sender
in any header, since that is an error. That message will be reported
multiple times, one for each call to the function.
Update MailFrom.pm's GetCurrentUser to report the parse failures
as a warning. These parse failures are reported even if a sender
was found in another header. Also update the existing error in
GetCurrentUser to indicate that a parsing failure can also
prevent finding a valid sender.
diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 43b501d..b4d13af 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -1071,23 +1071,37 @@ sub ParseCcAddressesFromHead {
=head2 ParseSenderAddressFromHead HEAD
-Takes a MIME::Header object. Returns a tuple: (user [at] hos, friendly name)
-of the From (evaluated in order of Reply-To:, From:, Sender)
+Takes a MIME::Header object. Returns (user [at] hos, friendly name, error msg)
+where the first two values are the From (evaluated in order of
+Reply-To:, From:, Sender).
+
+An error msg may be returned even when a Sender value is found, since it
+could be a parse error for another sender field. In this case, the
+error isn't fatal, but may be useful to investigate the parse failure.
=cut
sub ParseSenderAddressFromHead {
my $head = shift;
+ my @sender_headers = ('Reply-To', 'From', 'Sender');
+ my $error_msg; # Accumulate any errors
#Figure out who's sending this message.
- foreach my $header ('Reply-To', 'From', 'Sender') {
+ foreach my $header ( @sender_headers ) {
my $addr_line = $head->get($header) || next;
my ($addr, $name) = ParseAddressFromHeader( $addr_line );
# only return if the address is not empty
- return ($addr, $name) if $addr;
+ return ($addr, $name, $error_msg) if $addr;
+
+ chomp $addr_line;
+ $error_msg = "Failed to parse sender using " if not $error_msg;
+ $error_msg .= "$header: $addr_line";
}
- return (undef, undef);
+ $RT::Logger->warning("Couldn't find a sender in "
+ . join(', ', @sender_headers) );
+
+ return (undef, undef, $error_msg);
}
=head2 ParseErrorsToAddressFromHead HEAD
diff --git a/lib/RT/Interface/Email/Auth/MailFrom.pm b/lib/RT/Interface/Email/Auth/MailFrom.pm
index e733bda..e7fcd97 100644
--- a/lib/RT/Interface/Email/Auth/MailFrom.pm
+++ b/lib/RT/Interface/Email/Auth/MailFrom.pm
@@ -66,9 +66,11 @@ sub GetCurrentUser {
# We don't need to do any external lookups
- my ( $Address, $Name ) = ParseSenderAddressFromHead( $args{'Message'}->head );
+ my ( $Address, $Name, $Error_msg ) = ParseSenderAddressFromHead( $args{'Message'}->head );
+ $RT::Logger->warning($Error_msg) if $Error_msg;
+
unless ( $Address ) {
- $RT::Logger->error("Couldn't find sender's address");
+ $RT::Logger->error("Couldn't parse or find sender's address");
return ( $args{'CurrentUser'}, -1 );
}
diff --git a/t/mail/header-characters.t b/t/mail/header-characters.t
new file mode 100644
index 0000000..ece9c5d
--- /dev/null
+++ b/t/mail/header-characters.t
@@ -0,0 +1,90 @@
+use strict;
+use warnings;
+
+use RT::Test tests => 12;
+use Test::Warn;
+use utf8;
+use Encode;
+
+my ($baseurl, $m) = RT::Test->started_ok;
+
+diag "Testing non-ASCII in From: header";
+SKIP:{
+ skip "Test requires Email::Address 1.893 or later, "
+ . "you have $Email::Address::VERSION", 3,
+ if $Email::Address::VERSION < 1.893;
+
+ my $mail = encode( 'iso-8859-1', <<'.' );
+From: René@example.com>
+Reply-To: =?iso-8859-1?Q?Ren=E9?= <René@example.com>
+Subject: testing non-ASCII From
+Content-Type: text/plain; charset=iso-8859-1
+
+here's some content
+.
+
+ my ($status, $id);
+ warnings_like { ( $status, $id ) = RT::Test->send_via_mailgate($mail) }
+ [.qr/Couldn't find a sender in/,
+ qr/Couldn't find a sender in/,
+ qr/Failed to parse sender using/,
+ qr/Couldn't parse or find sender's address/
+ ],
+ 'Got parse error for non-ASCII in From';
+ is( $status >> 8, 0, "The mail gateway exited normally" );
+ TODO: {
+ local $TODO = "Currently don't handle non-ASCII for sender";
+ ok( $id, "Created ticket" );
+ }
+}
+
+diag "Testing iso-8859-1 encoded non-ASCII in From: header";
+SKIP:{
+ skip "Test requires Email::Address 1.893 or later, "
+ . "you have $Email::Address::VERSION", 3,
+ if $Email::Address::VERSION < 1.893;
+
+ my $mail = encode( 'iso-8859-1', <<'.' );
+From: =?iso-8859-1?Q?Ren=E9?= <René@example.com>
+Reply-To: =?iso-8859-1?Q?Ren=E9?= <René@example.com>
+Subject: testing non-ASCII From
+Content-Type: text/plain; charset=iso-8859-1
+
+here's some content
+.
+
+ my ($status, $id);
+ warnings_like { ( $status, $id ) = RT::Test->send_via_mailgate($mail) }
+ [.qr/Couldn't find a sender in/,
+ qr/Couldn't find a sender in/,
+ qr/Failed to parse sender using/,
+ qr/Couldn't parse or find sender's address/
+ ],
+ 'Got parse error for iso-8859-1 in From';
+ is( $status >> 8, 0, "The mail gateway exited normally" );
+ TODO: {
+ local $TODO = "Currently don't handle non-ASCII in sender";
+ ok( $id, "Created ticket" );
+ }
+}
+
+diag "No sender";
+{
+ my $mail = <<'.';
+To: rt [at] example
+Subject: testing non-ASCII From
+Content-Type: text/plain; charset=iso-8859-1
+
+here's some content
+.
+
+ my ($status, $id);
+ warnings_like { ( $status, $id ) = RT::Test->send_via_mailgate($mail) }
+ [.qr/Couldn't find a sender in/,
+ qr/Couldn't find a sender in/,
+ qr/Couldn't parse or find sender's address/
+ ],
+ 'Got parse error with no sender fields';
+ is( $status >> 8, 0, "The mail gateway exited normally" );
+ ok( !$id, "No ticket created" );
+}
-----------------------------------------------------------------------
|
