fix: nil pointer dereference in UpdateRunner when task not in pool#3727
fix: nil pointer dereference in UpdateRunner when task not in pool#3727
Conversation
GetTask now returns (nil, nil) when HA is disabled and a task is not in the local memory pool. The refactored UpdateRunner skipped the nil check for tsk, causing a panic on tsk.RunnerID access. Also fix StopTasksByTemplate logging the wrong error variable (err instead of taskErr), which always logged nil. Co-authored-by: Denis Gukov <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review
Result: No security issues found.
I reviewed both changes in this PR for injection risks, auth/authz bypasses, information disclosure, and DoS vectors:
-
Nil guard in
UpdateRunner(api/runners/runners.go): The addedtsk == nilcheck correctly prevents a nil pointer dereference (panic/crash) when a runner reports progress on a task no longer in the pool. The endpoint is protected byRunnerMiddleware(token-based auth), and the nil path logs server-side only — no sensitive data is returned to the caller. This is a net security improvement (prevents authenticated-runner-triggered DoS via panic). -
Logging fix in
StopTasksByTemplate(services/tasks/TaskPool.go): ReplacingerrwithtaskErris a correctness-only change with no security impact.
No new attack surface, no auth bypass, no information leakage introduced.
Sent by Cursor Automation: Find vulnerabilities
2 participants
Bug and Impact
Nil pointer dereference (server crash) in
UpdateRunner(api/runners/runners.go).When a remote runner reports progress for a task that is no longer in the local memory pool (e.g. it already finished and was removed),
GetTaskreturns(nil, nil). The code then dereferencestsk.RunnerIDwithout a nil check, causing a panic that crashes the HTTP handler and returns a 500 to the runner.Trigger scenario: Any non-HA deployment where a remote runner sends a progress update for a task that has already finished and been cleaned up from the in-memory task pool. This is a normal race condition in production — runners poll on a 1-second interval and the task can finish between polls.
Root Cause
The recent refactoring (commit
55422e02and related HA work) changedGetTaskfrom returning*TaskRunnerto(*TaskRunner, error). The old call sites inUpdateRunnerpreviously had an explicit nil-check-then-hydrate pattern. The refactored code correctly checkserr != nilbut removed thetsk == nilcheck.GetTaskonly hydrates from DB whenHAEnabled()is true, so on non-HA setupsGetTaskcan return(nil, nil).Fix
api/runners/runners.go: Added the missingtsk == nilguard after the error check, logging a warning and continuing to the next job (matching the prior behavior).services/tasks/TaskPool.go(secondary fix): InStopTasksByTemplate, the error log on line 631 usederr(which is alwaysnilat that point — it was checked earlier) instead oftaskErr. This masked actual errors during the stop-all-tasks-by-template operation.Validation
api/runners,services/tasks) compile cleanly.services/taskspass.