From 3900c6104e804ba3f434231ba1b251b1fab9c862 Mon Sep 17 00:00:00 2001 From: Sandro Date: Thu, 29 Oct 2026 14:28:39 +0100 Subject: [PATCH] fix(security): list items need write access to their list (FLAW-27) Creating, checking, editing, deleting, reordering and clearing list items did not check the list at all: any logged-in user with a list or item ID could change it, viewers of shared lists as well. All item endpoints and converting a thought into a list item now require owner or editor rights and that the item belongs to the list (404 without access, 403 for viewers). Co-Authored-By: Claude Opus 5.5 --- Doku/flaws.md | 6 ++++ internal/api/handlers_lists.go | 35 ++++++++++++++++++++++ internal/storage/lists.go | 45 +++++++++++++++++++++++++++++ internal/storage/thoughts.go | 4 +++ internal/storage/visibility_test.go | 35 ++++++++++++++++++++++ 5 files changed, 125 insertions(+) diff --git a/Doku/flaws.md b/Doku/flaws.md index f4c58cb..e8be04d 100644 --- a/Doku/flaws.md +++ b/Doku/flaws.md @@ -36,6 +36,7 @@ Schweregrade: **Hoch** (Datenabfluss oder Rechteumgehung möglich), **Mittel** ( | FLAW-24 | Gruppen | ~~Organisations-Endpunkte ohne Berechtigungsprüfung~~ | **Behoben** | | FLAW-25 | Workspace | ~~Struktur-Aktionen an fremd gesperrten Bullets nur teilweise ausgeführt~~ | **Behoben** | | FLAW-26 | Workspace | ~~Nach Enter landet sehr schnell Getipptes noch im vorherigen Bullet~~ | **Behoben** | +| FLAW-27 | Listen | ~~Listeneinträge ohne Berechtigungsprüfung änderbar (IDOR)~~ | **Behoben** | --- @@ -209,3 +210,8 @@ Schweregrade: **Hoch** (Datenabfluss oder Rechteumgehung möglich), **Mittel** ( ## Aus der früheren Fassung entfernt Die frühere `flaws.md` führte als „gelöst“, was im Code nicht existiert: In-Memory-DocRooms, `clientId`-Echo-Schutz, Cursor-Presence, 1,5-s-Debounce-Flush, `internal/ws/docroom.go`, `frontend/src/lib/useDocumentSync.ts` und Yjs/CRDT. Der Code enthält nur noch `internal/ws/hub.go` (Räume, Benutzer-Kanäle, SSE-Fallback). Die CRDT-Tabellen werden beim Start gelöscht (`DROP TABLE document_crdt_*`). Dokumente werden per REST gespeichert (`PATCH /documents/{id}/blocks`) und als `DOC_BLOCKS_PATCHED` verteilt. Die frühere Aussage zur Token-Versionierung (`token_version`, `revoked_tokens`) trifft dagegen zu und ist in [sharing-and-permissions.md](sharing-and-permissions.md) und [architecture.md](architecture.md) beschrieben. + +### FLAW-27: Listeneinträge ohne Berechtigungsprüfung +- **Status: Behoben.** Gefunden beim Bau der API-Key-Scopes (Oktober 2026). +- **Problem:** `POST/PATCH/PUT/DELETE /lists/{id}/items…`, `…/items/reorder` und `…/clear-completed` prüften weder den Zugriff auf die Liste noch, ob der Eintrag zu ihr gehört. Jeder angemeldete Nutzer mit einer Listen- oder Eintrags-ID konnte Einträge anlegen, abhaken, ändern und löschen, auch Leser einer geteilten Liste. Ebenso ließ sich ein Gedanke in einen Eintrag einer fremden Liste umwandeln. +- **Behebung:** `Store.ListRole` / `EnsureListItemsWritable` (Eigentümer oder Bearbeiter, Eintrag muss zur Liste gehören) in allen Eintrags-Handlern und in `UpdateThoughtTriage`; ohne Zugriff `404`, als Leser `403`. Test: `TestListItemsNeedWriteAccess`. diff --git a/internal/api/handlers_lists.go b/internal/api/handlers_lists.go index 300e010..169a8a0 100644 --- a/internal/api/handlers_lists.go +++ b/internal/api/handlers_lists.go @@ -2,12 +2,14 @@ package api import ( "encoding/json" + "errors" "net/http" "strings" "github.com/go-chi/chi/v5" "github.com/sandrodach/mindsift/internal/auth" "github.com/sandrodach/mindsift/internal/domain" + "github.com/sandrodach/mindsift/internal/storage" ) // --- REALTIME LISTS & SHARING --- @@ -138,6 +140,9 @@ func (s *Server) handleDeleteListShare(w http.ResponseWriter, r *http.Request) { func (s *Server) handleCreateListItem(w http.ResponseWriter, r *http.Request) { userID := auth.GetUserID(r.Context()) listID := chi.URLParam(r, "id") + if !s.listItemsWriteAllowed(w, r, "") { + return + } var req struct { Content string `json:"content"` Category string `json:"category"` @@ -162,6 +167,9 @@ func (s *Server) handleToggleListItem(w http.ResponseWriter, r *http.Request) { userID := auth.GetUserID(r.Context()) listID := chi.URLParam(r, "id") itemID := chi.URLParam(r, "itemId") + if !s.listItemsWriteAllowed(w, r, itemID) { + return + } var req struct { IsCompleted bool `json:"isCompleted"` @@ -190,6 +198,9 @@ func (s *Server) handleToggleListItem(w http.ResponseWriter, r *http.Request) { func (s *Server) handleUpdateListItem(w http.ResponseWriter, r *http.Request) { listID := chi.URLParam(r, "id") itemID := chi.URLParam(r, "itemId") + if !s.listItemsWriteAllowed(w, r, itemID) { + return + } var req struct { Content *string `json:"content"` @@ -214,6 +225,9 @@ func (s *Server) handleUpdateListItem(w http.ResponseWriter, r *http.Request) { func (s *Server) handleDeleteListItem(w http.ResponseWriter, r *http.Request) { listID := chi.URLParam(r, "id") itemID := chi.URLParam(r, "itemId") + if !s.listItemsWriteAllowed(w, r, itemID) { + return + } _, err := s.store.DeleteListItem(itemID) if err != nil { @@ -230,6 +244,9 @@ func (s *Server) handleDeleteListItem(w http.ResponseWriter, r *http.Request) { func (s *Server) handleClearCompletedItems(w http.ResponseWriter, r *http.Request) { listID := chi.URLParam(r, "id") + if !s.listItemsWriteAllowed(w, r, "") { + return + } removed, err := s.store.ClearCompletedListItems(listID) if err != nil { writeError(w, http.StatusInternalServerError, err.Error()) @@ -245,6 +262,9 @@ func (s *Server) handleClearCompletedItems(w http.ResponseWriter, r *http.Reques func (s *Server) handleReorderListItems(w http.ResponseWriter, r *http.Request) { listID := chi.URLParam(r, "id") + if !s.listItemsWriteAllowed(w, r, "") { + return + } var req struct { Items []domain.ListItemReorderItem `json:"items"` } @@ -264,3 +284,18 @@ func (s *Server) handleReorderListItems(w http.ResponseWriter, r *http.Request) }) writeJSON(w, http.StatusOK, map[string]bool{"success": true}) } + +// listItemsWriteAllowed answers 404 (no access, or item not in this list) or 403 (viewer) and returns +// false; every request changing list items goes through it. +func (s *Server) listItemsWriteAllowed(w http.ResponseWriter, r *http.Request, itemID string) bool { + err := s.store.EnsureListItemsWritable(auth.GetUserID(r.Context()), chi.URLParam(r, "id"), itemID) + switch { + case err == nil: + return true + case errors.Is(err, storage.ErrListReadOnly): + writeError(w, http.StatusForbidden, err.Error()) + default: + writeError(w, http.StatusNotFound, err.Error()) + } + return false +} diff --git a/internal/storage/lists.go b/internal/storage/lists.go index 6a92a74..d0da1bf 100644 --- a/internal/storage/lists.go +++ b/internal/storage/lists.go @@ -500,3 +500,48 @@ func (s *Store) ReorderListItems(listID string, items []domain.ListItemReorderIt _, _ = tx.Exec(`UPDATE lists SET updated_at = ? WHERE id = ?`, domain.NowISO(), listID) return tx.Commit() } + +// ErrListNotFound: the list does not exist or the user has no access to it (deliberately the same). +var ErrListNotFound = errors.New("Liste nicht gefunden") + +// ErrListReadOnly: the list is shared with the user as viewer. +var ErrListReadOnly = errors.New("Diese Liste ist für dich nur lesbar") + +// ListRole is the user's role in a list: owner, editor, viewer or "" without access. +func (s *Store) ListRole(userID, listID string) string { + var role string + err := s.db.QueryRow(` + SELECT CASE WHEN l.user_id = ? THEN 'owner' + WHEN MAX(CASE WHEN ls.role = 'editor' THEN 1 ELSE 0 END) = 1 THEN 'editor' + ELSE 'viewer' END + FROM lists l + LEFT JOIN list_shares ls ON ls.list_id = l.id AND ( + (ls.target_type = 'user' AND ls.target_id = ?) + OR (ls.target_type = 'team' AND ls.target_id IN (SELECT team_id FROM team_members WHERE user_id = ?)) + ) + WHERE l.id = ? AND (l.user_id = ? OR ls.id IS NOT NULL) + GROUP BY l.id`, userID, userID, userID, listID, userID).Scan(&role) + if err != nil { + return "" + } + return role +} + +// EnsureListItemsWritable checks that the user may change the items of a list (owner or editor) and, +// if itemID is given, that the item belongs to that list. +func (s *Store) EnsureListItemsWritable(userID, listID, itemID string) error { + switch s.ListRole(userID, listID) { + case "": + return ErrListNotFound + case "viewer": + return ErrListReadOnly + } + if itemID != "" { + var n int + _ = s.db.QueryRow(`SELECT COUNT(*) FROM list_items WHERE id = ? AND list_id = ?`, itemID, listID).Scan(&n) + if n == 0 { + return ErrListNotFound + } + } + return nil +} diff --git a/internal/storage/thoughts.go b/internal/storage/thoughts.go index 8cfa59c..3b8056f 100644 --- a/internal/storage/thoughts.go +++ b/internal/storage/thoughts.go @@ -375,6 +375,10 @@ func (s *Store) UpdateThoughtTriage(userID, thoughtID, action, convertTo, target convertedRefID = doc.ID } } else if convertTo == "list_item" && targetListID != "" { + // Same rule as adding an item in the list itself: owner or editor + if err := s.EnsureListItemsWritable(userID, targetListID, ""); err != nil { + return nil, err + } newStatus = domain.StatusConverted convertedType = "list_item" item, err := s.CreateListItem(targetListID, thought.Content, "", "", 0, userID) diff --git a/internal/storage/visibility_test.go b/internal/storage/visibility_test.go index 801d03c..5ce8645 100644 --- a/internal/storage/visibility_test.go +++ b/internal/storage/visibility_test.go @@ -169,3 +169,38 @@ func TestDataSurvivesRestart(t *testing.T) { t.Fatalf("user_version = %d, want %d", v, schemaVersion) } } + +func TestListItemsNeedWriteAccess(t *testing.T) { + store, err := NewStore(filepath.Join(t.TempDir(), "listacl.db")) + if err != nil { + t.Fatal(err) + } + defer store.Close() + openSharing(t, store) + anna, _ := store.CreateUser("anna@example.com", "hash", "Anna", "member") + ben, _ := store.CreateUser("ben@example.com", "hash", "Ben", "member") + eve, _ := store.CreateUser("eve@example.com", "hash", "Eve", "member") + list, _ := store.CreateList(anna.ID, "Einkauf", "", "shopping") + item, _ := store.CreateListItem(list.ID, "Milch", "", "", 0, anna.ID) + other, _ := store.CreateList(eve.ID, "Eves Liste", "", "shopping") + otherItem, _ := store.CreateListItem(other.ID, "Geheim", "", "", 0, eve.ID) + _, _ = store.AddListShare(anna.ID, list.ID, "user", ben.ID, "viewer") + + if err := store.EnsureListItemsWritable(anna.ID, list.ID, item.ID); err != nil { + t.Errorf("owner must write: %v", err) + } + if err := store.EnsureListItemsWritable(ben.ID, list.ID, item.ID); err != ErrListReadOnly { + t.Errorf("viewer must be read-only, got %v", err) + } + if err := store.EnsureListItemsWritable(eve.ID, list.ID, ""); err != ErrListNotFound { + t.Errorf("stranger must not see the list, got %v", err) + } + if err := store.EnsureListItemsWritable(anna.ID, list.ID, otherItem.ID); err != ErrListNotFound { + t.Errorf("an item of another list must not be reachable through my list, got %v", err) + } + // Turning a thought into an item of someone else's list is refused as well + th, _ := store.CreateThoughtWithDetails(eve.ID, "Spam", "normal", "manual", "", "", "todo", "") + if _, err := store.UpdateThoughtTriage(eve.ID, th.ID, "keep", "list_item", list.ID, "", ""); err == nil { + t.Errorf("thought must not become an item of a list without write access") + } +}