core: group fixes/improvements (#7660)

* core: group fixes/improvements

* Large messages dropped by relays and hosts (the same 15602 limit was applied, without accounting for forwarding header)
* Members below v18 did not receive forwarded messages or history in p2p groups.
* Join confirmations near the size limit could fail.
* SMP servers are limited to 2 host names each.

* simplify

* remove legacy forwarding and history support

* only restrict p2p groups

---------

Co-authored-by: Evgeny @ SimpleX Chat <259188159+evgeny-simplex@users.noreply.github.com>
This commit is contained in:
Evgeny
2026-10-08 15:06:18 +01:00
committed by GitHub
co-authored by Evgeny @ SimpleX Chat
parent dd7768e543
commit 0c1ffa65bc
11 changed files with 112 additions and 18 deletions
+5
View File
@@ -1931,10 +1931,13 @@ enum UserServersError: Decodable {
case storageMissing(protocol: ServerProtocol, user: UserRef?)
case proxyMissing(protocol: ServerProtocol, user: UserRef?)
case duplicateServer(protocol: ServerProtocol, duplicateServer: String, duplicateHost: String)
case tooManyHosts(protocol: ServerProtocol, tooManyHostsServer: String)
case duplicateChatRelayAddress(duplicateChatRelay: String, duplicateAddress: String)
var globalError: String? {
switch self {
case .tooManyHosts:
return globalSMPError
case let .noServers(`protocol`, _):
switch `protocol` {
case .smp: return globalSMPError
@@ -1977,6 +1980,8 @@ enum UserServersError: Decodable {
} else {
return text
}
case let .tooManyHosts(.smp, server):
return String.localizedStringWithFormat(NSLocalizedString("Server %@ has more than two host names.", comment: "servers error"), serverHostname(server))
default:
return nil
}
@@ -4886,6 +4886,7 @@ sealed class UserServersError {
@Serializable @SerialName("storageMissing") data class StorageMissing(val protocol: ServerProtocol, val user: UserRef?): UserServersError()
@Serializable @SerialName("proxyMissing") data class ProxyMissing(val protocol: ServerProtocol, val user: UserRef?): UserServersError()
@Serializable @SerialName("duplicateServer") data class DuplicateServer(val protocol: ServerProtocol, val duplicateServer: String, val duplicateHost: String): UserServersError()
@Serializable @SerialName("tooManyHosts") data class TooManyHosts(val protocol: ServerProtocol, val tooManyHostsServer: String): UserServersError()
@Serializable @SerialName("duplicateChatRelayAddress") data class DuplicateChatRelayAddress(val duplicateChatRelay: String, val duplicateAddress: String): UserServersError()
val globalError: String?
@@ -4901,6 +4902,7 @@ sealed class UserServersError {
is StorageMissing -> this.protocol
is ProxyMissing -> this.protocol
is DuplicateServer -> this.protocol
is TooManyHosts -> this.protocol
is DuplicateChatRelayAddress -> null
}
@@ -4916,6 +4918,8 @@ sealed class UserServersError {
is ProxyMissing -> this.user?.let { "${userStr(it)} ${generalGetString(MR.strings.no_message_servers_configured_for_private_routing)}" }
?: generalGetString(MR.strings.no_message_servers_configured_for_private_routing)
is TooManyHosts -> String.format(generalGetString(MR.strings.server_has_too_many_hosts), ServerAddress.parseServerAddress(this.tooManyHostsServer)?.hostnames?.firstOrNull() ?: this.tooManyHostsServer)
else -> null
}
} else {
@@ -140,6 +140,7 @@
<string name="no_message_servers_configured">No message servers.</string>
<string name="no_message_servers_configured_for_receiving">No servers to receive messages.</string>
<string name="no_message_servers_configured_for_private_routing">No servers for private message routing.</string>
<string name="server_has_too_many_hosts">Server %1$s has more than two host names.</string>
<string name="no_media_servers_configured">No media &amp; file servers.</string>
<string name="no_media_servers_configured_for_sending">No servers to send files.</string>
<string name="no_media_servers_configured_for_private_routing">No servers to receive files.</string>
+8 -9
View File
@@ -1414,7 +1414,7 @@ sendHistory user gInfo@GroupInfo {membership} m@GroupMember {activeConn = Just c
-- (regular groups only; never channels) is an authored element -- all batch together in order.
let fwdEls = map (uncurry encodeFwdElement) (concat fwdMsgsByItem)
welcomeEl <- welcomeElement
let (batches, dropped) = batchElements maxEncodedMsgLength (fwdEls <> maybe [] (: []) welcomeEl)
let (batches, dropped) = batchElements maxForwardBatchLength (fwdEls <> maybe [] (: []) welcomeEl)
when (dropped > 0) $ toView $ CEvtChatErrors [ChatError $ CEInternalError ("sendHistory: dropped " <> show dropped <> " oversized history messages")]
forM_ batches $ \body ->
void $ withAgent $ \a -> sendMessages a [(aConnId conn, PQEncOff, MsgFlags False, VRValue Nothing body)]
@@ -1552,9 +1552,7 @@ sendHistory user gInfo@GroupInfo {membership} m@GroupMember {activeConn = Just c
pure $ map ((,) fwd) (contentVM : fileDescrVMs)
memberShortenedName :: GroupMember -> ContactName
memberShortenedName GroupMember {memberProfile = LocalProfile {displayName}}
| T.length displayName <= 16 = displayName
| otherwise = T.take 16 displayName `T.snoc` '…'
memberShortenedName GroupMember {memberProfile = LocalProfile {displayName}} = fwdMemberName displayName
-- the description proof travels on the last part, so that part leaves room for it
badgeDescrPartSize :: Int
@@ -2457,9 +2455,10 @@ compressToLimit maxLen s
s' = compressedBatchMsgBody_ s
compressConnInfo :: PQSupport -> MsgBody -> CM MsgBody
compressConnInfo pqSup = compressToLimit $ case pqSup of
PQSupportOn -> maxEncodedInfoLengthPQ
PQSupportOff -> maxEncodedInfoLength
compressConnInfo pqSup = compressToLimit $ e2eEncConnInfoLength pqSup - 2 - maxReplyQueueFraming
maxReplyQueueFraming :: Int
maxReplyQueueFraming = 256
encodeConnInfo :: MsgEncodingI e => ChatMsgEvent e -> CM ByteString
encodeConnInfo = encodeConnInfoPQ PQSupportOff
@@ -2468,7 +2467,7 @@ encodeConnInfoPQ :: MsgEncodingI e => PQSupport -> ChatMsgEvent e -> CM ByteStri
encodeConnInfoPQ pqSup chatMsgEvent = do
cxt <- chatStoreCxt
let info = ChatMessage {chatVRange = vr cxt, msgId = Nothing, chatMsgEvent}
case encodeChatMessage maxEncodedInfoLength info of
case encodeChatMessage maxEncodedProfileMsgLength info of
ECMEncoded connInfo -> compressConnInfo pqSup connInfo
ECMLarge -> throwChatError $ CEException "large info"
@@ -2477,7 +2476,7 @@ encodeSignedConnInfo :: MsgEncodingI e => PQSupport -> MsgSigning -> ChatMsgEven
encodeSignedConnInfo pqSup signing chatMsgEvent = do
vr <- chatVersionRange
let info = ChatMessage {chatVRange = vr, msgId = Nothing, chatMsgEvent}
case encodeChatMessage maxEncodedInfoLength info of
case encodeChatMessage maxEncodedProfileMsgLength info of
ECMEncoded body -> compressConnInfo pqSup $ encodeBatchElement (Just $ signChatMsgBody signing body) body
ECMLarge -> throwChatError $ CEException "large signed info"
+6 -4
View File
@@ -3937,7 +3937,7 @@ processAgentMessageConn cxt user@User {userId} entity gks_ corrId agentConnId ag
createInternalChatItem user (CDDirectRcv ct) (CIRcvConnEvent RCEVerificationCodeReset) Nothing
xGrpMsgForward :: GroupInfoKeys -> Maybe GroupChatScopeInfo -> GroupMember -> GrpMsgForward -> ParsedMsg 'Json -> UTCTime -> CM ()
xGrpMsgForward g@(GIK gInfo _) scopeInfo m@GroupMember {localDisplayName} GrpMsgForward {fwdSender, fwdBrokerTs = msgTs} parsedMsg@(ParsedMsg _ _ chatMsg@ChatMessage {chatMsgEvent}) brokerTs = do
xGrpMsgForward g@(GIK gInfo@GroupInfo {membership} _) scopeInfo m@GroupMember {localDisplayName} GrpMsgForward {fwdSender, fwdBrokerTs = msgTs} parsedMsg@(ParsedMsg _ _ chatMsg@ChatMessage {chatMsgEvent}) brokerTs = do
unless (isMemberGrpFwdRelay gInfo m) $ throwChatError (CEGroupContactRole localDisplayName)
case fwdSender of
FwdMember memberId memberName -> do
@@ -3945,6 +3945,8 @@ processAgentMessageConn cxt user@User {userId} entity gks_ corrId agentConnId ag
let allowCreate = toCMEventTag chatMsgEvent /= XGrpLeave_
withStore (\db -> getCreateUnknownGMByMemberId db cxt user gInfo memberId memberName unknownRole allowCreate) >>= \case
Just (author, unknown)
| not (useRelays' gInfo) && groupMemberId' author == groupMemberId' membership ->
messageError $ "x.grp.msg.forward: content attributed to own membership, forwarder " <> tshow (groupMemberId' m) <> ", event " <> tshow (toCMEventTag chatMsgEvent)
| memberRemoved author ->
logInfo $ "x.grp.msg.forward: ignoring content from removed member, group " <> tshow (groupId' gInfo) <> ", member " <> safeDecodeUtf8 (strEncode memberId) <> ", event " <> tshow (toCMEventTag chatMsgEvent)
| not (useRelays' gInfo) && not (expectedForwarder author) ->
@@ -4160,7 +4162,7 @@ runDeliveryTaskWorker a deliveryKey Worker {doWork} = do
withStore' $ \db -> setDeliveryTaskErrStatus db (deliveryTaskId task) "relay inactive"
| otherwise ->
withWorkItems a doWork (withStore' $ \db -> getNextDeliveryTasks db gInfo task) $ \nextTasks -> do
let (body_, acceptedTasks, largeTasks) = batchDeliveryTasks1 (vr cxt) maxEncodedMsgLength nextTasks
let (body_, acceptedTasks, largeTasks) = batchDeliveryTasks1 (vr cxt) maxForwardBatchLength nextTasks
senderGMIds = S.toList . S.fromList $ map (\MessageDeliveryTask {senderGMId} -> senderGMId) acceptedTasks
withStore' $ \db -> do
forM_ body_ $ \body -> createMsgDeliveryJob db gInfo jobScope senderGMIds body
@@ -4290,8 +4292,8 @@ runDeliveryJobWorker a deliveryKey Worker {doWork} = do
else do
-- all members' profiles disseminate; privileged key/role come from the roster, not here
let (encoderErrs, validLabeled) = partitionEithers [(\bs -> (s, bs)) <$> encodeMemberNew (vr cxt) gInfo s | (s, _) <- senders]
(extBody', inBody, overflowLabeled, large1) = batchProfilesWithBody maxEncodedMsgLength body validLabeled
(overflowBatches', large2) = batchProfiles maxEncodedMsgLength overflowLabeled
(extBody', inBody, overflowLabeled, large1) = batchProfilesWithBody maxForwardBatchLength body validLabeled
(overflowBatches', large2) = batchProfiles maxForwardBatchLength overflowLabeled
packerErrs = [ChatError (CEInternalError $ "oversized profile element for member " <> show (groupMemberId' s)) | s <- large1 <> large2]
allErrs = encoderErrs <> packerErrs
unless (null allErrs) $ do
+5 -1
View File
@@ -525,6 +525,7 @@ data UserServersError
| USEStorageMissing {protocol :: AProtocolType, user :: Maybe User}
| USEProxyMissing {protocol :: AProtocolType, user :: Maybe User}
| USEDuplicateServer {protocol :: AProtocolType, duplicateServer :: Text, duplicateHost :: TransportHost}
| USETooManyHosts {protocol :: AProtocolType, tooManyHostsServer :: Text}
| USEDuplicateChatRelayAddress {duplicateChatRelay :: Text, duplicateAddress :: ShortLinkContact}
deriving (Show)
@@ -547,13 +548,16 @@ validateUserServers curr others = (currUserErrs <> concatMap otherUserErrs other
noServers cond = not $ any srvEnabled $ userServers p $ filter cond uss
srvEnabled (AUS _ UserServer {deleted, enabled}) = enabled && not deleted
serverErrs :: (UserServersClass u, ProtocolTypeI p, UserProtocol p) => SProtocolType p -> [u] -> [UserServersError]
serverErrs p uss = mapMaybe duplicateErr_ srvs
serverErrs p uss = mapMaybe duplicateErr_ srvs <> hostsErrs
where
p' = AProtocolType p
srvs = filter (\(AUS _ UserServer {deleted}) -> not deleted) $ userServers p uss
duplicateErr_ (AUS _ srv@UserServer {server}) =
USEDuplicateServer p' (safeDecodeUtf8 $ strEncode server)
<$> find (`S.member` duplicateHosts) (srvHost srv)
hostsErrs = case p of
SPSMP -> [USETooManyHosts p' (safeDecodeUtf8 $ strEncode server) | AUS _ srv@UserServer {server} <- srvs, L.length (srvHost srv) > 2]
_ -> []
duplicateHosts = snd $ foldl' addDuplicate (S.empty, S.empty) allHosts
allHosts = concatMap (\(AUS _ srv) -> L.toList $ srvHost srv) srvs
userServers :: (UserServersClass u, UserProtocol p) => SProtocolType p -> [u] -> [AUserServer p]
+10 -2
View File
@@ -924,6 +924,14 @@ $(JQ.deriveJSON defaultJSON ''MsgContainer)
maxEncodedMsgLength :: Int
maxEncodedMsgLength = 15602
maxForwardBatchLength :: Int
maxForwardBatchLength = maxEncodedMsgLength + 161
fwdMemberName :: ContactName -> ContactName
fwdMemberName displayName
| T.length displayName <= 16 = displayName
| otherwise = T.take 16 displayName `T.snoc` '…'
-- maxEncodedMsgLength - 2222, see e2eEncUserMsgLength in agent
maxEncodedMsgLengthPQ :: Int
maxEncodedMsgLengthPQ = 13380
@@ -965,8 +973,8 @@ rosterBlobP = do
maxEncodedInfoLength :: Int
maxEncodedInfoLength = 14694
maxEncodedInfoLengthPQ :: Int
maxEncodedInfoLengthPQ = 10968 -- maxEncodedInfoLength - 3726, see e2eEncConnInfoLength in agent
maxEncodedProfileMsgLength :: Int
maxEncodedProfileMsgLength = 16384
data EncodedChatMessage = ECMEncoded ByteString | ECMLarge
+1 -1
View File
@@ -156,7 +156,7 @@ getMsgDeliveryTask_ db taskId =
toTask ((Only taskId') :. jobScopeRow :. (senderGMId, senderMemberId, senderMemberName, brokerTs, Binary msgBody, chatBinding_, sigs_, BI showGroupAsSender)) =
case (toJobScope_ jobScopeRow, J.eitherDecodeStrict' msgBody) of
(Just jobScope, Right chatMsg) ->
let fwdSender = if showGroupAsSender && isNothing chatBinding_ then FwdChannel else FwdMember senderMemberId senderMemberName
let fwdSender = if showGroupAsSender && isNothing chatBinding_ then FwdChannel else FwdMember senderMemberId (fwdMemberName senderMemberName)
-- Re-parsed from msg_body: validates stored content against current code.
-- Signed: original bytes preserved (re-encoding would invalidate signature).
-- Unsigned: re-encoded from parsed ChatMessage on forward (sanitizes content).
+7
View File
@@ -1158,6 +1158,13 @@ testGetSetSMPServers =
alice <## " SMP servers"
alice <## " smp://2345-w==@smp2.example.im"
alice <## " smp://3456-w==@smp3.example.im:5224"
alice #$> ("/smp smp://2345-w==@smp2.example.im,smp4.example.im", id, "ok")
alice ##> "/smp smp://2345-w==@smp2.example.im,smp4.example.im,smp5.example.im"
alice <##. "bad chat command: user servers validation error(s): [USETooManyHosts"
alice ##> "/smp"
alice <## "Your servers"
alice <## " SMP servers"
alice <## " smp://2345-w==@smp2.example.im,smp4.example.im"
testTestSMPServerConnection :: HasCallStack => TestParams -> IO ()
testTestSMPServerConnection =
+58 -1
View File
@@ -38,7 +38,7 @@ import Simplex.Chat.Messages (CIMention (..), CIMentionMember (..), ChatItemId)
import Simplex.Chat.Messages.Batch (encodeBinaryBatch, encodeFwdElement)
import Simplex.Chat.Messages.CIContent (publicGroupNoE2EText)
import Simplex.Chat.Options
import Simplex.Chat.Protocol (ChatMessage (ChatMessage), ChatMsgEvent (XGrpMemNew, XMsgUpdate, XMsgNew, XMsgDel), FwdSender (FwdMember, FwdChannel), GrpMsgForward (GrpMsgForward), MsgContainer (..), MsgMention (..), MsgContent (..), VerifiedMsg (VMUnsigned), mcSimple, msgContentText)
import Simplex.Chat.Protocol (ChatMessage (ChatMessage), ChatMsgEvent (XGrpMemNew, XInfo, XMsgUpdate, XMsgNew, XMsgDel), FwdSender (FwdMember, FwdChannel), GrpMsgForward (GrpMsgForward), MsgContainer (..), MsgMention (..), MsgContent (..), VerifiedMsg (VMUnsigned), mcSimple, msgContentText)
import Simplex.Chat.Types
import Simplex.Chat.Types.MemberRelations (MemberRelation (..), getRelation, setRelation)
import Simplex.Chat.Types.Shared (GroupMemberRole (..), GroupAcceptance (..))
@@ -119,6 +119,7 @@ chatGroupTests = do
xit "create and join group when clients go offline" testGroupAsync
describe "group links" $ do
it "create group link, join via group link" testGroupLink
it "join via group link with group profile near the size limit" testGroupLinkLargeGroupProfile
it "invitees were previously connected as contacts" testGroupLinkInviteesWereConnected
it "all members were previously connected as contacts" testGroupLinkAllMembersWereConnected
it "delete group, re-join via same link" testGroupLinkDeleteGroupRejoin
@@ -176,6 +177,7 @@ chatGroupTests = do
it "manually accept contact with group member incognito" testMemberContactAcceptIncognito
describe "group message forwarding" $ do
it "forward messages between invitee and introduced (x.msg.new)" testGroupMsgForwardMessage
it "reject forwarded content attributed to own membership" testGroupMsgForwardOwnMembershipRejected
it "forward batched messages" testGroupMsgForwardBatched
it "forward reports to moderators, don't forward to members (x.msg.new, MCReport)" testGroupMsgForwardReport
it "deduplicate forwarded messages" testGroupMsgForwardDeduplicate
@@ -3042,6 +3044,39 @@ testPlanGroupLinkLeaveRejoin =
bob <## "group link: known group #team_1"
bob <## "use #team_1 <message> to send messages"
testGroupLinkLargeGroupProfile :: HasCallStack => TestParams -> IO ()
testGroupLinkLargeGroupProfile =
testChatOpts2 testOptsNoFullLinks aliceProfile bobProfile $
\alice bob -> do
threadDelay 100000
alice ##> "/g team"
alice <## "group #team is created"
alice <## "to add members use /a team <name> or /create link #team"
alice ##> "/set history #team off"
alice <## "updated group preferences:"
alice <## "Recent history: off"
withCCTransaction alice $ \db ->
DB.execute db "UPDATE group_profiles SET description = ? WHERE display_name = ?" (T.pack welcome, "team" :: T.Text)
alice ##> "/create link #team"
gLink <- getGroupLink_ alice "team" GRMember True
alice <// 100000
bob ##> ("/c " <> gLink)
bob <## "connection request sent!"
alice <## "bob (Bob): accepting request to join group #team..."
concurrentlyN_
[ alice <## "#team: bob joined the group",
do
bob <## "#team: joining the group..."
bob <## "#team: you joined the group"
]
alice #> "#team hello"
let waitHello = do
l <- getTermLine bob
if "alice> hello" `isSuffixOf` l then pure () else waitHello
waitHello
where
welcome = unwords $ replicate 1775 "welcome"
testGroupLink :: HasCallStack => TestParams -> IO ()
testGroupLink =
testChatOpts3 testOptsNoFullLinks aliceProfile bobProfile cathProfile $
@@ -5399,6 +5434,28 @@ testGroupMsgForwardMessage =
cath <# "#team bob> hi there [>>]"
cath <# "#team hey team"
testGroupMsgForwardOwnMembershipRejected :: HasCallStack => TestParams -> IO ()
testGroupMsgForwardOwnMembershipRejected =
testChat2 aliceProfile bobProfile $
\alice bob -> do
createGroup2 "team" alice bob
threadDelay 1000000
[Only bobMemId] <- withCCTransaction bob $ \db ->
DB.query db "SELECT member_id FROM group_members WHERE member_category = ?" (Only ("user" :: T.Text)) :: IO [Only ByteString]
connId <- relayConnIdToMember alice "bob"
ts <- getCurrentTime
let ChatController {smpAgent = aliceAgent} = chatController alice
forgedProfile = (bobProfile :: Profile) {displayName = "mallory", fullName = "Mallory"}
chatMsg = ChatMessage chatInitialVRange (Just $ SharedMsgId "forged_info1") (XInfo forgedProfile Nothing)
fwd = GrpMsgForward (FwdMember (MemberId bobMemId) "bob") ts
body = encodeBinaryBatch [encodeFwdElement fwd (VMUnsigned chatMsg)]
sent <- runExceptT $ sendMessages aliceAgent [(connId, PQEncOff, MsgFlags False, vrValue body)]
either (fail . show) (const $ pure ()) sent
bob <##. "error: x.grp.msg.forward: content attributed to own membership"
bob ##> "/p"
bob <## "user profile: bob (Bob)"
bob <## "use /p <name> [<bio>] to change it"
testGroupMsgForwardBatched :: HasCallStack => TestParams -> IO ()
testGroupMsgForwardBatched =
testChat3 aliceProfile bobProfile cathProfile $
+7
View File
@@ -33,6 +33,7 @@ import Simplex.Chat.Protocol
GrpMsgForward (GrpMsgForward),
MsgContent (MCText),
VerifiedMsg (VMUnsigned),
fwdMemberName,
maxBatchElementCount,
maxEncodedMsgLength,
mcSimple,
@@ -49,6 +50,7 @@ batchingTests = describe "message batching tests" $ do
it "splits a batch that exceeds the element count limit" testBatchElementCountLimit
it "does not create a relay delivery body when every task is oversized" testRelayBatchAllLarge
it "classifies a task that fits raw but not as a framed singleton as large" testRelayBatchSingletonOverflow
it "shortens forwarded member names" testFwdMemberName
instance IsString SndMessage where
fromString s = SndMessage {msgId, sharedMsgId = SharedMsgId "", msgBody = s', signedMsg_ = Nothing}
@@ -190,6 +192,11 @@ testRelayBatchSingletonOverflow = do
map deliveryTaskId accepted `shouldBe` []
map deliveryTaskId large `shouldBe` [1]
testFwdMemberName :: IO ()
testFwdMemberName = do
fwdMemberName "sixteen_chars_ab" `shouldBe` "sixteen_chars_ab"
fwdMemberName "seventeen_chars_a" `shouldBe` "seventeen_chars_…"
runBatcherTest :: BatchMode -> Int -> [SndMessage] -> [ChatError] -> [ByteString] -> Spec
runBatcherTest mode maxLen msgs expectedErrors expectedBatches =
it