From b7f383ea4017cb6c136de7021cb5392f6e97054e Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Sun, 19 Jul 2026 21:51:35 +0200 Subject: [PATCH 1/3] feat(sync): idempotent email import so retries do not duplicate IMAP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record applied Add/Modify/Remove (and remove-as-move) under the current SyncKey so a lost Sync response can be retried without appending another generation of Draft messages. Dual-write email Add clientids into the sync map; map Draft Modify old→new UIDs and suppress mirror Add/Delete export via mailmap. Closes #85. --- lib/Horde/ActiveSync/Connector/Importer.php | 237 ++++++++++++++-- lib/Horde/ActiveSync/State/Base.php | 109 ++++++++ lib/Horde/ActiveSync/State/Mongo.php | 262 ++++++++++++++++- lib/Horde/ActiveSync/State/Sql.php | 240 +++++++++++++++- .../ImporterIdempotentImportTest.php | 263 ++++++++++++++++++ 5 files changed, 1071 insertions(+), 40 deletions(-) create mode 100644 test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index 93f70925..352fb763 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -151,10 +151,53 @@ public function importMessageChange( // Don't support SMS, but can't tell client that. Send back a phoney // UID for any imported SMS objects. if ($class == Horde_ActiveSync::CLASS_SMS - || strpos($id, 'IGNORESMS_') === 0) { + || strpos((string) $id, 'IGNORESMS_') === 0) { return 'IGNORESMS_' . $clientid; } + // Idempotent email Draft Modify: same SyncKey + ServerId must not + // append another IMAP message when the client retries a lost response. + if ($id + && $synckey + && ($message instanceof Horde_ActiveSync_Message_Mail) + && !empty($message->airsyncbasebody) + && ($applied = $this->_state->getAppliedPIMChange($id, $synckey))) { + $this->_logger->notice( + sprintf( + 'Duplicate draft modify for %s under %s; returning %s', + $id, + $synckey, + $applied['id'] + ) + ); + return $this->_draftModifyStat($applied['id'], $message, $synckey, $id); + } + + // Idempotent email flag Modify under the same SyncKey. + if ($id + && $synckey + && ($message instanceof Horde_ActiveSync_Message_Mail) + && empty($message->airsyncbasebody) + && $this->_state->isMailMapChangeApplied( + $id, + Horde_ActiveSync::CHANGE_TYPE_FLAGS, + $synckey + )) { + $this->_logger->notice( + sprintf( + 'Duplicate flag modify for %s under %s', + $id, + $synckey + ) + ); + return [ + 'id' => $id, + 'mod' => 0, + 'flags' => [], + 'serverid' => $this->_folderId, + ]; + } + // Changing an existing object if ($id && $this->_flags == Horde_ActiveSync::CONFLICT_OVERWRITE_PIM) { // This is complicated by the fact that in EAS 16.0, clients @@ -243,22 +286,43 @@ public function importMessageChange( } $stat['serverid'] = $this->_folderId; - // Record the state of the message. - // Email messages are only changed if they are Drafts or if we are - // updating flags. - // When CHANGING a draft message, we are actually deleting the old one - // and replacing it with a message (since we can't edit an existing - // IMAP message while keeping the UID the same). So, do not call - // updateState() for these messages since we don't want to ignore - // this as a PIM sourced change - the change will be caught during - // normal ping/sync cycle. - if ($message instanceof Horde_ActiveSync_Message_Mail) { - if (!empty($message->airsyncbasebody) && !empty($id)) { - // Changing an existing Draft mail. - return $stat; + // Email Draft Modify: IMAP append+delete creates a new UID. Record the + // applied mapping before mailmap so a lost response can be retried + // without another append, and suppress mirror Add/Delete export. + if ($message instanceof Horde_ActiveSync_Message_Mail + && !empty($message->airsyncbasebody) + && !empty($id)) { + if ($synckey) { + $this->_state->recordAppliedPIMChange($id, $stat, $synckey); } + $user = $this->_as->driver->getUser(); + $this->_state->updateState( + Horde_ActiveSync::CHANGE_TYPE_DRAFT, + $stat, + Horde_ActiveSync::CHANGE_ORIGIN_PIM, + $user + ); + $this->_state->updateState( + Horde_ActiveSync::CHANGE_TYPE_DELETE, + [ + 'id' => $id, + 'mod' => !empty($stat['mod']) ? $stat['mod'] : 0, + 'serverid' => $this->_folderId, + ], + Horde_ActiveSync::CHANGE_ORIGIN_PIM, + $user + ); + return $this->_draftModifyStat( + $stat['id'], + $message, + $synckey ?: '', + $id, + $stat + ); + } - // Either a flag change, or adding a new Draft mail. + if ($message instanceof Horde_ActiveSync_Message_Mail) { + // Flag change or new Draft mail. $changeType = !empty($message->airsyncbasebody) ? Horde_ActiveSync::CHANGE_TYPE_DRAFT : Horde_ActiveSync::CHANGE_TYPE_FLAGS; @@ -274,9 +338,61 @@ public function importMessageChange( $clientid ); + // Email Add: mailmap has no sync_clientid; dual-write to sync_map so + // isDuplicatePIMAddition() can catch retries after a lost response. + if (!$id && $clientid + && ($message instanceof Horde_ActiveSync_Message_Mail)) { + $this->_state->recordPIMAddition( + $clientid, + $stat['id'], + $this->_folderId, + $synckey ?: null + ); + } + return $stat; } + /** + * Build a Draft Modify stat array suitable for Sync replies, including + * conversation fields so EAS 16 clients accept the response on retry. + * + * @param string|integer $newId + * @param Horde_ActiveSync_Message_Base $message + * @param string $synckey + * @param string|integer $oldId + * @param array $stat Optional driver stat to merge (atchash, etc.). + * + * @return array + */ + protected function _draftModifyStat( + $newId, + Horde_ActiveSync_Message_Base $message, + $synckey, + $oldId, + array $stat = [] + ) { + $out = array_merge( + [ + 'id' => $newId, + 'mod' => 0, + 'flags' => [], + ], + $stat + ); + $out['id'] = $newId; + $out['serverid'] = $this->_folderId; + // Stable conversation fields (not time()) so retries emit the same + // SyncReplies shape Gmail requires for Drafts up-sync. + if ($message instanceof Horde_ActiveSync_Message_Mail) { + $subject = (string) $message->subject; + $out['conversationid'] = bin2hex($subject); + $out['conversationindex'] = crc32($synckey . ':' . $oldId . ':' . $newId); + } + + return $out; + } + /** * Import message deletions. This may conflict if the local object has been * modified. @@ -318,18 +434,39 @@ public function importMessageDeletion(array $ids, $class, $instanceids = false) return $ids; } - // Ask the backend to delete the message. - $mod = $this->_as->driver->getSyncStamp($this->_folderId); - $ids = $this->_as->driver->deleteMessage($this->_folderId, $ids); + $already = []; + $toDelete = []; foreach ($ids as $id) { - // Ignore SMS changes. - if (strpos($id, "IGNORESMS_") === 0) { + if (strpos((string) $id, 'IGNORESMS_') === 0) { continue; } - $change = []; - $change['id'] = $id; - $change['mod'] = $mod; - $change['serverid'] = $this->_folderId; + if ($this->_state->isMailMapChangeApplied( + $id, + Horde_ActiveSync::CHANGE_TYPE_DELETE + )) { + $already[] = $id; + } else { + $toDelete[] = $id; + } + } + + $mod = $this->_as->driver->getSyncStamp($this->_folderId); + $deleted = $toDelete + ? $this->_as->driver->deleteMessage($this->_folderId, $toDelete) + : []; + if (!is_array($deleted)) { + $deleted = []; + } + + // UIDs the driver did not return are treated as already gone so a + // retried Remove after a lost response is not reported as missing. + $gone = array_diff($toDelete, $deleted); + foreach ($deleted as $id) { + $change = [ + 'id' => $id, + 'mod' => $mod, + 'serverid' => $this->_folderId, + ]; $this->_state->updateState( Horde_ActiveSync::CHANGE_TYPE_DELETE, $change, @@ -337,8 +474,20 @@ public function importMessageDeletion(array $ids, $class, $instanceids = false) $this->_as->driver->getUser() ); } + foreach ($gone as $id) { + $this->_state->updateState( + Horde_ActiveSync::CHANGE_TYPE_DELETE, + [ + 'id' => $id, + 'mod' => $mod, + 'serverid' => $this->_folderId, + ], + Horde_ActiveSync::CHANGE_ORIGIN_PIM, + $this->_as->driver->getUser() + ); + } - return $ids; + return array_values(array_unique(array_merge($already, $deleted, $gone))); } /** @@ -396,7 +545,31 @@ function ($e) { $collectionClass = Horde_ActiveSync::CLASS_EMAIL; } $dst = $collections->getBackendIdForFolderUid($dst); - $results = $this->_as->driver->moveMessage($this->_folderId, $uids, $dst); + $synckey = $this->_state->getCurrentSyncKey(); + + $results = []; + $pending = []; + foreach ($uids as $uid) { + if ($synckey + && ($prev = $this->_state->getAppliedMailMove($uid, $synckey))) { + $results[$uid] = $prev; + continue; + } + $pending[] = $uid; + } + + if ($pending) { + $moved = $this->_as->driver->moveMessage( + $this->_folderId, + $pending, + $dst + ); + if (is_array($moved)) { + foreach ($moved as $old => $new) { + $results[$old] = $new; + } + } + } // Check for any missing (not found) source messages. $missing = count($results) != count($uids) @@ -408,7 +581,7 @@ function ($e) { // sync, but some broken clients don't like this. Save the import // in the map table in case we need it later. $mod = $this->_as->driver->getSyncStamp($this->_folderId); - foreach ($uids as $uid) { + foreach ($pending as $uid) { if (empty($results[$uid])) { continue; } @@ -424,9 +597,17 @@ function ($e) { Horde_ActiveSync::CHANGE_ORIGIN_PIM, $this->_as->driver->getUser() ); + if ($synckey) { + $this->_state->recordAppliedMailMove( + $uid, + $results[$uid], + $synckey, + $dst + ); + } } - return ['results' => $results, 'missing' => $missing]; + return ['results' => $results, 'missing' => array_values($missing)]; } /** diff --git a/lib/Horde/ActiveSync/State/Base.php b/lib/Horde/ActiveSync/State/Base.php index 5235d347..d0e261b0 100644 --- a/lib/Horde/ActiveSync/State/Base.php +++ b/lib/Horde/ActiveSync/State/Base.php @@ -1501,6 +1501,115 @@ abstract public function deleteSyncCache($devid, $user); */ abstract public function isDuplicatePIMAddition($id); + /** + * Synthetic map clientid for an email Draft Modify (UID replace) under a + * given folder. Used so a retried Sync under the same SyncKey can return + * the already-created replacement UID without appending again. + * + * @param string $folderServerId Backend folder id. + * @param string|integer $oldUid Client-supplied ServerId (old IMAP UID). + * + * @return string + * @since 3.0.3 + */ + public static function draftModifyClientId($folderServerId, $oldUid) + { + return 'eas:draftmod:' . $folderServerId . ':' . $oldUid; + } + + /** + * Synthetic map clientid for an email Remove-as-move (deletesasmoves). + * + * @param string $folderServerId Source backend folder id. + * @param string|integer $oldUid Source message UID. + * + * @return string + * @since 3.0.3 + */ + public static function mailMoveClientId($folderServerId, $oldUid) + { + return 'eas:mailmove:' . $folderServerId . ':' . $oldUid; + } + + /** + * Return a previously applied PIM Modify for the given ServerId/SyncKey + * (email Draft UID replace), or null if none. + * + * @param string|integer $serverid The client ServerId (old UID). + * @param string $synckey SyncKey the command was applied under. + * + * @return array|null Stat-like array with at least 'id' (new UID), or null. + * @throws Horde_ActiveSync_Exception + * @since 3.0.3 + */ + abstract public function getAppliedPIMChange($serverid, $synckey); + + /** + * Record that a PIM Modify (email Draft UID replace) was applied so a + * retry under the same SyncKey can short-circuit. + * + * @param string|integer $oldId Client ServerId (old UID). + * @param array $stat Stat from the driver (must include 'id'). + * @param string $synckey SyncKey the command was applied under. + * + * @throws Horde_ActiveSync_Exception + * @since 3.0.3 + */ + abstract public function recordAppliedPIMChange($oldId, array $stat, $synckey); + + /** + * Record a client Add's clientid → server UID mapping in the sync map. + * Used for email collections (mailmap has no sync_clientid column). + * + * @param string $clientid Client ClientId from the Add. + * @param string|integer $uid Server UID assigned to the new message. + * @param string|integer $folderId Backend folder id. + * @param string|null $synckey SyncKey; defaults to current state key. + * + * @throws Horde_ActiveSync_Exception + * @since 3.0.3 + */ + abstract public function recordPIMAddition($clientid, $uid, $folderId, $synckey = null); + + /** + * Whether mailmap already records a client-origin change of $type for $uid + * (optional SyncKey scope for flag changes). + * + * @param string|integer $uid Message UID. + * @param string $type Horde_ActiveSync::CHANGE_TYPE_* constant. + * @param string|null $synckey If set, require this SyncKey (flag retries). + * + * @return boolean + * @throws Horde_ActiveSync_Exception + * @since 3.0.3 + */ + abstract public function isMailMapChangeApplied($uid, $type, $synckey = null); + + /** + * Look up a previously applied Remove-as-move mapping for $oldUid. + * + * @param string|integer $oldUid Source UID. + * @param string $synckey SyncKey the move was applied under. + * + * @return string|null Destination UID, or null. + * @throws Horde_ActiveSync_Exception + * @since 3.0.3 + */ + abstract public function getAppliedMailMove($oldUid, $synckey); + + /** + * Record a Remove-as-move old→new UID mapping for retry idempotency. + * + * @param string|integer $oldUid Source UID. + * @param string|integer $newUid Destination UID. + * @param string $synckey SyncKey the move was applied under. + * @param string $dstFolderId Destination backend folder id. + * + * @throws Horde_ActiveSync_Exception + * @since 3.0.3 + */ + abstract public function recordAppliedMailMove($oldUid, $newUid, $synckey, $dstFolderId); + /** * Close the underlying backend storage connection. * To be used during PING or looping SYNC operations. diff --git a/lib/Horde/ActiveSync/State/Mongo.php b/lib/Horde/ActiveSync/State/Mongo.php index 90bfda10..7a75471c 100644 --- a/lib/Horde/ActiveSync/State/Mongo.php +++ b/lib/Horde/ActiveSync/State/Mongo.php @@ -92,6 +92,8 @@ class Horde_ActiveSync_State_Mongo extends Horde_ActiveSync_State_Base implement public const SYNC_FLAGGED = 'sync_flagged'; public const SYNC_DELETED = 'sync_deleted'; public const SYNC_CHANGED = 'sync_changed'; + public const SYNC_DRAFT = 'sync_draft'; + public const SYNC_CATEGORY = 'sync_category'; public const SYNC_MODTIME = 'sync_modtime'; public const SYNC_CLIENTID = 'sync_clientid'; public const SYNC_DATA = 'sync_data'; @@ -998,8 +1000,10 @@ public function updateState( case Horde_ActiveSync::CHANGE_TYPE_FLAGS: if (isset($change['flags']['read'])) { $document[self::SYNC_READ] = !empty($change['flags']['read']); - } else { - $document[self::SYNC_FLAGGED] = $flag_value = !empty($change['flags']['flagged']); + } elseif (isset($change['flags']['flagged'])) { + $document[self::SYNC_FLAGGED] = !empty($change['flags']['flagged']); + } elseif (!empty($change['categories'])) { + $document[self::SYNC_CATEGORY] = md5(implode('', $change['categories'])); } break; case Horde_ActiveSync::CHANGE_TYPE_DELETE: @@ -1008,11 +1012,14 @@ public function updateState( case Horde_ActiveSync::CHANGE_TYPE_CHANGE: $document[self::SYNC_CHANGED] = true; break; + case Horde_ActiveSync::CHANGE_TYPE_DRAFT: + $document[self::SYNC_DRAFT] = true; + break; } try { $this->_db->selectCollection(self::COLLECTION_MAILMAP)->insert($document); } catch (Exception $e) { - throw Horde_ActiveSync_Exception($e); + throw new Horde_ActiveSync_Exception($e); } break; @@ -1706,6 +1713,9 @@ public function isDuplicatePIMAddition($id) self::SYNC_USER => $this->_deviceInfo->user, self::SYNC_DEVID => $this->_deviceInfo->id, ]; + if (!empty($this->_collection['serverid'])) { + $query[self::SYNC_FOLDERID] = $this->_collection['serverid']; + } try { $result = $this->_db->selectCollection(self::COLLECTION_MAP) @@ -1725,6 +1735,233 @@ public function isDuplicatePIMAddition($id) return $result[self::MESSAGE_UID]; } + /** + * Check if the UID provided was altered during the SYNC_KEY provided. + * + * @param string $uid The UID to check. + * @param string $synckey The synckey to check. + * + * @return boolean + * @since 2.31.0 + */ + public function isDuplicatePIMChange($uid, $synckey) + { + $query = [ + self::MESSAGE_UID => $uid, + self::SYNC_USER => $this->_deviceInfo->user, + self::SYNC_KEY => $synckey, + self::SYNC_DEVID => $this->_deviceInfo->id, + ]; + try { + $result = $this->_db->selectCollection(self::COLLECTION_MAP) + ->findOne($query, [self::MONGO_ID => true]); + } catch (Exception $e) { + $this->_logger->err($e->getMessage()); + throw new Horde_ActiveSync_Exception($e); + } + + return !empty($result); + } + + /** + * @since 3.0.3 + */ + public function getAppliedPIMChange($serverid, $synckey) + { + if (empty($this->_collection['serverid'])) { + return null; + } + $query = [ + self::SYNC_CLIENTID => self::draftModifyClientId( + $this->_collection['serverid'], + $serverid + ), + self::SYNC_KEY => $synckey, + self::SYNC_USER => $this->_deviceInfo->user, + self::SYNC_DEVID => $this->_deviceInfo->id, + self::SYNC_FOLDERID => $this->_collection['serverid'], + ]; + try { + $result = $this->_db->selectCollection(self::COLLECTION_MAP) + ->findOne($query, [self::MESSAGE_UID => true]); + } catch (Exception $e) { + $this->_logger->err($e->getMessage()); + throw new Horde_ActiveSync_Exception($e); + } + if (empty($result[self::MESSAGE_UID])) { + return null; + } + + return [ + 'id' => $result[self::MESSAGE_UID], + 'mod' => 0, + 'flags' => [], + ]; + } + + /** + * @since 3.0.3 + */ + public function recordAppliedPIMChange($oldId, array $stat, $synckey) + { + if (empty($this->_collection['serverid']) || empty($stat['id'])) { + return; + } + $this->_insertMapClientIdDoc( + self::draftModifyClientId($this->_collection['serverid'], $oldId), + $stat['id'], + $this->_collection['serverid'], + $synckey, + !empty($stat['mod']) ? $stat['mod'] : 0 + ); + } + + /** + * @since 3.0.3 + */ + public function recordPIMAddition($clientid, $uid, $folderId, $synckey = null) + { + if ($clientid === '' || $clientid === null || $clientid === false) { + return; + } + $syncKey = $synckey ?: (empty($this->_syncKey) + ? $this->getLatestSynckeyForCollection($this->_collection['id'] ?? '') + : $this->_syncKey); + $this->_insertMapClientIdDoc($clientid, $uid, $folderId, $syncKey, time()); + } + + /** + * @since 3.0.3 + */ + public function isMailMapChangeApplied($uid, $type, $synckey = null) + { + if (empty($this->_collection['serverid'])) { + return false; + } + $query = [ + self::MESSAGE_UID => (string) $uid, + self::SYNC_DEVID => $this->_deviceInfo->id, + self::SYNC_USER => $this->_deviceInfo->user, + self::SYNC_FOLDERID => $this->_collection['serverid'], + ]; + if ($synckey !== null) { + $query[self::SYNC_KEY] = $synckey; + } + switch ($type) { + case Horde_ActiveSync::CHANGE_TYPE_DELETE: + $query[self::SYNC_DELETED] = true; + break; + case Horde_ActiveSync::CHANGE_TYPE_DRAFT: + $query[self::SYNC_DRAFT] = true; + break; + case Horde_ActiveSync::CHANGE_TYPE_CHANGE: + $query[self::SYNC_CHANGED] = true; + break; + case Horde_ActiveSync::CHANGE_TYPE_FLAGS: + $query['$or'] = [ + [self::SYNC_READ => ['$exists' => true]], + [self::SYNC_FLAGGED => ['$exists' => true]], + [self::SYNC_CATEGORY => ['$exists' => true]], + ]; + break; + default: + return false; + } + try { + $result = $this->_db->selectCollection(self::COLLECTION_MAILMAP) + ->findOne($query, [self::MONGO_ID => true]); + } catch (Exception $e) { + $this->_logger->err($e->getMessage()); + throw new Horde_ActiveSync_Exception($e); + } + + return !empty($result); + } + + /** + * @since 3.0.3 + */ + public function getAppliedMailMove($oldUid, $synckey) + { + if (empty($this->_collection['serverid'])) { + return null; + } + $query = [ + self::SYNC_CLIENTID => self::mailMoveClientId( + $this->_collection['serverid'], + $oldUid + ), + self::SYNC_KEY => $synckey, + self::SYNC_USER => $this->_deviceInfo->user, + self::SYNC_DEVID => $this->_deviceInfo->id, + ]; + try { + $result = $this->_db->selectCollection(self::COLLECTION_MAP) + ->findOne($query, [self::MESSAGE_UID => true]); + } catch (Exception $e) { + $this->_logger->err($e->getMessage()); + throw new Horde_ActiveSync_Exception($e); + } + if (empty($result[self::MESSAGE_UID])) { + return null; + } + + return $result[self::MESSAGE_UID]; + } + + /** + * @since 3.0.3 + */ + public function recordAppliedMailMove($oldUid, $newUid, $synckey, $dstFolderId) + { + if (empty($this->_collection['serverid'])) { + return; + } + $this->_insertMapClientIdDoc( + self::mailMoveClientId($this->_collection['serverid'], $oldUid), + $newUid, + $dstFolderId, + $synckey, + time() + ); + } + + /** + * Insert a HAS_map document keyed by sync_clientid. + * + * @param string $clientid + * @param string|integer $messageUid + * @param string $folderId + * @param string $synckey + * @param integer $modtime + * + * @throws Horde_ActiveSync_Exception + */ + protected function _insertMapClientIdDoc( + $clientid, + $messageUid, + $folderId, + $synckey, + $modtime + ) { + $document = [ + self::MESSAGE_UID => (string) $messageUid, + self::SYNC_MODTIME => $modtime, + self::SYNC_KEY => $synckey, + self::SYNC_DEVID => $this->_deviceInfo->id, + self::SYNC_FOLDERID => $folderId, + self::SYNC_USER => $this->_deviceInfo->user, + self::SYNC_CLIENTID => $clientid, + self::SYNC_DELETED => false, + ]; + try { + $this->_db->selectCollection(self::COLLECTION_MAP)->insert($document); + } catch (Exception $e) { + $this->_logger->err($e->getMessage()); + throw new Horde_ActiveSync_Exception($e); + } + } + /** * Return the sync cache. * @@ -1994,7 +2231,9 @@ protected function _getMailMapChanges(array $changes) self::SYNC_READ => true, self::SYNC_FLAGGED => true, self::SYNC_DELETED => true, - self::SYNC_CHANGED, + self::SYNC_CHANGED => true, + self::SYNC_DRAFT => true, + self::SYNC_CATEGORY => true, ] ); $results = []; @@ -2004,16 +2243,23 @@ protected function _getMailMapChanges(array $changes) switch ($change['type']) { case Horde_ActiveSync::CHANGE_TYPE_FLAGS: $results[$row[self::MESSAGE_UID]][$change['type']] - = (!is_null($row[self::SYNC_READ]) && $row[self::SYNC_READ] == $change['flags']['read']) - || (!is_null($row[self::SYNC_FLAGGED] && $row[self::SYNC_FLAGGED] == $change['flags']['flagged'])); + = (isset($row[self::SYNC_READ]) && $row[self::SYNC_READ] == $change['flags']['read']) + || (isset($row[self::SYNC_FLAGGED]) && $row[self::SYNC_FLAGGED] == ($change['flags']['flagged'] ?? null)) + || (isset($row[self::SYNC_CATEGORY]) && !empty($change['categories']) + && $row[self::SYNC_CATEGORY] == md5(implode('', $change['categories']))); continue 3; case Horde_ActiveSync::CHANGE_TYPE_DELETE: $results[$row[self::MESSAGE_UID]][$change['type']] - = !is_null($row[self::SYNC_DELETED]) && $row[self::SYNC_DELETED] == true; + = !empty($row[self::SYNC_DELETED]); continue 3; case Horde_ActiveSync::CHANGE_TYPE_CHANGE: $results[$row[self::MESSAGE_UID]][$change['type']] - = !is_null($row[self::SYNC_CHANGED]) && $row[self::SYNC_CHANGED] == true; + = !empty($row[self::SYNC_CHANGED]); + continue 3; + case Horde_ActiveSync::CHANGE_TYPE_DRAFT: + $results[$row[self::MESSAGE_UID]][$change['type']] + = !empty($row[self::SYNC_DRAFT]); + continue 3; } } } diff --git a/lib/Horde/ActiveSync/State/Sql.php b/lib/Horde/ActiveSync/State/Sql.php index 2c639671..6c8c3fb7 100644 --- a/lib/Horde/ActiveSync/State/Sql.php +++ b/lib/Horde/ActiveSync/State/Sql.php @@ -1696,9 +1696,14 @@ public function removeState(array $options) public function isDuplicatePIMAddition($id) { $sql = 'SELECT message_uid FROM ' . $this->_syncMapTable - . ' WHERE sync_clientid = ? AND sync_user = ?'; + . ' WHERE sync_clientid = ? AND sync_user = ? AND sync_devid = ?'; + $params = [$id, $this->_deviceInfo->user, $this->_deviceInfo->id]; + if (!empty($this->_collection['serverid'])) { + $sql .= ' AND sync_folderid = ?'; + $params[] = $this->_collection['serverid']; + } try { - $uid = $this->_db->selectValue($sql, [$id, $this->_deviceInfo->user]); + $uid = $this->_db->selectValue($sql, $params); return $uid; } catch (Horde_Db_Exception $e) { @@ -1719,9 +1724,236 @@ public function isDuplicatePIMAddition($id) public function isDuplicatePIMChange($uid, $synckey) { $sql = 'SELECT count(*) FROM ' . $this->_syncMapTable - . ' WHERE message_uid = ? AND sync_user = ? AND sync_key = ?'; + . ' WHERE message_uid = ? AND sync_user = ? AND sync_key = ?' + . ' AND sync_devid = ?'; + try { + return (bool) $this->_db->selectValue( + $sql, + [$uid, $this->_deviceInfo->user, $synckey, $this->_deviceInfo->id] + ); + } catch (Horde_Db_Exception $e) { + throw new Horde_ActiveSync_Exception($e); + } + } + + /** + * @since 3.0.3 + */ + public function getAppliedPIMChange($serverid, $synckey) + { + if (empty($this->_collection['serverid'])) { + return null; + } + $clientid = self::draftModifyClientId( + $this->_collection['serverid'], + $serverid + ); + $sql = 'SELECT message_uid FROM ' . $this->_syncMapTable + . ' WHERE sync_clientid = ? AND sync_key = ? AND sync_user = ?' + . ' AND sync_devid = ? AND sync_folderid = ?'; + try { + $uid = $this->_db->selectValue( + $sql, + [ + $clientid, + $synckey, + $this->_deviceInfo->user, + $this->_deviceInfo->id, + $this->_collection['serverid'], + ] + ); + } catch (Horde_Db_Exception $e) { + throw new Horde_ActiveSync_Exception($e); + } + if ($uid === null || $uid === false) { + return null; + } + + return ['id' => $uid, 'mod' => 0, 'flags' => []]; + } + + /** + * @since 3.0.3 + */ + public function recordAppliedPIMChange($oldId, array $stat, $synckey) + { + if (empty($this->_collection['serverid']) || empty($stat['id'])) { + return; + } + $this->_insertMapClientIdRow( + self::draftModifyClientId($this->_collection['serverid'], $oldId), + $stat['id'], + $this->_collection['serverid'], + $synckey, + !empty($stat['mod']) ? $stat['mod'] : 0 + ); + } + + /** + * @since 3.0.3 + */ + public function recordPIMAddition($clientid, $uid, $folderId, $synckey = null) + { + if ($clientid === '' || $clientid === null || $clientid === false) { + return; + } + $syncKey = $synckey ?: (empty($this->_syncKey) + ? $this->getLatestSynckeyForCollection($this->_collection['id'] ?? '') + : $this->_syncKey); + $this->_insertMapClientIdRow($clientid, $uid, $folderId, $syncKey, time()); + } + + /** + * @since 3.0.3 + */ + public function isMailMapChangeApplied($uid, $type, $synckey = null) + { + if (empty($this->_collection['serverid'])) { + return false; + } + $column = null; + switch ($type) { + case Horde_ActiveSync::CHANGE_TYPE_DELETE: + $column = 'sync_deleted'; + break; + case Horde_ActiveSync::CHANGE_TYPE_FLAGS: + // Any flag column counts as applied for short-circuit. + $sql = 'SELECT COUNT(*) FROM ' . $this->_syncMailMapTable + . ' WHERE message_uid = ? AND sync_devid = ? AND sync_user = ?' + . ' AND sync_folderid = ? AND (' + . 'sync_read IS NOT NULL OR sync_flagged IS NOT NULL' + . ' OR sync_category IS NOT NULL)'; + $params = [ + $uid, + $this->_deviceInfo->id, + $this->_deviceInfo->user, + $this->_collection['serverid'], + ]; + if ($synckey !== null) { + $sql .= ' AND sync_key = ?'; + $params[] = $synckey; + } + try { + return (bool) $this->_db->selectValue($sql, $params); + } catch (Horde_Db_Exception $e) { + throw new Horde_ActiveSync_Exception($e); + } + case Horde_ActiveSync::CHANGE_TYPE_DRAFT: + $column = 'sync_draft'; + break; + case Horde_ActiveSync::CHANGE_TYPE_CHANGE: + $column = 'sync_changed'; + break; + default: + return false; + } + $sql = 'SELECT COUNT(*) FROM ' . $this->_syncMailMapTable + . ' WHERE message_uid = ? AND sync_devid = ? AND sync_user = ?' + . ' AND sync_folderid = ? AND ' . $column . ' IS NOT NULL' + . ' AND ' . $column . ' = ?'; + $params = [ + $uid, + $this->_deviceInfo->id, + $this->_deviceInfo->user, + $this->_collection['serverid'], + true, + ]; + if ($synckey !== null) { + $sql .= ' AND sync_key = ?'; + $params[] = $synckey; + } + try { + return (bool) $this->_db->selectValue($sql, $params); + } catch (Horde_Db_Exception $e) { + throw new Horde_ActiveSync_Exception($e); + } + } + + /** + * @since 3.0.3 + */ + public function getAppliedMailMove($oldUid, $synckey) + { + if (empty($this->_collection['serverid'])) { + return null; + } + $clientid = self::mailMoveClientId( + $this->_collection['serverid'], + $oldUid + ); + $sql = 'SELECT message_uid FROM ' . $this->_syncMapTable + . ' WHERE sync_clientid = ? AND sync_key = ? AND sync_user = ?' + . ' AND sync_devid = ?'; + try { + $uid = $this->_db->selectValue( + $sql, + [ + $clientid, + $synckey, + $this->_deviceInfo->user, + $this->_deviceInfo->id, + ] + ); + } catch (Horde_Db_Exception $e) { + throw new Horde_ActiveSync_Exception($e); + } + if ($uid === null || $uid === false) { + return null; + } + + return $uid; + } + + /** + * @since 3.0.3 + */ + public function recordAppliedMailMove($oldUid, $newUid, $synckey, $dstFolderId) + { + if (empty($this->_collection['serverid'])) { + return; + } + $this->_insertMapClientIdRow( + self::mailMoveClientId($this->_collection['serverid'], $oldUid), + $newUid, + $dstFolderId, + $synckey, + time() + ); + } + + /** + * Insert a sync_map row keyed by sync_clientid. + * + * @param string $clientid + * @param string|integer $messageUid + * @param string $folderId + * @param string $synckey + * @param integer $modtime + * + * @throws Horde_ActiveSync_Exception + */ + protected function _insertMapClientIdRow( + $clientid, + $messageUid, + $folderId, + $synckey, + $modtime + ) { + $sql = 'INSERT INTO ' . $this->_syncMapTable + . ' (message_uid, sync_modtime, sync_key, sync_devid,' + . ' sync_folderid, sync_user, sync_clientid, sync_deleted)' + . ' VALUES (?, ?, ?, ?, ?, ?, ?, ?)'; try { - return $this->_db->selectValue($sql, [$uid, $this->_deviceInfo->user, $synckey]); + $this->_db->insert($sql, [ + (string) $messageUid, + $modtime, + $synckey, + $this->_deviceInfo->id, + $folderId, + $this->_deviceInfo->user, + $clientid, + false, + ]); } catch (Horde_Db_Exception $e) { throw new Horde_ActiveSync_Exception($e); } diff --git a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php new file mode 100644 index 00000000..f5e7ad35 --- /dev/null +++ b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php @@ -0,0 +1,263 @@ + + * @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 idempotent email Sync import (Issue #85). + */ +class Horde_ActiveSync_Connector_ImporterIdempotentImportTest extends TestCase +{ + public function testDraftModifyRetryDoesNotCallChangeMessageAgain(): void + { + $synckey = '{uuid}5'; + $oldUid = '100'; + $newUid = '101'; + $message = $this->_draftMailMessage('Draft body', 'Meeting notes'); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->expects($this->once()) + ->method('getAppliedPIMChange') + ->with($oldUid, $synckey) + ->willReturn(['id' => $newUid]); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->never())->method('changeMessage'); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $oldUid, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + $synckey + ); + + $this->assertSame($newUid, $stat['id']); + $this->assertSame(bin2hex('Meeting notes'), $stat['conversationid']); + $this->assertArrayHasKey('conversationindex', $stat); + } + + public function testDraftModifyRecordsMapBeforeMailmapUpdate(): void + { + $synckey = '{uuid}5'; + $oldUid = '100'; + $newUid = '101'; + $message = $this->_draftMailMessage('Edited', 'Subject'); + $callOrder = []; + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + $state->expects($this->once()) + ->method('recordAppliedPIMChange') + ->with($oldUid, $this->callback(function ($stat) use ($newUid) { + return is_array($stat) && ($stat['id'] ?? null) == $newUid; + }), $synckey) + ->willReturnCallback(function () use (&$callOrder) { + $callOrder[] = 'record'; + }); + $state->expects($this->exactly(2)) + ->method('updateState') + ->willReturnCallback(function ($type) use (&$callOrder) { + $callOrder[] = 'update:' . $type; + }); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('getUser')->willReturn('alice@example.com'); + $driver->expects($this->once()) + ->method('changeMessage') + ->willReturn([ + 'id' => $newUid, + 'mod' => 0, + 'flags' => [], + 'atchash' => [], + ]); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $oldUid, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + $synckey + ); + + $this->assertSame($newUid, $stat['id']); + $this->assertSame( + [ + 'record', + 'update:' . Horde_ActiveSync::CHANGE_TYPE_DRAFT, + 'update:' . Horde_ActiveSync::CHANGE_TYPE_DELETE, + ], + $callOrder + ); + } + + public function testDraftAddDuplicateClientIdSkipsChangeMessage(): void + { + $clientid = 'client-draft-1'; + $uid = '200'; + $message = $this->_draftMailMessage('New draft', 'Hello'); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isDuplicatePIMAddition') + ->with($clientid) + ->willReturn($uid); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->never())->method('changeMessage'); + + $importer = $this->_importer($state, $driver); + $result = $importer->importMessageChange( + false, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + $clientid, + Horde_ActiveSync::CLASS_EMAIL, + '{uuid}5' + ); + + $this->assertSame($uid, $result); + } + + public function testRemoveAlreadyMappedAsDeletedSkipsDriver(): void + { + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isMailMapChangeApplied') + ->with('100', Horde_ActiveSync::CHANGE_TYPE_DELETE) + ->willReturn(true); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->never())->method('deleteMessage'); + $driver->method('getSyncStamp')->willReturn(1); + + $importer = $this->_importer($state, $driver); + $deleted = $importer->importMessageDeletion( + ['100'], + Horde_ActiveSync::CLASS_EMAIL + ); + + $this->assertSame(['100'], $deleted); + } + + public function testMoveRetryReturnsPriorMapping(): void + { + $synckey = '{uuid}5'; + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getCurrentSyncKey')->willReturn($synckey); + $state->method('getAppliedMailMove') + ->with('100', $synckey) + ->willReturn('999'); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->never())->method('moveMessage'); + $driver->method('getSyncStamp')->willReturn(1); + + $collections = $this->createMock(Horde_ActiveSync_Collections::class); + $collections->method('getCollectionClass') + ->willReturn(Horde_ActiveSync::CLASS_EMAIL); + $collections->method('getBackendIdForFolderUid') + ->willReturn('Trash'); + + $as = $this->createMock(Horde_ActiveSync::class); + $as->driver = $driver; + $as->method('getCollectionsObject')->willReturn($collections); + + $importer = new Horde_ActiveSync_Connector_Importer($as); + $importer->setLogger(new Horde_Log_Logger(new Horde_Log_Handler_Null())); + $importer->init($state, 'folder-uid', 0); + + $ref = new ReflectionClass($importer); + $folderId = $ref->getProperty('_folderId'); + $folderId->setAccessible(true); + $folderId->setValue($importer, 'INBOX/Drafts'); + + $result = $importer->importMessageMove(['100'], 'trash-uid'); + + $this->assertSame(['100' => '999'], $result['results']); + $this->assertSame([], $result['missing']); + } + + public function testFlagModifyRetryUnderSameSyncKeySkipsDriver(): void + { + $synckey = '{uuid}5'; + $message = new Horde_ActiveSync_Message_Mail([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + $message->read = Horde_ActiveSync_Message_Mail::FLAG_READ_SEEN; + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('isMailMapChangeApplied') + ->with('50', Horde_ActiveSync::CHANGE_TYPE_FLAGS, $synckey) + ->willReturn(true); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->expects($this->never())->method('changeMessage'); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + '50', + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + $synckey + ); + + $this->assertSame('50', $stat['id']); + } + + /** + * @return Horde_ActiveSync_Connector_Importer + */ + protected function _importer($state, $driver) + { + $collections = $this->createMock(Horde_ActiveSync_Collections::class); + $collections->method('getBackendIdForFolderUid') + ->willReturn('INBOX/Drafts'); + + $as = $this->createMock(Horde_ActiveSync::class); + $as->driver = $driver; + $as->method('getCollectionsObject')->willReturn($collections); + + $importer = new Horde_ActiveSync_Connector_Importer($as); + $importer->setLogger(new Horde_Log_Logger(new Horde_Log_Handler_Null())); + $importer->init($state, 'folder-uid', 0); + + return $importer; + } + + protected function _draftMailMessage(string $body, string $subject): Horde_ActiveSync_Message_Mail + { + $message = new Horde_ActiveSync_Message_Mail([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + $message->to = 'alice@example.com'; + $message->subject = $subject; + $airsyncBody = new Horde_ActiveSync_Message_AirSyncBaseBody([ + 'protocolversion' => Horde_ActiveSync::VERSION_SIXTEEN, + ]); + $airsyncBody->type = Horde_ActiveSync::BODYPREF_TYPE_PLAIN; + $airsyncBody->data = $body; + $message->airsyncbasebody = $airsyncBody; + + return $message; + } +} From a049ba615c781e30e534ae8419d9d37b7ea05759 Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Mon, 20 Jul 2026 07:30:27 +0200 Subject: [PATCH 2/3] fix(sync): skip mailmap lookups for non-email deletes Guard importMessageDeletion mailmap checks to CLASS_EMAIL so Notes (and other PIM) UUID server ids are not queried against the integer mailmap.message_uid column. Reject non-integer UIDs in isMailMapChangeApplied and roll back an open DB transaction on query failure to avoid PostgreSQL 22P02/25P02 cascades during Sync. --- lib/Horde/ActiveSync/Connector/Importer.php | 11 +- lib/Horde/ActiveSync/State/Base.php | 3 + lib/Horde/ActiveSync/State/Mongo.php | 8 ++ lib/Horde/ActiveSync/State/Sql.php | 39 +++++++ .../ImporterIdempotentImportTest.php | 34 ++++++ .../Sql/MailMapChangeAppliedTest.php | 102 ++++++++++++++++++ 6 files changed, 193 insertions(+), 4 deletions(-) create mode 100644 test/unit/Horde/ActiveSync/StateTest/Sql/MailMapChangeAppliedTest.php diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index 352fb763..9549135c 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -440,10 +440,13 @@ public function importMessageDeletion(array $ids, $class, $instanceids = false) if (strpos((string) $id, 'IGNORESMS_') === 0) { continue; } - if ($this->_state->isMailMapChangeApplied( - $id, - Horde_ActiveSync::CHANGE_TYPE_DELETE - )) { + // mailmap.message_uid is an IMAP integer; never query it for + // Notes/Calendar/Contacts/Tasks (UUID string server ids). + if ($class == Horde_ActiveSync::CLASS_EMAIL + && $this->_state->isMailMapChangeApplied( + $id, + Horde_ActiveSync::CHANGE_TYPE_DELETE + )) { $already[] = $id; } else { $toDelete[] = $id; diff --git a/lib/Horde/ActiveSync/State/Base.php b/lib/Horde/ActiveSync/State/Base.php index d0e261b0..c650cccc 100644 --- a/lib/Horde/ActiveSync/State/Base.php +++ b/lib/Horde/ActiveSync/State/Base.php @@ -1575,6 +1575,9 @@ abstract public function recordPIMAddition($clientid, $uid, $folderId, $synckey * Whether mailmap already records a client-origin change of $type for $uid * (optional SyncKey scope for flag changes). * + * Non-integer UIDs (Notes/Calendar/Contacts/Tasks) return false without + * querying mailmap — message_uid is an IMAP integer column. + * * @param string|integer $uid Message UID. * @param string $type Horde_ActiveSync::CHANGE_TYPE_* constant. * @param string|null $synckey If set, require this SyncKey (flag retries). diff --git a/lib/Horde/ActiveSync/State/Mongo.php b/lib/Horde/ActiveSync/State/Mongo.php index 7a75471c..8f3d94bb 100644 --- a/lib/Horde/ActiveSync/State/Mongo.php +++ b/lib/Horde/ActiveSync/State/Mongo.php @@ -1838,6 +1838,14 @@ public function isMailMapChangeApplied($uid, $type, $synckey = null) if (empty($this->_collection['serverid'])) { return false; } + // mailmap is email-only (IMAP integer UIDs); ignore PIM UUID ids. + if (is_int($uid)) { + if ($uid < 0) { + return false; + } + } elseif (!is_string($uid) || $uid === '' || !ctype_digit($uid)) { + return false; + } $query = [ self::MESSAGE_UID => (string) $uid, self::SYNC_DEVID => $this->_deviceInfo->id, diff --git a/lib/Horde/ActiveSync/State/Sql.php b/lib/Horde/ActiveSync/State/Sql.php index 6c8c3fb7..2ffc4d60 100644 --- a/lib/Horde/ActiveSync/State/Sql.php +++ b/lib/Horde/ActiveSync/State/Sql.php @@ -1811,6 +1811,11 @@ public function isMailMapChangeApplied($uid, $type, $synckey = null) if (empty($this->_collection['serverid'])) { return false; } + // PostgreSQL mailmap.message_uid is integer (IMAP UID). Non-mail + // collections use UUID strings; querying them aborts the txn (22P02). + if (!$this->_isMailMapMessageUid($uid)) { + return false; + } $column = null; switch ($type) { case Horde_ActiveSync::CHANGE_TYPE_DELETE: @@ -1836,6 +1841,7 @@ public function isMailMapChangeApplied($uid, $type, $synckey = null) try { return (bool) $this->_db->selectValue($sql, $params); } catch (Horde_Db_Exception $e) { + $this->_rollbackDbOnFailure(); throw new Horde_ActiveSync_Exception($e); } case Horde_ActiveSync::CHANGE_TYPE_DRAFT: @@ -1865,10 +1871,43 @@ public function isMailMapChangeApplied($uid, $type, $synckey = null) try { return (bool) $this->_db->selectValue($sql, $params); } catch (Horde_Db_Exception $e) { + $this->_rollbackDbOnFailure(); throw new Horde_ActiveSync_Exception($e); } } + /** + * Whether $uid is a valid mailmap message_uid (IMAP integer). + * + * @param mixed $uid Candidate server id. + * + * @return boolean + */ + protected function _isMailMapMessageUid($uid) + { + if (is_int($uid)) { + return $uid >= 0; + } + + return is_string($uid) && $uid !== '' && ctype_digit($uid); + } + + /** + * Roll back an open DB transaction after a failed query so PostgreSQL + * does not leave the connection in SQLSTATE 25P02 (aborted txn). + */ + protected function _rollbackDbOnFailure() + { + if (!$this->_db->transactionStarted()) { + return; + } + try { + $this->_db->rollbackDbTransaction(); + } catch (Horde_Db_Exception $e) { + $this->_logger->err($e->getMessage()); + } + } + /** * @since 3.0.3 */ diff --git a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php index f5e7ad35..451e0dc2 100644 --- a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php +++ b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php @@ -157,6 +157,40 @@ public function testRemoveAlreadyMappedAsDeletedSkipsDriver(): void $this->assertSame(['100'], $deleted); } + public function testNotesRemoveDoesNotQueryMailMap(): void + { + $noteUid = '6a5a2daf-881c-4a66-b8cc-574e00000000'; + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->expects($this->never())->method('isMailMapChangeApplied'); + $state->expects($this->once()) + ->method('updateState') + ->with( + Horde_ActiveSync::CHANGE_TYPE_DELETE, + $this->callback(function ($change) use ($noteUid) { + return ($change['id'] ?? null) === $noteUid; + }), + Horde_ActiveSync::CHANGE_ORIGIN_PIM, + 'alice@example.com' + ); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('getUser')->willReturn('alice@example.com'); + $driver->method('getSyncStamp')->willReturn(1); + $driver->expects($this->once()) + ->method('deleteMessage') + ->with($this->anything(), [$noteUid]) + ->willReturn([$noteUid]); + + $importer = $this->_importer($state, $driver); + $deleted = $importer->importMessageDeletion( + [$noteUid], + Horde_ActiveSync::CLASS_NOTES + ); + + $this->assertSame([$noteUid], $deleted); + } + public function testMoveRetryReturnsPriorMapping(): void { $synckey = '{uuid}5'; diff --git a/test/unit/Horde/ActiveSync/StateTest/Sql/MailMapChangeAppliedTest.php b/test/unit/Horde/ActiveSync/StateTest/Sql/MailMapChangeAppliedTest.php new file mode 100644 index 00000000..ea671be0 --- /dev/null +++ b/test/unit/Horde/ActiveSync/StateTest/Sql/MailMapChangeAppliedTest.php @@ -0,0 +1,102 @@ + + * @license http://www.horde.org/licenses/gpl GPLv2 + * @category Horde + * @package Horde_ActiveSync + * @subpackage UnitTests + */ + +namespace Horde\ActiveSync\StateTest\Sql; + +use PHPUnit\Framework\TestCase; +use Horde_ActiveSync; +use Horde_ActiveSync_Exception; +use Horde_ActiveSync_Log_Logger; +use Horde_ActiveSync_State_Sql; +use Horde_Db_Exception; +use Horde_Log_Handler_Null; +use ReflectionClass; + +class MailMapChangeAppliedTest extends TestCase +{ + public function testNonIntegerUidSkipsMailmapQuery(): void + { + $db = $this->createMock(\Horde_Db_Adapter::class); + $db->expects($this->never())->method('selectValue'); + + $state = $this->_newState($db); + $this->assertFalse( + $state->isMailMapChangeApplied( + '6a5a2daf-881c-4a66-b8cc-574e00000000', + Horde_ActiveSync::CHANGE_TYPE_DELETE + ) + ); + } + + public function testIntegerUidQueriesMailmap(): void + { + $db = $this->createMock(\Horde_Db_Adapter::class); + $db->expects($this->once()) + ->method('selectValue') + ->willReturn(1); + + $state = $this->_newState($db); + $this->assertTrue( + $state->isMailMapChangeApplied( + '100', + Horde_ActiveSync::CHANGE_TYPE_DELETE + ) + ); + } + + public function testDbFailureRollsBackOpenTransaction(): void + { + $db = $this->createMock(\Horde_Db_Adapter::class); + $db->method('selectValue') + ->willThrowException(new Horde_Db_Exception('SQLSTATE[22P02]')); + $db->method('transactionStarted')->willReturn(true); + $db->expects($this->once())->method('rollbackDbTransaction'); + + $state = $this->_newState($db); + + $this->expectException(Horde_ActiveSync_Exception::class); + $state->isMailMapChangeApplied( + 100, + Horde_ActiveSync::CHANGE_TYPE_DELETE + ); + } + + protected function _newState($db): Horde_ActiveSync_State_Sql + { + $state = new Horde_ActiveSync_State_Sql(['db' => $db]); + $ref = new ReflectionClass($state); + + $logger = $ref->getProperty('_logger'); + $logger->setAccessible(true); + $logger->setValue( + $state, + new Horde_ActiveSync_Log_Logger(new Horde_Log_Handler_Null()) + ); + + $device = $ref->getProperty('_deviceInfo'); + $device->setAccessible(true); + $device->setValue($state, (object) [ + 'id' => 'device', + 'user' => 'alice@example.com', + ]); + + $collection = $ref->getProperty('_collection'); + $collection->setAccessible(true); + $collection->setValue($state, [ + 'id' => 'Fmail', + 'serverid' => 'INBOX', + 'class' => Horde_ActiveSync::CLASS_EMAIL, + ]); + + return $state; + } +} From 06873aead66363940dfd610a3201dfeffe6309b9 Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Mon, 20 Jul 2026 08:15:18 +0200 Subject: [PATCH 3/3] fix(sync): echo client ServerId in Draft Modify replies IMAP append+delete gives an edited Draft a new UID, but MS-ASCMD Sync Responses must reference the ServerId the client sent. Returning the new UID made Gmail reject the response and re-send the whole Drafts batch under the old SyncKey. _draftModifyStat() now returns the client ServerId as 'id' (the new UID stays in the applied map and mailmap and still feeds conversationindex). Also fall back to a synthetic conversationid for drafts without a subject, since empty conversation data suppresses the Modify reply entirely. --- lib/Horde/ActiveSync/Connector/Importer.php | 25 +++++++--- .../ImporterIdempotentImportTest.php | 46 ++++++++++++++++++- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/lib/Horde/ActiveSync/Connector/Importer.php b/lib/Horde/ActiveSync/Connector/Importer.php index 9549135c..dc2ccb6c 100644 --- a/lib/Horde/ActiveSync/Connector/Importer.php +++ b/lib/Horde/ActiveSync/Connector/Importer.php @@ -164,12 +164,14 @@ public function importMessageChange( && ($applied = $this->_state->getAppliedPIMChange($id, $synckey))) { $this->_logger->notice( sprintf( - 'Duplicate draft modify for %s under %s; returning %s', + 'Duplicate draft modify for %s under %s; mapped to %s', $id, $synckey, $applied['id'] ) ); + // Reply ServerEntryId stays $id (client ServerId); $applied['id'] + // is only the post-append IMAP UID used for conversationindex. return $this->_draftModifyStat($applied['id'], $message, $synckey, $id); } @@ -357,10 +359,14 @@ public function importMessageChange( * Build a Draft Modify stat array suitable for Sync replies, including * conversation fields so EAS 16 clients accept the response on retry. * - * @param string|integer $newId + * SyncReplies must echo the client's ServerEntryId ($oldId). IMAP + * append+delete yields a new UID ($newId) that only the applied map and + * mailmap track; putting it in Replies makes Gmail reject the SyncKey. + * + * @param string|integer $newId Post-append IMAP UID (conversationindex). * @param Horde_ActiveSync_Message_Base $message * @param string $synckey - * @param string|integer $oldId + * @param string|integer $oldId Client ServerId for SyncReplies. * @param array $stat Optional driver stat to merge (atchash, etc.). * * @return array @@ -374,19 +380,24 @@ protected function _draftModifyStat( ) { $out = array_merge( [ - 'id' => $newId, + 'id' => $oldId, 'mod' => 0, 'flags' => [], ], $stat ); - $out['id'] = $newId; + // Force client ServerId even if $stat carried the new IMAP UID. + $out['id'] = $oldId; $out['serverid'] = $this->_folderId; // Stable conversation fields (not time()) so retries emit the same - // SyncReplies shape Gmail requires for Drafts up-sync. + // SyncReplies shape Gmail requires for Drafts up-sync. Never leave + // conversationid empty: Sync skips email Modify replies when both + // conversations and atchash are empty (!empty('') is false). if ($message instanceof Horde_ActiveSync_Message_Mail) { $subject = (string) $message->subject; - $out['conversationid'] = bin2hex($subject); + $out['conversationid'] = bin2hex( + $subject !== '' ? $subject : ('draft:' . $oldId) + ); $out['conversationindex'] = crc32($synckey . ':' . $oldId . ':' . $newId); } diff --git a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php index 451e0dc2..d89f4514 100644 --- a/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php +++ b/test/unit/Horde/ActiveSync/Connector/ImporterIdempotentImportTest.php @@ -49,7 +49,8 @@ public function testDraftModifyRetryDoesNotCallChangeMessageAgain(): void $synckey ); - $this->assertSame($newUid, $stat['id']); + // SyncReplies must echo the client's ServerId, not the new IMAP UID. + $this->assertSame($oldUid, $stat['id']); $this->assertSame(bin2hex('Meeting notes'), $stat['conversationid']); $this->assertArrayHasKey('conversationindex', $stat); } @@ -99,7 +100,7 @@ public function testDraftModifyRecordsMapBeforeMailmapUpdate(): void $synckey ); - $this->assertSame($newUid, $stat['id']); + $this->assertSame($oldUid, $stat['id']); $this->assertSame( [ 'record', @@ -110,6 +111,47 @@ public function testDraftModifyRecordsMapBeforeMailmapUpdate(): void ); } + public function testDraftModifyReplyUsesClientServerIdWhenSubjectEmpty(): void + { + $synckey = '{uuid}5'; + $oldUid = '100'; + $newUid = '101'; + $message = $this->_draftMailMessage('Body only', ''); + + $state = $this->createMock(Horde_ActiveSync_State_Base::class); + $state->method('getAppliedPIMChange')->willReturn(null); + $state->expects($this->once()) + ->method('recordAppliedPIMChange') + ->with($oldUid, $this->callback(function ($stat) use ($newUid) { + return is_array($stat) && ($stat['id'] ?? null) == $newUid; + }), $synckey); + $state->expects($this->exactly(2))->method('updateState'); + + $driver = $this->createMock(Horde_ActiveSync_Driver_Base::class); + $driver->method('getUser')->willReturn('alice@example.com'); + $driver->expects($this->once()) + ->method('changeMessage') + ->willReturn([ + 'id' => $newUid, + 'mod' => 0, + 'flags' => [], + ]); + + $importer = $this->_importer($state, $driver); + $stat = $importer->importMessageChange( + $oldUid, + $message, + $this->createMock(Horde_ActiveSync_Device::class), + false, + Horde_ActiveSync::CLASS_EMAIL, + $synckey + ); + + $this->assertSame($oldUid, $stat['id']); + $this->assertSame(bin2hex('draft:' . $oldUid), $stat['conversationid']); + $this->assertNotSame('', $stat['conversationid']); + } + public function testDraftAddDuplicateClientIdSkipsChangeMessage(): void { $clientid = 'client-draft-1';