From b0dd2ef466e24e0e16547b8c5d92285094bd3bc7 Mon Sep 17 00:00:00 2001 From: Sven Tennie Date: Thu, 30 Jul 2026 16:38:33 +0200 Subject: [PATCH 1/3] Add (potential) reproducer test --- integration/integration.cabal | 1 + .../test/Test/MixedConversationReproducer.hs | 164 ++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 integration/test/Test/MixedConversationReproducer.hs diff --git a/integration/integration.cabal b/integration/integration.cabal index 92abb7662b7..91b864c3fb2 100644 --- a/integration/integration.cabal +++ b/integration/integration.cabal @@ -182,6 +182,7 @@ library Test.Migration.DomainRegistration Test.Migration.TeamFeatures Test.Migration.Util + Test.MixedConversationReproducer Test.MLS Test.MLS.Clients Test.MLS.History diff --git a/integration/test/Test/MixedConversationReproducer.hs b/integration/test/Test/MixedConversationReproducer.hs new file mode 100644 index 00000000000..da4977abb79 --- /dev/null +++ b/integration/test/Test/MixedConversationReproducer.hs @@ -0,0 +1,164 @@ +{-# OPTIONS_GHC -Wno-ambiguous-fields #-} + +-- This file is part of the Wire Server implementation. +-- +-- Copyright (C) 2026 Wire Swiss GmbH +-- +-- This program is free software: you can redistribute it and/or modify it under +-- the terms of the GNU Affero General Public License as published by the Free +-- Software Foundation, either version 3 of the License, or (at your option) any +-- later version. +-- +-- This program is distributed in the hope that it will be useful, but WITHOUT +-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +-- details. +-- +-- You should have received a copy of the GNU Affero General Public License along +-- with this program. If not, see . + +module Test.MixedConversationReproducer where + +import API.Brig (addClient) +import API.Galley +import Control.Lens ((.~), (^?!)) +import qualified Data.Aeson as A +import qualified Data.ProtoLens as Proto +import Data.ProtoLens.Labels () +import MLS.Util (createMLSClient) +import Notifications +import Numeric.Lens (hex) +import qualified Proto.Otr as Proto +import qualified Proto.Otr_Fields as Proto +import SetupHelpers +import Testlib.Prelude + +-- copied from Test.FeatureFlags.MlsMigration to keep this reproducer self-contained +mlsEnableConfig :: Value +mlsEnableConfig = + object + [ "protocolToggleUsers" .= ([] :: [String]), + "defaultProtocol" .= "mls", + "supportedProtocols" .= ["mls"], + "allowedCipherSuites" .= ([1] :: [Int]), + "defaultCipherSuite" .= A.Number 1 + ] + +mlsEnable :: Value +mlsEnable = + object + [ "status" .= "enabled", + "config" .= mlsEnableConfig + ] + +-- | Reproducer for bug-report.md: "Not receiving messages in mixed and +-- migrated groups with MLS protocol if hosted on another backend". +-- +-- Steps, following bug-report.md: +-- 1. Team A on backend A: user A (admin), user C (no MLS device) +-- 2. Team B on backend B: user B (admin) +-- 3. Connect A<->B, C<->B +-- 4. User B creates a *proteus* group with A and C +-- 5. Enable MLS for team A and team B +-- 6. Refresh/create clients for A and B (C intentionally stays without an +-- MLS client, so the group cannot fully migrate to MLS) +-- 7. Start migration for team B (protocol -> mixed), don't finalise +-- 8. Start migration for team A too, don't finalise +-- 9. Send a message as B, assert A receives it +-- 10. Send a message as A, assert B receives it +-- +-- Per bug-report.md's "Additional Notes", step 10 is where the asymmetry +-- is expected to show: B stops receiving messages sent by A in this +-- conversation, even though A keeps receiving messages sent by B. +testFederatedMixedProtocolMessageDelivery :: (HasCallStack) => Domain -> App () +testFederatedMixedProtocolMessageDelivery secondDomain = do + -- Step 1: Team A on backend A with user A (admin) and user C + (userA, tidA, _) <- createTeam OwnDomain 1 + userC <- randomUser OwnDomain def + + -- Step 2: Team B on backend B with user B (admin) + (userB, tidB, _) <- createTeam secondDomain 1 + + -- Step 3: connect everyone + connectUsers [userA, userB, userC] + + -- legacy proteus clients, used to send/receive proteus messages + clientA <- addClient userA def >>= getJSON 201 >>= objId + clientB <- addClient userB def >>= getJSON 201 >>= objId + clientC <- addClient userC def >>= getJSON 201 >>= objId + + -- Step 4: User B creates a proteus group with A and C + convId <- + postConversation + userB + defProteus {qualifiedUsers = [userA, userC], team = Just tidB} + >>= getJSON 201 + >>= objConvId + + bindResponse (getConversation userB (convIdToQidObject convId)) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "protocol" `shouldMatch` "proteus" + + -- Step 5: enable the MLS team feature for both teams + void $ setTeamFeatureConfig userA tidA "mls" mlsEnable >>= getJSON 200 + void $ setTeamFeatureConfig userB tidB "mls" mlsEnable >>= getJSON 200 + + -- Step 6: refresh/create MLS clients for A and B; C stays without one so + -- the conversation cannot fully migrate to MLS and remains mixed. + void $ createMLSClient def userA + void $ createMLSClient def userB + + -- Step 7: start migration for team B (creator/owner of the conversation) + bindResponse (putConversationProtocol userB convId "mixed") $ \resp -> + resp.status `shouldMatchInt` 200 + + -- Step 8: start migration for team A too, without finalising + bindResponse (putConversationProtocol userA convId "mixed") $ \resp -> + resp.status `shouldMatchOneOf` [Number 200, Number 204] + + -- The conversation should now be mixed protocol from both sides + bindResponse (getConversation userA (convIdToQidObject convId)) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "protocol" `shouldMatch` "mixed" + bindResponse (getConversation userB (convIdToQidObject convId)) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "protocol" `shouldMatch` "mixed" + + -- Step 9: B sends a message; A should receive it + -- userA and userC are both on OwnDomain, so they share one recipient entry. + withWebSocket userA $ \wsA -> do + msgFromB <- + mkProteusRecipients + userA + [(userA, [clientA]), (userC, [clientC])] + "message from B" + let protoMsgFromB = + Proto.defMessage @Proto.QualifiedNewOtrMessage + & #sender . Proto.client .~ (clientB ^?! hex) + & #recipients .~ [msgFromB] + & #ignoreAll .~ Proto.defMessage + bindResponse (postProteusMessage userB (convIdToQidObject convId) protoMsgFromB) $ \resp -> + resp.status `shouldMatchInt` 201 + + n <- awaitMatch isNewMessageNotif wsA + n %. "payload.0.qualified_conversation" `shouldMatch` convIdToQidObject convId + + -- Step 10: A sends a message; B should receive it -- expected failure + -- point for the bug being reproduced. + -- userB is on secondDomain, userC is on OwnDomain -- different domains, + -- so each needs its own qualified recipient entry. + withWebSocket userB $ \wsB -> do + msgFromAToB <- mkProteusRecipients userB [(userB, [clientB])] "message from A" + msgFromAToC <- mkProteusRecipients userC [(userC, [clientC])] "message from A" + let protoMsgFromA = + Proto.defMessage @Proto.QualifiedNewOtrMessage + & #sender . Proto.client .~ (clientA ^?! hex) + & #recipients .~ [msgFromAToB, msgFromAToC] + & #ignoreAll .~ Proto.defMessage + bindResponse (postProteusMessage userA (convIdToQidObject convId) protoMsgFromA) $ \resp -> + resp.status `shouldMatchInt` 201 + + -- If the bug is present, this awaitMatch times out, proving the + -- asymmetric delivery failure described in bug-report.md. + n <- awaitMatch isNewMessageNotif wsB + n %. "payload.0.qualified_conversation" `shouldMatch` convIdToQidObject convId From 5abf908300db485d2c59daf31ed482e54aabd1ac Mon Sep 17 00:00:00 2001 From: Sven Tennie Date: Thu, 30 Jul 2026 18:19:41 +0200 Subject: [PATCH 2/3] Potential fix --- libs/wire-api/src/Wire/API/Message.hs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Message.hs b/libs/wire-api/src/Wire/API/Message.hs index 183c2a272b7..9aa1ea2e2c5 100644 --- a/libs/wire-api/src/Wire/API/Message.hs +++ b/libs/wire-api/src/Wire/API/Message.hs @@ -366,10 +366,10 @@ protolensOtrRecipientsToOtrRecipients entries = QualifiedOtrRecipients . QualifiedUserClientMap <$> protolensToQualifiedUCMap entries where protolensToQualifiedUCMap :: [Proto.Otr.QualifiedUserEntry] -> Either String (Map Domain (Map UserId (Map ClientId ByteString))) - protolensToQualifiedUCMap qualifiedEntries = parseMap (mkDomain . view Proto.Otr.domain) (protolensToUCMap . view Proto.Otr.entries) qualifiedEntries + protolensToQualifiedUCMap qualifiedEntries = parseMapMerge (mkDomain . view Proto.Otr.domain) (protolensToUCMap . view Proto.Otr.entries) qualifiedEntries protolensToUCMap :: [Proto.Otr.UserEntry] -> Either String (Map UserId (Map ClientId ByteString)) - protolensToUCMap es = parseMap parseUserId parseClientMap es + protolensToUCMap es = parseMapMerge parseUserId parseClientMap es parseUserId :: Proto.Otr.UserEntry -> Either String UserId parseUserId = @@ -413,6 +413,15 @@ qualifiedOtrRecipientsToProtolens (QualifiedOtrRecipients (QualifiedUserClientMa parseMap :: (Applicative f, Ord k) => (a -> f k) -> (a -> f v) -> [a] -> f (Map k v) parseMap keyParser valueParser xs = Map.fromList <$> traverse (\x -> (,) <$> keyParser x <*> valueParser x) xs +-- | Like 'parseMap', but for values that are themselves maps: entries sharing +-- a key are merged instead of the last one silently clobbering the rest. A +-- well-formed request can legitimately contain multiple protobuf entries for +-- the same key (e.g. two 'QualifiedUserEntry's for the same domain), and +-- 'Map.fromList' (used by 'parseMap') would otherwise silently drop every +-- recipient from all but the last entry for that key. +parseMapMerge :: (Applicative f, Ord k, Ord k2) => (a -> f k) -> (a -> f (Map k2 v)) -> [a] -> f (Map k (Map k2 v)) +parseMapMerge keyParser valueParser xs = Map.fromListWith Map.union <$> traverse (\x -> (,) <$> keyParser x <*> valueParser x) xs + -------------------------------------------------------------------------------- -- Filter From ed05ee7e0b86dec75adfcafa22549e5f1344a7d2 Mon Sep 17 00:00:00 2001 From: Sven Tennie Date: Thu, 30 Jul 2026 18:50:14 +0200 Subject: [PATCH 3/3] Add unit test to understand the bug better --- libs/wire-api/src/Wire/API/Message.hs | 4 + .../test/unit/Test/Wire/API/Message.hs | 82 +++++++++++++++++++ libs/wire-api/test/unit/Test/Wire/API/Run.hs | 2 + libs/wire-api/wire-api.cabal | 1 + 4 files changed, 89 insertions(+) create mode 100644 libs/wire-api/test/unit/Test/Wire/API/Message.hs diff --git a/libs/wire-api/src/Wire/API/Message.hs b/libs/wire-api/src/Wire/API/Message.hs index 9aa1ea2e2c5..c5aaa098bac 100644 --- a/libs/wire-api/src/Wire/API/Message.hs +++ b/libs/wire-api/src/Wire/API/Message.hs @@ -56,6 +56,10 @@ module Wire.API.Message UserClients (..), ReportMissing (..), IgnoreMissing (..), + + -- * Only exported for tests + parseMap, + parseMapMerge, ) where diff --git a/libs/wire-api/test/unit/Test/Wire/API/Message.hs b/libs/wire-api/test/unit/Test/Wire/API/Message.hs new file mode 100644 index 00000000000..8eb516ac642 --- /dev/null +++ b/libs/wire-api/test/unit/Test/Wire/API/Message.hs @@ -0,0 +1,82 @@ +-- This file is part of the Wire Server implementation. +-- +-- Copyright (C) 2026 Wire Swiss GmbH +-- +-- This program is free software: you can redistribute it and/or modify it under +-- the terms of the GNU Affero General Public License as published by the Free +-- Software Foundation, either version 3 of the License, or (at your option) any +-- later version. +-- +-- This program is distributed in the hope that it will be useful, but WITHOUT +-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +-- details. +-- +-- You should have received a copy of the GNU Affero General Public License along +-- with this program. If not, see . + +module Test.Wire.API.Message (tests) where + +import Data.Map qualified as Map +import Data.Set qualified as Set +import Imports +import Test.Tasty +import Test.Tasty.HUnit +import Wire.API.Message (parseMap, parseMapMerge) + +tests :: TestTree +tests = + testGroup + "Message" + [ testParseMapDropsRecipientsOnDuplicateDomain, + testParseMapMergeKeepsRecipientsOnDuplicateDomain + ] + +-- | Simulates the shape of a real OTR request: a list of qualified entries, +-- where each entry maps a "domain" to a set of "users" it targets. Nothing in +-- the wire protocol forbids two entries for the same domain -- e.g. a client +-- assembling a mixed-protocol proteus fallback message may independently emit +-- an entry for "recipients not yet migrated to MLS" and another for +-- "recipients needing legacy delivery for some other reason", both of which +-- can legitimately target the same domain. +sameDomainEntries :: [(String, Map String (Set Int))] +sameDomainEntries = + [ ("a.example.com", Map.fromList [("alice", Set.fromList [1])]), + ("a.example.com", Map.fromList [("bob", Set.fromList [2])]), + ("b.example.com", Map.fromList [("carl", Set.fromList [3])]) + ] + +-- | This is the bug: 'parseMap' is built on 'Map.fromList', which is +-- last-write-wins on duplicate keys. The second "a.example.com" entry +-- (bob) silently replaces the first (alice) instead of being combined with +-- it -- alice's recipients vanish with no error, no missing-clients report, +-- nothing. This is exactly how messages went missing for real users in +-- mixed-protocol conversations (see bug-report.md). +testParseMapDropsRecipientsOnDuplicateDomain :: TestTree +testParseMapDropsRecipientsOnDuplicateDomain = + testCase "parseMap silently drops earlier entries for a repeated domain" $ do + let result :: Either String (Map String (Map String (Set Int))) + result = parseMap (Right . fst) (Right . snd) sameDomainEntries + result + @?= Right + ( Map.fromList + [ ("a.example.com", Map.fromList [("bob", Set.fromList [2])]), -- alice is gone! + ("b.example.com", Map.fromList [("carl", Set.fromList [3])]) + ] + ) + +-- | 'parseMapMerge' is the fix: it uses 'Map.fromListWith Map.union', so +-- entries for a repeated domain are merged instead of one clobbering the +-- other. Both alice and bob end up reachable under "a.example.com". +testParseMapMergeKeepsRecipientsOnDuplicateDomain :: TestTree +testParseMapMergeKeepsRecipientsOnDuplicateDomain = + testCase "parseMapMerge merges entries for a repeated domain" $ do + let result :: Either String (Map String (Map String (Set Int))) + result = parseMapMerge (Right . fst) (Right . snd) sameDomainEntries + result + @?= Right + ( Map.fromList + [ ("a.example.com", Map.fromList [("alice", Set.fromList [1]), ("bob", Set.fromList [2])]), + ("b.example.com", Map.fromList [("carl", Set.fromList [3])]) + ] + ) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Run.hs b/libs/wire-api/test/unit/Test/Wire/API/Run.hs index cf0f89456c3..01c3894fcd2 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Run.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Run.hs @@ -24,6 +24,7 @@ import Test.Wire.API.Call.Config qualified as Call.Config import Test.Wire.API.Conversation qualified as Conversation import Test.Wire.API.MLS qualified as MLS import Test.Wire.API.MLS.Group qualified as Group +import Test.Wire.API.Message qualified as Message import Test.Wire.API.OAuth qualified as OAuth import Test.Wire.API.RawJson qualified as RawJson import Test.Wire.API.Roundtrip.Aeson qualified as Roundtrip.Aeson @@ -63,6 +64,7 @@ main = Roundtrip.CSV.tests, Routes.tests, Conversation.tests, + Message.tests, MLS.tests, Group.tests, Routes.Version.tests, diff --git a/libs/wire-api/wire-api.cabal b/libs/wire-api/wire-api.cabal index 6d607ffedf1..9cb8dfc3981 100644 --- a/libs/wire-api/wire-api.cabal +++ b/libs/wire-api/wire-api.cabal @@ -713,6 +713,7 @@ test-suite wire-api-tests Paths_wire_api Test.Wire.API.Call.Config Test.Wire.API.Conversation + Test.Wire.API.Message Test.Wire.API.MLS Test.Wire.API.MLS.Group Test.Wire.API.OAuth