net_imap: Properly wait for tagged response for proxied APPEND/MOVE.

Previously, we were not properly handling untagged responses
while waiting for the tagged response. Initially, not receiving
the tagged response immediately would cause failure, since we
were only checking if the first response read was expected.
The remote MOVE/COPY handler was updated previously to tolerate
untagged responses while waiting for the tagged response, but
these were not passed along to the client, as they should be.
A similar issue existed with APPENDs to a remote mailbox.

Now, in both cases, we properly wait for the tagged response, while
passing along everything received in the meantime to the client.

nets/net_imap.c

@@ -2622,15 +2622,12 @@ static int handle_append(struct imap_session *imap, char *s)
 		if (res < 0) {
 			return -1;
 		}
-		/* Relay response from remote, and no need to clean up */
-		res = bbs_readline(imap->client->client.rfd, &imap->client->client.rldata, "\r\n", SEC_MS(5));
-		if (res < 0) {
-			/* No need to clean up, since for remote server,
-			 * we're not appending seqnos/UIDs to a uintlist */
-			bbs_warning("APPEND tagged response not received from remote server?\n");
+		/* Relay response from remote, and no need to clean up.
+		 * We may (should) receive an untagged EXISTS for the uploaded message,
+		 * so use imap_client_wait_response to wait for the actual tagged response. */
+		if (imap_client_wait_response(imap->client, -1, SEC_MS(5))) {
 			return -1;
 		}
-		_imap_reply(imap, "%s\r\n", imap->client->client.buf);
 		return 0;
 	}
 
@@ -3035,13 +3032,16 @@ 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!
-			 * 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 */
+			/* Well, hopefully that all worked! Now, wait for confirmation of a successful upload.
+			 *
+			 * Here, we use imap_client_wait_response instead of IMAP_CLIENT_EXPECT_EVENTUALLY to wait for the tagged OK,
+			 * because any untagged data we receive prior to that should pass through directly to the client
+			 * (rather than being silently ignored/discarded as IMAP_CLIENT_EXPECT_EVENTUALLY does).
+			 * This way, the client can stay synchronized with the remote server, which is important
+			 * since APPEND can trigger untagged EXISTS's, etc. */
+			if (imap_client_wait_response(destclient, -1, SEC_MS(5))) {
+				goto cleanup;
+			}
 		}
 		if (move) {
 			/* Now that we copied everything to the destination mailbox, delete the source */

nets/net_imap/imap_client.c

@@ -440,6 +440,7 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const
 		}
 		res = bbs_readline(tcpclient->rfd, &tcpclient->rldata, "\r\n", ms);
 		if (res < 0) { /* Could include remote server disconnect */
+			bbs_warning("Failed to receive expected response from remote server\n");
 			return res;
 		}
 		if (cb) {
@@ -469,9 +470,13 @@ static ssize_t client_command_passthru(struct imap_client *client, int fd, const
 #endif
 		if (!strncmp(buf, tag, (size_t) taglen)) {
 			imap_debug(10, "<= %.*s\n", (int) res, buf);
-			if (STARTS_WITH(buf + taglen, "BAD")) {
+			if (!STARTS_WITH(buf + taglen, "OK")) { /* BAD or NO */
 				/* We did something we shouldn't have, oops */
-				bbs_warning("Command '%.*s%.*s' failed: %s\n", taglen, tag, cmdlen > 2 ? cmdlen - 2 : cmdlen, cmd, buf); /* Don't include trailing CR LF */
+				if (cmd) {
+					bbs_warning("Command '%.*s%.*s' failed: %s\n", taglen, tag, cmdlen > 2 ? cmdlen - 2 : cmdlen, cmd, buf); /* Don't include trailing CR LF */
+				} else {
+					bbs_warning("Received negatory response: %s\n", buf);
+				}
 			}
 			client->lastactive = time(NULL); /* Successfully just got data from remote server */
 			break; /* That's all, folks! */
@@ -510,6 +515,30 @@ ssize_t __imap_client_send_log(struct imap_client *client, int log, const char *
 	return bbs_write(client->client.wfd, buf, (size_t) len);
 }
 
+int __imap_client_wait_response(struct imap_client *client, int fd, int ms, int echo, int lineno, int (*cb)(struct imap_client *client, const char *buf, size_t len, void *cbdata), void *cbdata)
+{
+	char tagbuf[15];
+	int taglen;
+	const char *tag = "tag";
+
+	if (!client->imap) {
+		bbs_warning("No active IMAP client?\n"); /* Shouldn't happen... */
+		bbs_soft_assert(0);
+	} else if (strlen_zero(client->imap->tag)) {
+		bbs_warning("No active IMAP tag, using generic one\n");
+		bbs_soft_assert(0);
+	} else {
+		tag = client->imap->tag;
+	}
+
+	taglen = snprintf(tagbuf, sizeof(tagbuf), "%s ", tag); /* Reuse the tag the client sent us, so we can just passthrough the response */
+
+	UNUSED(lineno);
+
+	/* Read until we get the tagged respones */
+	return client_command_passthru(client, fd, tagbuf, taglen, NULL, 0, ms, echo, cb, cbdata) <= 0;
+}
+
 int __imap_client_send_wait_response(struct imap_client *client, int fd, int ms, int echo, int lineno, int (*cb)(struct imap_client *client, const char *buf, size_t len, void *cbdata), void *cbdata, const char *fmt, ...)
 {
 	char *buf;

nets/net_imap/imap_client.h

@@ -86,11 +86,27 @@ ssize_t __attribute__ ((format (gnu_printf, 3, 4))) __imap_client_send_log(struc
 #define imap_client_send(client, fmt, ...) __imap_client_send_log(client, 0, fmt, ## __VA_ARGS__)
 #define imap_client_send_log(client, fmt, ...) __imap_client_send_log(client, 1, fmt, ## __VA_ARGS__)
 
+#define imap_client_wait_response(client, fd, ms) __imap_client_wait_response(client, fd, ms, 1, __LINE__, NULL, NULL)
+
 #define imap_client_send_wait_response(client, fd, ms, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 1, __LINE__, NULL, NULL, fmt, ## __VA_ARGS__)
 #define imap_client_send_wait_response_cb(client, fd, ms, cb, cbdata, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 1, __LINE__, cb, cbdata, fmt, ## __VA_ARGS__)
 #define imap_client_send_wait_response_noecho(client, fd, ms, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 0, __LINE__, NULL, NULL, fmt, ## __VA_ARGS__)
 #define imap_client_send_wait_response_cb_noecho(client, fd, ms, cb, cbdata, fmt, ...) __imap_client_send_wait_response(client, fd, ms, 0, __LINE__, cb, cbdata, fmt, ## __VA_ARGS__)
 
+/*! \brief Same as __imap_client_send_wait_response, but don't send a command initially, just wait for the tagged response */
+int __imap_client_wait_response(struct imap_client *client, int fd, int ms, int echo, int lineno, int (*cb)(struct imap_client *client, const char *buf, size_t len, void *cbdata), void *cbdata);
+
+/*!
+ * \brief Send a command to remote IMAP server and wait for a response, passing through any receive untagged responses inbetween directly back to the client
+ * \param client
+ * \param fd Additional file descriptor on which to poll(used only for IDLE, -1 otherwise)
+ * \param ms Max time to wait for poll / bbs_readline
+ * \param echo Whether to relay output from remote server to our local client
+ * \param lineno
+ * \param cb Custom callback to run for each line received from remote server. Returning -1 from callback will terminate wait loop
+ * \param cbdata Callback data for callback function
+ * \param fmt printf-style fmt string
+ */
 int __attribute__ ((format (gnu_printf, 8, 9))) __imap_client_send_wait_response(struct imap_client *client, int fd, int ms, int echo, int lineno, int (*cb)(struct imap_client *client, const char *buf, size_t len, void *cbdata), void *cbdata, const char *fmt, ...);
 
 /*! \retval Number of replacements made */