Dancer::Error::dumper trashes password fields if Clone not installed #855

Closed
ErasmusDarwin opened this Issue Oct 22, 2012 · 8 comments

Comments

Projects
None yet
3 participants

If Clone isn't available, Dancer::Error::dumper will censor a shallow copy of the original structure instead of a deep copy. As a result, any config options with password-style keys nested in the hash get replaced with "Hidden (looks potentially sensitive)". As a result, when using Dancer::Plugin::Database, a dancer app that experiences an error would lose the password to the database and be unable to reconnect until the dancer app was restarted.

It looks like Dancer::Error got missed during the Clone purge from bug #825.

The key gotcha seems to be this bit of code in Dancer::Error::dumper:

# Take a copy of the data, so we can mask sensitive-looking stuff:
my %data = Dancer::ModuleLoader->load('Clone') ?
               %{ Clone::clone($obj) } :
               %$obj;
Owner

xsawyerx commented Oct 22, 2012

Nice catch! We'll fix it up and release a new version.

Thank you.

Glad I could help. And thanks for Dancer -- it just makes everything so much easier.

Owner

xsawyerx commented Oct 23, 2012

@ErasmusDarwin, it would appear that this code was not part of #825 at all. In fact, it was introduced in April 2011.

@bigpresh, how would you suggest addressing this?

Right, #825 didn't add this code, but it did cause the latent bug to manifest. #825 removed Clone from Dancer's dependencies, so prior to that change, Clone should have always been able to load, and this code would have avoided using the shallow copy.

My point (which was a little less than clear) was that when #825 fixed Dancer::Template::Abstract to gracefully switch between Clone::clone and Storable::dclone, Dancer::Error was apparently overlooked, possibly because Clone was being loaded dynamically via ModuleLoader rather than being loaded with "use Clone".

Owner

xsawyerx commented Oct 25, 2012

Originally Dancer didn't include Clone. Once we tried adding it to fix a problem we've gotten complaints that it's XS and breaking installation where no C compiler is available. So we basically returned to the situation prior. If it worked for you before (just installing Clone), then it will work for you now. :)

I don't understand what the problem with simply installing Clone is.

Ah. I didn't realize this issue predated Clone being mandatory. My mistake.

And to clarify, I did install Clone when I found the bug, and it fixed it. It's just other people, who may not know they need to install Clone, that I was worried about.

Contributor

mokko commented Apr 3, 2013

clone is not required in current D1. If clone is not installed, we apparently get shallow or censored output from Dancer::Error::dumper. It seems we can live with it. Let's close this.

Owner

xsawyerx commented Apr 3, 2013

@mokko thanks!

xsawyerx closed this Apr 3, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment