BUG Replace phpdotenv with thread-safe replacement #7484

tractorcow · 2017-10-16T04:00:18Z

Not strictly breaking, but any code which uses getenv php method will remain vulnerable until upgrading to the new Environment::getenv method.

In order to be as unobtrusive as possible the getenv and putenv methods on Environment class are designed as drop-in replacements for the global php methods.

Thanks to @NightJar for beginning this solution (see NightJar@e614ede).

Other PRs for various modules incoming to replace getenv usages.

Other prs to merge after this:

References to this issue (notably laravel based)

zanderwar

👍

dhensby · 2017-10-16T15:45:01Z

I'm not really behind this fix as the whole point in using an external library is so that we aren't maintaining this kind of code ourselves.

from my understanding putenv and getenv are not threadsafe methods and so this error is only happens on multi-threaded environments (IIS, HHVM and multithreaded apache on windows).

Have we investigated using Dotenv\Loader::setEnvironmentVariable and Dotenv\Loader::getEnvironmentVariable instead as they rely on the super globals first...

zanderwar · 2017-10-16T19:50:51Z

The external library is not intended for production, so using it in the first place only satisfies the environment when in development albeit is not in require-dev and as such is adding unnecessary bloat to production.

IMHO the dotenv dependency should be dropped entirely for methodology like this. Even if I am being biased here because this resolves my issue of wanting to use the .env file like laravel/symfony allow flawlessly

tractorcow · 2017-10-16T20:12:32Z

Our external library is parse_ini_string :)

flamerohr

A minor request, it's not blocking for this to be merged

flamerohr · 2017-10-16T20:22:22Z

src/Core/Environment.php

+        $variables = parse_ini_string($contents) ?: [];
+        foreach ($variables as $name => $value) {
+            // Don't overload vars
+            if (static::getenv($name) === false) {


could we have overloading vars as a second param for this method?

Yes that's a good idea thank you. :)

sminnee

Reviewed, few stylistic comments. Hand-rolling our own dotenv implementation surprised me a bit – we're trying to reduce wheel reinvention. Sure it's not lengthy code, but it's the kind of regex soup that can end up having bugs.

sminnee · 2017-10-16T21:42:26Z

src/Core/Environment.php

+     *
+     * @param string $string Setting to assign in KEY=VALUE or KEY="VALUE" syntax
+     */
+    public static function putenv($string)


Do we need this to be our API rather than setEnv($k, $v)? Who would you recommend calls putenv?

Having looked at the other code that calls it, it seems making the setEnv API public and removing putenv would be more appropriate to developers.

I've ensure that putenv() promises to be a drop-in replacement for the core putenv. Mirroring the API means that users don't have to do any string-manipulation in their usercode. As a developer I love the peace of mind of an api promising to work the same way; Less for me to think about and re-test. :)

I don't see much value in that. People will still have to change their code, and if they're going to change their code we may as well give them the API they're more likely to want, which is setEnv.

If you look at all our putenv calls we're doing string concatenation in them now, so providing setEnv() isn't going to force people to do string manipulation.

If we had 5 years of code built around putenv there might be a stronger argument for it, but we don't.

ok, fair enough.

Shall I make putEnv and setEnv both public?

Seems like a reasonable compromise.

sminnee · 2017-10-16T21:42:38Z

src/Core/Environment.php

+     * @param string $name
+     * @return mixed Value of the environment variable, or null if not set
+     */
+    public static function getenv($name)


Can we make this camel case, so getEnv()?

This api mirrors the old global getenv, so I've opted to maintain the semantics as close as possible, including case name.

We can make it getEnv

sminnee · 2017-10-16T21:43:06Z

src/Core/Environment.php

+     * @param string $path Path to the file
+     * @return array List of values parsed as an associative array
+     */
+    public static function putenvFromFile($path)


Why have we reimplemented dotenv?

Pretty much!

Wouldn't it be better to retain a dotenv dependency (either vlucas or symfony) and make reference to its loader within Environment?

I'll have a look into symfony.

Symfony seems to do exactly the same thing; symfony dotenv simply uses core-php getenv. Given our implementation is small and works for us, I suggest to leave it as is.

sminnee · 2017-10-16T21:43:54Z

src/includes/constants.php

@@ -58,39 +57,39 @@
 }

 // Allow a first class env var to be set that disables .env file loading
