WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#28982 closed defect (bug) (fixed)

Improper HTTP headers generated within WP_Http_Streams->request() method for URI's specifying port

Reported by: kamelkev Owned by: dd32
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.9.1
Component: HTTP API Keywords: needs-patch
Focuses: Cc:

Description

Discovered an error while attempting to fetch_feed() for a URI using port :8080.

Reproduction for WordPress 3.9.1 is as follows:

  • Log into admin
  • Go to "Add Widgets"
  • Add an RSS Widget, configure the widget with an RSS feed specifying port 8080

Receive the following error:
RSS Error: A feed could not be found at http://example.com:8080/feed.rss. A feed with an invalid mime type may fall victim to this error, or SimplePie was unable to auto-discover it.. Use force_feed() if you are certain this URL is a real feed.

However within your feed server's log file you can clearly see that the port is being omitted from the request, you will instead receive a request on port 80.

Some of the callbacks related to requesting an RSS feed through SimplePie are complex, but I dug down all the way to "class-http.php", where I found the following lines which construct the request header:

if ( $proxy->is_enabled() && $proxy->send_through_proxy( $url ) )
  $strHeaders .= 'Host: ' . $arrURL['host'] . ':' . $arrURL['port'] . "\r\n";
else
  $strHeaders .= 'Host: ' . $arrURL['host'] . "\r\n";

The significant part here is the second branch, which omits specifying the port entirely. The author should have included the port, I am 99.9% sure the the HTTP specification states the port number should accompany the Host: line whenever it is stated. I can clearly see my web clients doing this, so I believe this to be a notable error that needs correction, otherwise the request is effectively broken for such a case.

Proposed changeset to fix the problem is as follows:

924,926c924
< 		if ( $proxy->is_enabled() && $proxy->send_through_proxy( $url ) )
< 			$strHeaders .= 'Host: ' . $arrURL['host'] . ':' . $arrURL['port'] . "\r\n";
< 		else
---
> 		if ( $arrURL['port'] == 80 || $arrURL['port'] == 443 )
927a926,927
> 		else
> 			$strHeaders .= 'Host: ' . $arrURL['host'] . ':' . $arrURL['port'] . "\r\n";

The behavior with respect to port 443 is ambiguous to me. Web clients will actually stamp the port into the header for this case, but I am pretty sure this is a gray area.

Attachments (2)

proposed.patch (688 bytes) - added by kamelkev 6 years ago.
patch.txt (596 bytes) - added by kamelkev 6 years ago.

Download all attachments as: .zip

Change History (9)

@kamelkev
6 years ago

#1 @kamelkev
6 years ago

Pasted the wrong changeset above, patch added instead. I'm sure someone else can format this better to fit the code style...

#2 @SergeyBiryukov
6 years ago

  • Component changed from Feeds to HTTP API

#3 @SergeyBiryukov
6 years ago

Introduced in [8620], modified in [10692].

#4 @dd32
6 years ago

  • Keywords needs-patch added

This is correct, The port must be included in the Host header, unless it's the default for the protocol, in which case it's optional. http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23 describes that for HTTP/1.1, which is almost the same as 1.0.

Although we should be able to send the default port in 100% of cases, We should probably only send it when it's not optional to avoid any unexpected servers.

The logic needed would be:

  • If proxy enabled, always include port
  • If ( (http == scheme and port == 80 ) or (https == scheme and port == 443 ) ) drop port
  • else: Include port
Last edited 6 years ago by dd32 (previous) (diff)

@kamelkev
6 years ago

#5 @kamelkev
6 years ago

Hi Dion,

Makes sense. I omitted the portion relating to "proxy enabled" in the previous patch, I have attached another that should be closer to what you described, and matched the project whitespace style.

Last edited 6 years ago by kamelkev (previous) (diff)

#6 @dd32
6 years ago

  • Milestone changed from Awaiting Review to 4.1

#7 @dd32
6 years ago

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

In 29852:

When making a HTTP request to a non-standard port, include the port in the Host header for the Streams HTTP transport. This bring parity to the cURL transport and respects the HTTP RFC.
Props kamelkev for the initial patch; Fixes #28982

Note: See TracTickets for help on using tickets.