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. "archli...@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: <mailt...@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
