Ticket #4080 defect closed fixed

Opened 4 years ago

Last modified 21 months ago

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

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

Change History

Changed 4 years ago by wulczer

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.

2

  Changed 4 years ago by wulczer

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

3

  Changed 4 years ago by wulczer

  • owner exarkun deleted
  • keywords review added

4

  Changed 4 years ago by jesstess

  • owner set to jesstess

5

  Changed 4 years ago by jesstess

  • branch set to branches/imap-illegal-search-4080
  • branch_author set to jesstess

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

6

follow-up: ↓ 7   Changed 4 years ago by jesstess

  • owner changed from jesstess to wulczer
  • keywords review removed
  • cc jessica.mckellar@… added

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!

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 22 months ago by danmil

8

  Changed 22 months ago by danmil

  • keywords review added
  • owner wulczer deleted
  • cc dan@… added
  • branch branches/imap-illegal-search-4080 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.

9

  Changed 21 months ago by exarkun

  • status changed from new to assigned
  • owner set to exarkun

10

  Changed 21 months ago by exarkun

  • status changed from assigned to new
  • owner changed from exarkun to danmil
  • keywords review removed

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 21 months ago by danmil

Second attempt at patch, incorporating exarkun's comments.

11

  Changed 21 months 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.

12

  Changed 21 months ago by exarkun

  • branch set to branches/illegal-search-4080
  • branch_author changed from jesstess to exarkun, jesstess

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

13

  Changed 21 months ago by exarkun

(In [35072]) Apply imap_illegal_search_4080_2.patch

refs #4080

14

  Changed 21 months ago by exarkun

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

refs #4080

15

  Changed 21 months 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.

16

  Changed 21 months ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

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