fix(sync): skip no-op draft rewrites and treat \Deleted as removed

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,150 @@ 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 <torben@dannhauer.de>
+     *
+     * @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) {
+            $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;
+        }
+
+        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 $this->_logDraftMismatch($id, $field);
+            }
+        }
+
+        // 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
+        // empty <Flag/> 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 $this->_logDraftMismatch($id, 'flag');
+        }
+
+        if (!empty($message->categories)
+            && $message->categories != ($current->categories ?: [])) {
+            return $this->_logDraftMismatch($id, 'categories');
+        }
+
+        if ($this->_draftBodyString($body->data)
+            !== $this->_draftBodyString($current->airsyncbasebody->data)) {
+            return $this->_logDraftMismatch($id, 'body');
+        }
+
+        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;
+    }
+
+    /**
+     * 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.

lib/Horde/ActiveSync/Folder/Imap.php

@@ -1,3 +1,3 @@
 <?php
 
 /**
@@ -148,6 +148,17 @@
      */
     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 @@
         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 @@
         if (!empty($this->_draftUidAliases)) {
             $data['da'] = $this->_draftUidAliases;
         }
+        if ($this->_deletedSwept) {
+            $data['dsw'] = true;
+        }
 
         return $data;
     }
@@ -781,6 +813,7 @@
         $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);

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']),

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,29 @@ public function getChanges(array $options)
             $this->_logger->err($e->getMessage());
             throw new Horde_ActiveSync_Exception($e);
         }
-        $this->_folder->setRemoved($deleted->ids);
+        // 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.
+        $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)
             )
         );
 
@@ -201,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 <torben@dannhauer.de>
+     *
+     * @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.
      *
@@ -239,6 +298,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 +310,7 @@ protected function _buildModSeqChanges(
         &$changes,
         &$flags,
         &$categories,
+        &$flag_deleted,
         $fetch_ret,
         $options,
         $modseq
@@ -256,12 +320,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,