search

andrewbanchich / shreddit

Delete your Reddit data.
MIT License
235 stars 10 forks source link

Secrets printed in plaintext #60

Closed betamos closed 1 year ago

betamos commented 1 year ago

[Obligatory thanks for this excellent tool]

I noticed that the output includes password and client_secret, which probably should be hidden from debug info.

andrewbanchich commented 1 year ago

Hi, thanks!

Since these logs aren't supposed to be saved anywhere, I don't think this is a security issue.

The reason I added them to the logs is to help in debugging issues when users provide incorrect credentials or when there is something wrong with their config file.

timmc commented 1 year ago

I looked into what it would take to just output the config once, at the beginning of the run, but it seems like the output is based on instrumentation that prints function arguments. Not a completely trivial fix, I think.

andrewbanchich commented 1 year ago

@timmc Is there a reason you believe this is not secure? Are you saving these logs somewhere?

If it's insecure for some reason, I will implement a fix. But AFAIK you're the only one that sees the output.

andrewbanchich commented 1 year ago

CLI-based password managers such as password store print passwords in plain text to stdout. As far as I'm aware, the security issue is if they are stored in plaintext, which Shreddit does not do (unless you're referring to the shreddit.env file).

timmc commented 1 year ago

It's not terribly insecure, no. The script runs for a while, which means it would be easy to accidentally screen-share. Or if I want to share a stack trace, and the last few log messages, I'd have to take care to clip out the sensitive bits.

So really just mild concerns; if it's an easy fix it would be nice, but otherwise not worth it.

timmc commented 1 year ago

(Oh right, and: It just makes the output overly verbose, so it's harder to see what's happening. Again, not a major thing.)

andrewbanchich commented 1 year ago

Gotcha. I've been pretty busy lately, but if anyone wants to contribute this you could just set the config log to trace level and then make the default log level debug. That way you'd need to opt in to seeing the config.

I added this as a default log because I was getting issues opened related to invalid credentials, and they were probably due to special characters or wrong passwords. This made it more obvious to users.