WordPress.org

Make WordPress Core

Opened 11 years ago

Last modified 21 months ago

#5130 new defect (bug)

Linking to multiple posts on your site breaks pingback due to comment flooding

Reported by: Denis-de-Bernardy Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.3
Component: Pings/Trackbacks Keywords: has-patch needs-refresh needs-unit-tests
Focuses: Cc:

Attachments (1)

5130.diff (4.3 KB) - added by dshanske 2 years ago.
Add Support to Existing Functions to Rate Limit Differently Based on Pingback

Download all attachments as: .zip

Change History (24)

#1 @Otto42
11 years ago

  • Milestone changed from 2.5 to 2.4
  • Summary changed from pingPreserver to Linking to multiple posts on your own site breaks pingback due to comment flooding
  • Version set to 2.3

Useful, sure. But not particularly safe. The method he's using basically just opens up the ability for a comment flood through the xmlrpc pingback door.

A better approach would be to detect self-pings and perform them in a more direct manner, instead of through an http connection to xmlrpc. Preferably through an action of some kind, so self-pings can easily be disabled.

#2 @foolswisdom
11 years ago

  • Milestone changed from 2.4 to 2.5

Leaving as MS 2.5 until patch provided or developer takes ownership of the ticket.

#3 @scottsm
10 years ago

I'm the writer of the plugin. Sorry about the late comment, I never realized this made it up here until I was looking around today.

Anyway, I was worried about the possibility of a comment flood through the xmlrpc but since I haven't yet [knock on wood] ran into a comment flood but have been (slightly) annoyed with missing pings I figured I'd give this way a try. Detecting self-pings would be good but it seems like it would also be good to catch legitimate multiple pings from other blogs so I guess that wouldn't work.

So I guess that means fixing this would require some method similar to the plugin. It is worth mentioning that the plugin is only letting 5 pings through in 15 seconds compared to the default 1. What if the limit on pings was no more than 5 in say 75 seconds? That would offer the same protection as the current comment_flood_filter but still allow multiple pings.

This could be done easily by adding:

if (($time_newcomment - $time_lastcomment) < 15 ){
  if(strpos($_SERVER['PHP_SELF'],"xmlrpc.php")!==false){
    $pings=$wpdb->get_var("SELECT COUNT(comment_date_gmt) FROM $wpdb->comments 
            WHERE comment_author_IP = '".$_SERVER['REMOTE_ADDR']."' AND
            TIME_TO_SEC(TIMEDIFF(now(),comment_date_gmt)) < 75");
    if($pings>5) return true;
  } else return true;
}

to function wp_throttle_comment_flood in comment.php.

#4 @scottsm
10 years ago

Oh and the title "Linking to multiple posts on your own site breaks pingback due to comment flooding" doesn't cover the whole problem. An outside site linking to multiple posts on your own site also breaks pingback due to comment flooding. I've seen a decent number of times in my limited blogging experience where people have linked to more than one of my posts yet only one ping has shown up. I really like how pings improve the interconnectedness of blogging but the comment flood protection is getting in the way (unnecessarily?).

#5 @scottsm
10 years ago

  • Cc scottsm added

#6 @scottsm
10 years ago

  • Summary changed from Linking to multiple posts on your own site breaks pingback due to comment flooding to Linking to multiple posts on your site breaks pingback due to comment flooding

#7 @sardisson
9 years ago

  • Cc sardisson added

#8 @Denis-de-Bernardy
9 years ago

  • Component changed from General to Pings/Trackbacks
  • Owner anonymous deleted

@scott: is this still current?

#9 @scottsm
9 years ago

I still see the code:

function wp_throttle_comment_flood($block, $time_lastcomment, $time_newcomment) {
   if ( $block ) // a plugin has already blocked... we'll let that decision stand
      return $block;
   if ( ($time_newcomment - $time_lastcomment) < 15 )
      return true;
   return false;
}

in WP 2.7.1 without any checks for pings so I guess it should still be a problem. I'll check when I get home today.

#10 @Denis-de-Bernardy
9 years ago

  • Keywords needs-patch added

oh, ok. seeing it in trunk too.

#11 @janeforshort
8 years ago

  • Milestone changed from 2.9 to Future Release

Punting due to time and lack of traction during 2.9 dev cycle.

#12 @dd32
7 years ago

Closed #15676 and #14362 as duplicate of this

#13 @chriscct7
4 years ago

  • Severity changed from normal to minor

#14 @chriscct7
2 years ago

  • Severity changed from minor to normal

#15 @dshanske
2 years ago

#25141 was marked as a duplicate.

This ticket was mentioned in Slack in #core by dshanske. View the logs.


2 years ago

@dshanske
2 years ago

Add Support to Existing Functions to Rate Limit Differently Based on Pingback

#17 @dshanske
2 years ago

  • Keywords has-patch added; needs-patch removed

The proposed patch passes the full comment data to the comment_flood hook so the flood handling function can handle checking against multiple pings to the same URL as a flood and passes the comment type to the throttle filter to allow for different limits to be set based on comment type.

As commented on #25141, there are a number of possible solutions to the problem of multiple pings being received at the same time, either from the same site or a third-party and how it best be handled may be user preference.

At this time, being as we haven't reached a conclusion in nearly a decade since this was first reported, I'm suggesting we add support for plugins to address this externally and for possible future handling within Core itself.

This ticket was mentioned in Slack in #core-comments by dshanske. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-comments by dshanske. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-comments by dshanske. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-comments by dshanske. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by dshanske. View the logs.


21 months ago

#23 @pento
21 months ago

  • Keywords needs-refresh needs-unit-tests added

5130.diff doesn't really solve the problem, and it doesn't make it easy for plugins to solve the problem. (At least, not without extra queries on the part of the plugins, which is not ideal for anti-flood code.)

I'm inclined to agree with @nacin's suggestion in #25141 - allow up to 4 pingbacks quickly before dying.

This could be implemented by altering the query in check_comment_flood_db() - add comment_type to the WHERE condition, and when the comment_type is a pingback, change the LIMIT to LIMIT 1 OFFSET 4, so we're grabbing the time of the 5th most recent pingback. The comment_flood_filter filter will then act on the older pingback, meaning 4 pingbacks can be done in 15 seconds before comment_flood_filter returns true.

Note: See TracTickets for help on using tickets.