rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.91k stars 415 forks source link

Add Logger with inspect security check #1069

Closed epinault closed 11 months ago

epinault commented 1 year ago

This allows to flag logger calls with inspect in it. I am not sure if I am doing the correct way to navigate the AST but that seem to work and is fast

rrrene commented 1 year ago

I don't think this is a good check to include in Credo. Why would using inspect/2 be a bad thing in logs? According to your own argument it is only a bad thing if you don't know the data you are logging.

epinault commented 1 year ago

So this not the first company I work for where people log and inspect out of convenience a map or a config. (like kafka config and leaking Kafka Creds forgetting it s in there)

Change the map(often params) later on, and then leak PII (like inspect params for a user to debug something in prod) or password accidentally.

Things can also get missed in a code review, hence why credo is awesome for that. So flagging help potentially someone to not leak secure creds or PII.

When those data are leaked, that becomes a big legal problem to deal with. While this might not seem useful to you, I am sure it will be for other as it s too easy to shoot yourself in the foot or want some form of safety.

And would be best to be part of Credo and centralize it rather than having to have a CredoExtraCheck package as people can choose to use it or not?

epinault commented 1 year ago

by the way, even if you don't want to merge this, is that the right approach? or is there a better way to do what I am trying to achieve?

epinault commented 11 months ago

closing this since seems like it won t be merged