Skip to content

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

New issue
New issue
Open
#1124
Open
password.fetch command applies path normalization to non-path parameters#1021
#1124
@s-hamann

Description

@s-hamann
s-hamann
opened on Dec 23, 2022

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 c254b4a, 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

Activity

WhyNotHugo

WhyNotHugo commented on Dec 23, 2022

@WhyNotHugo
WhyNotHugo
on Dec 23, 2022 via email
Member
Witcher01

Witcher01 commented on Dec 24, 2022

@Witcher01
Witcher01
on Dec 24, 2022

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.

vdirsyncer/vdirsyncer/utils.py

Lines 20 to 24 in 90b6ce1

def expand_path(p: str) -> str:
"""Expand $HOME in a path and normalise slashes."""
p = os.path.expanduser(p)
p = os.path.normpath(p)
return p

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

s-hamann commented on Dec 24, 2022

@s-hamann
s-hamann
on Dec 24, 2022
Author

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

WhyNotHugo commented on Dec 24, 2022

@WhyNotHugo
WhyNotHugo
on Dec 24, 2022 via email
Member
alembiq

alembiq commented on Apr 15, 2024

@alembiq
alembiq
on Apr 15, 2024

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,
Witcher01
added a commit that references this issue on Apr 15, 2024

don't normalize paths in expand_path

1ecd318
Witcher01
linked a pull request that will close this issue on Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @WhyNotHugo@s-hamann@Witcher01@alembiq

      Issue actions
        password.fetch command applies path normalization to non-path parameters · Issue #1021 · pimutils/vdirsyncer