imaplib truncates some untagged responses

[e0c8f293-d3b2-4555-a9cd-c63f7bd1446f]

BPO	21815
Nosy	@warsaw, @ezio-melotti, @bitdancer, @berkerpeksag, @soltysh
Files	imaplib_log.txt imap_regex.patch imaplib_log_with_patch.txt test_imaplib.patch imap_doc.patch imap_regex.patch imaplib_bracket_fix.patch imaplib_after_review.patch imaplib_after_silentghost_review.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = ['type-bug', 'library', 'expert-email'] title = 'imaplib truncates some untagged responses' updated_at = user = 'https://bugs.python.org/rafales' ``` bugs.python.org fields: ```python activity = actor = 'python-dev' assignee = 'none' closed = True closed_date = closer = 'r.david.murray' components = ['Library (Lib)', 'email'] creation = creator = 'rafales' dependencies = [] files = ['35706', '35962', '35963', '35977', '35978', '35979', '37245', '41366', '41395'] hgrepos = [] issue_num = 21815 keywords = ['patch'] message_count = 39.0 messages = ['221091', '221092', '223070', '223163', '223164', '223168', '223170', '223172', '223204', '223207', '223239', '223254', '223265', '223297', '223302', '223303', '225322', '230467', '230517', '231516', '240657', '240701', '240712', '240792', '256077', '256121', '256126', '256534', '256599', '256752', '256879', '256885', '256902', '257172', '257195', '257241', '257372', '257374', '257376'] nosy_count = 9.0 nosy_names = ['barry', 'ezio.melotti', 'r.david.murray', 'jesstess', 'python-dev', 'berker.peksag', 'maciej.szulik', 'Lita.Cho', 'rafales'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue21815' versions = ['Python 2.7', 'Python 3.4', 'Python 3.5'] ```

[e0c8f293-d3b2-4555-a9cd-c63f7bd1446f]

The regexp in Response_code checks for the existence of [] characters. The problem is that the strings may contain [] characters. I attach debug log which shows the data received from server and what the python extracted from it.

You can see that the PERNANENTFLAGS is truncated and everything from the ] character is lost:

