Using builds.piwik.org probably triggers a false positive here as the URL is also available without https!

Look at this minimum PHP example to trigger the Location header:

$fsock = fsockopen('piwik.org', 80, $errno, $errstr);
fwrite($fsock, "GET / HTTP/1.0\r\nHost: piwik.org\r\nConnection: close\r\n\r\n");
while(!feof($fsock)) { echo fgets($fsock, 1024); }

You will receive the Location header here as seen in the tests. The weird part I found here is that Http::sendHttpRequestBy ignores https and connects using port 80. However if we would fix that to get the following snippet:

$fsock = fsockopen('ssl://piwik.org', 443, $errno, $errstr);
fwrite($fsock, "GET / HTTP/1.0\r\nHost: piwik.org:443\r\nConnection: close\r\n\r\n");
while(!feof($fsock)) { echo fgets($fsock, 1024); }

Still those location headers!

builds.piwik.org behaves as expected but looking at it being reachable via both http and https while piwik.org is broken it still looks fishy...

I have to amend the above comment with an example not triggering an error:

$fsock = fsockopen('ssl://piwik.org', 443, $errno, $errstr);
fwrite($fsock, "GET / HTTP/1.0\r\nHost: piwik.org\r\nConnection: close\r\n\r\n");
while(!feof($fsock)) { echo fgets($fsock, 1024); }

The port in the host header triggered the redirect!

Could you change your PR to include a modification of sendHttpRequestBy to incorporate the SSL port and ssl:// to see if nothing breaks?

Hi @mneudert

Could you change your PR to include a modification of sendHttpRequestBy to incorporate the SSL port and ssl:// to see if nothing breaks?

as on a quick read I'm not fully understanding what i should do, would be awesome if you could find some time to try this 🙂

@mneudert Will release 3.1.1 shortly. Should we merge this PR prior or wait for the next release?

Looks good Marc, I'll merge as it may help users auto-update and looks safe

I hope this goes well :D

The problem is with the last commit. It makes the test green, but as far as I know it may violate the HTTP specification.

The problem is that the Host header should contain a port if it differs from 80 (see spec here). It does not specifically mention you can just ignore the port for https.

So if we keep this one we should definitely be aware of potential side effects that may come up! There are places on the internet that say the modified behaviour is correct (like here) but I would still keep an eye on this.

@mneudert in which cases would the new implement create bugs, is it only when the HTTPS port is different from 443?

From my understanding before this change every socket connection has always been made using HTTP even if HTTPS was requested.

As HTTPS behaves a bit differently we might see something come up like certificate problems.

But as I just looked up the circumstances necessary for a socket connection to be used I would not worry at all. Having all of "no curl installed", "allow_url_fopen forbidden" AND some "HTTPS certificate issue" at once should be nearly impossible :D