Opened 5 years ago

Closed 2 years ago

#4080 defect closed fixed (fixed)

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

Reported by: exarkun Owned by: exarkun
Priority: low Milestone:
Component: mail Keywords:
Cc: jessica.mckellar@…, dan@… Branch: branches/illegal-search-4080
(diff, github, buildbot, log)
Author: exarkun, jesstess Launchpad Bug:

Description

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.

Attachments (3)

twisted-4080.patch (1.6 KB) - added by wulczer 4 years ago.
imap_illegal_search_4080.patch (6.5 KB) - added by danmil 2 years ago.
imap_illegal_search_4080_2.patch (7.1 KB) - added by danmil 2 years ago.
Second attempt at patch, incorporating exarkun's comments.

Download all attachments as: .zip

Change History (19)

Changed 4 years ago by wulczer

comment:1 Changed 4 years ago by 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.

comment:2 Changed 4 years ago by wulczer

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

comment:3 Changed 4 years ago by wulczer

  • Keywords review added
  • Owner exarkun deleted

comment:4 Changed 4 years ago by jesstess

  • Owner set to jesstess

comment:5 Changed 4 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/imap-illegal-search-4080

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

comment:6 follow-up: Changed 4 years ago by jesstess

  • Cc jessica.mckellar@… added
  • Keywords review removed
  • Owner changed from jesstess to wulczer

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!

comment:7 in reply to: ↑ 6 Changed 4 years ago by 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.

Changed 2 years ago by danmil

comment:8 Changed 2 years ago by danmil

  • Branch branches/imap-illegal-search-4080 deleted
  • Cc dan@… added
  • Keywords review added
  • Owner wulczer deleted

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.

comment:9 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:10 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to danmil
  • Status changed from assigned to new

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!

Changed 2 years ago by danmil

Second attempt at patch, incorporating exarkun's comments.

comment:11 Changed 2 years ago by danmil

  • Keywords review added
  • Owner changed from danmil to exarkun

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.

comment:12 Changed 2 years ago by exarkun

  • Author changed from jesstess to exarkun, jesstess
  • Branch set to branches/illegal-search-4080

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

comment:13 Changed 2 years ago by exarkun

(In [35072]) Apply imap_illegal_search_4080_2.patch

refs #4080

comment:14 Changed 2 years ago by exarkun

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

refs #4080

comment:15 Changed 2 years ago by exarkun

  • Keywords review removed

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.

comment:16 Changed 2 years ago by exarkun

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

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

Note: See TracTickets for help on using tickets.