From c12a9f0fa40d6b455f220ed49eaf4d8e5ee6cdc7 Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Fri, 24 Jul 2026 22:48:32 +0200 Subject: [PATCH 1/3] fix(sync): skip no-op draft rewrites and treat \Deleted as removed Gmail echoes a Modify for every draft Add it receives even when the user changed nothing. Applying it as IMAP append+delete rewrote the draft under a new UID and materialized the client's copy over server state, resurrecting prior versions a third-party IMAP client had flagged \Deleted (#94). Importer: before applying a draft Modify, compare the EAS-visible fields (recipients, subject, importance, read, follow-up flag, categories, body) against the current server copy and acknowledge a no-op without rewriting the message. Any doubt (fetch failure, attachment instructions, mismatch) falls through to the regular rewrite path. IMAP strategies: apply native Exchange semantics to \Deleted across all mail folders. Initial syncs exclude \Deleted messages; a tracked message that gains the flag is exported as a Remove (merged with expunged UIDs); an untracked message carrying the flag is never exported; losing the flag re-exports the message as a regular Add. Fixes #94 --- lib/Horde/ActiveSync/Connector/Importer.php | 136 ++++++++ .../ActiveSync/Imap/Strategy/Initial.php | 3 + lib/Horde/ActiveSync/Imap/Strategy/Modseq.php | 32 +- lib/Horde/ActiveSync/Imap/Strategy/Plain.php | 33 +- .../ImporterIdempotentImportTest.php | 166 ++++++++++ .../Imap/StrategyDeletedFlagTest.php | 304 ++++++++++++++++++ 6 files changed, 664 insertions(+), 10 deletions(-) create mode 100644 test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index da30afb8..5d14ba43 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -196,6 +196,32 @@ public function importMessageChange( $backendId = $alias; } + // No-op email Draft Modify: some clients (e.g. Gmail) echo a Modify + // for every draft Add/Change they receive even though the user + // changed nothing. Applying it as IMAP append+delete would rewrite + // the message under a new UID and materialize the client's copy + // over server-side state (e.g. resurrect a draft a third-party IMAP + // client already flagged \Deleted). Native Exchange does not rewrite + // an item when a Modify carries no changes; neither do we. + if ($id + && ($message instanceof Horde_ActiveSync_Message_Mail) + && !empty($message->airsyncbasebody) + && $this->_isNoOpDraftModify($message, $backendId)) { + $this->_logger->notice( + sprintf( + 'Draft modify for %s carries no changes; acknowledging without rewrite.', + $id + ) + ); + return $this->_draftModifyStat( + $backendId, + $message, + $synckey ?: '', + $id, + ['id' => $backendId, 'mod' => 0, 'flags' => []] + ); + } + // Idempotent email flag Modify under the same SyncKey. if ($id && $synckey @@ -439,6 +465,116 @@ protected function _draftModifyStat( return $out; } + /** + * Determine whether an incoming Draft Modify carries no changes compared + * to the message currently stored on the server. + * + * EAS Modify semantics: an omitted property keeps its server value, so + * only properties the client actually sent can change the message. The + * comparison is deliberately conservative — any doubt (fetch failure, + * attachment instructions, unexpected shape, mismatch) reports a change + * so the regular rewrite path runs. + * + * @author Torben Dannhauer + * + * @param Horde_ActiveSync_Message_Mail $message The incoming message. + * @param string|integer $id The live IMAP UID of the current draft. + * + * @return boolean True when the Modify would not change the message. + */ + protected function _isNoOpDraftModify( + Horde_ActiveSync_Message_Mail $message, + $id + ) { + // Attachment add/remove instructions always change the message. + if (!empty($message->airsyncbaseattachments)) { + return false; + } + + $body = $message->airsyncbasebody; + try { + $current = $this->_as->driver->fetch($this->_folderId, $id, [ + 'protocolversion' => $message->getProtocolVersion(), + 'bodyprefs' => [ + $body->type => [ + 'type' => $body->type, + 'truncationsize' => 0, + ], + ], + 'mimesupport' => 0, + ]); + } catch (Horde_Exception $e) { + return false; + } + if (!($current instanceof Horde_ActiveSync_Message_Mail) + || empty($current->airsyncbasebody) + || (int) $current->airsyncbasebody->type !== (int) $body->type) { + return false; + } + + foreach (['subject', 'to', 'cc', 'bcc'] as $field) { + $sent = $message->$field; + if (is_null($sent) || $sent === false || $sent === '') { + continue; + } + if (trim((string) $sent) !== trim((string) $current->$field)) { + return false; + } + } + foreach (['importance', 'read'] as $field) { + $sent = $message->$field; + if (is_null($sent) || $sent === false || $sent === '') { + continue; + } + if ((int) $sent !== (int) $current->$field) { + return false; + } + } + + // A requested follow-up flag state that differs is a change; an + // empty container carries no request. + $sentFlag = !empty($message->flag) ? (int) $message->flag->flagstatus : 0; + $currentFlag = !empty($current->flag) ? (int) $current->flag->flagstatus : 0; + if ($sentFlag && $sentFlag !== $currentFlag) { + return false; + } + + if (!empty($message->categories) + && $message->categories != ($current->categories ?: [])) { + return false; + } + + return $this->_draftBodyString($body->data) + === $this->_draftBodyString($current->airsyncbasebody->data); + } + + /** + * Normalize AirSyncBaseBody data for comparison: resolve streams and + * unify line endings / trailing whitespace. + * + * @param mixed $data The body data (string, stream resource, or + * Horde_Stream). + * + * @return string The normalized body text. + */ + protected function _draftBodyString($data) + { + // Leave streams rewound: when the comparison reports a change, the + // regular rewrite path still needs to read the body from the start. + if ($data instanceof Horde_Stream) { + $string = $data->getString(0); + $data->rewind(); + $data = $string; + } elseif (is_resource($data)) { + rewind($data); + $contents = stream_get_contents($data); + rewind($data); + $data = $contents; + } + + return rtrim(str_replace("\r\n", "\n", (string) $data)); + } + /** * Import message deletions. This may conflict if the local object has been * modified. diff --git a/lib/Horde/ActiveSync/Imap/Strategy/Initial.php b/lib/Horde/ActiveSync/Imap/Strategy/Initial.php index f58f456e..fa2be06e 100644 --- a/lib/Horde/ActiveSync/Imap/Strategy/Initial.php +++ b/lib/Horde/ActiveSync/Imap/Strategy/Initial.php @@ -31,6 +31,9 @@ public function getChanges(array $options) { $this->_logger->meta('INITIAL SYNC'); $query = new Horde_Imap_Client_Search_Query(); + // Native Exchange semantics: EAS cannot represent a message flagged + // for deletion, so never export \Deleted messages as live items. + $query->flag(Horde_Imap_Client::FLAG_DELETED, false); if (!empty($options['sincedate'])) { $query->dateSearch( new Horde_Date($options['sincedate']), diff --git a/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php b/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php index 16580c78..2fd6211a 100644 --- a/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php +++ b/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php @@ -120,6 +120,7 @@ public function getChanges(array $options) $query->flags(); $changes = []; $categories = []; + $flag_deleted = []; for ($i = 0; $i <= $cnt; $i++) { $ids = new Horde_Imap_Client_Ids( array_slice( @@ -142,6 +143,7 @@ public function getChanges(array $options) $changes, $flags, $categories, + $flag_deleted, $fetch_ret, $options, $current_modseq @@ -167,11 +169,16 @@ public function getChanges(array $options) $this->_logger->err($e->getMessage()); throw new Horde_ActiveSync_Exception($e); } - $this->_folder->setRemoved($deleted->ids); + // Messages that gained the \Deleted flag are removed from the client + // as well: native Exchange semantics know no "flagged for deletion" + // state, so EAS clients must not keep showing such mail as live. + $this->_folder->setRemoved(array_merge($deleted->ids, $flag_deleted)); $this->_logger->meta( sprintf( - 'Found %d deleted messages.', - $deleted->count() + 'Found %d deleted messages (%d expunged, %d flagged \\Deleted).', + $deleted->count() + count($flag_deleted), + $deleted->count(), + count($flag_deleted) ) ); @@ -239,6 +246,10 @@ protected function _searchQuery($options, $is_delete) * @param array &$changes Changes array. * @param array &$flags Flags array. * @param array &$categories Categories array. + * @param array &$flag_deleted Tracked UIDs that + * gained the \Deleted + * flag; to be removed + * from the client. * @param Horde_Imap_Client_Fetch_Results $fetch_ret Fetch results. * @param array $options Options array. * @param integer $modseq Current MODSEQ. @@ -247,6 +258,7 @@ protected function _buildModSeqChanges( &$changes, &$flags, &$categories, + &$flag_deleted, $fetch_ret, $options, $modseq @@ -256,12 +268,24 @@ protected function _buildModSeqChanges( // Filter out any changes that we already know about. $fetch_keys = $fetch_ret->ids(); - $result_set = array_diff($fetch_keys, $changes); + $result_set = array_diff($fetch_keys, $changes, $flag_deleted); + $tracked = $this->_folder->messages(); foreach ($result_set as $uid) { // Ensure no changes after the current modseq have been returned. $data = $fetch_ret[$uid]; if ($data->getModSeq() <= $modseq) { + if (array_search(Horde_Imap_Client::FLAG_DELETED, $data->getFlags()) !== false) { + // Native Exchange semantics: EAS cannot represent a + // message flagged for deletion. A tracked message that + // gained \Deleted is removed from the client; an + // untracked one is never exported. Losing the flag later + // makes the (then untracked) message a regular Add again. + if (in_array($uid, $tracked)) { + $flag_deleted[] = $uid; + } + continue; + } $changes[] = $uid; $flags[$uid] = [ 'read' => (array_search(Horde_Imap_Client::FLAG_SEEN, $data->getFlags()) !== false) ? 1 : 0, diff --git a/lib/Horde/ActiveSync/Imap/Strategy/Plain.php b/lib/Horde/ActiveSync/Imap/Strategy/Plain.php index 6fb7891d..3bef7e5b 100644 --- a/lib/Horde/ActiveSync/Imap/Strategy/Plain.php +++ b/lib/Horde/ActiveSync/Imap/Strategy/Plain.php @@ -60,6 +60,12 @@ public function getChanges(array $options) $query = new Horde_Imap_Client_Fetch_Query(); $query->flags(); $flags = []; + // Native Exchange semantics: EAS cannot represent a message flagged + // for deletion. A tracked message that gained \Deleted is removed + // from the client; an untracked one is never exported. + $tracked = $this->_folder->messages(); + $suppressed = []; + $flag_deleted = []; for ($i = 0; $i <= $cnt; $i++) { $ids = new Horde_Imap_Client_Ids( array_slice( @@ -79,6 +85,15 @@ public function getChanges(array $options) throw new Horde_ActiveSync_Exception($e); } foreach ($fetch_ret as $uid => $data) { + if (array_search(Horde_Imap_Client::FLAG_DELETED, $data->getFlags()) !== false) { + if (!in_array($uid, $suppressed)) { + $suppressed[] = $uid; + if (in_array($uid, $tracked)) { + $flag_deleted[] = $uid; + } + } + continue; + } $flags[$uid] = [ 'read' => (array_search(Horde_Imap_Client::FLAG_SEEN, $data->getFlags()) !== false) ? 1 : 0, ]; @@ -89,14 +104,20 @@ public function getChanges(array $options) } } if (!empty($flags)) { - $this->_folder->setChanges($search_ret['match']->ids, $flags); + $this->_folder->setChanges( + array_diff($search_ret['match']->ids, $suppressed), + $flags + ); } $this->_folder->setRemoved( - $this->_imap_ob->vanished( - $this->_mbox, - null, - ['ids' => new Horde_Imap_Client_Ids($this->_folder->messages())] - )->ids + array_merge( + $this->_imap_ob->vanished( + $this->_mbox, + null, + ['ids' => new Horde_Imap_Client_Ids($this->_folder->messages())] + )->ids, + $flag_deleted + ) ); return $this->_folder; diff --git a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php index 737d8225..38f394df 100644 --- a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php +++ b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php @@ -428,6 +428,172 @@ public function testReadFlagResolvesAliasedServerId(): void $importer->importMessageReadFlag($clientId, 1); } + public function testNoOpDraftModifySkipsRewrite(): void + { + $clientId = '100'; + // Same content modulo line endings and trailing whitespace. + $message = $this->_draftMailMessage("Line1\r\nLine2\n", 'Subject'); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + $state->expects($this->never())->method('recordAppliedPIMChange'); + $state->expects($this->never())->method('recordDraftUidAlias'); + $state->expects($this->never())->method('updateState'); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->once()) + ->method('fetch') + ->with( + 'INBOX/Drafts', + $clientId, + $this->callback(function ($collection) { + $pref = $collection['bodyprefs'][Horde_ActiveSync::BODYPREF_TYPE_PLAIN] ?? null; + return is_array($pref) && $pref['truncationsize'] === 0; + }) + ) + ->willReturn($this->_draftMailMessage("Line1\nLine2", 'Subject')); + $driver->expects($this->never())->method('changeMessage'); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $clientId, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}5' + ); + + $this->assertSame($clientId, $stat['id']); + $this->assertSame(bin2hex('Subject'), $stat['conversationid']); + $this->assertArrayHasKey('conversationindex', $stat); + } + + public function testDraftModifyWithChangedBodyStillRewrites(): void + { + $clientId = '100'; + $newUid = '101'; + $message = $this->_draftMailMessage('Edited body', 'Subject'); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('getUser')->willReturn('alice@example.com'); + $driver->method('fetch') + ->willReturn($this->_draftMailMessage('Original body', 'Subject')); + $driver->expects($this->once()) + ->method('changeMessage') + ->willReturn(['id' => $newUid, 'mod' => 0, 'flags' => []]); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $clientId, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}5' + ); + + $this->assertSame($clientId, $stat['id']); + } + + public function testDraftModifyWithAttachmentInstructionsSkipsNoOpCheck(): void + { + $clientId = '100'; + $message = $this->_draftMailMessage('Body', 'Subject'); + $atc = new Horde_ActiveSync_Message_AirSyncBaseAttachment([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + $message->airsyncbaseattachments = [$atc]; + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('getUser')->willReturn('alice@example.com'); + // Attachment add/remove always changes the message: no comparison + // fetch, straight to the rewrite. + $driver->expects($this->never())->method('fetch'); + $driver->expects($this->once()) + ->method('changeMessage') + ->willReturn(['id' => '101', 'mod' => 0, 'flags' => []]); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $clientId, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}5' + ); + + $this->assertSame($clientId, $stat['id']); + } + + public function testDraftModifyFetchFailureFallsBackToRewrite(): void + { + $clientId = '100'; + $message = $this->_draftMailMessage('Body', 'Subject'); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('getUser')->willReturn('alice@example.com'); + $driver->method('fetch') + ->willThrowException(new Horde_Exception_NotFound()); + $driver->expects($this->once()) + ->method('changeMessage') + ->willReturn(['id' => '101', 'mod' => 0, 'flags' => []]); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $clientId, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}5' + ); + + $this->assertSame($clientId, $stat['id']); + } + + public function testNoOpDraftModifyComparesAgainstAliasedLiveUid(): void + { + $clientId = '100'; + $liveUid = '150'; + $message = $this->_draftMailMessage('Body', 'Subject'); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + $state->method('getDraftUidForClientId') + ->with($clientId) + ->willReturn($liveUid); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->once()) + ->method('fetch') + ->with('INBOX/Drafts', $liveUid, $this->anything()) + ->willReturn($this->_draftMailMessage('Body', 'Subject')); + $driver->expects($this->never())->method('changeMessage'); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $clientId, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}7' + ); + + $this->assertSame($clientId, $stat['id']); + } + public function testFlagModifyRetryUnderSameSyncKeySkipsDriver(): void { $synckey = '{uuid}5'; diff --git a/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php b/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php new file mode 100644 index 00000000..c0c039d0 --- /dev/null +++ b/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php @@ -0,0 +1,304 @@ + + * @category Horde + * @copyright 2026 The Horde Project + * @license http://www.horde.org/licenses/gpl GPLv2 + * @package ActiveSync + * @subpackage UnitTests + */ + +use PHPUnit\Framework\TestCase; + +/** + * Unit tests for native Exchange \Deleted semantics in the IMAP change + * detection strategies (Issue #94): EAS cannot represent a message flagged + * for deletion, so such messages are removed from (or never exported to) + * the client instead of being synced as live mail. + */ +class Horde_ActiveSync_Imap_StrategyDeletedFlagTest extends TestCase +{ + public function testModseqTrackedMessageGainingDeletedFlagIsRemoved(): void + { + $folder = $this->_modseqFolder([52, 53]); + + $imap = $this->_imapMock(); + $imap->method('search')->willReturn([ + 'count' => 1, + 'match' => new Horde_Imap_Client_Ids([52]), + ]); + $this->_mockBatchedFetch($imap, $this->_fetchResults([ + 52 => [[Horde_Imap_Client::FLAG_DELETED], 8], + ])); + $imap->method('vanished') + ->willReturn(new Horde_Imap_Client_Ids([])); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Modseq( + $this->_factory($imap), + $this->_status(10), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + $this->assertSame([52], $result->removed()); + $this->assertSame([], $result->changed()); + $this->assertSame([], $result->added()); + } + + public function testModseqUntrackedDeletedMessageIsNeverExported(): void + { + // 60 arrives already flagged \Deleted, 61 is a regular new message. + $folder = $this->_modseqFolder([52]); + + $imap = $this->_imapMock(); + $imap->method('search')->willReturn([ + 'count' => 2, + 'match' => new Horde_Imap_Client_Ids([60, 61]), + ]); + $this->_mockBatchedFetch($imap, $this->_fetchResults([ + 60 => [[Horde_Imap_Client::FLAG_DELETED], 8], + 61 => [[], 9], + ])); + $imap->method('vanished') + ->willReturn(new Horde_Imap_Client_Ids([])); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Modseq( + $this->_factory($imap), + $this->_status(10), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + $this->assertSame([61], $result->added()); + $this->assertSame([], $result->removed()); + $this->assertArrayNotHasKey(60, $result->flags()); + } + + public function testModseqExpungedAndFlagDeletedRemovalsAreMerged(): void + { + $folder = $this->_modseqFolder([52, 53]); + + $imap = $this->_imapMock(); + $imap->method('search')->willReturn([ + 'count' => 1, + 'match' => new Horde_Imap_Client_Ids([52]), + ]); + $this->_mockBatchedFetch($imap, $this->_fetchResults([ + 52 => [[Horde_Imap_Client::FLAG_DELETED], 8], + ])); + $imap->method('vanished') + ->willReturn(new Horde_Imap_Client_Ids([53])); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Modseq( + $this->_factory($imap), + $this->_status(10), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + $this->assertSame([53, 52], $result->removed()); + } + + public function testPlainTrackedDeletedMessageIsRemovedNotChanged(): void + { + $folder = $this->_plainFolder([52, 53]); + + $imap = $this->_imapMock(); + $imap->method('search')->willReturn([ + 'count' => 2, + 'match' => new Horde_Imap_Client_Ids([52, 53]), + ]); + $this->_mockBatchedFetch($imap, $this->_fetchResults([ + 52 => [[Horde_Imap_Client::FLAG_DELETED]], + 53 => [[Horde_Imap_Client::FLAG_SEEN]], + ])); + $imap->method('vanished') + ->willReturn(new Horde_Imap_Client_Ids([])); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Plain( + $this->_factory($imap), + $this->_status(0), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + $this->assertSame([52], $result->removed()); + $this->assertNotContains(52, $result->changed()); + $this->assertArrayNotHasKey(52, $result->flags()); + // The unflagged message still syncs its flag change. + $this->assertContains(53, $result->changed()); + } + + public function testInitialSyncSearchExcludesDeletedMessages(): void + { + $folder = new Horde_ActiveSync_Folder_Imap( + 'INBOX', + Horde_ActiveSync::CLASS_EMAIL + ); + + $imap = $this->_imapMock(); + $imap->expects($this->once()) + ->method('search') + ->with( + $this->anything(), + $this->callback(function ($query) { + return strpos((string) $query, 'UNDELETED') !== false; + }), + $this->anything() + ) + ->willReturn([ + 'count' => 2, + 'match' => new Horde_Imap_Client_Ids([52, 53]), + ]); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Initial( + $this->_factory($imap), + $this->_status(10), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + // CONDSTORE priming path: the (UNDELETED-filtered) result set is + // primed as the initial Add list. + $this->assertSame([52, 53], $result->added()); + } + + /** + * A folder tracking $uids on a CONDSTORE server at MODSEQ 5. + */ + protected function _modseqFolder(array $uids): Horde_ActiveSync_Folder_Imap + { + $folder = new Horde_ActiveSync_Folder_Imap( + 'INBOX', + Horde_ActiveSync::CLASS_EMAIL + ); + $folder->setStatus([ + Horde_ActiveSync_Folder_Imap::UIDVALIDITY => 1, + Horde_ActiveSync_Folder_Imap::UIDNEXT => 55, + Horde_ActiveSync_Folder_Imap::HIGHESTMODSEQ => 5, + Horde_ActiveSync_Folder_Imap::MESSAGES => count($uids), + ]); + $folder->setChanges($uids); + $folder->updateState(); + + return $folder; + } + + /** + * A folder tracking $uids on a server without CONDSTORE support. + */ + protected function _plainFolder(array $uids): Horde_ActiveSync_Folder_Imap + { + $folder = new Horde_ActiveSync_Folder_Imap( + 'INBOX', + Horde_ActiveSync::CLASS_EMAIL + ); + $folder->setStatus([ + Horde_ActiveSync_Folder_Imap::UIDVALIDITY => 1, + Horde_ActiveSync_Folder_Imap::UIDNEXT => 55, + Horde_ActiveSync_Folder_Imap::MESSAGES => count($uids), + ]); + $flags = []; + foreach ($uids as $uid) { + $flags[$uid] = ['read' => 0, 'flagged' => 0]; + } + $folder->setChanges($uids, $flags); + $folder->updateState(); + + return $folder; + } + + /** + * The IMAP status array handed to the strategy for the current poll. + */ + protected function _status(int $modseq): array + { + return [ + Horde_ActiveSync_Folder_Imap::UIDVALIDITY => 1, + Horde_ActiveSync_Folder_Imap::UIDNEXT => 70, + Horde_ActiveSync_Folder_Imap::HIGHESTMODSEQ => $modseq, + Horde_ActiveSync_Folder_Imap::MESSAGES => 2, + ]; + } + + /** + * @param array $messages UID => [flags array, optional modseq]. + */ + protected function _fetchResults(array $messages): Horde_Imap_Client_Fetch_Results + { + $results = new Horde_Imap_Client_Fetch_Results(); + foreach ($messages as $uid => $spec) { + $data = new Horde_Imap_Client_Data_Fetch(); + $data->setFlags($spec[0]); + if (isset($spec[1])) { + $data->setModSeq($spec[1]); + } + $results[$uid] = $data; + } + + return $results; + } + + /** + * Mock a batched fetch: only requested ids are returned, an empty batch + * yields empty results (like a real IMAP server). + */ + protected function _mockBatchedFetch($imap, Horde_Imap_Client_Fetch_Results $all): void + { + $imap->method('fetch')->willReturnCallback( + function ($mbox, $query, $options) use ($all) { + $batch = new Horde_Imap_Client_Fetch_Results(); + foreach ($options['ids']->ids as $uid) { + if (isset($all[$uid])) { + $batch[$uid] = $all[$uid]; + } + } + return $batch; + } + ); + } + + protected function _imapMock() + { + return $this->getMockBuilder(Horde_Imap_Client_Socket::class) + ->disableOriginalConstructor() + ->onlyMethods(['search', 'fetch', 'vanished']) + ->getMock(); + } + + protected function _factory($imap): Horde_ActiveSync_Interface_ImapFactory + { + $factory = $this->createMock(Horde_ActiveSync_Interface_ImapFactory::class); + $factory->method('getImapOb')->willReturn($imap); + + return $factory; + } + + protected function _logger(): Horde_Log_Logger + { + return new Horde_Log_Logger(new Horde_Log_Handler_Null()); + } +} From 58299aae44527150a64b04ae0a97f769063e39d7 Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Sun, 26 Jul 2026 09:44:09 +0200 Subject: [PATCH 2/3] fix(sync): ignore read state in draft no-op comparison Field testing of #95 (issue #94) showed Gmail hardcodes Read 0 in its echoed draft Modifies, so any draft displayed (\Seen) by another client in the meantime failed the no-op comparison and was rewritten anyway, leaving a fresh \Deleted copy behind on every echo. A draft's read state is metadata, not content: exclude it from the comparison. Also log which field failed the comparison whenever a draft Modify is applied as a rewrite, so traffic logs show why a rewrite happened. --- lib/Horde/ActiveSync/Connector/Importer.php | 60 +++++++++++++++---- .../ImporterIdempotentImportTest.php | 32 ++++++++++ 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index 5d14ba43..48e21a14 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -504,11 +504,20 @@ protected function _isNoOpDraftModify( 'mimesupport' => 0, ]); } catch (Horde_Exception $e) { + $this->_logger->info(sprintf( + 'Draft no-op check for %s: fetch failed (%s); assuming change.', + $id, + $e->getMessage() + )); return false; } if (!($current instanceof Horde_ActiveSync_Message_Mail) || empty($current->airsyncbasebody) || (int) $current->airsyncbasebody->type !== (int) $body->type) { + $this->_logger->info(sprintf( + 'Draft no-op check for %s: unexpected message shape; assuming change.', + $id + )); return false; } @@ -518,17 +527,19 @@ protected function _isNoOpDraftModify( continue; } if (trim((string) $sent) !== trim((string) $current->$field)) { - return false; + return $this->_logDraftMismatch($id, $field); } } - foreach (['importance', 'read'] as $field) { - $sent = $message->$field; - if (is_null($sent) || $sent === false || $sent === '') { - continue; - } - if ((int) $sent !== (int) $current->$field) { - return false; - } + + // NOTE: the POOMMAIL:Read state is deliberately NOT compared. A + // draft's read state is metadata, not content — a difference must + // never cost an append+delete rewrite. Gmail hardcodes Read 0 in + // its echoed Modifies, so comparing read would rewrite any draft + // that was ever displayed (\Seen) by another client. + $sent = $message->importance; + if (!is_null($sent) && $sent !== false && $sent !== '' + && (int) $sent !== (int) $current->importance) { + return $this->_logDraftMismatch($id, 'importance'); } // A requested follow-up flag state that differs is a change; an @@ -536,16 +547,39 @@ protected function _isNoOpDraftModify( $sentFlag = !empty($message->flag) ? (int) $message->flag->flagstatus : 0; $currentFlag = !empty($current->flag) ? (int) $current->flag->flagstatus : 0; if ($sentFlag && $sentFlag !== $currentFlag) { - return false; + return $this->_logDraftMismatch($id, 'flag'); } if (!empty($message->categories) && $message->categories != ($current->categories ?: [])) { - return false; + return $this->_logDraftMismatch($id, 'categories'); + } + + if ($this->_draftBodyString($body->data) + !== $this->_draftBodyString($current->airsyncbasebody->data)) { + return $this->_logDraftMismatch($id, 'body'); } - return $this->_draftBodyString($body->data) - === $this->_draftBodyString($current->airsyncbasebody->data); + return true; + } + + /** + * Log which field failed the draft no-op comparison. + * + * @param string|integer $id The message UID being compared. + * @param string $field The mismatching field. + * + * @return boolean Always false, for use as a return shortcut. + */ + protected function _logDraftMismatch($id, $field) + { + $this->_logger->info(sprintf( + 'Draft no-op check for %s: %s differs; applying as rewrite.', + $id, + $field + )); + + return false; } /** diff --git a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php index 38f394df..b7426e7e 100644 --- a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php +++ b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php @@ -469,6 +469,38 @@ public function testNoOpDraftModifySkipsRewrite(): void $this->assertArrayHasKey('conversationindex', $stat); } + public function testNoOpDraftModifyIgnoresReadStateDifference(): void + { + // Gmail hardcodes Read 0 in echoed Modifies; a draft another client + // displayed (\Seen) must still be detected as a no-op — read state + // is metadata, not content (issue #94 follow-up). + $clientId = '100'; + $message = $this->_draftMailMessage('Body', 'Subject'); + $message->read = 0; + + $current = $this->_draftMailMessage('Body', 'Subject'); + $current->read = Horde_ActiveSync_Message_Mail::FLAG_READ_SEEN; + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('fetch')->willReturn($current); + $driver->expects($this->never())->method('changeMessage'); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $clientId, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}5' + ); + + $this->assertSame($clientId, $stat['id']); + } + public function testDraftModifyWithChangedBodyStillRewrites(): void { $clientId = '100'; From c9247ab73994415db6c338af216b95fabd34a83f Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Sun, 26 Jul 2026 09:44:09 +0200 Subject: [PATCH 3/3] fix(sync): sweep pre-existing \Deleted messages once per folder Messages flagged \Deleted before the server honored the flag never produce another MODSEQ event, so CONDSTORE change detection never converges them and they linger on clients indefinitely (the flag was ignored by all prior releases). Sweep tracked UIDs once per folder with SEARCH DELETED, remove matches from the client, and persist the swept state in the folder object; a failed search is retried on the next poll. Non-CONDSTORE servers converge without this: the Plain strategy re-reads all flags on every poll. This is one-time convergence logic for the semantic change introduced in this branch; the alternative would be requiring every user to re-provision their account after the upgrade. --- lib/Horde/ActiveSync/Folder/Imap.php | 33 +++++++ lib/Horde/ActiveSync/Imap/Strategy/Modseq.php | 52 ++++++++++ .../Imap/StrategyDeletedFlagTest.php | 94 ++++++++++++++++++- 3 files changed, 178 insertions(+), 1 deletion(-) diff --git a/lib/Horde/ActiveSync/Folder/Imap.php b/lib/Horde/ActiveSync/Folder/Imap.php index 2f6320ae..e30397c2 100644 --- a/lib/Horde/ActiveSync/Folder/Imap.php +++ b/lib/Horde/ActiveSync/Folder/Imap.php @@ -148,6 +148,17 @@ class Horde_ActiveSync_Folder_Imap extends Horde_ActiveSync_Folder_Base implemen */ protected $_draftUidAliases = []; + /** + * Whether the one-time convergence sweep for messages already flagged + * \Deleted has run for this folder. Messages flagged before the server + * started honoring \Deleted never produce another MODSEQ event, so the + * Modseq strategy polls them exactly once and removes them from the + * client. + * + * @var boolean + */ + protected $_deletedSwept = false; + /** * Set message changes. * @@ -683,6 +694,24 @@ public function draftUidAliases() return $this->_draftUidAliases; } + /** + * Whether the one-time \Deleted convergence sweep has run. + * + * @return boolean + */ + public function deletedSwept() + { + return $this->_deletedSwept; + } + + /** + * Mark the one-time \Deleted convergence sweep as done. + */ + public function setDeletedSwept() + { + $this->_deletedSwept = true; + } + /** * Forget recorded "ghost" UIDs, e.g. after the eviction deletion was * exported to the client. @@ -751,6 +780,9 @@ public function __serialize(): array if (!empty($this->_draftUidAliases)) { $data['da'] = $this->_draftUidAliases; } + if ($this->_deletedSwept) { + $data['dsw'] = true; + } return $data; } @@ -781,6 +813,7 @@ public function __unserialize(array $data): void $this->_draftUidAliases = !empty($data['da']) ? array_map('strval', $data['da']) : []; + $this->_deletedSwept = !empty($data['dsw']); if (!empty($this->_status[self::HIGHESTMODSEQ]) && is_string($this->_messages)) { $this->_messages = $this->_fromSequenceString($this->_messages); diff --git a/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php b/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php index 2fd6211a..fa3e0d18 100644 --- a/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php +++ b/lib/Horde/ActiveSync/Imap/Strategy/Modseq.php @@ -169,6 +169,19 @@ public function getChanges(array $options) $this->_logger->err($e->getMessage()); throw new Horde_ActiveSync_Exception($e); } + // One-time convergence sweep: messages that were already flagged + // \Deleted before this server started honoring the flag never bump + // MODSEQ again, so they would linger on the client forever. Poll + // them once per folder and remove them from the client too. + if (!$this->_folder->deletedSwept() + && ($swept = $this->_sweepFlagDeleted()) !== false) { + $flag_deleted = array_values(array_unique(array_merge( + $flag_deleted, + $swept + ))); + $this->_folder->setDeletedSwept(); + } + // Messages that gained the \Deleted flag are removed from the client // as well: native Exchange semantics know no "flagged for deletion" // state, so EAS clients must not keep showing such mail as live. @@ -208,6 +221,45 @@ public function getChanges(array $options) return $this->_folder; } + /** + * Search the folder once for tracked messages already carrying the + * \Deleted flag (e.g. flagged before the server honored the flag). + * + * @author Torben Dannhauer + * + * @return array|false The flagged tracked UIDs, or false when the + * search failed (the sweep must be retried). + */ + protected function _sweepFlagDeleted() + { + $tracked = $this->_folder->messages(); + if (empty($tracked)) { + return []; + } + + $query = new Horde_Imap_Client_Search_Query(); + $query->flag(Horde_Imap_Client::FLAG_DELETED, true); + $query->ids(new Horde_Imap_Client_Ids($tracked)); + try { + $search_ret = $this->_imap_ob->search( + $this->_mbox, + $query, + ['results' => [Horde_Imap_Client::SEARCH_RESULTS_MATCH]] + ); + } catch (Horde_Imap_Client_Exception $e) { + $this->_logger->err($e->getMessage()); + return false; + } + if ($search_ret['count']) { + $this->_logger->meta(sprintf( + 'Deleted-flag sweep found %d tracked messages to remove.', + $search_ret['count'] + )); + } + + return $search_ret['match']->ids; + } + /** * Return message UIDs that are now within the cureent FILTERTYPE value. * diff --git a/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php b/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php index c0c039d0..f0db07b6 100644 --- a/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php +++ b/test/unit/Horde/ActiveSync/Imap/StrategyDeletedFlagTest.php @@ -186,10 +186,99 @@ public function testInitialSyncSearchExcludesDeletedMessages(): void $this->assertSame([52, 53], $result->added()); } + public function testModseqSweepRemovesPreexistingDeletedOnFirstPoll(): void + { + // Folder tracking 52 and 53; 53 was flagged \Deleted before the + // server honored the flag (no further MODSEQ event will occur). + $folder = $this->_modseqFolder([52, 53], false); + + $imap = $this->_imapMock(); + $imap->expects($this->exactly(2)) + ->method('search') + ->willReturnCallback(function ($mbox, $query) { + // Second call is the sweep (SEARCH DELETED on tracked ids). + if (strpos((string) $query, 'DELETED') !== false) { + return [ + 'count' => 1, + 'match' => new Horde_Imap_Client_Ids([53]), + ]; + } + return [ + 'count' => 0, + 'match' => new Horde_Imap_Client_Ids([]), + ]; + }); + $this->_mockBatchedFetch($imap, $this->_fetchResults([])); + $imap->method('vanished') + ->willReturn(new Horde_Imap_Client_Ids([])); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Modseq( + $this->_factory($imap), + $this->_status(10), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + $this->assertSame([53], $result->removed()); + $this->assertTrue($result->deletedSwept()); + } + + public function testModseqSweepStateSurvivesSerialization(): void + { + $folder = $this->_modseqFolder([52], false); + $this->assertFalse($folder->deletedSwept()); + $folder->setDeletedSwept(); + + $restored = new Horde_ActiveSync_Folder_Imap( + 'INBOX', + Horde_ActiveSync::CLASS_EMAIL + ); + $restored->unserialize($folder->serialize()); + + $this->assertTrue($restored->deletedSwept()); + } + + public function testModseqSweepNotMarkedDoneWhenSearchFails(): void + { + $folder = $this->_modseqFolder([52, 53], false); + + $imap = $this->_imapMock(); + $imap->method('search') + ->willReturnCallback(function ($mbox, $query) { + if (strpos((string) $query, 'DELETED') !== false) { + throw new Horde_Imap_Client_Exception('sweep failed'); + } + return [ + 'count' => 0, + 'match' => new Horde_Imap_Client_Ids([]), + ]; + }); + $this->_mockBatchedFetch($imap, $this->_fetchResults([])); + $imap->method('vanished') + ->willReturn(new Horde_Imap_Client_Ids([])); + + $strategy = new Horde_ActiveSync_Imap_Strategy_Modseq( + $this->_factory($imap), + $this->_status(10), + $folder, + $this->_logger() + ); + $result = $strategy->getChanges([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + + // The sweep must be retried on the next poll. + $this->assertSame([], $result->removed()); + $this->assertFalse($result->deletedSwept()); + } + /** * A folder tracking $uids on a CONDSTORE server at MODSEQ 5. */ - protected function _modseqFolder(array $uids): Horde_ActiveSync_Folder_Imap + protected function _modseqFolder(array $uids, bool $swept = true): Horde_ActiveSync_Folder_Imap { $folder = new Horde_ActiveSync_Folder_Imap( 'INBOX', @@ -203,6 +292,9 @@ protected function _modseqFolder(array $uids): Horde_ActiveSync_Folder_Imap ]); $folder->setChanges($uids); $folder->updateState(); + if ($swept) { + $folder->setDeletedSwept(); + } return $folder; }