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

Fixes #474 - Don't leak env values into $_SERVER #598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

swalkinshaw
Copy link
Member

Updates to using a custom repository for Dotenv instead of the default which includes ServerConstAdapter.

The new custom repository only includes EnvConstAdapter.

The $_SERVER superglobal often gets dumped into logs or into monitoring services so it's better for security to avoid populating it with secrets contained in .env.

Note: this could be a breaking change for some users, but we at least need to ensure it's not breaking in the normal case.

@swalkinshaw swalkinshaw requested a review from QWp6t May 29, 2021 03:29
Updates to using a custom repository for `Dotenv` instead of the default
which includes `ServerConstAdapter`.

The new custom repository *only* includes `EnvConstAdapter`.

The `$_SERVER` superglobal often gets dumped into logs or into
monitoring services so it's better for security to avoid populating it
with secrets contained in `.env`.
@swalkinshaw swalkinshaw force-pushed the dont-load-env-values-into-server-superglobal branch from 4bf21d7 to c68c908 Compare May 29, 2021 03:39
$dotenv = Dotenv\Dotenv::createUnsafeImmutable($root_dir, ['.env', '.env.local'], false);
$repository = Dotenv\Repository\RepositoryBuilder::createWithNoAdapters()
->addAdapter(Dotenv\Repository\Adapter\EnvConstAdapter::class)
->addAdapter(Dotenv\Repository\Adapter\PutenvAdapter::class)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QWp6t needed this as well to mirror the default behaviour and get everything working

@montchr
Copy link

montchr commented Sep 18, 2024

Is this still necessary to fix #474?

@retlehs
Copy link
Member

retlehs commented Sep 21, 2024

@montchr I think so! Thanks for the bump, I think this just got forgotten about 😅 will get this rebased and hopefully reviewed soon

cc @QWp6t @tangrufus

@retlehs retlehs removed the request for review from austinpray September 21, 2024 17:48
@Tofandel
Copy link

Note: this could be a breaking change for some users, but we at least need to ensure it's not breaking in the normal case.

Given that this package doesn't have an upgrade path because it's a boilerplate, I wouldn't worry about breaking changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants