Contacting the mail server to verify that the mailbox exists seems a little excessive. I think just checking for an MX/A record is good in catching some obviously fake addresses and reliable enough. Maybe we should just stop there. On Tue, Mar 20, 2012 at 12:31 PM, Lukas Fleischer <archlinux@cryptocrack.de>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@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. * Use captchas.
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.
+ 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.
+ 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?
+ 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@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?
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