fix: Enforce non-zero positive sandbox timeouts to prevent indefinite hangs (#1808)

backend/secuscan/config.py

@@ -8,8 +8,11 @@
 from pydantic_settings import BaseSettings
 import base64
 import hashlib
+import logging
 import os
 
+logger = logging.getLogger(__name__)
+
 PROJECT_ROOT = Path(__file__).resolve().parent.parent
 
 # Mandatory network denylist. These ranges cover cloud metadata endpoints,
@@ -214,6 +217,14 @@ class Config:
         env_prefix = "SECUSCAN_"
         case_sensitive = False
 
+    @field_validator("sandbox_timeout", "parser_sandbox_timeout_seconds")
+    @classmethod
+    def validate_sandbox_timeouts(cls, v: int) -> int:
+        if v is None or v <= 0:
+            logger.warning("Refusing falsy or non-positive sandbox timeout: %s. A non-zero timeout is required.", v)
+            raise ValueError("Timeout settings must be a positive non-zero integer")
+        return v
+
     @field_validator(
         "cors_allowed_origins",
         "cors_allowed_methods",

backend/secuscan/models.py

@@ -4,8 +4,11 @@
 
 from typing import Optional, Dict, Any, List, Annotated
 from datetime import datetime
-from pydantic import BaseModel, Field, RootModel
+from pydantic import BaseModel, Field, RootModel, field_validator
 from enum import Enum
+import logging
+
+logger = logging.getLogger(__name__)
 
 
 MAX_BULK_DELETE = 500
@@ -33,6 +36,14 @@ class SandboxConfig(BaseModel):
     max_output_bytes: int = Field(default=5_242_880, description="Max bytes captured from stdout/stderr")
     allow_network: bool = Field(default=True, description="Whether subprocess can make network calls")
 
+    @field_validator("timeout_seconds")
+    @classmethod
+    def validate_timeout_seconds(cls, v: int) -> int:
+        if v is None or v <= 0:
+            logger.warning("Refusing falsy or non-positive sandbox timeout_seconds: %s. A non-zero timeout is required.", v)
+            raise ValueError("timeout_seconds must be a positive non-zero integer")
+        return v
+
 
 class SandboxViolation(Exception):
     """Raised when sandbox constraints are violated."""

backend/secuscan/parser_sandbox.py

@@ -250,6 +250,9 @@ def run_parser_in_sandbox(
     Raises:
         ParserSandboxError: on timeout, crash, oversized output, or malformed JSON.
     """
+    if timeout_seconds is None or timeout_seconds <= 0:
+        raise ValueError("Parser sandbox timeout_seconds must be a positive non-zero integer")
+
     if not parser_path.exists():
         raise ParserSandboxError(plugin_id, "parser.py not found")
 

backend/secuscan/sandbox_executor.py

@@ -1,4 +1,4 @@
-import asyncio
+import asyncio
 import logging
 import platform
 from asyncio import subprocess
@@ -105,15 +105,16 @@ async def sandbox_execute(
     Args:
         cmd: Command list to execute.
         config: SandboxConfig with timeout, memory, output limits.
-            When timeout_seconds is 0 or None, no wall-clock timeout is
-            applied internally (the caller handles it externally).
         broadcast_callback: Optional async callable(chunk: bytes, stream_name: str)
             invoked for each output chunk to enable live streaming.
 
     Returns (stdout_str, stderr_str, exit_code, violation_reason).
     violation_reason is None on success, or one of
     "timeout", "memory_limit", "output_limit".
     """
+    if config.timeout_seconds is None or config.timeout_seconds <= 0:
+        raise ValueError("Sandbox timeout_seconds must be a positive non-zero integer")
+
     preexec_fn = _build_preexec_fn(config) if IS_LINUX else None
 
     rss_before = 0

testing/backend/test_sandbox_executor.py

@@ -233,3 +233,21 @@ async def test_resolve_sandbox_config_plugin_overrides():
     assert resolved.timeout_seconds == 999
     assert resolved.max_memory_mb == 2048
     assert resolved.max_output_bytes == 5_242_880
+
+
+@pytest.mark.asyncio
+async def test_sandbox_execute_refuses_zero_timeout():
+    """Verify sandbox_execute raises ValueError if config timeout_seconds is 0."""
+    invalid_cfg = SandboxConfig.model_construct(timeout_seconds=0)
+    with pytest.raises(ValueError) as exc_info:
+        await sandbox_execute([sys.executable, "-c", "print('hello')"], invalid_cfg)
+    assert "Sandbox timeout_seconds must be a positive non-zero integer" in str(exc_info.value)
+
+
+@pytest.mark.asyncio
+async def test_sandbox_execute_refuses_none_timeout():
+    """Verify sandbox_execute raises ValueError if config timeout_seconds is None."""
+    invalid_cfg = SandboxConfig.model_construct(timeout_seconds=None)
+    with pytest.raises(ValueError) as exc_info:
+        await sandbox_execute([sys.executable, "-c", "print('hello')"], invalid_cfg)
+    assert "Sandbox timeout_seconds must be a positive non-zero integer" in str(exc_info.value)

testing/backend/unit/test_config_settings.py

@@ -286,3 +286,55 @@ def test_sandbox_settings_env_override():
     assert s.sandbox_timeout == 30
     assert s.sandbox_memory_mb == 128
     assert s.sandbox_allow_network is False
+
+
+def test_sandbox_settings_validation_refuses_zero():
+    """ValidationError is raised when sandbox_timeout or parser_sandbox_timeout_seconds is 0."""
+    from pydantic import ValidationError
+    import pytest
+    with pytest.raises(ValidationError) as exc_info:
+        Settings(sandbox_timeout=0)
+    assert "Timeout settings must be a positive non-zero integer" in str(exc_info.value)
+
+    with pytest.raises(ValidationError) as exc_info:
+        Settings(parser_sandbox_timeout_seconds=0)
+    assert "Timeout settings must be a positive non-zero integer" in str(exc_info.value)
+
+
+def test_sandbox_settings_validation_refuses_negative():
+    """ValidationError is raised when sandbox_timeout or parser_sandbox_timeout_seconds is negative."""
+    from pydantic import ValidationError
+    import pytest
+    with pytest.raises(ValidationError) as exc_info:
+        Settings(sandbox_timeout=-10)
+    assert "Timeout settings must be a positive non-zero integer" in str(exc_info.value)
+
+
+def test_sandbox_settings_env_var_validation(monkeypatch):
+    """Verify that env var loading fails if timeouts are invalid (0, negative, None, empty, or non-int)."""
+    from pydantic import ValidationError
+    import pytest
+
+    # Test environment variable SECUSCAN_SANDBOX_TIMEOUT=0
+    monkeypatch.setenv("SECUSCAN_SANDBOX_TIMEOUT", "0")
+    with pytest.raises(ValidationError) as exc_info:
+        Settings()
+    assert "Timeout settings must be a positive non-zero integer" in str(exc_info.value)
+
+    # Test environment variable SECUSCAN_PARSER_SANDBOX_TIMEOUT_SECONDS=0
+    monkeypatch.delenv("SECUSCAN_SANDBOX_TIMEOUT", raising=False)
+    monkeypatch.setenv("SECUSCAN_PARSER_SANDBOX_TIMEOUT_SECONDS", "0")
+    with pytest.raises(ValidationError) as exc_info:
+        Settings()
+    assert "Timeout settings must be a positive non-zero integer" in str(exc_info.value)
+
+    # Test environment variable SECUSCAN_SANDBOX_TIMEOUT="" (empty string)
+    monkeypatch.delenv("SECUSCAN_PARSER_SANDBOX_TIMEOUT_SECONDS", raising=False)
+    monkeypatch.setenv("SECUSCAN_SANDBOX_TIMEOUT", "")
+    with pytest.raises(ValidationError):
+        Settings()
+
+    # Test environment variable SECUSCAN_SANDBOX_TIMEOUT="invalid" (non-integer string)
+    monkeypatch.setenv("SECUSCAN_SANDBOX_TIMEOUT", "invalid")
+    with pytest.raises(ValidationError):
+        Settings()

testing/backend/unit/test_parser_sandbox.py

@@ -403,3 +403,19 @@ def test_stderr_excerpt_truncated_to_2000_chars(self):
     def test_str_contains_plugin_id(self):
         err = ParserSandboxError("my_plugin", "bad thing")
         assert "my_plugin" in str(err)
+
+
+class TestParserSandboxTimeoutValidation:
+    def test_refuses_zero_timeout(self, tmp_path):
+        """Verify run_parser_in_sandbox raises ValueError if timeout_seconds is 0."""
+        p = _write_parser(tmp_path, "def parse(output): return {}")
+        with pytest.raises(ValueError) as exc_info:
+            run_parser_in_sandbox(p, "test_plugin", "data", timeout_seconds=0)
+        assert "Parser sandbox timeout_seconds must be a positive non-zero integer" in str(exc_info.value)
+
+    def test_refuses_none_timeout(self, tmp_path):
+        """Verify run_parser_in_sandbox raises ValueError if timeout_seconds is None."""
+        p = _write_parser(tmp_path, "def parse(output): return {}")
+        with pytest.raises(ValueError) as exc_info:
+            run_parser_in_sandbox(p, "test_plugin", "data", timeout_seconds=None)
+        assert "Parser sandbox timeout_seconds must be a positive non-zero integer" in str(exc_info.value)

testing/backend/unit/test_sandbox_config_model.py

@@ -81,3 +81,21 @@ def test_repr_includes_all_fields(self):
         repr_str = repr(config)
         assert "45" in repr_str
         assert "128" in repr_str
+
+
+class TestSandboxConfigValidation:
+    def test_timeout_seconds_refuses_zero(self):
+        """ValidationError is raised when timeout_seconds is 0."""
+        from pydantic import ValidationError
+        import pytest
+        with pytest.raises(ValidationError) as exc_info:
+            SandboxConfig(timeout_seconds=0)
+        assert "timeout_seconds must be a positive non-zero integer" in str(exc_info.value)
+
+    def test_timeout_seconds_refuses_negative(self):
+        """ValidationError is raised when timeout_seconds is negative."""
+        from pydantic import ValidationError
+        import pytest
+        with pytest.raises(ValidationError) as exc_info:
+            SandboxConfig(timeout_seconds=-10)
+        assert "timeout_seconds must be a positive non-zero integer" in str(exc_info.value)

testing/backend/unit/test_saved_views.py

@@ -54,6 +54,7 @@ async def app_client():
         ) as client:
             client.api_key = api_key
             client.test_transport = transport
+            client.app = _app
             yield client
 
     await test_db.disconnect()
@@ -358,20 +359,32 @@ async def test_filter_json_with_null_values_rejected(app_client: AsyncClient):
 @pytest.mark.asyncio
 async def test_unauthenticated_request_rejected(app_client: AsyncClient):
     """Requests without a valid API key/session are rejected, not served."""
-    res = await app_client.get(
-        "/api/v1/saved-views", headers={"X-Api-Key": ""}
-    )
-    assert res.status_code == 401
+    if hasattr(app_client, "app"):
+        app_client.app.dependency_overrides.pop(require_api_key, None)
+    try:
+        res = await app_client.get(
+            "/api/v1/saved-views", headers={"X-Api-Key": ""}
+        )
+        assert res.status_code == 401
+    finally:
+        if hasattr(app_client, "app"):
+            app_client.app.dependency_overrides[require_api_key] = _mock_require_api_key
 
 
 @pytest.mark.skip(reason="pre-existing upstream issue: app_client overrides auth so 401 cannot be tested here")
 @pytest.mark.asyncio
 async def test_wrong_api_key_rejected(app_client: AsyncClient):
     """A malformed/incorrect API key is rejected."""
-    res = await app_client.get(
-        "/api/v1/saved-views", headers={"X-Api-Key": "not-the-real-key"}
-    )
-    assert res.status_code == 401
+    if hasattr(app_client, "app"):
+        app_client.app.dependency_overrides.pop(require_api_key, None)
+    try:
+        res = await app_client.get(
+            "/api/v1/saved-views", headers={"X-Api-Key": "not-the-real-key"}
+        )
+        assert res.status_code == 401
+    finally:
+        if hasattr(app_client, "app"):
+            app_client.app.dependency_overrides[require_api_key] = _mock_require_api_key
 
 
 @pytest.mark.asyncio