classification
Title: imaplib truncates some untagged responses
Type: behavior Stage: resolved
Components: email, Library (Lib) Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Lita.Cho, barry, berker.peksag, ezio.melotti, jesstess, maciej.szulik, python-dev, r.david.murray, rafales
Priority: normal Keywords: patch

Created on 2014-06-20 14:39 by rafales, last changed 2016-01-02 22:26 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
imaplib_log.txt rafales, 2014-06-20 14:40
imap_regex.patch Lita.Cho, 2014-07-16 01:05
imaplib_log_with_patch.txt Lita.Cho, 2014-07-16 01:06
test_imaplib.patch Lita.Cho, 2014-07-16 23:29
imap_doc.patch Lita.Cho, 2014-07-17 00:13
imap_regex.patch Lita.Cho, 2014-07-17 00:14
imaplib_bracket_fix.patch Lita.Cho, 2014-11-22 10:34 review
imaplib_after_review.patch Lita.Cho, 2015-12-20 07:27 review
imaplib_after_silentghost_review.patch Lita.Cho, 2015-12-23 07:03 review
Messages (39)
msg221091 - (view) Author: Rafał Stożek (rafales) Date: 2014-06-20 14:39
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
msg221092 - (view) Author: Rafał Stożek (rafales) Date: 2014-06-20 14:40
Oops, forgot the file.
msg223070 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-15 00:41
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.
msg223163 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 01:05
I have a patch for this. With my patch, the debug output is fixed.
msg223164 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 01:06
Here is a log of the output.
msg223168 - (view) Author: Jessica McKellar (jesstess) * Date: 2014-07-16 03:55
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.
msg223170 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 04:53
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()
msg223172 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 05:09
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')
msg223204 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-07-16 14:07
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).
msg223207 - (view) Author: Rafał Stożek (rafales) Date: 2014-07-16 14:18
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>
> _______________________________________
>
msg223239 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 17:43
>
> 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>
> _______________________________________
>
msg223254 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-07-16 19:22
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.
msg223265 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 20:23
Okay, sounds good. I will also create a patch in the documentation that explains this, as well as comment on the regex patch.
msg223297 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-16 23:29
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.
msg223302 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-17 00:13
Updated the documentation in imaplib.rst to describe the RFC violation.
msg223303 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-07-17 00:14
Updated my regex patch to include a comment about how we are violating the RFC and allowing all characters rather thane excluding the ] character.
msg225322 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-08-14 23:18
pinging for another review. I have included tests for the patch as well as documentation!
msg230467 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-11-01 22:32
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.
msg230517 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-11-02 18:48
Sure, let me combine it into one change.
msg231516 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-11-22 10:34
Here is the patch merged together. I apologize for not getting it to you sooner.
msg240657 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-04-13 16:50
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.
msg240701 - (view) Author: Lita Cho (Lita.Cho) * Date: 2015-04-13 18:29
Hi Maciej,

I am not seeing berkerpeksag review...? What was his comment? I apologize, but I haven't used the bug tracker in awhile.
msg240712 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-04-13 19:32
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)
msg240792 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-04-13 22:38
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 :)
msg256077 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-12-07 21:58
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.
msg256121 - (view) Author: Lita Cho (Lita.Cho) * Date: 2015-12-08 18:24
I apologize, I completely forgot. I will do it this week. Thanks for the
reminder!
msg256126 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-12-08 20:33
Perfect, thanks!
msg256534 - (view) Author: Lita Cho (Lita.Cho) * Date: 2015-12-16 21:45
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>
> _______________________________________
>
msg256599 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-12-17 12:05
Lita if you still have problems or want to ask questions reach out to me on IRC, I'm soltysh on freenode.
msg256752 - (view) Author: Lita Cho (Lita.Cho) * Date: 2015-12-20 07:27
Applied the changes that berkerpeksag made. Please review, and let me know if further changes need to be made.
msg256879 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-12-22 23:05
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.
msg256885 - (view) Author: Lita Cho (Lita.Cho) * Date: 2015-12-23 00:05
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>
> _______________________________________
>
msg256902 - (view) Author: Lita Cho (Lita.Cho) * Date: 2015-12-23 07:03
Here is a patch after SlientGhost's review. I have added back the newline and included the comments for the imaplib documentation.
msg257172 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-12-29 09:14
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.
msg257195 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-29 15:43
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...)
msg257241 - (view) Author: Maciej Szulik (maciej.szulik) * Date: 2015-12-30 21:01
I was looking for that but I missed the Stage field, thanks David will remember for the future :)
msg257372 - (view) Author: Roundup Robot (python-dev) Date: 2016-01-02 22:20
New changeset 54b36229021a by R David Murray in branch 'default':
#21815: violate IMAP RFC to be compatible with, e.g., gmail
https://hg.python.org/cpython/rev/54b36229021a
msg257374 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-01-02 22:24
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.
msg257376 - (view) Author: Roundup Robot (python-dev) Date: 2016-01-02 22:26
New changeset 53271aa4d84c by R David Murray in branch 'default':
#21815: Make the doc change match what I actually did.
https://hg.python.org/cpython/rev/53271aa4d84c
History
Date User Action Args
2016-01-02 22:26:16python-devsetmessages: + msg257376
2016-01-02 22:24:50r.david.murraysetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2016-01-02 22:24:37r.david.murraysetmessages: + msg257374
2016-01-02 22:20:04python-devsetnosy: + python-dev
messages: + msg257372
2015-12-30 21:01:39maciej.szuliksetmessages: + msg257241
stage: patch review -> commit review
2015-12-29 15:43:44r.david.murraysetmessages: + msg257195
2015-12-29 09:14:41maciej.szuliksetmessages: + msg257172
2015-12-23 07:03:31Lita.Chosetfiles: + imaplib_after_silentghost_review.patch

