Opened 12 years ago

Closed 8 years ago

#1105 defect closed fixed (fixed)

IMAP Client does not seem to handle unsolicited server responses

Reported by: cablehead Owned by:
Priority: highest Milestone:
Component: mail Keywords:
Cc: Jean-Paul Calderone, cablehead Branch: branches/imap4-unsolicited-responses-1105
branch-diff, diff-cov, branch-cov, buildbot
Author: jhohm, exarkun

Description


Attachments (5)

imaptest.patch (3.0 KB) - added by cablehead 12 years ago.
imap4_flags.py (1014 bytes) - added by therve 11 years ago.
Patch for unsolicited flags response on imap4
test_imap_flags.py (2.3 KB) - added by therve 11 years ago.
Test imap unsolocited flags messages
twistedmail-imap-flags-anywhere-test.diff (4.4 KB) - added by jhohm 10 years ago.
Test FLAGS anywhere inside FETCH response
twistedmail-imap-flags-anywhere.diff (2.2 KB) - added by jhohm 10 years ago.
Patch to handle FLAGS anywhere inside FETCH response

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by cablehead

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

Changed 12 years ago by cablehead

Attachment: imaptest.patch added

comment:2 Changed 12 years ago by cablehead

stab at a test case for this issue

comment:3 Changed 11 years ago by 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.

Changed 11 years ago by therve

Attachment: imap4_flags.py added

Patch for unsolicited flags response on imap4

Changed 11 years ago by therve

Attachment: test_imap_flags.py added

Test imap unsolocited flags messages

comment:4 Changed 11 years ago by Jean-Paul Calderone

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?

comment:5 Changed 11 years ago by 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.

comment:6 Changed 11 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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

Changed 10 years ago by jhohm

Test FLAGS anywhere inside FETCH response

Changed 10 years ago by jhohm

Patch to handle FLAGS anywhere inside FETCH response

comment:7 Changed 10 years ago by jhohm

Resolution: fixed
Status: closedreopened

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.

comment:8 Changed 8 years ago by Jean-Paul Calderone

Priority: highhighest

comment:9 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/imap4-unsolicited-responses-1105

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

comment:10 Changed 8 years ago by Jean-Paul Calderone

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

refs #1105

comment:11 Changed 8 years ago by Jean-Paul Calderone

(In [26392]) Coding standard updates etc

refs #1105

comment:12 Changed 8 years ago by Jean-Paul Calderone

Author: exarkunjhohm, exarkun
Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: reopenednew

Should be somewhat more generally fixed now.

comment:13 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone
  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

comment:14 Changed 8 years ago by Jean-Paul Calderone

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

refs #1105

comment:15 Changed 8 years ago by Jean-Paul Calderone

(In [26484]) a docstring for _parseFetchPairs

refs #1105

comment:16 Changed 8 years ago by Jean-Paul Calderone

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

refs #1105

comment:17 Changed 8 years ago by Jean-Paul Calderone

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

refs #1105

comment:18 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
  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.

comment:19 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

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

comment:20 Changed 8 years ago by Jean-Paul Calderone

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

refs #1105

comment:21 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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.

comment:22 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.