[WebProfilerBundle] Add a pane with loaded envs #23951
Conversation
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| License | MIT |
|
See also #22406 still not sure about this :) |
|
Yep, already we have this information in Server parameters section see #22406 (comment) |
|
@ro0NL, thanks for the link! The main difference between that PR and this one is: in my PR the pane shows only the variables loaded by the Dotenv component (using a special variable |
|
@voronkovich sorry.. didnt noticed you solely used SYMFONY_DOTENV_VARS (so it's indeed a different PR!). Yet @yceruto comment still stands; we do display this already under server parameters. However your PR made me realize we dont in CLI, so see #24144 for that :) What about tweaking the server parameters table in profiler a bit to highlight those loaded by dotenv (or group those first)? I think a special panel is too much. |
|
I'm still not convinced by this proposal 😑 Is that useful to expose separately the env vars loaded from .env file? We already have that information in the server parameters. Not sure if it's worth it. |
| @@ -245,6 +245,22 @@ | |||
| </div> | |||
| </div> | |||
|
|
|||
| <div class="tab {{ profile.collector('dotenv').envs is empty ? 'disabled' }}"> | |||
There was a proble 8000 m hiding this comment.
this would break in case the dotenv panel is not registered
There was a problem hiding this comment.
@stof, you're right! I've added an if expression to check whether the dotenv collector is registered.
|
Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw. |
|
Closing in favor of #25166 |
This PR was squashed before being merged into the 4.1-dev branch (closes #25166). Discussion ---------- [WebProfilerBundle] Expose dotenv variables | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes/no | Fixed tickets | #23951 | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> Variant of #23951  Commits ------- de33d87 [WebProfilerBundle] Expose dotenv variables
|
Isn't this a security issue when passwords are in env parameters? |
|
You mean displaying/caching (sensitive) server parameters in general? Is it really bad with debug+profiler enabled? Are we worried about profiler used in prod; where envs might leak into cache? |
|
Most env passwords are compiled to container parameters and/or service arguments. Logging them in profiler data implies similar access as to the raw compiled container. I don't really see added risk there. |
Not hardcoded with new env config :) |
|
I mean sensitive server parameters in the profiler panel in general. Misconfigured debug env.. |
|
@King2500, IMHO, you should use tools like the Ansible vault to store your application's secrets and not use them in environment variables. |
|
Undeniably true, but in the real world we have clients expecting us to deploy their shiny new websites on $0.99/mth shared hosting. In many cases .env is simply the best we have, with parameters.yml the only viable alternative. @ro0NL that protection is pretty much non-existent if your environment is truly misconfigured. I just tried opening |
|
Note that we explicitly rejected security issues in the profiler before. If someone has access to the profiler, it gets almost anything he wants. So, securing just one part does not change anything in terms of security. |
|
@fabpot I basically agree with that, up to the point where the first security issue should not lead to an avalanche of followup problems. That profiler access compromises the entire website it's part of makes sense, it should not have to compromise the entire server automatically as well. So the Profiler should still be protected against directory walking, reading hidden files et al. |
|
At this point.. why assume dotfile is senstive and others not? Agree with @fabpot
You really opt-in to a certain risk now.
But do we care? |
|
I do. I've had developers accidentally activate a dev environment on a staging server for days until the customer asked whether that techie bullshit was going to be removed before final release. On that same staging server we may run other sites, some with potentially highly sensitive information during testing/staging. Starting May 2018 the maximum fine for leaking information like that is increased to 20 mln euro. So please let's not take a lax approach that access to the profiler, a mistake easily made, must mean total server loss, and take basic sensible precautions. Full server access should be limited to console/SSH, not a profiler most often not even served over HTTPS. |
|
Maybe continue the discussion in a dedicated issue? |
|
Giving a false of security to our users is IMHO even worse. Current stand is very clear: You MUST be sure to NEVER activate the profiler in production (or a server exposed to the Internet). I can tell you that having access to the profiler means giving access to the physical server (I won't tell you how but trust me, that's possible). Trying to make the profiler "a bit more secure" does not make sense. When security is involved, it's black or white. |