Skip to content

feat: Security fixes for executable validation and state file permissions #8

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.

Sign up for GitHub

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

Closed
Patoruzuy wants to merge 3 commits into from
Closed

feat: Security fixes for executable validation and state file permissions #8

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdcba08
test: Mock is_user_writable in test_validate_caddy_success to prevent…
Feb 7, 2026
09ed954
fix: Correct @patch decorator order in test_validate_caddy_success
Feb 7, 2026
276c267
chore: Trigger CI workflow
Feb 7, 2026
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
7 changes: 5 additions & 2 deletions tests/test_executable_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,22 +118,25 @@ def test_find_nonexistent_in_path(self):
result = find_executable_in_path("definitely_not_a_real_executable_12345")
self.assertIsNone(result)

@patch("devhost_cli.executable_validation.is_user_writable")
@patch("subprocess.run")
def test_validate_caddy_success(self, mock_run):
def test_validate_caddy_success(self, mock_run, mock_is_writable):
Comment on lines +121 to +123

Copilot AI Feb 7, 2026

Copy link

Choose a reason for hiding this comment

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

The PR title/description says this is a security fix for executable validation and state file permissions, but this change is only updating a unit test by mocking is_user_writable. If the intended security behavior changed in production code, that code change isn’t reflected here; otherwise, consider updating the PR title/description to match the actual scope.

Copilot uses AI. Check for mistakes.
"""Test Caddy validation with successful version check."""
# Create a fake executable
test_file = self.temp_path / "caddy"
test_file.write_text("#!/bin/bash\necho Caddy v2.7.4")
if sys.platform != "win32":
test_file.chmod(0o755)

# Mock writability check to return safe (not user-writable)
mock_is_writable.return_value = (False, None)

Comment on lines +131 to +133

Copilot AI Feb 7, 2026

Copy link

Choose a reason for hiding this comment

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

Consider asserting that the writability check was actually invoked (e.g., mock_is_writable.assert_called() / assert called with the Path for test_file) so this test doesn’t silently pass if validate_executable() stops performing the security check or the patch target becomes stale.

Copilot uses AI. Check for mistakes.
# Mock subprocess.run to return Caddy version
mock_result = MagicMock()
mock_result.returncode = 0
mock_result.stdout = "Caddy v2.7.4 h1:abc123"
mock_run.return_value = mock_result

# Skip writability check
is_valid, error = validate_caddy_executable(str(test_file))

# On Windows, we might not have execute permission
Expand Down
Loading