odan / slim4-skeleton

A Slim 4 Skeleton
https://odan.github.io/slim4-skeleton/
MIT License
439 stars 80 forks source link

Settings file depends on superglobal $_ENV #62

Closed DigiLive closed 3 years ago

DigiLive commented 3 years ago

https://github.com/odan/slim4-skeleton/blob/2f635b50c52d517236a4addcbe5f52befcc25a0f/config/settings.php#L13-L16

$_ENV might not always be populated, depending on php.ini directive variables_order and/or cli mode + os.

variables_order string

Sets the order of the EGPCS (Environment, Get, Post, Cookie, and Server) variable parsing. For example, if variables_order is set to "SP" then PHP will create the superglobals $_SERVER and $_POST, but not create $_ENV, $_GET, and $_COOKIE. Setting to "" means no superglobals will be set.

See: https://www.php.net/manual/en/ini.core.php#ini.variables-order

It seems function getenv() is to be the most solid way to get a value of environment variables. See: https://www.php.net/manual/en/function.getenv

I think it's more fail-save to add the following to defaults.php:

// Environment settings
$settings['environment'] = $_GET['APP_ENV'] ?? getenv('APP_ENV');

and line 14 to 16 of settings.php to:

if ($settings['environment'] !== false) {
    require __DIR__ . '/local.' . $settings['environment'] . '.php';
}

You could also evaluate the return-value of getenv('APP_ENV') everywhere where needed, but imho that creates overhead.

odan commented 3 years ago

Hi @DigiLive

Please note: Your example code contains a security issue, because somebody could change the environment from outside with a query string like https://www.example.com/?APP_ENV=development.

$settings['environment'] = $_GET['APP_ENV'] ?? getenv('APP_ENV');

My objective is to use a thread safe method to get my env vars. getenv is not thread-safe and thus I try not to use it anymore. The discussion about that feature seems to be summarized in this statement: vlucas/phpdotenv#446 (comment).

Indeed, the population of superglobals like $_SERVER and $_ENV can depend on the server's configuration (variables_order in php.ini). To make it "really" work a code like this should work. This example uses getenv only as "fallback" when nothing works.

So simplified to our use case it should work like this:

$environment =  $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? getenv('APP_ENV');
DigiLive commented 3 years ago

Of course it was supposed to be $_ENV.

I have absolutely no idea why I placed $_GET in there. 😳 I guess I was too busy with my mind mastering Slim 4 a bit. Anyway... My bad!

For now I'll be using: $environment = $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? getenv('APP_ENV'); until I safe way presents itself.

Thank you very much. While I don't have the intention to spam issues, I hope you don't mind my posting many comments and questions while I go trough the code of this skeleton. I'm determined to understand how it operates.

odan commented 3 years ago

No problem. I like good questions :-)