(\Answered \Flagged \Draft \Deleted \Seen OIB-Seen-OIB/Social Networking $Phishing $Forwarded OIB-Seen-OIB/Home OIB-Seen-OIB/Shopping OIB-Seen-INBOX OIB-Seen-OIB/Business OIB-Seen-OIB/Entertainment $NotJunk $NotPhishing $Junk OIB-Seen-[Gmail

[e0c8f293-d3b2-4555-a9cd-c63f7bd1446f]

Oops, forgot the file.

[3c449b37-58db-4fad-a8ce-48269629ff90]

I was reading the RFC spec, and it looks like it doesn't specificy '[' and ']' are not allowed in the PERNANENTFLAGS names.

I can try to add a fix for this. But if anyone else knows if you are not allowed to have '[' or ']' in flag names, please let me know.

[3c449b37-58db-4fad-a8ce-48269629ff90]

I have a patch for this. With my patch, the debug output is fixed.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Here is a log of the output.

[jesstess]

Thanks for the patch, Lita! Lita: I have some patch feedback for you at the end of this message.

--

Lita and I talked about this a bit via email; here's some additional context from that conversation:

There were a couple of questions to answer before working on a patch:

1. Are brackets allowed in flag names according to the RFC?
2. If brackets aren't allowed, do other popular IMAP services permit them anyway? If so, we should consider allowing them even though this deviates from the RFC.

To answer (1), my read of http://tools.ietf.org/html/rfc3501 is as follows:

a. Look at http://tools.ietf.org/html/rfc3501, search for PERMANENTFLAGS definition. Note use of "flag-perm" variable.

b. Search for "flag-perm". Find it on page 85, in section 9, Formal Syntax. This is describing the formal syntax in Backus-Naur Form (BNF).

c. flag-perm = flag / "\*"

d. flag = "\Answered" / "\Flagged" / "\Deleted" / "\Seen" / "\Draft" / flag-keyword / flag-extension ; Does not include "\Recent"

e. flag-keyword = atom

f. atom = 1*ATOM-CHAR

g. ATOM-CHAR = \<any CHAR except atom-specials>

h. atom-specials = "(" / ")" / "{" / SP / CTL / list-wildcards / quoted-specials / resp-specials

i. resp-specials = "]"

My reading of this series of definitions is that "]" is not allowed in flag names.

To answer (2), Lita ran some tests against GMail's IMAP service and found that it does let you create flags containing "]".

--

Patch feedback:

1. Can you attach the script you used to probe GMail's IMAP service for what flag names were permitted?

2. Since this patch causes us to violate the RFC (but with good reason!), can you add a comment above the regex noting the violation and stating what characters we allow for flags and why?

3. This change need tests.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Yes! I agree, this change will need tests. I will start working on creating those now.

Here is test I did to create a flag with brackets.

import imaplib

mail = imaplib.IMAP4_SSL('imap.gmail.com')
mail.login('username@gmail.com', 'password') # Enter your login here
mail.select('test') # Mailbox selection. I have a test inbox with 6
                    # messages in it.
code, [msg_ids] = mail.search(None, 'ALL')
first_id = msg_ids.split()[0]
mail.store(first_id, "+FLAGS", "[test]")
typ, response = mail.fetch(first_id, '(FLAGS)')
print("Flags: %s" % response)
mail.close()
mail.logout()

[3c449b37-58db-4fad-a8ce-48269629ff90]

Here is the code in order to see the bug.

imaplib.Debug = 5
mail = imaplib.IMAP4_SSL('imap.gmail.com')
mail.login(username, password) # Enter your login here
mail.select('test')

[bitdancer]

Just to make sure I understand: the issue is that gmail may produce flags with [] in them, and imaplib currently fails to process such flags when it receives them from gmail?

In principle I think we would not want to allow imaplib to be used to create such flags unless the user specifies some sort of "I want to violate the RFC" flag (which they might want to do, for example, to run tests against gmail :) But currently it looks like it can? (I haven't looked at this in enough detail to be sure.) If that's true we probably have to continue to allow it for backward compatibility reasons, but we should document the RFC violation and possible consequences (an IMAP server rejecting such flags).

[e0c8f293-d3b2-4555-a9cd-c63f7bd1446f]

Yeah, basically. The flags with [] characters are added to gmail (in my case) by OtherInbox's Organizer app. I contacted them but haven't heard back yet.

On Wed, Jul 16, 2014 at 4:07 PM, R. David Murray \report@bugs.python.org\ wrote:

R. David Murray added the comment:

Just to make sure I understand: the issue is that gmail may produce flags with [] in them, and imaplib currently fails to process such flags when it receives them from gmail?

In principle I think we would not want to allow imaplib to be used to create such flags unless the user specifies some sort of "I want to violate the RFC" flag (which they might want to do, for example, to run tests against gmail :) But currently it looks like it can? (I haven't looked at this in enough detail to be sure.) If that's true we probably have to continue to allow it for backward compatibility reasons, but we should document the RFC violation and possible consequences (an IMAP server rejecting such flags).

----------

---

Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue21815\

---

[3c449b37-58db-4fad-a8ce-48269629ff90]

R. David Murray added the comment:

Just to make sure I understand: the issue is that gmail may produce flags with [] in them, and imaplib currently fails to process such flags when it receives them from gmail?

This is correct. Gmail allows you to create flags with [], and the Response_code regex doesn't process them properly.

In principle I think we would not want to allow imaplib to be used to create such flags unless the user specifies some sort of "I want to violate the RFC" flag (which they might want to do, for example, to run tests against gmail :) But currently it looks like it can? (I haven't looked at this in enough detail to be sure.) If that's true we probably have to continue to allow it for backward compatibility reasons, but we should document the RFC violation and possible consequences (an IMAP server rejecting such flags).

Yes, currently we can. I've posted the code in order to do this. This is basically the result.

>>> first_id = msg_ids.split()[0]
>>> mail.store(first_id, "+FLAGS", "[test]")
('OK', [b'1 (FLAGS (\\Seen Answered [test] NotJunk $NotJunk [Brackets]
[testing2]))'])

However, I would think it would be the server's job to uphold this rule, not the library. The server should return with a BAD response, but right now, Gmail allows you to do this.

Should we throw a warning in the "store" method? Otherwise, I can update the documenation in the "store" method stating that having '[]' is allowed but violates the RFC protocol.

----------

---

Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue21815\

---

[bitdancer]

Postel's Law says: be conservative in what you do, be liberal in what you accept from others. So the client should not violate the RFC when sending data to the server. The server, on the other hand, by that law could accept "dirty" data if it can do something reasonable with it...except that in general the IMAP RFC in particular says "don't do that". IMAP servers are supposed to be IMAP RFC sticklers, even when accepting input. So gmail is broken for some definition of broken, and so is imaplib.

Since the code has been this way for a long time, and gmail does in fact accept it, I think a doc warning about []s violating the RFC should be sufficient; a warning would probably just be annoying to little real benefit.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Okay, sounds good. I will also create a patch in the documentation that explains this, as well as comment on the regex patch.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Here is a patch for test_imaplib.py, adding the test for brackets. It should fail with the current version of imaplib, but should pass with the imap_regex.patch.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Updated the documentation in imaplib.rst to describe the RFC violation.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Updated my regex patch to include a comment about how we are violating the RFC and allowing all characters rather thane excluding the ] character.

[3c449b37-58db-4fad-a8ce-48269629ff90]

pinging for another review. I have included tests for the patch as well as documentation!

[ezio-melotti]

Thanks for the patches Lita, however it's better if you can upload a single patch with all the required changes. This will make it easier to apply/review it.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Sure, let me combine it into one change.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Here is the patch merged together. I apologize for not getting it to you sooner.

[soltysh]

Lita thanks for your patch!. Are you able to address berkerpeksag comments from review? Otherwise if you don't mind I'll take it over and I'll do that.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Hi Maciej,

I am not seeing berkerpeksag review...? What was his comment? I apologize, but I haven't used the bug tracker in awhile.

[berkerpeksag]

Hi Lita, my review comments are at http://bugs.python.org/review/21815/ (sorry, I forgot to add a comment here after I made the review)

[soltysh]

Lita always look for them next to your uploaded file, there's a review link that points to Rietveld Code Review Tool. There you'll find all those information, just for future use :)

[soltysh]

Hey Lita, final call ;) Can you address berkerpeksag comments from review? Otherwise, I'm planning to take ownership of this issue and resubmit your patch with addressed comments.

[3c449b37-58db-4fad-a8ce-48269629ff90]

I apologize, I completely forgot. I will do it this week. Thanks for the reminder!

[soltysh]

Perfect, thanks!

[3c449b37-58db-4fad-a8ce-48269629ff90]

Had some trouble setting up my dev environment for Python. Definitely going to work on it today and tomorrow.

On Tue, Dec 8, 2015 at 12:33 PM, Maciej Szulik \report@bugs.python.org\ wrote:

Maciej Szulik added the comment:

Perfect, thanks!

----------

---

Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue21815\

---

[soltysh]

Lita if you still have problems or want to ask questions reach out to me on IRC, I'm soltysh on freenode.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Applied the changes that berkerpeksag made. Please review, and let me know if further changes need to be made.

[soltysh]

Lita can you please apply the changes from latest review (from SilentGhost). Especially the one regarding newline, which currently fails to apply this patch on default. If all those will be cleaned I'll recommend this patch for David for inclusion.

[3c449b37-58db-4fad-a8ce-48269629ff90]

Sounds good.

On Tuesday, December 22, 2015, Maciej Szulik \report@bugs.python.org\ wrote:

Maciej Szulik added the comment:

Lita can you please apply the changes from latest review (from SilentGhost). Especially the one regarding newline, which currently fails to apply this patch on default. If all those will be cleaned I'll recommend this patch for David for inclusion.

----------

---

Python tracker \<report@bugs.python.org \<javascript:;>> \http://bugs.python.org/issue21815\

---

[3c449b37-58db-4fad-a8ce-48269629ff90]

Here is a patch after SlientGhost's review. I have added back the newline and included the comments for the imaplib documentation.

[soltysh]

I've checked the patch: it looks good, applies cleanly to latest default and passes all the tests. I'm ok with merging this as is.

[bitdancer]

When you think a patch is ready for commit, you can move it to commit review, which will put it in my queue (not that I've been getting to that very fast lately, but I'm going to try to be better about it...)

[soltysh]

I was looking for that but I missed the Stage field, thanks David will remember for the future :)

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 54b36229021a by R David Murray in branch 'default': bpo-21815: violate IMAP RFC to be compatible with, e.g., gmail https://hg.python.org/cpython/rev/54b36229021a

[bitdancer]

I rewrote the comments, and I changed the name of the test helper class from GmailHandler to BracketFlagHandler, since this is not gmail specific. (There was a review comment about that that was missed, I guess).

I've applied this only to 3.6 because there is a backward compatibility concern, as I mention in the revised comments: if a server includes a ']' in the text portion, the new regex will parse it incorrectly. (Which is why the rfc restriction exists in the first place.) I'm hoping that this is very unlikely, but because it would be a nasty bug if it happens, I only feel comfortable applying it to 3.6.

Thanks Lita, and everyone who reviewed.

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 53271aa4d84c by R David Murray in branch 'default': bpo-21815: Make the doc change match what I actually did. https://hg.python.org/cpython/rev/53271aa4d84c