Make WordPress Core

Opened 13 years ago

Closed 9 years ago

Last modified 9 years ago

#20226 closed enhancement (fixed)

Don't advertise pingback URL on resources that don't support pingbacks

Reported by: solarissmoke's profile solarissmoke Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.4 Priority: normal
Severity: minor Version: 3.3
Component: Pings/Trackbacks Keywords: has-patch
Focuses: template Cc:

Description

Currently, WordPress indiscriminately adds an X-Pingback header to all responses. Similarly all the bundled themes add link rel="pingback" to all pages.

The purpose of X-Pingback and link rel="pingback" is to advertise that the current resource supports pingbacks and provide a pingback URL. They should only be inserted when the resource actually accepts pingbacks - i.e., only when is_single() is true, and maybe even only when pings are open on that post.

Attachments (1)

20226.diff (826 bytes) - added by wonderboymusic 9 years ago.

Download all attachments as: .zip

Change History (26)

#1 @nacin
12 years ago

  • Type changed from defect (bug) to enhancement

I could go for a fresh look at this.

add_theme_support( 'automatic-pingback-link' ) could remove rel="pingback" from themes and put it into our hands.

From there, it's a matter of figuring out exactly when to issue the HTTP header and <head> element, and making it extensible.

#2 @nacin
12 years ago

  • Keywords nacin-likes added

#3 @solarissmoke
12 years ago

The rel="pingback" bit looks relatively easy to implement.

The X-Pingback header is not however, because currently headers are sent way before WP has any idea what it is serving. I think this is a bigger issue because it means that things like Last-Modified headers are generic and don't actually reflect the last modified time of the content being served (which sort of defeats the purpose of having them).

#4 @nacin
11 years ago

  • Component changed from Template to Pings/Trackbacks
  • Focuses template added
  • Keywords nacin-likes removed
  • Milestone changed from Awaiting Review to Future Release

@wonderboymusic
9 years ago

#5 @wonderboymusic
9 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 4.4

#6 @wonderboymusic
9 years ago

In 34442:

PINGBACKS: rather than sending the X-Pingback HTTP header on every single request for fun, perhaps only send it on single posts with pings open.

See #20226.

#7 @swissspidy
9 years ago

Does [34442] also work for custom post types?

#8 @wonderboymusic
9 years ago

@swissspidy:

function pings_open( $post_id = null ) {

	$_post = get_post($post_id);

	$open = ( 'open' == $_post->ping_status );

	/**
	 * Filter whether the current post is open for pings.
	 *
	 * @since 2.5.0
	 *
	 * @param bool        $open    Whether the current post is open for pings.
	 * @param int|WP_Post $post_id The post ID or WP_Post object.
	 */
	return apply_filters( 'pings_open', $open, $post_id );
}

#9 @ocean90
9 years ago

is_single() works for any post type, except attachments and pages. Should it be is_singular() instead?

#10 @wonderboymusic
9 years ago

yes, yes it should - incoming

#11 @wonderboymusic
9 years ago

In 34443:

PINGBACKS: After [34442], switch to is_singular() to check attachments and pages as well.

See #20226.

#12 @wonderboymusic
9 years ago

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

Marking this fixed, made PR for TwentySixteen: https://github.com/WordPress/twentysixteen/pull/255

#13 @pento
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[34443] broke unit tests.

1) Tests_Canonical_PageOnFront::test with data set #0 ('/page/2/', '/page/2/', 20385)
Ticket #20385
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/page/2/'
+'/'

/srv/www/wordpress-develop/tests/phpunit/includes/testcase-canonical.php:206
/srv/www/wordpress-develop/tests/phpunit/tests/canonical/pageOnFront.php:37

#14 @wonderboymusic
9 years ago

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

In 34476:

WP: after [34443], calling get_queried_object() messes up unit tests. We can just clone the $post prop and call it a day.

Fixes #20226.

#15 @chris@…
9 years ago

I'm slightly confused by this. send_headers() job is to figure out what headers should be sent. What's nice about this is that right before actually sending them the hook wp_headers is called allowing us to filter them. However, now the X-Pingback header has been moved to handle_404() so that function is really handle_404_and_maybe_send_pingback_header()?

I'm actually really for this ticket and have been unsetting that header using wp_headers on pretty much every site for a long time now, but this seems really hacky.

If this does go through, the docs for send_headers() and possibly handle_404() should be updated.

#16 @wonderboymusic
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

handle_404() is a bad name for the method. It has to be handled somewhere after posts have been queried

#17 @wonderboymusic
9 years ago

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

In 34632:

Update the docs in WP to explain the need to do [34476].

Fixes #20226.

#18 @DrewAPicture
9 years ago

In 34635:

Docs: Fix some syntatical issues in the DocBlock for WP::send_headers() following [34632].

See #20226. See #32246.

#19 @mark-k
9 years ago

This is wrong per #14971 (obviously Nacing is just human and forgets from time to time his previous decisions ;) )

There is no reason not to handle pingbacks for home page and category pages. Just because core has no built-in mechanism to display them doesn't mean that it will not be handle by a plugin.

If the point of this is to prevent pingback spam, or using pingback as DDOS medium, then I think you will be surprised with how lazy spammers are. They know that pingbacks on wordpress should be sent to /xmlrpc.php so why should they even bother reading the header or parsing the page when all they need is the URL to produce the pingback.

