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

Dancer2::Core::Error::_censor(): Improve detection of sensitive data. #1206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jkeenan
Copy link
Contributor

@jkeenan jkeenan commented Jul 8, 2016

In addition to checking the keys of each hash at each level, we should also
check the keys of hashes blessed into classes.

We should also check for the values of sensitive keys in key-value pairs in
query clauses, e.g., '&pass=my_top_hush_pwrd'.

t/error.t: Add subtest block explicitly testing _censor().

In addition to checking the keys of each hash at each level, we should also
check the keys of hashes blessed into classes.

We should also check for the values of sensitive keys in key-value pairs in
query clauses, e.g., '&pass=my_top_hush_pwrd'.

t/error.t: Add subtest block explicitly testing _censor().
@jkeenan
Copy link
Contributor Author

jkeenan commented Jul 8, 2016

While working my way through Dancer2::Manual, I encountered errors which dumped my config.yml and session data into the browser, thereby exposing my password. Investigation suggested that Dancer2::Core::Error::_censor() was at fault. Most parts of that subroutine were also unexercised by the test suite. So I revised _censor() and added a test block with dummy data worked up from the error output I encountered earlier in the day. The additional tests may not be in approved Dancer2 style, but they get the job done. Test coverage of Dancer2::Core::Error has been improved as follows:

----------------------------------- ------ ------ ------ ------ ------ ------
File                                  stmt   bran   cond    sub   time  total
----------------------------------- ------ ------ ------ ------ ------ ------
before: lib/Dancer2/Core/Error.pm     96.8   82.3   72.2  100.0  100.0   89.5
after:  lib/Dancer2/Core/Error.pm     99.3   85.2   79.6  100.0  100.0   92.8

@ambs
Copy link
Member

ambs commented Jul 8, 2016

It might be extra zealous in some situations, but I shouldn't hurt. For me, 👍.

@jkeenan
Copy link
Contributor Author

jkeenan commented Jul 8, 2016

On 07/08/2016 01:21 PM, Alberto Simões wrote:

It might be extra zealous in some situations, but I shouldn't hurt. For me, 👍.

Thanks for the feedback.

After creating the p.r., I got to thinking: Isn't this a candidate for
a stand-alone CPAN module? It's buried deep inside Dancer not really
Dancer-specific.

@ambs
Copy link
Member

ambs commented Jul 8, 2016

Yes, that's true. I think that if you want to create one, nobody would oppose to start using it. But that is me speaking (not the core group).

@jkeenan
Copy link
Contributor Author

jkeenan commented Jul 9, 2016

Okay, I might attempt that in a few weeks. In the mean time, I'd like to see if this is an acceptable patch for Dancer2.

Thank you very much.
Jim Keenan

@veryrusty
Copy link
Member

@jkeenan Read through #530 before tackling that.

@jkeenan
Copy link
Contributor Author

jkeenan commented Jul 9, 2016

Since there already exists #530 with the same feature request, I will close this request. I hope that some improved detection of sensitive data can be implemented soon.

@jkeenan jkeenan closed this Jul 9, 2016
@ambs ambs reopened this Jul 9, 2016
@ambs
Copy link
Member

ambs commented Jul 9, 2016

Let it open,until something is done. :-)

@cromedome cromedome added this to the April 2020 Release milestone Apr 13, 2020
@cromedome cromedome removed this from the April 2020 Release milestone Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants