harden: validate ToolUseID and SubagentID centrally in DispatchLifecycleEvent

98f8e37→main·

Soph·1mo ago·2 files·+30 added/-6 removed

handleLifecycleSubagentEnd builds a subagent transcript path from event.SubagentID (AgentTranscriptPath -> filepath.Join), then stats and reads it via ExtractModifiedFilesFromOffset — before the value is validated. A "../"-laden SubagentID escapes transcriptDir for a file read. The task-checkpoint *write* paths (CapturePreTaskState, WriteTemporaryTask) already validate ToolUseID/AgentID, but this read ran first and unguarded.

Extend the central dispatcher guard (added for SessionID) to also reject path-unsafe event.ToolUseID (ValidateToolUseID) and event.SubagentID (ValidateAgentID) before routing, so every handler that turns these hook-supplied identifiers into a path is covered at one choke point. Empty values pass through to each handler's own empty-handling.

Taint is local hook input (same-privilege) and the sink is a read, so severity is low; this closes the last identifier in the lifecycle path family.

Sessions

92ddb0963aa9View transcript

Changes

2

• cmd/entire/cli

• Mlifecycle.go+17/-6

• Mlifecycle_test.go+13

52 unmodified lines

return errors.New("event cannot be nil")
}

// Reject path-unsafe session IDs once, here, before any handler uses the ID
// to build filesystem paths. Handlers historically validated individually,
// Reject path-unsafe identifiers once, here, before any handler uses them to
// build filesystem paths. Handlers historically validated individually,
// which is fragile — handleLifecycleTurnEnd builds .entire/metadata/<id>/
// via os.MkdirAll + os.WriteFile without its own check. Centralizing the
// guard covers every handler (and any future one) uniformly. Empty IDs pass
// through: handlers apply their own empty-handling (e.g. TurnEnd falls back
to a safe constant).
// via os.MkdirAll + os.WriteFile, and handleLifecycleSubagentEnd builds a
// subagent transcript path from SubagentID and reads it, without their own
// checks. Centralizing the guard covers every handler (and any future one)
// uniformly. Empty IDs pass through: handlers apply their own empty-handling
// (e.g. TurnEnd falls back to a safe constant; SubagentEnd skips the path).
if event.SessionID != "" {
    if err := validation.ValidateSessionID(event.SessionID); err != nil {
        return fmt.Errorf("invalid session ID in %s event: %w", event.Type, err)
    }
}
if event.ToolUseID != "" {
    if err := validation.ValidateToolUseID(event.ToolUseID); err != nil {
        return fmt.Errorf("invalid tool use ID in %s event: %w", event.Type, err)
    }
}
if event.SubagentID != "" {
    if err := validation.ValidateAgentID(event.SubagentID); err != nil {
        return fmt.Errorf("invalid subagent ID in %s event: %w", event.Type, err)
    }
}

// Filter forwarded hooks: when Cursor IDE forwards events to both
// .cursor/hooks.json and .claude/settings.json, only the agent that owns

Mcmd/entire/cli/lifecycle.go+17/-6

372 unmodified lines

t.Errorf("%v event: error = %q, want \"invalid session ID\"", evType, err)
}
}

// ToolUseID and SubagentID also build filesystem paths (task metadata dir,
// subagent transcript path) and must be rejected too.
if err := DispatchLifecycleEvent(context.Background(), ag, &agent.Event{
    Type: agent.SubagentEnd, SessionID: "ok-session", ToolUseID: "../../evil", SessionRef: "/dev/null",
}); err == nil || !strings.Contains(err.Error(), "invalid tool use ID") {
    t.Errorf("traversal tool use ID: error = %v, want \"invalid tool use ID\"", err)
}
if err := DispatchLifecycleEvent(context.Background(), ag, &agent.Event{
    Type: agent.SubagentEnd, SessionID: "ok-session", SubagentID: "../../evil", SessionRef: "/dev/null",
}); err == nil || !strings.Contains(err.Error(), "invalid subagent ID") {
    t.Errorf("traversal subagent ID: error = %v, want \"invalid subagent ID\"", err)
}
}

// --- handleLifecycleSessionStart tests ---

Mcmd/entire/cli/lifecycle_test.go+13