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@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@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