diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 92982ee0..25aa3d2d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,6 +1,10 @@ Changelog ========= +3.3.13 (Unreleased) +---------------------- +- Fix #512: Validate `from` pagination cursor to prevent unbounded conversation loading + 3.3.12 (July 16, 2026) ---------------------- - Fix #504: Message entries overflow horizontally on iOS Safari diff --git a/models/Message.php b/models/Message.php index bc124ab6..ddcf10a4 100644 --- a/models/Message.php +++ b/models/Message.php @@ -62,10 +62,18 @@ public function getEntryUpdates($from = null) $query = $this->hasMany(MessageEntry::class, ['message_id' => 'id']); $query->addOrderBy(['created_at' => SORT_ASC]); - if ($from) { + // Normalize $from: only a strictly positive integer counts as a valid cursor. + // (Avoids PHP loose-comparison pitfalls, e.g. the string "0" being falsy but != null.) + $from = is_numeric($from) ? (int) $from : null; + + if ($from !== null && $from > 0) { $query->andWhere(['>', 'message_entry.id', $from]); } + // Always bound the result set, otherwise an invalid/empty cursor (e.g. from=0 or + // no cursor at all) would load the entire conversation in one go. + $query->limit(Module::getModuleInstance()->conversationUpdatePageSize); + return $query; } @@ -78,12 +86,16 @@ public function getEntryPage($from = null) $query = $this->getEntries(); $query->addOrderBy(['created_at' => SORT_DESC]); - if ($from) { + // Normalize $from: only a strictly positive integer counts as a valid cursor. + $from = is_numeric($from) ? (int) $from : null; + $hasCursor = ($from !== null && $from > 0); + + if ($hasCursor) { $query->andWhere(['<', 'message_entry.id', $from]); } $module = Module::getModuleInstance(); - $limit = $from ? $module->conversationUpdatePageSize : $module->conversationInitPageSize; + $limit = $hasCursor ? $module->conversationUpdatePageSize : $module->conversationInitPageSize; $query->limit($limit); return array_reverse($query->all()); diff --git a/module.json b/module.json index 211a8abe..1660bf24 100644 --- a/module.json +++ b/module.json @@ -8,7 +8,7 @@ "messenger", "communication" ], - "version": "3.3.12", + "version": "3.3.13", "humhub": { "minVersion": "1.18.1", "maxVersion": "1.18" diff --git a/tests/codeception/unit/MessageEntryPaginationTest.php b/tests/codeception/unit/MessageEntryPaginationTest.php new file mode 100644 index 00000000..2f1baff5 --- /dev/null +++ b/tests/codeception/unit/MessageEntryPaginationTest.php @@ -0,0 +1,162 @@ +becomeUser('User1'); + + $form = new CreateMessage([ + 'title' => 'Pagination Test', + 'message' => 'Entry 0', + 'recipient' => [User::findOne(['id' => 3])->guid], + ]); + + $this->assertTrue($form->save(), 'Message creation failed: ' . json_encode($form->getErrors())); + + $message = $form->messageInstance; + $this->setEntryCreatedAt($message->getLastEntry(), 0); + + return $message; + } + + /** + * Appends $count additional entries to $message, as the currently logged in + * user, each one second apart (starting one second after the initial entry) + * so their relative order is always unambiguous. + */ + private function addEntries(Message $message, int $count): void + { + for ($i = 0; $i < $count; $i++) { + $entry = MessageEntry::createForMessage($message, Yii::$app->user->getIdentity(), 'Extra entry ' . $i); + $this->assertTrue($entry->save(), 'Entry creation failed: ' . json_encode($entry->getErrors())); + $this->setEntryCreatedAt($entry, $i + 1); + } + } + + /** + * Forces a deterministic created_at on an already-saved entry, $secondsOffset + * seconds after a fixed base time. + */ + private function setEntryCreatedAt(MessageEntry $entry, int $secondsOffset): void + { + $entry->created_at = (new DateTime('2026-01-01 00:00:00')) + ->modify("+{$secondsOffset} seconds") + ->format('Y-m-d H:i:s'); + + $this->assertTrue($entry->save(false), 'Failed to set deterministic created_at on entry ' . $entry->id); + } + + public function testGetEntryUpdatesIsAlwaysBoundedByPageSize(): void + { + $module = Yii::$app->getModule('mail'); + $module->conversationUpdatePageSize = 3; + + $message = $this->createConversation(); + $this->addEntries($message, 10); // 11 entries total, well above the page size of 3 + + // Every one of these must be treated as "no valid cursor" and still be + // capped at conversationUpdatePageSize - none of them must load everything. + foreach ([null, '0', 0, '', false] as $from) { + $entries = $message->getEntryUpdates($from)->all(); + $this->assertLessThanOrEqual( + 3, + count($entries), + 'getEntryUpdates(' . var_export($from, true) . ') must never exceed conversationUpdatePageSize' + ); + } + } + + public function testGetEntryUpdatesFromZeroBehavesLikeNoCursor(): void + { + $module = Yii::$app->getModule('mail'); + $module->conversationUpdatePageSize = 50; // large enough to not truncate any of these results + + $message = $this->createConversation(); + $this->addEntries($message, 4); // 5 entries total + + $withNull = array_map(fn($e) => $e->id, $message->getEntryUpdates(null)->all()); + $withStringZero = array_map(fn($e) => $e->id, $message->getEntryUpdates('0')->all()); + $withIntZero = array_map(fn($e) => $e->id, $message->getEntryUpdates(0)->all()); + + $this->assertSame($withNull, $withStringZero, 'from="0" must be treated the same as no cursor at all'); + $this->assertSame($withNull, $withIntZero, 'from=0 (int) must be treated the same as no cursor at all'); + } + + public function testGetEntryUpdatesRespectsARealPositiveCursor(): void + { + $module = Yii::$app->getModule('mail'); + $module->conversationUpdatePageSize = 50; + + $message = $this->createConversation(); + $this->addEntries($message, 4); // 5 entries total, ascending ids + + $allEntries = $message->getEntryUpdates(null)->all(); + $this->assertCount(5, $allEntries); + + $cursor = $allEntries[1]->id; // second oldest entry + $expectedRemainingIds = array_map(fn($e) => $e->id, array_slice($allEntries, 2)); + + $filtered = array_map(fn($e) => $e->id, $message->getEntryUpdates($cursor)->all()); + + $this->assertSame($expectedRemainingIds, $filtered); + } + + public function testGetEntryPageFromZeroBehavesLikeNoCursor(): void + { + $module = Yii::$app->getModule('mail'); + $module->conversationInitPageSize = 3; + $module->conversationUpdatePageSize = 3; + + $message = $this->createConversation(); + $this->addEntries($message, 10); // 11 entries total + + $withNull = array_map(fn($e) => $e->id, $message->getEntryPage(null)); + $withStringZero = array_map(fn($e) => $e->id, $message->getEntryPage('0')); + + $this->assertSame($withNull, $withStringZero, 'getEntryPage("0") must behave like getEntryPage(null)'); + $this->assertCount(3, $withStringZero); + } + + public function testGetEntryPageRespectsARealPositiveCursor(): void + { + $module = Yii::$app->getModule('mail'); + $module->conversationInitPageSize = 50; + $module->conversationUpdatePageSize = 50; + + $message = $this->createConversation(); + $this->addEntries($message, 4); // 5 entries total, ascending ids + + $allEntries = $message->getEntryUpdates(null)->all(); + $this->assertCount(5, $allEntries); + + $cursor = $allEntries[3]->id; // fourth oldest entry (index 3) + $expectedOlderIds = array_map(fn($e) => $e->id, array_slice($allEntries, 0, 3)); + + $page = array_map(fn($e) => $e->id, $message->getEntryPage($cursor)); + + $this->assertSame($expectedOlderIds, $page); + } +}