And if this stays then all core themes should be changed to have the same behavior on X-pingback and rel="pingback. Right now on my about week old 4.4 I see the old behavior on 2014 and 2015

#20 @nacin
9 years ago

I guarantee that this will break the situation where someone is checking the homepage to see if it's up/working/WordPress by looking for X-Pingback. They're also probably doing the same for <link>. I don't necessarily care either way. @dd32, does this all look kosher to you?

I don't think this is to prevent pingback spam. I imagine it's just for sanity purposes. But I'm not convinced this will make things any better. All it does is add another check to themes (when we could probably stand to remove it and use X-Pingback only).

#21 @mark-k
9 years ago

@nacin, I was actually about to reopen that ticket to argue with the reasoning filosofo gave to closing it, but decided that following my reasoning to the end will require changing how wordpress behaves as a pingback client which is most likely just a waste of time for everybody.

But if we don't want to fight filosofo's reasoning we have to get to the conclusion that this change is wrong in the way it is done. The pingback standard AFAIK do not have a facility to communicate in the HTTP header that you do not support pingbacks, therefor what will happen based on wordpress core code (and with 25% of known CMS market I think it is just safe to ignore all other pingback implementations) is that in post that linking to the home page of a site there will be an attempt to send pingback to the site. The first request sent will be an HTTP HEAD which will result in no X-pingback header. Since there is no header wordpress will issue a full GET to get the whole page and try to parse it to get the rel="pingback" link. All themes that followed core theme development style have the rel="pingback" link on all pages including the home page. Now wordpress have the URL of the XML-RPC end point and issues a pingback to it.

So with the current change there will be 3 HTTP request to the site for like 99% of the wordpress sites instead of the two that were up to 4.4.

The only possible way forward is probably to add the pingback generation into a wp_head hook and then core will be able to control both aspects, but it will take years for themes to change to use it, although I think it will be faster then assuming that all themes will implement the change as it was done in 2016.

And then there is the plugin override issue. The way the http header code works it is very hard to override a header (but maybe I am wrong here, corrections are welcome) therefor this code and the change in 2016 will prevent me implementing pingback for my home page if I wanted this functionality.

#22 @dd32
9 years ago

I'm on the fence here.

On one hand, WordPress itself doesn't support pingbacks for non-singular URLs, advertising it as such is probably the wrong thing to do. It doesn't do any harm though.

On the other hand, it doesn't do any damage to advertise the URL for all resources, it makes it easier to identify a WordPress site, some services might be checking the root for pingback support and aborting if not, and any existing plugins which are managing pingbacks for non-singular URLs will now break (especially since the theme <link> tag is now locked behind an is_singular()).

I kind of think that the is_singular() checks should at a minimum be moved into pings_open(), with pings_open() being checked for any valid queried object.

The first request sent will be an HTTP HEAD which will result in no X-pingback header.
...
So with the current change there will be 3 HTTP request to the site for like 99% of the wordpress sites instead of the two that were up to 4.4.

If that were true, then I would agree that we can't make the change, but that's not the behaviour of Stock WordPress, it only happens if there's something else short-circuiting HEAD requets.

#23 follow-up: @mark-k
9 years ago

from the discover_pingback_server_uri function in .4.4

	$response = wp_safe_remote_head( $url, array( 'timeout' => 2, 'httpversion' => '1.0' ) );

	if ( is_wp_error( $response ) )
		return false;

	if ( wp_remote_retrieve_header( $response, 'x-pingback' ) )
		return wp_remote_retrieve_header( $response, 'x-pingback' );

	// Not an (x)html, sgml, or xml page, no use going further.
	if ( preg_match('#(image|audio|video|model)/#is', wp_remote_retrieve_header( $response, 'content-type' )) )
		return false;

	// Now do a GET since we're going to look in the html headers (and we're sure it's not a binary file)
	$response = wp_safe_remote_get( $url, array( 'timeout' => 2, 'httpversion' => '1.0' ) );

pinging home page of a 4.3 site scenario:
the call to wp_remote_retrieve_header return the xmlrpc end point of the site and we can continue to sending the pingback

pinging home page of a 4.4 site scenario:
the call to wp_remote_retrieve_header return false
therefor we call wp_safe_remote_get an parse the content

One more request.

Of course that code is bad as it should just issue a GET in the first place and then decide from which source, header or content, to get the endpoint URL.

And since this change do not remove the rel="pingback" from the content it will not prevent the sending of the pingback itself.

At the minimum the decision when to send X-pingback should be filterable/overideable. With the 4.4 code it is kinda possible but just too hard.

#24 in reply to: ↑ 23 ; follow-up: @dd32
9 years ago

Replying to mark-k:
Sorry, I misunderstood your message, I thought I read a request to a post, not the homepage, my mistake.

You're right, this will indeed cause an extra HTTP request when a page links to a non-singular URI.

#25 in reply to: ↑ 24 @mark-k
9 years ago

Replying to dd32:

You're right, this will indeed cause an extra HTTP request when a page links to a non-singular URI.

And if the pings are closed it will be like that for all pingbacks.

Note: See TracTickets for help on using tickets.