WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 5 months ago

#23915 assigned defect (bug)

discover_pingback_server_uri cases an error when discovery URI sets multiple Content-type headers

Reported by: tomdxw Owned by: dd32
Milestone: Future Release Priority: low
Severity: normal Version: 2.7
Component: Pings/Trackbacks Keywords: needs-patch 3.9-early
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Line 1673 in wp-includes/comment.php uses preg_match() on the output of wp_remote_retrieve_header(). When multiple headers are set of the same name, wp_remote_retrieve_header() returns an array.

Thus occasionally the following error occurs:

PHP Warning: preg_match() expects parameter 2 to be string, array given in xxxxxxx/wp-includes/comment.php on line 1673

I have no idea how to make pingbacks happen so to reproduce, add this line to functions.php: discover_pingback_server_uri('http://localhost:8080/');

And in a terminal window run this (you may need to install the netcat-openbsd package in Debuntu):
while true; do echo -en 'HTTP/1.1 200 OK\r\nContent-type: text/html\r\nContent-type: text/plain\r\n\r\n' | nc -lp 8080; done

Then visit your WP installation and you shall get the aforementioned error message.

Change History (6)

comment:1 dd3213 months ago

Related: #16890

Arguably, returning an array of headers isn't the expected behaviour for a HTTP Client, I believe the HTTP spec says they should be concatenated together when multiple headers are present.

comment:2 tomdxw5 months ago

  • Cc tom@… added

comment:3 SergeyBiryukov5 months ago

  • Description modified (diff)
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.8
  • Version changed from 3.5.1 to 2.7

Appears to be introduced in [9012]. Changed to use wp_remote_retrieve_header() in [17928].

According to the inline docs, wp_remote_retrieve_header() always returns a string:
tags/3.7.1/src/wp-includes/http.php#L229

If it returns an array when multiple headers of the same name are set, it should be either fixed or noted in the docs.

comment:4 samuelsidler5 months ago

  • Priority changed from normal to low

comment:5 nacin5 months ago

  • Owner set to dd32
  • Status changed from new to assigned

I think generating an E_WARNING based on external (pingback) output is not great. We should probably avoid that. At this point, it's a bit late to patch wp_remote_retrieve_header() (I agree with concatenating into a string) but maybe we can just do a cast/implode/pluck here.

Alas, in a few other places we use wp_remote_retrieve_header(), we also don't expect the array. So maybe punt and fix later. dd32?

comment:6 dd325 months ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

Alas, in a few other places we use wp_remote_retrieve_header(), we also don't expect the array. So maybe punt and fix later. dd32?

It's not a new thing, and I'm almost certain this isn't the only case that can trigger it, I don't think we need to fix this case urgently in 3.8, but tagging for 3.9 triage.

I dislike that the API returns multiple headers as a array (which is against the spec technically), but it's what we've got and what WP_HTTP users "expect" - strange thing here is that most code (plugins, etc) don't expect a array as a header value and break, but some make use of this for their own purposes.

Note: See TracTickets for help on using tickets.