Message info
 
To:Arch User Repository (AUR) Development From:Slavi Pantaleev Subject:Re: [aur-dev] [PATCH] valid_email :: add dns check Date:Mon, 19 Mar 2012 23:33:29 +0200
 

On Mon, Mar 19, 2012 at 11:14 PM, Lukas Fleischer
<archlinux@cryptocrack.de>wrote:

> On Mon, Mar 19, 2012 at 09:48:36PM +0100, BlackEagle wrote:
> > When an email address is of the correct format check if there is an mx
> > record for the domain part. This forces people wanting to trash
> > something in aur to use a domain part with an mx record.
> >
> > This will not stop invalid email, it only adds an extra check and a
> > possible help for typos.
> >
> > Signed-off-by: BlackEagle <ike.devolder@gmail.com>
> > ---
> > web/lib/aur.inc.php | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
> > index c662b80..7e9d7de 100644
> > --- a/web/lib/aur.inc.php
> > +++ b/web/lib/aur.inc.php
> > @@ -80,7 +80,14 @@ 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);
> > + if (filter_var($addy, FILTER_VALIDATE_EMAIL) === false)
> > + return false;
>
> This isn't noted down for the AUR yet but we always use opening and
> closing braces in other Arch projects, such as pacman (even if it's just
> a one-line block). It probably makes sense to add such a rule to our
> coding guidelines.
>
> Could you please change that when resubmitting the patch? I'll take care
> of adding that to our guidelines later...
>
> > +
> > + $domain = substr($addy, strrpos($addy, '@')+1);
>
> Please add spaces before and after the plus sign (it should look like
> "strrpos($addy, '@') + 1").
>
> > + if (!checkdnsrr($domain, 'MX') || checkdnsrr($domain, 'A'))
>
> That check doesn't look correct to me. Why would we reject mail
> addresses if there's an A record for the domain part? You probably
> forgot parentheses and/or a negation ("!") here.
>
> Also, I'm not sure why you're checking for A records at all? Could you
> please elaborate on this?
>

Email delivery falls back to where the A record points if an MX record does
not exist.
https://en.wikipedia.org/wiki/MX_record#History_of_fallback_to_A

I think the correct check should be:

if (! (checkdnsrr($domain, 'MX') || checkdnsrr($domain, 'A'))) {
return false;
}


> Oh, and don't forget to add braces here as well :)
>
> > + return false;
> > +
> > + return true;
> > }
> >
> > # a new seed value for mt_srand()
> > --
> > 1.7.9.4
>