Skip to content

fix: keychain stores -U flag as password instead of actual secret #21

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

Merged
kaushalpaneri merged 1 commit into from
Mar 23, 2026
Merged

fix: keychain stores -U flag as password instead of actual secret #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/crux_cli/secrets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def _service(mcp_name: str) -> str:
def set(self, mcp_name: str, key: str, value: str) -> None: # noqa: A003
"""Store a secret in macOS Keychain via ``security add-generic-password``."""
service = self._service(mcp_name)
cmd = ["security", "add-generic-password", "-s", service, "-a", key, "-w", "-U"] # noqa: S607
cmd = ["security", "add-generic-password", "-U", "-s", service, "-a", key, "-w"] # noqa: S607
result = subprocess.run(cmd, input=value.encode(), capture_output=True) # noqa: S603
if result.returncode != 0:
msg = result.stderr.decode().strip() if result.stderr else "unknown error"
Expand Down
91 changes: 51 additions & 40 deletions tests/unit/test_secrets_backends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
# Helpers
# ---------------------------------------------------------------------------


@pytest.fixture
def secrets_home(monkeypatch, tmp_path):
"""Point crux_home() and secrets_path() at a temp directory."""
Expand All @@ -38,6 +39,7 @@ def secrets_home(monkeypatch, tmp_path):
# W1B.1 Protocol
# ===========================================================================


class TestBackendProtocol:
"""Verify that the SecretsBackend protocol has the right shape."""

Expand All @@ -62,6 +64,7 @@ def test_protocol_methods_exist(self):
# W1B.1 get_backend factory
# ===========================================================================


class TestGetBackend:
def test_get_backend_keychain(self):
cfg = {"secrets": {"backend": "keychain"}}
Expand Down Expand Up @@ -98,6 +101,7 @@ def test_get_backend_default_from_config(self, monkeypatch):
# W1B.2 macOS Keychain backend (mocked)
# ===========================================================================


class TestKeychainSet:
def test_keychain_set(self, secrets_home, mocker):
mock_run = mocker.patch("crux_cli.secrets.subprocess.run")
Expand All @@ -112,40 +116,52 @@ def test_keychain_set(self, secrets_home, mocker):
assert "myvalue" not in args
assert mock_run.call_args[1].get("input") == b"myvalue"

def test_keychain_set_w_flag_is_last(self, secrets_home, mocker):
"""Ensure -w is the last flag so security reads the password from stdin.

Regression test for #18: when -U followed -w, macOS security
interpreted -U as the password value instead of reading stdin.
"""
mock_run = mocker.patch("crux_cli.secrets.subprocess.run")
mock_run.return_value = MagicMock(returncode=0, stderr=b"")
backend = MacOSKeychainBackend()
backend.set("mymcp", "MY_KEY", "myvalue")
args = mock_run.call_args[0][0]
assert args[-1] == "-w", f"-w must be last arg so stdin is read; got {args}"
# -U must come before -w, not after
u_idx = args.index("-U")
w_idx = args.index("-w")
assert u_idx < w_idx, f"-U (idx {u_idx}) must precede -w (idx {w_idx})"

def test_keychain_set_updates_index(self, secrets_home, mocker):
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=0, stderr=b""))
mocker.patch("crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=0, stderr=b""))
backend = MacOSKeychainBackend()
backend.set("mymcp", "MY_KEY", "val")
idx = load_secrets_index()
assert "MY_KEY" in idx["mymcp"]

def test_keychain_set_exits_on_failure(self, secrets_home, mocker):
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=1, stderr=b"error"))
mocker.patch("crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=1, stderr=b"error"))
backend = MacOSKeychainBackend()
with pytest.raises(SystemExit):
backend.set("mymcp", "MY_KEY", "val")


class TestKeychainGet:
def test_keychain_get(self, mocker):
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=0, stdout="secret\n"))
mocker.patch("crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=0, stdout="secret\n"))
backend = MacOSKeychainBackend()
assert backend.get("mymcp", "MY_KEY") == "secret"

def test_keychain_not_found(self, mocker):
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=44, stdout=""))
mocker.patch("crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=44, stdout=""))
backend = MacOSKeychainBackend()
assert backend.get("mymcp", "MY_KEY") is None


class TestKeychainDelete:
def test_keychain_delete(self, secrets_home, mocker):
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=0))
mocker.patch("crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=0))
save_secrets_index({"mymcp": ["K1", "K2"]})
backend = MacOSKeychainBackend()
backend.delete("mymcp", "K1")
Expand Down Expand Up @@ -177,12 +193,11 @@ def test_keychain_list_keys_missing_mcp(self, secrets_home):
# W1B.3 Linux Secret Service backend (mocked)
# ===========================================================================


class TestSecretServiceSet:
def test_secret_service_set(self, secrets_home, mocker):
mock_collection = MagicMock()
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection)
backend = LinuxSecretServiceBackend()
backend.set("mymcp", "MY_KEY", "myvalue")
mock_collection.create_item.assert_called_once_with(
Expand All @@ -201,18 +216,14 @@ def test_secret_service_get(self, mocker):
mock_item.get_secret.return_value = b"secret_value"
mock_collection = MagicMock()
mock_collection.search_items.return_value = [mock_item]
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection)
backend = LinuxSecretServiceBackend()
assert backend.get("mymcp", "KEY") == "secret_value"

