From bef158941a1caca000b308f6a06cd575955e9b8a Mon Sep 17 00:00:00 2001 From: richjohnrambo Date: Wed, 9 Sep 2026 15:48:55 +0800 Subject: [PATCH] fix: validate group avatar attachments --- server/im_http.go | 35 ++++++++++++++++++++++------------- server/im_http_test.go | 20 ++++++++++++++++---- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/server/im_http.go b/server/im_http.go index 8b024fe..f1e508e 100644 --- a/server/im_http.go +++ b/server/im_http.go @@ -4655,6 +4655,12 @@ func serveIMGroupCreate(wrt http.ResponseWriter, req *http.Request) { CreatedAt: now, UpdatedAt: now, } + if settings.AvatarFileID != "" { + if err := linkIMGroupAvatarAttachment(principal.TenantID, topicName, settings.AvatarFileID); err != nil { + writeIMError(wrt, req, http.StatusBadRequest, 30000, imReasonMalformedRequest, "malformed request", map[string]any{"field": "avatar_file_id"}) + return + } + } memberIDs := make([]types.Uid, 0, len(body.MemberUserIDs)) seen := map[types.Uid]bool{principal.Uid: true} for _, rawID := range body.MemberUserIDs { @@ -4671,11 +4677,6 @@ func serveIMGroupCreate(wrt http.ResponseWriter, req *http.Request) { writeIMRelationError(wrt, req, err) return } - if settings.AvatarFileID != "" { - if err := store.Files.LinkAttachments(principal.TenantID, topicName, types.ZeroUid, []string{settings.AvatarFileID}); err != nil { - logs.Warn.Printf("group[%s]: failed to link avatar attachment: %v", topicName, err) - } - } dispatchIMGroupEvents(principal.TenantID) created, err := store.Topics.Get(principal.TenantID, topicName) if err != nil { @@ -4932,7 +4933,14 @@ func serveIMGroupDetail(wrt http.ResponseWriter, req *http.Request, topic string public["fn"] = name } if body.AvatarFileID != nil { - public["avatar_file_id"] = strings.TrimSpace(*body.AvatarFileID) + avatar := strings.TrimSpace(*body.AvatarFileID) + if avatar != "" { + if err := linkIMGroupAvatarAttachment(principal.TenantID, topic, avatar); err != nil { + writeIMError(wrt, req, http.StatusBadRequest, 30000, imReasonMalformedRequest, "malformed request", map[string]any{"field": "avatar_file_id"}) + return + } + } + public["avatar_file_id"] = avatar } if body.Description != nil { description := strings.TrimSpace(*body.Description) @@ -4961,13 +4969,6 @@ func serveIMGroupDetail(wrt http.ResponseWriter, req *http.Request, topic string return } } - if body.AvatarFileID != nil { - if avatar := strings.TrimSpace(*body.AvatarFileID); avatar != "" { - if err := store.Files.LinkAttachments(principal.TenantID, topic, types.ZeroUid, []string{avatar}); err != nil { - logs.Warn.Printf("group[%s]: failed to link avatar attachment: %v", topic, err) - } - } - } dispatchIMGroupEvents(principal.TenantID) updated, err := store.Topics.Get(principal.TenantID, topic) if err != nil { @@ -6949,6 +6950,14 @@ func normalizeIMUploadedFileID(raw string) string { return "" } +func linkIMGroupAvatarAttachment(tenantID types.TenantID, topic, avatarFileID string) error { + fileID := normalizeIMUploadedFileID(avatarFileID) + if fileID == "" { + return types.ErrMalformed + } + return store.Files.LinkAttachments(tenantID, topic, types.ZeroUid, []string{fileID}) +} + func parseIMLimit(raw string, fallback, max int) int { limit := fallback if raw != "" { diff --git a/server/im_http_test.go b/server/im_http_test.go index 44d8f37..fbd0b95 100644 --- a/server/im_http_test.go +++ b/server/im_http_test.go @@ -4095,6 +4095,7 @@ func TestServeIMGroupCreate(t *testing.T) { defer ctrl.Finish() oldStore := store.Store oldGroupBusiness := store.GroupBusiness + oldFiles := store.Files mockStore := mock_store.NewMockPersistentStorageInterface(ctrl) tokenAuth := mock_auth.NewMockAuthHandler(ctrl) mockStore.EXPECT().GetLogicalAuthHandler("token").Return(tokenAuth) @@ -4107,23 +4108,27 @@ func TestServeIMGroupCreate(t *testing.T) { oldSubs := store.Subs topics := mock_store.NewMockTopicsPersistenceInterface(ctrl) subs := mock_store.NewMockSubsPersistenceInterface(ctrl) + files := mock_store.NewMockFilePersistenceInterface(ctrl) store.Topics = topics store.Subs = subs + store.Files = files groupBusiness := &testGroupBusinessStore{} store.GroupBusiness = groupBusiness defer func() { store.Store = oldStore store.Topics = oldTopics store.Subs = oldSubs + store.Files = oldFiles store.GroupBusiness = oldGroupBusiness }() + avatarID := types.Uid(101).String() topics.EXPECT().Get(types.TenantID(7), "grpnewgroup").Return(&types.Topic{ ObjHeader: types.ObjHeader{Id: "grpnewgroup"}, TenantID: 7, Owner: types.Uid(42).String(), SubCnt: 2, - Public: map[string]any{"name": "New Group", "avatar_file_id": "file-1", "description": "Project discussion", "version": 1}, + Public: map[string]any{"name": "New Group", "avatar_file_id": avatarID + ".jpg", "description": "Project discussion", "version": 1}, }, nil) subs.EXPECT().Get(types.TenantID(7), "grpnewgroup", types.Uid(42), false).Return(&types.Subscription{ TenantID: 7, @@ -4132,8 +4137,9 @@ func TestServeIMGroupCreate(t *testing.T) { ModeWant: types.ModeCFull, ModeGiven: types.ModeCFull, }, nil) + files.EXPECT().LinkAttachments(types.TenantID(7), "grpnewgroup", types.ZeroUid, []string{avatarID}).Return(nil) - req := httptest.NewRequest(http.MethodPost, "/v1/im/groups", bytes.NewBufferString(`{"name":"New Group","description":"Project discussion","member_user_ids":["usrAgAAAAAAAAA"],"avatar_file_id":"file-1"}`)) + req := httptest.NewRequest(http.MethodPost, "/v1/im/groups", bytes.NewBufferString(`{"name":"New Group","description":"Project discussion","member_user_ids":["usrAgAAAAAAAAA"],"avatar_file_id":"`+avatarID+`.jpg"}`)) req.Header.Set("Authorization", "Bearer token-1") rec := httptest.NewRecorder() @@ -4243,6 +4249,7 @@ func TestServeIMGroupDetailPatchKeepsTopicProfileWhenBusinessSettingsMissing(t * defer ctrl.Finish() oldStore := store.Store oldGroupBusiness := store.GroupBusiness + oldFiles := store.Files mockStore := mock_store.NewMockPersistentStorageInterface(ctrl) tokenAuth := mock_auth.NewMockAuthHandler(ctrl) mockStore.EXPECT().GetLogicalAuthHandler("token").Return(tokenAuth) @@ -4254,13 +4261,16 @@ func TestServeIMGroupDetailPatchKeepsTopicProfileWhenBusinessSettingsMissing(t * oldSubs := store.Subs topics := mock_store.NewMockTopicsPersistenceInterface(ctrl) subs := mock_store.NewMockSubsPersistenceInterface(ctrl) + files := mock_store.NewMockFilePersistenceInterface(ctrl) store.Topics = topics store.Subs = subs + store.Files = files store.GroupBusiness = &testGroupBusinessStore{updateSettingsErr: types.ErrNotFound} defer func() { store.Store = oldStore store.Topics = oldTopics store.Subs = oldSubs + store.Files = oldFiles store.GroupBusiness = oldGroupBusiness }() selfSub := &types.Subscription{ @@ -4278,9 +4288,11 @@ func TestServeIMGroupDetailPatchKeepsTopicProfileWhenBusinessSettingsMissing(t * SubCnt: 3, Public: map[string]any{"name": "Alpha", "version": 1}, } + avatarID := types.Uid(100).String() var savedPublic map[string]any subs.EXPECT().Get(types.TenantID(7), "grpAlpha", types.Uid(42), false).Return(selfSub, nil) topics.EXPECT().Get(types.TenantID(7), "grpAlpha").Return(group, nil) + files.EXPECT().LinkAttachments(types.TenantID(7), "grpAlpha", types.ZeroUid, []string{avatarID}).Return(nil) topics.EXPECT().Update(types.TenantID(7), "grpAlpha", gomock.Any()).DoAndReturn( func(_ types.TenantID, _ string, update map[string]any) error { public, ok := update["Public"].(map[string]any) @@ -4303,7 +4315,7 @@ func TestServeIMGroupDetailPatchKeepsTopicProfileWhenBusinessSettingsMissing(t * }, ) - req := httptest.NewRequest(http.MethodPatch, "/v1/im/groups/grpAlpha", bytes.NewBufferString(`{"avatar_file_id":"file-avatar"}`)) + req := httptest.NewRequest(http.MethodPatch, "/v1/im/groups/grpAlpha", bytes.NewBufferString(`{"avatar_file_id":"`+avatarID+`.jpg"}`)) req.Header.Set("Authorization", "Bearer token-1") rec := httptest.NewRecorder() serveIMGroupResource(rec, req) @@ -4311,7 +4323,7 @@ func TestServeIMGroupDetailPatchKeepsTopicProfileWhenBusinessSettingsMissing(t * if rec.Code != http.StatusOK { t.Fatalf("patch status: got %d body=%s", rec.Code, rec.Body.String()) } - if data := decodeIMHTTPResponse(t, rec).Data.(map[string]any); data["avatar_file_id"] != "file-avatar" || data["version"].(float64) != 2 { + if data := decodeIMHTTPResponse(t, rec).Data.(map[string]any); data["avatar_file_id"] != avatarID+".jpg" || data["version"].(float64) != 2 { t.Fatalf("unexpected patch response: %#v", data) } }