After a great deal of work with this module, I ended up doing several tuning:

- Fixed a FALSE negative in some greylist responses from SMTP servers.

- Improved overall SMTP communication to accept multi-line responses and read response code.

- Implemented socket timeout for unresponsive servers.

- Improved output messages for log that include complete communication with server and e-mail addresses.

- On high traffic sites, bots will use once and again the same e-mail address, you server's IP will eventually get blocked. Added a caching layer to mail verification.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

david_garcia’s picture

FileSize
5.29 KB
oadaeh’s picture

Category: bug » task

It will be more likely to be incorporated, if you provide a patch.

With a patch, I can quickly scan the changes, apply the patch, test the functionality, and commit.

Without a patch, I also need to research your changes and where they are likely to go, manually apply them to the code base, see how they work and if they work, etc. It is more time and effort, and is something I am not likely to do, with my limited availability.

david_garcia’s picture

I was posting just as reference, but it is good to see the mantainer is "awake". I will then provide a patch ASAP.

Greetings,

david_garcia’s picture

Not a GIT patch, but hope it will help.

Just to note, to improve detection rate make sure that the IP used in your SMTP server has proper Reverse DNS setup, spf records and the like.

etron770’s picture

same here

Rejected email address: xxxx@yyyy.de. Reason: 450 4.2.0 : Recipient address rejected: Greylisted, see ..

david_garcia’s picture

Issue summary: View changes
FileSize
5.64 KB

etron770, give a try to my patch, actually, you can completely replace the email_verify.inc with the one I uploaded.

Using Hidden Captcha + Honeypot + This module I am getting 0, litterally 0 bot in my websites, without annoying users with regular captcha.

I'm not sure if I made more modifications to the file since I first uploaded this, so I attach it again.

Greetings and provide some feedback so the maintainers can take care of this!

oadaeh’s picture

Status: Active » Needs work

Your most recent attachement is not a patch. See the Drupal documentation on patches.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
6.88 KB

Glad to see someone alive with commit rights.

Those patches were the first thing I had ever worked on Drupal related, quite a while ago, so excuses for lack of formality.

Here goes a decently revised patch, there is an extra addition to the original changes, I will update the issue summary.

I know that better to have every small change in it's single issue, but these changes are all related and the module needed some looking into after years of abandonement.

david_garcia’s picture

Issue summary: View changes
etron770’s picture

#6
Hi David,
actually I have no information that a user was greylisted.
so I can not test your patch.

david_garcia’s picture

Because the current module only reads single-line output, it is very difficult to debug grayliting and other problems from current watchdog messages because full SMTP response is not logged.

This is the change responsible for the fix of false positives with graylisting:

       // Any 4xx error also means we couldn't really check except 450, which is
       // explcitely a non-existing mailbox: 450 = "Requested mail action not
       // taken: mailbox unavailable".
