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

when ENVIRONMENT=development is set, the server does not work on https #16

Closed
ylebre opened this issue Oct 2, 2020 · 5 comments
Closed
Assignees
Milestone

Comments

@ylebre
Copy link
Member

ylebre commented Oct 2, 2020

There is an issue with the following bit of code:

/*/ Make sure HTTPS is always used in production /*/
$scheme = 'http';
if (getenv('ENVIRONMENT') !== 'development') {
    $router->map('GET', '/{page:(?:.|/)*}', HttpToHttpsController::class)->setScheme($scheme);
    $scheme = 'https';
}

I would expect https to work as well with the development environment, but because all the routes are tied to $scheme, they are only available on http.

@Potherca
Copy link
Member

Potherca commented Oct 7, 2020

That code was originally created to enable development before a (Docker) HTTPS server was available as the internal PHP HTTP server does (explicitly) not support HTTPS.

It might be more sensible to change the getenv('ENVIRONMENT') check to something in the direction of "unsafe" rather than forcing all development environments to be unsafe...

@Potherca Potherca modified the milestones: v0.3.0, v0.4.0 Oct 7, 2020
@ylebre
Copy link
Member Author

ylebre commented Oct 8, 2020

Maybe we can just check for env http or https to make it explicit? and even add routes for both schemes.

@Potherca
Copy link
Member

Potherca commented Oct 15, 2020

Strictly speaking, there shouldn't be any HTTP at all. It should just always redirect to HTTPS.

At this point, I'm inclined to just remove the HTTP code entirely...

@ylebre
Copy link
Member Author

ylebre commented Oct 17, 2020

I agree, having a secure default is the better option here, so we could just remove the http scheme completely. If someone really needs http:// it should be easy enough to modify that in the code.

@Potherca
Copy link
Member

We had a discussion offline, the current conclusion is as follows:

  1. Instead of using ->setScheme($scheme) for evey route or route-group, we remove them everywhere. This makes the routes scheme-agnostic.
  2. We add a check that forces the use of HTTPS (via a redirect on non-https). This check can be enabled (or disabled) through a config/env var.

This means that unsafe (i.e. HTTP) URLs are supported, in case HTTPS is provided through a server-side proxy.

The question that remains is whether it makes more sense to have the HTTPS check on or off.

Thoughts? @ylebre / @poef / @michielbdejong

@Potherca Potherca self-assigned this Oct 23, 2020
@Potherca Potherca modified the milestones: v0.4.0, v0.5.0 Oct 23, 2020
@Potherca Potherca mentioned this issue Nov 11, 2021
6 tasks
@Potherca Potherca modified the milestones: v0.5.0, v0.6.0 Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants