Conversation
odrobnik
left a comment
There was a problem hiding this comment.
Requesting changes for four correctness/security defects in the new ENABLE/QRESYNC paths plus one contradictory public documentation entry. UIDONLY flag updates and concurrent VANISHED deletions can be silently lost, arbitrary Capability strings can inject extra IMAP commands, and ordinary SELECT does not honor the CLOSED mailbox boundary after QRESYNC is enabled. No builds or tests were run, per review instructions.
One review with 5 findings — those on changed lines are detailed in inline comments:
- HIGH
Sources/SwiftMail/IMAP/IMAP/Handler/ResyncSelectHandler.swift:83 - HIGH
Sources/SwiftMail/IMAP/IMAP/Handler/ResyncSelectHandler.swift:68 - MEDIUM
Sources/SwiftMail/IMAP/IMAP/Commands/EnableCommand.swift:12 - MEDIUM
Sources/SwiftMail/IMAP/IMAP/Handler/SelectHandler.swift:56 - LOW
README.md— The public capability matrix still marks both ENABLE and QRESYNC as unsupported by IMAPServer even though this PR adds and documents those APIs. A user relying on the primary README will incorrectly conclude the new synchronization feature is unavailable.
To unblock the merge, address every finding: react with 👍 if it is accurate (👎 if not), reply with a brief comment on what was fixed, and mark the item as resolved.
|
Thanks for the review, @odrobnik. Those all make sense to me. I’ve pushed fixes in 23fdb8f and 3d15849:
I also added tests for these cases. The local build, all tests, and strict SwiftLint pass.. |
|
@thatjuan you have to resolve the review comments for a re-review to occur |
|
MissionControl review timed out for head |
Summary
Add support for incremental mailbox synchronization using IMAP ENABLE and QRESYNC.
HIGHESTMODSEQin mailbox selections and support storing checkpoints withCodable.CLOSEDresponse boundaries, clearing old mailbox state for ordinary and QRESYNC selections.Testing
swift buildswift test(494 tests across 72 suites)