Conversation
dejanstrbac
commented
Sep 21, 2025
dejanstrbac
commented
- According to RFC 4731, a server must ignore any unrecognized RETURN options
- handle empty NumSets
- readSearchKeyWithAtom should return errors instead of swallowing them
- tag in search response should be quoted, would cause iOS Mail mishandling
- replaced deprecated strings.Title
…ptions; readSearchKeyWithAtom should return errors instead of swallowing them; tag in search response should be quoted; replaced deprecated strings.Title
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
This PR mixes multiple unrelated things together. This makes review quite tedious.
go-imap uses linear, recipe style commits. Each commit is small and self-contained. Commits start with the name of the package they touch as a prefix.
If a PR does a single thing, I can squash the changes into a single commit and edit the commit message to conform. If the PR does multiple things but they are split into logical commits, I can review commits one-by-one, and merge them incrementally if desired.
| options.ReturnSave = true | ||
| default: | ||
| return newClientBugError("unknown SEARCH RETURN option") | ||
| // RFC 4731: A server MUST ignore any unrecognized return options. |
There was a problem hiding this comment.
I don't see where this is specified in RFC 4731? This would also be surprising to clients: they ask for something and silently get a reply without it.
RFC 4466 says that return options may be followed by optional search-mod-params, so we can't just skip the name anyways.
|
|
||
| func searchKeyFlag(key string) imap.Flag { | ||
| return imap.Flag("\\" + strings.Title(strings.ToLower(key))) | ||
| return imap.Flag("\\" + cases.Title(language.English).String(strings.ToLower(key))) |
There was a problem hiding this comment.
We'll never get anything other than ASCII here. I'd prefer not to introduce a new dependency with large Unicode tables just for this.
| if err := readSearchKey(criteria, dec); err != nil { | ||
| return err | ||
| } | ||
| if !dec.SP() { |
| } | ||
| if !dec.ExpectNumSet(numKind, &data.All) { | ||
| return "", nil, dec.Err() | ||
| if dec.SP() { |
There was a problem hiding this comment.
Grammar from RFC 4731 says that ALL must always be followed by a seq-set:
"ALL" SP sequence-setIs a client not conforming?
|
|
||
| if c.enabled.Has(imap.CapIMAP4rev2) || extended { | ||
| var supportsESEARCH bool | ||
| if capSession, ok := c.session.(SessionCapabilities); ok { |
There was a problem hiding this comment.
Seems like this is the only place where SessionCapabilities is used? I would've expected it to affect everything, not just SEARCH.
I do think it would be nice to be able to customize supported caps per-conn, FWIW.
3 participants
