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

Add Logger with inspect security check #1069

Closed

Conversation

epinault
Copy link

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
Copy link
Owner

rrrene commented Aug 28, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

epinault commented Oct 6, 2023

closing this since seems like it won t be merged

@epinault epinault closed this Oct 6, 2023
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.

2 participants