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

Disclosing critical information stored in $_SERVER #124

Closed
olvlvl opened this issue Oct 7, 2015 · 6 comments
Closed

Disclosing critical information stored in $_SERVER #124

olvlvl opened this issue Oct 7, 2015 · 6 comments

Comments

@olvlvl
Copy link

olvlvl commented Oct 7, 2015

Hi,

Exception handling tools such as filp/whoops are dumping $_SERVER, possibly disclosing critical information put there by phpdotenv. This is a security issue that could easily be fixed. What's the point in having this values in $_SERVER in the first place? If you setup environment variables properly they are not polluting $_SERVER. More over, environment variables should be read from $_ENV or obtained using getenv(), not $_SERVER although it may seem handy.

As an example when a run ICANBOOGIE_INSTANCE=dev php -S localhost:8000, ICANBOOGIE_INSTANCE is not in $_SERVER but can be read from $_ENV or obtained using getenv().

Your tool is introducing a double standard.

What do you think?

@josegonzalez
Copy link

I work on a related project have a few thoughts on this:

  • You should never dump errors like whoops does in production. Definitely in development, maybe staging, never production.
  • Many error reporting tools allow you to whitelist/blacklist variables from being output. I know sentry does, and whoops should support the same. CakePHP has blanked out anything it detects as a password field and has done so for a while, and I assume other frameworks do as well.
  • Even if you don't use $_SERVER for env vars, it's likely your tool will try $_ENV, defined constants, or any number of other options that a tool like dotenv might output. Again, allowing configurability here is useful, but there isn't a "sane" default other than forcing the developer to make a conscious decision.
  • A security notice in the readme is probably a great idea.

Don't know how @vlucas feels about this, but those are my thoughts.

josegonzalez added a commit to josegonzalez/php-dotenv that referenced this issue Oct 9, 2015
@vlucas
Copy link
Owner

vlucas commented Oct 10, 2015

@olvlvl Tools like whoops only prints exceptions stack traces and dumps superglobals when in development mode. Even if phpdotenv excludes variables from $_SERVER, whoops still dumps $_ENV in development mode, so it likely would not make a difference.

That said, phpdotenv could definitely provide an option in a non BC-breaking way that would not write to $_SERVER if that is a major concern of yours.

@tlshaheen
Copy link

@vlucas +1 for phpdotenv could definitely provide an option in a non BC-breaking way that would not write to $_SERVER if that is a major concern of yours. It would be great to see!

I do not print debugging information to the screen in Production, however, I do like to log $_SERVER settings when I catch exceptions. If I do that when using phpdotenv, I'm now printing all my creds to my logs, which are being transmitted out to 3rd party sites like LogEntries and PaperTrail.

@olvlvl
Copy link
Author

olvlvl commented Oct 22, 2015

From my point of view, it would definitely be an improvement if $_SERVER was not polluted with credentials.

@sergeyklay
Copy link

+1

@GrahamCampbell
Copy link
Collaborator

#76 (comment)

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

No branches or pull requests

6 participants