Skip to content

fix(cli): disable click Windows argv expansion so secret values are stored byte-for-byte (KSM-1186) - #1102

Merged
stas-schaller merged 2 commits into
release/tool/cli/v1.5.0from
fix/KSM-1186-windows-expand-args
Aug 12, 2026
Merged

fix(cli): disable click Windows argv expansion so secret values are stored byte-for-byte (KSM-1186)#1102
stas-schaller merged 2 commits into
release/tool/cli/v1.5.0from
fix/KSM-1186-windows-expand-args

Conversation

@mgallego-keeper

@mgallego-keeper mgallego-keeper commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Bug

KSM-1186 — on Windows, click ≥ 8.0 expands every command-line argument before parsing (BaseCommand.main() defaults windows_expand_args=True_expand_args(): os.path.expanduser, then os.path.expandvars, then glob). ntpath.expandvars expands %VAR%/$VAR and collapses $$$, so secret values passed as arguments were silently corrupted before storage:

typed stored (pre-fix)
password=a$$b a$b
login=x%OS%y xWindows_NTy

Windows-only (posix never collapses $$; click gates the expansion on os.name == "nt"), affects every click argument (field values, titles, notes, sync --map, …) on both pip and frozen surfaces, and also pre-expands %VAR% inside ksm exec command arguments so the documented Windows exec pattern delivered raw notation instead of the resolved secret. Pre-existing — reproduced byte-identically on 1.4.0 — not a 1.5.0 regression.

Fix

  • __main__.py main(): pass windows_expand_args=False (the click 8.0.1+ opt-out) in the group invocation — secret values are not shell globs.
  • setup.py: bump click>=8.0click>=8.0.1 (the version that introduced the opt-out parameter).
  • tests/windows_expand_args_test.py (new): asserts main() passes the opt-out. click.testing.CliRunner bypasses BaseCommand.main() entirely — exactly how this bug evaded the unit suite — so the wiring is the honest unit-testable surface; byte fidelity end-to-end is covered by the release regression suite driving the packaged CLI in a real subprocess.

ksm shell inner commands dispatch through click-repl's make_context/invoke, never BaseCommand.main(), so the KSM-1162/1165 shell tokenizer paths are untouched.

Verification (Windows 11, live QA US vault, editable install of this branch)

  • a$$b, x%OS%y, glob *, ~admin, and undefined-var controls all stored byte-for-byte on secret add field and secret update (each corrupted pre-fix except the controls).
  • ksm exec -- cmd /c "echo RESOLVED=%MYPW%" now delivers the resolved secret to the child (pre-fix: raw notation).
  • New test fails on unpatched main() (verified via stash), passes patched.
  • Full CLI unit suite: 181 passed / 13 skipped / 1 failed — the failure is the pre-existing privilege-gated test_symlink_rejection; no regressions.
  • macOS/Linux behavior unchanged (expansion was Windows-gated; the kwarg is accepted everywhere).

Full QA test plan (13 test cases) and acceptance criteria are on KSM-1186 (comment of 2026-08-12). Note for release planning: the current 1.5.0 publish artifacts (run 31418361581) do not contain this fix; the frozen surface needs re-verification on whichever rebuilt installer ships it.

…tored byte-for-byte (KSM-1186)

click >= 8.0 expands every command-line argument on Windows before parsing
(BaseCommand.main -> _expand_args: os.path.expanduser, os.path.expandvars,
then glob). ntpath.expandvars expands %VAR%/$VAR and collapses $$ to $, so
'password=a$$b' was stored as 'a$b' and 'login=x%OS%y' as 'xWindows_NTy' -
silent data corruption of stored secrets, plus premature %VAR% expansion
inside 'ksm exec' command arguments. Windows-only; pip and frozen surfaces.

Pass windows_expand_args=False (the click 8.0.1+ opt-out) in main()'s group
invocation, bump the click floor accordingly, and add a wiring test
(CliRunner bypasses BaseCommand.main(), which is how this evaded the unit
suite - the test asserts the opt-out is passed).
@mgallego-keeper
mgallego-keeper force-pushed the fix/KSM-1186-windows-expand-args branch from caa77ba to 4d8c192 Compare August 12, 2026 17:11
@mgallego-keeper mgallego-keeper self-assigned this Aug 12, 2026

@stas-schaller stas-schaller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the fix against the click 8.0.1 source. windows_expand_args is the correct opt-out, and click gates it to os.name == "nt". The new test is a solid wiring test. Good stuff!

One thing: setup.py bumps click>=8.0 to click>=8.0.1. The sibling file integration/keeper_secrets_manager_cli/requirements.txt still pins click>=8.0. I installed click 8.0.0 and called cli(obj={}, prog_name='ksm', windows_expand_args=False). This raised TypeError: Context.__init__() got an unexpected keyword argument 'windows_expand_args'. Please bump requirements.txt to click>=8.0.1 too.

@mgallego-keeper

Copy link
Copy Markdown
Contributor Author

I checked the fix against the click 8.0.1 source. windows_expand_args is the correct opt-out, and click gates it to os.name == "nt". The new test is a solid wiring test. Good stuff!

One thing: setup.py bumps click>=8.0 to click>=8.0.1. The sibling file integration/keeper_secrets_manager_cli/requirements.txt still pins click>=8.0. I installed click 8.0.0 and called cli(obj={}, prog_name='ksm', windows_expand_args=False). This raised TypeError: Context.__init__() got an unexpected keyword argument 'windows_expand_args'. Please bump requirements.txt to click>=8.0.1 too.

Great catch! Will do, thank you!

…(KSM-1186)

Review follow-up: requirements.txt still allowed click 8.0.0, which lacks
the windows_expand_args parameter and raises TypeError when main() passes
it. Both dependency declarations now require 8.0.1+.
@mgallego-keeper

Copy link
Copy Markdown
Contributor Author

Addressed: requirements.txt bumped to click>=8.0.1 to match setup.py — both dependency declarations now exclude click 8.0.0 (which lacks the windows_expand_args parameter and raises the TypeError you reproduced). Thanks for catching the sibling file.

@stas-schaller
stas-schaller merged commit b3a71e7 into release/tool/cli/v1.5.0 Aug 12, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants