ShokoAnime / ShokoServer

Repository for Shoko Server.
http://shokoanime.com/shoko-server/
MIT License
376 stars 74 forks source link

Adds additional censorship when logging #1042

Closed fearnlj01 closed 1 year ago

fearnlj01 commented 1 year ago

The first commit uses regex to avoid logging the auth key when utilising AVDump. As @Cazzar has noted in #support (https://discord.com/channels/96234011612958720/96235819366371328/1079930915054170172), it probably makes more sense to just simply log the filename here instead, however I daren't make such a change myself.

The second commit just additionally censors settings (when dumping) that have names ending in key & token (rather than just password). Admittedly, my inclusion of token in this change is a bit of a guess since I don't use Plex nor Trakt, however I'd imagine that users might expect these credentials to be kept secret if sharing the logfile?

Cazzar commented 1 year ago

I would more say log the filename as regex, while I can be a fan of it can be hard to maintain and make sure isn't working as well, plus worst case we can reconstruct the parameters .

As for the other reasons I don't think there's much of a need to do those specifically, the plex token is actually never set (nor logged as it's done as a HTTP header to plex) (The setting is actually just a thing that existed for a short period before it was moved to the database)

as for the simple matter of censoring, at a thought maybe instead of a string-match would it be better to make it attribute driven (cc @da3dsoul thoughts?) for example

[LogCensor] public string SomeKey {get; set;}
fearnlj01 commented 1 year ago

New commits refactor the AVDump process somewhat to use a list of arguments rather than a single argument string. Regex is completely avoided this way!

Tested for function on Windows and it seems to play nice however I haven't yet had the chance to test on any other OS. I will try to get a VM spun up later today to test this on Ubuntu. I've also now tested this on Ubuntu 22.04 (via WSL) and thankfully, this appears to work correctly as well.

If we wanted to log the 'full' command line it should be equally trivial to change with this refactor.

As far as censorship for the settings file dump is concerned, I could remove this from this PR (although the AVDump key would still be dumped if so) - just let me know.

fearnlj01 commented 1 year ago

as for the simple matter of censoring, at a thought maybe instead of a string-match would it be better to make it attribute driven (cc @da3dsoul thoughts?) for example

[LogCensor] public string SomeKey {get; set;}

Since this was easier than I thought to do... I've got an implementation for this ready as well - Will await a further thumbs up to making the censorship attribute driven before adding this to the PR. Hopefully storing the new attribute in /Shoko.Server/Settings/Attributes/LogCensorAttribute.cs is agreeable?

da3dsoul commented 1 year ago

I think nlog just has a filtering system, and we could add values to it when needed, that could prevent keys from showing up in logged URLs and stuff

fearnlj01 commented 1 year ago

Swapped out the filtering with the latest two commits to filter out (via nlog config) any error message containing password, token and key when the log message starts with Settings.. Restarted a few times and the dumped settings look good to me.

For the sake of including the links... The nlog documentation pages I used for this was 'Filtering log messages' & 'When filter'.

Haven't added the filters to the console output, only the file output - not sure if it'd be best to filter both?

da3dsoul commented 1 year ago

Looks good