Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
s-hamann opened this issue Dec 23, 2022 · 5 comments · May be fixed by #1124
Open

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

s-hamann opened this issue Dec 23, 2022 · 5 comments · May be fixed by #1124

Comments

@s-hamann
Copy link

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
@WhyNotHugo
Copy link
Member

WhyNotHugo commented Dec 23, 2022 via email

@Witcher01
Copy link

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
Copy link
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
Copy link
Member

WhyNotHugo commented Dec 24, 2022 via email

@alembiq
Copy link

alembiq commented 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 to Witcher01/vdirsyncer that referenced this issue Apr 15, 2024
@Witcher01
don't normalize paths in expand_path
1ecd318 
Remove normalizing paths in "expand_path" and expand environment
variables. Paths don't need to be normalized, but doing this on
non-path parameters (i.e. URLs) might cause bugs.

Reported-by: s-hamann
Closes: pimutils#1021
Signed-off-by: Thomas Böhler <[email protected]>
@Witcher01 Witcher01 linked a pull request Apr 15, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

don't normalize paths in expand_path Witcher01/vdirsyncer
4 participants
@WhyNotHugo @s-hamann @Witcher01 @alembiq