Skip to content
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.

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

Add Ruff dependencies and warning message when custom nodes are using eval or exec calls #218

Merged
merged 4 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions comfy_cli/command/custom_nodes/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,27 @@ def publish(
typer.echo("Validating node configuration...")
config = extract_node_configuration()

# Run security checks first
typer.echo("Running security checks...")
try:
# Run ruff check with security rules and --exit-zero to only warn
cmd = ["ruff", "check", ".", "-q", "--select", "S102,S307", "--exit-zero"]
result = subprocess.run(cmd, capture_output=True, text=True)

if result.stdout: # Changed from checking returncode to checking if there's output
print("[yellow]Security warnings found:[/yellow]") # Changed from red to yellow to indicate warning
print(result.stdout)
print("[bold yellow]We will soon disable exec and eval, so this will be an error soon.[/bold yellow]")
# TODO: re-enable exit when we disable exec and eval
# raise typer.Exit(code=1)

except FileNotFoundError:
print("[red]Ruff is not installed. Please install it with 'pip install ruff'[/red]")
raise typer.Exit(code=1)
except Exception as e:
print(f"[red]Error running security check: {e}[/red]")
raise typer.Exit(code=1)

# Prompt for API Key
if not token:
token = typer.prompt(
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ dependencies = [
"typing-extensions>=4.7.0",
"uv",
"websocket-client",
"ruff",
"semver~=3.0.2",
]

Expand Down
89 changes: 89 additions & 0 deletions tests/comfy_cli/command/nodes/test_publish.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
from unittest.mock import MagicMock, patch

from typer.testing import CliRunner

from comfy_cli.command.custom_nodes.command import app

runner = CliRunner()


def test_publish_fails_on_security_violations():
# Mock subprocess.run to simulate security violations
mock_result = MagicMock()
mock_result.returncode = 1
mock_result.stdout = "S102 Use of exec() detected"

with (
patch("subprocess.run", return_value=mock_result),
patch("typer.prompt", return_value="test-token"),
):
result = runner.invoke(app, ["publish"])

# TODO: re-enable exit when we disable exec and eval
# assert result.exit_code == 1
# assert "Security issues found" in result.stdout
assert "Security warnings found" in result.stdout


def test_publish_continues_on_no_security_violations():
# Mock subprocess.run to simulate no violations
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("typer.prompt") as mock_prompt,
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,
):
# Setup the mocks
mock_extract.return_value = {"name": "test-node"}
mock_prompt.return_value = "test-token"
mock_publish.return_value = MagicMock(signedUrl="https://test.url")

# Run the publish command
_result = runner.invoke(app, ["publish"])

# Verify the publish flow continued
assert mock_extract.called
assert mock_publish.called
assert mock_zip.called
assert mock_upload.called


def test_publish_handles_missing_ruff():
with patch("subprocess.run", side_effect=FileNotFoundError()):
result = runner.invoke(app, ["publish"])

assert result.exit_code == 1
assert "Ruff is not installed" in result.stdout


def test_publish_with_token_option():
# Mock subprocess.run to simulate no violations
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,
):
# Setup the mocks
mock_extract.return_value = {"name": "test-node"}
mock_publish.return_value = MagicMock(signedUrl="https://test.url")

# Run the publish command with token
_result = runner.invoke(app, ["publish", "--token", "test-token"])

# Verify the publish flow worked with provided token
assert mock_extract.called
assert mock_publish.called
assert mock_zip.called
assert mock_upload.called
Loading