Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made HTTPS detection a bit better. #160

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

Frzk
Copy link

@Frzk Frzk commented Mar 30, 2019

Introduced a new private method isHttps that checks if the request was sent over HTTPS or not.
To do so, it checks if one of the following is true:

  • URI scheme is 'https',
  • a 'X-Forwarded-Proto' header is present and its value is 'https' (useful when running behind a load balancer).

Introduced a new private method `isHttps` that checks if the request was sent over HTTPS or not.
To do so, it checks if one of the following is true:
  - URI scheme is 'https',
  - a 'X-Forwarded-Proto' header is present and its value is 'https' (useful when running behind a load balancer).
This made the tests in Travis-CI fail.
@alokdhir
Copy link

Please merge. This is crucial for our env; otherwise we will have to deploy all our code with secure->false.

@tuupola
Copy link
Owner

tuupola commented Apr 22, 2019

@alokdhir technically there is almost no difference. The deployment is still unsecure if it trusts an user settable X-Forwarded-Proto header. Request itself still is not https.

@Frzk Sorry did not notice this earlier. Trusting headers should not be enabled by default. There was a similar PR for slim-basic-auth. Configuration should be same as this, ie adding headersto the relaxed setting.

$app->add(new Tuupola\Middleware\JwtAuthentication([
    "path" => "/admin",
    "secure" => true,
    "relaxed" => ["localhost", "headers"],
]));

@Frzk
Copy link
Author

Frzk commented May 21, 2019

@tuupola : I guess it makes sense :-)

Maybe we could figure out a better way to handle these edge cases with support for X-Forwarded-* headers, but also for RFC 7239 (Forwarded header) in a dedicated option ?

I was thinking about something like:

$app->add(new Tuupola\Middleware\JwtAuthentication([
    "path" => "/admin",
    "secure" => true,
    "trustedProxies" => [
        "proxies": [192.0.2.1, 192.0.2.16],
        "headers": ["X-Forwarded-Proto", "Forwarded"]
    ],
]));

@kgrosvenor
Copy link

kgrosvenor commented Apr 25, 2020

Has anyone tried this with jwilder/nginx-proxy? Using the secure option doesnt work for me, then again i am forcing https anyway so is there any point in me bothering @tuupola? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants