From 912106470958bf6611e6d628ecb3414f98f06337 Mon Sep 17 00:00:00 2001 From: DawnFire42 Date: Mon, 10 Aug 2026 16:27:13 -0400 Subject: [PATCH] Extract evaluateCreateCounter helper to directly test reserved-name guard in cmdCreateCounter --- .../server/remote/game/server_player.cpp | 20 +++++++++--- .../server/remote/game/server_player.h | 14 ++++++++ .../counter_command_auth_test.cpp | 32 +++++++++++++------ 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp index 9622bd0bb..42d382733 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp @@ -495,22 +495,32 @@ Server_Player::cmdIncCounter(const Command_IncCounter &cmd, ResponseContainer & } Response::ResponseCode -Server_Player::cmdCreateCounter(const Command_CreateCounter &cmd, ResponseContainer & /*rc*/, GameEventStorage &ges) +Server_Player::evaluateCreateCounter(bool gameStarted, bool playerConceded, const QString &counterName) { - if (!game->getGameStarted()) { + if (!gameStarted) { return Response::RespGameNotStarted; } - if (conceded) { + if (playerConceded) { return Response::RespContextError; } - - const QString counterName = nameFromStdString(cmd.counter_name()); // Reserved system counter names (commander/partner tax) are how clients identify // server-managed tax counters for rendering and logging; a client must not be able // to spoof one via a user-created counter. if (CounterNames::isTaxCounter(counterName)) { return Response::RespFunctionNotAllowed; } + return Response::RespOk; +} + +Response::ResponseCode +Server_Player::cmdCreateCounter(const Command_CreateCounter &cmd, ResponseContainer & /*rc*/, GameEventStorage &ges) +{ + const QString counterName = nameFromStdString(cmd.counter_name()); + + const Response::ResponseCode authResult = evaluateCreateCounter(game->getGameStarted(), conceded, counterName); + if (authResult != Response::RespOk) { + return authResult; + } auto *c = new Server_Counter(newCounterId(), counterName, cmd.counter_color(), cmd.radius(), cmd.value()); addCounter(c); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h index 21f288b1d..2b2be7fe1 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.h @@ -86,6 +86,20 @@ public: int counterId, const Server_Counter *counter); + /** + * @brief Decide whether a client may create a counter with the given name. + * + * Reserved system counter names (commander/partner tax) are rejected to prevent + * clients from spoofing server-managed tax counters. + * + * @param gameStarted Whether the game has started + * @param playerConceded Whether the player has conceded + * @param counterName Name requested for the new counter + * @return Response::RespOk if permitted, otherwise the error response for the client. + */ + static Response::ResponseCode + evaluateCreateCounter(bool gameStarted, bool playerConceded, const QString &counterName); + /** @} */ void setupZones() override; diff --git a/tests/command_zone_tests/counter_command_auth_test.cpp b/tests/command_zone_tests/counter_command_auth_test.cpp index 8214efd12..e7905428d 100644 --- a/tests/command_zone_tests/counter_command_auth_test.cpp +++ b/tests/command_zone_tests/counter_command_auth_test.cpp @@ -150,23 +150,37 @@ TEST(EvaluateSetCounterActive, RejectsDisablingPartnerTaxWhenAccumulated) Response::RespContextError); } -// CounterNames::isTaxCounter (guards cmdCreateCounter against reserved names) +// evaluateCreateCounter -TEST(CounterNamesIsTaxCounter, RejectsCommanderTaxName) +TEST(EvaluateCreateCounter, RejectsWhenGameNotStarted) { - EXPECT_TRUE(CounterNames::isTaxCounter(CounterNames::CommanderTax)); + EXPECT_EQ(Server_Player::evaluateCreateCounter(/*gameStarted=*/false, /*playerConceded=*/false, "mycounter"), + Response::RespGameNotStarted); } -TEST(CounterNamesIsTaxCounter, RejectsPartnerTaxName) +TEST(EvaluateCreateCounter, RejectsWhenPlayerConceded) { - EXPECT_TRUE(CounterNames::isTaxCounter(CounterNames::PartnerTax)); + EXPECT_EQ(Server_Player::evaluateCreateCounter(/*gameStarted=*/true, /*playerConceded=*/true, "mycounter"), + Response::RespContextError); } -TEST(CounterNamesIsTaxCounter, AllowsOrdinaryName) +TEST(EvaluateCreateCounter, RejectsCommanderTaxName) { - EXPECT_FALSE(CounterNames::isTaxCounter("life")); - EXPECT_FALSE(CounterNames::isTaxCounter("poison")); - EXPECT_FALSE(CounterNames::isTaxCounter("")); + EXPECT_EQ(Server_Player::evaluateCreateCounter(true, false, CounterNames::CommanderTax), + Response::RespFunctionNotAllowed); +} + +TEST(EvaluateCreateCounter, RejectsPartnerTaxName) +{ + EXPECT_EQ(Server_Player::evaluateCreateCounter(true, false, CounterNames::PartnerTax), + Response::RespFunctionNotAllowed); +} + +TEST(EvaluateCreateCounter, AllowsOrdinaryNames) +{ + EXPECT_EQ(Server_Player::evaluateCreateCounter(true, false, "life"), Response::RespOk); + EXPECT_EQ(Server_Player::evaluateCreateCounter(true, false, "poison"), Response::RespOk); + EXPECT_EQ(Server_Player::evaluateCreateCounter(true, false, ""), Response::RespOk); } // evaluateModifyCounter (shared by cmdIncCounter / cmdSetCounter)