messages: + msg256902
2015-12-23 00:05:28Lita.Chosetmessages: + msg256885
2015-12-22 23:05:33maciej.szuliksetmessages: + msg256879
2015-12-20 07:27:17Lita.Chosetfiles: + imaplib_after_review.patch

messages: + msg256752
2015-12-17 12:05:17maciej.szuliksetmessages: + msg256599
2015-12-16 21:45:22Lita.Chosetmessages: + msg256534
2015-12-08 20:33:21maciej.szuliksetmessages: + msg256126
2015-12-08 18:24:02Lita.Chosetmessages: + msg256121
2015-12-07 21:58:19maciej.szuliksetmessages: + msg256077
2015-04-13 22:38:44maciej.szuliksetmessages: + msg240792
2015-04-13 19:32:38berker.peksagsetnosy: + berker.peksag
messages: + msg240712
2015-04-13 18:29:25Lita.Chosetmessages: + msg240701
2015-04-13 16:50:07maciej.szuliksetnosy: + maciej.szulik
messages: + msg240657
2014-11-22 16:11:46berker.peksagsetversions: + Python 3.4, Python 3.5
2014-11-22 10:34:12Lita.Chosetfiles: + imaplib_bracket_fix.patch

messages: + msg231516
2014-11-02 18:48:58Lita.Chosetmessages: + msg230517
2014-11-01 22:32:23ezio.melottisetmessages: + msg230467
stage: test needed -> patch review
2014-08-14 23:18:42Lita.Chosetmessages: + msg225322
2014-07-17 00:14:18Lita.Chosetfiles: + imap_regex.patch

messages: + msg223303
2014-07-17 00:13:35Lita.Chosetfiles: + imap_doc.patch

messages: + msg223302
versions: - Python 3.4, Python 3.5
2014-07-16 23:29:27Lita.Chosetfiles: + test_imaplib.patch

messages: + msg223297
2014-07-16 20:23:42Lita.Chosetmessages: + msg223265
2014-07-16 19:22:37r.david.murraysetmessages: + msg223254
versions: + Python 3.4, Python 3.5
2014-07-16 17:43:37Lita.Chosetmessages: + msg223239
2014-07-16 14:18:04rafalessetmessages: + msg223207
2014-07-16 14:07:25r.david.murraysetmessages: + msg223204
2014-07-16 05:09:45Lita.Chosetmessages: + msg223172
2014-07-16 04:53:32Lita.Chosetmessages: + msg223170
2014-07-16 04:28:35ezio.melottisetnosy: + ezio.melotti

stage: test needed
2014-07-16 03:55:39jesstesssetmessages: + msg223168
2014-07-16 01:06:02Lita.Chosetfiles: + imaplib_log_with_patch.txt

messages: + msg223164
2014-07-16 01:05:46Lita.Chosetfiles: + imap_regex.patch
keywords: + patch
messages: + msg223163
2014-07-15 00:41:47Lita.Chosetmessages: + msg223070
2014-07-14 22:32:57Lita.Chosetnosy: + jesstess, Lita.Cho
2014-06-22 02:17:02r.david.murraysetnosy: + barry, r.david.murray
components: + email
2014-06-20 14:40:45rafalessetfiles: + imaplib_log.txt

messages: + msg221092
2014-06-20 14:39:56rafalescreate