feat(cron): restrict list/get/update to accessible jobs per channel
This commit is contained in:
parent
1d8ef7dcfb
commit
be13201f02
3 changed files with 163 additions and 6 deletions
|
|
@ -31,15 +31,20 @@ picoclaw cron add --name "Ping" --message "heartbeat" --every 300 --deliver
|
||||||
The agent-facing `cron` tool supports these actions:
|
The agent-facing `cron` tool supports these actions:
|
||||||
|
|
||||||
- `add`: create a new job.
|
- `add`: create a new job.
|
||||||
- `list`: show job names, ids, and schedules.
|
- `list`: show accessible job names, ids, and schedules.
|
||||||
- `get`: fetch one full persisted job by `job_id`, including its saved payload.
|
- `get`: fetch one accessible persisted job by `job_id`, including its saved payload.
|
||||||
- `update`: partially update one job by `job_id`; omitted fields are preserved.
|
- `update`: partially update one accessible job by `job_id`; omitted fields are preserved.
|
||||||
- `remove`, `enable`, `disable`: existing management actions.
|
- `remove`, `enable`, `disable`: existing management actions.
|
||||||
|
|
||||||
When rescheduling an existing task, use `list -> get -> update`. Do not use
|
When rescheduling an existing task, use `list -> get -> update`. Do not use
|
||||||
`remove -> add` just to change the schedule, because recreating a job can drop
|
`remove -> add` just to change the schedule, because recreating a job can drop
|
||||||
the original prompt, delivery target, or command payload.
|
the original prompt, delivery target, or command payload.
|
||||||
|
|
||||||
|
Remote channel access is scoped to the current `channel/chat_id`: remote callers
|
||||||
|
can only list, get, or update jobs whose saved `payload.channel` and `payload.to`
|
||||||
|
match the current conversation. Command jobs include a shell command payload, so
|
||||||
|
they can only be listed, inspected, or updated from internal channels.
|
||||||
|
|
||||||
Example tool calls:
|
Example tool calls:
|
||||||
|
|
||||||
```json
|
```json
|
||||||
|
|
|
||||||
|
|
@ -92,7 +92,7 @@ func (t *CronTool) Parameters() map[string]any {
|
||||||
"action": map[string]any{
|
"action": map[string]any{
|
||||||
"type": "string",
|
"type": "string",
|
||||||
"enum": []string{"add", "list", "get", "update", "remove", "enable", "disable"},
|
"enum": []string{"add", "list", "get", "update", "remove", "enable", "disable"},
|
||||||
"description": "Action to perform. Use 'get' before editing and 'update' to change existing jobs without losing their payload.",
|
"description": "Action to perform. Use 'get' before editing and 'update' to change existing jobs without losing their payload. Remote channels can only list/get/update jobs for the current channel/chat_id.",
|
||||||
},
|
},
|
||||||
"name": map[string]any{
|
"name": map[string]any{
|
||||||
"type": "string",
|
"type": "string",
|
||||||
|
|
@ -142,7 +142,7 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *ToolResult
|
||||||
case "add":
|
case "add":
|
||||||
return t.addJob(ctx, args)
|
return t.addJob(ctx, args)
|
||||||
case "list":
|
case "list":
|
||||||
return t.listJobs()
|
return t.listJobs(ctx)
|
||||||
case "get":
|
case "get":
|
||||||
return t.getJob(ctx, args)
|
return t.getJob(ctx, args)
|
||||||
case "update":
|
case "update":
|
||||||
|
|
@ -250,9 +250,17 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult
|
||||||
return SilentResult(fmt.Sprintf("Cron job added: %s (id: %s)", job.Name, job.ID))
|
return SilentResult(fmt.Sprintf("Cron job added: %s (id: %s)", job.Name, job.ID))
|
||||||
}
|
}
|
||||||
|
|
||||||
func (t *CronTool) listJobs() *ToolResult {
|
func (t *CronTool) listJobs(ctx context.Context) *ToolResult {
|
||||||
jobs := t.cronService.ListJobs(false)
|
jobs := t.cronService.ListJobs(false)
|
||||||
|
|
||||||
|
var accessibleJobs []cron.CronJob
|
||||||
|
for _, job := range jobs {
|
||||||
|
if t.canAccessJob(ctx, &job) {
|
||||||
|
accessibleJobs = append(accessibleJobs, job)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
jobs = accessibleJobs
|
||||||
|
|
||||||
if len(jobs) == 0 {
|
if len(jobs) == 0 {
|
||||||
return SilentResult("No scheduled jobs")
|
return SilentResult("No scheduled jobs")
|
||||||
}
|
}
|
||||||
|
|
@ -286,6 +294,9 @@ func (t *CronTool) getJob(ctx context.Context, args map[string]any) *ToolResult
|
||||||
if !ok {
|
if !ok {
|
||||||
return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
|
return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
|
||||||
}
|
}
|
||||||
|
if !t.canAccessJob(ctx, job) {
|
||||||
|
return ErrorResult(fmt.Sprintf("Job %s is not accessible from this channel", jobID))
|
||||||
|
}
|
||||||
|
|
||||||
return SilentResult(formatCronJobJSON(job))
|
return SilentResult(formatCronJobJSON(job))
|
||||||
}
|
}
|
||||||
|
|
@ -300,6 +311,9 @@ func (t *CronTool) updateJob(ctx context.Context, args map[string]any) *ToolResu
|
||||||
if !ok {
|
if !ok {
|
||||||
return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
|
return ErrorResult(fmt.Sprintf("Job %s not found", jobID))
|
||||||
}
|
}
|
||||||
|
if !t.canAccessJob(ctx, job) {
|
||||||
|
return ErrorResult(fmt.Sprintf("Job %s is not accessible from this channel", jobID))
|
||||||
|
}
|
||||||
|
|
||||||
patches := 0
|
patches := 0
|
||||||
|
|
||||||
|
|
@ -476,6 +490,21 @@ func (t *CronTool) validateCommandMutation(ctx context.Context, args map[string]
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (t *CronTool) canAccessJob(ctx context.Context, job *cron.CronJob) bool {
|
||||||
|
channel := ToolChannel(ctx)
|
||||||
|
if constants.IsInternalChannel(channel) {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
chatID := ToolChatID(ctx)
|
||||||
|
if channel == "" || chatID == "" {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
if job.Payload.Command != "" {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
return job.Payload.Channel == channel && job.Payload.To == chatID
|
||||||
|
}
|
||||||
|
|
||||||
func formatCronJobJSON(job *cron.CronJob) string {
|
func formatCronJobJSON(job *cron.CronJob) string {
|
||||||
data, err := json.Marshal(job)
|
data, err := json.Marshal(job)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
|
||||||
|
|
@ -421,6 +421,71 @@ func TestCronTool_UpdateValidationErrors(t *testing.T) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestCronTool_ListFiltersJobsForRemoteChannel(t *testing.T) {
|
||||||
|
tool := newTestCronTool(t)
|
||||||
|
ctx := WithToolContext(context.Background(), "telegram", "chat-1")
|
||||||
|
everyMS := int64(60_000)
|
||||||
|
|
||||||
|
ownJob, err := tool.cronService.AddJob(
|
||||||
|
"own",
|
||||||
|
cron.CronSchedule{Kind: "every", EveryMS: &everyMS},
|
||||||
|
"visible",
|
||||||
|
"telegram",
|
||||||
|
"chat-1",
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("AddJob() error: %v", err)
|
||||||
|
}
|
||||||
|
otherChatJob, err := tool.cronService.AddJob(
|
||||||
|
"other-chat",
|
||||||
|
cron.CronSchedule{Kind: "every", EveryMS: &everyMS},
|
||||||
|
"hidden",
|
||||||
|
"telegram",
|
||||||
|
"chat-2",
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("AddJob() error: %v", err)
|
||||||
|
}
|
||||||
|
otherChannelJob, err := tool.cronService.AddJob(
|
||||||
|
"other-channel",
|
||||||
|
cron.CronSchedule{Kind: "every", EveryMS: &everyMS},
|
||||||
|
"hidden",
|
||||||
|
"feishu",
|
||||||
|
"chat-1",
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("AddJob() error: %v", err)
|
||||||
|
}
|
||||||
|
commandJob, err := tool.cronService.AddJob(
|
||||||
|
"command",
|
||||||
|
cron.CronSchedule{Kind: "every", EveryMS: &everyMS},
|
||||||
|
"hidden command",
|
||||||
|
"telegram",
|
||||||
|
"chat-1",
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("AddJob() error: %v", err)
|
||||||
|
}
|
||||||
|
commandJob.Payload.Command = "df -h"
|
||||||
|
if err := tool.cronService.UpdateJob(commandJob); err != nil {
|
||||||
|
t.Fatalf("UpdateJob() error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
result := tool.Execute(ctx, map[string]any{"action": "list"})
|
||||||
|
|
||||||
|
if result.IsError {
|
||||||
|
t.Fatalf("list failed: %s", result.ForLLM)
|
||||||
|
}
|
||||||
|
if !strings.Contains(result.ForLLM, ownJob.ID) {
|
||||||
|
t.Fatalf("list should include own job %s, got: %s", ownJob.ID, result.ForLLM)
|
||||||
|
}
|
||||||
|
for _, hiddenID := range []string{otherChatJob.ID, otherChannelJob.ID, commandJob.ID} {
|
||||||
|
if strings.Contains(result.ForLLM, hiddenID) {
|
||||||
|
t.Fatalf("list should not include hidden job %s, got: %s", hiddenID, result.ForLLM)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestCronTool_RemoteCannotAccessOtherChatJob(t *testing.T) {
|
func TestCronTool_RemoteCannotAccessOtherChatJob(t *testing.T) {
|
||||||
tool := newTestCronTool(t)
|
tool := newTestCronTool(t)
|
||||||
job, err := tool.cronService.AddJob(
|
job, err := tool.cronService.AddJob(
|
||||||
|
|
@ -444,6 +509,13 @@ func TestCronTool_RemoteCannotAccessOtherChatJob(t *testing.T) {
|
||||||
if !updateResult.IsError || !strings.Contains(updateResult.ForLLM, "not accessible") {
|
if !updateResult.IsError || !strings.Contains(updateResult.ForLLM, "not accessible") {
|
||||||
t.Fatalf("expected inaccessible update, got: %+v", updateResult)
|
t.Fatalf("expected inaccessible update, got: %+v", updateResult)
|
||||||
}
|
}
|
||||||
|
unchanged, ok := tool.cronService.GetJob(job.ID)
|
||||||
|
if !ok {
|
||||||
|
t.Fatal("job should still exist")
|
||||||
|
}
|
||||||
|
if unchanged.Payload.Message != "secret" {
|
||||||
|
t.Fatalf("unauthorized update mutated job: %+v", unchanged.Payload)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestCronTool_RemoteCannotAccessCommandJob(t *testing.T) {
|
func TestCronTool_RemoteCannotAccessCommandJob(t *testing.T) {
|
||||||
|
|
@ -473,6 +545,13 @@ func TestCronTool_RemoteCannotAccessCommandJob(t *testing.T) {
|
||||||
if !updateResult.IsError || !strings.Contains(updateResult.ForLLM, "not accessible") {
|
if !updateResult.IsError || !strings.Contains(updateResult.ForLLM, "not accessible") {
|
||||||
t.Fatalf("expected inaccessible update, got: %+v", updateResult)
|
t.Fatalf("expected inaccessible update, got: %+v", updateResult)
|
||||||
}
|
}
|
||||||
|
unchanged, ok := tool.cronService.GetJob(job.ID)
|
||||||
|
if !ok {
|
||||||
|
t.Fatal("job should still exist")
|
||||||
|
}
|
||||||
|
if unchanged.Payload.Message != "run command" || unchanged.Payload.Command != "df -h" {
|
||||||
|
t.Fatalf("unauthorized update mutated command job: %+v", unchanged.Payload)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestCronTool_CommandUpdateSafetyGates(t *testing.T) {
|
func TestCronTool_CommandUpdateSafetyGates(t *testing.T) {
|
||||||
|
|
@ -562,6 +641,50 @@ func TestCronTool_CommandUpdateSafetyGates(t *testing.T) {
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestCronTool_InternalCanAccessCommandJobFromAnyChannel(t *testing.T) {
|
||||||
|
tool := newTestCronTool(t)
|
||||||
|
ctx := WithToolContext(context.Background(), "cli", "direct")
|
||||||
|
job, err := tool.cronService.AddJob(
|
||||||
|
"command",
|
||||||
|
cron.CronSchedule{Kind: "cron", Expr: "0 8 * * *"},
|
||||||
|
"run command",
|
||||||
|
"telegram",
|
||||||
|
"chat-1",
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("AddJob() error: %v", err)
|
||||||
|
}
|
||||||
|
job.Payload.Command = "df -h"
|
||||||
|
if err := tool.cronService.UpdateJob(job); err != nil {
|
||||||
|
t.Fatalf("UpdateJob() error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
getResult := tool.Execute(ctx, map[string]any{"action": "get", "job_id": job.ID})
|
||||||
|
if getResult.IsError {
|
||||||
|
t.Fatalf("get failed: %s", getResult.ForLLM)
|
||||||
|
}
|
||||||
|
got := parseCronJobResult(t, getResult)
|
||||||
|
if got.Payload.Command != "df -h" || got.Payload.Channel != "telegram" || got.Payload.To != "chat-1" {
|
||||||
|
t.Fatalf("get returned wrong command job: %+v", got.Payload)
|
||||||
|
}
|
||||||
|
|
||||||
|
updateResult := tool.Execute(ctx, map[string]any{
|
||||||
|
"action": "update",
|
||||||
|
"job_id": job.ID,
|
||||||
|
"cron_expr": "30 10 * * *",
|
||||||
|
})
|
||||||
|
if updateResult.IsError {
|
||||||
|
t.Fatalf("update failed: %s", updateResult.ForLLM)
|
||||||
|
}
|
||||||
|
updated, _ := tool.cronService.GetJob(job.ID)
|
||||||
|
if updated.Payload.Command != "df -h" {
|
||||||
|
t.Fatalf("command should be preserved: %+v", updated.Payload)
|
||||||
|
}
|
||||||
|
if updated.Schedule.Kind != "cron" || updated.Schedule.Expr != "30 10 * * *" {
|
||||||
|
t.Fatalf("schedule not updated: %+v", updated.Schedule)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestCronTool_ExecuteJobPublishesErrorWhenExecDisabled(t *testing.T) {
|
func TestCronTool_ExecuteJobPublishesErrorWhenExecDisabled(t *testing.T) {
|
||||||
cfg := config.DefaultConfig()
|
cfg := config.DefaultConfig()
|
||||||
cfg.Tools.Exec.Enabled = false
|
cfg.Tools.Exec.Enabled = false
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue