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

[5.5] Add config option for whoops blacklist #21336

Merged
merged 1 commit into from
Sep 23, 2017

Conversation

jpuck
Copy link
Contributor

@jpuck jpuck commented Sep 22, 2017

Sometimes I'm not the only one looking at my development work. As I'm working on a project, I will often show little previews of what I'm working on. Even within my local network, I will have someone checkout my dev site from their machine. It's not always convenient to spin up a test site for these previews. I don't want to expose some secrets to these folks if an exception is thrown, or others who might be looking over my shoulder.

This PR leverages Whoops's built-in blacklisting ability to mask certain variables. It is backward compatible in that if no config/whoops.php file exists, the behavior is the same. If you would like this option, then one would just have to create the config file. If the PR is accepted, then perhaps a base config file could be added to the laravel installer too with some sensible defaults.

For example, given this config:

return [
    'blacklist' => [
        '_ENV' => [
            'APP_KEY',
            'DB_PASSWORD',
            'REDIS_PASSWORD',
            'MAIL_PASSWORD',
            'PUSHER_APP_KEY',
            'PUSHER_APP_SECRET',
        ],
        '_SERVER' => [
            'APP_KEY',
            'DB_PASSWORD',
            'REDIS_PASSWORD',
            'MAIL_PASSWORD',
            'PUSHER_APP_KEY',
            'PUSHER_APP_SECRET',
        ],
        '_POST' => [
            'password',
        ],
    ],
];

Results in this output:

whoops exception page

@jpuck jpuck changed the title add config option for whoops blacklist [5.5] add config option for whoops blacklist Sep 22, 2017
@GrahamCampbell GrahamCampbell changed the title [5.5] add config option for whoops blacklist [5.5] Add config option for whoops blacklist Sep 23, 2017
@taylorotwell taylorotwell merged commit c39faf7 into laravel:5.5 Sep 23, 2017
@jpuck jpuck deleted the whoopsBlacklist branch September 23, 2017 19:20
@yajra
Copy link
Contributor

yajra commented Sep 27, 2017

FYI to those implementing this. The config key is now app.debug_blacklist instead of proposed whoops.blacklist.

@jpuck
Copy link
Contributor Author

jpuck commented Sep 27, 2017

@yajra indeed I noticed that a83ebc1 too. should put something in the docs, I guess, but maybe too trivial? in the meantime if anyone goes specifically searching, then hopefully they'll find the answer on stack overflow: https://stackoverflow.com/a/46407010/4233593

also, I'm not sure what thoughts there are on including some defaults in the laravel/laravel installer..?

@TrieBr
Copy link

TrieBr commented Mar 7, 2018

Personally, I would like to see the default installation hide all environment variables by default. I really don't see any point in exposing environment variables and creating a security risk when the developer can easily just open up their .env file and view the variables. It should be an opt-in feature rather than opt-out in my opinion.
@taylorotwell

@brajnacs
Copy link

I completely agree with @TrieBr. Exposing environment variables is such a big issue that it may put your whole security checklists and practices in the trash within a second.

Moreover, rather than blacklisting we should have a whitelist config as blacklisting in security doesn't sound good and it does not align with the spirit of "Principle of least privilege". One may forget to put sensitive variables in blacklist as the project grows.

@KlodianLula
Copy link

KlodianLula commented Jun 23, 2018

Hi I'm a professor of programming languages and I'm sure a lot of people have requested this to be default and I would only suggest to take into consideration the fact that one of my students risked maybe the jail by forgetting to disable the dev mode configuration in the .env file.
I tried myself the framework and think to apply it in my projects now on but please consider this as an issue of great importance because nobody cares sometimes of the exceptions or if there is no 404 page... until they realize the db password is shown there, which is a catastrophe!

Also other comments in Laracast: https://laracasts.com/discuss/channels/laravel/security-issue-no1-suggestion-masking-by-default-the-information-that-resides-in-env-file?page=1

By the way really great framework @taylorotwell

Thank You!

@tontonsb
Copy link
Contributor

tontonsb commented Nov 5, 2018

Yeah, this is a nightmare.

Steps to reproduce:

  1. Create a fresh Laravel project on a virtual server (amazon, digitalocean, linode - whatever you like)
  2. Enter your database credentials in .env
  3. visit your_app_url/logout
  4. Witness your database credentials dumped publicly

How is that acceptable as the default behaviour?

It's unfourtunate that whoops package doesn't yet support whitelisting or even just hiding the variable section.

For now I've made a package to destroy the variables before they get to Whoops handler. The approach is rather blunt but at least I can now configure the displayable fields in either whitelist or blacklist manner. And it doesn't reveal the string length with asterisks. And can completely remove fields.

@kktsvetkov
Copy link

I've taken a different approach to masking sensitive data with this package, Fuko\Masked: you "declare" all the passwords and tokens and sensitive data you have and then all of that will be redacted out of any variables you pass. In this way you do not have to worry where your passwords might pop up in any of the dumps you create. I'll appreciate the feedback

@electroidru
Copy link

Personally, I would like to see the default installation hide all environment variables by default. I really don't see any point in exposing environment variables and creating a security risk when the developer can easily just open up their .env file and view the variables. It should be an opt-in feature rather than opt-out in my opinion. @taylorotwell

Definitely agree with @TrieBr

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.

None yet

9 participants