-if (!getenv('SS_IGNORE_DOT_ENV')) {
+if (!Environment::getenv('SS_IGNORE_DOT_ENV')) {


This should probably still be getenv, otherwise it's a bit of a circular dependency, and this var can't be set by .env

This behaviour is required in case user-specific bootstrap files set environment vars.

Note that Environment::getenv doesn't actually require .env files to be loaded; It also acts as an accessor to the underlying system environment. This just maintains consistency between all getenv accessors.

The risk of using old getenv is that if a user does custom loading of env vars in user-code or custom index.php, then that call is going to be vulnerable to the same race condition we are trying to address.

Fair enough

tractorcow · 2017-10-17T00:06:23Z

Ok, all feedback has been implemented and also on all other repos.

flamerohr · 2017-10-17T07:31:15Z

docs/en/00_Getting_Started/03_Environment_Management.md

@@ -32,13 +32,18 @@ You can set "real" environment variables using Apache. Please

 ## How to access the environment variables

-Accessing the environment varaibles is easy and can be done using the `getenv` method or in the `$_ENV` and `$_SERVER`
-super-globals:
+Accessing the environment varaibles should be done via the `Environment::getenv()` method


referencing old API, maybe double check all docs :)

flamerohr · 2017-10-17T07:32:13Z

docs/en/04_Changelogs/4.0.0.md

@@ -486,6 +486,8 @@ SS_BASE_URL="//localhost/"
 The global values `$database` and `$databaseConfig` have been deprecated, as has `ConfigureFromEnv.php`
 which is no longer necessary.

+To access environment variables you can use the `SilverStripe\Core\Environment::getenv()` method.


same in changelogs

dhensby · 2017-10-17T11:21:30Z

src/includes/constants.php

@@ -58,39 +57,39 @@
 }

 // Allow a first class env var to be set that disables .env file loading
