WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 5 years ago

#2933 closed enhancement (wontfix)

Drop ping servers that systematically fail from the ping list

Reported by: c960657 Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.8
Component: XML-RPC Keywords: has-patch commit early
Focuses: Cc:

Description

When pinging update services like http://rpc.pingomatic.com/, any error message returned by the service is ignored.

I run a weblog index like Weblogs.com, except that my index is only for weblogs written in Danish. In order to ping my site, users have to sign up in advance. When I receive a ping on my XML-RPC service, I verify that the site mentioned in the ping is subscribed to my service. If it isn't, I respond with an error message (flerror = true) as described in the weblogUpdate.ping specification, http://www.xmlrpc.com/weblogsCom. However, Wordpress just drops this error message without letting the user know that shouldn't ping my site.

This may not sound like a big problem, but it is for me. 99% of the pings I recieve are from users who are not registered and who are clearly not Danish so they are not eligible for inclusion in my index. They have probably got the URL for my ping service from lists like this http://www.instant-info-online.com/wordpress-compressed-all-inclusive-ping-list.html that they have blindly copy-pasted to the Wordpress installation without checking whether their pings are relevant on all the listed services. I currently have no way of asking these users to stop pinging my site.

I would like Wordpress to show these error messages to the user, so that he can remove any services that reject his ping.

Attachments (6)

ping.patch (2.9 KB) - added by c960657 9 years ago.
ping-2.patch (3.2 KB) - added by c960657 9 years ago.
Updated patch
ping-2.2.patch (3.3 KB) - added by c960657 9 years ago.
ping-3.patch (3.3 KB) - added by c960657 8 years ago.
Updated patch (the old had bitrotted)
ping-4.patch (5.9 KB) - added by c960657 8 years ago.
Updated patch
2933.diff (2.4 KB) - added by Denis-de-Bernardy 6 years ago.

Download all attachments as: .zip

Change History (42)

@c9606579 years ago

comment:1 @c9606579 years ago

The patch is an attempt to solve the problem. It modifies weblog_ping() to return an error message. Error messages may be either on the XML-RPC layer or error messages reported as specified in the weblogUpdates.ping specification (when flerror == true).

The error count for each service and the last error message is saved in the option ping_errors. If a successful ping is done, any previos errors are cleared.

On the settings page where the update services are listed, all update services that have failed the last three times or more are displayed. This allows a service to be temporarily down without being listed.

This is my first contribution to Wordpress, so please bear with if I don't follow the coding guidelines etc. I am not a regular Wordpress user myself, but a lot the users pinging my site are, and this is why I have written this patch :-)

comment:2 @matt9 years ago

All ping services are spammed mercilessly, it has nothing to do with WordPress. They load up lists of any place that possibly accepts a ping and hammer it. I know people who have been returning a 404 on their ping script for over a year and they still get 20-30 a second. I'll look at this patch but I don't think it will do anything to help your ping volume problem.

comment:3 @c9606579 years ago

Perhaps it is optimistic to think that spammers will care a bit about this. But spammers do use Wordpress, at least according to the user agent string in the XML-RPC request (about 75% of the hits on my XML-RPC service have Wordpress user agents). Of course Wordpress cannot be blamed for this, but if Wordpress could somehow alleviate the problem, I think this would be a Good Thing. I have two suggestions:

  1. Instead of just warning the user, the service could automatically be removed after e.g. 25 unsuccesful pings (this could be fine-tuned so that a rejected ping counts more than a server being down or returning a 500 Server Error).
  2. If the message part of the response contains a specific "magic" string, e.g. "DO NOT EVER PING ME AGAIN", this will make Wordpress remove the service immediately.

What do you think of these suggestions? Of course, spammers could just reinsert the URLs of the failing services, but hopefully they will realize that this will not have any affect.

But even though it only has limited effect to show the error message on the options page, I still think it is worth doing. The specific error message may actually be useful to the user (e.g. "your URL should start with 'http://'") and at least it allows Good People to stop pinging sites that reject pings or have been shut down.

@c9606579 years ago

Updated patch

@c9606579 years ago

comment:4 @c9606579 years ago

The updated patch, ping-2.2.patch, implements the two suggestions in my last comment. If the server returns the magic string three times, or when a ping server fails for the 26th time, the server is removed from the list of ping sites. What do you think of this?

Assuming that a spammer is actually using WordPress (and not just spoofing a WordPress user-agent string), this will allow the ping server to have itself removed from the spammer's list of ping sites. Of course the spammer can just add the site again or remove the auto-remove feature from the PHP code, but hopefully he will realize that there is no point in pinging sites that are able to detect that he is a spammer.

@c9606578 years ago

Updated patch (the old had bitrotted)

