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

Dump frame arguments below the source code #387

Merged
merged 10 commits into from
Mar 6, 2016

Conversation

jonasdt
Copy link
Contributor

@jonasdt jonasdt commented Feb 18, 2016

This builds on top of #337, but dumps the arguments below the source code (similar to #384).

Collapsed by default:
http://imgur.com/9augmq7
Expand second argument:
http://imgur.com/oXfrSfa
Expand first and second argument:
http://imgur.com/JJQ1oF0
No arguments:
http://imgur.com/uWVch0e

staabm and others added 8 commits February 18, 2016 00:26
…umper::dump

HtmlDumper::dump calls HtmlDumper::setOutput and this method resets the headerIsDumped property every time the $output argument changes.
This results in including the global styles and scripts on each dump.
We prevent this from happening by reusing the same HtmlDumperOutput object.
Tries to be as close as possible to the source code syntax highlighting.
@jonasdt jonasdt changed the title Dump frame arguments below source code Dump frame arguments below the source code Feb 18, 2016
@prisis
Copy link
Contributor

prisis commented Feb 18, 2016

👍 can you squash it? :)

$('.sf-dump-expanded')
.removeClass("sf-dump-expanded")
.addClass("sf-dump-compact");
$(".sf-dump-toggle span").html('▶');
Copy link
Contributor

Choose a reason for hiding this comment

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

please use a html sequence instead of a UTF8 char (to make it work on non utf8 websites)

@staabm
Copy link
Contributor

staabm commented Feb 18, 2016

really like it. besides the few nits I commented above it looks like a better alternative to my initial PR

@jonasdt thx for picking it up.

@jonasdt
Copy link
Contributor Author

jonasdt commented Feb 18, 2016

@staabm I've made the changes (except for the static variable thing). I can squash the commits if it is required.

@staabm
Copy link
Contributor

staabm commented Feb 18, 2016

👍

@shadowhand
Copy link

Please merge this! It is really nice to have arguments, especially when working with templates where multiple variables might be passed through the same functions. Determining what variable is causing an issue is extremely hard when no arguments are visible.

@damnedest
Copy link

Looks great

@damnedest
Copy link

Any news about merging this request?

@grevus
Copy link

grevus commented Feb 24, 2016

+1

@denis-sokolov
Copy link
Collaborator

I will get to it, sorry for the delay.

@damnedest
Copy link

Any updates?

@grevus
Copy link

grevus commented Mar 2, 2016

2 similar comments
@serafimovich
Copy link

@vvolynets
Copy link

@prisis
Copy link
Contributor

prisis commented Mar 2, 2016

Guys wait a bit, maybe @denis-sokolov has a bit to do the last past days

Repository owner locked and limited conversation to collaborators Mar 2, 2016
denis-sokolov added a commit that referenced this pull request Mar 6, 2016
Dump frame arguments below the source code
@denis-sokolov denis-sokolov merged commit 1de5602 into filp:master Mar 6, 2016
Repository owner unlocked this conversation Mar 6, 2016
@denis-sokolov
Copy link
Collaborator

Seems good, thank you for your contributions, @staabm, @jonasdt!

@kktsvetkov
Copy link

kktsvetkov commented Nov 11, 2018

@denis-sokolov @staabm @jonasdt Doesn't this "dump frame arguments" change have some security implications? Just like $_SERVER and $_ENV, stack frame arguments might contain security-sensitive data, such as credentials or API keys and tokens. Perhaps the blacklist feature must expand to cover arguments as well ?

Here's a somewhat real example: imagine a method/function that takes credentials as arguments and uses them in a Guzzle request, but the request fails and an exception is thrown; the exception message itself might not contain any sensitive data (although, sometimes they do - mainly tokens and API keys), but when you start looking at the backtrace, you are going to see the credentials as the arguments in the stack frame.

EDIT: I think I managed to find a better example, database connections. Here's a page I found googling for "Whoops! There was an error". It is an old Whoops version, but is fine to illustrate my point: the PDO::__construct() is in the backtrace, and if the arguments are dumped, you can see them:

whoops pdo

@staabm
Copy link
Contributor

staabm commented Nov 11, 2018

I think you have a valid point. Please open a new issue

@jonasdt
Copy link
Contributor Author

jonasdt commented Nov 11, 2018

It is not important for our workflow as we only use Whoops in development, but I understand your point that some people might forget to disable it on production. The same can be said about phpinfo ...

@staabm
Copy link
Contributor

staabm commented Nov 11, 2018

Running it in production is not a valid use-case. Nevertheless I feel a blacklist for some sensitive parameters makes sense though (people which can access a development machine are not necessarily the ones who should know all the security related tokens/passwords etc. Involved)

@kktsvetkov
Copy link

I thought about this more today and there are several places where sensitive informations can be exposed. It is not just the data tables and the arguments dump, but sometimes also the exception messages, and the source code chunks. Blacklist needs more wholistic approach, but even if there is such a solution, I agree that this is all meant to be used for development only.

I've posted another idea I have about how to "hide" Whoops on non-development environments at 604

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

10 participants