WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#37991 closed defect (bug) (fixed)

fsockopen logic bug

Reported by: amandato Owned by: rmccue
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.6.1
Component: HTTP API Keywords:
Focuses: Cc:

Description

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.

Attachments (2)

fsockopen.php.patch (748 bytes) - added by amandato 9 months ago.
Patch for setting host: without port for http and https when default port is used
fsockopen.php.2.patch (571 bytes) - added by amandato 9 months ago.
Latest patch, written as a switch case statement which handles the situations more efficiently

Download all attachments as: .zip

Change History (11)

@amandato
9 months ago

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

#1 @dd32
9 months ago

  • Component changed from General to HTTP API
  • Owner set to dd32
  • Status changed from new to accepted

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.

#3 @amandato
9 months ago

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)
Last edited 9 months ago by amandato (previous) (diff)

@amandato
9 months ago

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

#4 @rmccue
8 months ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner changed from dd32 to rmccue
  • Status changed from accepted to assigned

PR merged; needs update in core.

#5 @dd32
8 months ago

#38163 was marked as a duplicate.

#6 @dd32
8 months ago

  • Milestone changed from 4.7 to 4.6.2

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

#7 @dd32
8 months ago

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

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.

#8 @dd32
8 months ago

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.

#9 @dd32
8 months ago

#36253 was marked as a duplicate.

Note: See TracTickets for help on using tickets.