def test_secret_service_get_not_found(self, mocker):
mock_collection = MagicMock()
mock_collection.search_items.return_value = []
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection)
backend = LinuxSecretServiceBackend()
assert backend.get("mymcp", "KEY") is None

Expand All @@ -222,9 +233,7 @@ def test_secret_service_delete(self, secrets_home, mocker):
mock_item = MagicMock()
mock_collection = MagicMock()
mock_collection.search_items.return_value = [mock_item]
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=mock_collection)
save_secrets_index({"mymcp": ["KEY"]})
backend = LinuxSecretServiceBackend()
backend.delete("mymcp", "KEY")
Expand All @@ -236,26 +245,20 @@ def test_secret_service_delete(self, secrets_home, mocker):
class TestSecretServiceDBusFallback:
def test_secret_service_dbus_unavailable_falls_back(self, secrets_home, mocker):
"""When D-Bus is unavailable, falls back to AgeEncryptedBackend."""
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=None
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=None)
mock_age_set = mocker.patch.object(AgeEncryptedBackend, "set")
backend = LinuxSecretServiceBackend()
backend.set("mymcp", "KEY", "val")
mock_age_set.assert_called_once_with("mymcp", "KEY", "val")

def test_secret_service_get_falls_back(self, secrets_home, mocker):
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=None
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=None)
mocker.patch.object(AgeEncryptedBackend, "get", return_value="fallback_val")
backend = LinuxSecretServiceBackend()
assert backend.get("mymcp", "KEY") == "fallback_val"

def test_secret_service_delete_falls_back(self, secrets_home, mocker):
mocker.patch.object(
LinuxSecretServiceBackend, "_get_collection", return_value=None
)
mocker.patch.object(LinuxSecretServiceBackend, "_get_collection", return_value=None)
mock_age_delete = mocker.patch.object(AgeEncryptedBackend, "delete")
backend = LinuxSecretServiceBackend()
backend.delete("mymcp", "KEY")
Expand All @@ -266,17 +269,21 @@ def test_secret_service_delete_falls_back(self, secrets_home, mocker):
# W1B.4 Age encrypted file backend (mocked)
# ===========================================================================


class TestAgeSet:
def test_age_set(self, secrets_home, mocker):
identity = secrets_home / "identity"
identity.write_text("# public key: age1abc\nAGE-SECRET-KEY-1XYZ\n")
identity.chmod(0o600)

mocker.patch("crux_cli.secrets.subprocess.run", side_effect=[
# _load_store (decrypt) — no existing file, won't be called
# _save_store (encrypt)
MagicMock(returncode=0, stdout="", stderr=""),
])
mocker.patch(
"crux_cli.secrets.subprocess.run",
side_effect=[
# _load_store (decrypt) — no existing file, won't be called
# _save_store (encrypt)
MagicMock(returncode=0, stdout="", stderr=""),
],
)
# No existing secrets.age file
backend = AgeEncryptedBackend()
backend.set("mymcp", "KEY", "value123")
Expand All @@ -292,8 +299,9 @@ def test_age_get(self, secrets_home, mocker):
(secrets_home / "secrets.age").write_text("encrypted")

store = {"mymcp": {"KEY": "secret_value"}}
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=0, stdout=json.dumps(store), stderr=""))
mocker.patch(
"crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=0, stdout=json.dumps(store), stderr="")
)
backend = AgeEncryptedBackend()
assert backend.get("mymcp", "KEY") == "secret_value"

Expand All @@ -308,8 +316,9 @@ def test_age_get_decrypt_failure_raises(self, secrets_home, mocker):
identity.write_text("# public key: age1abc\nAGE-SECRET-KEY-1XYZ\n")
identity.chmod(0o600)
(secrets_home / "secrets.age").write_text("corrupted")
mocker.patch("crux_cli.secrets.subprocess.run",
return_value=MagicMock(returncode=1, stdout="", stderr="bad key"))
mocker.patch(
"crux_cli.secrets.subprocess.run", return_value=MagicMock(returncode=1, stdout="", stderr="bad key")
)
backend = AgeEncryptedBackend()
with pytest.raises(RuntimeError, match="Failed to decrypt"):
backend.get("mymcp", "KEY")
Expand Down Expand Up @@ -379,6 +388,7 @@ def test_age_identity_permissions(self, secrets_home, mocker):
# W1B.6 Secrets index
# ===========================================================================


class TestSecretsIndexFormat:
def test_secrets_index_format(self, secrets_home):
data = {"wikijs-mcp": ["WIKIJS_URL", "WIKIJS_API_KEY"]}
Expand Down Expand Up @@ -452,6 +462,7 @@ def test_secrets_index_atomic_write_creates_parent(self, monkeypatch, tmp_path):
# W1B.5 CLI integration (mocked backend)
# ===========================================================================


class TestCLISecretSet:
def test_cli_secret_set(self, secrets_home, mocker):
"""Verify the backend set method is callable with expected args."""
Expand Down
Loading