[5.8] Revert "[5.8] Allow retrieving env variables with getenv"

[GrahamCampbell]

Reverts #27958. People can already configure their apps to do this, without changing the core.

[GrahamCampbell]

I am strongly against Laravel calling putenv by default, as it will cause Laravel to leak environment variables to other websites on the server due non-threadsafety of that function. This will cause security problems for anyone who isn't familiar with the details of this, and hasn't cached the config, for whatever reason. It actually looks like this will be burning a lot of people, looking at the other issue that came up - lots of people are not caching config because they want the env variables populated!

[GrahamCampbell]

The arguments about breaking third party libraries are not valid in my opinion. The 3rd party library maintainers just need to be educated to not call functions that are not thread-safe, as explained by Sebastiaan Stok on Twitter.

[GrahamCampbell]

Note also, that worse than just variables leaking out of Laravel apps, variables can leak in to, since if anyone is using the inline variable syntax of phpenv, they will find that if another thread has set a variable, overiding the correct one, then the bad variable will be loaded, since the putenv adapter also calls getenv.

[driesvints]

Just left a comment here about this: #27949 (comment) and even though I still think this is the right way forward, it might be better to do this in a next release and provide people with enough time to update their libraries.

[mfn]

FWIF, I'm with @GrahamCampbell here.

The change is necessary, the breaking change was done on a point release, it was documented (and improved too since then) and I, too, think this should not have been reverted.

Christ, @taylorotwell was on holiday; give the men (and every wo/men on vacation) some rest! 😄

[mnabialek]

If decided to revert, shouldn't we revert also c37702c ? By the way it's a bit problematic when we call this factory in 2 completely different places and when any change is made we forget to update second place.

[kieranbrown]

I'm impartial to whether this gets reverted or not, however, if this does get reverted I think it needs to be highlighted in the documentation how to handle this in a production environment without config caching (Lumen).

As Sebastiaan Stok has highlighted it is recommended to use the $_ENV super variable over the getenv function, which is fine, but PHP's default production settings will not populate $_ENV.

If changing PHP ini settings is required to get this to work in Lumen then it needs to be documented.

I think we have to remember there is a vast skillset difference in the Laravel community and some users will need guiding with this kind of stuff 😄

[kieranbrown]

Also, if this does get reverted, the change will need replicating in Lumen's environment bootstrapper and env helper.

[matt-allan]

I originally opened #27958 because I thought it was an accidental change, but I do think that Laravel should default to calling putenv. By not making the environment variables accessible with getenv it's inconsistent with how real environment variables work in production.

The README of the dotenv project Laravel is using says this (Edit: it appears this section of the README was deleted after I wrote this):

phpdotenv is made for development environments, and generally should not be used in production. In production, the actual environment variables should be set so that there is no overhead of loading the .env file on each request.

If you are using a real environment variable in production, the variable is accessible with getenv. If the variable is not accessible with getenv in development, it's inconsistent. If the whole point of a .env file is to make it easier to set environment variables for development, it seems like it should work consistently with production.

If I understand correctly the security risk of leaking environment variables would only be an issue if you were using shared hosting and using .env files in production. In this scenario wouldn't environment variables in general be insecure?

If Laravel needs to cater to users that can't safely use environment variables it should offer an alternative way to keep secrets instead of changing the way .env is loaded IMO. There are already packages like laravel-credentials you can use instead.

Regarding the risk of a rouge thread calling putenv, wouldn't that same risk exist with real environment variables? I don't agree that Laravel needs to protect developers against this since the same risk exists with any code calling getenv and it's a natural consequence of how the putenv and getenv functions work.

[matt-allan]

This same line of code is duplicated in LoadEnvironmentVariables.php. You would also need to revert c37702c.

[deleugpn]

I see myself agreeing with both arguments made in the PR and when questioning myself why the only answer I have is context.
One group wants to fix the behaviour of Laravel's interaction with phpdotenv for production system. The other group wants an easy to use development tool.
There has to be a way to make both parties happy by declaring phpdotenv as a dev dependency and configuring it to use putenv during development while disallowing that process for production.
Looks like the vast majority of complaints won't hold up if you tell them they're forced to correctly set environment variables in their production environment while still allowing them to hack up local development.

[taylorotwell]

How do they do it without changing the core?

[taylorotwell]

Override the bootstrapper?

[GrahamCampbell]

Yeh, you can just define your own bootstapper, by extending the one from the core, and overring the method that makes a dotenv instance. That's why I made it it's own method.

[seriquynh]

I agree with GrahamCampbell about this change. Calling some functions that interacts with environment variables directly in the source code will cause dangerous risks.

[staudenmeir]

If we revert this, how do we fix php artisan serve --env (#27828)?

[GrahamCampbell]

If we revert this, how do we fix php artisan serve --env (#27828)?

I don't see how that bug is related to the getenv function?

[staudenmeir]

@GrahamCampbell Restoring getenv() and putenv() fixed php artisan serve --env.

[matt-allan]

The reason this breaks the serve command is if we don't use getenv/putenv any child process will fail to inherit the environment variables, and the serve command is using passthru.

What happens when you run php artisan serve --env testing is:

• the console kernel boots and LoadEnvironmentVariables::checkForSpecificEnvironmentFile sees the --env flag so loads the .env.testing file, which contains APP_ENV=testing
• the PutenvAdapter calls putenv for APP_ENV
• The serve command calls passthru to spawn the server process
• the server process doesn't see a --env flag so it calls the env helper. The env helper calls getenv('APP_ENV'), which because putenv was called on the parent process, returns testing.

If we don't use getenv and putenv the child process never receives the APP_ENV environment variable. If the child process receives the wrong APP_ENV the rest of the environment variables are likely to be wrong. If anything else calls env before the bootstrapping is done it will receive null.

This doesn't just affect the serve command, it will affect any spawned process. If Laravel is affected by this I can only imagine there will be plenty of third party packages that run into the same issue.

Anything using the Symfony Process component will not break because the Process component explicitly adds any environment variables from $_SERVER, $_ENV, and getenv. That is why queue:listen, horizon, etc. were not affected.

[matt-allan]

Yeh, you can just define your own bootstapper, by extending the one from the core, and overring the method that makes a dotenv instance. That's why I made it it's own method.

Even if we override that method it will not change how the env helper works. Laravel calls env before bootstrapping so that would need to be overridden as well.

Can we put Dotenv\Environment\DotenvFactory in the container so it's easier to override and only instantiated in one place?

[GrahamCampbell]

We can't put it in the container, because it is instianated very early in the bootstrapping process, before any service providers are called. The correct way to override it is to set your own bootstrapper in your app' two kernel classes.

[taylorotwell]

Too big of a breaking change to be the default. Until a large number of other PHP libraries stop using getenv directly it's just too much headache and I'm not dealing with the blowback again. If people want to disable it they can override the method.

[deleugpn]

The conclusion here seemed that libraries have to gradually move away from getenv. OTOH, this article recommends the use of getenv with a well established argument.