password.fetch command applies path normalization to non-path parameters

[s-hamann]

Problem Description

I configured vdirsyncer to get the password from the user keyring:

password.fetch = ["command", "secret-tool", "lookup", "URL", "https://example.com/"]

This used to work fine before version 0.19.0. Since 0.19.0, however, the configured parameters get changed and, thus, secret-tool does not find the password any more:

vdirsyncer -vdebug sync
debug: Fetching value for password.fetch with command strategy.
error: Unknown error occurred: Command '['secret-tool', 'lookup', 'URL', 'https:/example.com']' returned non-zero exit status 1.
error: Use `-vdebug` to see the full traceback.
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/__init__.py", line 32, in inner
debug:     f(*a, **kw)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/__init__.py", line 150, in sync
debug:     asyncio.run(main(collections))
debug:   File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
debug:     return loop.run_until_complete(main)
debug:   File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
debug:     return future.result()
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/__init__.py", line 133, in main
debug:     async for collection, config in prepare_pair(
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/tasks.py", line 24, in prepare_pair
debug:     await collections_for_pair(
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/discover.py", line 57, in collections_for_pair
debug:     cache_key = _get_collections_cache_key(pair)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/discover.py", line 32, in _get_collections_cache_key
debug:     pair.config_b,
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/utils.py", line 158, in __get__
debug:     obj.__dict__[self.__name__] = result = self.fget(obj)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/config.py", line 271, in config_b
debug:     return self._config.get_storage_args(self.name_b)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/config.py", line 204, in get_storage_args
debug:     return expand_fetch_params(args)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/fetchparams.py", line 24, in expand_fetch_params
debug:     config[newkey] = _fetch_value(config[key], key)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/utils.py", line 189, in wrapper
debug:     return f(*args, **kwargs)
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/fetchparams.py", line 61, in _fetch_value
debug:     rv = strategy_fn(*opts[1:])
debug:   File "/usr/lib/python3.10/site-packages/vdirsyncer/cli/fetchparams.py", line 85, in _strategy_command
debug:     stdout = subprocess.check_output(expanded_command, text=True, shell=shell)
debug:   File "/usr/lib/python3.10/subprocess.py", line 421, in check_output
debug:     return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
debug:   File "/usr/lib/python3.10/subprocess.py", line 526, in run
debug:     raise CalledProcessError(retcode, process.args,

The key information is here, I believe:

error: Unknown error occurred: Command '['secret-tool', 'lookup', 'URL', 'https:/example.com']' returned non-zero exit status 1.

vdirsyncer calls secret-tool lookup URL https:/example.com instead of secret-tool lookup URL https://example.com/. The difference is subtle: vdirsyncer stripped two /.

I guess that the cause is the commit c254b4ad1d86745fc0389eea02ea687d48c4feee, which applies path normalization to every parameter of the fetch command. In my case, this causes to URL to be unsuitably normalized.

My Environment

• vdirsyncer 0.19.0
• Python 3.10.9
• running on Gentoo Linux, all relevant packages installed from Gentoo's package repository

[WhyNotHugo]

The bug is indeed due to how expand_path works.

I believe we should expand variables on all parameters, but not normalise paths like this.

Cc: @Witcher01

[Witcher01]

Good catch, and sorry for the bug, I didn't think about this scenario!

I agree: variables should be expanded, but paths shouldn't be normalised like this. I'm unsure whether the call to os.path.normpath in expand_path is needed at all as a non-normalised path should be just fine to work with, but correct me if I'm wrong. Slashes could then be converted another way.

https://github.com/pimutils/vdirsyncer/blob/90b6ce1d04af2d1e98f2f4a9956c67d8c3abe15d/vdirsyncer/utils.py#L20-L24

Alternatively, one could detect whether an argument is a path at all and apply expand_path accordingly. This is probably preferable and avoids indirectly modifying other code, potentially introducing more bugs.

[s-hamann]

Alternatively, one could detect whether an argument is a path at all and apply expand_path accordingly.

I believe, detecting paths is inherently hard. Sure, you can reliably detect URLs with a protocol indicator and rule them out. But you're still left with e.g. 10/min, his/her or cGF0aCBvciBub3Q/Cg==. All of these might be relative paths. Even if you check for existence, I don't think there's a reliable way to tell what the user meant. Therefore, I'd advise for the least-surprising option: Pass parameters as supplied by the user.

[WhyNotHugo]

I think expanding variables and a leading ~ makes sense, but I'm not sure what we gain by normalising filepaths. We do both things in one function right now, but normalising paths likely doesn't belong in the expand_path function.

It would be good to double check all usages of the function tho.

[alembiq]

I believe I found another case when this "feature/bug" is impacting me (version 0.19.2) - using sed while getting value from a file :(

username.fetch = ["command", "sh", "-c", "/sed -n '/^USERNAME=/s///p' /run/user/1111/secrets/charles/nextcloud"]
password.fetch = ["shell", "sed -n '/^PASSWORD=/s///p'  /run/user/1111/secrets/charles/nextcloud"]

The funny thing is that while using vdirsyncer showconfig, the values are returned correctly, but while executing the command, all the slashes are converted into a single one.

vdirsyncer -vdebug sync
debug: Fetching value for username.fetch with command strategy.
/nix/store/237dff1igc3v09p9r23a37yw8dr04bv6-gnused-4.9/bin/sed: -e expression #1, char 15: unterminated `s' command
error: Unknown error occurred: Command '['sh', '-c', "sed -n '/^USERNAME=/s/p' /run/user/1111/secrets/charles/nextcloud"]' returned non-zero exit status 1.
error: Use `-vdebug` to see the full traceback.
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/__init__.py", line 32, in inner
debug:     f(*a, **kw)
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/__init__.py", line 150, in sync
debug:     asyncio.run(main(collections))
debug:   File "/nix/store/gd3shnza1i50zn8zs04fa729ribr88m9-python3-3.11.8/lib/python3.11/asyncio/runners.py", line 190, in run
debug:     return runner.run(main)
debug:            ^^^^^^^^^^^^^^^^
debug:   File "/nix/store/gd3shnza1i50zn8zs04fa729ribr88m9-python3-3.11.8/lib/python3.11/asyncio/runners.py", line 118, in run
debug:     return self._loop.run_until_complete(task)
debug:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/gd3shnza1i50zn8zs04fa729ribr88m9-python3-3.11.8/lib/python3.11/asyncio/base_events.py", line 654, in run_until_complete
debug:     return future.result()
debug:            ^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/__init__.py", line 133, in main
debug:     async for collection, config in prepare_pair(
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/tasks.py", line 24, in prepare_pair
debug:     await collections_for_pair(
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/discover.py", line 57, in collections_for_pair
debug:     cache_key = _get_collections_cache_key(pair)
debug:                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/discover.py", line 32, in _get_collections_cache_key
debug:     pair.config_b,
debug:     ^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/utils.py", line 158, in __get__
debug:     obj.__dict__[self.__name__] = result = self.fget(obj)
debug:                                            ^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/config.py", line 271, in config_b
debug:     return self._config.get_storage_args(self.name_b)
debug:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/config.py", line 204, in get_storage_args
debug:     return expand_fetch_params(args)
debug:            ^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/fetchparams.py", line 24, in expand_fetch_params
debug:     config[newkey] = _fetch_value(config[key], key)
debug:                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/utils.py", line 189, in wrapper
debug:     return f(*args, **kwargs)
debug:            ^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/fetchparams.py", line 61, in _fetch_value
debug:     rv = strategy_fn(*opts[1:])
debug:          ^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/vgipkfar0ykiplbzp947wwd9l1l3zz21-python3.11-vdirsyncer-0.19.2/lib/python3.11/site-packages/vdirsyncer/cli/fetchparams.py", line 85, in _strategy_command
debug:     stdout = subprocess.check_output(expanded_command, text=True, shell=shell)
debug:              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/gd3shnza1i50zn8zs04fa729ribr88m9-python3-3.11.8/lib/python3.11/subprocess.py", line 466, in check_output
debug:     return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
debug:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
debug:   File "/nix/store/gd3shnza1i50zn8zs04fa729ribr88m9-python3-3.11.8/lib/python3.11/subprocess.py", line 571, in run
debug:     raise CalledProcessError(retcode, process.args,