search

tulir / gomuks

A terminal based Matrix client written in Go.
https://maunium.net/go/gomuks
GNU Affero General Public License v3.0
1.3k stars 124 forks source link

Change the way of logging gomuks? #429

Closed nileshpatra closed 11 months ago

nileshpatra commented 12 months ago

I recently happened to package gomuks fro debian (currently in new queue) but some discussion on IRC:

-> I'd argue that it's bad practice to have it write debug logs by default, so I'd patch it to only do that when called with a special flag.
-> Write access in world writable directories is an invitation for security bugs. And logs don't belong in /tmp. So please try to make sure that the default install (or better any reasonable config) does not log to files in /tmp
-> Best would be if upstream had a better plan with regard to logging than dump it in /tmp and hope the user reboots often enough.
-> Making sure the tmp file access is secure against the whole lot of races and other problems and leaving it in /tmp is possible in principle. Just not very nice.

Do you think you can do something on those lines to mitigate these concerns? IMHO the security bit is a valid point.

nileshpatra commented 12 months ago

Is ~/.config/gomuks-logs/debug.log or same dir in ~/.cache a sensible enough place for this? Does it also make sense to deisable logging by default and start it only when user asks for it?

nileshpatra commented 12 months ago

/cc @tulir @alexmyczko

nileshpatra commented 12 months ago

Here's the spec for it: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html ~/.config/state

n-peugnet commented 12 months ago

I wouldn't use .config for log files. I would prefer .local/share. IMO .config should only contain config files, for instance ones that should be backed up. .local/share seems more appropriate for random data. It is corresponding to the $XDG_DATA_HOME in the page you linked @nileshpatra.

Also after reading this page it seems that you made a mistake, ~/.config/state should be ~/.local/state.

nileshpatra commented 12 months ago

On 6 July 2023 12:27:10 pm IST, Nicolas Peugnet @.***> wrote:

Also after reading this page it seems that you make a mistake, ~/.config/state should be ~/.local/state.

Yes, absolutely. Made a mistake while typing. My bad. But how do you think about changing the logging to this loc?

It might also be a good idea for the user to specify whether or not they even want the logging, somewhere in the logging options. By default it can be set to off or on, whatever you prefer.

That said, would you have time to prepare a patch? It'd be quite helpful from (Linux) distribution's perspective.

nileshpatra commented 12 months ago

I found some time and have opened #430 to address these issues. I'd be great if I can get a review.

nileshpatra commented 11 months ago

Thanks for merging the PR and closing the bug. I had one ask -- would it be possible to cut a patch release at this point and update the debug docs to reflect the change in debug log thingy starting from the new version?

This is so because I could update the package to the new patch release in debian and it'd work. Otherwise, I'd have to backport this patch (not an issue) but users maybe confused to see a different behaviour than upstream in debian package for the same (0.3.0) version.

Please let me know what you think?

nileshpatra commented 11 months ago

Thanks for merging the PR and closing the bug. I had one ask -- would it be possible to cut a patch release at this point and update the debug docs to reflect the change in debug log thingy starting from the new version?

This is so because I could update the package to the new patch release in debian and it'd work. Otherwise, I'd have to backport this patch (not an issue) but users maybe confused to see a different behaviour than upstream in debian package for the same (0.3.0) version.

/cc: @tulir given that there have not been many commits (and breaking changes) since last release. Is this something you'd consider doing? I'd appreciate any response.