Add ability to set whether environment variables to be displayed

[MarnuLombard]

If you are able, you are welcome to ignore the first commit of the two - it's just an amendment to the .gitignore file.

The second commit's message explains the code changes well.

[denis-sokolov]

Thank you for your contribution.
What is the use case for not displaying environment variables?

[GrahamCampbell]

no

[denis-sokolov]

Perhaps you may want to use a user-global gitignore file to minimize noise and also to minimize work for yourself.

[MarnuLombard]

Yeah cool, This wasn't meant to be part if the pull request - I don't have a lot of experience with pull requests.

[MarnuLombard]

Thanks for the advice about the global .gitignore.

[denis-sokolov]

Don't worry about it, if we choose to merge, I'll quickly rebase it myself.

[MarnuLombard]

Hi Denis.

A lot of people use environment variables to store sensitive config (third party services passwords, etc).

While one would never use Whoops in production, I've found that the environment between my local development environment and the production ready site (staging environment) needs to be accessible from the outside - whether I'm reviewing with remote design teams or getting client's input.

In the staging environment I use leave all debug ability turned on - being that at this stage bugfixes are still really likely. But leaving my database username & password, my AWS username and password and all of that open for anyone to stumble upon is extremely dangerous.

When looking into whether there was a flag to disable the display of $_ENV i came across #119 - Indicating to me that it was a function of Whoops others wanted too.

The fact that you added it to the Features to add list motivated me to do the work.

[MarnuLombard]

Do you want me to write tests for this?

[MarnuLombard]

Perhaps there could be an apposing method to addDataTable().
Sort of a filterOutData(), that could look an unset data - instead of just a yes-no for $_ENV?

[MarnuLombard]

I'm also going to wrap these strings in double quotes, it seems that's the style you've used in the class.

[MarnuLombard]

Will fix the short array syntax I used.

[denis-sokolov]

Edit: see the more up-to-date comment.

I have now realized why I am against this. I do not want to create an illusion of security.
If we add any single feature that masks some data, developers will begin to expect that this has somehow made PrettyPageHandler "secure".
Even if nothing else reveals their sensitive data (code snippet, traceback function values), we have no guarantees that we will not add more information to PrettyPageHandler display in the future.
For example, what if one day PHP allows us to collect local variables from the point of error?
What if we add an interactive console for better debugging?
PrettyPageHandler is absolutely insecure and must not ever be used in any context where you might want to hide something. Just replace it with an EmailHandler.

If you absolutely really need a short term hack, just mask the data yourself:

$run->pushHandler(new PrettyPageHandler());
$run->pushHandler(function($exception, $inspector, $run) {
  $_ENV['SENSITIVE_KEY'] = 'censored';
});

Thank you for your contribution.
The feature you request will not be added to PrettyPageHandler in any form.

[MarnuLombard]

That actually makes a lot of sense,
Thanks for explaining.

Stay well.

[MarnuLombard]

That actually makes a lot of sense,
Thanks for explaining.

Stay well.

[garygreen]

@denis-sokolov

I have now realized why I am against this. I do not want to create an illusion of security.

I think you have totally missed the point behind creating a way of masking sensitive values or hiding them at all. For a user of Whoops to think that because there is now an option to hide e.g. envs means they can now enable PrettyPageHandler for every request would be a ludicrous thing to suggest.

We realise that it's not secure, in any case, to enable that mode on production - the real value here is accidental enabling of the handler e.g. enabling debug mode (in Laravel) and suddenly BOOM you have exposed every database env password, API key, everything. One silly mistake is all it takes. We're all human, and this extra bit of security on top will only help in those cases.

Your view is basically this - don't enable debugging / tracing in production. What if MySQL connection by PDO took the same view and exposed the "Using password: < MY LOVELY PASSWORD >" because, hey, you shouldn't be displaying errors / stack traces in production.

Mistakes happen. This is a way of helping when things go badly wrong. It's nothing to do with adding a false sense of security, it's about realising we're human.

[garygreen]

Another example - Whoops has now been officially added back into Laravel 5.5

What if a malicious user or very bad pull request came in for Laravel which meant that PrettyPageHandler is accidentally running in production. People pull down the update, deploy, not realising the change.

Now sites are exposing database passwords, API keys, everything. Yes, it's also exposing code traces, and other stuff, but at least information that is clearly sensitive is hidden and damage is kept to a minimum.

Don't get me wrong, this would be a terrible thing to happen and I really hope it doesn't, but it's a frightening thought. Anything we can do to help mitigate potential problems is a win in my mind.

PS - I've submitted a PR to Laravel to only install Whoops on development and not production, this will help mitigate any security problems by exposing envs and stuff accidentally.

[denis-sokolov]

My more up-to-date comment might be of interest to you, @garygreen.

[garygreen]

@denis-sokolov thanks for the link, glad to see you have changed your thoughts towards masking of sensitive information, the PR looks awesome. It might be an idea to update your original comment above with the link so people coming to this issue will see the most recent changes.