If the IMAP4 server receives a SEARCH command with an illegal term, it will raise an unhandled error and never respond to the client

[exarkun]

This is because search is implemented with dynamic dispatch but without considering the case where there is no search_ method corresponding to the query term.

This is related to #1977 which encountered the problem when it misinterpreted a message sequence set as an illegal term.

[wulczer]

Proposed patch.

It makes a SEARCH FOO command not to fail in the server, but to not return any results.

This seems to differ from the original intention of the code, which was missing an argument to getattr, but just adding that argument would make SEARCH FOO commands to return all messages in the mailbox.

IMHO not returning anything makes more sense than returning everything (even more sensible would be returning an error to the client, but that'd require some more work.

[wulczer]

Oh, that patch applies over the one from #2278.

[jesstess]

(In [28796]) Branching to 'imap-illegal-search-4080'

[jesstess]

wulczer: before I apply the patch on this branch - in my brief perusal of RFC 3501, "6.4.4.SEARCH Command" it says

Result: OK - search completed

NO - search error: can't search that [CHARSET] or

criteria

BAD - command unknown or arguments invalid

which suggests that one of NO or BAD should be the result. Can you comment on why not returning anything as in your patch is more correct or update the patch?

Thanks for working on these IMAP tickets!

[wulczer]

Replying to jesstess:

which suggests that one of NO or BAD should be the result. Can you comment on why not returning anything as in your patch is more correct or update the patch?

Because it meant less work :-)

I'll try to come up with a more complete fix.

[danmil]

This properly fixes the IMAP server so that, when it receives a SEARCH command with an illegal term, it returns an IMAP error code to the client, and logs an error server side.

I added unit tests and fixes for both 'manual' search (where the server loads all messages from a mailbox and runs the search itself), and also search of a mailbox which implements ISearchableMailbox.

A key problem turned out to be that the do_SEARCH method was stacking together the callback and errback via addCallbacks, which meant that error backs firing out of the first callback were not getting caught. I switched that so that the errBack is attached to the first callback, then had the actual search code raise an exception.

Implementers of ISearchableMailbox are now responsible for raising IllegalQueryError if they find an error in the search criteria (before, there was nothing specified about what they could or should do in that case -- in fact, there was no way for an error to be signalled back to the client, I believe). I added a 'raises' description to the interface to make that clear, and mocked it up in the test.

[exarkun]

Thanks for working on Twisted's IMAP4 support. It's really great to see improvements in this area.

1. twisted/mail/imap4.py
   1. The old code in _singleSearchStep looked like it was trying to handle this case, at least. It tested f (the result of the getattr) against None. But it forgot to pass None as the third argument to the getattr call, so it didn't actually catch the case where no such search method existed. The if f is not None: spelling is preferable to the if not f: spelling: the boolean truth of f isn't of much interest, it's whether the fallback/default case of getattr that was hit that's important.
   2. There's a bunch of new trailing whitespace added in the patch, which we try to keep out of Twisted.
2. twisted/mail/test/test_imap.py
   1. There's a helper in trial, TestCase.assertFailure, that I think will simplify these new tests. It should be usable to replace the searchSucceeded callback.
   2. There are some trivial but critical mistakes in the two new test methods: use of the undefined name result, for example. Since the tests pass anyway, this reveals a big problem with them: even in the presence of random exceptions, they don't fail. I suggest applying just the tests to a pristine Twisted trunk working copy and then working on them until they fail in the anticipated way. Then you should be able to make them pass by applying the imap4.py parts of the patch. This should ensure the tests are providing good coverage of the functionality.
   3. Also, boringly, make sure everything has a docstring, name test methods like test_foo, and also clean up the trailing whitespace in the new test methods.
3. It'd also be great if you could supply a news fragment for this change.

Thanks again!

[danmil]

Second attempt at patch, incorporating exarkun's comments.

[danmil]

Great comments, thanks for the detailed response. The newly attached patch applies as much of this as possible, details below:

1- First off, as to the big, bad gotcha -- the tests have a syntax error but don't fail

Short version: argh. Longer version: I did actually develop these tests failure first, but that one line got caught by my not fully understanding the combo of error backs + the trial unit test context. Specifically, I copied in some other code that attached a _cbStopClient errBack, which was silently eating some (but not all, AFAICT) errors from my actual error back code. I've replaced that with an errBack which reraises errors, and things are much better.

I belive I've now got that all properly sorted out. I did run the tests against a clean tree -- as expected, one of them (test_searchInvalidCriteria) fails. The other does not -- now that I look carefully, I see that, if an ISearchableMailbox had raised IllegalQueryError, the right thing was happening (the issue with where the errback got attached is not crippling in that case, as it was for the other). I'd still like to leave the slight adjustment to the first branch in do_SEARCH, because it properly handles errors if there's an exception in __cbSearch, which the old code did not.

2- assertFailure

I tried to get this to work, but AFAICT, whenever I used it, it was 'handling' the errors before my more-specific error handling code could get at it. Which wasn't actually okay -- I need my specific error handling code to fire so I can verify both the error which gets logged and the results sent back to the client. This might be related to the fact that the test case is running both the client and the server, somehow attached to that deferred. But, in any event, whenever I used assertFailure, I couldn't get my error code to run, and I was seeing the test fail in various ways.

3- test_foo

I did not rename testInvalidTerm to test_invalidTerm because all of the other test methods in FetchSearchStoreTestCase are, e.g. testSearch, testUIDSearch, etc. If you'd like, I'd be happy to rename all of those.

4- Trailing whitespace, docstring, topfiles, not f -> f is None

Got 'em all, I think.

Finally, I'm assigning back to you -- I'm not sure if that's the right review protocol, if not, my apologies.

[exarkun]

(In [35071]) Branching to 'illegal-search-4080'

[exarkun]

(In [35072]) Apply imap_illegal_search_4080_2.patch

refs #4080

[exarkun]

(In [35073]) Switch to using TestCase.assertFailure instead of a custom implementation of the same

refs #4080

[exarkun]

I tried to get [assertFailure] to work, but AFAICT, whenever I used it, it was 'handling' the errors before my more-specific error handling code could get at it.

It handles them by verifying they occur, then makes the exception instance the result of the Deferred. I made the change in r35073 so you can see how it's done.

I did not rename testInvalidTerm to test_invalidTerm because all of the other test methods in FetchSearchStoreTestCase are, e.g. testSearch, testUIDSearch, etc. If you'd like, I'd be happy to rename all of those.

I'm not a fan of mass renamings. Given that, the only way we'll get the code updated to the new standard is if new code follows the new standard, even if it disagrees with local convention. Disagreeing with local convention isn't super awesome either, but I think it's preferable in this case.

I'd still like to leave the slight adjustment to the first branch in do_SEARCH, because it properly handles errors if there's an exception in cbSearch, which the old code did not.

That's okay, I suppose. In practice, there will probably never be an exception from __cbSearch though. And if it is possible to have one, there should be a unit test covering it. Just a couple things to keep in mind in case the situation arises again. :)

The changes look good to me now. If the  build results look good, I'll merge. Thanks.

[exarkun]

(In [35079]) Merge illegal-search-4080

Author: danmil Reviewer: exarkun Fixes: #4080

Change IMAP4Server to handle errors from the search implementation and send back a proper error response to the client when they occur.