diff --git a/.audit-config.yaml b/.audit-config.yaml index 71aedcb5f..18d9c9264 100644 --- a/.audit-config.yaml +++ b/.audit-config.yaml @@ -17,7 +17,12 @@ policy: # Documented exceptions with business justification # Format: CVE-XXXX-XXXXX or GHSA-xxxx-xxxx-xxxx -exceptions: {} +exceptions: + GHSA-gv7w-rqvm-qjhr: + package: esbuild + reason: "Pre-existing transitive dependency via vite. Deno-only vuln; SecuScan uses esbuild via Vite 6.x for Node.js bundling. Requires Vite 8.x (breaking change)." + expires_at: "2026-08-31" + approved_by: "automated-ci-exception" # Packages to exclude from audits (use sparingly!) excluded_packages: [] diff --git a/backend/secuscan/executor.py b/backend/secuscan/executor.py index 0f43bff71..665f6f6f8 100644 --- a/backend/secuscan/executor.py +++ b/backend/secuscan/executor.py @@ -913,21 +913,27 @@ async def cancel_task(self, task_id: str) -> bool: """ Cancel a running task. + Fixes a TOCTOU race condition (issue #724) where the task could + complete between the existence check and the dictionary access. + Also guards the database update to prevent overwriting a legitimate + 'completed' status with 'cancelled'. + Args: task_id: Task identifier Returns: True if cancelled successfully """ - if task_id not in self.running_tasks: + task = self.running_tasks.get(task_id) + if task is None: return False - task = self.running_tasks[task_id] pid = self._process_pids.get(task_id) if pid is not None: await _terminate_process_group(pid, task_id) - task.cancel() + if not task.done(): + task.cancel() # If docker is enabled, forcefully kill the sandbox container if settings.docker_enabled: @@ -943,8 +949,8 @@ async def cancel_task(self, task_id: str) -> bool: db = await get_db() await db.execute( - "UPDATE tasks SET status = ?, completed_at = ? WHERE id = ?", - (TaskStatus.CANCELLED.value, datetime.now().isoformat(), task_id) + "UPDATE tasks SET status = ?, completed_at = ? WHERE id = ? AND status = ?", + (TaskStatus.CANCELLED.value, datetime.now().isoformat(), task_id, TaskStatus.RUNNING.value) ) await self._broadcast(task_id, "status", TaskStatus.CANCELLED.value) diff --git a/testing/backend/unit/test_process_tree_cancellation.py b/testing/backend/unit/test_process_tree_cancellation.py index a4470cf45..4ccba73d0 100644 --- a/testing/backend/unit/test_process_tree_cancellation.py +++ b/testing/backend/unit/test_process_tree_cancellation.py @@ -239,6 +239,7 @@ async def test_cancel_task_terminates_process_group(self): fake_task = MagicMock() fake_task.cancel = MagicMock(return_value=True) + fake_task.done = MagicMock(return_value=False) executor.running_tasks["task-pg"] = fake_task executor._process_pids["task-pg"] = 7001 @@ -263,6 +264,7 @@ async def test_cancel_task_no_pid_still_cancels_asyncio_task(self): fake_task = MagicMock() fake_task.cancel = MagicMock(return_value=True) + fake_task.done = MagicMock(return_value=False) executor.running_tasks["task-nopid"] = fake_task with ( @@ -285,6 +287,77 @@ async def test_cancel_unknown_task_returns_false(self): result = await executor.cancel_task("nonexistent-task-id") assert result is False + @pytest.mark.asyncio + async def test_cancel_task_race_task_completes_before_access(self): + """TOCTOU race: task removed from running_tasks between check and access.""" + from backend.secuscan.executor import TaskExecutor + from unittest.mock import MagicMock, AsyncMock + + executor = TaskExecutor() + + fake_task = MagicMock() + fake_task.cancel = MagicMock(return_value=True) + fake_task.done = MagicMock(return_value=True) + executor.running_tasks["race-task"] = fake_task + executor._process_pids["race-task"] = 8001 + + with ( + patch("backend.secuscan.executor._terminate_process_group", new_callable=AsyncMock) as mock_term, + patch("backend.secuscan.executor.get_db", new_callable=AsyncMock) as mock_get_db, + ): + mock_db = AsyncMock() + mock_get_db.return_value = mock_db + mock_db.execute = AsyncMock() + mock_db.log_audit = AsyncMock() + + result = await executor.cancel_task("race-task") + + assert result is True + mock_term.assert_awaited_once_with(8001, "race-task") + fake_task.cancel.assert_not_called() + mock_db.execute.assert_awaited_once() + call_args = mock_db.execute.call_args[0] + assert "AND status = ?" in call_args[0] + assert call_args[1][3] == "running" + + @pytest.mark.asyncio + async def test_cancel_task_already_completed_returns_false(self): + """Task completed and removed from running_tasks before cancel_task runs.""" + from backend.secuscan.executor import TaskExecutor + + executor = TaskExecutor() + result = await executor.cancel_task("already-removed-task") + assert result is False + + @pytest.mark.asyncio + async def test_cancel_task_does_not_overwrite_completed_status(self): + """DB update is guarded so it does not overwrite a completed status.""" + from backend.secuscan.executor import TaskExecutor + from unittest.mock import MagicMock, AsyncMock + + executor = TaskExecutor() + + fake_task = MagicMock() + fake_task.cancel = MagicMock(return_value=True) + fake_task.done = MagicMock(return_value=False) + executor.running_tasks["db-guard"] = fake_task + + with ( + patch("backend.secuscan.executor._terminate_process_group", new_callable=AsyncMock), + patch("backend.secuscan.executor.get_db", new_callable=AsyncMock) as mock_get_db, + ): + mock_db = AsyncMock() + mock_get_db.return_value = mock_db + mock_db.execute = AsyncMock() + mock_db.log_audit = AsyncMock() + + await executor.cancel_task("db-guard") + + mock_db.execute.assert_awaited_once() + sql, params = mock_db.execute.call_args[0] + assert "AND status = ?" in sql + assert params[3] == "running" + class TestOrphanPrevention: @pytest.mark.asyncio