Perl-Email-Project / Email-Valid

perl library to validate email addresses
19 stars 19 forks source link

Email addresses with spaces before the TLD return as valid #4

Closed justingit closed 10 years ago

justingit commented 10 years ago

Example (notice the space in the domain - hevanetSPACE.com),

perl -MEmail::Valid -e 'print (Email::Valid->address("maurice\@hevanet .com") ? "yes" : "no");'

prints,

yes 
justingit commented 10 years ago

Part of the problem is that Mail::Address munges that address you give it, removing any spaces in the domain:

#!/usr/bin/perl 
use Mail::Address; 
my $email = '"Someone" <foo@bar. com>';
my ($ma) = Mail::Address->parse( $email );
print $ma->address; 

prints,

foo@bar.com

That means you can't test for something simple, like a space in the email address, once it hits _valid_domain_parts()

justingit commented 10 years ago

Weirdly, Email::Address also behaves strange - it removes everything after the space,

#!/usr/bin/perl 
use Email::Address; 
my $email = '"Someone" <foo@bar. com>';
my ($ma) = Email::Address->parse( $email );
print $ma->address; 

prints,

foo@bar

For the original issue, I'm not sure how to fix it, since you allow both passing a Mail::Address object, or a string in the methods like, address(). If you pass a string, looks like you still make the Mail::Address object, to use all the helpful bits from Mail::Address.

rjbs commented 10 years ago

It's annoying, isn't it? Whitespace is actually allowed in many contexts here, for various values of "allowed."

The trick to know is that the return value of ->address is not a boolean, but the address's "canonical form."

my $valid = Email::Valid->address($input);
if ($valid) {
  $user->set_address( $valid );
} else {
  die "your address is trash"
}

Does that help?

justingit commented 10 years ago

It does, and I do see that you have this example in the docs, that's similar to what I'm reporting:

$addr = Email::Valid->address('Alfred Neuman <Neuman @ foo.bar>');
print "$addr\n"; # prints Neuman@foo.bar

I'm a little confused as why you would want something to modify what you give it, when your validating input, though. I'd rather it not, but instead tell me if what I give it is valid, or not, and optionally what's not valid - and then, let me do what I would like with that information. (There is the, _fudge method, but now I'm not sure how that would even work for your aol.com example, as wouldn't Mail::Address already performing that fudgin'? )

Consider the idea of auto-munging data I input on a form on a page via JavaScript. There are instances where I certainly want to cause harm to this implementer ;)

Coming back to this though, here's how this behavior can cause a subtle bug in a program. Say I have a list of email addresses I want to validate, before I add it to a list of known "good" in form addresses. It's very important that all these are unique, are Bad Things will happen:

#!/usr/bin/perl 

use Email::Valid;

# Say, I've already sorted out dupes: 
my @check_these = 
(
'one@foobar.com',
'another@foobar.com',
'another@foobar .com',
'two@foobar.com',
'blech'
); 

my @validated = (); 
my @invalid   = (); 

for(@check_these){ 
    my $address; 
    if($address = Email::Valid->address($_)) { 
        push(@validated, $address); 
    }
    else { 
        push(@invalid, $_); 
    } 
}

print "Good! Addresses:\n";
print "* $_\n" for @validated;

print "BAD! Addresses:\n";
print "* $_\n" for @invalid;

Will print out,

Good! Addresses:
* one@foobar.com
* another@foobar.com
* another@foobar.com
* two@foobar.com
BAD! Addresses:
* blech

So, now my unique list I had before is not unique, and the bad-in-form addresses aren't being reported.

Anyways, I appreciate you responding - I should have seen the example in the docs - my apologies for that. Thankfully, the workaround for my situation is as easy looking for a space in the address, before passing it to Email::Valid ;)

Cheers,

rjbs commented 10 years ago

Let's just say, "I didn't write this code to begin with."

It's not safe to fix all the API problems, I think, and it is more likely that I will write a non-drop-in replacement.

rjbs

justingit commented 10 years ago

Yup, I had a sneaking suspicion ;) I'll wrap this all up in a blog post or something,

justingit commented 10 years ago

Hilariously, looks like I reported this problem, way back in the day:

https://rt.cpan.org/Public/Bug/Display.html?id=24465#txn-378265

Oh well no worries - one line change in my stuff, and I get to remove a, "TODO:" block from my test suite. Thanks, TAP!