fix: Capability atom conversion for CONDSTORE / QRESYNC

[kylefeng28]

The atoms CONDSTORE and QRESYNC are incorrectly converted to Data::Capability::Unselect instead of Data::Capability::CondStore and Data::Capability::QResync respectively.

I noticed this bug (introduced in #629) when trying to use pimalaya/imap-client to list capabilities and it reported my IMAP server didn't have CONDSTORE, even though other clients report it correctly.

Here is a small proof-of-concept example to demonstrate the bug / fix:
$ cargo run --features ext_condstore_qresync --example condstore_qresync

# examples/condstore_qresync.rs
use imap_types::{core::Atom, response::Capability};

fn main() {
    let test_cases = vec!["IMAP4REV1", "CONDSTORE", "QRESYNC", "UNSELECT", "ENABLE"];

    for capability_str in test_cases {
        let atom = Atom::try_from(capability_str).unwrap();
        let capability = Capability::from(atom);

        println!(
            "Input: {:15} -> Parsed as: {:?}",
            capability_str, capability
        );
    }
}

Before:

Input: IMAP4REV1       -> Parsed as: Imap4Rev1
Input: CONDSTORE       -> Parsed as: Unselect <-- wrong
Input: QRESYNC         -> Parsed as: Unselect <-- wrong
Input: UNSELECT        -> Parsed as: Unselect
Input: ENABLE          -> Parsed as: Enable

After:

Input: IMAP4REV1       -> Parsed as: Imap4Rev1
Input: CONDSTORE       -> Parsed as: CondStore <-- correct
Input: QRESYNC         -> Parsed as: QResync <-- correct
Input: UNSELECT        -> Parsed as: Unselect
Input: ENABLE          -> Parsed as: Enable

[coveralls]

Pull Request Test Coverage Report for Build 22220288471

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 91.324%

---

Totals
Change from base Build 22156260227:	0.0%
Covered Lines:	10557
Relevant Lines:	11560

---

💛 - Coveralls

[duesee]

Oh wow... that's... embarassing. I'm 99% sure our structured fuzz testing would have catched the bug. Thing is: I believe I never ran a fuzz campaign after introducing the extension as we are still on -beta. I'll try to see what to do to still avoid this mistake for unstable extensions!

Thank you very much!

@soywod Would a new release help you? :-)

[duesee]

Thanks!

[soywod]

No need for release at the moment, it can definitely wait!

[duesee]

Ah, I remember. Running ...

cd imap-codec
cargo +nightly fuzz run --features=ext_condstore_qresync,debug response_to_bytes_and_back

... quickly reveals the few remaining issues with ext_condstore_qresync. One of them is that this extension requires (theoretically) a custom SequenceSet that doesn't allow *, see #633. This is a minor problem for consumers of this library because an application can avoid using * when creating IMAP messages. But the promise of imap-codec is to be resistant to misuse, so we will need to fix this before we want to consider this extension stable.

Since the fuzzer finds this instantly -- and crashes, as it should :-) -- I couldn't enable fuzzing of this extension just yet. Good: The fuzzer won't let us stabilize this extension as is. Bad: We have to fix at some point. All help would be greatly appreciated!

[kylefeng28]

Awesome, thanks for the quick review! I'm still new to Rust (and contributing to open source in general) but I'll be happy to help with #633 and other issues I find as well, since I'll be helping out with the CONDSTORE implementation downstream in himalaya/mirador/neverest as well. Appreciate all the great work you guys do here!

[duesee]

That would be awesome! Since you are new to Rust and open source: Your PR + description were perfect! :-)

Should you want to tackle #633, I would ask to first discuss together how we want to tackle it. The SequenceSet implementation is quite elaborate and I feel that maybe it is not worth to try to mix the standard SequenceSet with the custom SequenceSet that CONDSTORE/QRESYNC requires. (Still, I would be open to do breaking changes for v2.0.0 if they are worth it.) Maybe we want to create a wrapper (to accomodate for the fact that we want a subset of all allowed standard sequences). Maybe we want a whole new type.

Very happy to discuss it further!