Incorrect scheme/port when using a proxy-server

[mindplay-dk]

FYI

kodus#1

[drupol]

Hi,

I'm working at the European Commission and we also have the same issue.

I'm going to test with your fork and report back here.

[drupol]

Allright,

Prior testing your fork I tested the dev-master branch of this package and the issue with proxy-server and HTTPS are fixed on our side.

I let @Nyholm know about this and he will tag a new release today (0.4.0).

@mindplay-dk It would be super nice if you could test your issues with 0.4.0 and report back here, if your issue is still valid.

I'm now able to test myself because I'm not having the same testing infrastructure of the clients and I cannot ask them to test this for me unfortunately.

Thanks in advance.

[drupol]

Dear @mindplay-dk,

I reviewed your PR and I have a question.

Why do you include:

if (isset($server['SERVER_PORT'])) {
    $uri = $uri->withPort($server['SERVER_PORT']);
}

In the else part of the new condition that you are introducing ?

Shouldn't it be out of it ? Like this:

if (isset($server['HTTP_X_FORWARDED_PROTO'])) {
    $uri = $uri->withScheme($server['HTTP_X_FORWARDED_PROTO']);
} else {
    if (isset($server['REQUEST_SCHEME'])) {
        $uri = $uri->withScheme($server['REQUEST_SCHEME']);
    } elseif (isset($server['HTTPS'])) {
        $uri = $uri->withScheme('on' === $server['HTTPS'] ? 'https' : 'http');
    }
}

if (isset($server['SERVER_PORT'])) {
    $uri = $uri->withPort($server['SERVER_PORT']);
}

[Zegnat]

My guess is that there is no forwarded port information available and he found SERVER_PORT to only be reliable in non-forwarded cases. By keeping it inside the else you make sure not to assume a port for the scheme that is being forwarded.

[mindplay-dk]

According to the PHP manual:

['SERVER_PORT'] can be spoofed and it may or may not return the physical port value. It is not safe to rely on this value in security-dependent contexts

So probably like @Zegnat says.

I honestly don't recall the details almost a year later.

There is test coverage and several large sites have been running with NGINX proxies on this fork of the package for a while, so that should give you some reassurance.

If you wanted to be more explicit, you might even check for the presence of a non-trustworthy SERVER_PORT and throw an exception, not sure.

[drupol]

Hey !

Thanks for coming back after a year 👍

I've submitted a PR that should fix all of this in a more reliable (complete) way, see #32

Let me know what you think ?

[mindplay-dk]

@drupol to be honest, I can't tell how this makes it more reliable - you didn't add any tests, you only changed a value in the test-conditions from 1 to off, which sounds to me like you reversed one of the test-conditions?

You also didn't add a regression test for the missing feature, assuming your code solves the same problem? Honestly, this is just very far away in my mind at this point, and I don't currently use the package, so it doesn't make sense for me to get deep into this again now.

