From c971b8e8e0060431deb59f59c674e3f8561d3382 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 16 Jul 2026 22:43:42 -0700 Subject: [PATCH 1/2] fix: type registry API failures and surface machine-readable error codes (BE-3271) Replace the 5 bare `raise Exception(...)` in comfy_cli/registry/api.py with a typed RegistryAPIError carrying the HTTP status/body, and map it at the publish/install command boundary to renderer.error(code=..., details={status, body}) so node publish/install failures surface a machine-readable code instead of a raw traceback or generic message. Messages are kept equivalent. --- comfy_cli/command/custom_nodes/command.py | 17 ++++++++ comfy_cli/error_codes.py | 15 +++++++ comfy_cli/registry/__init__.py | 3 +- comfy_cli/registry/api.py | 39 ++++++++++++++++--- .../command/nodes/test_node_install.py | 27 +++++++++++++ tests/comfy_cli/command/nodes/test_publish.py | 33 ++++++++++++++++ tests/comfy_cli/registry/test_api.py | 23 +++++++++-- 7 files changed, 147 insertions(+), 10 deletions(-) diff --git a/comfy_cli/command/custom_nodes/command.py b/comfy_cli/command/custom_nodes/command.py index 7f4b1c80..1ab72422 100644 --- a/comfy_cli/command/custom_nodes/command.py +++ b/comfy_cli/command/custom_nodes/command.py @@ -23,9 +23,11 @@ upload_file_to_signed_url, zip_files, ) +from comfy_cli.output import get_renderer from comfy_cli.output import rprint as print # context-aware: stderr in JSON mode from comfy_cli.registry import ( RegistryAPI, + RegistryAPIError, extract_node_configuration, initialize_project_config, ) @@ -1119,6 +1121,13 @@ def publish( # Upload the zip file to the signed URL typer.echo("Uploading zip file...") upload_file_to_signed_url(signed_url, zip_filename) + except RegistryAPIError as e: + get_renderer().error( + code="node_publish_failed", + message=str(e), + details={"status": e.status, "body": e.body}, + ) + raise typer.Exit(code=1) from e except Exception as e: ui.display_error_message({str(e)}) raise typer.Exit(code=1) @@ -1215,6 +1224,14 @@ def registry_install( ui.display_error_message(f"Failed to download the custom node {node_id}.") return + except RegistryAPIError as e: + logging.error(f"Encountered an error while installing the node. error: {str(e)}") + get_renderer().error( + code="node_install_failed", + message=f"Failed to download the custom node {node_id}.", + details={"node_id": node_id, "status": e.status, "body": e.body}, + ) + raise typer.Exit(code=1) from e except Exception as e: logging.error(f"Encountered an error while installing the node. error: {str(e)}") ui.display_error_message(f"Failed to download the custom node {node_id}.") diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 4cba21d3..cfe20ff1 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -577,6 +577,21 @@ class ErrorCode: "`generate --emit-workflow` could not build the partner-node workflow.", "check the model name and that all required inputs are provided", ), + # --- custom node registry ------------------------------------------------ + ErrorCode( + "node_publish_failed", + "Publishing a custom-node version to the registry failed: either a " + "client-side validation gap (missing publisher id / project name in " + "pyproject.toml) or a non-2xx from the registry. `details.status` and " + "`details.body` carry the response when it was an HTTP failure.", + "check `details.body`; ensure `[tool.comfy] publisher_id` and `[project] name` are set, and the token is valid", + ), + ErrorCode( + "node_install_failed", + "Fetching a custom node from the registry for install failed with a non-2xx. " + "`details.status` and `details.body` carry the response.", + "check the node id and version exist in the registry (`comfy node registry-list`); check `details.body`", + ), # --- feedback ------------------------------------------------------------ ErrorCode( "feedback_message_required", diff --git a/comfy_cli/registry/__init__.py b/comfy_cli/registry/__init__.py index 912438a9..398818f8 100644 --- a/comfy_cli/registry/__init__.py +++ b/comfy_cli/registry/__init__.py @@ -1,9 +1,10 @@ -from .api import RegistryAPI +from .api import RegistryAPI, RegistryAPIError from .config_parser import extract_node_configuration, initialize_project_config from .types import Node, NodeVersion, PublishNodeVersionResponse, PyProjectConfig __all__ = [ "RegistryAPI", + "RegistryAPIError", "extract_node_configuration", "PyProjectConfig", "PublishNodeVersionResponse", diff --git a/comfy_cli/registry/api.py b/comfy_cli/registry/api.py index 928b3606..7c6f9508 100644 --- a/comfy_cli/registry/api.py +++ b/comfy_cli/registry/api.py @@ -14,6 +14,23 @@ ) +class RegistryAPIError(Exception): + """Raised when a Registry API call fails. + + Carries the HTTP ``status`` and response ``body`` when the failure came + from a non-2xx response, so the command boundary can surface a + machine-readable ``renderer.error(code=..., details={status, body})`` + instead of a bare traceback. Client-side validation failures (missing + publisher id / project name) carry no status/body. + """ + + def __init__(self, message: str, *, status: int | None = None, body: str | None = None): + super().__init__(message) + self.message = message + self.status = status + self.body = body + + class RegistryAPI: def __init__(self): self.base_url = self.determine_base_url() @@ -44,10 +61,10 @@ def publish_node_version( """ # Local import to prevent circular dependency if not node_config.tool_comfy.publisher_id: - raise Exception("Publisher ID is required in pyproject.toml to publish a node version") + raise RegistryAPIError("Publisher ID is required in pyproject.toml to publish a node version") if not node_config.project.name: - raise Exception("Project name is required in pyproject.toml to publish a node version") + raise RegistryAPIError("Project name is required in pyproject.toml to publish a node version") license_json = serialize_license(node_config.project.license) request_body = { "personal_access_token": token, @@ -88,7 +105,11 @@ def publish_node_version( signedUrl=data["signedUrl"], ) else: - raise Exception(f"Failed to publish node version: {response.status_code} {response.text}") + raise RegistryAPIError( + f"Failed to publish node version: {response.status_code} {response.text}", + status=response.status_code, + body=response.text, + ) def list_all_nodes(self): """ @@ -103,7 +124,11 @@ def list_all_nodes(self): raw_nodes = response.json()["nodes"] return [map_node_to_node_class(node) for node in raw_nodes] else: - raise Exception(f"Failed to retrieve nodes: {response.status_code} - {response.text}") + raise RegistryAPIError( + f"Failed to retrieve nodes: {response.status_code} - {response.text}", + status=response.status_code, + body=response.text, + ) def install_node(self, node_id, version=None): """ @@ -127,7 +152,11 @@ def install_node(self, node_id, version=None): logging.debug(f"RegistryAPI install_node response: {response.json()}") return map_node_version(response.json()) else: - raise Exception(f"Failed to install node: {response.status_code} - {response.text}") + raise RegistryAPIError( + f"Failed to install node: {response.status_code} - {response.text}", + status=response.status_code, + body=response.text, + ) def map_node_version(api_node_version): diff --git a/tests/comfy_cli/command/nodes/test_node_install.py b/tests/comfy_cli/command/nodes/test_node_install.py index 93fdbe54..2acfb295 100644 --- a/tests/comfy_cli/command/nodes/test_node_install.py +++ b/tests/comfy_cli/command/nodes/test_node_install.py @@ -6,6 +6,7 @@ from comfy_cli.command.custom_nodes.command import app from comfy_cli.file_utils import DownloadException +from comfy_cli.registry import RegistryAPIError runner = CliRunner() @@ -383,3 +384,29 @@ def test_no_traceback_in_output(self, tmp_path): assert "Traceback" not in result.output assert "DownloadException" not in result.output + + +class TestRegistryInstallApiError: + """A RegistryAPIError from install_node must surface a machine-readable + renderer.error(code="node_install_failed", details={status, body}) and + exit non-zero — not a bare traceback and not a silent exit 0.""" + + def test_api_error_surfaced_with_code_and_exit_1(self, tmp_path): + with ( + patch("comfy_cli.command.custom_nodes.command.registry_api") as mock_api, + patch("comfy_cli.command.custom_nodes.command.workspace_manager") as mock_ws, + patch("comfy_cli.command.custom_nodes.command.get_renderer") as mock_get_renderer, + ): + mock_ws.workspace_path = str(tmp_path) + mock_api.install_node.side_effect = RegistryAPIError( + "Failed to install node: 404 - Not Found", status=404, body="Not Found" + ) + + result = runner.invoke(app, ["registry-install", "test-node"]) + + assert result.exit_code == 1 + assert "Traceback" not in result.output + mock_get_renderer.return_value.error.assert_called_once() + _, kwargs = mock_get_renderer.return_value.error.call_args + assert kwargs["code"] == "node_install_failed" + assert kwargs["details"] == {"node_id": "test-node", "status": 404, "body": "Not Found"} diff --git a/tests/comfy_cli/command/nodes/test_publish.py b/tests/comfy_cli/command/nodes/test_publish.py index 42905a0e..d5df7559 100644 --- a/tests/comfy_cli/command/nodes/test_publish.py +++ b/tests/comfy_cli/command/nodes/test_publish.py @@ -5,6 +5,7 @@ from typer.testing import CliRunner from comfy_cli.command.custom_nodes.command import app +from comfy_cli.registry import RegistryAPIError from comfy_cli.registry.types import ComfyConfig, ProjectConfig, PyProjectConfig runner = CliRunner() @@ -148,6 +149,38 @@ def test_publish_exits_on_upload_failure(): assert mock_upload.called +def test_publish_surfaces_registry_api_error_with_code(): + # A RegistryAPIError from the publish call must be surfaced as a + # machine-readable renderer.error(code=..., details={status, body}) and + # exit 1 — not a bare traceback. + mock_result = MagicMock() + mock_result.returncode = 0 + mock_result.stdout = "" + + with ( + patch("subprocess.run", return_value=mock_result), + patch("comfy_cli.command.custom_nodes.command.extract_node_configuration") as mock_extract, + patch("comfy_cli.command.custom_nodes.command.registry_api.publish_node_version") as mock_publish, + patch("comfy_cli.command.custom_nodes.command.zip_files") as mock_zip, + patch("comfy_cli.command.custom_nodes.command.upload_file_to_signed_url") as mock_upload, + patch("comfy_cli.command.custom_nodes.command.get_renderer") as mock_get_renderer, + ): + mock_extract.return_value = create_mock_config() + mock_publish.side_effect = RegistryAPIError( + "Failed to publish node version: 400 Bad Request", status=400, body="Bad Request" + ) + + result = runner.invoke(app, ["publish", "--token", "test-token"]) + + assert result.exit_code == 1 + assert not mock_zip.called + assert not mock_upload.called + mock_get_renderer.return_value.error.assert_called_once() + _, kwargs = mock_get_renderer.return_value.error.call_args + assert kwargs["code"] == "node_publish_failed" + assert kwargs["details"] == {"status": 400, "body": "Bad Request"} + + def test_publish_fails_when_config_is_none(): # extract_node_configuration returns None when pyproject.toml is missing; # validate_node_for_publishing must exit 1 (not crash on the subsequent diff --git a/tests/comfy_cli/registry/test_api.py b/tests/comfy_cli/registry/test_api.py index 9521b65e..c355972c 100644 --- a/tests/comfy_cli/registry/test_api.py +++ b/tests/comfy_cli/registry/test_api.py @@ -5,7 +5,7 @@ from unittest.mock import MagicMock, patch from comfy_cli.registry import PyProjectConfig -from comfy_cli.registry.api import RegistryAPI +from comfy_cli.registry.api import RegistryAPI, RegistryAPIError from comfy_cli.registry.types import ComfyConfig, License, ProjectConfig, URLs @@ -69,9 +69,20 @@ def test_publish_node_version_failure(self, mock_post): mock_response.text = "Bad Request" mock_post.return_value = mock_response - with self.assertRaises(Exception) as context: + with self.assertRaises(RegistryAPIError) as context: self.registry_api.publish_node_version(self.node_config, self.token) self.assertIn("Failed to publish node version", str(context.exception)) + self.assertEqual(context.exception.status, 400) + self.assertEqual(context.exception.body, "Bad Request") + + def test_publish_node_version_requires_publisher_id(self): + # Client-side validation failure: typed error, no HTTP status/body. + self.node_config.tool_comfy.publisher_id = "" + with self.assertRaises(RegistryAPIError) as context: + self.registry_api.publish_node_version(self.node_config, self.token) + self.assertIn("Publisher ID is required", str(context.exception)) + self.assertIsNone(context.exception.status) + self.assertIsNone(context.exception.body) def _mock_publish_response(self, changelog=""): mock_response = MagicMock() @@ -168,9 +179,11 @@ def test_list_all_nodes_failure(self, mock_get): mock_response.text = "Internal Server Error" mock_get.return_value = mock_response - with self.assertRaises(Exception) as context: + with self.assertRaises(RegistryAPIError) as context: self.registry_api.list_all_nodes() self.assertIn("Failed to retrieve nodes", str(context.exception)) + self.assertEqual(context.exception.status, 500) + self.assertEqual(context.exception.body, "Internal Server Error") @patch("requests.get") def test_install_node_success(self, mock_get): @@ -197,6 +210,8 @@ def test_install_node_failure(self, mock_get): mock_response.text = "Not Found" mock_get.return_value = mock_response - with self.assertRaises(Exception) as context: + with self.assertRaises(RegistryAPIError) as context: self.registry_api.install_node("node1") self.assertIn("Failed to install node", str(context.exception)) + self.assertEqual(context.exception.status, 404) + self.assertEqual(context.exception.body, "Not Found") From 55d620c33fcdc7547c9bd4cd9e970481367444a1 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 00:00:21 -0700 Subject: [PATCH 2/2] fix(registry): redact PAT and bound untrusted body in registry API errors (BE-3271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the cursor-review panel findings on the new renderer.error(details={status, body}) surface: - Add sanitize_error_body(): redacts known secrets, escapes CR/LF, and truncates to MAX_ERROR_BODY_CHARS before the untrusted registry response body reaches a log record or an error envelope. - publish_node_version() passes the PAT as a secret, so a registry error that echoes the request payload back can no longer leak the token into terminal output or JSON logs. - Apply the same bounding/escaping to list_all_nodes() and install_node(), which log the full str(e) — an attacker-controlled registry could otherwise forge log lines or flood CI logs. - Fix the adjacent set-literal quirk in the publish fallback: display_error_message({str(e)}) rendered as {'...'} instead of a plain string. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/command/custom_nodes/command.py | 2 +- comfy_cli/registry/api.py | 40 +++++++++++++++++--- tests/comfy_cli/registry/test_api.py | 45 ++++++++++++++++++++++- 3 files changed, 79 insertions(+), 8 deletions(-) diff --git a/comfy_cli/command/custom_nodes/command.py b/comfy_cli/command/custom_nodes/command.py index 1ab72422..d5fe48a5 100644 --- a/comfy_cli/command/custom_nodes/command.py +++ b/comfy_cli/command/custom_nodes/command.py @@ -1129,7 +1129,7 @@ def publish( ) raise typer.Exit(code=1) from e except Exception as e: - ui.display_error_message({str(e)}) + ui.display_error_message(str(e)) raise typer.Exit(code=1) diff --git a/comfy_cli/registry/api.py b/comfy_cli/registry/api.py index 7c6f9508..1c57d5fc 100644 --- a/comfy_cli/registry/api.py +++ b/comfy_cli/registry/api.py @@ -13,6 +13,29 @@ PyProjectConfig, ) +MAX_ERROR_BODY_CHARS = 2000 + + +def sanitize_error_body(text: str, *, secrets: tuple[str | None, ...] = ()) -> str: + """Make an untrusted registry response body safe to log and render. + + The registry is upstream of us: its response body can echo back what we + sent (including the publish PAT), embed newlines that forge extra log + lines, or be arbitrarily large. Redact known secrets, flatten CR/LF to + escapes, and bound the length before the body reaches a log record or a + ``renderer.error`` envelope. + """ + for secret in secrets: + if secret: + text = text.replace(secret, "***REDACTED***") + + text = text.replace("\r", "\\r").replace("\n", "\\n") + + if len(text) > MAX_ERROR_BODY_CHARS: + text = f"{text[:MAX_ERROR_BODY_CHARS]}... [truncated, {len(text)} chars total]" + + return text + class RegistryAPIError(Exception): """Raised when a Registry API call fails. @@ -105,10 +128,13 @@ def publish_node_version( signedUrl=data["signedUrl"], ) else: + # The publish request body carries the PAT, so a registry error that + # echoes the payload back would otherwise leak it into logs. + safe_body = sanitize_error_body(response.text, secrets=(token,)) raise RegistryAPIError( - f"Failed to publish node version: {response.status_code} {response.text}", + f"Failed to publish node version: {response.status_code} {safe_body}", status=response.status_code, - body=response.text, + body=safe_body, ) def list_all_nodes(self): @@ -124,10 +150,11 @@ def list_all_nodes(self): raw_nodes = response.json()["nodes"] return [map_node_to_node_class(node) for node in raw_nodes] else: + safe_body = sanitize_error_body(response.text) raise RegistryAPIError( - f"Failed to retrieve nodes: {response.status_code} - {response.text}", + f"Failed to retrieve nodes: {response.status_code} - {safe_body}", status=response.status_code, - body=response.text, + body=safe_body, ) def install_node(self, node_id, version=None): @@ -152,10 +179,11 @@ def install_node(self, node_id, version=None): logging.debug(f"RegistryAPI install_node response: {response.json()}") return map_node_version(response.json()) else: + safe_body = sanitize_error_body(response.text) raise RegistryAPIError( - f"Failed to install node: {response.status_code} - {response.text}", + f"Failed to install node: {response.status_code} - {safe_body}", status=response.status_code, - body=response.text, + body=safe_body, ) diff --git a/tests/comfy_cli/registry/test_api.py b/tests/comfy_cli/registry/test_api.py index c355972c..03fb0bea 100644 --- a/tests/comfy_cli/registry/test_api.py +++ b/tests/comfy_cli/registry/test_api.py @@ -5,7 +5,12 @@ from unittest.mock import MagicMock, patch from comfy_cli.registry import PyProjectConfig -from comfy_cli.registry.api import RegistryAPI, RegistryAPIError +from comfy_cli.registry.api import ( + MAX_ERROR_BODY_CHARS, + RegistryAPI, + RegistryAPIError, + sanitize_error_body, +) from comfy_cli.registry.types import ComfyConfig, License, ProjectConfig, URLs @@ -84,6 +89,22 @@ def test_publish_node_version_requires_publisher_id(self): self.assertIsNone(context.exception.status) self.assertIsNone(context.exception.body) + @patch("requests.post") + def test_publish_node_version_failure_redacts_token_echoed_by_registry(self, mock_post): + # The publish payload carries the PAT; a registry error that echoes the + # payload back must not leak it into the message or the error body. + mock_response = MagicMock() + mock_response.status_code = 400 + mock_response.text = f'{{"error":"bad request","sent":{{"personal_access_token":"{self.token}"}}}}' + mock_post.return_value = mock_response + + with self.assertRaises(RegistryAPIError) as context: + self.registry_api.publish_node_version(self.node_config, self.token) + + self.assertNotIn(self.token, str(context.exception)) + self.assertNotIn(self.token, context.exception.body) + self.assertIn("***REDACTED***", context.exception.body) + def _mock_publish_response(self, changelog=""): mock_response = MagicMock() mock_response.status_code = 201 @@ -215,3 +236,25 @@ def test_install_node_failure(self, mock_get): self.assertIn("Failed to install node", str(context.exception)) self.assertEqual(context.exception.status, 404) self.assertEqual(context.exception.body, "Not Found") + + +class TestSanitizeErrorBody(unittest.TestCase): + def test_leaves_ordinary_body_untouched(self): + self.assertEqual(sanitize_error_body("Not Found"), "Not Found") + + def test_redacts_secrets(self): + body = '{"personal_access_token":"s3cr3t"}' + self.assertEqual(sanitize_error_body(body, secrets=("s3cr3t",)), '{"personal_access_token":"***REDACTED***"}') + + def test_ignores_empty_secrets(self): + # A falsy token must not turn every empty string into a redaction marker. + self.assertEqual(sanitize_error_body("plain", secrets=(None, "")), "plain") + + def test_escapes_newlines_to_prevent_log_forgery(self): + forged = "error\nINFO: everything is fine\r\n" + self.assertEqual(sanitize_error_body(forged), "error\\nINFO: everything is fine\\r\\n") + + def test_truncates_oversized_body(self): + result = sanitize_error_body("x" * (MAX_ERROR_BODY_CHARS + 500)) + self.assertTrue(result.startswith("x" * MAX_ERROR_BODY_CHARS)) + self.assertIn(f"truncated, {MAX_ERROR_BODY_CHARS + 500} chars total", result)