fix: workflow scheduler now applies route-level security controls#689
Conversation
b54a940 to
2b73be5
Compare
utksh1
left a comment
There was a problem hiding this comment.
This needs changes before review can proceed. The branch currently has merge conflicts, and the new _execute_scan_safe() path schedules executor.execute_task() internally while start_task() also adds executor.execute_task() to BackgroundTasks, so API-created scans can be executed twice. Please rebase on current main, resolve conflicts, and make task scheduling happen in exactly one place while preserving the existing TestClient/background task behavior.
987c6c4 to
8188905
Compare
|
@utksh1 I've address the requested changes. You may review it now |
utksh1
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push. This route-level scheduler security change is still too broad to merge without a clean rebase and focused tests for the workflow scheduler path. Please reduce unrelated churn, rebase on main, and demonstrate the route-level controls are applied consistently.
1ed1439 to
4be01d8
Compare
|
Rebased on latest main. Scope reduced to 7 backend files + focused tests. No unrelated docs, frontend, or test deletions. Single clean commit. See diff for details. |
- Extracted _execute_scan_safe() in routes.py as shared security entry point used by both start_task() and run_workflow_once() - Scheduler _run_workflow() now validates targets, enforces rate limits, checks network policy, acquires concurrency slots, and logs source - Added WorkflowRateLimiter to ratelimit.py with per-workflow and per-user limits - Added source parameter (api|workflow|scheduler) to executor audit logging - Added workflow config settings (min interval, max per user, consent refresh) - Added focused tests for workflow scheduler security path Closes utksh1#655
4be01d8 to
8fc5290
Compare
|
@utksh1 I've made the changes you may review now. |
utksh1
left a comment
There was a problem hiding this comment.
Checks are green, but this is still too broad for the workflow scheduler security issue. The PR rewrites a large part of routes.py, changes executor audit/completion behavior, adds a new global workflow rate limiter, changes main.py shutdown behavior, and modifies plugin input handling. Please split this down to the minimum scheduler hardening path: validate/enforce policy for scheduler-triggered workflow scans with focused tests, and move unrelated executor lifecycle, shutdown, and route refactors into separate PRs.
…low scans - _run_workflow() now validates plugin existence, targets in safe mode, enforces network policy, rate limits per (client, plugin), and acquires concurrency slots before executing each step - tick() enforces workflow_min_interval_seconds via WorkflowRateLimiter - run_workflow_once() applies the same workflow rate limit - Added WorkflowRateLimiter with per-workflow rate limiting - Added workflow_min_interval_seconds config setting - Each check failure gracefully logs and skips the step Executor lifecycle, shutdown handling, route refactoring, and plugin input handling split into separate PRs. Closes utksh1#655
|
@utksh1 I've addressed the changes asked. Please review it. |
utksh1
left a comment
There was a problem hiding this comment.
Approved current head after re-review. The latest update narrows the PR to scheduler workflow security controls, removes the unrelated route/executor/shutdown/plugin-input refactors from the earlier version, and keeps focused coverage for workflow rate limiting, target validation, task rate limiting, safe-mode handling, and concurrency acquisition. I also updated the branch onto current main; fresh CI is green.
|
@utksh1 Thanks for your valuable time. Please review my other PRs too. |
Description
Fixes #655 — Workflow Scheduler Bypasses All Route-Level Security Controls.
Changes
routes.py— Extracted_execute_scan_safe()shared function that applies consent validation, safe mode enforcement, target validation, rate limiting, and concurrency limits. Bothstart_task()andrun_workflow_once()now use this entry point.workflows.py— Refactored_run_workflow()to apply all security checks directly: plugin existence validation, target validation (safe mode + timeout), network policy enforcement, rate limiting per client+plugin, concurrency slot acquisition, and source-tagged audit logging. Each check failure gracefully skips the step instead of crashing the scheduler loop.ratelimit.py— AddedWorkflowRateLimiterclass with two methods:check_workflow_rate_limit(workflow_id, min_interval)— prevents a single workflow from firing faster than the configured minimum intervalcheck_user_workflow_limit(user_id, max_workflows)— limits the number of workflows a single user can createexecutor.py— Addedsourceparameter tocreate_task()and included"source": "api" | "workflow" | "scheduler"in audit log context for all task lifecycle events.config.py— Added workflow configuration settings:max_workflows_per_user(default 50)workflow_min_interval_seconds(default 60)workflow_consent_refresh_days(default 30)Impact
sourcein audit logrequire_consentsetting