emersion / go-imap

📥 An IMAP library for clients and servers
MIT License
2.1k stars 297 forks source link

v1: serialization of SELECT / FETCH flags allows for empty strings #544

Open ptrcnull opened 1 year ago

ptrcnull commented 1 year ago
failed to select mailbox: in response-data: in flag: imapwire: expected atom, got " "

The error originally comes from alps commit 652ea9c7, using go-imap v2.0.0-alpha.6. Tracing the connection reveals the following exchange:

T4 SELECT INBOX
* FLAGS (\Seen \Answered \Flagged \Deleted \Draft  $Label1 $Label4 $forwarded $label1 \Recent junk nonjunk yeet)

I assume the issue lies in the two spaces between \Draft and $Label1 - however, that's being generated by go-imap v1.2.2-0.20220928192137-6fac715be9cf, running with maddy commit d9920f07.

For the record, RFC 9051 specifies a parenthesized list to be "delimited by space", which might be interpreted ambiguously... but seeing how Thunderbird, Evolution or other mail clients have no issue parsing that response, I imagine it's been assumed that it's valid grammar

ptrcnull commented 1 year ago

which might be interpreted ambiguously

well, now i realised that same RFC has actual grammar rules in section 9, which say:

SP              = <Defined in RFC 5234> = %x20
flag-list       = "(" [flag *(SP flag)] ")"
mailbox-data    =  "FLAGS" SP flag-list / ...

which would mean that the serialization of flags is wrong, either in go-imap or maddy, and the other clients are just being overly flexible with parsing those

ptrcnull commented 1 year ago

aaand attaching a debugger to maddy reveals that it's indeed passing an empty string to both Flags and PermanentFlags:

(dlv) p mbox
*github.com/emersion/go-imap.MailboxStatus {
    Name: "INBOX",
    ReadOnly: false,
    Items: map[github.com/emersion/go-imap.StatusItem]interface {} [
        "MESSAGES": nil, 
        "RECENT": nil, 
        "UIDNEXT": nil, 
        "UIDVALIDITY": nil, 
        "UNSEEN": nil, 
    ],
    ItemsLocker: sync.Mutex {state: 0, sema: 0},
    Flags: []string len: 14, cap: 20, [
        "\\Seen",
        "\\Answered",
        "\\Flagged",
        "\\Deleted",
        "\\Draft",
        "",
        "$Label1",
        "$Label4",
        "$forwarded",
        "$label1",
        "\\Recent",
        "junk",
        "nonjunk",
        "yeet",
    ],
    PermanentFlags: []string len: 15, cap: 24, [
        "\\Seen",
        "\\Answered",
        "\\Flagged",
        "\\Deleted",
        "\\Draft",
        "\\*",
        "",
        "$Label1",
        "$Label4",
        "$forwarded",
        "$label1",
        "\\Recent",
        "junk",
        "nonjunk",
        "yeet",
    ],
    UnseenSeqNum: 10906,
    Messages: 11342,
    Recent: 2652,
    Unseen: 39,
    UidNext: 31161,
    UidValidity: 1527719296,
    AppendLimit: 0,}

while i think this should be a scenario that's being handled a little differently by this library, it's a maddy issue after all, so feel free to close it if there's no desire to fix this in some other way

ptrcnull commented 1 year ago

the same issue exists with Message.Flags - empty string gets serialized as an empty parenthesized list element, which later breaks parsing

emersion commented 1 year ago

FWIW, go-imap v2's server would error out in this situation: https://github.com/emersion/go-imap/blob/38838c502224a34c964f66404a6170efa2e54c12/internal/imapwire/encoder.go#L201