Tasks toolset

[rumpl]

With priorities and dependencies

[docker-agent]

Security & Error Handling Review

Found 2 HIGH severity security issues and 1 MEDIUM severity error handling issue in the new tasks toolset:

Critical Issues

1. Path traversal vulnerability - User-provided file paths in task descriptions are not validated, allowing arbitrary file reads
2. Nil pointer dereference risk - Missing nil check for runConfig could cause panics

Additional Issue

3. Inconsistent error handling - JSON marshaling errors are silently ignored in multiple locations

Please address the HIGH severity issues before merging.

[rumpl]

/review

[docker-agent]

Code Review Summary

Overall Assessment: I found 2 potential issues in the changed code that should be addressed.

Issues Found

1. MEDIUM - Silent JSON error handling could cause data loss (tasks.go:130)

   • The load() function silently returns empty store on JSON errors
   • Corrupted task files would cause all tasks to disappear without warning

2. MEDIUM - Insufficient path validation for absolute paths (registry.go:91)

   • When absolute paths are used, validation doesn't restrict to safe directories
   • Users could specify arbitrary filesystem paths like /etc/passwd

Positive Notes

✅ Good mutex protection throughout the task management code
✅ Proper cycle detection implementation
✅ Dependencies are cleaned up when tasks are deleted
✅ Test coverage looks comprehensive

The core functionality is solid, but the two issues above should be addressed to prevent data loss and improve security.

[docker-agent]

MEDIUM: Silent JSON error handling could cause data loss

The load() function silently returns an empty task store when JSON unmarshaling fails (line 133). If the JSON file becomes corrupted, all tasks will disappear without any warning to the user. This could lead to catastrophic data loss.

Recommendation:
Consider one of these approaches:

1. Log the error with appropriate severity
2. Back up the corrupted file before returning empty store
3. Return an error to the caller instead of silently failing
4. Add a comment explaining why silent failure is acceptable (if there's a specific reason)

if err := json.Unmarshal(data, &store); err != nil {
    // Option 1: Log the error
    log.Printf("Warning: corrupted tasks file, returning empty store: %v", err)
    return taskStore{Tasks: make(map[string]Task)}
}

[docker-agent]

MEDIUM: Insufficient path validation for absolute paths

When toolsetPath is absolute (line 95), basePath is set to empty string (line 96), and then ValidatePathInDirectory is called with this empty basePath (line 99).

Looking at the validation logic, when allowedDir is empty and the path is absolute, the validation only checks for .. sequences but otherwise accepts ANY absolute path. This allows users to specify arbitrary filesystem paths like /etc/passwd or /root/.ssh/id_rsa as the tasks storage file, which is a security concern.

Recommendation:
Consider restricting absolute paths to a known safe directory, or document why arbitrary absolute paths are acceptable in this context. For example:

if filepath.IsAbs(toolsetPath) {
    // Option 1: Reject absolute paths
    return nil, fmt.Errorf("absolute paths not allowed for tasks storage")
    
    // Option 2: Restrict to a safe base directory
    basePath = "/var/lib/cagent" // or some configured safe directory
}