fsockopen logic bug

[amandato]

I came across this while debugging a handful of users who had an issue with our service. Currently the Transport/fsockopen.php library is including the connection port to the HTTP headers for https, even though it is using the default port 443.

Referencing the RFC:

A "host" without any trailing port information implies the default port for the service requested (e.g., "80" for an HTTP URL).

Since https has a default port of 443, I would recommend the following change attached as a patch to the request() function in fsockopen.php

Note: it is possible that the service http is used on port 443, as well as https on port 80. This is why the logic also checks the 'scheme'.

I've provided a readable patch how this can be written. How it is written ultimately could change.

Also note that as-is you are not violating the RFC, but also not following standard practice of leaving the port off when it is the default port for the service / 'scheme'.

Before WordPress 4.6 the behavior was to not include the port for https when port 443 is used. I will be submitting a 2nd trac that ties into this one as well.

[amandato]

Patch for setting host: without port for http and https when default port is used

[dd32]

Hi @amandato and welcome to Trac.

Thanks for reporting this, In WordPress 4.6 we're using the external package ​Requests and issues/patches such as these need to be made against that upstream package.

I'll take a look into this and open a PR upstream.

Cross-referencing #37992 also.

[dd32]

​https://github.com/rmccue/Requests/pull/238

[amandato]

Great!

I did some testing for situations such as custom scheme, no scheme and custom ports with http and https and found my original patch had an error. I rewrote the patch using a switch case as it better handles the situations that can happen. Will upload patch here as well as on the github thread. (see my latest attachment)

test cases:

http://example.com/ (Host: example.com)
http://example.com:443/ (Host: example.com:443)
http://example.com:123/ (Host: example.com:123)
https://example.com/ (Host: example.com)
https://example.com:80/ (Host: example.com:80)
https://example.com:400/ (Host: example.com:400)
foo://example.com/ (Host: example.com)
foo://example.com:443/ (Host: example.com:443)
foo://example.com:999/ (Host: example.com:999)
//example.com/  (Host: example.com)
//example.com:443/ (Host: example.com:443) - this one is up for comment if port should be specified if SSL is used. I believe the port should be specified, we cannot predict why a custom/blank scheme would be using 443.
//example.com:999/ (Host: example.com:999)

[amandato]

Latest patch, written as a switch case statement which handles the situations more efficiently

[rmccue]

PR merged; needs update in core.

[dd32]

#38163 was marked as a duplicate.

[dd32]

Moving to 4.6.2 along with all the other HTTP API fixes.

[dd32]

In 38727:

HTTP: Update Requests to master (0048f3c) which fixes a number of outstanding issues.

Fixes #38070, #37733 by reverting part of [38429] and using the fix in Requests.
Fixes #37992 allowing for connecting to SSL resources on ports other than 443.
Fixes #37991 by not sending default ports in the Host: header.
Fixes #37839 to match and decode Chunked responses correctly.
Fixes #38232 allowing a SSL connection to ignore the hostname of the certificate when verification is disabled.

[dd32]

In 38728:

HTTP: Update Requests to master (0048f3c) which fixes a number of outstanding issues.

Merges [38727] to the 4.6 branch.

Fixes #38070, #37733 by reverting part of [38429] and using the fix in Requests.
Fixes #37992 allowing for connecting to SSL resources on ports other than 443.
Fixes #37991 by not sending default ports in the Host: header.
Fixes #37839 to match and decode Chunked responses correctly.
Fixes #38232 allowing a SSL connection to ignore the hostname of the certificate when verification is disabled.

[dd32]

#36253 was marked as a duplicate.