From 1e97ccf2b4e73954e9201820a4d56be630ddee96 Mon Sep 17 00:00:00 2001 From: Bhavay Goyal Date: Wed, 28 Oct 2026 20:56:00 +0530 Subject: [PATCH 1/2] Don't adopt unknown session IDs from the request cookie findSessionForRequest() passed any non-empty session cookie value to SessionManager::getSession(), which created a session under that key if none existed. A client could therefore pick its own session IDs: session fixation, unbounded growth of the session cache, and keys chosen to collide in the cache's unordered_map. When the ID comes from the cookie and does not name a live session, create the session under a new server-generated ID instead. It is marked to be sent to the client, so the cookie is replaced on the response. --- lib/src/SessionManager.cc | 13 +++++++ lib/tests/CMakeLists.txt | 1 + lib/tests/unittests/SessionManagerTest.cc | 44 +++++++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 lib/tests/unittests/SessionManagerTest.cc diff --git a/lib/src/SessionManager.cc b/lib/src/SessionManager.cc index 39c9682f3b..2cd3fb1d4c 100644 --- a/lib/src/SessionManager.cc +++ b/lib/src/SessionManager.cc @@ -94,6 +94,19 @@ SessionPtr SessionManager::getSession(const std::string &sessionID, { assert(!sessionID.empty()); SessionPtr sessionPtr; + if (!needToSet) + { + // The ID came from the request's cookie. Only reuse it when it names a + // live session; otherwise start a new session under a server-generated + // ID. Adopting the client's value would let anyone choose session keys + // (session fixation, unbounded cache growth, and chosen keys that all + // land in the same hash bucket). + if (sessionMapPtr_->findAndFetch(sessionID, sessionPtr) && sessionPtr) + { + return sessionPtr; + } + return getSession(idGeneratorCallback_(), true); + } sessionMapPtr_->modify( sessionID, [&sessionPtr, &sessionID, needToSet](SessionPtr &sessionInCache) { diff --git a/lib/tests/CMakeLists.txt b/lib/tests/CMakeLists.txt index 514e8692e7..5f95af65a8 100644 --- a/lib/tests/CMakeLists.txt +++ b/lib/tests/CMakeLists.txt @@ -26,6 +26,7 @@ set(UNITTEST_SOURCES unittests/HttpFullDateTest.cc unittests/MainLoopTest.cc unittests/CacheMapTest.cc + unittests/SessionManagerTest.cc unittests/StringOpsTest.cc unittests/ControllerCreationTest.cc unittests/MultiPartParserTest.cc diff --git a/lib/tests/unittests/SessionManagerTest.cc b/lib/tests/unittests/SessionManagerTest.cc new file mode 100644 index 0000000000..dbb9a0aa90 --- /dev/null +++ b/lib/tests/unittests/SessionManagerTest.cc @@ -0,0 +1,44 @@ +#include "../../lib/src/SessionManager.h" +#include +#include + +using namespace drogon; + +DROGON_TEST(SessionManagerTest) +{ + trantor::EventLoopThread loopThread; + loopThread.run(); + std::vector startAdvices; + std::vector destroyAdvices; + int generated = 0; + SessionManager manager(loopThread.getLoop(), + 0, + startAdvices, + destroyAdvices, + [&generated]() { + return "server-id-" + + std::to_string(++generated); + }); + + // A new session under a server-generated ID is stored. + auto session = manager.getSession("server-id-0", true); + CHECK(session->sessionId() == "server-id-0"); + + // The client sends that ID back: the same session is returned. + CHECK(manager.getSession("server-id-0", false) == session); + CHECK(generated == 0); + + // An ID the server never issued is not adopted: the client gets a new + // session with a fresh server-generated ID. + auto other = manager.getSession("attacker-chosen", false); + CHECK(other != session); + CHECK(other->sessionId() == "server-id-1"); + CHECK(generated == 1); + + // The new session is reachable under its own ID, and the unknown ID still + // isn't. + CHECK(manager.getSession("server-id-1", false) == other); + auto again = manager.getSession("attacker-chosen", false); + CHECK(again->sessionId() == "server-id-2"); + CHECK(again != other); +} From 51ca7f24b9bacac4a783f301e9bee034fceb2419 Mon Sep 17 00:00:00 2001 From: Bhavay Goyal Date: Thu, 29 Oct 2026 14:12:47 +0530 Subject: [PATCH 2/2] fix(session): export SessionManager and fix test formatting CI failures on the PR branch, both pre-existing in the test added here: - format: clang-format-17 reformats the lambda-taking constructor call in SessionManagerTest.cc to one line plus a hanging block, not the manually wrapped one it was written with. - windows/msvc SHARED: the unit test links against drogon.dll and calls SessionManager's constructor and getSession() directly, but SessionManager (lib/src, internal) was never marked DROGON_EXPORT, so those symbols aren't in the DLL's export table on a Windows shared build -- LNK2019/LNK1120. MultipartStreamParser and HttpResponseImpl, the other lib/src classes with direct unit tests, already use this pattern. Co-Authored-By: Claude Sonnet 5 --- lib/src/SessionManager.h | 3 ++- lib/tests/unittests/SessionManagerTest.cc | 12 ++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/src/SessionManager.h b/lib/src/SessionManager.h index 70f300e458..9b2a8d8dcc 100644 --- a/lib/src/SessionManager.h +++ b/lib/src/SessionManager.h @@ -14,6 +14,7 @@ #pragma once +#include #include #include #include @@ -27,7 +28,7 @@ namespace drogon { -class SessionManager : public trantor::NonCopyable +class DROGON_EXPORT SessionManager : public trantor::NonCopyable { public: using IdGeneratorCallback = std::function; diff --git a/lib/tests/unittests/SessionManagerTest.cc b/lib/tests/unittests/SessionManagerTest.cc index dbb9a0aa90..3e5d83d610 100644 --- a/lib/tests/unittests/SessionManagerTest.cc +++ b/lib/tests/unittests/SessionManagerTest.cc @@ -11,14 +11,10 @@ DROGON_TEST(SessionManagerTest) std::vector startAdvices; std::vector destroyAdvices; int generated = 0; - SessionManager manager(loopThread.getLoop(), - 0, - startAdvices, - destroyAdvices, - [&generated]() { - return "server-id-" + - std::to_string(++generated); - }); + SessionManager manager( + loopThread.getLoop(), 0, startAdvices, destroyAdvices, [&generated]() { + return "server-id-" + std::to_string(++generated); + }); // A new session under a server-generated ID is stored. auto session = manager.getSession("server-id-0", true);