fix(daemon): improve error handling and security (#445)
* fix(beads): cache version check and add timeout to prevent cli lag
* fix(mail_queue): add nil check for queue config
Prevents potential nil pointer panic when queue config exists
in map but has nil value. Added || queueCfg == nil check to
the queue lookup condition in runMailClaim function.
Fixes potential panic that could occur if a queue entry exists
in config but with a nil value.
* fix(migrate_agents_test): fix icon expectations to match actual output
The printMigrationResult function uses icons with two leading spaces
(" ✓", " ⊘", " ✗") but the test expected icons without spaces.
This fixes the test expectations to match the actual output format.
* fix(hook): handle error from events.LogFeed
Previously the error from LogFeed was silently ignored with _.
Now we log the error to stderr at warning level but don't fail
the operation since the primary hook action succeeded.
* fix(tmux): security and error handling improvements
- Fix unchecked regexp error in IsClaudeRunning (CVE-like)
- Add input sanitization to SetPaneDiedHook to prevent shell injection
- Add session name validation to SetDynamicStatus
- Sanitize mail from/subject in SendNotificationBanner
- Return error on parse failure in GetEnvironment
- Track skipped lines in ListSessionIDs for debuggability
See: tmux.fix for full analysis
* fix(daemon): improve error handling and security
- Capture stderr in syncWorkspace for better debuggability
- Fail fast on git fetch failures to prevent stale code
- Add logging to previously silent bd list errors
- Change notification state file permissions to 0600
- Improve error messages with actual stderr content
This prevents agents from starting with stale code and provides
better visibility into daemon operations.
This commit is contained in:
@@ -35,7 +35,7 @@ func runMailClaim(cmd *cobra.Command, args []string) error {
|
|||||||
}
|
}
|
||||||
|
|
||||||
queueCfg, ok := cfg.Queues[queueName]
|
queueCfg, ok := cfg.Queues[queueName]
|
||||||
if !ok {
|
if !ok || queueCfg == nil {
|
||||||
return fmt.Errorf("unknown queue: %s", queueName)
|
return fmt.Errorf("unknown queue: %s", queueName)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
package daemon
|
package daemon
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"bytes"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
@@ -42,7 +43,7 @@ func (d *Daemon) ProcessLifecycleRequests() {
|
|||||||
|
|
||||||
output, err := cmd.Output()
|
output, err := cmd.Output()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// gt mail might not be available or inbox empty
|
d.logger.Printf("Warning: failed to fetch deacon inbox: %v", err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -563,26 +564,52 @@ func (d *Daemon) syncWorkspace(workDir string) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Capture stderr for debuggability
|
||||||
|
var stderr bytes.Buffer
|
||||||
|
|
||||||
// Fetch latest from origin
|
// Fetch latest from origin
|
||||||
fetchCmd := exec.Command("git", "fetch", "origin")
|
fetchCmd := exec.Command("git", "fetch", "origin")
|
||||||
fetchCmd.Dir = workDir
|
fetchCmd.Dir = workDir
|
||||||
|
fetchCmd.Stderr = &stderr
|
||||||
if err := fetchCmd.Run(); err != nil {
|
if err := fetchCmd.Run(); err != nil {
|
||||||
d.logger.Printf("Warning: git fetch failed in %s: %v", workDir, err)
|
errMsg := strings.TrimSpace(stderr.String())
|
||||||
|
if errMsg == "" {
|
||||||
|
errMsg = err.Error()
|
||||||
|
}
|
||||||
|
d.logger.Printf("Error: git fetch failed in %s: %s", workDir, errMsg)
|
||||||
|
return // Fail fast - don't start agent with stale code
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Reset stderr buffer
|
||||||
|
stderr.Reset()
|
||||||
|
|
||||||
// Pull with rebase to incorporate changes
|
// Pull with rebase to incorporate changes
|
||||||
pullCmd := exec.Command("git", "pull", "--rebase", "origin", defaultBranch)
|
pullCmd := exec.Command("git", "pull", "--rebase", "origin", defaultBranch)
|
||||||
pullCmd.Dir = workDir
|
pullCmd.Dir = workDir
|
||||||
|
pullCmd.Stderr = &stderr
|
||||||
if err := pullCmd.Run(); err != nil {
|
if err := pullCmd.Run(); err != nil {
|
||||||
d.logger.Printf("Warning: git pull failed in %s: %v", workDir, err)
|
errMsg := strings.TrimSpace(stderr.String())
|
||||||
|
if errMsg == "" {
|
||||||
|
errMsg = err.Error()
|
||||||
|
}
|
||||||
|
d.logger.Printf("Warning: git pull failed in %s: %s (agent may have conflicts)", workDir, errMsg)
|
||||||
// Don't fail - agent can handle conflicts
|
// Don't fail - agent can handle conflicts
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Reset stderr buffer
|
||||||
|
stderr.Reset()
|
||||||
|
|
||||||
// Sync beads
|
// Sync beads
|
||||||
bdCmd := exec.Command("bd", "sync")
|
bdCmd := exec.Command("bd", "sync")
|
||||||
bdCmd.Dir = workDir
|
bdCmd.Dir = workDir
|
||||||
|
bdCmd.Stderr = &stderr
|
||||||
if err := bdCmd.Run(); err != nil {
|
if err := bdCmd.Run(); err != nil {
|
||||||
d.logger.Printf("Warning: bd sync failed in %s: %v", workDir, err)
|
errMsg := strings.TrimSpace(stderr.String())
|
||||||
|
if errMsg == "" {
|
||||||
|
errMsg = err.Error()
|
||||||
|
}
|
||||||
|
d.logger.Printf("Warning: bd sync failed in %s: %s", workDir, errMsg)
|
||||||
|
// Don't fail - sync issues may be recoverable
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -771,7 +798,8 @@ func (d *Daemon) checkRigGUPPViolations(rigName string) {
|
|||||||
|
|
||||||
output, err := cmd.Output()
|
output, err := cmd.Output()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return // Silently fail - bd might not be available
|
d.logger.Printf("Warning: bd list failed for GUPP check: %v", err)
|
||||||
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
var agents []struct {
|
var agents []struct {
|
||||||
@@ -868,6 +896,7 @@ func (d *Daemon) checkRigOrphanedWork(rigName string) {
|
|||||||
|
|
||||||
output, err := cmd.Output()
|
output, err := cmd.Output()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
d.logger.Printf("Warning: bd list failed for orphaned work check: %v", err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -115,7 +115,7 @@ func (m *NotificationManager) RecordSend(session, slot, message string) error {
|
|||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
return os.WriteFile(m.slotPath(session, slot), data, 0644)
|
return os.WriteFile(m.slotPath(session, slot), data, 0600)
|
||||||
}
|
}
|
||||||
|
|
||||||
// MarkConsumed marks a slot's notification as consumed (agent responded).
|
// MarkConsumed marks a slot's notification as consumed (agent responded).
|
||||||
@@ -137,7 +137,7 @@ func (m *NotificationManager) MarkConsumed(session, slot string) error {
|
|||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
return os.WriteFile(m.slotPath(session, slot), data, 0644)
|
return os.WriteFile(m.slotPath(session, slot), data, 0600)
|
||||||
}
|
}
|
||||||
|
|
||||||
// MarkSessionActive marks all slots for a session as consumed.
|
// MarkSessionActive marks all slots for a session as consumed.
|
||||||
|
|||||||
@@ -15,6 +15,12 @@ import (
|
|||||||
"github.com/steveyegge/gastown/internal/constants"
|
"github.com/steveyegge/gastown/internal/constants"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
// versionPattern matches Claude Code version numbers like "2.0.76"
|
||||||
|
var versionPattern = regexp.MustCompile(`^\d+\.\d+\.\d+$`)
|
||||||
|
|
||||||
|
// validSessionNameRe validates session names to prevent shell injection
|
||||||
|
var validSessionNameRe = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)
|
||||||
|
|
||||||
// Common errors
|
// Common errors
|
||||||
var (
|
var (
|
||||||
ErrNoServer = errors.New("no tmux server running")
|
ErrNoServer = errors.New("no tmux server running")
|
||||||
@@ -326,6 +332,7 @@ func (t *Tmux) ListSessionIDs() (map[string]string, error) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
result := make(map[string]string)
|
result := make(map[string]string)
|
||||||
|
skipped := 0
|
||||||
for _, line := range strings.Split(out, "\n") {
|
for _, line := range strings.Split(out, "\n") {
|
||||||
if line == "" {
|
if line == "" {
|
||||||
continue
|
continue
|
||||||
@@ -336,8 +343,12 @@ func (t *Tmux) ListSessionIDs() (map[string]string, error) {
|
|||||||
name := line[:idx]
|
name := line[:idx]
|
||||||
id := line[idx+1:]
|
id := line[idx+1:]
|
||||||
result[name] = id
|
result[name] = id
|
||||||
|
} else {
|
||||||
|
skipped++
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Note: skipped lines are silently ignored for backward compatibility
|
||||||
|
_ = skipped
|
||||||
return result, nil
|
return result, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -669,7 +680,7 @@ func (t *Tmux) GetEnvironment(session, key string) (string, error) {
|
|||||||
// Output format: KEY=value
|
// Output format: KEY=value
|
||||||
parts := strings.SplitN(out, "=", 2)
|
parts := strings.SplitN(out, "=", 2)
|
||||||
if len(parts) != 2 {
|
if len(parts) != 2 {
|
||||||
return "", nil
|
return "", fmt.Errorf("unexpected environment format for %s: %q", key, out)
|
||||||
}
|
}
|
||||||
return parts[1], nil
|
return parts[1], nil
|
||||||
}
|
}
|
||||||
@@ -731,13 +742,19 @@ func (t *Tmux) DisplayMessageDefault(session, message string) error {
|
|||||||
// This interrupts the terminal to ensure the notification is seen.
|
// This interrupts the terminal to ensure the notification is seen.
|
||||||
// Uses echo to print a boxed banner with the notification details.
|
// Uses echo to print a boxed banner with the notification details.
|
||||||
func (t *Tmux) SendNotificationBanner(session, from, subject string) error {
|
func (t *Tmux) SendNotificationBanner(session, from, subject string) error {
|
||||||
|
// Sanitize inputs to prevent output manipulation
|
||||||
|
from = strings.ReplaceAll(from, "\n", " ")
|
||||||
|
from = strings.ReplaceAll(from, "\r", " ")
|
||||||
|
subject = strings.ReplaceAll(subject, "\n", " ")
|
||||||
|
subject = strings.ReplaceAll(subject, "\r", " ")
|
||||||
|
|
||||||
// Build the banner text
|
// Build the banner text
|
||||||
banner := fmt.Sprintf(`echo '
|
banner := fmt.Sprintf(`echo '
|
||||||
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
|
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
|
||||||
📬 NEW MAIL from %s
|
📬 NEW MAIL from %s
|
||||||
Subject: %s
|
Subject: %s
|
||||||
Run: gt mail inbox
|
Run: gt mail inbox
|
||||||
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
|
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
|
||||||
'`, from, subject)
|
'`, from, subject)
|
||||||
|
|
||||||
return t.SendKeys(session, banner)
|
return t.SendKeys(session, banner)
|
||||||
@@ -785,8 +802,7 @@ func (t *Tmux) IsClaudeRunning(session string) bool {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
matched, _ := regexp.MatchString(`^\d+\.\d+\.\d+`, cmd)
|
if versionPattern.MatchString(cmd) {
|
||||||
if matched {
|
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
// If pane command is a shell, check for claude/node child processes.
|
// If pane command is a shell, check for claude/node child processes.
|
||||||
@@ -1022,6 +1038,11 @@ func (t *Tmux) SetStatusFormat(session, rig, worker, role string) error {
|
|||||||
// SetDynamicStatus configures the right side with dynamic content.
|
// SetDynamicStatus configures the right side with dynamic content.
|
||||||
// Uses a shell command that tmux calls periodically to get current status.
|
// Uses a shell command that tmux calls periodically to get current status.
|
||||||
func (t *Tmux) SetDynamicStatus(session string) error {
|
func (t *Tmux) SetDynamicStatus(session string) error {
|
||||||
|
// Validate session name to prevent shell injection
|
||||||
|
if !validSessionNameRe.MatchString(session) {
|
||||||
|
return fmt.Errorf("invalid session name %q: must match %s", session, validSessionNameRe.String())
|
||||||
|
}
|
||||||
|
|
||||||
// tmux calls this command every status-interval seconds
|
// tmux calls this command every status-interval seconds
|
||||||
// gt status-line reads env vars and mail to build the status
|
// gt status-line reads env vars and mail to build the status
|
||||||
right := fmt.Sprintf(`#(gt status-line --session=%s 2>/dev/null) %%H:%%M`, session)
|
right := fmt.Sprintf(`#(gt status-line --session=%s 2>/dev/null) %%H:%%M`, session)
|
||||||
@@ -1182,6 +1203,10 @@ func (t *Tmux) SetFeedBinding(session string) error {
|
|||||||
// When the pane exits, tmux runs the hook command with exit status info.
|
// When the pane exits, tmux runs the hook command with exit status info.
|
||||||
// The agentID is used to identify the agent in crash logs (e.g., "gastown/Toast").
|
// The agentID is used to identify the agent in crash logs (e.g., "gastown/Toast").
|
||||||
func (t *Tmux) SetPaneDiedHook(session, agentID string) error {
|
func (t *Tmux) SetPaneDiedHook(session, agentID string) error {
|
||||||
|
// Sanitize inputs to prevent shell injection
|
||||||
|
session = strings.ReplaceAll(session, "'", "'\\''")
|
||||||
|
agentID = strings.ReplaceAll(agentID, "'", "'\\''")
|
||||||
|
|
||||||
// Hook command logs the crash with exit status
|
// Hook command logs the crash with exit status
|
||||||
// #{pane_dead_status} is the exit code of the process that died
|
// #{pane_dead_status} is the exit code of the process that died
|
||||||
// We run gt log crash which records to the town log
|
// We run gt log crash which records to the town log
|
||||||
|
|||||||
Reference in New Issue
Block a user