[aur-dev] [PATCH] validate email and fully check existence of it

[Ike Devolder]

On Tue, Mar 20, 2012 at 11:31:32AM +0100, Lukas Fleischer wrote:
> On Tue, Mar 20, 2012 at 10:36:11AM +0100, BlackEagle wrote:
> > - check if the format is valid
> > - go and connect to the smtp server of the given domain and verify if
> >   the given email exists there
> 
> I'm still not convinced that this is the right way to go. This check is
> quite technical and looks error-prone, while it still allows anyone to
> use arbitrary mail addresses (e.g. "archlinux at cryptocrack.de" or random
> mail addresses from an email address list created by a crawler).
> 
> As I said on IRC, there's probably only two (proper) ways to fix this:
> 
> * Send verification mails.

is the best solution, but this patch could be an intermediate until the
verification stuff is in place

> * Use captchas.

this is only annoying the user, and rendered useless when using
verification mail.

> 
> I'll still add some comments to your patch below...
> 
> > ---
> >  web/lib/aur.inc.php |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 74 insertions(+), 1 deletions(-)
> > 
> > diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
> > index c662b80..3fc0a14 100644
> > --- a/web/lib/aur.inc.php
> > +++ b/web/lib/aur.inc.php
> > @@ -80,7 +80,80 @@ function check_sid($dbh=NULL) {
> >  # verify that an email address looks like it is legitimate
> >  #
> >  function valid_email($addy) {
> > -	return (filter_var($addy, FILTER_VALIDATE_EMAIL) !== false);
> > +    // check against RFC 3696
> 
> Please use tabs, not spaces, for indentation.

sorry was on another configuration since i have to use spaces there

> 
> > +    if(filter_var($addy, FILTER_VALIDATE_EMAIL) === false) {
> > +        return false;
> > +    }
> > +
> > +    // check dns for mx, a, aaaa records
> > +    list($local, $domain) = explode('@', $addy);
> > +    if(! (checkdnsrr($domain, 'MX') || checkdnsrr($domain, 'A') || checkdnsrr($domain, 'AAAA'))) {
> > +        return false;
> > +    }
> 
> This check could be dropped if we actually decided for this complicated
> method. We check for MX records below.
> 
> > +
> > +    // get mx records and check full email address
> > +    $mxlist = array();
> > +    $mxweight = array();
> > +    getmxrr($domain, $mxlist, $mxweight);
> > +    $mx = array_combine($mxweight, $mxlist);
> > +    ksort($mx);
> 
> You checked for A and AAAA records above but don't add them to the "$mx"
> array here?
> 
> > +
> > +    //smtp_test_email($addy, current($mx));
> 
> Please strip commented code from patches.

sorry

> 
> > +    foreach($mx as $prio => $mxsrv) {
> > +        if(smtp_test_email($addy, $mxsrv) === true) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +# verify that an email address exists on the smtp server
> > +#
> > +function smtp_test_email($addy, $mxsrv) {
> > +    if(($smtp = fsockopen($mxsrv, 25)) === false) {
> > +        return false;
> > +    }
> > +
> > +    if(intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp))) !== 220) {
> 
> Better use substr() here:
> 
>     if (intval(substr(fgets($smtp), 0, 3)) !== 220) {
>     [...]
> 
> Also, what about other error codes, such as 421?

i think we can consider 421 as an error since we want to verify the
complete email address

> 
> > +        smtp_close($smtp);
> > +        return false;
> > +    }
> > +
> > +    fwrite($smtp, "HELO $mxsrv\r\n");
> > +    if(intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp))) !== 250) {
> 
> Same here.
> 
> > +        smtp_close($smtp);
> > +        return false;
> > +    }
> > +
> > +    fwrite($smtp, "MAIL FROM: <mailtest at archlinux.org>\r\n");
> > +    if(intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp))) !== 250) {
> 
> Same here.
> 
> > +        smtp_close($smtp);
> > +        return false;
> > +    }
> > +
> > +    fwrite($smtp, "RCPT TO: <$addy>\r\n");
> > +    $code = intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp)));
> 
> Same here.
> 
> > +    /**
> > +     * 250 = success
> > +     * 451 or 452 = address got greylisted but another error occured
> > +     *              so assume ok
> > +     */
> > +    if($code !== 250 && $code !== 451 && $code !== 452) {
> 
> What about 251? What about 450?

251 should be taken in
then 551 comes in the picture and both would require this function to be
run with the returned <forward-path>
when we follow the forward-path stuff then we could run out of time if
the following ones are also forwarding

450 is considered an error so i would let it fail there

> 
> This is what I don't like about this patch... There's a dozens of corner
> cases to consider and I don't think this is the right place to check for
> these.
> 
> > +        smtp_close($smtp);
> > +        return false;
> > +    }
> > +
> > +    smtp_close($smtp);
> > +    return true;
> > +}
> > +
> > +# close smtp conneciton
> > +#
> > +function smtp_close(&$smtp) {
> > +    fwrite($smtp, "RSET\r\n");
> > +    fwrite($smtp, "QUIT\r\n");
> > +    fclose($smtp);
> >  }
> >  
> >  # a new seed value for mt_srand()
> > -- 
> > 1.7.9.1

-- 
Ike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://mailman.archlinux.org/pipermail/aur-dev/attachments/20120320/9293deaa/attachment.asc>