[5.8] Reworked env to allow getenv/putenv to be disabled

[GrahamCampbell]

It is currently not easily possible to disable putenv and getenv. It is now super easy to disable this by adding Illuminate\Support\Env::disablePutenv(); to your bootstrap/app.php file.

This is particularly important for Lumen apps, where config caching is not available. This PR can be regarded as a security fix affecting Lumen apps running on server shared with any other sites.

---

I really do think that in Laravel 5.9 we should toggle the default from putenv being enabled to being disabled, in the interest of security. Now that it can easily be turned on in the boostrap file, we no longer have the developer experience issues we had before.

[danilopinotti]

Any tests?
Any additional tests?

[GrahamCampbell]

Yes, there are. They're still there.

[deleugpn]

I don't get why this is still being pushed. From last time it seems there's a very large community relying on the current behavior for thousands of PHP libraries.
I don't think any PHP framework alone will be enough to rollout this change to the PHP community. Perhaps proposing a PSR for a component similar to Laravel's Config Repository and standarizing the flow from environment variable to configuration data would have a higher chance.

[GrahamCampbell]

From last time it seems there's a very large community relying on the current behavior for thousands of PHP libraries.

And all these library authors are calling a function that is not thread-safe. We have to try and make them aware. I mean, the docs already have a big ass warning, but people still blindly use it anyway...

Note that this PR does not change the default behaviour, it just allows people to make their Lumen apps secure by disabling putenv and getenv (if the app author specifically chooses to).

[GrahamCampbell]

Perhaps proposing a PSR for a component similar to Laravel's Config Repository and standarizing the flow from environment variable to configuration data would have a higher chance.

I disagree. The ONLY problem here is that most people are not aware that getenv and putenv are not the best way to get and set environment variables, because they are not thread-safe. There is no need to force config caching or other patterns and interfaces on people. Just use the proper superglobals instead of the functions.

[deleugpn]

There is no 'proper' superglobal for library development. They're not reliable[1] for framework agnostic or context-agnostic packages such as AWS SDK or thousands of similar packages. getenv is the best bet even for people using pthreads, which is also not in the majority.

It's also pretty presumptuous of you to think that thousands of package developers, including major fortune 500 companies are all doing it wrong and you single-handedly knows what's right.

There is indeed an annoyance around how php handles environment variables, but its not up to Laravel to try and fix a language constraint.

[1] https://mattallan.me/posts/how-php-environment-variables-actually-work/

[taylorotwell]

Can you send this to master where we already have Illuminate\Support\Env.

[GrahamCampbell]

@taylorotwell I'd rather we actually brought this in to 5.8, since it's not just a refactoring, it's a security fix for Lumen apps.

[GrahamCampbell]

I can resolve the merge conflicts 5.8 -> master for you if you'd like.

[sisve]

And all these library authors are calling a function that is not thread-safe.

I would like to remark on the tread-safety here. These methods are totally thread-safe; the problem is that they change a process-wide environment. Threads within the same process will see the side-effects of calling these methods, but they work as intended and are thread-safe.

The problem here seems to be apache users that uses a mpm that executes several requests within the same process, so they share the environment variables. Couldn't we just educate people, tell them to use fastcgi and the php-fpm instead? (I believe setting up different pools in fpm is also the way to have your sites running under different user accounts so that one wordpress site you are being forced to maintain doesn't infect all other sites...)

If we should consider this as a security fix; then the secure mode should be default. The secure mode in this issue is to disable getenv/putenv, which will break existing installations.

[GrahamCampbell]

It is only a security fix for those not using config caching, or those using Lumen. I don't think we should change any defaults in Laravel 5.8.

[GrahamCampbell]

Couldn't we just educate people, tell them to use fastcgi and the php-fpm instead? (I believe setting up different pools in fpm is also the way to have your sites running under different user accounts so that one wordpress site you are being forced to maintain doesn't infect all other sites...)

Actually fpm is vulnerable to this too. We have had many, many reports of variables leaking in our issue tracker over the years.

[sisve]

If the fpm has this behavior then I've misunderstood the entire problem. The fpm will boot up a new process for every request, and the different processes should not share the environment space. If the problem is reproducible on fpm, that implies that nginx users are also affected. This makes me slightly worried since I'm using nginx+fpm in production. This also increases the scope of the issue from some apache configurations, to almost all apache+nginx configurations.

Could you point me to an issue or any information on how to reproduce the problem on fpm so that I can verify the problem (and thus the fix) in my production environment?

[thecrypticace]

FPM does not boot up a process for every request. FPM workers can and do handle multiple requests (one at a time per worker). The default setup is that workers live indefinitely but can be changed such that a worker handles at most N requests and then is restarted. This is controllable by the pm.max_requests setting in the FPM config file.

Edit:
Additionally, FPM uses a forked process model. This means that the parent and child "share" memory. I believe FPM's forking model means that the environment is copied not shared so issues should only arise w/in the same process but I'm not 100% positive about this.

[taylorotwell]

Feel free to send to master and I am fine this functionality. We are just a few weeks from that release.