IMAP Client does not seem to handle unsolicited server responses

[cablehead]

http://twistedmatrix.com/pipermail/twisted-python/2005-July/010933.html

[cablehead]

stab at a test case for this issue

[therve]

OK I managed to make something work. The problem relies in the Command, because fetch command doesn't check the data it received : if your fetch a SUBJECT, you should raise an error if your receive a BODY, or add the result in the unuse list (to handle FLAGS messages).

Attached a quick and dirty patch that handles the problem for FETCH that raises FLAGS messages. Also patch the test for not using defferedResult.

[therve]

Patch for unsolicited flags response on imap4

[therve]

Test imap unsolocited flags messages

[exarkun]

Test looks really nice, and the patch to imap4.py 'almost' looks cool. Help me refresh my memory though... It's possible for responses other than FLAGS to come back unsolicited, isn't it? So to be really correct, the patch needs to account for any response that wasn't explicitly requested? Or am I imagining things?

[cablehead]

Well, the rfc reads:

A client MUST be prepared to accept any server response at all times.

In practice though, this has only been an issue for me with FLAGS.

[exarkun]

(In [16253]) Apply patch from cablehead/therve - adds support for recognizing unsolicited FLAGS responses to IMAP4Client; Fixes #1105

[jhohm]

Test FLAGS anywhere inside FETCH response

[jhohm]

Patch to handle FLAGS anywhere inside FETCH response

[jhohm]

The already-applied patch is not sufficient. It only recognizes unsolicited FETCH responses which contain only FLAGS, like this (for explanatory purposes the entire RFC822 body of the email is just 0123456789 followed by a CRLF):

* 1 FETCH (UID 9 RFC822 {12}
0123456789
)
* 1 FETCH (FLAGS (\Seen))

Cyrus IMAPD (version 2.2.13) sends FLAGS updates before the requested parts inside a single FETCH response, like this:

* 1 FETCH (FLAGS (\Seen) UID 9 RFC822 {12}
0123456789
)

Microsoft Exchange Server 2003 (version 6.5.7638.1) sends FLAGS updates after the requested parts inside a single FETCH response, like this:

* 1 FETCH (RFC822 {12}
0123456789
 UID 9 FLAGS (\Seen))

With the already-applied patch, such responses cause message data to be dropped from the fetch results, or cause an IllegalServerResponse exception.

I have attached a patch which add tests for FLAGS anywhere inside FETCH responses; I have attached a patch which removes the partial fix to Command from the applied patch and fixes the fetch callbacks to handle FLAGS anywhere inside FETCH responses; and I am reopening this ticket.

The fix to fetchSpecific is a bit ugly, but I don't see how to cleanly fix it without redefining its interface to return a more-fully-parsed result instead of the raw results of parseNestedParens.

I have to agree with exarkun that the IMAP spec requires the client to accept any responses at any time, and to take note of any that it needs to care about. Unfortunately the twisted IMAP4 code in general is not structured to do this.

[exarkun]

(In [26390]) Branching to 'imap4-unsolicited-responses-1105'

[exarkun]

(In [26391]) Apply twistedmail-imap-flags-anywhere-test.diff

refs #1105

[exarkun]

(In [26392]) Coding standard updates etc

refs #1105

[exarkun]

Should be somewhat more generally fixed now.

[glyph]

1. IMAP4Client.__cbSelect - you appear to have changed the log.err line such that parts is no longer in scope. Pyflakes reports this as an error, and it has no test coverage.
2. pairs may well be a lie, but I'd like to know what form of lie it is. Please write a docstring for _parseFetchPairs.
3. XXX Used to strip here, still need to? comments in IMAP4Client.__cbSelect - since these are all being modified... do we still need to strip here? What does this even mean? File a new ticket?
4. needs more tests:
   1. XXX in IMAP4Client._store
   2. comments at the bottom of IMAP4ClientFetchTests - I'm assuming these describe as-yet-unwritten tests, but I don't really understand for what. Enumerated here for your responding-to-feedback convenience:
      1. test BODY[1.TEXT]
      2. test BODY[HEADER.FIELDS (SUBJECT)]
      3. test BODY.PEEK variations
      4. test error conditions

[exarkun]

(In [26483]) Remove notes; I wrote those tests

refs #1105

[exarkun]

(In [26484]) a docstring for _parseFetchPairs

refs #1105

[exarkun]

(In [26486]) Get rid of XXXs about strip. We don't need to anymore.

refs #1105

[exarkun]

(In [26489]) Test some extra cases of the SELECT response parser (one of which turns out not to be real

refs #1105

[exarkun]

1. Fixed in r26489. The case wasn't actually reachable. I added a test demonstrating this.
2. Fixed in r26484.
3. Fixed in r26486. Since the general parser is used now, there can't be random extra whitespace lying around unless the server tries really hard to put it there. I'm happy for such a case to not be handled by this code (if the server says FETCH foo instead of FETCH foo that's one thing, probably some moron fat fingered it. If the server says FETCH " foo" though, then something fishy is going on).
4. Fixed
   1. in r26485. Added tests and fixed the implementation.
   2. in r26483. I added these tests before the previous submission for review, I just forgot to delete the comments.

[glyph]

As we discussed on IRC, please fix the documentation for the pairs parameter. Afterwards, go ahead and merge.

[exarkun]

(In [26516]) Rename pairs something more accurate; adjust docstring and method to suit

refs #1105

[exarkun]

(In [26520]) Merge imap4-unsolicited-responses-1105

Author: jhohm, exarkun Reviewer: glyph Fixes: #1105

Handle more unsolicited data received from the server instead of printing it out and otherwise ignoring it.

In particular, now if unsolicited data is received on the same line as solicited data, it is separated from the solicited data and delivered to the appropriate callback. Previously, this case could cause the entire line to be ignored or it could cause the unsolicited data to be delivered to the application callback with the solicited data.