-if (!getenv('SS_IGNORE_DOT_ENV')) {
+if (!Environment::getEnv('SS_IGNORE_DOT_ENV')) {


Just revisiting what @sminnee said - this should only ever be set by a global bonefido environment variable that won't change from execution to execution. As the comment says "Allow a first class env var to be set that disables .env file loading" but this allows second-class / psuedo environment variables to manipulate/unset this value which is the opposite of what's intended here.

but this allows second-class / psuedo environment variables to manipulate/unset this value which is the opposite of what's intended here.

getenv allows exactly the same if you use putenv, so what's the introduced risk that didn't exist before?

dhensby · 2017-10-17T11:24:42Z

src/Core/Environment.php

+        // If PHP is running as an Apache module and an existing
+        // Apache environment variable exists, overwrite it
+        if (function_exists('apache_getenv') && function_exists('apache_setenv') && apache_getenv($name)) {
+            apache_setenv($name, $value);


I don't understand the preference for handrolling our own dotenv work when this is practically identical to using Dotenv\Loader::setEnvironmentVariable and Dotenv\Loader::getEnvironmentVariable and means we don't have to maintain this code :/

This is what the setEnvironmentVariable is in the library: https://github.com/vlucas/phpdotenv/blob/828d19e597052ddeee65890bb2b1a0912d79fea8/src/Loader.php#L353-L375

It's super close to what we have just we also have an extra layer of the local static $env too.

just we also have an extra layer of the local static $env too.

That is the critical point of this PR. That's not simply a cache, that's the key feature that solves the process isolation issue.

dhensby · 2017-10-17T11:38:23Z

tests/php/Core/EnvironmentTest/test.env

@@ -0,0 +1,13 @@
+# Test file


There are no tests for nested variables, eg:

TEST="value" TEST_AGAIN="$TEST"

I didn't write any code to support it either.

Are nested variables a thing? :o

please don't let this be a thing... at least not here :P

My thoughts exactly. We're not implementing a POSIX shell.

This is a feature of the dotenv lib so if we're replacing it like-for-like it needs to be here.

I've restored this. ;)

dhensby · 2017-10-17T11:42:18Z

@zanderwar

The external library is not intended for production, so using it in the first place only satisfies the environment when in development albeit is not in require-dev and as such is adding unnecessary bloat to production.

I don't see how lifting the code from the library and putting it in our codebase makes this any more suitable for production. If we feel that this PR solves the problem, then using the built in methods (instead of php's getenv and putenv) will solve the problem too.

IMHO the dotenv dependency should be dropped entirely for methodology like this. Even if I am being biased here because this resolves my issue of wanting to use the .env file like laravel/symfony allow flawlessly

Laravel uses the same library for their .env so if you're happy with how laravel works then there should be no complaints. We could move to symfony's if it's more robust/resilient.

tractorcow · 2017-10-17T20:10:26Z

I don't see how lifting the code from the library and putting it in our codebase makes this any more suitable for production.

We aren't lifting it, we have made a completely unique implementation. We store the vars in a static $env to protected it from cross-process pollution (which apparently $_ENV and both $_SERVER were subject to in testing).

so if you're happy with how laravel works

I'm not happy, as even laravel / symfony suffer from the risks documented at https://gist.github.com/progmars/1e545d96dd48676a2aa7

tractorcow · 2017-10-17T20:24:39Z

Maybe someone who has an environment which can reproduce this issue (e.g. @zanderwar ) could take over development of this fix and see if they can minimise the fix to one that still solves the issue, but maybe without removal of the existing backend dotenv library?

tractorcow · 2017-10-17T20:27:05Z

Maybe we should request feedback from one of the laravel devs; Maybe they can provide some insight that I haven't seen.

tractorcow · 2017-10-17T20:57:40Z

Well, I've tracked down the appropriate laravel discussion.

laravel/framework#8191 (comment)

You should avoid calling env()in your application.

The way this is addressed in laravel is that the result of the combined config is cached, and thus there is no race condition when setting / loading env() vars, because getenv() is rarely called, and loading .env files is only done to populate this cache.

In the case of SilverStripe, what we could do is use .env to populate (and cache) config, and we use the config API.

We already use the back-tick operator to interpolate env / constants into config. What we could do is actually cache these within the config manifest (rather than lazy-evaluating on each request).

For example, this is how the default cache factory sets the temp path (cache.yml)

---
Name: corecache
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\Core\Cache\CacheFactory:
    class: 'SilverStripe\Core\Cache\DefaultCacheFactory'
    constructor:
      args:
        directory: '`TEMP_PATH`'

This could be solved in a number of ways:

Option 1

Move all used env vars to a config var on some relevant class
Update config module to interpolate these values during flush / cache generation
Replace all getenv() calls to the appropriate config calls

Option 2

Alternatively, we could not interpolate these variables, but instead create a separate config transformer which populates config directly from the environment into a holder class (e.g. SilverStripe\Core\Environment). Thus getenv('SS_DATABASE_CLASS') becomes Environment::config()->get('SS_DATABASE_CLASS').

tractorcow · 2017-10-17T20:59:20Z

@dhensby what do you think about these other options?

sminnee · 2017-10-17T21:23:25Z

We're close to an RC1 now is not the time to be radically re-thinking APIs.

Are we all 👍 on Environment::setEnv($key, $value) and Environment::getEnv($key) as the public API for this?

I love the idea of relying on the config's backtick system as much as possible, but that's a styleguide question, we'd still need the public API. I think that making more use of the backtick system is something that we can look at incrementing towards in 4.x.

I, like Dan, questioned why we rewrote the dotenv package instead of just including it as a dependency, but I don't think that an argument over that should have anything to do with the public API we expose. And, because of that, we could change this in 4.1 if we want.

I don't think that we should expose Dotenv\Loader::setEnvironmentVariable and Dotenv\Loader::getEnvironmentVariable as our public API as it ties us to that specific implementation, and if we realise (like we did a few days ago) that we have a problem with their API then we're stuffed until SS5. However, I think that Environment::setEnv() calling these methods behind the scenes would make a lot of sense.

Re-reading, I can see that Damian has found specific issues with the 3rd party implementations. I would say that we bless our current public API, go with Damian's implementation for now, and if we want, we can look at:

an upstream PR to one of the dotenv implementations that improves it in the way that Damian has done here
a 4.x change to SilverStripe to remove our internal implementation and switch back to a 3rd party dependency.

tractorcow · 2017-10-17T21:27:00Z

@sminnee pro-tip; If we switch to Environment::getEnv() now, we can still switch the back-end cache at some later point (e.g. 4.1).

sminnee · 2017-10-17T21:27:55Z

docs/en/00_Getting_Started/03_Environment_Management.md

-$_ENV['SS_DATABASE_CLASS'];
-$_SERVER['SS_DATABASE_CLASS'];
+use SilverStripe\Core\Environment;
+Environment::getenv('SS_DATABASE_CLASS');


sminnee · 2017-10-17T21:28:12Z

docs/en/00_Getting_Started/03_Environment_Management.md

+
+```php
+use SilverStripe\Core\Environment;
+Environment::putenv('API_KEY="AABBCCDDEEFF012345"');


setEnv('API_KEY', 'AABBCCDDEEFF012345')

sminnee · 2017-10-17T21:28:29Z

docs/en/00_Getting_Started/03_Environment_Management.md

-}
+use SilverStripe\Core\Environment;
+$env = BASE_PATH . '/mysite/.env';
+Environment::putenvFromFile($env);


putEnvFromFile

tractorcow · 2017-10-17T21:29:23Z

I'll fix up the docs now. :)

sminnee · 2017-10-17T21:29:57Z

tests/php/Core/Manifest/ConfigManifestTest.php

@@ -100,8 +100,7 @@ public function testModuleRules()

    public function testEnvVarSetRules()
    {
-        $loader = new Loader(null);
-        $loader->setEnvironmentVariable('ENVVARSET_FOO', 1);
+        Environment::putEnv('ENVVARSET_FOO=1');


setEnv('ENVVARSET_FOO', 1)

tractorcow · 2017-10-18T04:26:22Z

I'll defer to your better experience in this case. :)

sminnee · 2017-10-18T04:44:13Z

So in summary:

Environment::getEnv() uses getenv() as its starting point
.env files are loaded into static properties, and don't rely on putenv()
We recommend system-defined environment variables and not .env in prod environments

Right?

Perhaps we ditch the putenv() call to make it obvious that you should use Environment::getEnv() in your application code and not getenv(). In particular, if you're running with a .env file that means it will fail consistently, instead of only failing in a race condition, which seems like better developer experience.

dhensby · 2017-10-18T14:51:49Z

I don't think that we should expose Dotenv\Loader::setEnvironmentVariable and Dotenv\Loader::getEnvironmentVariable as our public API as it ties us to that specific implementation, and if we realise (like we did a few days ago) that we have a problem with their API then we're stuffed until SS5. However, I think that Environment::setEnv() calling these methods behind the scenes would make a lot of sense.

This is my view. We can use our own API which wraps the DotEnv library and even adds the local caching layer, but we shouldn't be removing it as a dependency at this time.

It also seems that this fix doesn't actually solve the problem anyway as we still rely on putenv (and getenv to an extent) which aren't thread-safe...

tractorcow · 2017-10-18T19:43:13Z

I may update so that Environment::setEnv() simply sets the $env static, and we don't modify the environment at all.

tractorcow · 2017-10-18T21:40:28Z

One more iteration, with these changes:

Move env file loading into EnvironmentLoader. It's <120 lines so I don't feel too bad about replicating dotenv library logic.
Implement variable interpolation at @dhensby's request.
No longer uses putenv(). Environment::setEnv() now exclusively sets a protected static var, which is used as the authoritative source for vars loaded from .env.
getenv() no longer loads variables loaded in this request; Environment::getEnv() must be used by ss applications.

sminnee · 2017-10-20T00:51:53Z

Final version looks good to me; let's merge this, assuming that it works for @zanderwar and @NightJar

NightJar · 2017-10-20T02:14:41Z

MY_SECRET="has a \" in it; because #edge-cases dominate my life."
This solution is well good enough for now, but I think we've lost sight of the goal a fair bit.
The goal of a .env file is to shift named strings into memory. No more no less. The same as getenv() would do. Is there any particular reason we need type checking & parsing, nested variables or any of the other features an external library such as dotenv provides us, and we've gone for feature parity with?

sminnee · 2017-10-20T02:22:02Z

@NightJar does the current solution fix the bugs that you experienced?

Also: in your comment above you didn't say whether or not that edge-case worked.

tractorcow · 2017-10-20T02:22:43Z

I think supporting escaped quotes can be a later release fix, if necessary.

tractorcow · 2017-10-20T02:40:07Z

Shall we escew dotenv / parse_ini_string() for say, one of the below:

Unlike the prior library, these support the ability for us to parse and extract, but not populate putenv.

@dhensby @sminnee opinions?

I'm not really interested in complicating my regexp any more.

tractorcow · 2017-10-20T03:21:58Z

Switched out env parser for m1/env. @dhensby see, no internal-env-parser implementation anymore. :) And feature parity.

NightJar · 2017-10-20T04:16:10Z

Attempting to test.

NightJar · 2017-10-20T04:48:06Z

This patch; good news.
Can replicate without.
Cannot replicate with.

zanderwar

Working for me 100%

sminnee · 2017-10-20T05:46:37Z

Merged, and merged 4.0 -> 4 as well on cms and framework

tractorcow · 2017-10-20T05:48:08Z

Yay thank you. :)

dhensby · 2017-10-20T08:45:30Z

Yay

GrahamCampbell · 2019-01-26T17:55:05Z

The current version of phpdotenv is robust and thread-safe. The following example will work in a multithreaded environment. This avoids using the adapter that would have called putenv and getenv, which are not threadsafe.

<?php

use Dotenv\Environment\Adapter\EnvConstAdapter;
use Dotenv\Environment\Adapter\ServerConstAdapter;
use Dotenv\Environment\DotenvFactory;
use Dotenv\Dotenv;

$factory = new DotenvFactory([new EnvConstAdapter(), new ServerConstAdapter()]);

Dotenv::create($path, null, $factory)->load();

tractorcow · 2019-01-28T19:49:19Z

That's excellent, thank you @GrahamCampbell. I think the approach we've gone with is the best for us, however, but we appreciate your getting back to us.

NightJar approved these changes Oct 16, 2017

View reviewed changes

tractorcow force-pushed the pulls/4.0/environment-getenv branch from ed6976f to 301a09b Compare October 16, 2017 04:44

zanderwar approved these changes Oct 16, 2017

View reviewed changes

flamerohr approved these changes Oct 16, 2017

View reviewed changes

sminnee requested changes Oct 16, 2017

View reviewed changes

tractorcow force-pushed the pulls/4.0/environment-getenv branch from 301a09b to 8e6bb43 Compare October 16, 2017 23:59

flamerohr suggested changes Oct 17, 2017

View reviewed changes

dhensby suggested changes Oct 17, 2017

View reviewed changes

sminnee reviewed Oct 17, 2017

View reviewed changes

tractorcow force-pushed the pulls/4.0/environment-getenv branch from 008bb3a to 883dda6 Compare October 18, 2017 21:37

tractorcow force-pushed the pulls/4.0/environment-getenv branch 2 times, most recently from 4dcfc5b to be85c99 Compare October 18, 2017 21:49

BUG Replace phpdotenv with thread-safe replacement

a5d8fcb

tractorcow force-pushed the pulls/4.0/environment-getenv branch from be85c99 to a5d8fcb Compare October 20, 2017 03:21

flamerohr approved these changes Oct 20, 2017

View reviewed changes

zanderwar approved these changes Oct 20, 2017

View reviewed changes

sminnee merged commit b9cb1e6 into silverstripe:4.0 Oct 20, 2017

tractorcow deleted the pulls/4.0/environment-getenv branch October 20, 2017 05:43

maxime-rainville mentioned this pull request Jan 31, 2019

Upgrade to vlucas/phpdotenv:^3.3 #8764

Closed

DracoBlue mentioned this pull request Jan 18, 2023

Replaced getenv/putenv with $_ENV and unset googleapis/google-auth-library-php#429

Closed

bshaffer mentioned this pull request Jan 25, 2023

feat: add support for using $_ENV instead of getenv/putenv googleapis/google-auth-library-php#432

Open

BUG Replace phpdotenv with thread-safe replacement #7484

BUG Replace phpdotenv with thread-safe replacement #7484

Conversation

tractorcow commented Oct 16, 2017 • edited Loading

zanderwar left a comment

Choose a reason for hiding this comment

dhensby commented Oct 16, 2017

zanderwar commented Oct 16, 2017 • edited Loading

tractorcow commented Oct 16, 2017

flamerohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightJar Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Oct 17, 2017

tractorcow commented Oct 17, 2017 • edited Loading

tractorcow commented Oct 17, 2017

tractorcow commented Oct 17, 2017

tractorcow commented Oct 17, 2017 • edited Loading

Option 1

Option 2

tractorcow commented Oct 17, 2017

sminnee commented Oct 17, 2017 • edited Loading

tractorcow commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Oct 17, 2017

Choose a reason for hiding this comment

tractorcow commented Oct 18, 2017

sminnee commented Oct 18, 2017

dhensby commented Oct 18, 2017

tractorcow commented Oct 18, 2017

tractorcow commented Oct 18, 2017

sminnee commented Oct 20, 2017 • edited Loading

NightJar commented Oct 20, 2017 • edited Loading

sminnee commented Oct 20, 2017 • edited Loading

tractorcow commented Oct 20, 2017

tractorcow commented Oct 20, 2017 • edited Loading

tractorcow commented Oct 20, 2017

tractorcow commented Oct 16, 2017 •

edited

Loading

zanderwar commented Oct 16, 2017 •

edited

Loading

tractorcow Oct 16, 2017 •

edited

Loading

sminnee Oct 16, 2017 •

edited

Loading

tractorcow Oct 16, 2017 •

edited

Loading

tractorcow Oct 17, 2017 •

edited

Loading

NightJar Oct 18, 2017 •

edited

Loading

tractorcow commented Oct 17, 2017 •

edited

Loading

tractorcow commented Oct 17, 2017 •

edited

Loading

sminnee commented Oct 17, 2017 •

edited

Loading

sminnee commented Oct 20, 2017 •

edited

Loading

NightJar commented Oct 20, 2017 •

edited

Loading

sminnee commented Oct 20, 2017 •

edited

Loading

tractorcow commented Oct 20, 2017 •

edited

Loading

sminnee commented Oct 20, 2017 •

edited

Loading