Comments sent immediately to Trash for matching keyword blacklist should not generate email notifications

[scottbrownconsulting]

Going from 4.3.1 to 4.4 the behavior of comments blacklisted for matching the keywords set in Discussion settings seems to have changed. Previously these would go quietly to Spam folder. Now they go to Trash folder and, most aggravatingly, generate an email notification. This seems erroneous. If a comment hits on my blacklist, I don't care if it goes to Spam or Trash, but I don't want to hear about it, it should do so quietly.

[voldemortensen]

The change to default behavior was intentionally made in r34726. The only problem I see here is that its generating email notices, but I haven't been unable to reproduce being notified erroneously.

[peterwilsoncc]

@scottbrownconsulting

Welcome to trac.

I'm unable to replicate this locally, I'm sorry. Do you have Akismet or any other anti-spam plugins running on your site?

If so, are you able to disable them and double check this bug still occurs?

Thanks.

[scottbrownconsulting]

@peterwilsoncc

I run sans akismet. My keyword blacklist is my spam defense.

I submitted this report because I received what I identified as an email notification about an automatically Trashed comment in the first hours after upgrading to 4.4. I haven't received any further since, so I admit I could have been mistaken. It's hard to tell because I only see what makes it through mail transport spam filters, which can be unpredictable.

I'll try to study the core codebase and see what I can figure out.

[peterwilsoncc]

Replying to scottbrownconsulting:

@peterwilsoncc

I run sans akismet. My keyword blacklist is my spam defense.

I submitted this report because I received what I identified as an email notification about an automatically Trashed comment in the first hours after upgrading to 4.4. I haven't received any further since, so I admit I could have been mistaken. It's hard to tell because I only see what makes it through mail transport spam filters, which can be unpredictable.

I'll try to study the core codebase and see what I can figure out.

Thanks,

As neither you, @voldemortensen or I could replicate I'll close the ticket off for now. If it turns out to be an intermittent bug we can re-open down the track.

[scottbrownconsulting]

I have had one happen again (an email notification arrive to my inbox on an automatically trashed comment). I am reopening the ticket and attaching screenshots. Please review. Thanks.

[scottbrownconsulting]

Email notification to inbox from autotrashed comment

[scottbrownconsulting]

The autotrashed message in the wordpress Trash folder

[peterwilsoncc]

Replying to scottbrownconsulting:

I have had one happen again (an email notification arrive to my inbox on an automatically trashed comment). I am reopening the ticket and attaching screenshots. Please review. Thanks.

I'm still unable to reproduce the problem locally.

I can see on the site you are running the SI CAPTCHA Anti-Spam plugin, are you able to run some tests:

• comment with spam keyword (spam comment), valid capture, honeypot empty
• spam comment, invalid capture, honeypot empty
• spam comment, valid capture, text in honeypot
• spam comment, invalid capture, text in honeypot
• valid comment, valid capture, honeypot empty
• valid comment, invalid capture, honeypot empty
• valid comment, valid capture, text in honeypot
• valid comment, invalid capture, text in honeypot

thanks.

[scottbrownconsulting]

Replying to peterwilsoncc:

Will need a few days to have time to get to it.

[scottbrownconsulting]

unit test results

[scottbrownconsulting]

There are the requested test findings.

The SI-CAPTCHA plugin does not do content filtering, it just presents and validates a CAPTCHA. So the effect on all tests with invalid CAPTCHAs is the pipeline stops right there, an "ERROR: Wrong CAPTCHA" error screen is shown, and the comment never makes it downstream. So those tests don't add much information.

I did positively produce the offending behavior in test #3 though. I included a blacklisted keyword (the keyword was "prescription") in the comment body, my blacklist was populated in Discussion Settings in admin, the CAPTCHA was valid. The HTTP response strangely hanged, which is definitely a clue. The comment went straight to "Trash" folder. A minute later, I got an unwanted notification.

The notifications generated on this defect are different from notifications on non-spam (non-autotrashed) comments in that their From: address spoofs the "Name" field from the comment form. Normal (non-spam, non-autotrashed) comments generate a notification from wordpress@mydomain only. That is also interesting.

[peterwilsoncc]

This is sounding more like a plugin update. Are you able to try and duplicate on an install running a default theme without any plugins?

[scottbrownconsulting]

Replying to peterwilsoncc:

I will test with all plugins deactivated.

[scottbrownconsulting]

unit tests 2

[scottbrownconsulting]

That set of tests is with all plugins deactivated. The source of the issue is not a plugin, it seems to be in core. I produce the issue in test B. A comment with a blacklisted keyword "prescription" went automatically to trash but generated an email notification.

[scottbrownconsulting]

I have located the issue in the code of wp-includes/comment-functions.php.

The notification I'm receiving isn't an admin moderation notification (from function wp_new_comment_notify_moderator), it's a post author notification (from function wp_new_comment_notify_postauthor). I am both the site administrator and the author of all my posts, so I get both.

In the case of an autotrashed comment for violating the keyword blacklist, line 1711 in function wp_new_comment_notify_moderator traps out any comment that doesn't have comment_approved=0 which means "Pending" status.

However, line 1738 in function wp_new_comment_notify_postauthor is deficient in its screening comment_approved values since the change from Spam to Trash in 4.4. It is only screening out values of 'spam' but not values of 'trash'. A value of 'trash' needs to be newly trapped here on line 1738, just like 'spam' is. This was apprently overlooked in the last changeset.

Well, see, I am not imagining things :)

[scottbrownconsulting]

Basically line 1738 of wp-includes/comment-functions.php could just read

if ( '1' != $comment->comment_approved ) {

instead of trying all the possibilities it's checking now. Comparable logic was good enough for line 1711 so why not do the same idea here.

[peterwilsoncc]

@scottbrownconsulting thanks for hunting it down, I was going down the admin path :)

Is there any chance you could upload a patch file? You can follow either the ​git guide or the ​svn guide, depending on your preferred version control software.

[peterwilsoncc]

False starts out of the way, putting this back on the milestone.

[scottbrownconsulting]

Replying to peterwilsoncc:

Is there any chance you could upload a patch file?

Regrettably I'm not set up with SVN so I have to rely on someone else to create the patch. But I've put the gist of it right above.

[scottbrownconsulting]

In trunk (as opposed to 4.4), the affected bit is line 1805 of src/wp-includes/comment.php. Although the entire body of wp_new_comment_notify_postauthor bears cleanup to conform with the order of operations in wp_new_comment_notify_moderator right above it.

[peterwilsoncc]

Patch in 35006.diff​

For the immediate bug, I've added a check against 'trash' to the if statement mentioned by @scottbrownconsulting.

You may be right about the function needing a tidy-up and worth a seperate enhancement to target a major release.

[johnbillion]

We've some test coverage of wp_new_comment_notify_postauthor() in the Tests_Comment class.

[swissspidy]

35006.2.diff​ adds the needed unit tests.

[jorbin]

In 36119:

Ensure only approved comments trigger post author notifications

Posts that are trashed shouldn't trigger post author notifications. Adds unit tests to enforce this.

Props scottbrownconsulting, peterwilsoncc, swissspidy
Fixes #35006

[jorbin]

Reopening for 4.4.1

[dd32]

In 36146:

Comments: Ensure only approved comments trigger post author notifications
Posts that are trashed shouldn't trigger post author notifications. Adds unit tests to enforce this.

Merges [36119] to the 4.4 branch.
Props scottbrownconsulting, peterwilsoncc, swissspidy.
Fixes #35006.

[dimioorg]

Comments sent immediately to Trash for filtered captcha generate email notifications. Use Invisible Captchs plugin.
WP 4.4.1 (at three sites)

<?php
$ sed -n "1783,1813p" comment.php
function wp_new_comment_notify_postauthor( $comment_ID ) {
        $comment = get_comment( $comment_ID );

        $maybe_notify = get_option( 'comments_notify' );

        /**
         * Filter whether to send the post author new comment notification emails,
         * overriding the site setting.
         *
         * @since 4.4.0
         *
         * @param bool $maybe_notify Whether to notify the post author about the new comment.
         * @param int  $comment_ID   The ID of the comment for the notification.
         */
        $maybe_notify = apply_filters( 'notify_post_author', $maybe_notify, $comment_ID );

        /*
         * wp_notify_postauthor() checks if notifying the author of their own comment.
         * By default, it won't, but filters can override this.
         */
        if ( ! $maybe_notify ) {
                return false;
        }

        // Only send notifications for approved comments.
        if ( ! isset( $comment->comment_approved ) || '1' != $comment->comment_approved ) {
                return false;
        }

        return wp_notify_postauthor( $comment_ID );
}