kindle-send icon indicating copy to clipboard operation
kindle-send copied to clipboard

[Linux] Configuration files do not belong in the root of the user's home directory

Open kseistrup opened this issue 1 year ago • 6 comments

Describe the bug

The help text says:

$ kindle-send -h 2>&1 | head -3
Usage of kindle-send:
  -config string
    	Path to the configuration file (optional) (default "/home/myusername/KindleConfig.json")

Expected behavior

Never put files in the root of the user's home directory unless the user explicitly asks you to do so.

On Linux, and similar OSes, kindle-send ought to put its configuration file(s) in the $XDG_CONFIG_HOME/kindle-send/ directory. If the $XDG_CONFIG_HOME environment variable is unset or empty, use $HOME/.config/kindle-send/ instead.

This is the path of least surprise.

kseistrup avatar Sep 06 '22 08:09 kseistrup

I was about to open the same issue.

bronikowski avatar Sep 06 '22 08:09 bronikowski

Fixed in release 1.0.2

nikhil1raghav avatar Sep 07 '22 04:09 nikhil1raghav

I'm sorry, but it's still not okay:

V1.0.2 puts the configuration file in

/home/myusername/.config/KindleConfig.json

It should have been:

/home/myusername/.config/kindle-send/KindleConfig.json

or it may collide with other applications that uses the filename KindleConfig.json.


$ echo $XDG_CONFIG_HOME
/home/myusername/.config

If the application configuration directory doesn't exist, the application should create it.

Before creating the application configuration directory, umask should be set to at least 0027 (in octal notation) in order to ensure that the directory and file are not world readable.

kseistrup avatar Sep 07 '22 05:09 kseistrup

Sure, I'll make these changes. Thanks for explaining the rationale behind keeping separate folder for config and clear instructions.

So, new behavior will be

  • Save config in $XDG_CONFIG_HOME/kindle-send
  • If $XDG_CONFIG_HOME doesn't exist, store in $HOME/.config/kindle-send Configuration directly shouldn't be world readable

Right? Can I add you as a reviewer, when I raise a PR for this?

nikhil1raghav avatar Sep 07 '22 15:09 nikhil1raghav

Right?

I think it's right, but I'd like to be explicit about the existance:

  • If the environment variable $XDG_CONFIG_HOME is set: Store configuration files in the directory $XDG_CONFIG_HOME/kindle-send/.
  • If the environment variable $XDG_CONFIG_HOME is unset: Store configuration files in the directory $HOME/.config/kindle-send/.

In both cases:

  • It is up to the application to make sure its configuration directory exists. It's okay to notify the user that it has been created if it was missing, but please don't bother them with a yes/no prompt.
  • It is prudent to use the octal umask 0027 when creating the configuration directory and file(s) so that neither are world readable.
    • If the configuration directory already exists and has other permissions, don't touch the permissions, but create the configuration file such that is it world-unreadable (if you overwrite an existing configuration file it will keep its previous permissions, I think).

Can I add you as a reviewer, when I raise a PR for this?

Sure thing (I'm not a go programmer, though, just dabbling a little in python).

kseistrup avatar Sep 07 '22 17:09 kseistrup

Regarding world readability of config file. I think it is better to allow users to edit the file directly if they want to change things instead of deleting the old config and going through that prompt again.

For now I'm thinking, I'll take time to incorporate world unreadability. But surely store the file in $XDG_CONFIG_HOME/kindle-send or $HOME/.config/kindle-send .

nikhil1raghav avatar Sep 08 '22 03:09 nikhil1raghav

I am still not sure it is correct (commit 6ff9c1f717ccf98ecd7f160493d480b8cfd528b5):

$ # Let's first try with an empty $XDG_CONFIG_HOME
$ env XDG_CONFIG_HOME= ./kindle-send -h 2>&1 | head -4
Config home not set, will look for config at  /home/myusername/.config/kindle-send
Usage of ./kindle-send:
  -config string
    	Path to the configuration file (optional) (default "/home/myusername/.config/kindle-send/KindleConfig.json")
$ # That looked fine :)
$ # Next, let's try with XDG_CONFIG_HOME=/tmp/test
$ env XDG_CONFIG_HOME=/tmp/test ./kindle-send -h 2>&1 | head -3
Usage of ./kindle-send:
  -config string
    	Path to the configuration file (optional) (default "/tmp/test/KindleConfig.json")
$ # Hm, “kindle-send” is missing from the path…

It seems it appends kindle-send only when $XDG_CONFIG_HOME is unset. Let's check config/config.go:

 39		if len(xdgConfigHome)==0{
 40			configFolder =path.Join(user.HomeDir, ".config")
 41			configFolder = path.Join(configFolder, ConfigFolderName)
 42			util.Cyan.Println("Config home not set, will look for config at ", configFolder)
 43		}else{
 44			configFolder = xdgConfigHome
 45		}
 46		_=os.Mkdir(configFolder, os.ModePerm)
 47
 48		return path.Join(configFolder, "KindleConfig.json"), nil

Indeed!

Suggestion: Move the currfent line 41 (configFolder = path.Join(configFolder, ConfigFolderName) down below the current line 45 (}). In that way the if clause opened in line 39 will result in configFolder set to either $HOME/.config or $XDG_CONFIG_HOME, and kindle-send will then be appended.

kseistrup avatar Sep 13 '22 06:09 kseistrup

Sorry to miss that, It should be fixed now. Working on some changes related to image optimization, will create a release after that

nikhil1raghav avatar Sep 13 '22 15:09 nikhil1raghav