comment:5 @c9606578 years ago

Ignoring the spam problem, I still think it would be useful if WordPress showed error messages returned by the update services.

I frequently get mail from users who have problems pinging my XML-RPC. This is often because they ping with a slightly different URL than the one they are subscribed with. My reason for rejecting the ping is explained in the error message, but this isn't displayed to WordPress users, so it is difficult for them to find out exactly what is wrong.

Also this will alert honest users who ping update services that have been taken out of service so that they don't waste time and bandwidth on abandoned services, and so that the abandoned hostname isn't "cursed" forever with pings from WordPress users who have a larger number of update services copy-and-pasted from lists like the one on http://codex.wordpress.org/Update_Services.

comment:6 @c9606578 years ago

  • Keywords has-patch 2nd-opinion needs-testing added

comment:7 @foolswisdom8 years ago

  • Milestone set to 2.2

comment:8 @DD328 years ago

I agree that theres not much that can be done about the spam issue,
But I think it would be a good idea to have a way of reporting the error to the end-user, it would at least make those who mean good to stay that way.

comment:9 @foolswisdom8 years ago

  • Milestone changed from 2.2 to 2.3

comment:10 @C9606578 years ago

I am new to WordPress development, so please let me know if I need to do more to get this patch accepted, thanks.

comment:11 @c9606578 years ago

If this is targeted for 2.3, I suggest checking in the patch so that it will be properly tested before the release.

comment:12 @Otto428 years ago

I don't like the idea of requiring a "magic string" for removal. I think it would be better if the thing used standard http error codes instead.

A "410 Gone" response should cause a removal of the address from the ping list. Anybody wanting to disconnect their ping can replace it with the 410 response, which is unlikely to happen under any normal circumstances. With Apache, this is a simple .htaccess directive to do.

Similarly, a "301 Moved Permanently" response should cause the address in the ping list to be updated to the new one. This lets servers change their ping addresses easily.

@c9606578 years ago

Updated patch

comment:13 @c9606578 years ago

The updated patch, ping-4.patch, now uses HTTP status codes for removal.

If the server returns 403 Forbidden, 404 Not found or 410 Gone, and this is the third failure in a row, the server is removed. If other errors happen, the server is removed after 20 failures in a row.

Also, the client no longer tries to call weblogUpdates.ping, if the call to weblogUpdates.extendedPing failed, unless the first call returned a "200 OK". If the server returns anything else, it is an indication that the URL does not point to an XML-RPC service (according to the XML-RPC spec, servers should always return 200 OK). This will also ease the load on servers that are rejecting pings.

I like your suggestion about "301 Moved permanently", but my patch doesn't address that. Perhaps this can be implemented in another issue.

Comments are welcome. I hope people will try out the patch and see if it works for them.

comment:14 follow-up: @Otto428 years ago

I don't like the idea of using 403/404 responses there. These often happen due to server misconfigurations or other circumstances that are out of control of the webmaster.

410, on the other hand, virtually never occurs unless you explicitly force it to occur. Suggest making 403/404 one of the "20 failure" scenarios.

comment:15 follow-up: @westi8 years ago

-1 to this.

I don't think WordPress core should be doing this.

I would be +1 to adding the relevant hooks to allow a plugin to do it.

The said same plugin could the provide better UI to manage the ping list and display the failure info.

comment:16 in reply to: ↑ 14 @c9606578 years ago

Replying to Otto42:

I don't like the idea of using 403/404 responses there. These often happen due to
server misconfigurations or other circumstances that are out of control of the
webmaster.

This is why the server isn't removed instantly but only after 4 failed attempts. But I agree that 410 is a lot less likely to happen by mistake.

I don't really care either way, though. The patch can easily be changed to allow a different number of retries for different error codes, or perhaps even more levels than the existing two.

comment:17 in reply to: ↑ 15 @c9606578 years ago

Replying to westi:

-1 to this.

This ticket includes two suggestions:

  1. Wordpress should inform the user if he is pinging a server that is no longer in use or for some reason is rejecting his pings.
  2. Wordpress should automatically remove servers that keep failing.

Are you objecting to both suggestions?

I believe # 1 is important in order to be a good netizen. Without it, Wordpress users are wasting resources on ping servers without their knowledge. # 2 is rather a nice-to-have.

Alternative suggestions on how to implement # 1 are welcome.

comment:18 @c9606578 years ago

I'll be happy to make a patch that includes only parts of the suggestion if that makes it more likely to be accepted. Please let me know what is and what isn't acceptable about my current suggestion.

comment:19 @westi8 years ago

  • Milestone changed from 2.3 to 2.5 (future)

comment:20 @c9606578 years ago

I am sad to see that this has now been pushed to version 2.5.

