unexpected search command breaks imap search

[tvachon]

twisted.mail.imap4.IMAP4Server.singleSearchStep has code to handle unexpected search commands, but this code cannot be reached (line 1466):

            f = getattr(self, 'search_' + c)
            if f:
                if not f(query, id, msg):
                    return False
            else:
                # IMAP goes *out of its way* to be complex
                # Sequence sets to search should be specified
                # with a command, like EVERYTHING ELSE.
                try:
                    m = parseIdList(c)
                except:
                    log.err('Unknown search term: ' + c)
                else:
                    if id not in m:
                        return False

Since f = getattr(self, 'search_' + c) raises an AttributeError if 'search_' + c does not exist, f can never evaluate to False, so the "else" suite never runs.

The fix is to replace

f = getattr(self, 'search_' + c) 

with

f = getattr(self, 'search_' + c, None)

This problem arises from IMAP client commands like

00000003 SEARCH 1 UNDELETED UNSEEN

The only client I've seen use this is Pine, but since this does break compatibility with Pine, I've marked its priority as High.

[tvachon]

fix for this defect

[tvachon]

This and ticket #1978 are my first bug submissions to Twisted. I expect that just submission of these patches is not enough to get them included, and would be more than happy to follow up with whatever steps are needed to move the inclusion process along.

Thanks,

Travis Vachon

[glyph]

These fixes will probably be included in due course. The thing you can do which will move them along the fastest is to write unit tests. No new code (even bugfixes!) goes into Twisted without tests: otherwise we don't have a rigorous way of determining that the bug was actually fixed, fixed on all platforms, remains fixed, etc.

[tvachon]

Unittest for this defect

[exarkun]

Thanks for the patch and test. I modified the test a bit to actually assert something about the results from search(). Unfortunately, search support with message sets seems to be completely broken. The patch prevents an exception from being raised, but does not correctly restrict the result set.

Searching for 1 should return at most a result set containing 1. However, with Twisted's search implementation, it returns the entire contents of the mailbox. I'm not exactly sure why this is right now.

I've checked your patch and my modifications into a branch, source:/branches/imap4-search-1977, if you'd like to take a look and make further changes.

I probably won't get back to this for at least a week, quite possibly longer.

[exarkun]

btw, that link won't work until tomorrow.

[exarkun]

I can't see any reason this needs to be targeted at 9.0 (8.2+1).

[jpsimons]

I ran into the same issue. The iPhone's IMAP client sends a search like 1:* NOT DELETED which ends up hitting this bug. The problem is you can fix it like you mention with a None default for get_attr, but then it's running new code than never ran before, which is itself buggy. My 1:* creates a MessageSet with an undefined last property, so contains ends up failing with the *. I patched it like this (there's a better way I'm sure):

            c = q.upper()
-           f = getattr(self, 'search_' + c)
+           f = getattr(self, 'search_' + c, None)
            if f:
                if not f(query, id, msg):
                    return False
            else:
                # IMAP goes *out of its way* to be complex
                # Sequence sets to search should be specified
                # with a command, like EVERYTHING ELSE.
                try:
                    m = parseIdList(c)
+                   m.last = 99999999999
                except:
                    log.err('Unknown search term: ' + c)
                else:
                    if id not in m:
                        return False

[exarkun]

#4048 was a duplicate of this.

[therve]

(In [27367]) Branching to 'imap4-search-1977-2'

[therve]

I went on and had a look. The branch wasn't merging cleanly, and the test didn't pass anymore. I fixed it, and fix the other problem with '1:*'. I can't review that anymore, though.

[exarkun]

There are some IMAP4 issues. :/ Itamar and I will try to fix them.

[exarkun]

Okay! Hopefully things are a bit better now.

[therve]

Alright! Following the great rule that I reviewed what I didn't write, please merge this one.

[exarkun]

(In [27483]) Merge imap4-search-1977-2

Author: tvachon, therve, exarkun, itamar Reviewer: exarkun, itamar, therve Fixes: #1977

Fix twisted.mail.imap4.IMAP4Server's handling of the SEARCH command so that it handles the bare <sequence set> case. Previously this would result in an unhandled exception.

[exarkun]

(In [27489]) Revert r27483 - test suite regression on Python < 2.6

Reopens #1977

[exarkun]

(In [27490]) Use zip range len + 1 instead of enumerate because Python 2.5 and earlier don't have two-argument enumerate

refs #1977

[exarkun]

(In [27499]) Merge imap4-search-1977-2

Author: tvachon, therve, exarkun, itamar Reviewer: exarkun, itamar, therve Fixes: #1977

Fix twisted.mail.imap4.IMAP4Server's handling of the SEARCH command so that it handles the bare <sequence set> case. Previously this would result in an unhandled exception.

This remerge replaces the use of a Python 2.6-only construct (two-arg enumerate) with a more Python 2.3-friendly construct.