From c28f843ef1b9ef070f2bdea15ec79a31287f40a3 Mon Sep 17 00:00:00 2001
From: Pierre Nicolas <pierre.nicolas@savoirfairelinux.com>
Date: Thu, 18 Jul 2024 13:46:16 -0400
Subject: [PATCH] conversation: refuse malformed uri
Second part of this fix: https://review.jami.net/c/jami-daemon/+/28337
GitLab: #1721
Change-Id: Ie428a57d89ac3db4e32660735cb3a5b0bf0f5c94
---
src/client/conversation_interface.cpp | 20 +++++++++++++++----
src/jamidht/conversation_module.cpp | 20 ++++++++++---------
src/jamidht/conversation_module.h | 4 ++--
test/unitTest/conversation/conversation.cpp | 2 +-
.../conversation/conversationMembersEvent.cpp | 6 +++---
5 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/src/client/conversation_interface.cpp b/src/client/conversation_interface.cpp
index ce751194c5..95ce7a6895 100644
--- a/src/client/conversation_interface.cpp
+++ b/src/client/conversation_interface.cpp
@@ -140,8 +140,14 @@ addConversationMember(const std::string& accountId,
const std::string& contactUri)
{
if (auto acc = jami::Manager::instance().getAccount<jami::JamiAccount>(accountId))
- if (auto convModule = acc->convModule(true))
- convModule->addConversationMember(conversationId, contactUri);
+ if (auto convModule = acc->convModule(true)) {
+ dht::InfoHash h(contactUri);
+ if (not h) {
+ JAMI_ERROR("addConversationMember: invalid contact URI `{}`", contactUri);
+ return;
+ }
+ convModule->addConversationMember(conversationId, h);
+ }
}
void
@@ -150,8 +156,14 @@ removeConversationMember(const std::string& accountId,
const std::string& contactUri)
{
if (auto acc = jami::Manager::instance().getAccount<jami::JamiAccount>(accountId))
- if (auto convModule = acc->convModule(true))
- convModule->removeConversationMember(conversationId, contactUri);
+ if (auto convModule = acc->convModule(true)) {
+ dht::InfoHash h(contactUri);
+ if (not h) {
+ JAMI_ERROR("removeConversationMember: invalid contact URI `{}`", contactUri);
+ return;
+ }
+ convModule->removeConversationMember(conversationId, h);
+ }
}
std::vector<std::map<std::string, std::string>>
diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp
index d5582154ec..d389cf777a 100644
--- a/src/jamidht/conversation_module.cpp
+++ b/src/jamidht/conversation_module.cpp
@@ -2596,7 +2596,7 @@ ConversationModule::fetchNewCommits(const std::string& peer,
void
ConversationModule::addConversationMember(const std::string& conversationId,
- const std::string& contactUri,
+ const dht::InfoHash& contactUri,
bool sendRequest)
{
auto conv = pimpl_->getConversation(conversationId);
@@ -2606,19 +2606,20 @@ ConversationModule::addConversationMember(const std::string& conversationId,
}
std::unique_lock lk(conv->mtx);
- if (conv->conversation->isMember(contactUri, true)) {
- JAMI_DEBUG("{:s} is already a member of {:s}, resend invite", contactUri, conversationId);
+ auto contactUriStr = contactUri.toString();
+ if (conv->conversation->isMember(contactUriStr, true)) {
+ JAMI_DEBUG("{:s} is already a member of {:s}, resend invite", contactUriStr, conversationId);
// Note: This should not be necessary, but if for whatever reason the other side didn't
// join we should not forbid new invites
auto invite = conv->conversation->generateInvitation();
lk.unlock();
- pimpl_->sendMsgCb_(contactUri, {}, std::move(invite), 0);
+ pimpl_->sendMsgCb_(contactUriStr, {}, std::move(invite), 0);
return;
}
conv->conversation->addMember(
- contactUri,
- [this, conv, conversationId, sendRequest, contactUri](bool ok, const std::string& commitId) {
+ contactUriStr,
+ [this, conv, conversationId, sendRequest, contactUriStr](bool ok, const std::string& commitId) {
if (ok) {
std::unique_lock lk(conv->mtx);
pimpl_->sendMessageNotification(*conv->conversation,
@@ -2627,7 +2628,7 @@ ConversationModule::addConversationMember(const std::string& conversationId,
if (sendRequest) {
auto invite = conv->conversation->generateInvitation();
lk.unlock();
- pimpl_->sendMsgCb_(contactUri, {}, std::move(invite), 0);
+ pimpl_->sendMsgCb_(contactUriStr, {}, std::move(invite), 0);
}
}
});
@@ -2635,14 +2636,15 @@ ConversationModule::addConversationMember(const std::string& conversationId,
void
ConversationModule::removeConversationMember(const std::string& conversationId,
- const std::string& contactUri,
+ const dht::InfoHash& contactUri,
bool isDevice)
{
+ auto contactUriStr = contactUri.toString();
if (auto conv = pimpl_->getConversation(conversationId)) {
std::lock_guard lk(conv->mtx);
if (conv->conversation)
return conv->conversation->removeMember(
- contactUri, isDevice, [this, conversationId](bool ok, const std::string& commitId) {
+ contactUriStr, isDevice, [this, conversationId](bool ok, const std::string& commitId) {
if (ok) {
pimpl_->sendMessageNotification(conversationId, true, commitId);
}
diff --git a/src/jamidht/conversation_module.h b/src/jamidht/conversation_module.h
index bfbd9462b9..546590b805 100644
--- a/src/jamidht/conversation_module.h
+++ b/src/jamidht/conversation_module.h
@@ -356,7 +356,7 @@ public:
* @param sendRequest If we need to inform the peer (used for tests)
*/
void addConversationMember(const std::string& conversationId,
- const std::string& contactUri,
+ const dht::InfoHash& contactUri,
bool sendRequest = true);
/**
* Remove a member from a conversation (this will trigger a member event + new message on success)
@@ -365,7 +365,7 @@ public:
* @param isDevice
*/
void removeConversationMember(const std::string& conversationId,
- const std::string& contactUri,
+ const dht::InfoHash& contactUri,
bool isDevice = false);
/**
* Get members
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index a9a0baef78..d893aa7581 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -781,7 +781,7 @@ ConversationTest::testMergeTwoDifferentHeads()
auto convId = libjami::startConversation(aliceId);
auto msgSize = aliceData.messages.size();
- aliceAccount->convModule()->addConversationMember(convId, carlaUri, false);
+ aliceAccount->convModule()->addConversationMember(convId, dht::InfoHash(carlaUri), false);
CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return aliceData.messages.size() == msgSize + 1; }));
// Cp conversations & convInfo
diff --git a/test/unitTest/conversation/conversationMembersEvent.cpp b/test/unitTest/conversation/conversationMembersEvent.cpp
index 15405b532e..7865729c6a 100644
--- a/test/unitTest/conversation/conversationMembersEvent.cpp
+++ b/test/unitTest/conversation/conversationMembersEvent.cpp
@@ -1105,7 +1105,7 @@ ConversationMembersEventTest::testMemberJoinsNoBadFile()
auto convId = libjami::startConversation(aliceId);
auto aliceMsgSize = aliceData.messages.size();
- aliceAccount->convModule()->addConversationMember(convId, carlaUri, false);
+ aliceAccount->convModule()->addConversationMember(convId, dht::InfoHash(carlaUri), false);
CPPUNIT_ASSERT(cv.wait_for(lk, 5s, [&] { return aliceMsgSize + 1 == aliceData.messages.size(); }));
// Cp conversations & convInfo
@@ -1164,7 +1164,7 @@ ConversationMembersEventTest::testMemberAddedNoCertificate()
auto convId = libjami::startConversation(aliceId);
auto aliceMsgSize = aliceData.messages.size();
- aliceAccount->convModule()->addConversationMember(convId, carlaUri, false);
+ aliceAccount->convModule()->addConversationMember(convId, dht::InfoHash(carlaUri), false);
CPPUNIT_ASSERT(cv.wait_for(lk, 5s, [&] { return aliceMsgSize + 1 == aliceData.messages.size(); }));
// Cp conversations & convInfo
@@ -1214,7 +1214,7 @@ ConversationMembersEventTest::testMemberJoinsInviteRemoved()
auto convId = libjami::startConversation(aliceId);
auto aliceMsgSize = aliceData.messages.size();
- aliceAccount->convModule()->addConversationMember(convId, carlaUri, false);
+ aliceAccount->convModule()->addConversationMember(convId, dht::InfoHash(carlaUri), false);
CPPUNIT_ASSERT(cv.wait_for(lk, 5s, [&] { return aliceMsgSize + 1 == aliceData.messages.size(); }));
// Cp conversations & convInfo
--
GitLab