From 62c2401418705936786fef443eef611a518cfacc Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 16:54:08 -0700 Subject: [PATCH 1/2] test(auth): regression-guard no unredacted secret in login/whoami/set-key/auth-set output (BE-3363) --- tests/comfy_cli/test_secret_redaction.py | 221 +++++++++++++++++++++++ 1 file changed, 221 insertions(+) create mode 100644 tests/comfy_cli/test_secret_redaction.py diff --git a/tests/comfy_cli/test_secret_redaction.py b/tests/comfy_cli/test_secret_redaction.py new file mode 100644 index 00000000..0cc2a089 --- /dev/null +++ b/tests/comfy_cli/test_secret_redaction.py @@ -0,0 +1,221 @@ +"""Regression guard: no command may echo an *unredacted* secret (BE-3363). + +A 2026-07-16 dogfooding transcript reported ``comfy cloud login`` printing an +API key in plaintext. On current ``main`` that is no longer reproducible — +every echo path funnels its secret through :func:`store._redact` (``auth set`` +and ``cloud set-key`` print ``record.to_dict()['key']``; ``cloud login`` / +``cloud whoami`` emit ``session.to_dict(redact=True)``). But that redaction is +only ever a *default* argument at each call site — nothing keeps a future +command (or a refactor that flips a ``redact=`` default) from leaking the raw +value again. + +This module is that keep-it-that-way guard. For every secret-bearing command, +in BOTH pretty and ``--json`` output modes, it feeds a long sentinel secret and +asserts the FULL sentinel never appears in captured stdout **or** stderr. The +redacted ``sk-S…cdef`` form is expected and fine. + +Adding coverage for a NEW secret-bearing command is a single ``_Case`` entry in +``CASES`` below (or, if the command needs a mocked network handshake like +``cloud login``, a sibling of ``test_login_success_never_echoes_secret``). +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from collections.abc import Callable +from dataclasses import dataclass, field +from unittest.mock import patch + +import pytest + +from comfy_cli.auth import store + +# Sentinels are all > 16 chars so they take the ``sk-S…cdef`` redaction branch, +# not the ``<=16 → ***`` one (`store._redact`). Distinct values per secret slot +# so a leak of *any* of them is caught individually. +SENTINEL_KEY = "sk-SENTINEL-KEY-0123456789abcdef" +SENTINEL_ACCESS = "sk-SENTINEL-ACCESS-0123456789abcdef" +SENTINEL_REFRESH = "sk-SENTINEL-REFRESH-0123456789abcdef" + +# A far-future expiry keeps ``cloud whoami`` from spending the (fake) refresh +# token — no network, the seeded access token is reported as-is (redacted). +_FAR_FUTURE_EPOCH = 4102444800 # 2100-01-01T00:00:00Z + + +def _assert_redacts(text: str) -> None: + assert len(text) > 16, "sentinel must exceed the `<=16 → ***` redaction cap" + + +for _s in (SENTINEL_KEY, SENTINEL_ACCESS, SENTINEL_REFRESH): + _assert_redacts(_s) + + +@dataclass(frozen=True) +class _Case: + """One secret-bearing invocation to guard. + + ``args`` are the CLI args *after* the global output-mode flag. ``secrets`` + lists every full sentinel that must never appear in the output. ``seed`` + (optional) plants on-disk state (e.g. a stored session) before the command + runs; it is called with ``COMFY_SECRETS_PATH`` already pointing at the temp + store. + """ + + id: str + args: tuple[str, ...] + secrets: tuple[str, ...] + seed: Callable[[], object] | None = field(default=None) + + +def _seed_cloud_session() -> None: + store.save_cloud_session( + base_url="https://testcloud.comfy.org", + resource="https://testcloud.comfy.org/mcp", + client_id="mcp-dyn-fake-id", + scope="mcp:tools:read mcp:tools:call", + access_token=SENTINEL_ACCESS, + refresh_token=SENTINEL_REFRESH, + token_type="Bearer", + expires_at=_FAR_FUTURE_EPOCH, + ) + + +CASES: tuple[_Case, ...] = ( + _Case( + id="auth-set-civitai", + args=("auth", "set", "civitai", "--key", SENTINEL_KEY), + secrets=(SENTINEL_KEY,), + ), + _Case( + id="cloud-set-key", + args=("cloud", "set-key", "--key", SENTINEL_KEY), + secrets=(SENTINEL_KEY,), + ), + _Case( + id="cloud-whoami", + args=("cloud", "whoami"), + secrets=(SENTINEL_ACCESS, SENTINEL_REFRESH), + seed=_seed_cloud_session, + ), +) + +# `--json` → JSON envelope mode; `--no-json` forces pretty even under a +# non-tty test stdout (see comfy_cli/output/renderer.py mode resolution). +MODES = ("--json", "--no-json") + + +@pytest.fixture +def cli_env(tmp_path, monkeypatch): + env = os.environ.copy() + env["NO_COLOR"] = "1" + env["DO_NOT_TRACK"] = "1" # never touch telemetry from a test + env["COMFY_SECRETS_PATH"] = str(tmp_path / "secrets.json") + # Mirror the secrets path into this process too, so a case's `seed` + # (which calls store.save_cloud_session) writes the same file the + # subprocess reads. + monkeypatch.setenv("COMFY_SECRETS_PATH", env["COMFY_SECRETS_PATH"]) + monkeypatch.setenv("DO_NOT_TRACK", "1") + return env + + +def _run(args, env): + return subprocess.run( + [sys.executable, "-m", "comfy_cli", *args], + capture_output=True, + text=True, + env=env, + check=False, + ) + + +@pytest.mark.parametrize("case", CASES, ids=[c.id for c in CASES]) +@pytest.mark.parametrize("mode", MODES) +def test_command_never_echoes_unredacted_secret(case: _Case, mode: str, cli_env): + if case.seed is not None: + case.seed() + + res = _run([mode, *case.args], cli_env) + + # A silent early exit (nonzero, empty output) would pass the "secret + # absent" check for the wrong reason — require the command to succeed so + # the redaction path is genuinely exercised. + assert res.returncode == 0, f"{case.id}/{mode} failed:\nstdout={res.stdout!r}\nstderr={res.stderr!r}" + + combined = res.stdout + "\n" + res.stderr + for secret in case.secrets: + assert secret not in combined, f"{case.id}/{mode} leaked an unredacted secret in output:\n{combined!r}" + + +def test_auth_set_json_carries_only_the_redacted_key(cli_env): + """Positive control: prove the guard actually observes the redaction, not + an empty/errored output that would trivially satisfy 'secret absent'.""" + res = _run(["--json", "auth", "set", "civitai", "--key", SENTINEL_KEY], cli_env) + assert res.returncode == 0, res.stderr + last = [line for line in res.stdout.splitlines() if line.strip()][-1] + payload = json.loads(last) + providers = {p["provider"]: p for p in payload["data"]["providers"]} + assert providers["civitai"]["key"] == store._redact(SENTINEL_KEY) + assert providers["civitai"]["key_redacted"] is True + assert SENTINEL_KEY not in json.dumps(payload) + + +# --------------------------------------------------------------------------- +# cloud login — needs the OAuth handshake mocked, so it runs in-process via +# Typer's CliRunner (a subprocess can't patch `run_login`). The success path +# emits `session.to_dict(redact=True)`, same as whoami, but we cover it +# explicitly per the ticket. +# --------------------------------------------------------------------------- + + +def _fake_login_result(): + from comfy_cli.cloud.oauth import LoginResult, TokenSet + + tokens = TokenSet( + access_token=SENTINEL_ACCESS, + refresh_token=SENTINEL_REFRESH, + token_type="Bearer", + expires_in=None, + expires_at=_FAR_FUTURE_EPOCH, + scope="mcp:tools:read mcp:tools:call", + ) + return LoginResult( + tokens=tokens, + client_id="mcp-dyn-fake-id", + base_url="https://testcloud.comfy.org", + resource="https://testcloud.comfy.org/mcp", + scope="mcp:tools:read mcp:tools:call", + redirect_uri="http://127.0.0.1:0/callback", + ) + + +@pytest.mark.parametrize("mode", MODES) +def test_login_success_never_echoes_secret(mode: str, tmp_path, monkeypatch): + from typer.testing import CliRunner + + from comfy_cli.cmdline import app + + secrets_path = tmp_path / "secrets.json" + invoke_env = { + "NO_COLOR": "1", + "DO_NOT_TRACK": "1", + "COMFY_SECRETS_PATH": str(secrets_path), + } + + runner = CliRunner() # click 8.1: mix_stderr=True → result.output covers both streams + with patch("comfy_cli.cloud.command.run_login", return_value=_fake_login_result()): + result = runner.invoke(app, [mode, "cloud", "login", "--no-browser"], env=invoke_env) + + assert result.exit_code == 0, f"login/{mode} failed: output={result.output!r} exc={result.exception!r}" + for secret in (SENTINEL_ACCESS, SENTINEL_REFRESH): + assert secret not in result.output, f"login/{mode} leaked a secret:\n{result.output!r}" + + # Positive control: a real session was persisted, so the "secret absent + # from output" assertion above passed because redaction fired — not because + # the command no-op'd and emitted nothing. The on-disk store intentionally + # keeps the RAW token (only command *output* is redacted), so finding the + # sentinel in the file is expected and is what proves the write happened. + assert secrets_path.exists() + assert SENTINEL_ACCESS in secrets_path.read_text() From ad5ffe3985442a6ce7ae317f20daac539cb1e547 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 17:11:46 -0700 Subject: [PATCH 2/2] test(auth): assert login secret-absence against stdout+stderr, not just output (BE-3363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The subprocess-based cases fold stdout+stderr before checking for a leaked sentinel, but the in-process `cloud login` test asserted only against `result.output`. Under Click 8.1 that is fine (mix_stderr=True merges the streams), but Click >= 8.2 dropped mix_stderr and captures stderr separately, making result.output stdout-only — a secret leaked to stderr would slip past. Fold stderr back in when it is a separate stream, honoring the module's stated 'stdout OR stderr' invariant across Click versions. Co-Authored-By: Claude Opus 4.8 --- tests/comfy_cli/test_secret_redaction.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/comfy_cli/test_secret_redaction.py b/tests/comfy_cli/test_secret_redaction.py index 0cc2a089..241c3a3f 100644 --- a/tests/comfy_cli/test_secret_redaction.py +++ b/tests/comfy_cli/test_secret_redaction.py @@ -204,13 +204,23 @@ def test_login_success_never_echoes_secret(mode: str, tmp_path, monkeypatch): "COMFY_SECRETS_PATH": str(secrets_path), } - runner = CliRunner() # click 8.1: mix_stderr=True → result.output covers both streams + runner = CliRunner() with patch("comfy_cli.cloud.command.run_login", return_value=_fake_login_result()): result = runner.invoke(app, [mode, "cloud", "login", "--no-browser"], env=invoke_env) assert result.exit_code == 0, f"login/{mode} failed: output={result.output!r} exc={result.exception!r}" + # Honor the module's 'stdout OR stderr' invariant like the subprocess cases. + # Click 8.1's default mix_stderr=True folds stderr into result.output (and + # result.stderr raises); Click >= 8.2 dropped mix_stderr and captures stderr + # separately, so result.output is stdout-only — fold stderr back in when it + # is its own stream, otherwise a secret leaked to stderr would slip past. + combined = result.output + try: + combined += result.stderr + except ValueError: + pass # click 8.1 mix_stderr=True: stderr already merged into result.output for secret in (SENTINEL_ACCESS, SENTINEL_REFRESH): - assert secret not in result.output, f"login/{mode} leaked a secret:\n{result.output!r}" + assert secret not in combined, f"login/{mode} leaked a secret:\n{combined!r}" # Positive control: a real session was persisted, so the "secret absent # from output" assertion above passed because redaction fired — not because