net_imap: Various IMAP-related fixes.

* net_imap: Fix passing imap->dir for untagged FETCH rather than the
  maildir of the affected message.
* net_imap: If multiple IDLE updates are received simultaneously from
  a remote server, properly flush them to our client. Previously, only
  the first one was flushed for a single poll() hit, leading to the
  remaining ones to be flushed only after the next command was received.
* net_imap: Tolerate untagged responses prior to OK after remote move/copy
  on destination server.
* net_imap: Fix uninitialized variable preventing nonsecure IMAP client
  proxy connections.
* tcpclient: Null terminate buffer if no data received to avoid
  printing previous response.
* oauth_helper.sh: Fix interactive command session post IMAP login.
* mod_oauth: Fix printing token creation time rather than expire time.

include/mod_mail.h

@@ -494,8 +494,8 @@ int maildir_ordered_traverse(const char *path, int (*on_file)(const char *dir_na
 int maildir_uidless_traverse(const char *path, int (*on_file)(const char *dir_name, const char *filename, int seqno, void *obj), void *obj);
 
 /* IMAP client */
-#define IMAP_CLIENT_EXPECT(client, s) if (bbs_tcp_client_expect(client, "\r\n", 1, 2000, s)) { bbs_debug(3, "Didn't receive expected '%s'\n", s); goto cleanup; }
-#define IMAP_CLIENT_EXPECT_EVENTUALLY(client, x, s) if (bbs_tcp_client_expect(client, "\r\n", x, 2000, s)) { bbs_debug(3, "Didn't receive expected '%s', got '%s'\n", s, (client)->rldata.buf); goto cleanup; }
+#define IMAP_CLIENT_EXPECT(client, s) if (bbs_tcp_client_expect(client, "\r\n", 1, 2500, s)) { bbs_debug(3, "Didn't receive expected '%s'\n", s); goto cleanup; }
+#define IMAP_CLIENT_EXPECT_EVENTUALLY(client, x, s) if (bbs_tcp_client_expect(client, "\r\n", x, 2500, s)) { bbs_debug(3, "Didn't receive expected '%s', got '%s'\n", s, (client)->rldata.buf); goto cleanup; }
 #define IMAP_CLIENT_SEND(client, fmt, ...) bbs_tcp_client_send(client, fmt "\r\n", ## __VA_ARGS__);
 
 #define IMAP_CAPABILITY_IDLE (1 << 0)

modules/mod_mail.c

@@ -1781,6 +1781,11 @@ static int imap_client_parse_capabilities(struct bbs_tcp_client *client, int *ca
 		PARSE_CAPABILITY("BINARY", IMAP_CAPABILITY_BINARY)
 		PARSE_CAPABILITY("MULTIAPPEND", IMAP_CAPABILITY_MULTIAPPEND)
 		/* Not currently used */
+		IGNORE_CAPABILITY("UNAUTHENTICATE")
+		IGNORE_CAPABILITY("QUOTA=RES-STORAGE")
+		IGNORE_CAPABILITY("QUOTA=RES-MESSAGE")
+		IGNORE_CAPABILITY("URLAUTH")
+		IGNORE_CAPABILITY("APPENDLIMIT") /* Will pass through as needed, no need to do anything with it here */
 		IGNORE_CAPABILITY("LOGIN-REFERRALS")
 		IGNORE_CAPABILITY("SORT=DISPLAY")
 		IGNORE_CAPABILITY("URL-PARTIAL")

modules/mod_oauth.c

@@ -266,9 +266,9 @@ static int fetch_token(struct oauth_client *client, char *buf, size_t len)
 		return 0;
 	} else if (client->tokentime) {
 		time_t ago = now - expiretime;
-		bbs_debug(5, "Token refresh required (expired %" TIME_T_FMT " seconds ago)\n", ago);
+		bbs_debug(3, "Token refresh required (expired %" TIME_T_FMT " seconds ago)\n", ago);
 	} else {
-		bbs_debug(5, "Token refresh required (no access token pre-seeded)\n");
+		bbs_debug(3, "Token refresh required (no access token pre-seeded)\n");
 	}
 
 	res = refresh_token(client);
@@ -434,7 +434,7 @@ static int cli_get_token(struct bbs_cli_args *a)
 				time_t diff;
 				match++;
 				bbs_mutex_lock(&client->lock);
-				diff = client->tokentime - now;
+				diff = client->tokentime + client->expires - now;
 				bbs_dprintf(a->fdout, "-- %s --\n", client->name);
 				if (client->userid) {
 					bbs_dprintf(a->fdout, "%-15s %u\n", "User ID:", client->userid);

modules/mod_webmail.c

@@ -3499,10 +3499,12 @@ static int on_poll_activity(struct ws_session *ws, void *data)
 	client->idlerefresh &= ~IDLE_REFRESH_STATUS; /* This doesn't count for needing a page refresh */
 	if (client->idlerefresh) { /* If there were multiple lines in the IDLE update, batch any updates up and send a single refresh */
 		int r = client->idlerefresh;
-		const char *reason = r & IDLE_REFRESH_EXISTS ? "EXISTS" : r & IDLE_REFRESH_FETCH ? "FETCH" : r & IDLE_REFRESH_EXPUNGE ? "EXPUNGE" : "";
+		/* EXPUNGE takes priority, since it involves a mailbox shrinking */
+		const char *reason = r & IDLE_REFRESH_EXPUNGE ? "EXPUNGE" : r & IDLE_REFRESH_EXISTS ? "EXISTS" : r & IDLE_REFRESH_FETCH ? "FETCH" : "";
 		/* In our case, since we're webmail, we can cheat a little and just refresh the current listing.
 		 * The nice thing is this handles both EXISTS and EXPUNGE responses just fine. */
 		idle_stop(ws, client);
+
 		REFRESH_LISTING(reason);
 		if (r & IDLE_REFRESH_EXISTS) {
 			/* Send the metadata for message with this sequence number as an unsolicited EXISTS.

nets/net_imap.c

@@ -76,6 +76,7 @@
  * - RFC 8508 REPLACE
  * - RFC 8514 SAVEDATE
  * - RFC 8970 PREVIEW
+ * - RFC 9208 QUOTA (partially)
  * - RFC 9394 PARTIAL
  * - RFC 9586 UIDONLY
  * - CLIENTID: https://datatracker.ietf.org/doc/html/draft-yu-imap-client-id-10
@@ -270,7 +271,7 @@ static int cli_imap_clients(struct bbs_cli_args *a)
 	struct imap_session *imap;
 	time_t now = time(NULL);
 
-	bbs_dprintf(a->fdout, "%4s %-25s %6s %4s %9s %s\n", "Node", "Name", "Active", "Dead", "Idle", "Age");
+	bbs_dprintf(a->fdout, "%4s %-25s %6s %4s %8s %10s %s\n", "Node", "Name", "Active", "Dead", "Idle", "Age", "TCP Client");
 	RWLIST_RDLOCK(&sessions);
 	RWLIST_TRAVERSE(&sessions, imap, entry) {
 		struct imap_client *client;
@@ -280,8 +281,8 @@ static int cli_imap_clients(struct bbs_cli_args *a)
 			time_t idle_elapsed = now - client->idlestarted;
 			bbs_soft_assert(now >= client->idlestarted);
 			print_time_elapsed(client->created, now, elapsed, sizeof(elapsed));
-			bbs_dprintf(a->fdout, "%4u %-25s %6s %4s %" TIME_T_FMT "/%4d %s\n",
-				imap->node->id, client->name, BBS_YN(client->active), BBS_YN(client->dead), client->idling ? idle_elapsed : 0, client->maxidlesec, elapsed);
+			bbs_dprintf(a->fdout, "%4u %-25s %6s %4s %3" TIME_T_FMT "/%4d %10s %p\n",
+				imap->node->id, client->name, BBS_YN(client->active), BBS_YN(client->dead), client->idling ? idle_elapsed : 0, client->maxidlesec, elapsed, &client->client);
 		}
 		RWLIST_UNLOCK(&imap->clients);
 	}
@@ -395,17 +396,22 @@ static int generate_mailbox_name(struct imap_session *imap, const char *restrict
 	userid = imap->node->user->id;
 	rootlen = strlen(mailbox_maildir(NULL));
 
-	if (mailbox_maildir_validate(fulldir)) {
+	if (mailbox_maildir_validate(maildir)) {
 		return -1;
 	}
 
+	/* This check is not redundant by mailbox_maildir_validate, since that
+	 * will return 0 if the argument is completely empty. */
+	bbs_assert(!strlen_zero(maildir));
+	bbs_assert(strlen(maildir) >= rootlen);
+
 	fulldir += rootlen;
 	if (*fulldir == '/') {
 		fulldir++;
 	}
 
 	if (strlen_zero(fulldir)) {
-		bbs_error("Empty maildir?\n");
+		bbs_error("Empty maildir? (%s)\n", maildir);
 		bbs_soft_assert(0);
 		return -1;
 	}
@@ -469,7 +475,7 @@ static int generate_mailbox_name(struct imap_session *imap, const char *restrict
 	return 0;
 }
 
-void send_untagged_fetch(struct imap_session *imap, int seqno, unsigned int uid, unsigned long modseq, const char *newflags)
+void send_untagged_fetch(struct imap_session *imap, const char *maildir, int seqno, unsigned int uid, unsigned long modseq, const char *newflags)
 {
 	struct imap_session *s;
 	char normalmsg[256];
@@ -498,7 +504,7 @@ void send_untagged_fetch(struct imap_session *imap, int seqno, unsigned int uid,
 		 * This also applies anywhere else generate_status is called.
 		 */
 		/*! \todo Ideally this would be a static mail store based callback, and then instead of imap->dir we could use e->maildir */
-		if (generate_mailbox_name(s, imap->dir, mboxname, sizeof(mboxname))) {
+		if (generate_mailbox_name(s, maildir, mboxname, sizeof(mboxname))) {
 			continue;
 		}
 		res = imap_notify_applicable(s, NULL, mboxname, imap->dir, IMAP_EVENT_FLAG_CHANGE);
@@ -2938,6 +2944,8 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
 								res = imap_client_send(destclient, "(%s) {%ld%s}\r\n", S_IF(flags), size, synchronizing ? "" : "+");
 							}
 						} else {
+							/* If we're moving/copying multiple messages, we do end up using the same tag
+							 * for each message, which is not ideal, but okay, since we're not pipelining. */
 							if (idate) {
 								res = imap_client_send_log(destclient, "%s APPEND \"%s\" (%s) \"%s\" {%ld%s}\r\n",
 									imap->tag, remotename, S_IF(flags), idate, size, synchronizing ? "" : "+");
@@ -3001,8 +3009,13 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
 			if (SWRITE(destclient->client.wfd, "\r\n") != STRLEN("\r\n")) {
 				goto cleanup;
 			}
-			/* Well, hopefully that all worked! */
-			IMAP_CLIENT_EXPECT(&destclient->client, " OK"); /* tagged OK */
+			/* Well, hopefully that all worked!
+			 * Use IMAP_CLIENT_EXPECT_EVENTUALLY since we may also get untagged EXISTS's for the copied messages. */
+
+			/*! \todo BUGBUG For any IMAP_CLIENT_EXPECT_EVENTUALLY, all the untagged responses that we "ignore
+			 * should actually pass through to the client directly, since an offline client will need those
+			 * in order to be synchronized. */
+			IMAP_CLIENT_EXPECT_EVENTUALLY(&destclient->client, appended + 1, " OK"); /* tagged OK */
 		}
 		if (move) {
 			/* Now that we copied everything to the destination mailbox, delete the source */
@@ -4178,8 +4191,17 @@ static int handle_idle(struct imap_session *imap)
 				bbs_warning("Got activity on a client that wasn't idling? (%s)\n", client->virtprefix);
 				continue;
 			} else if (imap->client == client) {
+				ssize_t bres;
 				/* This is the actual mailbox that is selected. Just relay anything we receive. */
-				_imap_reply(imap, "%s\r\n", tcpclient->rldata.buf);
+				do {
+					_imap_reply(imap, "%s\r\n", tcpclient->rldata.buf);
+					/* Okay, now because bbs_readline might have read MULTIPLE lines from the server,
+					 * call it again to make sure there isn't any further input.
+					 * (For example, this would happen if multiple EXPUNGEs occur simultaneously.)
+					 * We use a timeout of 0, because if there isn't another message ready already,
+					 * then we should just go back to the outer poll. */
+					bres = bbs_readline(tcpclient->rfd, &tcpclient->rldata, "\r\n", 0);
+				} while (bres > 0);
 			} else {
 				do {
 					int seqno;

nets/net_imap/imap.h

@@ -184,7 +184,16 @@ extern int imap_debug_level;
 		return 0; \
 	}
 
-void send_untagged_fetch(struct imap_session *imap, int seqno, unsigned int uid, unsigned long modseq, const char *newflags);
+/*!
+ * \brief Generate untagged FETCH messages for clients that have selected the current mailbox
+ * \param imap
+ * \param maildir Full path to maildir of the affected message
+ * \param seqno
+ * \param uid
+ * \param modseq
+ * \param newflags
+ */
+void send_untagged_fetch(struct imap_session *imap, const char *maildir, int seqno, unsigned int uid, unsigned long modseq, const char *newflags);
 
 int imap_in_range(struct imap_session *imap, const char *s, int num);
 

nets/net_imap/imap_client.c

@@ -734,7 +734,9 @@ struct imap_client *__imap_client_get_by_url(struct imap_session *imap, const ch
 		return NULL;
 	} else if (!strcmp(url.prot, "imaps")) {
 		secure = 1;
-	} else if (strcmp(url.prot, "imap")) {
+	} else if (!strcmp(url.prot, "imap")) {
+		secure = 0;
+	} else {
 		bbs_warning("Unsupported protocol: %s\n", url.prot);
 		return NULL;
 	}

nets/net_imap/imap_client_status.c

@@ -523,6 +523,10 @@ ssize_t remote_status(struct imap_client *client, const char *remotename, const
 	tag = client->imap->tag;
 	bbs_assert_exists(tag);
 
+	if (client->idling) {
+		imap_client_idle_stop(client);
+	}
+
 	/* In order for caching of SIZE to be reliable, we must invalidate it whenever anything
 	 * in the original STATUS response changes, including UIDNEXT/UIDVALIDITY. These are needed to
 	 * reliably detect changes to the mailbox (e.g. if a message is added and delete,

nets/net_imap/imap_server_flags.c

@@ -500,7 +500,7 @@ int maildir_msg_setflags_modseq(struct imap_session *imap, int seqno, const char
 	if (seqno) { /* Skip for merely translating flag mappings between maildirs */
 		unsigned int uid;
 		maildir_parse_uid_from_filename(filename, &uid);
-		send_untagged_fetch(imap, seqno, uid, modseq, newflags);
+		send_untagged_fetch(imap, dirpath, seqno, uid, modseq, newflags);
 	}
 	return 0;
 }

scripts/oauth_helper.sh

@@ -94,7 +94,8 @@ calculate_xoauth2() {
 
 server_connect() {
 	printf "Connecting to %s:%d\n" $IMAP_SERVER 993
-	sh -c "echo \"a1 AUTHENTICATE XOAUTH2 $XOAUTH2_ENCODED\"; while read x; do echo \"$x\"; done" | openssl s_client -quiet -connect $IMAP_SERVER:993 -crlf
+	# Since we are piping to openssl s_client, it will handle the LF to CR LF conversions for us
+	sh -c "echo \"a1 AUTHENTICATE XOAUTH2 $XOAUTH2_ENCODED\" && cat" | openssl s_client -quiet -connect $IMAP_SERVER:993 -crlf
 }
 
 read -r opt