From 998926145a1459578dd3369067fd48d5160b61d4 Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 14 Sep 2026 08:31:32 +0000 Subject: [PATCH 1/2] fix: verify checklist ownership before updating or deleting an item updateItem and deleteItem checked that the task belongs to the caller's project, but fetched/mutated the target item by bare id without also verifying it belongs to the checklist named in the URL. A caller with write access to their own task could hijack and reparent an arbitrary item from a checklist belonging to a different project by guessing its UUID. Co-Authored-By: Claude Sonnet 5 --- backend/items.go | 20 ++++++-- backend/plugin_test.go | 114 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 5 deletions(-) diff --git a/backend/items.go b/backend/items.go index bbe70ec..151cfc4 100644 --- a/backend/items.go +++ b/backend/items.go @@ -116,6 +116,9 @@ func (p *checklistPlugin) updateItem(req *plugin.Request, res *plugin.Response) if !p.taskBelongsToProject(taskID, projectID, res) { return } + if !p.checklistOwnedByTask(checklistID, taskID, res) { + return + } type updateItemBody struct { Title *string `json:"title"` @@ -133,11 +136,14 @@ func (p *checklistPlugin) updateItem(req *plugin.Request, res *plugin.Response) return } - // Fetch current state so we can preserve unpatched fields. + // Fetch current state so we can preserve unpatched fields. Scoped by + // checklist_id too (not just id) so an itemID belonging to a different + // checklist/task/project 404s here instead of being adopted into this + // checklist by the re-INSERT below. cur, curErr := p.db.Query( `SELECT id, checklist_id, title, is_checked, assignee_id, position, created_by, created_at, updated_at - FROM task_checklist_items WHERE id = $1`, - itemID, + FROM task_checklist_items WHERE id = $1 AND checklist_id = $2`, + itemID, checklistID, ) if curErr != nil { p.log.Error("updateItem fetch: " + curErr.Error()) @@ -169,9 +175,10 @@ func (p *checklistPlugin) updateItem(req *plugin.Request, res *plugin.Response) } // Simulate UPDATE as DELETE + re-INSERT. task_checklist_items has no child - // FK references, so this is safe in both tests and production. + // FK references, so this is safe in both tests and production. Scoped by + // checklist_id as defense-in-depth, matching the fetch above. if _, err = p.db.Exec( - `DELETE FROM task_checklist_items WHERE id = $1`, itemID, + `DELETE FROM task_checklist_items WHERE id = $1 AND checklist_id = $2`, itemID, checklistID, ); err != nil { p.log.Error("updateItem delete: " + err.Error()) res.Error(500, "failed to update item") @@ -237,6 +244,9 @@ func (p *checklistPlugin) deleteItem(req *plugin.Request, res *plugin.Response) if !p.taskBelongsToProject(taskID, projectID, res) { return } + if !p.checklistOwnedByTask(checklistID, taskID, res) { + return + } // Fetch item title before deletion for activity record. titleResult, err := p.db.Query( diff --git a/backend/plugin_test.go b/backend/plugin_test.go index 10a0e9d..c06a60d 100644 --- a/backend/plugin_test.go +++ b/backend/plugin_test.go @@ -425,3 +425,117 @@ func TestListChecklists_UnknownTask(t *testing.T) { t.Fatalf("expected 404, got %d", res.StatusCode) } } + +// ── Cross-project ownership guards ─────────────────────────────────────────── + +const ( + otherProjectID = "project-2" + otherTaskID = "task-2" +) + +// otherProjectCallerReq mints a foreign checklist/item under otherTaskID, +// which genuinely belongs to otherProjectID — used to set up the victim +// resource for the cross-project tests below. +func otherProjectCallerReq() plugintest.Request { + return plugintest.Request{ + Caller: plugin.CallerIdentity{ + ProjectID: otherProjectID, + CallerID: "member-2", + CallerRole: "PROJECT_MEMBER", + }, + PathParams: map[string]string{}, + } +} + +// setupForeignItem re-seeds the tasks table with both the default +// project-1/task-1 pair and a second, genuinely unrelated project-2/task-2 +// pair, then creates a real checklist + item under task-2 as a project-2 +// caller. Returns the foreign checklist and item IDs. +func setupForeignItem(t *testing.T, tc *plugintest.Context) (foreignChecklistID, foreignItemID string) { + t.Helper() + tc.DB.SeedRows("tasks", []string{"id", "project_id", "deleted_at"}, [][]any{ + {testTaskID, testProjectID, nil}, + {otherTaskID, otherProjectID, nil}, + }) + + clRes := tc.Call("POST", "/tasks/:taskId/checklists", + withPathParams(otherProjectCallerReq(), map[string]string{"taskId": otherTaskID}). + WithJSONBody(map[string]string{"title": "Someone else's checklist"})) + var clEnv struct { + Data checklist `json:"data"` + } + if err := json.Unmarshal(clRes.Body, &clEnv); err != nil { + t.Fatalf("failed to create foreign checklist: %s", clRes.BodyString()) + } + + itemRes := tc.Call("POST", "/tasks/:taskId/checklists/:checklistId/items", + withPathParams(otherProjectCallerReq(), map[string]string{ + "taskId": otherTaskID, + "checklistId": clEnv.Data.ID, + }).WithJSONBody(map[string]string{"title": "Someone else's item"})) + var itemEnv struct { + Data checklistItem `json:"data"` + } + if err := json.Unmarshal(itemRes.Body, &itemEnv); err != nil { + t.Fatalf("failed to create foreign item: %s", itemRes.BodyString()) + } + return clEnv.Data.ID, itemEnv.Data.ID +} + +// TestUpdateItem_CrossProjectChecklistRejected pins the fix for a real IDOR: +// updateItem previously fetched/deleted/re-inserted the target item by bare +// id, with no check that the checklist in the URL actually owns it — a +// caller with tasks.write on their own project (testTaskID/testProjectID, +// which legitimately passes taskBelongsToProject) could hijack and +// reparent an arbitrary item from a checklist belonging to a completely +// different project, just by knowing its UUID. +func TestUpdateItem_CrossProjectChecklistRejected(t *testing.T) { + tc := setupPlugin(t) + foreignChecklistID, foreignItemID := setupForeignItem(t, tc) + + res := tc.Call("PATCH", "/tasks/:taskId/checklists/:checklistId/items/:itemId", + withPathParams(callerReq(), map[string]string{ + "taskId": testTaskID, // caller's own, legitimate task + "checklistId": foreignChecklistID, + "itemId": foreignItemID, + }).WithJSONBody(map[string]any{"title": "hijacked"})) + if res.StatusCode != 404 { + t.Fatalf("expected 404 (checklist belongs to a different task/project), got %d: %s", res.StatusCode, res.BodyString()) + } + + // The foreign item must be completely untouched. + rows := tc.DB.AllRows("task_checklist_items") + for _, row := range rows { + if row[0] == foreignItemID && row[2] == "hijacked" { + t.Fatal("foreign item was modified despite the 404") + } + } +} + +// TestDeleteItem_CrossProjectChecklistRejected mirrors the update case for +// delete: deleteItem checked taskBelongsToProject but never verified the +// URL's checklistId actually belongs to that task. +func TestDeleteItem_CrossProjectChecklistRejected(t *testing.T) { + tc := setupPlugin(t) + foreignChecklistID, foreignItemID := setupForeignItem(t, tc) + + res := tc.Call("DELETE", "/tasks/:taskId/checklists/:checklistId/items/:itemId", + withPathParams(callerReq(), map[string]string{ + "taskId": testTaskID, + "checklistId": foreignChecklistID, + "itemId": foreignItemID, + })) + if res.StatusCode != 404 { + t.Fatalf("expected 404 (checklist belongs to a different task/project), got %d: %s", res.StatusCode, res.BodyString()) + } + + found := false + for _, row := range tc.DB.AllRows("task_checklist_items") { + if row[0] == foreignItemID { + found = true + } + } + if !found { + t.Fatal("foreign item was deleted despite the 404") + } +} From 538996936a1643fa9817e9600f932e93ee105a0b Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 14 Sep 2026 08:31:43 +0000 Subject: [PATCH 2/2] chore: bump patch version in plugin.json Co-Authored-By: Claude Sonnet 5 --- plugin.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin.json b/plugin.json index 06391b0..5c812b5 100644 --- a/plugin.json +++ b/plugin.json @@ -2,7 +2,7 @@ "id": "com.paca.checklist", "displayName": "Checklist", "description": "Adds named checklists with checkable items to tasks.", - "version": "0.2.11", + "version": "0.2.12", "minCoreVersion": "v0.13.3", "permissions": ["db.read", "db.write", "events.subscribe"], "backend": {