WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35006 closed defect (bug) (fixed)

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

Reported by: scottbrownconsulting Owned by: jorbin
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description

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.

Attachments (6)

1.png (83.2 KB) - added by scottbrownconsulting 4 years ago.
Email notification to inbox from autotrashed comment
2.png (78.5 KB) - added by scottbrownconsulting 4 years ago.
The autotrashed message in the wordpress Trash folder
table.png (30.8 KB) - added by scottbrownconsulting 4 years ago.
unit test results
table2.png (15.1 KB) - added by scottbrownconsulting 4 years ago.
unit tests 2
35006.diff (635 bytes) - added by peterwilsoncc 4 years ago.
35006.2.diff (2.0 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (35)

#1 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#2 @swissspidy
4 years ago

  • Keywords needs-patch needs-unit-tests added

#3 @voldemortensen
4 years ago

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.

#4 follow-up: @peterwilsoncc
4 years ago

  • Keywords reporter-feedback added

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

#5 in reply to: ↑ 4 ; follow-up: @scottbrownconsulting
4 years ago

  • Keywords reporter-feedback removed

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

#6 in reply to: ↑ 5 @peterwilsoncc
4 years ago

  • Milestone 4.4.1 deleted
  • Resolution set to worksforme
  • Status changed from new to closed

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.

#7 follow-up: @scottbrownconsulting
4 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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
4 years ago

Email notification to inbox from autotrashed comment

@scottbrownconsulting
4 years ago

The autotrashed message in the wordpress Trash folder

#8 @SergeyBiryukov
4 years ago

  • Milestone set to Awaiting Review

#10 in reply to: ↑ 7 ; follow-up: @peterwilsoncc
4 years ago

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.

#11 in reply to: ↑ 10 @scottbrownconsulting
4 years ago

Replying to peterwilsoncc:

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

@scottbrownconsulting
4 years ago

unit test results

#12 @scottbrownconsulting
4 years ago

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.

#13 follow-up: @peterwilsoncc
4 years ago

  • Keywords close reporter-feedback added

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?

#14 in reply to: ↑ 13 @scottbrownconsulting
4 years ago

Replying to peterwilsoncc:

I will test with all plugins deactivated.

@scottbrownconsulting
4 years ago

unit tests 2

#15 @scottbrownconsulting
4 years ago

  • Keywords reporter-feedback removed

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.

#16 @scottbrownconsulting
4 years ago

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 :)

#17 @scottbrownconsulting
4 years ago

  • Keywords needs-unit-tests removed

#18 @scottbrownconsulting
4 years ago

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.

Last edited 4 years ago by scottbrownconsulting (previous) (diff)

#19 @scottbrownconsulting
4 years ago

  • Keywords close removed

#20 follow-up: @peterwilsoncc
4 years ago

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

#21 @peterwilsoncc
4 years ago

  • Milestone changed from Awaiting Review to 4.4.1

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

#22 in reply to: ↑ 20 @scottbrownconsulting
4 years ago

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.

#23 @scottbrownconsulting
4 years ago

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
4 years ago

#24 @peterwilsoncc
4 years ago

  • Keywords has-patch added; needs-patch removed

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.

#25 @johnbillion
4 years ago

  • Keywords needs-unit-tests added

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

@swissspidy
4 years ago

#26 @swissspidy
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

35006.2.diff adds the needed unit tests.

#27 @jorbin
4 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from reopened to closed

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

#28 @jorbin
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.4.1

#29 @dd32
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#30 @dimioorg
4 years ago

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 );
}
Note: See TracTickets for help on using tickets.