Skip to content

Commit

Permalink
refactor: enhance security checks in publish command and update tests
Browse files Browse the repository at this point in the history
Updated the security check implementation in the publish command to use 'ruff' with an '--exit-zero' flag, changing the output handling to display warnings instead of errors. Adjusted the test cases to reflect these changes, ensuring that security warnings are correctly asserted. This improves the clarity of security feedback during the publishing process.
  • Loading branch information
yoland68 committed Dec 20, 2024
1 parent e031120 commit 0601faa
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 8 deletions.
12 changes: 7 additions & 5 deletions comfy_cli/command/custom_nodes/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -709,14 +709,16 @@ def publish(
# Run security checks first
typer.echo("Running security checks...")
try:
# Run ruff check with security rules
cmd = ["ruff", "check", ".", "--select", "S102,S307"]
# Run ruff check with security rules and --exit-zero to only warn
cmd = ["ruff", "check", ".", "--select", "S102,S307", "--exit-zero"]
result = subprocess.run(cmd, capture_output=True, text=True)

if result.returncode != 0:
print("[red]Security issues found:[/red]")
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)
raise typer.Exit(code=1)
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]")
Expand Down
11 changes: 8 additions & 3 deletions tests/comfy_cli/command/nodes/test_publish.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@ def test_publish_fails_on_security_violations():
mock_result.returncode = 1
mock_result.stdout = "S102 Use of exec() detected"

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

assert result.exit_code == 1
assert "Security issues found" in result.stdout
# 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():
Expand Down

0 comments on commit 0601faa

Please sign in to comment.