Skip to content
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
8 changes: 6 additions & 2 deletions openhands-sdk/openhands/sdk/utils/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from collections.abc import Mapping

from openhands.sdk.logger import get_logger
from openhands.sdk.utils.redact import redact_text_secrets


logger = get_logger(__name__)
Expand Down Expand Up @@ -61,11 +62,14 @@ def execute_command(
if isinstance(cmd, str):
cmd_to_run = cmd
use_shell = True
logger.info("$ %s", cmd)
cmd_str = cmd
else:
cmd_to_run = cmd
use_shell = False
logger.info("$ %s", " ".join(shlex.quote(c) for c in cmd))
cmd_str = " ".join(shlex.quote(c) for c in cmd)

# Log the command with sensitive values redacted
logger.info("$ %s", redact_text_secrets(cmd_str))
Comment thread
simonrosenberg marked this conversation as resolved.
Comment on lines 68 to +72
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Good taste: Clean implementation.

Extracting cmd_str once eliminates duplication and makes the redaction application obvious. The security fix is minimal and uses existing utilities. Well done.


proc = subprocess.Popen(
cmd_to_run,
Expand Down
81 changes: 80 additions & 1 deletion tests/sdk/utils/test_command.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
from collections import OrderedDict
from unittest.mock import patch

import pytest

from openhands.sdk.utils.command import sanitized_env
from openhands.sdk.utils.command import execute_command, sanitized_env


def test_sanitized_env_returns_copy():
Expand Down Expand Up @@ -47,3 +48,81 @@ def test_sanitized_env_removes_ld_library_path_when_orig_empty():
"""When LD_LIBRARY_PATH_ORIG is empty, removes LD_LIBRARY_PATH."""
env = {"LD_LIBRARY_PATH": "/pyinstaller", "LD_LIBRARY_PATH_ORIG": ""}
assert "LD_LIBRARY_PATH" not in sanitized_env(env)


# ---------------------------------------------------------------------------
# execute_command logging redaction
# ---------------------------------------------------------------------------


class TestExecuteCommandLoggingRedaction:
Comment on lines +53 to +58
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: This test is checking the wrong abstraction.

You're testing redact_text_secrets() directly instead of verifying that execute_command() actually applies redaction to logs. If someone removed the redact_text_secrets call from command.py, this test would still pass.

Better approach:

def test_redacts_api_key_from_logged_command(self, caplog):
    """execute_command redacts API keys from logged output."""
    with patch("subprocess.Popen") as mock_popen:
        mock_process = mock_popen.return_value
        mock_process.communicate.return_value = (b"", b"")
        mock_process.returncode = 0
        
        cmd = "curl -H 'Authorization: sk-ant-api00-abcd1234' https://api.anthropic.com"
        execute_command(cmd)
        
        # Verify secret is redacted in logs
        assert "sk-ant-api00-abcd1234" not in caplog.text
        assert "<redacted>" in caplog.text
        assert "curl" in caplog.text

This tests the actual integration: does execute_command log with redaction applied?

"""Tests for sensitive value redaction in execute_command logging."""

def test_logs_command_without_errors(self, caplog):
Comment on lines 50 to +61
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: This test doesn't verify secrets are redacted.

You have a command with LMNR_PROJECT_API_KEY=secret123 but don't assert that secret123 is redacted from logs. Add:

# Verify secret is redacted
assert "secret123" not in caplog.text
assert "<redacted>" in caplog.text

Also, the mock should return a valid process to avoid the RuntimeError catch:

mock_process.communicate.return_value = (b"", b"")
mock_process.returncode = 0

"""Command logging with redaction doesn't raise errors."""
with patch("subprocess.Popen") as mock_popen:
mock_process = mock_popen.return_value
mock_process.stdout = None
mock_process.stderr = None

cmd = ["docker", "run", "-e", "LMNR_PROJECT_API_KEY=secret123", "image"]

try:
execute_command(cmd)
except RuntimeError:
Comment on lines +60 to +72
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: Same issue - testing redact_text_secrets() directly instead of the execute_command() integration.

This should call execute_command() with a command containing api_key=secretvalue and verify the logged output has it redacted. Testing the utility function directly belongs in tests/sdk/utils/test_redact.py.

# Logging should happen even if subprocess fails
pass

# Command should be logged
assert "docker" in caplog.text
assert "run" in caplog.text
assert "image" in caplog.text

def test_redacts_api_key_from_string_command(self):
"""API keys in string commands are properly redacted."""
from openhands.sdk.utils.redact import redact_text_secrets

# Test the redaction function directly
# Valid Anthropic key format: sk-ant-api[2 digits]-[20+ chars]
cmd_str = "curl -H 'Authorization: sk-ant-api00-abcd1234567890abcdefghijklmnop' https://api.anthropic.com"
redacted = redact_text_secrets(cmd_str)

# The secret should be redacted in the output of the function
assert "sk-ant-api00-abcd1234567890abcdefghijklmnop" not in redacted
assert "<redacted>" in redacted
# Command structure should be preserved
assert "curl" in redacted
assert "https://api.anthropic.com" in redacted

def test_redacts_key_value_env_format(self):
"""KEY=VALUE environment variable format is redacted."""
from openhands.sdk.utils.redact import redact_text_secrets

cmd_str = "docker run -e api_key='secretvalue123456789' -e DEBUG=true image"
redacted = redact_text_secrets(cmd_str)

# api_key value should be redacted
assert "secretvalue123456789" not in redacted
# But non-sensitive DEBUG value should be present
assert "DEBUG" in redacted
# Command structure preserved
assert "docker" in redacted

def test_preserves_non_sensitive_args(self, caplog):
"""Non-sensitive arguments are preserved in logs."""
with patch("subprocess.Popen") as mock_popen:
mock_process = mock_popen.return_value
mock_process.stdout = None
mock_process.stderr = None

cmd = ["docker", "run", "-e", "DEBUG=true", "image:latest"]

try:
execute_command(cmd)
except RuntimeError:
pass

# Non-sensitive values should be visible
assert "DEBUG=true" in caplog.text
assert "image:latest" in caplog.text
assert "docker" in caplog.text
Loading