From 2ddafb87559a69a8f73848d269156821a759bdce Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sat, 17 Oct 2026 22:13:29 +0200 Subject: [PATCH] docs(19): code review of Chat and ChatChannels Co-authored-by: Cursor --- .../19-chat-and-chatchannels/19-REVIEW.md | 217 ++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 .planning/phases/19-chat-and-chatchannels/19-REVIEW.md diff --git a/.planning/phases/19-chat-and-chatchannels/19-REVIEW.md b/.planning/phases/19-chat-and-chatchannels/19-REVIEW.md new file mode 100644 index 00000000..5c550dcb --- /dev/null +++ b/.planning/phases/19-chat-and-chatchannels/19-REVIEW.md @@ -0,0 +1,217 @@ +--- +phase: 19-chat-and-chatchannels +reviewed: 2026-10-17T20:12:19Z +depth: standard +files_reviewed: 64 +files_reviewed_list: + - scripts/check-phase19.sh + - ../sm-chat-plugin/api/attachments.go + - ../sm-chat-plugin/api/auth.go + - ../sm-chat-plugin/api/conversations.go + - ../sm-chat-plugin/api/dm.go + - ../sm-chat-plugin/api/download.go + - ../sm-chat-plugin/api/errors.go + - ../sm-chat-plugin/api/json.go + - ../sm-chat-plugin/api/messages.go + - ../sm-chat-plugin/api/mute.go + - ../sm-chat-plugin/api/read.go + - ../sm-chat-plugin/api/replies.go + - ../sm-chat-plugin/api/threads.go + - ../sm-chat-plugin/api/typing.go + - ../sm-chat-plugin/boot.go + - ../sm-chat-plugin/classes/attachment_policy.go + - ../sm-chat-plugin/classes/attachments.go + - ../sm-chat-plugin/classes/authorizer.go + - ../sm-chat-plugin/classes/emit.go + - ../sm-chat-plugin/classes/queries.go + - ../sm-chat-plugin/classes/serialize.go + - ../sm-chat-plugin/classes/service.go + - ../sm-chat-plugin/classes/writes.go + - ../sm-chat-plugin/contracts/context.go + - ../sm-chat-plugin/contracts/registry.go + - ../sm-chat-plugin/jobs/maintain_thread_summary.go + - ../sm-chat-plugin/models/conversation.go + - ../sm-chat-plugin/models/message.go + - ../sm-chat-plugin/plugin.go + - ../sm-chat-plugin/queststream_trace_test.go + - ../sm-chat-plugin/routes.go + - ../sm-chat-plugin/updates/202610160001_create_golem15_chat_baseline.go + - ../sm-chatchannels-plugin/api/auth.go + - ../sm-chatchannels-plugin/api/categories.go + - ../sm-chatchannels-plugin/api/channels.go + - ../sm-chatchannels-plugin/api/errors.go + - ../sm-chatchannels-plugin/api/json.go + - ../sm-chatchannels-plugin/api/members.go + - ../sm-chatchannels-plugin/api/mentions.go + - ../sm-chatchannels-plugin/api/pins.go + - ../sm-chatchannels-plugin/api/prefs.go + - ../sm-chatchannels-plugin/api/search.go + - ../sm-chatchannels-plugin/boot.go + - ../sm-chatchannels-plugin/classes/channel_service.go + - ../sm-chatchannels-plugin/classes/deny_all.go + - ../sm-chatchannels-plugin/classes/mention_parser.go + - ../sm-chatchannels-plugin/classes/search_indexer.go + - ../sm-chatchannels-plugin/contracts/access_resolver.go + - ../sm-chatchannels-plugin/models/channel.go + - ../sm-chatchannels-plugin/models/message_index.go + - ../sm-chatchannels-plugin/models/prefs.go + - ../sm-chatchannels-plugin/plugin.go + - ../sm-chatchannels-plugin/queststream_trace_test.go + - ../sm-chatchannels-plugin/routes.go + - ../sm-chatchannels-plugin/updates/202610160001_create_golem15_chatchannels_baseline.go + - ../fonoteka.go/plugins/golem15/user/classes/authorizer.go + - ../fonoteka.go/plugins/golem15/user/classes/events.go + - ../fonoteka.go/plugins/golem15/user/classes/handle_generator.go + - ../fonoteka.go/plugins/golem15/user/console/backfill_handles.go + - ../fonoteka.go/plugins/golem15/user/controllers/registration.go + - ../fonoteka.go/plugins/golem15/user/controllers/users_admin_controller.go + - ../fonoteka.go/plugins/golem15/user/models/user.go + - ../fonoteka.go/plugins/golem15/user/plugin.go + - ../fonoteka.go/plugins/golem15/user/updates/202610160002_add_handle_to_users.go +findings: + critical: 2 + warning: 4 + info: 2 + total: 8 +status: issues_found +--- + +# Phase 19: Code Review Report + +**Reviewed:** 2026-10-17T20:12:19Z +**Depth:** standard +**Files Reviewed:** 64 (sm-chat-plugin, sm-chatchannels-plugin, sm-user-plugin under fonoteka.go, and `scripts/check-phase19.sh`) +**Status:** issues_found + +## Summary + +Phase 19 ports Golem15.Chat and Golem15.ChatChannels as compiled sibling plugins, plus `users.handle`, `user:{id}` lighthouse authz, and `user.Deleting` as the GDPR seam. Several locked gates hold: `created.chat.message` is ids-only (no `content` key), the chat authorizer splits `chat:conversation:{id}` locally and does not use `ChannelID`, HTTP IDOR collapses to 404, attachment download 404s for stranger and missing, mentions resolve only among participants excluding the sender, ChannelAccessResolver defaults to deny-all, `jwt.auth` wraps before throttle on both prefixes, and `TestNoQuestStreamTrace` keeps D-02 tokens out of production sources. + +Two defects must be fixed before this ships. `canSend` lets any host-context member inject messages into another pair's DM, which PHP `ChatService::sendMessage` refuses. Deleting a user who created a conversation then CASCADE-deletes that conversation and its channel, so GDPR `SeverUserFromContext` does not preserve remaining members' history. + +## Critical Issues + +### CR-01: Context members can send into other users' DMs + +**File:** `../sm-chat-plugin/classes/service.go:255-273` +**Issue:** `SendMessage` authorizes with `canSend`, which returns true for a non-participant whenever the conversation's host context reports `HasMember`. `GetOrCreateDm` stores the host alias (`teams`) on the DM row, so every teammate is a context member of every DM in that context. `POST /conversations/{id}/messages` therefore accepts a JWT from user C writing into A↔B's DM; the handler returns 201 with full `MessageAPI` body. List/get still 404 via `CanView` (participant or overseer only), so this is a write IDOR, not a read IDOR. + +PHP `ChatService::sendMessage` requires `$conversation->hasParticipant($sender)` and has no HasMember fallback. HTTP tracers only prove a non-member of the context is 404; they do not cover a context member who is not a DM participant. + +**Fix:** Require participation, matching PHP. Lazy-join for group chats already happens in `GetGroupChat` via `addParticipant`. + +```go +func (s *Service) canSend(ctx context.Context, conv *models.Conversation, senderID uint) (bool, error) { + if conv == nil || senderID == 0 { + return false, nil + } + return s.isParticipant(ctx, conv.ID, senderID) +} +``` + +Add a postgres case: two context members open a DM; a third context member POSTs to that conversation id and must get 404. + +### CR-02: Deleting a conversation creator CASCADE-wipes the shared channel + +**File:** `../sm-chat-plugin/updates/202610160001_create_golem15_chat_baseline.go:26-27` +**Issue:** `golem15_chat_conversations.created_by` is `NOT NULL` with `ON DELETE CASCADE`. ChatChannels then has `golem15_chatchannels_channels.conversation_id` `ON DELETE CASCADE` (`../sm-chatchannels-plugin/updates/202610160001_create_golem15_chatchannels_baseline.go:48-49`). Admin `FormAfterDelete` fires `user.Deleting` then force-deletes the user row (`users_admin_controller.go:525-532`). `SeverUserFromContext` only removes that user's members, prefs, personal categories, and chat participants. It does not reassign `created_by`. + +Deleting the user who created a channel therefore deletes the conversation, then the channel, then remaining members' prefs and messages. `TestDeletingCascade` deletes Bob as a member of Alice's channel, so it never observes creator deletion. Mentions, pins (`pinned_by`), and user-context prefs already CASCADE from `users.id` and are not the problem. + +**Fix:** Before the user row is deleted, reassign `created_by` to another participant (or a designated remainder) on every conversation the user created, then let `SeverUserFromContext` run. Keep the PHP constraint names for import; do not rely on changing CASCADE on an `IF NOT EXISTS` baseline that already landed in PHP databases. + +```go +// in onUserDeleting, before or as part of SeverUserFromContext: +var convs []chatmodels.Conversation +if err := db.Where("created_by = ?", ev.User.ID).Find(&convs).Error; err != nil { + return err +} +for _, c := range convs { + var next uint + if err := db.Model(&chatmodels.Participant{}). + Where("conversation_id = ? AND user_id <> ?", c.ID, ev.User.ID). + Limit(1).Pluck("user_id", &next).Error; err != nil { + return err + } + if next == 0 { + // no remaining participant: deleting the user may drop an empty conv + continue + } + if err := db.Model(&c).Update("created_by", next).Error; err != nil { + return err + } +} +``` + +Extend `TestDeletingCascade` so Alice creates the channel, Alice is deleted, and Bob's member row and the channel row still exist. + +## Warnings + +### WR-01: Channel search is not scoped to the requested context + +**File:** `../sm-chatchannels-plugin/api/search.go:25-38` +**Issue:** After `ChannelsFor` confirms the actor may see the context, `SearchIDs` runs against the global `messages` collection with only `q`. `MessageIndex.ToSearchableArray` indexes `content`. A member of context A can probe whether a string occurs in context B's messages: matching IDs are returned; `GET /messages/{id}` then 404s, but the ID list is already a cross-tenant existence oracle. + +**Fix:** Filter hits to conversation IDs that back channels in `{type}/{id}` the actor can access (join `golem15_chatchannels_channels.conversation_id` for that context, then keep `SearchIDs` results in that set). Prefer a Typesense filter on `conversation_id` if the engine supports it. + +### WR-02: Public-channel join never grants send rights + +**File:** `../sm-chatchannels-plugin/classes/channel_service.go:958-968` +**Issue:** `JoinChannel` only clears `has_left` on the prefs row. It does not insert `golem15_chatchannels_channel_members` or a chat participant. `AddMember` returns `ErrMembersPrivate` for public channels (`channel_service.go:621-622`). HTTP `CreateChannel` passes `participantIDs: nil`, so only the creator is a member. Channel-backed conversations use context alias `channel`; `canSend`'s HasMember fallback then looks at the members table. A context member who can `GET` a public channel cannot `POST` a message on its conversation, and join does not change that. + +**Fix:** For a public channel, `JoinChannel` should `firstOrCreateMember` and `addChatParticipant` (the same pairing `CreateChannel` uses). Keep private channels on `AddMember` / membership. + +### WR-03: Pins accept a message id from another conversation + +**File:** `../sm-chatchannels-plugin/classes/channel_service.go:1332-1344` +**Issue:** `PinMessage` checks channel access, then `FirstOrCreate`s `{channel_id, message_id}` with `ConversationID` taken from the channel. It never loads the message or checks `message.conversation_id == ch.ConversationID`. Any accessible channel can pin an arbitrary id. The pin list then exposes that id to every member of the channel. + +**Fix:** Load the message; require `msg.ConversationID` to equal `*ch.ConversationID`; otherwise return `ErrNotFound` (IDOR collapse). + +```go +msg, err := /* load golem15_chat_messages by messageID */ +if err != nil || ch.ConversationID == nil || msg.ConversationID != *ch.ConversationID { + return nil, ErrNotFound +} +``` + +### WR-04: Attachment Content-Disposition interpolates an unsanitized filename + +**File:** `../sm-chat-plugin/api/download.go:63-68` +**Issue:** Download sets `Content-Disposition` to `filename="` plus `FileName` with only `"` stripped. `attach.clientBaseName` trims a path element and whitespace; it does not strip CR/LF. A crafted upload name can break the header. Participation-gated 404 is otherwise correct (`AttachmentForDownload` maps missing and unauthorized to `ErrNotFound`). + +**Fix:** Restrict the filename token to a safe subset before quoting (no CR/LF, no `"`, no `;`), or use `filename*` RFC 5987 with a sanitized fallback. + +```go +name := strings.Map(func(r rune) rune { + if r < 32 || r == '"' || r == ';' || r == '\\' { + return -1 + } + return r +}, f.FileName) +if name == "" { + name = "download" +} +``` + +## Info + +### IN-01: Existing group-chat access ignores addParticipant errors + +**File:** `../sm-chat-plugin/classes/writes.go:38-40` +**Issue:** `GetGroupChat` on an existing conversation does `_ = addParticipant(...)`. A failed insert leaves a context member who received 200 for the conversation but is still not a participant. After CR-01 (participant-only send) that user would 404 on send until a retry succeeds. + +**Fix:** Return the `addParticipant` error (mapped to 500), or treat a unique-index conflict as success. + +### IN-02: user.Deleting is only fired from admin FormAfterDelete + +**File:** `../fonoteka.go/plugins/golem15/user/controllers/users_admin_controller.go:516-532` +**Issue:** `FireDeletingEvent` is documented as the GDPR seam and is called from `FormAfterDelete` before `ForceDeleteCleanup` / unscoped delete. No other production delete path in this diff fires it. A later API or console hard-delete would skip `SeverUserFromContext` (CR-02's reassignment would also be skipped). + +**Fix:** Route every permanent user delete through `FireDeletingEvent` (shared helper already exists in `registration.go:116-121`). + +--- + +_Reviewed: 2026-10-17T20:12:19Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_