From b41845be4700a16e606895acd635b505da5e4d8c Mon Sep 17 00:00:00 2001 From: InterLinked1 <24227567+InterLinked1@users.noreply.github.com> Date: Sat, 30 Nov 2024 17:14:24 -0500 Subject: [PATCH] net_imap: Fix missing untagged notifications on MOVE/COPY/STORE. * Fix construction of Other Users mailbox path with extra erroneous leading hierarchy delimiter. * Fix not getting notifications of flag changes with NOTIFY enabled, caused by wrong maildir path being passed to send_untagged_fetch. * Fix missing EXPUNGE notifications on cross-server MOVE from local server to remote server. * Fix missing EXISTS notifications on COPY operations. * Fix missing EXISTS notifications on MOVE operations. * Don't send untagged updates after tagged response. * Fix logic for UIDVALIDITY rotation. * mod_webmail: Don't automark seen when requesting raw message, to prevent message downloads from automarking seen. --- modules/mod_mail.c | 49 ++++++++++++++++++++-- modules/mod_webmail.c | 38 ++++++++++++++++- nets/net_imap.c | 56 ++++++++++++++++++++++--- nets/net_imap/imap.h | 3 ++ nets/net_imap/imap_server_flags.c | 9 ++++ nets/net_imap/imap_server_list.c | 6 ++- nets/net_imap/imap_server_maildir.c | 12 +++++- nets/net_imap/imap_server_maildir.h | 10 ++++- nets/net_imap/imap_server_notify.c | 64 +++++++++++++++++++++++++---- tests/test.c | 8 +++- tests/test_imap.c | 26 +++++++++++- tests/test_imap_notify.c | 59 ++++++++++++++++++++++++-- 12 files changed, 311 insertions(+), 29 deletions(-) diff --git a/modules/mod_mail.c b/modules/mod_mail.c index 2b28471d..58d752f1 100644 --- a/modules/mod_mail.c +++ b/modules/mod_mail.c @@ -987,7 +987,7 @@ static int parse_uidfile(FILE *fp, const char *uidfile, unsigned int *uidvalidit char *uidvaliditystr, *tmp; if (fread(uidvalidity, sizeof(unsigned int), 1, fp) != 1) { - bbs_debug(4, "Failed to read UIDVALIDITY from %s (empty file?)\n", uidfile); /* If we just created the file, it's empty, so this could happen */ + bbs_debug(2, "Failed to read UIDVALIDITY from %s: %s (empty file?)\n", uidfile, strerror(errno)); /* If we just created the file, it's empty, so this could happen */ return 1; } else if (fread(uidnext, sizeof(unsigned int), 1, fp) != 1) { bbs_error("Failed to read UID from %s\n", uidfile); @@ -1030,7 +1030,7 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c { FILE *fp = NULL; char uidfile[256]; - int uidvchange = 0; + int uidvchange = 0, uidfile_existed = 1; unsigned int uidvalidity = 0, uidnext = 0; int ascii = 0; @@ -1077,6 +1077,7 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c fp = fopen(uidfile, "r+"); /* Open for reading and writing, don't truncate it, and create if it doesn't exist */ if (!fp) { /* Assume the file doesn't yet exist. */ + uidfile_existed = bbs_file_exists(uidfile); fp = fopen(uidfile, "a"); if (unlikely(!fp)) { bbs_error("fopen(%s) failed: %s\n", uidfile, strerror(errno)); @@ -1104,7 +1105,22 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c parse_uidfile(fp, uidfile, &uidvalidity, &uidnext, &ascii); } - if (!uidvalidity || !uidnext || !fp) { + if (!uidvalidity || !fp) { /* uidnext can be 0 here */ + struct stat st; + /* If there's no uidvalidity currently, not a big deal, that's expected and we'll create one now. + * If there is one and we failed, then this is really, really bad. */ + if (uidfile_existed) { + bbs_error("Couldn't read already existing uidvalidity file (%s)\n", uidfile); + } + if (!stat(uidfile, &st)) { + if (st.st_size > 0) { + bbs_error("Failed to read current UID file %s\n", uidfile); + } else if (uidfile_existed) { + bbs_debug(3, "File %s is currently empty\n", uidfile); + } + } else { + bbs_debug(3, "Failed to stat(%s): %s\n", uidfile, strerror(errno)); + } /* UIDVALIDITY must be strictly increasing, so time is a good thing to use. */ uidvalidity = (unsigned int) time(NULL); /* If this isn't the first access to this folder, this will invalidate the client's cache of this entire folder. */ /* Since we're starting over, we must broadcast the new UIDVALIDITY value (we always do for SELECTs). */ @@ -1177,9 +1193,10 @@ unsigned int mailbox_get_next_uid(struct mailbox *mbox, struct bbs_node *node, c *newuidnext = uidnext; mailbox_uid_unlock(mbox); - if (uidvchange) { + if (uidvchange && uidfile_existed) { /* Don't do this while mailbox UID lock is held, or we could cause a deadlock * if an event callback is triggered that tries to grab that lock. */ + bbs_debug(2, "UIDVALIDITY has changed (UIDVALIDITY=%d,UIDNEXT=%d)\n", uidvalidity, uidnext); mailbox_dispatch_event_basic(EVENT_MAILBOX_UIDVALIDITY_CHANGE, node, mbox, directory); } @@ -1626,6 +1643,28 @@ static int gen_newname(struct mailbox *mbox, struct bbs_node *node, const char * return (int) uid; } +static int copy_move_untagged_exists(struct bbs_node *node, struct mailbox *mbox, const char *newpath, size_t size) +{ + char maildir[256]; + char *tmp; + + safe_strncpy(maildir, newpath, sizeof(maildir)); + + tmp = strrchr(maildir, '/'); /* newpath can be mutiliated at this point */ + if (!tmp) { + return -1; + } + *tmp = '\0'; /* Truncating once gets rid of the filename, need to do it again to get the maildir without /new or /cur at the end */ + /* Since we're already near the end of the string, it's more efficient to back up from here than start from the beginning a second time */ + while (tmp > maildir && *tmp != '/') { + tmp--; + } + *tmp = '\0'; + + mailbox_notify_new_message(node, mbox, maildir, newpath, size); + return 0; +} + int maildir_move_msg(struct mailbox *mbox, struct bbs_node *node, const char *curfile, const char *curfilename, const char *destmaildir, unsigned int *uidvalidity, unsigned int *uidnext) { return maildir_move_msg_filename(mbox, node, curfile, curfilename, destmaildir, uidvalidity, uidnext, NULL, 0); @@ -1645,6 +1684,7 @@ int maildir_move_msg_filename(struct mailbox *mbox, struct bbs_node *node, const return -1; } bbs_debug(6, "Renamed %s -> %s\n", curfile, newpath); + copy_move_untagged_exists(node, mbox, newpath, len); if (newfile) { safe_strncpy(newfile, newpath, len); } @@ -1699,6 +1739,7 @@ int maildir_copy_msg_filename(struct mailbox *mbox, struct bbs_node *node, const } /* Rather than invalidating quota usage for no reason, just update it so it stays in sync */ mailbox_quota_adjust_usage(mbox, copied); + copy_move_untagged_exists(node, mbox, newpath, len); return (int) uid; } diff --git a/modules/mod_webmail.c b/modules/mod_webmail.c index fe28906d..cc95f1c5 100644 --- a/modules/mod_webmail.c +++ b/modules/mod_webmail.c @@ -2913,11 +2913,26 @@ static int handle_fetch(struct ws_session *ws, struct imap_client *client, uint3 fetch_att = mailimap_fetch_att_new_internaldate(); mailimap_fetch_type_new_fetch_att_list_add(fetch_type, fetch_att); section = mailimap_section_new(NULL); + if (raw) { + /* The client requests the raw message in two circumstances: + * 1. The user actually wants to view the raw message source. + * 2. The user downloads the message. + * For #2, we should NOT necessarily mark the message seen, + * because the user may not have really "viewed" the messages, + * i.e. user should be able to download without marking seen. + * For #1, the user has likely already clicked on the message + * to view it BEFORE toggling to raw. Not necessarily, but + * pretty likely. + * So, if requesting raw body, don't automark seen, + * even if we would normally. */ + fetch_att = mailimap_fetch_att_new_body_peek_section(section); + } else { #ifdef AUTO_MARK_SEEN - fetch_att = mailimap_fetch_att_new_body_section(section); + fetch_att = mailimap_fetch_att_new_body_section(section); #else - fetch_att = mailimap_fetch_att_new_body_peek_section(section); + fetch_att = mailimap_fetch_att_new_body_peek_section(section); #endif + } mailimap_fetch_type_new_fetch_att_list_add(fetch_type, fetch_att); /* Fetch by UID */ @@ -3409,6 +3424,10 @@ static int process_idle(struct imap_client *client, char *s) } else if (STARTS_WITH(tmp, "RECENT")) { /* RECENT is basically always accompanied by EXISTS, so this is almost academic, * since we don't currently use this flag for anything, but for sake of completeness: */ + /*! \todo We should use this, because not all EXISTS are accompanied by EXISTS. + * For example, if a seen message is copied to this folder. + * We should provide explicit information to the client about whether this + * message is seen or not, using this information. */ client->idlerefresh |= IDLE_REFRESH_RECENT; } else if (STARTS_WITH(tmp, "EXPUNGE")) { if (client->messages) { @@ -3430,6 +3449,21 @@ static int process_idle(struct imap_client *client, char *s) } else { bbs_debug(6, "Ignoring FETCH update since not visible on current page\n"); } + /*! \todo XXX + * If the message was previously \Seen but now isn't, or wasn't, but now is, + * then we should also adjust the number of unseen messages in the folder. + * However, we don't actually keep track of the original flags for this message, + * so we can't do that. Even the frontend only knows the flags for the messages + * that are on the current page, but not other messages. + * As such, the correct count will "drift" currently. Workarounds would be: + * - Store a bit for each message, seen/unseen, and compare before/after (lots of inefficient accounting) + * - Issue a STATUS to get the current number of unseen messages. Slow/clunky from an IMAP perspective, + * but more efficient from a bookkeeping one for us. + * + * We have a similar problem for EXISTS and EXPUNGE, actually. + * For EXPUNGE, we correctly decrement client->messages, but we don't know what the new mailbox size is. + * For EXISTS, we correctly increment client->messages, but we don't know what the new mailbox size is. + */ } else { bbs_debug(3, "Ignoring IDLE data: %s", s); /* Already ends in LF */ } diff --git a/nets/net_imap.c b/nets/net_imap.c index a88fcd5e..8b210bde 100644 --- a/nets/net_imap.c +++ b/nets/net_imap.c @@ -437,10 +437,14 @@ static int generate_mailbox_name(struct imap_session *imap, const char *restrict if (atoi(bname)) { /* INBOX */ safe_strncpy(buf, "INBOX", len); - } else { + } else if (*bname == '.') { /* Skip leading . for maildir++ */ bname++; /* Skip leading . */ safe_strncpy(buf, bname, len); + } else { + strcpy(buf, "."); /* Copy something that will never match a real maildir name */ + bbs_warning("Unexpected base name: %s\n", bname); + bbs_soft_assert(0); } /* No need to check our own ACL, we should have full permission */ } else { @@ -495,7 +499,7 @@ void send_untagged_fetch(struct imap_session *imap, const char *maildir, int seq RWLIST_TRAVERSE(&sessions, s, entry) { int res; char mboxname[256]; - if (s == imap) { /* Skip if the update was caused by the client */ + if (s == imap) { /* Skip if the update was caused by the client. The STORE handler already sent a response to this client if needed. */ continue; } /* imap->folder is maybe not the same name for s. @@ -750,7 +754,7 @@ static void send_untagged_uidvalidity(struct mailbox *mbox, const char *maildir, size_t len; struct imap_session *s; - /* This should never happen, but if the UIDVALIDITY of a mailbox changes, + /* This should never happen, except for the first message, but if the UIDVALIDITY of a mailbox changes, * we MUST notify any clients currently using it. * Use same format as UIDVALIDITY response to SELECT/EXAMINE (RFC 2683 3.4.3) */ len = (size_t) snprintf(buf, sizeof(buf), "* OK [UIDVALIDITY %u] New UIDVALIDITY value!\r\n", uidvalidity); @@ -790,6 +794,11 @@ static void imap_mbox_watcher(struct mailbox_event *event) case EVENT_MESSAGE_EXPIRE: send_untagged_expunge(event->node, event->mbox, event->maildir, event->expungesilent, event->uids, event->seqnos, event->numuids); break; + case EVENT_FLAGS_SET: + case EVENT_FLAGS_CLEAR: + /* Do nothing here. + * send_untagged_fetch is already called in maildir_msg_setflags_modseq() */ + break; case EVENT_MAILBOX_CREATE: case EVENT_MAILBOX_DELETE: case EVENT_MAILBOX_RENAME: @@ -1323,7 +1332,7 @@ static int select_examine_response(struct imap_session *imap, enum select_type r if (was_selected) { /* Best practice to always include some response text after [] */ - imap_send(imap, "OK [CLOSED] Closed"); /* RFC 7162 3.2.11, example in 3.2.5.1 */ + imap_send(imap, "OK [CLOSED] Previous mailbox closed"); /* RFC 7162 3.2.11, example in 3.2.5.1 */ } imap_send(imap, "FLAGS (%s%s)", IMAP_FLAGS, numkeywords ? keywords : ""); @@ -2092,6 +2101,8 @@ static int handle_copy(struct imap_session *imap, const char *sequences, const c } else if (!numcopies && quotaleft <= 0) { imap_reply(imap, "NO [OVERQUOTA] Insufficient quota remaining"); } else { + /* maildir_copy_msg_filename sends the untagged EXISTS for the new message in the destination mailbox. + * No untagged EXISTS should be sent to the client running the command. */ if (IMAP_HAS_ACL(imap->acl, IMAP_ACL_READ)) { imap_reply(imap, "OK [COPYUID %u %s %s] COPY completed", uidvalidity, S_IF(olduidstr), S_IF(newuidstr)); } else { @@ -2205,6 +2216,8 @@ static int handle_move(struct imap_session *imap, const char *sequences, const c if (expunged) { imap->highestmodseq = maildir_indicate_expunged(EVENT_MESSAGE_EXPUNGE, imap->node, imap->mbox, imap->curdir, expunged, expungedseqs, exp_lengths, 0); free(expunged); + /* maildir_move_msg_filename sends the untagged EXISTS for the new message in the destination mailbox. + * No untagged EXISTS should be sent to the client running the command. */ } mailbox_unlock(imap->mbox); @@ -2632,6 +2645,13 @@ static int handle_append(struct imap_session *imap, char *s) return -1; } +struct uintlist_metadata { + unsigned int *uids; + unsigned int *seqs; + int lengths; + int allocsizes; +}; + struct copy_append { struct imap_session *imap; struct imap_client *appendclient; @@ -2642,11 +2662,14 @@ struct copy_append { unsigned int synchronizing:1; unsigned int multiappend:1; int appended; + struct uintlist_metadata *meta; }; +/*! \brief Part 2: Delete the source messages after a MOVE operation from local mailbox to a remote IMAP server */ static int copy_append_move_cb(const char *dir_name, const char *filename, int seqno, void *obj) { struct copy_append *ca = obj; + struct uintlist_metadata *meta = ca->meta; unsigned int msguid; char fullname[256]; int error = 0; @@ -2662,10 +2685,13 @@ static int copy_append_move_cb(const char *dir_name, const char *filename, int s snprintf(fullname, sizeof(fullname), "%s/%s", dir_name, filename); if (unlink(fullname)) { bbs_error("unlink(%s) failed: %s\n", fullname, strerror(errno)); + } else { + uintlist_append2(&meta->uids, &meta->seqs, &meta->lengths, &meta->allocsizes, msguid, (unsigned int) seqno); /* store UID and seqno */ } return 0; } +/*! \brief Part 1: MOVE a message from a local mailbox to a remote IMAP server by APPEND'ing it */ static int copy_append_cb(const char *dir_name, const char *filename, int seqno, void *obj) { struct copy_append *ca = obj; @@ -3058,8 +3084,17 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char IMAP_CLIENT_EXPECT(&destclient->client, " OK"); /* tagged OK */ } if (move) { + struct uintlist_metadata meta; + memset(&meta, 0, sizeof(meta)); + ca.meta = &meta; /* Now, delete all the source messages if this was a move operation */ maildir_ordered_traverse(imap->curdir, copy_append_move_cb, &ca); /* Delete */ + if (meta.uids) { + /* We need to send an untagged EXPUNGE for all messages that no longer exist locally. */ + imap->highestmodseq = maildir_indicate_expunged(EVENT_MESSAGE_EXPUNGE, imap->node, imap->mbox, imap->curdir, meta.uids, meta.seqs, meta.lengths, 0); + free(meta.uids); + } + free_if(meta.seqs); } imap_reply(imap, "OK %s completed", move ? "MOVE" : "COPY"); } else { @@ -4847,10 +4882,21 @@ static int imap_process(struct imap_session *imap, char *s) done: if (res) { bbs_debug(4, "%s command returned %d\n", command, res); - } else if (!strlen_zero(command)) { + } +#if 0 + /* Can't call flush_updates here, the command has already completed + * (this is when we are "supposed" to send updates to the client, but that is + * BEFORE the tagged response, not AFTER. + * + * \todo XXX What we SHOULD be doing is calling flush_updates + * right before every command sends an untagged response; + * unfortunately, there is no easy way to do that besides + * copying that to every command's function. */ + else if (!strlen_zero(command)) { res = flush_updates(imap, command, NULL); res = res < 0 ? -1 : 0; } +#endif return res; } diff --git a/nets/net_imap/imap.h b/nets/net_imap/imap.h index b51a9303..e8cd462c 100644 --- a/nets/net_imap/imap.h +++ b/nets/net_imap/imap.h @@ -123,6 +123,9 @@ struct imap_traversal { char curdir[260]; }; +/* Note: Although these are used frequently, + * the hierarchy delimiter has become hardcoded in several places + * that should be updated to use this macro. */ #define HIERARCHY_DELIMITER "." #define HIERARCHY_DELIMITER_CHAR '.' diff --git a/nets/net_imap/imap_server_flags.c b/nets/net_imap/imap_server_flags.c index d5fb670e..bed80d3f 100644 --- a/nets/net_imap/imap_server_flags.c +++ b/nets/net_imap/imap_server_flags.c @@ -498,8 +498,17 @@ int maildir_msg_setflags_modseq(struct imap_session *imap, int seqno, const char /* Send unilateral untagged FETCH responses to everyone except this session, to notify of the new flags */ generate_flag_names_full(imap, newflagletters, newflags, sizeof(newflags), &newbuf, &newlen); if (seqno) { /* Skip for merely translating flag mappings between maildirs */ + char *end; unsigned int uid; maildir_parse_uid_from_filename(filename, &uid); + /* Right now, dir ends in '/cur', since it's the physical maildir cur dir path, + * here, we want just the base maildir path (without '/cur' at the end). */ + end = strrchr(dirpath, '/'); + if (end) { + *end = '\0'; + } else { + bbs_soft_assert(0); + } send_untagged_fetch(imap, dirpath, seqno, uid, modseq, newflags); } return 0; diff --git a/nets/net_imap/imap_server_list.c b/nets/net_imap/imap_server_list.c index 67848dcd..a0682f6c 100644 --- a/nets/net_imap/imap_server_list.c +++ b/nets/net_imap/imap_server_list.c @@ -370,6 +370,7 @@ int list_iterate(struct imap_session *imap, struct list_command *lcmd, int level char fulldir[257]; char mailboxbuf[256]; char relativepath[512]; + char *relpath = relativepath; const char *mailboxname = entry->d_name; /* Only care about directories, not files. */ @@ -416,8 +417,11 @@ int list_iterate(struct imap_session *imap, struct list_command *lcmd, int level if (*mailboxname == HIERARCHY_DELIMITER_CHAR) { mailboxname++; } + if (*relpath == HIERARCHY_DELIMITER_CHAR) { + relpath++; + } - if (cb(imap, lcmd, relativepath, data)) { + if (cb(imap, lcmd, relpath, data)) { goto cleanup; } diff --git a/nets/net_imap/imap_server_maildir.c b/nets/net_imap/imap_server_maildir.c index 8e5a6dc8..8a74ecc3 100644 --- a/nets/net_imap/imap_server_maildir.c +++ b/nets/net_imap/imap_server_maildir.c @@ -58,6 +58,16 @@ static void set_current_mailbox(struct imap_session *imap, struct mailbox *mbox) } } +/*! + * \brief Translate an IMAP directory path to the full path of the IMAP mailbox on disk + * \param imap + * \param directory IMAP directory path (mailbox name) + * \param buf + * \param len + * \param[out] acl + * \param[out] mboxptr + * \retval 0 on success, -1 on failure + */ static int __imap_translate_dir(struct imap_session *imap, const char *directory, char *buf, size_t len, int *acl, struct mailbox **mboxptr) { enum mailbox_namespace ns; @@ -74,7 +84,7 @@ static int __imap_translate_dir(struct imap_session *imap, const char *directory safe_strncpy(buf, mailbox_maildir(mbox), len); ns = NAMESPACE_PRIVATE; } else if (strstr(directory, "..")) { - bbs_warning("Invalid IMAP directory: %s\n", directory); + bbs_warning("Invalid IMAP mailbox name: %s\n", directory); return -1; } else { /* Determine what namespace this mailbox is in */ diff --git a/nets/net_imap/imap_server_maildir.h b/nets/net_imap/imap_server_maildir.h index 55b89d7f..053be54c 100644 --- a/nets/net_imap/imap_server_maildir.h +++ b/nets/net_imap/imap_server_maildir.h @@ -19,7 +19,15 @@ /*! \brief Wrapper around uidsort */ int imap_uidsort(const struct dirent **da, const struct dirent **db); -/*! \brief Translate an IMAP directory path to the full path of the IMAP mailbox on disk */ +/*! + * \brief Translate an IMAP directory path to the full path of the IMAP mailbox on disk + * \param imap + * \param directory The mailbox name (a misnomer, it's not referring to any actual directory path) + * \param buf + * \param len + * \param[out] acl + * \retval 0 on success, -1 on failure + */ int imap_translate_dir(struct imap_session *imap, const char *directory, char *buf, size_t len, int *acl); /*! \brief Same as imap_translate_dir, but do not update the currently active mbox */ diff --git a/nets/net_imap/imap_server_notify.c b/nets/net_imap/imap_server_notify.c index 6bf1ed14..d4b6830b 100644 --- a/nets/net_imap/imap_server_notify.c +++ b/nets/net_imap/imap_server_notify.c @@ -33,6 +33,8 @@ #include "nets/net_imap/imap_server_notify.h" #include "nets/net_imap/imap_client.h" +/* #define DEBUG_NOTIFY */ + enum notify_mailbox_specifier { /* Selected mailbox (overrides everything else) */ /* Only one of either SELECTED or SELECTED_DELAYED may exist for the entire NOTIFY command */ @@ -43,7 +45,7 @@ enum notify_mailbox_specifier { NOTIFY_INBOXES, /*!< Any selectable mailbox in the personal namespace to which an MDA may deliver messages, i.e. just INBOX for us */ NOTIFY_SUBSCRIBED, /*!< All mailboxes subscribed to by the user */ NOTIFY_SUBTREE, /*!< Specified mailbox(es) and all selected mailboxes subordinate to it */ - NOTIFY_MAILBOXES, /*!< Specified ailbox(es). No wildcard expansion. */ + NOTIFY_MAILBOXES, /*!< Specified mailbox(es). No wildcard expansion. */ }; /*! \brief A single watch */ @@ -105,6 +107,9 @@ static struct notify_watch *notify_get_match(struct imap_session *imap, const ch /* Currently selected folder is treated specially */ selected = imap->folder && !strcmp(name, imap->folder); +#ifdef DEBUG_NOTIFY + bbs_debug(3, "Currently selected mailbox selected match: %d (%s|%s)\n", selected, name, imap->folder); +#endif RWLIST_TRAVERSE(¬ify->watchlist, w, entry) { /* Only SELECTED and NOTIFY_SELECTED_DELAYED match the currently selected mailbox */ @@ -155,6 +160,36 @@ int imap_notify_applicable(struct imap_session *imap, struct mailbox *mbox, cons return imap_notify_applicable_fetchargs(imap, mbox, folder, maildir, e, NULL); } +#ifdef DEBUG_NOTIFY +static void dump_events_str(enum mailbox_event_type events) +{ +#define DUMP_EVENT(e) if (events & e) { bbs_debug(3, "Event present: %s\n", #e); } + DUMP_EVENT(EVENT_MESSAGE_APPEND); + DUMP_EVENT(EVENT_MESSAGE_EXPIRE); + DUMP_EVENT(EVENT_MESSAGE_EXPUNGE); + DUMP_EVENT(EVENT_MESSAGE_NEW); + DUMP_EVENT(EVENT_QUOTA_EXCEED); + DUMP_EVENT(EVENT_QUOTA_WITHIN); + DUMP_EVENT(EVENT_QUOTA_CHANGE); + DUMP_EVENT(EVENT_MESSAGE_READ); + DUMP_EVENT(EVENT_MESSAGE_TRASH); + DUMP_EVENT(EVENT_FLAGS_SET); + DUMP_EVENT(EVENT_FLAGS_CLEAR); + DUMP_EVENT(EVENT_LOGIN); + DUMP_EVENT(EVENT_LOGOUT); + DUMP_EVENT(EVENT_MAILBOX_CREATE); + DUMP_EVENT(EVENT_MAILBOX_DELETE); + DUMP_EVENT(EVENT_MAILBOX_RENAME); + DUMP_EVENT(EVENT_MAILBOX_SUBSCRIBE); + DUMP_EVENT(EVENT_MAILBOX_UNSUBSCRIBE); + DUMP_EVENT(EVENT_METADATA_CHANGE); + DUMP_EVENT(EVENT_SERVER_METADATA_CHANGE); + DUMP_EVENT(EVENT_ANNOTATION_CHANGE); + DUMP_EVENT(EVENT_MAILBOX_UIDVALIDITY_CHANGE); +#undef DUMP_EVENT +} +#endif + int imap_notify_applicable_fetchargs(struct imap_session *imap, struct mailbox *mbox, const char *folder, const char *maildir, enum mailbox_event_type e, const char **fetchargs) { if (!imap->notify) { @@ -186,7 +221,10 @@ int imap_notify_applicable_fetchargs(struct imap_session *imap, struct mailbox * /* By default, no events for non-selected mailboxes */ events = selected ? DEFAULT_EVENTS : 0; } - +#ifdef DEBUG_NOTIFY + bbs_debug(3, "'%s'|'%s', '%s'|'%s', mailbox selected: %d, spec: %d, event match: %d\n", folder, imap->folder, maildir, imap->dir, selected, w->spec, events & e ? 1 : 0); + dump_events_str(events); +#endif RWLIST_UNLOCK(¬ify->watchlist); if (events & e) { return selected ? 1 : -1; @@ -351,7 +389,7 @@ static int add_watch(struct imap_session *imap, struct imap_notify *notify, char events |= IMAP_EVENT_MAILBOX_NAME; } else if (!strcasecmp(e, "SubscriptionChange")) { events |= IMAP_EVENT_SUBSCRIPTION_CHANGE; -#if 0 /* Remove once these IMAP extensions are supported */ +#if 0 /*! \todo Remove once these IMAP extensions are supported */ } else if (!strcasecmp(e, "AnnotationChange")) { events |= IMAP_EVENT_ANNOTATION_CHANGE; } else if (!strcasecmp(e, "MailboxMetadataChange")) { @@ -389,6 +427,10 @@ static int add_watch(struct imap_session *imap, struct imap_notify *notify, char } w->spec = spec; w->events = events; +#ifdef DEBUG_NOTIFY + bbs_debug(3, "Adding NOTIFY watch for spec %d, events:\n", spec); + dump_events_str(w->events); +#endif SET_BITFIELD(w->none, none); if (fetchargs) { strcpy(w->data, fetchargs); /* Safe */ @@ -413,11 +455,16 @@ static int notify_list_cb(struct imap_session *imap, struct list_command *lcmd, return 0; /* No STATUS for currently selected mailbox */ } - if (lcmd->ns == NAMESPACE_OTHER) { - snprintf(fullname, sizeof(fullname), OTHER_NAMESPACE_PREFIX HIERARCHY_DELIMITER "%s", name); - name = fullname; - } else if (lcmd->ns == NAMESPACE_SHARED) { - snprintf(fullname, sizeof(fullname), SHARED_NAMESPACE_PREFIX HIERARCHY_DELIMITER "%s", name); + if (lcmd->ns == NAMESPACE_OTHER || lcmd->ns == NAMESPACE_SHARED) { + if (lcmd->ns == NAMESPACE_OTHER) { + snprintf(fullname, sizeof(fullname), OTHER_NAMESPACE_PREFIX HIERARCHY_DELIMITER "%s", name); + } else if (lcmd->ns == NAMESPACE_SHARED) { + snprintf(fullname, sizeof(fullname), SHARED_NAMESPACE_PREFIX HIERARCHY_DELIMITER "%s", name); + } + if (strstr(fullname, "..")) { + bbs_warning("Unexpected folder: '%s' (maildir path '%s' is invalid)\n", name, fullname); + bbs_soft_assert(!strstr(fullname, "..")); /* If this happens, something got malformed */ + } name = fullname; } @@ -538,7 +585,6 @@ int handle_notify(struct imap_session *imap, char *s) struct list_command lcmd; memset(&lcmd, 0, sizeof(lcmd)); - lcmd.ns = NAMESPACE_PRIVATE; list_iterate(imap, &lcmd, 0, "", mailbox_maildir(imap->mbox), notify_list_cb, notify); if (notify->nonpersonal) { diff --git a/tests/test.c b/tests/test.c index 69089029..ca4d8813 100644 --- a/tests/test.c +++ b/tests/test.c @@ -57,6 +57,11 @@ static const char *testfilter = NULL; int startup_run_unit_tests; +/* Log file for BBS STDOUT output (empty after execution???) + * Note: Normal BBS file logging is still done to the normal BBS log files. */ +#define TEST_LOGFILE "/tmp/test_lbbs.log" + +/* Log file for valgrind output */ #define VALGRIND_LOGFILE "/tmp/test_lbbs/valgrind.log" static const char *loglevel2str(enum bbs_log_level level) @@ -383,7 +388,7 @@ static void *io_relay(void *varg) int found; char c = '\n'; - logfd = open("/tmp/test_lbbs.log", O_CREAT | O_TRUNC, 0600); + logfd = open(TEST_LOGFILE, O_CREAT | O_TRUNC, 0600); if (logfd < 0) { bbs_error("open failed: %s\n", strerror(errno)); return NULL; @@ -394,6 +399,7 @@ static void *io_relay(void *varg) res = read(pipefd[0], buf, sizeof(buf) - 1); if (res <= 0) { bbs_debug(4, "read returned %ld\n", res); + close(logfd); return NULL; } write(logfd, buf, (size_t) res); diff --git a/tests/test_imap.c b/tests/test_imap.c index 56f38829..bb3ff385 100644 --- a/tests/test_imap.c +++ b/tests/test_imap.c @@ -715,11 +715,33 @@ static int run(void) SWRITE(client1, "DONE" ENDL); CLIENT_EXPECT(client1, "e17 OK"); - close_if(client2); + SWRITE(client2, "a5 IDLE" ENDL); + CLIENT_EXPECT_EVENTUALLY(client2, "+ idling"); + + /* Test COPY and MOVE again, to test that an idling client on the destination mailbox receives untagged EXISTS + * (we already test the untagged EXPUNGE for MOVE earlier in this test sequence). */ + SWRITE(client1, "f1 COPY 1 INBOX" ENDL); /* Who says we can't copy to the current mailbox? Also, this ensures we don't get the untagged EXISTS for ourself before command completion. */ + CLIENT_EXPECT(client1, "f1 OK"); + CLIENT_EXPECT(client2, "* 6 EXISTS"); /* Idling client should see the new message */ + SWRITE(client1, "f2 NOOP" ENDL); /* Next time we can get updates flushed to us, we should see it as well */ + CLIENT_EXPECT(client1, "* 6 EXISTS"); + + /* Repeat, with MOVE */ + SWRITE(client1, "f3 MOVE 1 INBOX" ENDL); /* Again, who says we can't move to ourself? This basically just updates the UID for no reason */ + CLIENT_EXPECT_EVENTUALLY(client1, "f3 OK"); + CLIENT_EXPECT_EVENTUALLY(client2, "* 6 EXISTS"); + SWRITE(client1, "f2 NOOP" ENDL); + CLIENT_EXPECT_EVENTUALLY(client1, "* 6 EXISTS"); /* LOGOUT */ SWRITE(client1, "z999 LOGOUT" ENDL); - CLIENT_EXPECT(client1, "* BYE"); + CLIENT_EXPECT_EVENTUALLY(client1, "* BYE"); + + SWRITE(client2, "DONE" ENDL); + CLIENT_EXPECT(client2, "a5 OK"); + + SWRITE(client2, "z998 LOGOUT" ENDL); + CLIENT_EXPECT(client2, "* BYE"); res = 0; diff --git a/tests/test_imap_notify.c b/tests/test_imap_notify.c index 63cfddda..b8f9abeb 100644 --- a/tests/test_imap_notify.c +++ b/tests/test_imap_notify.c @@ -174,7 +174,7 @@ static int make_messages2(void) static int run(void) { - int client1 = -1, client2 = -1; + int client1 = -1, client2 = -1, client3 = -1; int res = -1; if (make_messages(TARGET_MESSAGES)) { @@ -193,6 +193,11 @@ static int run(void) return -1; } + client3 = test_make_socket(143); + if (client3 < 0) { + return -1; + } + /* Connect and log in */ CLIENT_EXPECT(client2, "OK"); SWRITE(client2, "a1 LOGIN \"" TEST_USER "\" \"" TEST_PASS "\"" ENDL); @@ -356,21 +361,69 @@ static int run(void) CLIENT_EXPECT(client1, "* STATUS"); /* Not selected */ CLIENT_EXPECT(client2, "* 2 EXISTS"); /* Selected */ - /* LOGOUT */ SWRITE(client1, "DONE" ENDL); CLIENT_EXPECT(client1, "e4 OK"); SWRITE(client2, "DONE" ENDL); CLIENT_EXPECT(client2, "e6 OK"); + /* If one client marks a message as seen or unseen, the other client should also see it. */ + CLIENT_EXPECT(client3, "OK"); + SWRITE(client3, "f1 LOGIN \"" TEST_USER "\" \"" TEST_PASS "\"" ENDL); + CLIENT_EXPECT(client3, "f1 OK"); + + SWRITE(client3, "f2 SELECT INBOX" ENDL); + CLIENT_EXPECT_EVENTUALLY(client3, "f2 OK"); + + /* FlagChange is included in SELECTED-DELAYED, but not in personal. + * Among other things, this test ensures that we use SELECTED-DELAYED for the selected mailbox, not personal (which is a less specific match). */ + SWRITE(client3, "f3 NOTIFY SET (SELECTED-DELAYED (MessageNew MessageExpunge FlagChange)) (personal (MessageNew MessageExpunge)) (subtree \"Other Users\" (MessageNew (FLAGS) MessageExpunge MailboxName FlagChange))" ENDL); + CLIENT_EXPECT_EVENTUALLY(client3, "f3 OK"); + + SWRITE(client3, "f4 IDLE" ENDL); + CLIENT_EXPECT(client3, "+ idling"); + + SWRITE(client1, "g1 SELECT INBOX" ENDL); + CLIENT_EXPECT_EVENTUALLY(client1, "g1 OK"); + + SWRITE(client1, "g2 STORE 1 -FLAGS (\\Seen)" ENDL); + CLIENT_EXPECT_EVENTUALLY(client1, "* 1 FETCH"); + CLIENT_EXPECT_EVENTUALLY(client3, "* 1 FETCH"); /* Client 3 should receive FLAG changes for currently selected mailbox */ + + /* Repeat with slightly different NOTIFY watch. Previously, this would trigger soft assertions caused by invalid path construction, due to client2 being active, shouldn't anymore though. */ + SWRITE(client3, "DONE" ENDL); + CLIENT_EXPECT(client3, "f4 OK"); + SWRITE(client3, "f5 NOTIFY SET STATUS (SELECTED-DELAYED (MessageNew (FLAGS) MessageExpunge FlagChange)) (personal (MessageNew (FLAGS) MessageExpunge MailboxName FlagChange)) (subtree \"Other Users\" (MessageNew (FLAGS) MessageExpunge MailboxName FlagChange))" ENDL); + CLIENT_EXPECT_EVENTUALLY(client3, "f5 OK"); + + SWRITE(client3, "f6 IDLE" ENDL); + CLIENT_EXPECT(client3, "+ idling"); + + SWRITE(client1, "g3 SELECT INBOX" ENDL); + CLIENT_EXPECT_EVENTUALLY(client1, "g3 OK"); + + SWRITE(client1, "g4 STORE 1 +FLAGS (\\Seen)" ENDL); + CLIENT_EXPECT_EVENTUALLY(client1, "* 1 FETCH"); + CLIENT_EXPECT_EVENTUALLY(client3, "* 1 FETCH"); /* Client 3 should receive FLAG changes for currently selected mailbox */ + + /* Done... */ + SWRITE(client3, "DONE" ENDL); + CLIENT_EXPECT(client3, "f6 OK"); + + /* LOGOUT */ + SWRITE(client3, "z997 LOGOUT" ENDL); + CLIENT_EXPECT(client3, "* BYE"); + SWRITE(client2, "z998 LOGOUT" ENDL); CLIENT_EXPECT(client2, "* BYE"); + SWRITE(client1, "z999 LOGOUT" ENDL); - CLIENT_EXPECT(client1, "* BYE"); + CLIENT_EXPECT_EVENTUALLY(client1, "* BYE"); res = 0; cleanup: close_if(client1); close_if(client2); + close_if(client3); return res; }