Opened 9 years ago

Closed 5 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: exarkun, cablehead Branch: branches/imap4-unsolicited-responses-1105
(diff, github, buildbot, log)
Author: jhohm, exarkun Launchpad Bug:

Description


Attachments (5)

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

Download all attachments as: .zip

Change History (27)

comment:1 Changed 9 years ago by cablehead

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

Changed 9 years ago by cablehead

comment:2 Changed 9 years ago by cablehead

stab at a test case for this issue

comment:3 Changed 9 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 9 years ago by therve

Patch for unsolicited flags response on imap4

Changed 9 years ago by therve

Test imap unsolocited flags messages

comment:4 Changed 9 years ago by 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?

comment:5 Changed 9 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 8 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

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

Changed 7 years ago by jhohm

Test FLAGS anywhere inside FETCH response

Changed 7 years ago by jhohm

Patch to handle FLAGS anywhere inside FETCH response

comment:7 Changed 7 years ago by jhohm

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 5 years ago by exarkun

  • Priority changed from high to highest

comment:9 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/imap4-unsolicited-responses-1105

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

comment:10 Changed 5 years ago by exarkun

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

refs #1105

comment:11 Changed 5 years ago by exarkun

(In [26392]) Coding standard updates etc

refs #1105

comment:12 Changed 5 years ago by exarkun

  • Author changed from exarkun to jhohm, exarkun
  • Keywords review added
  • Owner exarkun deleted
  • Status changed from reopened to new

Should be somewhat more generally fixed now.

comment:13 Changed 5 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun
  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 5 years ago by exarkun

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

refs #1105

comment:15 Changed 5 years ago by exarkun

(In [26484]) a docstring for _parseFetchPairs

refs #1105

comment:16 Changed 5 years ago by exarkun

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

refs #1105

comment:17 Changed 5 years ago by exarkun

(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 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 5 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

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

comment:20 Changed 5 years ago by exarkun

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

refs #1105

comment:21 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.