nikhil1raghav / kindle-send

Send webpages, documents and bookmarks to kindle
GNU Affero General Public License v3.0
199 stars 23 forks source link

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

Closed kseistrup closed 2 years ago

kseistrup commented 2 years ago

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.

bronikowski commented 2 years ago

I was about to open the same issue.

nikhil1raghav commented 2 years ago

Fixed in release 1.0.2

kseistrup commented 2 years ago

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.

nikhil1raghav commented 2 years ago

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

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

kseistrup commented 2 years ago

Right?

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

In both cases:

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).

nikhil1raghav commented 2 years ago

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 .

kseistrup commented 2 years ago

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.

nikhil1raghav commented 2 years ago

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