emersion / go-imap

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

Allow special characters in unquoted mailbox names #640

Closed quzhi1 closed 2 months ago

quzhi1 commented 2 months ago

Hi @emersion ,

Can I make this change to allow special character in unquoted mailbox names? IMAP providers such as StartMail does that.

I created an issue here: https://github.com/emersion/go-imap/issues/639. Then I found out may be we can do this change to solve the problem. Do you think my change is sufficient?

I used a startmail account to create a bunch of folders with special characters, and then verified my changes. It is working. Here is the related output while using DebugWriter: os.Stdout option:

T2 LIST "" "*"
* LIST (\HasNoChildren) "." Special#Hash
* LIST (\HasNoChildren) "." "Special%Percentage"
* LIST (\HasNoChildren) "." "(Parenthesis)"
* LIST (\HasNoChildren \Marked) "." [Special]
* LIST (\HasNoChildren \UnMarked \Sent) "." "Sent Messages"
* LIST (\HasNoChildren \UnMarked \Trash) "." "Deleted Messages"
* LIST (\HasNoChildren \UnMarked \Junk) "." Junk
* LIST (\HasNoChildren \UnMarked \Drafts) "." Drafts
* LIST (\HasNoChildren) "." INBOX
T2 OK List completed (0.001 + 0.000 secs).

Zhi Qu

quzhi1 commented 2 months ago

@emersion Can you take a look at this proposed change? I am not sure what I did is correct 😊

emersion commented 2 months ago

Thanks for the PR! Could we maybe introduce a new isAStringChar helper function, which would work like IsAtomChar but follow the ASTRING-CHAR rules? It could return IsAtomChar(ch) || ch == ']':

ASTRING-CHAR   = ATOM-CHAR / resp-specials
resp-specials   = "]"

Then we can use

return dec.Expect(dec.Func(ptr, isAStringChar), "ASTRING-CHAR")

at the end of ExpectAString.

Does this make sense?

After reading the diff, it seems like your changes are functionally equivalent, but I'd prefer to structure the code the way indicated above to follow more closely the RFC (in terms of names and logic) and have less unused exported functions.

quzhi1 commented 2 months ago

@emersion Thanks for the review! I agree we should introduce isAStringChar helper function. I took a look at the definition of ASTRING-CHAR in RFC 9051, I found the % and * characters also needs special casing. I updated this PR. Is the change looking good now?

emersion commented 2 months ago

I found the % and * characters also needs special casing

That sounds weird to me... My reading of the RFC is that they shouldn't need special casing.

quzhi1 commented 2 months ago

@emersion You are right... I misread the resp-specials definition. It is only ]. * and % are not included. I just updated my PR. Can you take a look again?