docs(19): code review of Chat and ChatChannels
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Jakub Zych
217
.planning/phases/19-chat-and-chatchannels/19-REVIEW.md
Normal file
217
.planning/phases/19-chat-and-chatchannels/19-REVIEW.md
Normal file
@@ -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_
|
||||
Reference in New Issue
Block a user