net_imap: Fix assertion after UNSUBSCRIBE, remote STATUS parsing bug.

* imap_mbox_watcher: Fix missing break in case leading to
  EVENT_MAILBOX_UNSUBSCRIBE calling send_untagged_uidvalidity,
  leading to an assertion, when the event should just be ignored.
* remote_status_cached has logic to search for the mailbox name with
  quotes and then to repeat without quotes. Fix typo where the
  search was repeated again with quotes a second time.
* Respond with OK to UNSUBSCRIBE on a nonexistent folder, rather
  than NO.
* Add additional descriptive error message for when remote STATUS fails.

Related enhancements:
* Add a few soft assertions on failure paths.
* imap_client.c: Add assertions for invariant failures.

modules/mod_mail.c

@@ -1049,6 +1049,7 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c
 		fp = fopen(uidfile, "a");
 		if (unlikely(!fp)) {
 			bbs_error("fopen(%s) failed: %s\n", uidfile, strerror(errno));
+			bbs_soft_assert(0);
 		} else {
 			fclose(fp);
 			/* Now, the file should exist. Reopen it (it'll be empty) */

nets/net_imap.c

@@ -790,6 +790,7 @@ static void imap_mbox_watcher(struct mailbox_event *event)
 			/* We should basically do send_untagged_list,
 			 * however we can ignore currently since all mailboxes are implicitly subscribed
 			 * so no change occurs here. */
+			break;
 		/*! \todo If METADATA extension added, updates need to be sent here */
 		/*! \todo ACL changes should also be taken into account.
 		 * Fortunately, ACLs can be changed through the IMAP protocol which means we could get events for them.
@@ -3666,6 +3667,7 @@ static int sub_rename(const char *path, const char *prefix, const char *newprefi
 					res = -1;
 					break;
 				}
+				bbs_debug(3, "Renamed %s -> %s\n", oldpath, newpath);
 			}
 		}
 	}
@@ -3725,6 +3727,7 @@ static int handle_rename(struct imap_session *imap, char *s)
 			imap_reply(imap, "NO [SERVERBUG] System error");
 		} else {
 			struct mailbox_event e;
+			bbs_debug(3, "Renamed %s -> %s\n", oldpath, newpath);
 			imap_reply(imap, "OK RENAME completed");
 			mailbox_initialize_event(&e, EVENT_MAILBOX_RENAME, imap->node, imap->mbox, newpath);
 			e.oldmaildir = oldpath;
@@ -4557,10 +4560,23 @@ static int imap_process(struct imap_session *imap, char *s)
 		REQUIRE_ARGS(s);
 		STRIP_QUOTES(s);
 		IMAP_NO_READONLY(imap);
-		bbs_warning("Unsubscription attempt for %s for mailbox %d\n", S_IF(s), mailbox_id(imap->mbox));
-		imap_reply(imap, "NO [NOPERM] Permission denied");
-		imap_translate_dir(imap, s, fullmaildir, sizeof(fullmaildir), &myacl);
-		mailbox_dispatch_event_basic(EVENT_MAILBOX_UNSUBSCRIBE, imap->node, imap->mbox, s);
+		if (imap_translate_dir(imap, s, fullmaildir, sizeof(fullmaildir), &myacl)) {
+			/* Mailbox doesn't exist (some clients so RENAME, then UNSUBSCRIBE to the old mailbox,
+			 * but since it doesn't exist, there's nothing really to unsubscribe from anyways,
+			 * so just say OK. Otherwise, Thunderbird-based clients try to repeat the UNSUBSCRIBE
+			 * request again, so just placate them.
+			 *
+			 * This is technically not correct from a protocol perspective, since we should
+			 * store subscriptions independent of whether the mailbox exists or not,
+			 * but since we don't, this is the sanest thing to do for now. */
+			imap_reply(imap, "OK UNSUBSCRIBE ignored as mailbox does not exist");
+		} else {
+			/* XXX We don't store subscriptions, so this isn't really the right error,
+			 * but return an error to indicate we are not complying with the client's request. */
+			bbs_debug(1, "Ignoring unsubscription attempt for %s for mailbox %d\n", S_IF(s), mailbox_id(imap->mbox));
+			imap_reply(imap, "NO [NOPERM] Permission denied");
+		}
+		mailbox_dispatch_event_basic(EVENT_MAILBOX_UNSUBSCRIBE, imap->node, imap->mbox, fullmaildir);
 	} else if (!strcasecmp(command, "GENURLAUTH")) {
 		char *resource, *mechanism;
 		REQUIRE_ARGS(s);

nets/net_imap/imap_client_list.c

@@ -226,7 +226,9 @@ static int remote_list(struct imap_client *client, struct list_command *lcmd, co
 		}
 
 		/* Always use remote_status, never direct passthrough, to avoid sending a tagged OK response each time */
-		remote_status(client, s, items, want_size);
+		if (remote_status(client, s, items, want_size)) {
+			bbs_error("Remote STATUS failed for %s on client %s\n", s, client->name);
+		}
 		free(s);
 	}
 

nets/net_imap/imap_client_status.c

@@ -489,11 +489,11 @@ static int remote_status_cached(struct imap_client *client, const char *mb, char
 	snprintf(findstr, sizeof(findstr), "* STATUS \"%s\"", mb);
 	tmp = strstr(client->virtlist, findstr);
 	if (!tmp && !strchr(mb, ' ')) { /* Retry, without quotes, if the mailbox name has no spaces */
-		snprintf(findstr, sizeof(findstr), "* STATUS \"%s\"", mb);
+		snprintf(findstr, sizeof(findstr), "* STATUS %s", mb);
 		tmp = strstr(client->virtlist, findstr);
 	}
 	if (!tmp) {
-		bbs_warning("Cached LIST-STATUS response missing response for '%s'\n", mb);
+		bbs_warning("Cached LIST-STATUS response missing response for '%s' on client %s (response was '%s')\n", mb, client->name, client->virtlist);
 		return -1;
 	}
 	end = strchr(tmp, '\n');
@@ -582,22 +582,26 @@ ssize_t remote_status(struct imap_client *client, const char *remotename, const
 		for (;;) {
 			res = bbs_readline(tcpclient->rfd, &tcpclient->rldata, "\r\n", 5000);
 			if (res <= 0) {
+				bbs_warning("Failed to receive response to STATUS command\n");
 				return -1;
 			}
 			/* Tolerate unrelated untagged responses interleaved */
 			if (STARTS_WITH(buf, "* STATUS ")) {
 				break;
 			} else if (!STARTS_WITH(buf, "* ")) {
+				bbs_warning("Unexpected response: %s\n", buf);
 				return -1;
 			}
 		}
 		safe_strncpy(remote_status_resp, buf, sizeof(remote_status_resp)); /* Save the STATUS response from the server */
 		for (;;) {
 			res = bbs_readline(tcpclient->rfd, &tcpclient->rldata, "\r\n", 5000);
 			if (res <= 0) {
+				bbs_warning("Failed to receive response to STATUS command\n");
 				return -1;
 			}
 			if (STARTS_WITH(buf, "* ")) { /* Tolerate unrelated untagged responses interleaved */
+				bbs_warning("Unexpected response: %s\n", buf);
 				continue;
 			}
 			if (strncasecmp(buf, rtag, taglen)) {