diff --git a/include/session/client.hpp b/include/session/client.hpp index 24f3d3ef..0d2240b7 100644 --- a/include/session/client.hpp +++ b/include/session/client.hpp @@ -1015,6 +1015,7 @@ class Client { void _require_dm(std::string_view op, const ConversationId& id); void _require_contact(std::string_view op, const ConversationId& id); + void _require_page(std::string_view op, int limit); void _require_readable(const std::vector& attachments); // Everything a send must be able to reject before storing anything: the conversation kind, the diff --git a/include/session/client/conversation.hpp b/include/session/client/conversation.hpp index fc512ea4..05b27dc7 100644 --- a/include/session/client/conversation.hpp +++ b/include/session/client/conversation.hpp @@ -197,6 +197,11 @@ class Conversation { /// Filtered in the query rather than left to the caller, because the alternative breaks paging: /// a page of 50 that is mostly deleted would hand back a handful of rows with nothing to say /// that another page is warranted. + /// + /// @throws std::invalid_argument if `limit` is not positive. In particular there is no value + /// meaning "all of it" -- SQLite reads `LIMIT -1` that way, this does not. History grows + /// without bound, so reading all of it means paging with `before` until a page comes back + /// short. void messages(failable_function)> cb) const; void messages(int limit, failable_function)> cb) const; void messages( diff --git a/src/client/client.cpp b/src/client/client.cpp index 75bf7416..2dfd30b5 100644 --- a/src/client/client.cpp +++ b/src/client/client.cpp @@ -627,6 +627,14 @@ void Client::_require_contact(std::string_view op, const ConversationId& id) { throw std::invalid_argument{"{}: not applicable to your own account"_format(op)}; } +void Client::_require_page(std::string_view op, int limit) { + // The value reaches SQLite as `LIMIT ?`, where a negative is no limit at all and zero is the + // end of the history -- so an unchecked one loads the whole conversation instead of failing. + if (limit <= 0) + throw std::invalid_argument{ + "{}: limit must be a positive page size (got {})"_format(op, limit)}; +} + void Client::log_operation_failure(const std::exception& e) { log::error(cat, "Client operation failed: {}", e.what()); } diff --git a/src/client/conversation.cpp b/src/client/conversation.cpp index b31a8c57..01713242 100644 --- a/src/client/conversation.cpp +++ b/src/client/conversation.cpp @@ -41,6 +41,7 @@ void Conversation::messages( std::optional before, bool include_deleted, failable_function)> cb) const { + _client->_require_page("messages", limit); _client->_async( [c = _client, id = id, limit, before, include_deleted] { return c->_messages(id, limit, before, include_deleted); @@ -60,6 +61,7 @@ std::vector Conversation::messages( } std::vector Conversation::messages( int limit, std::optional before, bool include_deleted, await_t) const { + _client->_require_page("messages", limit); return _client->loop.call_get([this, limit, before, include_deleted] { return _client->_messages(id, limit, before, include_deleted); }); diff --git a/tests/test_client/conversation_api.cpp b/tests/test_client/conversation_api.cpp index 9aa6084d..04ad48ad 100644 --- a/tests/test_client/conversation_api.cpp +++ b/tests/test_client/conversation_api.cpp @@ -84,6 +84,29 @@ TEST_CASE("Client: settings from another device reach the conversation", "[clien CHECK(convo->display_name() == "Mr Underhill"); } +TEST_CASE("Client: a page size has to be a page", "[client][convos]") { + TempClient c; + auto id = dm_from_hex("05" + std::string(64, 'a')); + c->open_dm(id, await); + c->send_message(id, {.body = "hi"}, await); + + // Unchecked, these reach SQLite as `LIMIT ?`, where a negative is no limit at all: asking for + // one message would load every message in the conversation. + for (int limit : {0, -1, -50}) { + CHECK_THROWS_AS(c->conversation(id, await)->messages(limit, await), std::invalid_argument); + CHECK_THROWS_AS( + c->conversation(id, await)->messages(limit, std::nullopt, true, await), + std::invalid_argument); + } + + // The handler form refuses on the calling thread too, rather than reporting it: the caller is + // still there to catch, and a bad page size is its bug rather than a runtime condition. + auto ignore = [](std::optional, std::vector) {}; + CHECK_THROWS_AS(c->conversation(id, await)->messages(0, ignore), std::invalid_argument); + + CHECK(c->conversation(id, await)->messages(1, await).size() == 1); +} + TEST_CASE("Client: a conversation knows which kind it is", "[client][convos]") { TempClient c; SenderKeys them;