-      preg_match("/^4/", $to) && !preg_match("/^450/", $to)) {
+     preg_match("/^4/", $to)) {

The thing is that code 450 is used by mailservers to indicate graylisting (and other temporary issues), meaning that if we receive any 450 it should be treated like the e-mail cannot be verified, not that it is an invalid e-mail:

A 450 email error is typically a temporary routing issue on the receiving mail server's end. Sometimes these are also Greylisting responses from the receiving mail server used to filter incoming mail.

Greylisting is a method of accepting messages a mail server can use that will block the first attempt to deliver a message, and require a certain number of successful retries from the sending mail server, before allowing the original message to get relayed through the receiving mail server. This tactic is typically used to help cut down on spam from spammers that don't utilize a legitimate mail server that will retry delivery attempts to ensure they go through.

You usually would not receive a bounce-back message for this type of error. If your message is going to be delayed more than 24 hours, then sometimes a "delivery has been delayed" soft bounce will be returned to you, to let you know that the delivery has yet to complete successfully.

david_garcia’s picture

Updated comments to fix wrong asumption that 450 error code means invalid e-mail. Any 4xx code just means temporary unavailable, for example, when graylisting applied.

oadaeh’s picture

Category: Task » Feature request
Status: Needs review » Needs work

I'm sorry to take so long to get back to you on this, however, I have a lot going on in my life. Your various proposed feature requests sound like excellent ideas, but since you've created a patch with a lot of different changes in it, it will take me a while to review it.

Also, it looks like you've included code changes from another patch (with no indication of that in this issue or in the other one), and so I'm going to have to spend time figuring out what code in the other patch is included in this one (so I can determine if any of the other issue is still valid or not).

Maybe you don't realize this, but I don't just blindly accept and include patches without some sort of code and functionality review and testing. So, when someone provides a patch that includes code changes for multiple features (and a little code clean up here and there), it takes time to determine which code changes are relevant to which proposed feature and which are not, before I can even start reviewing the feature.

It takes me (and anyone else) far less time to review smaller, discrete patches and changes, which means they are more likely to be included far sooner.

So, please create separate issues and patches for each feature and/or discrete set of changes you are proposing being made. You don't have to create a separate patch for each watchdog message wording change. You may include all of those that are unrelated to other proposed feature changes in one patch, but please be sparing about your "code cleanup" changes in one patch, as again, too many changes in one file makes reviewing them difficult and time consuming.

Also, when you create your patches, please follow the Drupal standards, security and best practices outlined in these pages: https://www.drupal.org/developing/best-practices

Thank you.

oadaeh’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev

Moving to the new current development branch.

killes@www.drop.org’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
8.07 KB

I've updated the patch.

I think that supporting greylisting is fairly important nowadays and wouldn't recommend using this module without this patch.

oadaeh’s picture

Unfortunately, the patch still fails in some or all of the points I mentioned last year.
Also unfortunately, the update came in after several other patches were already in the pipeline to be included, and they significantly modified the code this patch attempts to modify.
Fortunately, however, I have the time, the means, and the desire to update the patch and include it in the latest 2.x branch.
Also, having just been doing some significant changes in many parts of the module, it was not as time consuming to review and figure out as it would have been last year.
So, I've attached an updated patch that should apply to the latest code base.
I took the liberty of updating the comments in several places to be clearer and applying Drupal Code Standards to the code.
My basic testing was successful, but let me know if I affected any of the functionality, as I did not have any multi-line server responses.
My patch is very different from the previous ones, and so I did not bother to create an interdiff, as that would probably take as much time to review as the patch itself.

killes@www.drop.org’s picture

Thanks for updating the patch!

I've reviewed it and noticed that it removes "WATCHDOG_WARNING" in some instances. Is that intentional?

I believe it would be best if WATCHDOG_WARNING would remain for the cases where an email is actually rejected and WATCHDOG_INFO remains to be used for the cases where it is not.

oadaeh’s picture

Hello Gerhard.

Sorry for not responding sooner. I had some other things I was working on that had a higher priority on my list.

I've reviewed it and noticed that it removes "WATCHDOG_WARNING" in some instances. Is that intentional?

I believe it would be best if WATCHDOG_WARNING would remain for the cases where an email is actually rejected and WATCHDOG_INFO remains to be used for the cases where it is not.

That was part of the patch that both David and you submitted.

I'm not necessarily as concerned about the logging level, as I am that the messages get logged, so I left it as it was presented in the patch.

However, since you brought it up, here's a new patch with those logging levels restored. Anything else?

BTW, do you happen to know of any publicly available mail servers that return multi-line responses? I can configure mine to do it, but that won't really help other people when testing that function of the patch.

Thanks.

killes@www.drop.org’s picture

Status: Needs review » Needs work

1st rule of Open Source development: Never apologize for delivering a patch. ;)

The patch looks good, but there's a problem: I can't register new users anymore, even if the email is ok.

This is due to the error check in function email_verify_verify_address($form, &$form_state) {

$error = email_verify_check($form_state['values'][$field]);
// Report the error and flag the form field.
if (!empty($error)) {
form_set_error($field, $error);

$error is now an array and will have something in it (for example debug info) at all times. So the check needs to be changed somehow.

killes@www.drop.org’s picture

Status: Needs work » Needs review
FileSize
885 bytes

The added patch ( to be applied after yours) does the job for me.

oadaeh’s picture

Ugh! That was supposed to go in with the patch on this comment #2493515-8: Add debugging logging, but I must have missed it merging in 4 or 5 conflicting patches at one time.
I created a new issue to address it: #2704229: Fix tests of return values

  • oadaeh committed 8af410c on 7.x-2.x authored by david_garcia
    Issue #2102323 by david_garcia, oadaeh, killes@www.drop.org: Greylist +...
oadaeh’s picture

Status: Needs review » Fixed

Thanks!

  • oadaeh committed 8af410c on 8.x-2.x authored by david_garcia
    Issue #2102323 by david_garcia, oadaeh, killes@www.drop.org: Greylist +...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.