fix(security): list items need write access to their list (FLAW-27)
MindSift CI & Docker Release / Build, Test in Container & Push (push) Successful in 59s
MindSift CI & Docker Release / Build, Test in Container & Push (push) Successful in 59s
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
11c14fa9ff
commit
3900c6104e
5 files changed
+125
No files matched your search
@@ -36,6 +36,7 @@ Schweregrade: **Hoch** (Datenabfluss oder Rechteumgehung möglich), **Mittel** (
|
|||||||
| FLAW-24 | Gruppen | ~~Organisations-Endpunkte ohne Berechtigungsprüfung~~ | **Behoben** |
|
| FLAW-24 | Gruppen | ~~Organisations-Endpunkte ohne Berechtigungsprüfung~~ | **Behoben** |
|
||||||
| FLAW-25 | Workspace | ~~Struktur-Aktionen an fremd gesperrten Bullets nur teilweise ausgeführt~~ | **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-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
|
## 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.
|
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`.
|
||||||
@@ -2,12 +2,14 @@ package api
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"net/http"
|
"net/http"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"github.com/go-chi/chi/v5"
|
"github.com/go-chi/chi/v5"
|
||||||
"github.com/sandrodach/mindsift/internal/auth"
|
"github.com/sandrodach/mindsift/internal/auth"
|
||||||
"github.com/sandrodach/mindsift/internal/domain"
|
"github.com/sandrodach/mindsift/internal/domain"
|
||||||
|
"github.com/sandrodach/mindsift/internal/storage"
|
||||||
)
|
)
|
||||||
|
|
||||||
// --- REALTIME LISTS & SHARING ---
|
// --- 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) {
|
func (s *Server) handleCreateListItem(w http.ResponseWriter, r *http.Request) {
|
||||||
userID := auth.GetUserID(r.Context())
|
userID := auth.GetUserID(r.Context())
|
||||||
listID := chi.URLParam(r, "id")
|
listID := chi.URLParam(r, "id")
|
||||||
|
if !s.listItemsWriteAllowed(w, r, "") {
|
||||||
|
return
|
||||||
|
}
|
||||||
var req struct {
|
var req struct {
|
||||||
Content string `json:"content"`
|
Content string `json:"content"`
|
||||||
Category string `json:"category"`
|
Category string `json:"category"`
|
||||||
@@ -162,6 +167,9 @@ func (s *Server) handleToggleListItem(w http.ResponseWriter, r *http.Request) {
|
|||||||
userID := auth.GetUserID(r.Context())
|
userID := auth.GetUserID(r.Context())
|
||||||
listID := chi.URLParam(r, "id")
|
listID := chi.URLParam(r, "id")
|
||||||
itemID := chi.URLParam(r, "itemId")
|
itemID := chi.URLParam(r, "itemId")
|
||||||
|
if !s.listItemsWriteAllowed(w, r, itemID) {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
var req struct {
|
var req struct {
|
||||||
IsCompleted bool `json:"isCompleted"`
|
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) {
|
func (s *Server) handleUpdateListItem(w http.ResponseWriter, r *http.Request) {
|
||||||
listID := chi.URLParam(r, "id")
|
listID := chi.URLParam(r, "id")
|
||||||
itemID := chi.URLParam(r, "itemId")
|
itemID := chi.URLParam(r, "itemId")
|
||||||
|
if !s.listItemsWriteAllowed(w, r, itemID) {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
var req struct {
|
var req struct {
|
||||||
Content *string `json:"content"`
|
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) {
|
func (s *Server) handleDeleteListItem(w http.ResponseWriter, r *http.Request) {
|
||||||
listID := chi.URLParam(r, "id")
|
listID := chi.URLParam(r, "id")
|
||||||
itemID := chi.URLParam(r, "itemId")
|
itemID := chi.URLParam(r, "itemId")
|
||||||
|
if !s.listItemsWriteAllowed(w, r, itemID) {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
_, err := s.store.DeleteListItem(itemID)
|
_, err := s.store.DeleteListItem(itemID)
|
||||||
if err != nil {
|
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) {
|
func (s *Server) handleClearCompletedItems(w http.ResponseWriter, r *http.Request) {
|
||||||
listID := chi.URLParam(r, "id")
|
listID := chi.URLParam(r, "id")
|
||||||
|
if !s.listItemsWriteAllowed(w, r, "") {
|
||||||
|
return
|
||||||
|
}
|
||||||
removed, err := s.store.ClearCompletedListItems(listID)
|
removed, err := s.store.ClearCompletedListItems(listID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
writeError(w, http.StatusInternalServerError, err.Error())
|
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) {
|
func (s *Server) handleReorderListItems(w http.ResponseWriter, r *http.Request) {
|
||||||
listID := chi.URLParam(r, "id")
|
listID := chi.URLParam(r, "id")
|
||||||
|
if !s.listItemsWriteAllowed(w, r, "") {
|
||||||
|
return
|
||||||
|
}
|
||||||
var req struct {
|
var req struct {
|
||||||
Items []domain.ListItemReorderItem `json:"items"`
|
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})
|
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
|
||||||
|
}
|
||||||
@@ -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)
|
_, _ = tx.Exec(`UPDATE lists SET updated_at = ? WHERE id = ?`, domain.NowISO(), listID)
|
||||||
return tx.Commit()
|
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
|
||||||
|
}
|
||||||
@@ -375,6 +375,10 @@ func (s *Store) UpdateThoughtTriage(userID, thoughtID, action, convertTo, target
|
|||||||
convertedRefID = doc.ID
|
convertedRefID = doc.ID
|
||||||
}
|
}
|
||||||
} else if convertTo == "list_item" && targetListID != "" {
|
} 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
|
newStatus = domain.StatusConverted
|
||||||
convertedType = "list_item"
|
convertedType = "list_item"
|
||||||
item, err := s.CreateListItem(targetListID, thought.Content, "", "", 0, userID)
|
item, err := s.CreateListItem(targetListID, thought.Content, "", "", 0, userID)
|
||||||
|
|||||||
@@ -169,3 +169,38 @@ func TestDataSurvivesRestart(t *testing.T) {
|
|||||||
t.Fatalf("user_version = %d, want %d", v, schemaVersion)
|
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")
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user