Эх сурвалжийг харах

fix: address IMAP messages by UID consistently, not by sequence number

The IMAP client mixed the two message numbering spaces IMAP defines, in a
way that only stayed hidden because the trigger never ran.

ImapClient::search sent a plain SEARCH, which per RFC 3501 returns message
SEQUENCE NUMBERS - positional, renumbered on every expunge, meaningless
between sessions. Those numbers were stored in a field named uids and handed
to callers as uids. ImapClient::fetch then built the URL <mailbox>;UID=<n>,
which curl issues as a UID FETCH. So a sequence number was used as a UID.

The two coincide only in a mailbox that has never had a message deleted. In
any real mailbox UIDs run well above the sequence numbers, the UID FETCH
matches nothing, and curl returns CURLE_REMOTE_FILE_NOT_FOUND - surfacing as
the reported 'IMAP fetch failed: Remote file not found'.

This never showed up before because the path had never executed against a
live mailbox: wf_520f6f05's imap-trigger was not registered as scheduled
until the config-defaults fix, so search-then-fetch ran for the first time
today.

The failing fetch was the loud half. The flag operations had the same
confusion and failed silently:

- modify() built a ;UID= URL but sent a plain 'STORE <n> +FLAGS ...',
  so mark-read, flag and delete could act on a different message than the
  one fetched. Including 'STORE <n> +FLAGS (\\Deleted)'.
- move() sent a plain 'COPY <n>' and then deleted by the same route, so it
  could copy one email and delete another.
- fetch()'s peek restore removed \\Seen from whatever sat at that sequence
  position, not from the message just read.

Nothing dedupes on a stored cursor - imap-trigger.js keeps none, relying
entirely on UNSEEN plus mark-as-read - so correct \\Seen targeting is the only
thing standing between the trigger and reprocessing or skipping mail.

Every command is now UID-addressed: UID SEARCH, UID STORE, UID COPY, matching
the ;UID= URLs already in use and the uids field name. modify() and move()
also drop ;UID= from their URLs, which selects the mailbox only: leaving it
asks curl to fetch the message as well as run the command, and a fetch sets
\\Seen - which would have defeated markUnread outright.

UID SEARCH replies with the same untagged '* SEARCH' line as a plain SEARCH,
so the response parser is unchanged and correct for both; noted in a comment
so it is not later 'fixed' to look for '* UID SEARCH'.
fszontagh 1 сар өмнө
parent
commit
27bd6ad288

+ 30 - 12
src/runner/imap/imap_client.cpp

@@ -96,9 +96,14 @@ common::Result<SearchResult> ImapClient::search(
     LOG_INFO("IMAP search - mailbox: '{}', criteria: '{}', limit: {}",
              mailbox, criteria, limit);
 
-    // For IMAP SEARCH, we use CUSTOMREQUEST
+    // For IMAP SEARCH, we use CUSTOMREQUEST.
+    // "UID SEARCH", not plain "SEARCH": a plain SEARCH returns message sequence
+    // numbers, which renumber on every expunge and are meaningless between
+    // sessions. Every other operation here addresses messages by UID (the
+    // ";UID=" URL form, "UID STORE", "UID COPY"), so search must return UIDs
+    // as well or the numbers do not refer to the same messages.
     std::string url_path = escapeMailbox(mailbox);
-    std::string search_cmd = "SEARCH " + criteria;
+    std::string search_cmd = "UID SEARCH " + criteria;
 
     auto response = performCommand(url_path, search_cmd);
 
@@ -107,7 +112,10 @@ common::Result<SearchResult> ImapClient::search(
             "IMAP search failed: " + response.error);
     }
 
-    // Parse search results (format: "* SEARCH 1 2 3 4 5")
+    // Parse search results (format: "* SEARCH 1 2 3 4 5").
+    // UID SEARCH replies with the same untagged "* SEARCH" line as a plain
+    // SEARCH - only the numbers it carries differ - so this parser is correct
+    // for both and must not be "fixed" to look for "* UID SEARCH".
     SearchResult result;
     std::istringstream stream(response.data);
     std::string line;
@@ -149,7 +157,7 @@ common::Result<EmailContent> ImapClient::fetch(
     // If peek mode, mark the message as unread to undo the \Seen flag
     if (peek && response.error.empty()) {
         std::string store_path = escapeMailbox(mailbox);
-        std::string store_cmd = "STORE " + uid + " -FLAGS (\\Seen)";
+        std::string store_cmd = "UID STORE " + uid + " -FLAGS (\\Seen)";
         auto store_response = performCommand(store_path, store_cmd);
         if (!store_response.error.empty()) {
             LOG_WARN("Failed to remove \\Seen flag after peek fetch: {}", store_response.error);
@@ -302,19 +310,24 @@ common::Result<ModifyResult> ImapClient::modify(
     LOG_INFO("IMAP modify - mailbox: '{}', uid: '{}', action: '{}'",
              mailbox, uid, action);
 
-    std::string url_path = escapeMailbox(mailbox) + ";UID=" + uid;
+    // The URL selects the mailbox only. It must not carry ";UID=", which would
+    // ask curl to fetch the message as well as run the command below - and a
+    // fetch sets \Seen, which would defeat "markUnread" outright.
+    std::string url_path = escapeMailbox(mailbox);
     std::string command;
 
+    // "UID STORE", not "STORE": the caller passes a UID, and a plain STORE
+    // would read it as a sequence number and flag an unrelated message.
     if (action == "markRead") {
-        command = "STORE " + uid + " +FLAGS (\\Seen)";
+        command = "UID STORE " + uid + " +FLAGS (\\Seen)";
     } else if (action == "markUnread") {
-        command = "STORE " + uid + " -FLAGS (\\Seen)";
+        command = "UID STORE " + uid + " -FLAGS (\\Seen)";
     } else if (action == "flag") {
-        command = "STORE " + uid + " +FLAGS (\\Flagged)";
+        command = "UID STORE " + uid + " +FLAGS (\\Flagged)";
     } else if (action == "unflag") {
-        command = "STORE " + uid + " -FLAGS (\\Flagged)";
+        command = "UID STORE " + uid + " -FLAGS (\\Flagged)";
     } else if (action == "delete") {
-        command = "STORE " + uid + " +FLAGS (\\Deleted)";
+        command = "UID STORE " + uid + " +FLAGS (\\Deleted)";
     } else {
         return common::Error(common::ErrorCode::InvalidArgument,
             "Unknown action: " + action);
@@ -341,8 +354,13 @@ common::Result<ModifyResult> ImapClient::move(
     LOG_INFO("IMAP move - from: '{}', uid: '{}', to: '{}'",
              mailbox, uid, target_mailbox);
 
-    std::string url_path = escapeMailbox(mailbox) + ";UID=" + uid;
-    std::string command = "COPY " + uid + " " + target_mailbox;
+    // Mailbox-only URL and a UID-addressed COPY, for the same reasons as
+    // modify() above. This one is the most damaging to get wrong: a plain COPY
+    // copies whichever message currently sits at that sequence position, and
+    // the delete below then removes a message chosen the same way - so a move
+    // could copy one email and delete another.
+    std::string url_path = escapeMailbox(mailbox);
+    std::string command = "UID COPY " + uid + " " + target_mailbox;
 
     auto response = performCommand(url_path, command);