Skip to content

Ensure subquery in findNewIds never returns NULL #8590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
IchbinkeinReh wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Ensure subquery in findNewIds never returns NULL #8590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions lib/Db/MessageMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ class MessageMapper extends QBMapper {
*/
private const MAX_IN_PARAMETERS = 1000;

/** sent_at is a 4-byte INTEGER column; bounds must stay within its range across all supported DBs */
private const SENT_AT_MIN = -2147483648;
private const SENT_AT_MAX = 2147483647;

/** @var ITimeFactory */
private $timeFactory;

Expand Down Expand Up @@ -1436,10 +1440,20 @@ public function findNewIds(Mailbox $mailbox, array $ids, ?int $lastMessageTimest
$select = $this->db->getQueryBuilder();
$subSelect = $this->db->getQueryBuilder();

// MIN/MAX return NULL when no known id matches, so COALESCE to a bound that keeps all rows.
// sent_at is a 4-byte INTEGER column, so the bound must stay within its range (PHP_INT_MIN/MAX
// overflow PostgreSQL's native integer type and get rejected).
if ($sortOrder === IMailSearch::ORDER_NEWEST_FIRST) {
$fallback = (string)$select->createNamedParameter(self::SENT_AT_MIN, IQueryBuilder::PARAM_INT);
$aggregate = (string)$subSelect->func()->min('sent_at');
$sentAtBound = $subSelect->createFunction('COALESCE(' . $aggregate . ', ' . $fallback . ')');
} else {
$fallback = (string)$select->createNamedParameter(self::SENT_AT_MAX, IQueryBuilder::PARAM_INT);
$aggregate = (string)$subSelect->func()->max('sent_at');
$sentAtBound = $subSelect->createFunction('COALESCE(' . $aggregate . ', ' . $fallback . ')');
}
$subSelect
->select($sortOrder === IMailSearch::ORDER_NEWEST_FIRST
? $subSelect->func()->min('sent_at')
: $subSelect->func()->max('sent_at'))
->select($sentAtBound)
->from($this->getTableName())
->where(
$subSelect->expr()->eq('mailbox_id', $select->createNamedParameter($mailbox->getId(), IQueryBuilder::PARAM_INT)),
Expand Down
51 changes: 51 additions & 0 deletions tests/Integration/Db/MessageMapperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use ChristophWurst\Nextcloud\Testing\DatabaseTransaction;
use ChristophWurst\Nextcloud\Testing\TestCase;
use OCA\Mail\Account;
use OCA\Mail\Contracts\IMailSearch;
use OCA\Mail\Db\MailAccount;
use OCA\Mail\Db\MailAccountMapper;
use OCA\Mail\Db\Mailbox;
Expand Down Expand Up @@ -401,6 +402,56 @@ public function testResetPreviewDataFlag(): void {
self::assertEquals(0, $cnt);
}

/**
* Verify we still find the one message of the mailbox if the passed IDs do not exist
*/
public function testFindNewIdsCoalescence(): void {
$uid = time();
$mailbox = new Mailbox();
$mailbox->setId(1);
$qb = $this->db->getQueryBuilder();
$insert = $qb->insert($this->mapper->getTableName())
->values([
'uid' => $qb->createNamedParameter($uid, IQueryBuilder::PARAM_INT),
'message_id' => $qb->createNamedParameter('<abc@123.com>'),
'mailbox_id' => $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT),
'subject' => $qb->createNamedParameter('TEST'),
'sent_at' => $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT),
]);
$insert->executeStatement();
$id = $insert->getLastInsertId();

$foundNewest = $this->mapper->findNewIds($mailbox, [$id + 1], null, IMailSearch::ORDER_NEWEST_FIRST);
$foundOldest = $this->mapper->findNewIds($mailbox, [$id + 1], null, IMailSearch::ORDER_OLDEST_FIRST);

self::assertCount(1, $foundNewest);
self::assertCount(1, $foundOldest);
}

/**
* A spoofed Date header before the Unix epoch yields a negative sent_at;
* the newest-first fallback boundary must not exclude it
*/
public function testFindNewIdsCoalescenceWithNegativeSentAt(): void {
$mailbox = new Mailbox();
$mailbox->setId(1);
$qb = $this->db->getQueryBuilder();
$insert = $qb->insert($this->mapper->getTableName())
->values([
'uid' => $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT),
'message_id' => $qb->createNamedParameter('<abc@123.com>'),
'mailbox_id' => $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT),
'subject' => $qb->createNamedParameter('TEST'),
'sent_at' => $qb->createNamedParameter(-100, IQueryBuilder::PARAM_INT),
]);
$insert->executeStatement();
$id = $insert->getLastInsertId();

$foundNewest = $this->mapper->findNewIds($mailbox, [$id + 1], null, IMailSearch::ORDER_NEWEST_FIRST);

self::assertCount(1, $foundNewest);
}

public function testFindIdsByQuery(): void {
$mailbox = new Mailbox();
$mailbox->setId(1);
Expand Down
Loading