(on a personal note, I don't work there anymore, but I would probably continue to choose the kodus/psr7-server fork myself, because it didn't seem like this project was really being maintained - and I know that the fork is actively used on more than a dozen of the largest news sites in Denmark, so I'm more comfortable trusting this would be maintained.)

[drupol]

@drupol to be honest, I can't tell how this makes it more reliable - you didn't add any tests, you only changed a value in the test-conditions from 1 to off, which sounds to me like you reversed one of the test-conditions?

True, this should have been part of another PR. Let me explain.
In the tests fixtures, the key 'HTTPS' is set to '1', it means that it should be in HTTPS.
See that in the fixture: https://github.com/Nyholm/psr7-server/blob/master/tests/ServerRequestCreatorTest.php#L347
Then this line compare the URL which should be HTTPS but it's in HTTP: https://github.com/Nyholm/psr7-server/blob/master/tests/ServerRequestCreatorTest.php#L393
Normally if HTTPS is set to 1, then it must be in HTTPS, but in the test it's not.

You also didn't add a regression test for the missing feature, assuming your code solves the same problem? Honestly, this is just very far away in my mind at this point, and I don't currently use the package, so it doesn't make sense for me to get deep into this again now.

I agree, "reliable" was maybe not the best term in here, sorry about that, maybe more "complete" would have been better. Bear with me, I'm not a native english speaker.

What I really meant is that the PR I submitted includes check against those HTTP headers:

• HTTP_X_FORWARDED_PROTO
• HTTP_X_FORWARDED_PROTOCOL
• HTTP_X_FORWARDED_SSL
• HTTP_FRONT_END_HTTPS
• HTTP_X_URL_SCHEME

That list is based on: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Proto

Those headers are not taken in account in your PR which only uses HTTP_X_FORWARDED_PROTO.

(on a personal note, I don't work there anymore, but I would probably continue to choose the kodus/psr7-server fork myself, because it didn't seem like this project was really being maintained - and I know that the fork is actively used on more than a dozen of the largest news sites in Denmark, so I'm more comfortable trusting this would be maintained.)

I do agree and understand, that's the OpenSource spirit :-)

That said, if we could join our forces and make the original package better, it would be even better.

I'm working at the European Commission and it's full of reverse proxies, nginx and stuff, so it's important for me to have this fixed in the original package.

[mindplay-dk]

As I recall, some of those headers were rejected because the office network/security/HTTP/proxy expert (who I'd trust with my life on these matters) told me these were basically relics, long-since superseded by standards or de-facto standards.

Unless or until I've seen an actual real-world requirement for something more exotic, personally, I'd never even consider it. If I were to consider it, I'd have to do considerable research (investigate real-world proxies, etc. - which I did when I made those changes) and add test-cases to prove it.

Merely adding a bunch of fallbacks because "this might work" is not a safe approach, in my opinion - could lead to unpredictable bugs or security issues I haven't thought of.

Again, just my two cents, but we were always very conservative about adding code at my last work-place, and I'd rather trust a library that has only exactly what's needed to satisfy a small set of known requirements - I never add lines of code for anything until there's a demonstrated need. YMMV. 🙂

[drupol]

Allright, thanks for the input, I do share most of the points. Maybe there are more exotic headers, I don't know. However, I think that the one that I have added in my PR are still used nowadays.

Now the issue is to see if @Nyholm wants to see them in his package or not then :-)

[mindplay-dk]

Quick glance at Wikipedia, for example:

X-Forwarded-For ... Superseded by Forwarded header.
X-Forwarded-Host ... Superseded by Forwarded header.
X-Forwarded-Proto ... Superseded by Forwarded header.

So Forwarded is what proxies should be using.

Could there be older software using non-standard headers for historical reasons? Sure, of course. Those probably shouldn't be in production - I'm certainly not convinced we're helping anyone by enabling them to put it off when launching new services.

Now the issue is to see if @Nyholm wants to see them in his package or not then :-)

I think you're stilling missing the tests to at least demonstrate that this works.

Also I'd encourage someone else to carefully review that change to the tests - I don't understand it and I'm not convinced that changing the test-condition was correct. (But I'm also not willing to spend anymore time on it, sorry.)

[Nyholm]

I agree with many of Rasmus’ concerns. I would be happy to merge a PR like kodus#1

[drupol]

Allrighty, I cherry-picked this commit (kodus@a9ed564).

Let me know if I should do something else to get this done :-)

[drupol]

I have a question though.

When requesting an HTTPS url, the default port is 443.

From Wikipedia:

Port numbers are sometimes seen in web or other uniform resource locators (URLs). By default, HTTP uses port 80 and HTTPS uses port 443, but a URL like http://www.example.com:8080/path/ specifies that the web browser connects instead to port 8080 of the HTTP server.

In this case, the test add those two $_SERVER key parameters:

• 'HTTP_X_FORWARDED_PROTO' => 'https'
• 'SERVER_PORT' => '80'

When I read this, I understand that we are overriding the default HTTPS port(443) in favor of 80, and the resulting URL should be (as weird as it can be): https://www.blakesimpson.co.uk:80/blog/article.php?id=10&user=foo

In this case, the test validate the URL to: https://www.blakesimpson.co.uk/blog/article.php?id=10&user=foo and those kind of URL uses the port 443, which is different from the parameter SERVER_PORT that we are using.

I think that there is something wrong here, could you confirm or not ?

[drupol]

I guess we can now close this, I'll re-open another issue for the $_SERVER['HTTPS'] stuff.