I have been writing patches, responding to comments and really trying to find a solution to the basic problem that everybody can agree on, but so far there hasn't been much response.

Showing status messages from pings will probably not solve the spam issue, although one could hope that it will reduce it just a little. Still, it is a simple feature that will be helpful to honest users who are using ping servers that have been taken out of service or are rejecting their pings for other reasons. It is very unusual for a piece of software to silently ignore errors without notifying the user in any way. This part of the patch is only a few lines of code. I think this is pretty basic functionality that should be present in the core (the auto-removal feature probably rather belongs in an extension, though).

However, if there is agreement among WordPress developers that WordPress should not show these error messages, let's just close this ticket and get this off the radar. I do hope, though, that we can find a compromise that is acceptable to everybody. I'd be happy to produce another patch, but only if there is a fair change that it will be accepted. So please let me know what kind of solution to the problem you'd accept. Thanks :-)

comment:21 @FFEMTcJ6 years ago

  • Keywords dev-feedback added
  • Milestone changed from 2.9 to Future Release

comment:22 @Denis-de-Bernardy6 years ago

  • Milestone changed from Future Release to 2.8
  • Priority changed from normal to high
  • Severity changed from normal to major
  • Summary changed from Show messages from weblogUpdates.ping/extendedPing to Drop ping servers that systematically fail from the ping list
  • Version set to 2.8

+1 to this idea.

Apart from the obvious issue for those who actually run ping servers, a patch that trims the ping list when a server systematically rejects would also do the users some good. I've seen fair amounts of hosts complaining about sites that hogged too much resources, not to mention the occasional out of memory errors when cron is doing its stuff. And most of the time it so happens to be a ludicrously long ping list.

Patch might need an update, but as you highlight there isn't much point in rewriting it if no dev is willing to commit the thing. Bringing the ticket down to 2.8, and changing its title, to make it more likely that a core dev bumps into it.

comment:23 @Otto426 years ago

Does anybody even use these things anymore? XMLRPC Pings seem so outdated now.

comment:24 @Denis-de-Bernardy6 years ago

Not for newbies... Mm... You know, real users? :-)

comment:25 @ShaneF6 years ago

  • Keywords needs-patch added; has-patch 2nd-opinion needs-testing removed
  • Milestone changed from 2.8 to Future Release

I am also -1 to this idea, but I like to see an updated patch before testing this.

Patch needs to re-developed. There are so many changes to the files that when applying the patch nothing is matching up.

comment:26 @Denis-de-Bernardy6 years ago

  • Keywords has-patch commit added; needs-patch removed
  • Milestone changed from Future Release to 2.8

@Denis-de-Bernardy6 years ago

comment:27 @Denis-de-Bernardy6 years ago

new patch uploaded implementing this as follows:

  • on failure: increment a counter for the service, once per day at most
  • after 14 such failures (that would mean 14 distinct days during which an error occurred!), drop service
  • on success, reset the counter if it's incremented

in other words, ping services get dropped if they consistently fail, no matter what their error message. fixes issues such as:

  • services returning 403/404/410
  • services returning junk (erroneous urls)
  • services that are always down

I could amend the patch a little bit to have it send a message to the site's admin's email, if feedback is absolutely necessary.

comment:28 @Denis-de-Bernardy6 years ago

  • Owner changed from anonymous to Denis-de-Bernardy
  • Status changed from new to accepted

comment:29 @ryan6 years ago

  • Priority changed from high to normal

comment:30 @westi6 years ago

  • Milestone changed from 2.8 to Future Release
  • Severity changed from major to normal

Moving out of 2.8 now we are in Beta.

I am still not convinced there is need for a core feature to be automatically messing with the list of ping servers.

I suspect that most users don't even touch the list at all.

This feels very much like plugin territory.

I am happy to add any filters/hooks that are necessary to enable a plugin to achieve this.

comment:31 @Denis-de-Bernardy6 years ago

  • Keywords dev-feedback removed

Agreeing on punting, but definitely not on the fact that it should be in a plugin. the guy who opened the ticket is enduring painful amounts of needless pings because sploggers and users who don't know better mindlessly copy a huge ping list and never change it afterwards.

Placing the code in a plugin would defeat the whole idea: these same sploggers and mindless users would *not* install the plugin.

comment:32 @Denis-de-Bernardy6 years ago

  • Milestone changed from Future Release to 2.9

comment:33 @Denis-de-Bernardy6 years ago

  • Keywords early added

still applies clean.

comment:34 @Denis-de-Bernardy6 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from accepted to assigned

comment:35 @ryan5 years ago

  • Milestone changed from 2.9 to Future Release

comment:36 @Denis-de-Bernardy5 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

clearly no interest here...

Note: See TracTickets for help on using tickets.