diff --git a/comfy/cli_args.py b/comfy/cli_args.py index 659e772edd2..a63da32c05b 100644 --- a/comfy/cli_args.py +++ b/comfy/cli_args.py @@ -313,3 +313,67 @@ def enables_dynamic_vram(): if args.enable_dynamic_vram: return True return not args.disable_dynamic_vram and not args.highvram and not args.gpu_only and not args.novram and not args.cpu + + +# CLI flags whose value is a local filesystem path (or a connection string that +# embeds one, like --database-url's sqlite:///... URI). /system_stats echoes +# sys.argv back to any unauthenticated client, so these values are redacted by +# redact_sensitive_argv() below before that happens. Flags take exactly one +# value unless listed in SENSITIVE_ARGV_MULTI_FLAGS. +SENSITIVE_ARGV_FLAGS = { + "--tls-keyfile", + "--tls-certfile", + "--base-directory", + "--output-directory", + "--temp-directory", + "--input-directory", + "--front-end-root", + "--user-directory", + "--models-directory", + "--database-url", +} + +# Flags that accept one or more values after them (argparse nargs='+'), so every +# value up to the next "-"-prefixed token must be redacted, not just the first. +SENSITIVE_ARGV_MULTI_FLAGS = { + "--extra-model-paths-config", +} + + +def redact_sensitive_argv(argv): + """Return a copy of argv with the values of path-bearing flags replaced with "*". + + Flag names (and every other argument) are left untouched so callers such as + the frontend's system-stats panel and "Copy System Info" feature can still + show which flags were passed, without leaking local directory layout, + usernames, or other filesystem details contained in their values. + """ + redacted = [] + i = 0 + n = len(argv) + while i < n: + tok = argv[i] + name = tok.partition("=")[0] + if name in SENSITIVE_ARGV_FLAGS: + if "=" in tok: + redacted.append(f"{name}=*") + else: + redacted.append(tok) + if i + 1 < n and not argv[i + 1].startswith("-"): + redacted.append("*") + i += 1 + elif name in SENSITIVE_ARGV_MULTI_FLAGS: + if "=" in tok: + redacted.append(f"{name}=*") + else: + redacted.append(tok) + had_value = False + while i + 1 < n and not argv[i + 1].startswith("-"): + had_value = True + i += 1 + if had_value: + redacted.append("*") + else: + redacted.append(tok) + i += 1 + return redacted diff --git a/server.py b/server.py index c9ffcaa0da9..80a46e8d4f4 100644 --- a/server.py +++ b/server.py @@ -34,7 +34,7 @@ import logging import mimetypes -from comfy.cli_args import args +from comfy.cli_args import args, redact_sensitive_argv from comfy.deploy_environment import get_deploy_environment import comfy.utils import comfy.model_management @@ -730,7 +730,7 @@ async def system_stats(request): "pytorch_version": comfy.model_management.torch_version, "embedded_python": os.path.split(os.path.split(sys.executable)[0])[1] == "python_embeded", "deploy_environment": get_deploy_environment(), - "argv": sys.argv + "argv": redact_sensitive_argv(sys.argv) }, "devices": device_entries } diff --git a/tests-unit/server_test/test_system_stats_argv_redaction.py b/tests-unit/server_test/test_system_stats_argv_redaction.py new file mode 100644 index 00000000000..37b87a5b864 --- /dev/null +++ b/tests-unit/server_test/test_system_stats_argv_redaction.py @@ -0,0 +1,148 @@ +"""CI unit guard for redacting sys.argv in the /system_stats endpoint. + +The /system_stats endpoint (server.py, PromptServer.add_routes -> system_stats) +returns "argv": sys.argv verbatim in its JSON response. That endpoint has no +authentication, so any client that can reach it -- including a page open in a +browser tab, per ComfyUI's threat model for local-network deployments -- could +read back the full raw command line, including values passed to path-bearing +flags such as --extra-model-paths-config, --output-directory, --database-url, +etc. Those values commonly contain usernames, internal directory layouts, or +otherwise-private filesystem paths that have nothing to do with diagnostics. + +The fix is comfy.cli_args.redact_sensitive_argv(), which replaces only the +*values* that follow a known path-bearing flag with "*", leaving every flag +name (and every non-path argument) untouched. server.py calls this helper +instead of exposing sys.argv directly. + +server.py cannot be imported in a unit test (importing it pulls in nodes/torch +and spins up the full PromptServer/aiohttp app), so -- following the existing +tests-unit/security_test/test_ghsa_779p_05_dangerous_content_types.py pattern +-- this file tests the redaction helper directly rather than the route. + +Preserving the flag names (not just truncating argv to [sys.argv[0]]) matters +because the frontend's system-stats panel and "Copy System Info" support +feature (ComfyUI_frontend's useCopySystemInfo.ts / +systemStatsColumns.ts) both render the full `argv` array for legitimate +debugging/bug-report purposes; blanking it out entirely would regress that. +""" + +from comfy.cli_args import ( + SENSITIVE_ARGV_FLAGS, + SENSITIVE_ARGV_MULTI_FLAGS, + redact_sensitive_argv, +) + + +def test_no_sensitive_flags_is_unchanged(): + argv = ["main.py", "--cpu", "--listen", "0.0.0.0", "--port", "8188"] + assert redact_sensitive_argv(argv) == argv + + +def test_single_value_flag_value_is_redacted(): + """The exact repro from the issue report.""" + argv = [ + "main.py", + "--extra-model-paths-config", + r"D:\Private\models.yaml", + "--output-directory", + r"D:\Confidential\Renders", + ] + redacted = redact_sensitive_argv(argv) + assert redacted == [ + "main.py", + "--extra-model-paths-config", + "*", + "--output-directory", + "*", + ] + # None of the original path fragments survive. + joined = " ".join(redacted) + assert "Private" not in joined + assert "Confidential" not in joined + + +def test_flag_names_are_preserved_for_frontend_display(): + """Flag names must stay intact -- the frontend's system-stats panel and + "Copy System Info" feature display which flags were passed.""" + argv = ["main.py", "--base-directory", "/home/alice/comfy"] + redacted = redact_sensitive_argv(argv) + assert redacted[0] == "main.py" + assert "--base-directory" in redacted + assert "alice" not in " ".join(redacted) + + +def test_equals_syntax_is_redacted(): + argv = ["main.py", "--database-url=sqlite:////home/alice/user/comfyui.db"] + redacted = redact_sensitive_argv(argv) + assert redacted == ["main.py", "--database-url=*"] + + +def test_multi_value_flag_redacts_every_value_up_to_next_flag(): + """--extra-model-paths-config uses argparse nargs='+', so it can take + several path values before the next flag.""" + argv = [ + "main.py", + "--extra-model-paths-config", + "/a/extra1.yaml", + "/a/extra2.yaml", + "--cpu", + ] + redacted = redact_sensitive_argv(argv) + assert redacted == ["main.py", "--extra-model-paths-config", "*", "--cpu"] + + +def test_flag_with_missing_value_is_not_corrupted(): + """A malformed/truncated argv (flag as the last token) must not raise or + swallow adjacent tokens -- just leave it as-is.""" + argv = ["main.py", "--tls-keyfile"] + assert redact_sensitive_argv(argv) == ["main.py", "--tls-keyfile"] + + +def test_flag_with_missing_value_does_not_swallow_the_next_flag(): + """If a sensitive single-value flag has no value because another flag + immediately follows it, that following flag must survive untouched -- + not be consumed and replaced with "*".""" + argv = ["main.py", "--tls-keyfile", "--cpu"] + assert redact_sensitive_argv(argv) == ["main.py", "--tls-keyfile", "--cpu"] + + +def test_multi_value_flag_equals_syntax_is_redacted(): + """--extra-model-paths-config=/path.yaml (argparse also accepts "=" for + nargs='+' flags with a single value) must be redacted like the + single-value flags' equals syntax already is.""" + argv = ["main.py", "--extra-model-paths-config=/home/alice/extra.yaml"] + redacted = redact_sensitive_argv(argv) + assert redacted == ["main.py", "--extra-model-paths-config=*"] + assert "alice" not in " ".join(redacted) + + +def test_non_path_arguments_are_left_alone(): + argv = ["main.py", "--multi-user", "--fast", "--preview-method", "auto"] + assert redact_sensitive_argv(argv) == argv + + +def test_empty_argv_is_handled(): + assert redact_sensitive_argv([]) == [] + + +def test_original_argv_is_not_mutated(): + argv = ["main.py", "--output-directory", "/secret/out"] + original = list(argv) + redact_sensitive_argv(argv) + assert argv == original + + +def test_all_documented_sensitive_flags_are_redacted(): + """Every flag in SENSITIVE_ARGV_FLAGS/SENSITIVE_ARGV_MULTI_FLAGS must + actually get its value replaced -- guards against the set growing without + the redaction logic being exercised for it.""" + for flag in SENSITIVE_ARGV_FLAGS: + argv = ["main.py", flag, "/some/private/path"] + redacted = redact_sensitive_argv(argv) + assert redacted == ["main.py", flag, "*"], flag + assert "private" not in " ".join(redacted) + + for flag in SENSITIVE_ARGV_MULTI_FLAGS: + argv = ["main.py", flag, "/some/private/path.yaml"] + redacted = redact_sensitive_argv(argv) + assert redacted == ["main.py", flag, "*"], flag