emersion / go-imap

📥 An IMAP library for clients and servers
MIT License
2.02k stars 288 forks source link

imapclient/search: handle UID SEARCH results without hits #599

Closed Shugyousha closed 3 months ago

Shugyousha commented 3 months ago

When searching for unseen mails we can get the following error if there are none.

in response-data: imapwire: expected SP, got "\r"

The reason is that the IMAP server is sending a response like this in case there are no search hits.

T7 UID SEARCH RETURN (ALL) (UNSEEN)

The wire protocol parser is expecting a search option (like "ALL") to be present in any search result. As we can see above, this is not the case if there are no results, which results in the parser hitting a CRLF when it expected a space.

To account for this we peek into the reply of the server to see whether there is a CRLF after the "UID" atom. If there is, we assume there were no results and return an empty result.

emersion commented 3 months ago

Good catch!

It seems like ESEARCH UID\r\n is also valid per the ABNF from the RFC (and that doesn't seem handled here):

esearch-response  = "ESEARCH" [search-correlator] [SP "UID"]
                    *(SP search-return-data)
                  ; ESEARCH response replaces SEARCH response
                  ; from IMAP4rev1.

Same for a search-correlator followed by CRLF.

Suggestion to handle that case and simplify the code:

func readESearchResponse(dec *imapwire.Decoder) (tag string, data *imap.SearchData, err error) {
    data = &imap.SearchData{}
    if dec.Special('(') { // search-correlator
        var correlator string
        if !dec.ExpectAtom(&correlator) || !dec.ExpectSP() || !dec.ExpectAString(&tag) || !dec.ExpectSpecial(')') {
            return "", nil, dec.Err()
        }
        if correlator != "TAG" {
            return "", nil, fmt.Errorf("in search-correlator: name must be TAG, but got %q", correlator)
        }
    }

    var name string
    if !dec.SP() {
        return tag, data, nil
    } else if !dec.ExpectAtom(&name) {
        return "", nil, dec.Err()
    }
    data.UID = name == "UID"

    if data.UID {
        if !dec.SP() {
            return tag, data, nil
        } else if !dec.ExpectAtom(&name) {
            return "", nil, dec.Err()
        }
    }

    for {
        switch strings.ToUpper(name) {
        // ...
        }

        if !dec.SP() {
            break
        } else if !dec.ExpectAtom(&name) {
            return "", nil, dec.Err()
        }
    }

    return tag, data, nil
}
emersion commented 3 months ago

I've added a basic ESEARCH test in ba82060e47b647fb5afec7025aa3707789599a9a FWIW.

Shugyousha commented 3 months ago

I've added a basic ESEARCH test in https://github.com/emersion/go-imap/commit/ba82060e47b647fb5afec7025aa3707789599a9a FWIW.

Nice, thanks!

I have used your suggestion but had to add an extra "ExpectSP" to get the tests to pass. Please have another look!

emersion commented 3 months ago

Ah right! I think we need to move the ExpectSP inside the loop though, to handle cases where multiple options are parsed?

Shugyousha commented 3 months ago

Ah right! I think we need to move the ExpectSP inside the loop though, to handle cases where multiple options are parsed?

I think you are correct!

I have moved the ExpectSP into the loop and tested that it worked for my use cases.