sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
950 stars 405 forks source link

config: `homedir` path should be expanded and/or absolutized #2499

Open dgw opened 11 months ago

dgw commented 11 months ago

It's possible to kinda break things by accident if one sets the core.homedir option to e.g. ~/path/to/somewhere (note the ~).

For example, this breaks finding additional plugins in ~/.sopel/plugins, even though it should be equivalent to the default value:

[core]
nick = homedir_bot
# server, port, etc.
# skip the boilerplate
homedir = ~/.sopel
$ ls ~/.sopel/plugins
__pycache__  homedir.py

$ sopel-plugins list -c homedir
[...]
help/setup-entrypoint Sopel Help plugin (help = sopel_help.plugin) [enabled]
invite/python-module invite.py - Sopel Invite Plugin (sopel.modules.invite) [enabled]
[...]

Running the .homedir command provided by homedir.py:

from sopel import plugin

@plugin.command('homedir')
def get_homedir(bot, trigger):
    bot.say('Core: ' + bot.settings.core.homedir)
    bot.say('config: ' + bot.settings.homedir)

… (using core.extra to force looking in /home/dgw/.sopel/plugins too) yields:

<dgw> .homedir with `core.homedir` option set to `~/.sopel`
<homedir_bot> Core: ~/.sopel
<homedir_bot> config: ~/.sopel
*** Quits: homedir_bot
*** Joins: homedir_bot
<dgw> .homedir with `core.homedir` option commented out
<homedir_bot> Core: /home/dgw/.sopel
<homedir_bot> config: /home/dgw/.sopel

With core.homedir set to ~/.sopel, core code (e.g. logging) and plugins like ip, remind, safety, or tell (which save files to bot.settings.homedir) will actually write to /home/dgw/.sopel/~/.sopel. To adapt one of @xnaas' favorite quips, this does exactly what the user said, but probably not what they meant.

I propose that the core.homedir path be passed through os.path.expanduser() and possibly os.path.abspath(), so downstream code that uses it can just take the value as-is without worrying about shell-isms (as sopel-twitter does now). It should be relatively simple for us to remove this foot-gun from the user/plugin-dev experience.

I'm starting out by assigning this to the 8.1 milestone. Left alone, I would put it in 8.0, but in prior IRC discussion the consensus from @Exirel and @SnoopJ was that it can wait for the next minor version.

SnoopJ commented 11 months ago

I propose that the core.homedir path be passed through os.path.expanduser() and possibly os.path.abspath()

Where do you stand on Sopel's treatment of symlinks? abspath() will resolve any relativeness in a path but not links, but there is also realpath() which will resolve links.

I think we want to only consider the canonical path to whatever the user pointed at, even if it is a link, but I thought it was worth asking about.

dgw commented 11 months ago

Unlike a relative path, an absolute path pointing to a symlink is still usable by downstream plugin code. If I open() a symlink I can read (or write) the real file's contents.

The problem with relative paths and unexpanded placeholders like ~ is that they lead to unexpected behavior, a foot-gun which I don't think symlinks share. Or if they do, I haven't found a situation that triggers it.

One possible argument against using realpath() is that the homedir could be printed or logged by plugin code in certain cases, and the real path could reveal filesystem layout information that the bot owner has contrived to hide by using symlinks. (But this is, I admit, kind of a reach.)

More importantly, os.path.realpath()'s documentation says we shouldn't generally need to call it:

Note: This function emulates the operating system’s procedure for making a path canonical, which differs slightly between Windows and UNIX with respect to how links and subsequent path components interact.

Operating system APIs make paths canonical as needed, so it’s not normally necessary to call this function.

Exirel commented 11 months ago

I'm in favor of using expanduser and abspath for homedir.

I'm not in favor of using realpath, as I'd let the OS do that. How simlinks are processed is none of Sopel's business.

I'm in favor of this change for Sopel 8.1 and not 8.0.