diff --git a/CLAUDE.md b/CLAUDE.md index e1bc635..30b98a8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,14 +35,16 @@ Things that bite in this suite: `cli.main` wires the three halves together: `load_conf` produces the configuration, `get_env` turns it into an environment, and `run_cmd`/`run_shell` hand that environment to a shell. Errors reach the user through `logs.error`, which exits with status 1; `logs.vlog` output only appears under `--verbose` and is the first thing to reach for when debugging a configuration. -**`conf.py` — where the configuration comes from.** Later sources win, merged deeply by `merge_conf`: defaults, then `~/.keycmd`, then every `.keycmd` found walking up from the working directory (outermost first), then the first `pyproject.toml` found walking up, whose `[tool.keycmd]` table is used. `find_file` stops at a `.git` directory, at the home folder, and at the root of the file system, so the walk never escapes a repository. `USERPROFILE` is a module attribute so tests can point the user config elsewhere. The merged result is `cast` to `Conf` rather than validated: it is user authored, and `get_env` reports violations as user errors. +**`conf.py` — where the configuration comes from.** Later sources win, merged deeply by `merge_conf`: defaults, then `~/.keycmd`, then every `.keycmd` found walking up from the working directory (outermost first), then the first `pyproject.toml` found walking up, whose `[tool.keycmd]` table is used. Both searches cover the same ground, so `load_conf` collects them in a single pass over `walk_up`, which stops at a `.git` directory, at the home folder, and at the root of the file system, so the walk never escapes a repository. `USERPROFILE` is a module attribute so tests can point the user config elsewhere. The merged result is `cast` to `Conf` rather than validated: it is user authored, and `get_env` reports violations as user errors. **`creds.py` — configuration to environment.** `get_env` copies `os.environ` and adds a variable per entry of `[keys]`, looking each credential up in the keyring; `[aliases]` re-expose an existing key under another name with different `b64`/`format` options, without a second keyring lookup. `expose` applies `format` first and `b64` second, which is what makes `{username}:{password}` basic auth work. **`wsl.py` — the boundary between WSL and Windows.** WSL users install keycmd on Windows, which leaves it a Windows process with a Windows idea of a shell. `from_wsl` decides whether it was called from a distro — a `wsl.exe`/`wslhost.exe` ancestor decides it, a Windows shell found first decides against it, and a UNC working directory settles the rest — after which `run_shell`/`run_cmd` hand the work to `wsl.exe` rather than to a Windows shell. A UNC working directory also names the distro, which `wsl_argv` passes as `--distribution` so that a second distro does not send the command to the default one. `KEYCMD_WSL` overrides that decision in either direction. Neither side of the boundary inherits the other's environment, so `share_env` lists the exposed variables in `WSLENV`, which is the only thing that crosses. -**`shell.py` — where the platform differences live.** `get_shell` asks shellingham which shell invoked the process and falls back to `$SHELL` or `%COMSPEC%`. `cmd` takes `/C` and keeps the command's arguments separate; every other shell takes `-c` and a single joined string. `exec` replaces the process with `execvpe` on posix, but runs a subprocess on Windows, which has no equivalent; `USE_SUBPROCESS` and the `IS_WINDOWS`/`IS_POSIX` flags are module attributes so tests can drive both paths on either platform. +**`shell.py` — where the platform differences live.** `get_shell` asks shellingham which shell invoked the process and falls back to `$SHELL` or `%COMSPEC%`. `cmd` takes `/C` and keeps the command's arguments separate; every other shell takes `-c` and the single string `join_cmd` builds. One argument is already a command line — the form the README recommends — and is handed over as typed, so the shell interprets it; several arguments are an argv vector, and `quote` protects each so that the shell does not split them into words a second time. `quote` reaches for `shlex` for posix shells and doubles the quote for powershell, which also needs the call operator once its command name ends up quoted. `exec` replaces the process with `execvpe` on posix, but runs a subprocess on Windows, which has no equivalent; `USE_SUBPROCESS` and the `IS_WINDOWS`/`IS_POSIX` flags are module attributes so tests can drive both paths on either platform. ## Conventions The package is fully annotated and ships a `py.typed` marker, so `ANN` rules apply to `keycmd/` while the test suite is exempt. `ty` is configured with `python-version = "3.13"`, the oldest supported release, so it catches typing features that are newer than `requires-python` allows. CI matches that split: the latest Python on all three platforms, plus a single job on the oldest. + +keycmd sits in front of every command a user runs through it, so its own startup is latency the user pays each time. A handful of imports cost more than everything else in the package together, and none of them is needed on every run: `keyring` (and the backend it goes on to discover) only once a credential is looked up, `pprint` only under `--verbose`, and `subprocess` only on the Windows path, which cannot replace its own process. They are imported inside the function that needs them, which keeps `import keycmd.cli` at roughly a third of what it would otherwise cost, and `test_cli_import_stays_lean` fails if one of them wanders back up to module level. Reach for a lazy import when adding a dependency that most runs will not touch, and leave the rest at the top of the file where they belong. diff --git a/README.md b/README.md index e3578c6..1dc42bd 100644 --- a/README.md +++ b/README.md @@ -176,6 +176,17 @@ There are two main ways to use the CLI: The first is the most preferred method, since your secrets will only be exposed as environment variables during a one-off command. The latter is less preferable, but can be convenient if you are debugging some process that depends on the credentials you are exposing. +Quoting the whole command as one argument is what lets you use your shell's syntax inside it, as in `keycmd 'echo $SECRET | tr a-z A-Z'`: keycmd hands that line to your shell exactly as you typed it, and your shell does the rest. + +You can also write the command out as separate arguments, and then keycmd keeps them separate: + +```bash +# arrives as a single argument, spaces and all +keycmd mytool --message 'hello world' +``` + +Since each argument is passed on as the word it was, your shell's syntax is *not* interpreted a second time in this form. If you want `$SECRET` expanded, either let your own shell expand it, or use the single argument form above. + ## Configuration > **Note** @@ -390,8 +401,9 @@ keycmd: merged config: 'ARTIFACTS_TOKEN_B64': {'b64': True, 'credential': 'korijn@poetry-repository-main', 'username': 'korijn'}}} -keycmd: exposing credential korijn@poetry-repository-main belonging to user korijn as environment variable ARTIFACTS_TOKEN (b64: False) -keycmd: exposing credential korijn@poetry-repository-main belonging to user korijn as environment variable ARTIFACTS_TOKEN_B64 (b64: True) +keycmd: keyring backend: +keycmd: exposing credential korijn@poetry-repository-main with user korijn as environment variable ARTIFACTS_TOKEN (b64: False, format: None) +keycmd: exposing credential korijn@poetry-repository-main with user korijn as environment variable ARTIFACTS_TOKEN_B64 (b64: True, format: None) keycmd: detected shell: C:\Windows\System32\cmd.exe keycmd: running command: ['C:\\Windows\\System32\\cmd.exe', '/C', 'echo', '%ARTIFACTS_TOKEN_B64%'] aSdtIG5vdCB0aGF0IHN0dXBpZCA6KQ== @@ -403,6 +415,19 @@ Since keycmd uses keyring as its backend, you're not limited to just working wit See the [third party backends](https://github.com/jaraco/keyring/#third-party-backends) list for all options. +### Startup time + +Left to itself, keyring works out which backend to use by loading every backend registered by every installed package and picking the most suitable one. That search runs on each `keycmd` invocation and, on a machine with a few packages installed, costs more time than the whole of the rest of a `keycmd` run put together. + +If that shows up in your shell, name the backend you already know you want, and keyring will load that one instead of going looking: + +```bash +# in your shell profile; use the backend your platform actually uses +export PYTHON_KEYRING_BACKEND=keyring.backends.SecretService.Keyring +``` + +`keyring --list-backends` prints the names to choose from, and `keycmd --verbose` will tell you which one ends up being used. The setting is keyring's own, so it applies to everything else using keyring too. + ## Development This project uses [uv](https://docs.astral.sh/uv/) for dependency management, [ruff](https://docs.astral.sh/ruff/) for linting and formatting, and [ty](https://docs.astral.sh/ty/) for type checking. @@ -425,7 +450,7 @@ uv run pytest tests ### Testing -CI runs the test suite on Windows, macOS and Linux on the latest Python, plus one job on the oldest supported Python to catch anything newer than it allows. The suite adapts to the platform it runs on: it exercises every shell of the platform that is installed (`sh`, `bash` and `zsh` on posix, `cmd`, `powershell` and `pwsh` on Windows), and it skips the process replacement tests on Windows, which has no `execvpe`. +CI runs the test suite on Windows, macOS and Linux on the latest Python, plus one job on the oldest supported Python to catch anything newer than it allows. The suite adapts to the platform it runs on: it exercises every shell of the platform that is installed (`sh`, `bash` and `zsh` on posix, `cmd` and `powershell` on Windows, and `pwsh` on either, since it installs everywhere and quotes its own way), and it skips the process replacement tests on Windows, which has no `execvpe`. The tests that read and write credentials need a real OS keyring that can be unlocked without user interaction. They are skipped with a message if there is no such keyring, so the rest of the suite still runs. Set `KEYCMD_REQUIRE_OS_KEYRING=1` to turn those skips into failures instead; CI sets it so that a broken keyring setup can't quietly reduce the coverage of a run. diff --git a/keycmd/conf.py b/keycmd/conf.py index dacaf39..9f05c59 100644 --- a/keycmd/conf.py +++ b/keycmd/conf.py @@ -1,9 +1,9 @@ import tomllib +from collections.abc import Iterator from pathlib import Path -from pprint import pformat -from typing import Any, Literal, NotRequired, TypedDict, cast, overload +from typing import Any, NotRequired, TypedDict, cast -from .logs import vlog +from .logs import vlog, vlog_pretty class KeyConf(TypedDict): @@ -54,27 +54,16 @@ def load_pyproj(path: Path) -> dict[str, Any]: return data.get("tool", {}).get("keycmd", {}) -@overload -def find_file(fname: str, first_only: Literal[True] = True) -> Path | None: ... +def walk_up() -> Iterator[Path]: + """Yield the working directory and its parents, nearest first - -@overload -def find_file(fname: str, first_only: Literal[False]) -> list[Path]: ... - - -def find_file(fname: str, first_only: bool = True) -> Path | list[Path] | None: - """Find a file by walking up the filesystem, starting at cwd""" + The walk stops at a git repository, so that it never leaves one, and + otherwise just below the home folder or at the root of the file system. + """ cur = Path.cwd() home = Path.home() - results: list[Path] = [] while True: - candidate = cur / fname - if candidate.is_file(): - hit = candidate.resolve() - if first_only: - return hit - else: - results.append(hit) + yield cur # don't search outside git repositories if (cur / ".git").is_dir(): break @@ -85,12 +74,6 @@ def find_file(fname: str, first_only: bool = True) -> Path | list[Path] | None: if cur.parent == cur: break cur = cur.parent - if not first_only: - # return .keycmd files in order in which they should - # be loaded and merged - results.reverse() - return results - return None def defaults() -> dict[str, Any]: @@ -129,9 +112,20 @@ def load_conf() -> Conf: vlog(f"loading config file {user_keyconf}") conf = merge_conf(conf, load_toml(user_keyconf)) - # .keycmd - local_keycmds = find_file(".keycmd", first_only=False) - for local_keycmd in local_keycmds: + # both searches cover the same ground, so they share a single walk + local_keycmds: list[Path] = [] + pyproj: Path | None = None + for directory in walk_up(): + candidate = directory / ".keycmd" + if candidate.is_file(): + local_keycmds.append(candidate.resolve()) + if pyproj is None: + candidate = directory / "pyproject.toml" + if candidate.is_file(): + pyproj = candidate.resolve() + + # .keycmd, outermost first, so that the nearest one wins + for local_keycmd in reversed(local_keycmds): if local_keycmd == user_keyconf: vlog(f"skipping config file {local_keycmd} (already loaded)") continue @@ -139,12 +133,11 @@ def load_conf() -> Conf: conf = merge_conf(conf, load_toml(local_keycmd)) # pyproject.toml - pyproj = find_file("pyproject.toml") if pyproj is not None: vlog(f"loading config file {pyproj}") conf = merge_conf(conf, load_pyproj(pyproj)) - vlog(f"merged config:\n{pformat(conf)}") + vlog_pretty("merged config:\n", conf) # the config is user authored, so this is a statement of the shape keycmd # expects rather than a guarantee; get_env reports violations as user errors diff --git a/keycmd/creds.py b/keycmd/creds.py index fdee4c1..8021b0b 100644 --- a/keycmd/creds.py +++ b/keycmd/creds.py @@ -1,14 +1,12 @@ import base64 from os import environ -import keyring - -from .conf import Conf +from .conf import AliasConf, Conf, KeyConf from .logs import error, vlog from .wsl import share_env -# credential, username, password, apply_b64, format string -KeyData = tuple[str, str, str, bool, str | None] +# credential, username, password +KeyData = tuple[str, str, str] def b64(value: str) -> str: @@ -36,52 +34,72 @@ def expose( env[key] = password +def expose_conf( + env: dict[str, str], + name: str, + data: KeyData, + src: KeyConf | AliasConf, + what: str, +) -> None: + """Expose a credential under the b64 and format options of one config entry + + A key and an alias differ in where the credential comes from, not in + what happens to it on the way into the environment. + """ + apply_b64 = src.get("b64", False) + format_string = src.get("format") + expose(env, name, *data, apply_b64, format_string) + vlog( + f"{what} as environment variable {name}" + f" (b64: {apply_b64}, format: {format_string})" + ) + + def get_env(conf: Conf) -> dict[str, str]: """Load credentials from the OS keyring according to user configuration""" env = environ.copy() key_data: dict[str, KeyData] = {} - for key, src in conf["keys"].items(): - password = keyring.get_password(src["credential"], src["username"]) - if password is None: - error( - f"MISSING credential {src['credential']}" - f" with user {src['username']}" - f" as it does not exist" + keys = conf["keys"] + if keys: + # keyring, and the backend it goes on to discover, together cost + # more time and memory than everything else keycmd does; a run that + # looks up no credential should not have to pay for either + import keyring + + # which backend keyring settles on decides where the credentials + # come from, so it is the first thing to check when they are not + # the ones that were expected + vlog(f"keyring backend: {keyring.get_keyring()}") + + for key, src in keys.items(): + credential = src["credential"] + username = src["username"] + password = keyring.get_password(credential, username) + if password is None: + error( + f"MISSING credential {credential}" + f" with user {username}" + f" as it does not exist" + ) + key_data[key] = (credential, username, password) + expose_conf( + env, + key, + key_data[key], + src, + f"exposing credential {credential} with user {username}", ) - apply_b64 = src.get("b64", False) - format_string = src.get("format") - key_data[key] = ( - src["credential"], - src["username"], - password, - apply_b64, - format_string, - ) - expose(env, key, *key_data[key]) - vlog( - f"exposing credential {src['credential']}" - f" with user {src['username']}" - f" as environment variable {key}" - f" (b64: {apply_b64}, format: {format_string})" - ) - for alias, alias_src in conf.get("aliases", {}).items(): + aliases = conf.get("aliases", {}) + for alias, alias_src in aliases.items(): + # the credential is already in hand, only the options differ data = key_data.get(alias_src["key"]) if data is None: error(f"MISSING alias key {alias_src['key']}") - # re-use base data but replace apply_b64 and format_string - credential, username, password, _, _ = data - apply_b64 = alias_src.get("b64", False) - format_string = alias_src.get("format") - expose(env, alias, credential, username, password, apply_b64, format_string) - vlog( - f"aliasing {alias_src['key']}" - f" as environment variable {alias}" - f" (b64: {apply_b64}, format: {format_string})" - ) + expose_conf(env, alias, data, alias_src, f"aliasing {alias_src['key']}") # an environment does not cross the boundary between WSL and windows # by itself, whichever side of it the command ends up running on - share_env(env, [*conf["keys"], *conf.get("aliases", {})]) + share_env(env, [*keys, *aliases]) return env diff --git a/keycmd/logs.py b/keycmd/logs.py index 5d33b6e..e180d93 100644 --- a/keycmd/logs.py +++ b/keycmd/logs.py @@ -19,7 +19,20 @@ def log(msg: object, err: bool = False) -> None: def vlog(msg: object) -> None: if _verbose: - print(f"keycmd: {msg}") + log(msg) + + +def vlog_pretty(prefix: str, value: object) -> None: + """Log a value under --verbose, pretty printed over as many lines as it takes + + pprint costs more to import than everything else keycmd reaches for in + the standard library, and only this function ever needs it, so it stays + out of the import path until a verbose run asks for it. + """ + if _verbose: + from pprint import pformat + + log(f"{prefix}{pformat(value)}") def error(msg: object) -> NoReturn: diff --git a/keycmd/shell.py b/keycmd/shell.py index c1ea0aa..1a07040 100644 --- a/keycmd/shell.py +++ b/keycmd/shell.py @@ -1,27 +1,32 @@ import os +import shlex from collections.abc import Mapping, Sequence from pathlib import Path -from pprint import pformat -from subprocess import run from sys import exit from typing import NoReturn from shellingham import ShellDetectionFailure, detect_shell -from .logs import vlog, vwarn +from .logs import vlog, vlog_pretty, vwarn from .wsl import cmd_argv, from_wsl, shell_argv USE_SUBPROCESS: bool = False # exposed for testing IS_WINDOWS: bool = os.name == "nt" IS_POSIX: bool = os.name == "posix" +# shells that take -c, but do not quote the way a posix shell does +POWERSHELL: frozenset[str] = frozenset({"powershell", "pwsh"}) + def exec(args: list[str], env: Mapping[str, str] | None = None) -> NoReturn: if env is None: env = os.environ if USE_SUBPROCESS or IS_WINDOWS: # windows does not support process replacement - # as well as posix systems do + # as well as posix systems do; the posix path below never spawns a + # subprocess, so it does not pay to import one either + from subprocess import run + p = run(args, shell=False, env=env) exit(p.returncode) # i know this looks like a bug @@ -50,6 +55,55 @@ def get_shell() -> tuple[str, str]: return shell_name, shell_path +def quote(shell_name: str, arg: str) -> str: + """Quote one argument so that a shell hands it on as a single word + + Everything that is not powershell is quoted the posix way, which + covers every shell this is tested against and the great majority of + what shellingham can detect. The exotic ones it also detects, csh and + fish and nu among them, spell quoting their own way, and an argument + that needs quoting may not survive one intact. An argument that needs + no quoting is untouched, so the common command is unaffected either + way. + + Quoting an argument is not always enough to deliver it. Windows + powershell passes arguments to a native command the way it always + has, which drops an embedded double quote and an empty argument no + matter how they are written; powershell 7.3 fixed that, and pwsh + carries both. cmd reaches a command through the windows command line, + which cannot hold a newline at all. + """ + quoted = shlex.quote(arg) + if shell_name not in POWERSHELL or quoted == arg: + # a posix shell, or an argument that needs no quoting in any shell + return quoted + # where a posix shell ends a single quoted string to spell a quote, + # powershell doubles the quote and stays inside the string + return "'" + arg.replace("'", "''") + "'" + + +def join_cmd(shell_name: str, cmd: Sequence[str]) -> str: + """Turn a command into the single string a shell takes after -c + + One argument is a command line already. `keycmd 'echo $SECRET'` is the + form the README recommends, and the shell is there precisely to + interpret it, so it is handed over as typed. + + Several arguments are an argv vector, and joining them raw would feed + their contents back to the shell to be split into words a second time. + Quoting each one is what keeps `keycmd mytool 'hello world'` a single + argument by the time mytool sees it. + """ + if len(cmd) == 1: + return cmd[0] + quoted = [quote(shell_name, arg) for arg in cmd] + if shell_name in POWERSHELL and quoted[0] != cmd[0]: + # powershell reads a quoted command name as a string to print, and + # needs the call operator to run it instead + quoted.insert(0, "&") + return " ".join(quoted) + + def run_shell(env: Mapping[str, str] | None = None) -> NoReturn: """Open an interactive shell for the user to interact with.""" @@ -75,7 +129,7 @@ def run_cmd(cmd: Sequence[str], env: Mapping[str, str] | None = None) -> NoRetur opt = "/C" else: opt = "-c" - cmd = [" ".join(cmd)] + cmd = [join_cmd(shell_name, cmd)] full_command = [shell_path, opt, *cmd] - vlog(f"running command: {pformat(full_command)}") + vlog_pretty("running command: ", full_command) exec(full_command, env) diff --git a/tests/conftest.py b/tests/conftest.py index d5446ab..82e4d26 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,7 +25,11 @@ # shells to exercise, if installed POSIX_SHELLS = ("sh", "bash", "zsh") -WINDOWS_SHELLS = ("cmd", "powershell", "pwsh") +WINDOWS_SHELLS = ("cmd", "powershell") +# pwsh installs on every platform keycmd supports, and is the one shell +# taking -c that does not quote the way a posix shell does, so it is worth +# exercising wherever it turns up rather than on windows alone +ANY_PLATFORM_SHELLS = ("pwsh",) # credential used by the keyring backed fixtures KEY = "__keycmd_testß" @@ -41,8 +45,17 @@ def pytest_report_header(config): - """Report which keyring backend the run picked up""" - return f"keyring backend: {keyring.get_keyring()}" + """Report what this run picked up, both of which vary per machine + + Which shells a run covers is otherwise only visible in the ids of the + tests that failed, so a run where they all pass does not say whether a + shell was exercised or simply absent. + """ + shells = ", ".join(shell.name for shell in installed_shells()) + return [ + f"keyring backend: {keyring.get_keyring()}", + f"shells exercised: {shells or 'none'}", + ] @dataclass(frozen=True) @@ -78,10 +91,35 @@ def command_not_found_statuses(self): # posix shells standardize on 127 return {127} + def carries(self, args): + """Can this shell hand these arguments on to a command unchanged? + + A shell keycmd hands a quoted command line to can carry anything, + and the posix shells and pwsh do. The two windows shells reach a + command through the windows command line instead, which is a + narrower thing than an argv vector, and neither limit below is one + that quoting on keycmd's side can lift. + """ + if self.name == "cmd": + # cmd is handed its arguments separately, and what quotes them + # on the way is the windows runtime, which knows nothing of + # cmd's own metacharacters: cmd parses those in any argument + # the runtime saw no reason to quote, and expands %VAR% even + # inside one that it did. A command line is also a line, so a + # newline in an argument ends it early. + return not any(set(arg) & set('&|<>()^%"\n') for arg in args) + if self.name == "powershell": + # windows powershell passes arguments to a native command the + # way it always has, dropping an embedded double quote and an + # empty argument outright. Powershell 7.3 fixed that, so pwsh + # is held to the whole battery. + return all(arg and '"' not in arg for arg in args) + return True + def installed_shells(): """The shells of this platform's candidate list that are installed""" - candidates = WINDOWS_SHELLS if IS_WINDOWS else POSIX_SHELLS + candidates = (WINDOWS_SHELLS if IS_WINDOWS else POSIX_SHELLS) + ANY_PLATFORM_SHELLS found = [] for name in candidates: path = which(name) diff --git a/tests/test_cli.py b/tests/test_cli.py index 825acd5..8fcfe1d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,8 +1,17 @@ +import sys +from subprocess import run + import pytest from keycmd import __version__ from keycmd.cli import cli, main +# modules that cost more to import than the rest of keycmd together, and +# that nothing needs until it is asked for: keyring only once a credential +# is looked up, pprint only under --verbose, subprocess only on the windows +# code path, which cannot replace its own process +LAZY_IMPORTS = ("keyring", "pprint", "subprocess") + def test_cli_version(capfd): main(["--version"]) @@ -10,10 +19,12 @@ def test_cli_version(capfd): def test_cli(capfd, shell_credentials, local_conf, userprofile, subprocess, shell): + # one argument, the form the README recommends, so that the shell + # keycmd hands the command line to is the one that expands the variable var = shell.env_var(local_conf.varname) with pytest.raises(SystemExit) as exc_info: - main(["echo", var]) + main([f"echo {var}"]) assert exc_info.value.args[0] == 0 assert capfd.readouterr().out.strip() == shell_credentials.password @@ -55,6 +66,22 @@ def test_cli_invalid_conf(capfd, ch_tmpdir, userprofile): assert "invalid TOML in" in capfd.readouterr().err +def test_cli_import_stays_lean(): + """Importing the cli does not pay for what a run may never use + + A fresh interpreter, because the test suite has imported keyring long + before this point. Every one of these is a module level import away + from landing back on the startup path of every invocation. + """ + code = ( + "import sys, keycmd.cli;" + f"print(' '.join(m for m in {LAZY_IMPORTS!r} if m in sys.modules))" + ) + p = run([sys.executable, "-c", code], capture_output=True) + assert p.returncode == 0, p.stderr.decode() + assert p.stdout.decode().split() == [] + + def test_cli_extra_args(): command = ["echo", "foo", "-f", "bla", "--something"] args = cli.parse_args(command) diff --git a/tests/test_conf.py b/tests/test_conf.py index 381e75a..661f7ea 100644 --- a/tests/test_conf.py +++ b/tests/test_conf.py @@ -7,11 +7,11 @@ import keycmd.conf from keycmd.conf import ( defaults, - find_file, load_conf, load_pyproj, load_toml, merge_conf, + walk_up, ) @@ -84,32 +84,40 @@ def test_load_pyproj(ch_tmpdir): assert path.name in err.value.args[0] -def test_find_file(ch_tmpdir, monkeypatch, tmp_path): +def found(fname): + """The files the walk turns up, nearest first""" + return [ + candidate.resolve() + for directory in walk_up() + if (candidate := directory / fname).is_file() + ] + + +def test_walk_up(ch_tmpdir, monkeypatch, tmp_path): # the walk stops just below the home folder home = set_home(monkeypatch, tmp_path) p1 = create_path("../.blabla") p2 = create_path("../../.blabla") p3 = create_path("../../../.blabla") create_path(home / ".blabla") - assert find_file(".blabla") == p1 - assert find_file(".blabla", first_only=False) == [p3, p2, p1] + assert found(".blabla") == [p1, p2, p3] + # and at a git repository, so it never leaves one (p2.parent / ".git").mkdir(exist_ok=True, parents=True) - assert find_file(".blabla", first_only=False) == [p2, p1] + assert found(".blabla") == [p1, p2] -def test_find_file_missing(ch_tmpdir, monkeypatch, tmp_path): +def test_walk_up_missing(ch_tmpdir, monkeypatch, tmp_path): set_home(monkeypatch, tmp_path) - assert find_file(".blabla") is None - assert find_file(".blabla", first_only=False) == [] + assert found(".blabla") == [] -def test_find_file_stops_at_filesystem_root(ch_tmpdir, monkeypatch, tmp_path): +def test_walk_up_stops_at_filesystem_root(ch_tmpdir, monkeypatch, tmp_path): # a home folder that is nowhere near the current directory, so the walk # runs all the way into the root of the file system instead of stopping # at the home folder set_home(monkeypatch, tmp_path / "somewhere" / "else") p = create_path(".blabla") - assert find_file(".blabla", first_only=False) == [p] + assert found(".blabla") == [p] def test_merge_conf(): diff --git a/tests/test_creds.py b/tests/test_creds.py index 562339e..360bba5 100644 --- a/tests/test_creds.py +++ b/tests/test_creds.py @@ -100,9 +100,12 @@ def test_get_env_no_keys(os_keyring): assert env == dict(environ) -def test_get_env_verbose(capsys, credentials, verbose): +def test_get_env_verbose(capsys, credentials, os_keyring, verbose): get_env(make_conf(credentials)) out = capsys.readouterr().out + # where the credentials came from, which is the first thing to check + # when they are not the ones that were expected + assert f"keyring backend: {os_keyring}" in out assert ( f"exposing credential {credentials.key}" f" with user {credentials.username}" diff --git a/tests/test_shell.py b/tests/test_shell.py index 89b9da5..907c481 100644 --- a/tests/test_shell.py +++ b/tests/test_shell.py @@ -1,3 +1,4 @@ +import json import sys from os import environ from pprint import pformat @@ -88,15 +89,17 @@ def test_run_cmd_env(capfd, subprocess, shell): env = environ.copy() var_value = "foobar" env[VARNAME] = var_value - var = shell.env_var(VARNAME) + # one argument, the form the README recommends, so that the variable is + # expanded by the shell keycmd hands the command line to + cmd = [f"echo {shell.env_var(VARNAME)}"] with pytest.raises(SystemExit) as exc_info: - run_cmd(["echo", var], env=env) + run_cmd(cmd, env=env) assert exc_info.value.args[0] == 0 assert capfd.readouterr().out.strip() == var_value with pytest.raises(SystemExit) as exc_info: - run_cmd(["echo", var]) + run_cmd(cmd) assert exc_info.value.args[0] == 0 assert capfd.readouterr().out.strip() == shell.unset_env_var(VARNAME) @@ -142,6 +145,90 @@ def test_run_cmd_invocation_per_shell(monkeypatch, shell_name, expected): assert invocations == [([shell_path, *expected], {"FOO": "bar"})] +@pytest.mark.parametrize( + ("shell_name", "cmd", "expected"), + [ + # one argument is a command line already, and is handed over as + # typed, so that the shell interprets it + ("bash", ["echo $FOO bar"], ["-c", "echo $FOO bar"]), + ("powershell", ["echo $env:FOO"], ["-c", "echo $env:FOO"]), + # several arguments are an argv vector, and keep their word + # boundaries rather than being split a second time by the shell + ("bash", ["mytool", "hello world"], ["-c", "mytool 'hello world'"]), + ("bash", ["mytool", "$FOO"], ["-c", "mytool '$FOO'"]), + ("bash", ["mytool", "it's"], ["-c", """mytool 'it'"'"'s'"""]), + # powershell spells an embedded quote by doubling it + ("powershell", ["mytool", "hello world"], ["-c", "mytool 'hello world'"]), + ("powershell", ["mytool", "it's"], ["-c", "mytool 'it''s'"]), + # and needs the call operator once the command name is quoted + ("powershell", ["my tool", "arg"], ["-c", "& 'my tool' arg"]), + # nothing to quote, so nothing changes + ("bash", ["echo", "foo"], ["-c", "echo foo"]), + # cmd keeps the arguments separate and never joins them at all + ("cmd", ["mytool", "hello world"], ["/C", "mytool", "hello world"]), + ], +) +def test_run_cmd_quoting(monkeypatch, shell_name, cmd, expected): + """Arguments survive the trip through the shell as the words they were + + Covers the shells that are not installed on the current platform. + """ + shell_path = f"/path/to/{shell_name}" + monkeypatch.setattr( + keycmd.shell, "detect_shell", lambda pid: (shell_name, shell_path) + ) + invocations = [] + monkeypatch.setattr( + keycmd.shell, "exec", lambda args, env=None: invocations.append(args) + ) + run_cmd(cmd) + assert invocations == [[shell_path, *expected]] + + +# every way a shell might be tempted to read an argument as something +# other than the word it is: quoting of its own, expansions, globs, command +# separators, redirections, and whitespace it would otherwise split on +ROUNDTRIP_ARGS = [ + ["hello world"], + ["it's"], + ['say "hi"'], + ["mixed 'single' and \"double\""], + [r"C:\path\to", "back\\slash"], + ["$HOME", "${X}", "$(id)"], + ["`id`"], + ["*", "?", "[a-z]"], + ["a;b", "a&b", "a|b"], + ["a\nb"], + ["a\tb"], + [""], + ["a!b"], + ["~", "~root"], + ["ünïcødeß"], + [">out", "&1"], + ["(a)", "{b}"], + ["#c", "a#b"], + ["a b'c\"d\\e$f`g;h|i*j"], +] + + +@pytest.mark.parametrize("args", ROUNDTRIP_ARGS, ids=lambda args: repr(args)) +def test_run_cmd_preserves_argv(capfd, subprocess, shell, args): + """Arguments arrive as the words they were, whatever is in them + + Through python rather than echo, because it is the argv the command + receives that is under test, and every platform running this suite has + an interpreter that can report it back. + """ + if not shell.carries(args): + pytest.skip(f"{shell.name} parses these itself before the command sees them") + + show = "import sys, json; print(json.dumps(sys.argv[1:]))" + with pytest.raises(SystemExit) as exc_info: + run_cmd([sys.executable, "-c", show, *args]) + assert exc_info.value.args[0] == 0 + assert json.loads(capfd.readouterr().out) == args + + @pytest.fixture def called_from_wsl(monkeypatch): """Pretend the windows install was called from a distribution shell"""