Refuse a page size that is not a page

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<OutgoingAttachment>& attachments);
 
     // Everything a send must be able to reject before storing anything: the conversation kind, the

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<void(std::vector<Message>)> cb) const;
     void messages(int limit, failable_function<void(std::vector<Message>)> cb) const;
     void messages(

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());
 }

src/client/conversation.cpp

@@ -41,6 +41,7 @@ void Conversation::messages(
         std::optional<MessageCursor> before,
         bool include_deleted,
         failable_function<void(std::vector<Message>)> 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<Message> Conversation::messages(
 }
 std::vector<Message> Conversation::messages(
         int limit, std::optional<MessageCursor> 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);
     });

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::string>, std::vector<Message>) {};
+    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;