2012/3/20 Lukas Fleischer <[email protected]> > On Tue, Mar 20, 2012 at 12:06:03PM +0100, Ike Devolder wrote: > > 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. "[email protected]" 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 > > To be honest, I would prefer the first version (the one that checks > MX/A/AAAA records only and doesn't replace the filter_var() check) of > your patch as an intermediate. This is "good enough" for now - at least > until someone steps up and implements email verification. You probably > noticed yourself that dealing with all SMTP corner cases is tricky, > error-prone and a pain to maintain; and, as I said before, this can be > bypassed just as easy as the MX record check. > > > > > > * 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: <[email protected]>\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 > > > i'll send a new patch
-- Ike
