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.
* SUBSCRIBE/UNSUBSCRIBE: Don't trigger event if we return failure.
* 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.
* mod_mail_events: Emit warning if missing mandatory options.

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

include/mod_mail.h

@@ -344,6 +344,7 @@ int maildir_mktemp(const char *path, char *buf, size_t len, char *newbuf);
  * \note This operation is used by both the IMAP and POP3 servers, although UIDs are only relevant for IMAP.
  *       POP3 calls this function to ensure consistency for IMAP operations.
  * \note This operation internally maintains the .uidvalidity of a maildir directory.
+ * \note This function pretty much assumes as given that the specified maildir exists. Checks should be done prior to calling this function.
  * \todo This function should really be renamed maildir_get_next_uid, it's a maildir function, not a mailbox function. Currently, it's a misnomer.
  */
 unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, const char *directory, int allocate, unsigned int *newuidvalidity, unsigned int *newuidnext) __attribute__((nonnull (1, 3, 5, 6)));

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) */

modules/mod_mail_events.c

@@ -224,6 +224,7 @@ static int load_config(void)
 
 	/* General */
 	if (bbs_config_val_set_str(cfg, "general", "logfile", logfile, sizeof(logfile))) {
+		bbs_warning("No logfile specified, declining to load\n");
 		return -1;
 	}
 	fp = fopen(logfile, "a");

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;
@@ -4549,18 +4552,33 @@ static int imap_process(struct imap_session *imap, char *s)
 		/* Since we don't check for mailbox existence (and everything is always subscribed anyways), no real need to check ACLs here */
 		bbs_debug(1, "Ignoring subscription attempt for %s for mailbox %d\n", S_IF(s), mailbox_id(imap->mbox));
 		imap_reply(imap, "OK SUBSCRIBE completed"); /* Everything available is already subscribed anyways, so can't hurt */
-		imap_translate_dir(imap, s, fullmaildir, sizeof(fullmaildir), &myacl);
-		mailbox_dispatch_event_basic(EVENT_MAILBOX_SUBSCRIBE, imap->node, imap->mbox, fullmaildir);
+		if (!imap_translate_dir(imap, s, fullmaildir, sizeof(fullmaildir), &myacl)) {
+			/* Don't trigger event for nonexistent mailbox if the SUBSCRIBE failed */
+			mailbox_dispatch_event_basic(EVENT_MAILBOX_SUBSCRIBE, imap->node, imap->mbox, fullmaildir);
+		}
 	} else if (!strcasecmp(command, "UNSUBSCRIBE")) {
 		char fullmaildir[256];
 		int myacl;
 		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");
+			mailbox_dispatch_event_basic(EVENT_MAILBOX_UNSUBSCRIBE, imap->node, imap->mbox, fullmaildir);
+		} 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");
+		}
 	} 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)) {