Skip to content

Remove args label#11

Open
bhcleek wants to merge 1 commit intoExpansiveWorlds:masterfrom
bhcleek:remove-args-label
Open

Remove args label#11
bhcleek wants to merge 1 commit intoExpansiveWorlds:masterfrom
bhcleek:remove-args-label

Conversation

@bhcleek
Copy link

@bhcleek bhcleek commented Apr 28, 2018

Remove all the labels and logs that surface parameterized query
arguments, because doing so severely impacts performance due to the use
of github.com/kr/pretty and because parameterized query arguments often
contain sensitive data.

Because parameterized query arguments often contain sensitive data (passwords, api tokens, etc.) or highly regulated data (e.g. HIPAA, GDPR, et al), it's generally a good idea to not log them.

Remove all the labels and logs that surface parameterized query
arguments, because doing so severely impacts performance due to the use
of github.com/kr/pretty and because parameterized query arguments often
contain sensitive data.
@bhcleek bhcleek force-pushed the remove-args-label branch from 8b2d061 to bfe9c0a Compare April 28, 2018 00:58
@luna-duclos
Copy link

Hey there, https://github.com/luna-duclos/instrumentedsql has an Option already to disable query argument logging. Please have a look and let me know if it's sufficient.

@bhcleek
Copy link
Author

bhcleek commented Apr 29, 2018

It looks like that will nearly be sufficient. The query args in prepared statements don't leverage the option, though. Is that an oversight or intentional?.

Is your fork the new canonical repository instead of this one?

@luna-duclos
Copy link

I'll happily accept a PR to fix the prepared statement issue.
I maintain the fork, I can't speak for this repo as I no longer work at EW.

@pavelnikolov
Copy link

IMHO, this needs to be an option of the driver. The user can chose whether to print args or not...

slagiewka pushed a commit to slagiewka/instrumentedsql that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants