CATENATE command returns BADURL

[brong]

From: Jan Schneider Bugzilla-Id: 3613 Version: 2.4.11 Owner: Greg Banks

[brong]

From: Jan Schneider

This is an excerpt of client/server communication, and AFAICS the request is correct. But Cyrus obviously isn't able to parse the CATENATE parts correctly:

S: OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE STARTTLS LOGINDISABLED] xxx Cyrus IMAP v2.4.11-TUGraz server ready C: 1 STARTTLS S: 1 OK Begin TLS negotiation now C: [LOGIN Command - username: koarl] S: 2 OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE ACL RIGHTS=kxte QUOTA MAILBOX-R EFERRALS NAMESPACE UIDPLUS NO_ATOMIC_RENAME UNSELECT CHILDREN MULTIAPPEND BINARY CATENATE CONDSTORE ESEARCH SORT SORT=MODSEQ SORT=DISPLAY THREAD=ORDEREDSUBJECT THREAD=REFERENCES ANNOTATEMORE LIST-EXTENDED WITHIN QRESYNC SCAN XLIST URLAUTH U RLAUTH=BINARY X-NETSCAPE LOGINDISABLED AUTH=LOGIN AUTH=PLAIN COMPRESS=DEFLATE ID LE] User logged in SESSIONID=<xxx-7241-1323876204-1> C: 3 EXAMINE INBOX S: 163 EXISTS S: 0 RECENT S: FLAGS (\Answered \Flagged \Draft \Deleted \Seen) S: OK [PERMANENTFLAGS ()] Ok S: OK [UNSEEN 15] Ok S: OK [UIDVALIDITY 1258492234] Ok S: OK [UIDNEXT 1006] Ok S: OK [HIGHESTMODSEQ 386] Ok S: OK [URLMECH INTERNAL] Ok S: 3 OK [READ-ONLY] Completed C: 4 UID FETCH 1004 (BODYSTRUCTURE) S: 162 FETCH (UID 1004 BODYSTRUCTURE (("TEXT" "PLAIN" ("CHARSET" "ISO-8859-15" "FORMAT" "flowed") NIL NIL "7BIT" 313 12 NIL NIL NIL NIL)("IMAGE" "JPEG" ("NAME " "51XsE0eZ9KL.SS500.jpg") NIL NIL "BASE64" 61810 NIL ("ATTACHMENT" ("FILENAME " "51XsE0eZ9KL.SS500.jpg")) NIL NIL) "MIXED" ("BOUNDARY" "------------00030205 0904080105050701") NIL NIL NIL)) S: 4 OK Completed (0.000 sec) C: 5 UID FETCH 1004 (INTERNALDATE FLAGS) S: 162 FETCH (FLAGS (\Seen) UID 1004 INTERNALDATE "13-Dec-2011 09:06:32 +0100" ) S: 5 OK Completed (0.000 sec) C: 6 UNSELECT S: 6 OK Completed C: 7 APPEND INBOX (\seen) "13-Dec-2011 09:06:32 +0100" CATENATE (URL "/INBOX;UID VALIDITY=1258492234/;UID=1004/;SECTION=HEADER" TEXT {42+} C: [LITERAL DATA - 42 bytes] C: URL "/INBOX;UIDVALIDITY=1258492234/;UID=1004/;SECTION=1.MIME" URL "/INBOX;UI DVALIDITY=1258492234/;UID=1004/;SECTION=1" TEXT {42+} C: [LITERAL DATA - 42 bytes] C: TEXT {126+} C: [LITERAL DATA - 126 bytes] C: TEXT {44+} C: [LITERAL DATA - 44 bytes] C: ) >> Slow IMAP Command: 1,161 seconds S: 7 NO [BADURL "TEXT"] Mailbox does not exist C: 8 LOGOUT S: * BYE LOGOUT received S: 8 OK Completed

[brong]

From: Jan Schneider

Can anybody at least confirm that this is a Cyrus bug?

This currently breaks stripping attachments in any Horde Groupware/Webmail installation running on a 2.4 server.

[brong]

From: Greg Banks

Hi Jan,

Sorry it took so long for someone to look at this.

From my reading of RFC4469, it appears that the APPEND command being sent by the client is entirely correct. Further, the Cyrus server definitely has at least one bug. The string after BADURL is supposed to be the first URL that failed. Instead, Cyrus is replying with either

• the LAST url that failed, or

• the "TEXT" keyword, if there's any of those after the last failing URL.

The culprit is a missing 'return' statement in append_catenate().

However, that bug is just in the error reporting and doesn't explain why you're seeing failures, which is a little more mysterious. The possible reasons I can see are:

• INBOX does not exist in the mboxlist db

• INBOX exists but the user has neither r nor l rights

• INBOX has the MBTYPE_MOVING flag in the mboxlist db

• INBOX has OPT_MAILBOX_DELETED set

• the mailbox name was confused somehow during URL parsing

All those explanations except the last one are unlikely given that the user's INBOX was successfully EXAMINEd just two commands before. I ran an experiment and the URL parsing code does in fact correctly parse URLs like those shown, so that last explanation isn't the case either.

Another possibility is that Cyrus is getting confused and trying to parse the keyword "TEXT" as a URL. The URL parsing code is sufficiently liberal that it would report was a correct URL with mailbox="TEXT", which is probably not the name of an existing mailbox. I don't see quite how that would happen in the parsing code, but it's a possibility.

Jan, can you send us the offending traffic, without Bugzilla whitespace damage? An attachment to this Bug should be fine.

[brong]

Attachment-Id: 1459 From: Jan Schneider Type: text/plain File: imap.log

IMAP log

[brong]

From: Greg Banks

Sigh...there were actually 4 bugs here.

• mistaken BADURL error reporting, as described above.

• when opening the mailbox to pull out the section data described by a URL, no authentication information was provided to index_open(), which then concluded that the user had no rights at all to the mailbox, and specifically neither the r(ead) nor l(ookup) rights, resulting in a spurious "Mailbox does not exist" error.

• when fetching data from a message section as specified by a URL, the cached section structure was misinterpreted, resulting in a spurious "No such message part" error.

• when fetching data from a message section as specified by a URL and the section name did not start with a number, index_urlfetch() would incorrectly return -1 on success, resulting in a spurious "Unknown code ____ 255" error.

Fixed in the master branch by

The following changes since commit 60282ea9432679014ce0daa53473ded474b56a14:

idle: don't do FD_ISSET(-1,) it's unhelpful (2012-03-28 08:48:05 +1100)

are available in the git repository at: ssh://git.cyrusimap.org/git/cyrus-imapd master

Greg Banks (3): Bug 3613 - fix BADURL response from CATENATE Bug 3613 - pass auth to index_open in CATENATE Bug 3613 - fix index_urlfetch()

imap/imapd.c | 16 ++++++++++++++-- imap/index.c | 6 ++++-- 2 files changed, 18 insertions(+), 4 deletions(-)

I don't have an automatic test yet (sorry).

I'll look into the feasibility of backporting for 2.4.

[brong]

From: Greg Banks

> * mistaken BADURL error reporting [...]

Has been in the URLFETCH and CATENATE code since it was introduced in the 2.3 merge, and is in all 2.3 and 2.4 releases.

> [...] no authentication information was > provided to index_open() [...] > > [...] the cached section structure was misinterpreted, [...] > > * [...] index_urlfetch() would incorrectly return -1 on success, [...]

All three were regressions introduced in the same commit during the 2.4 merge, and are in all 2.4 releases.

Jeroen: Fortunately this code has been relatively stable since the 2.4 merge; backporting is an easy cherry-pick and will be correct.

Jan: Thanks for doing our testing for us :(

[brong]

From: Leena Heino

(In reply to comment #5) > Jeroen: Fortunately this code has been relatively stable since the 2.4 merge; > backporting is an easy cherry-pick and will be correct.

It seems commit 545dba13f94dc48a17357be2b0566643368d4c45 adds this to code in imap/imapd.c: if (!r) { struct index_init init; memset(&init, 0, sizeof(init)); init.qresync = imapd_client_capa & CAPA_QRESYNC; init.userid = imapd_userid; init.authstate = imapd_authstate; init.out = imapd_out; r = index_open(mailboxname, &init, &state); if (init.vanishedlist) seqset_free(init.vanishedlist); }

Which leads to compile error in 2.4.14: imapd.c: In function catenate_url': imapd.c:3092: error: structure has no member namedvanishedlist' imapd.c:3092: error: structure has no member named `vanishedlist' gmake: *** [imapd.o] Error 1

[brong]

From: Greg Banks

Leena: it should be fine if you remove the 1 line which mentions the vanishedlist.

[brong]

From: Jeroen van Meeuwen (Kolab Systems)

picked to cyrus-imapd-2.4, will be in 2.4.15

[brong]

From: Jeroen van Meeuwen (Kolab Systems)

Closing ticket in preparation of the 2.4.15 release.