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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:05 | admin | set | github: 66014 |
2016-01-02 22:26:16 | python-dev | set | messages:
+ msg257376 |
2016-01-02 22:24:50 | r.david.murray | set | status: open -> closed resolution: fixed stage: commit review -> resolved |
2016-01-02 22:24:37 | r.david.murray | set | messages:
+ msg257374 |
2016-01-02 22:20:04 | python-dev | set | nosy:
+ python-dev messages:
+ msg257372
|
2015-12-30 21:01:39 | maciej.szulik | set | messages:
+ msg257241 stage: patch review -> commit review |
2015-12-29 15:43:44 | r.david.murray | set | messages:
+ msg257195 |
2015-12-29 09:14:41 | maciej.szulik | set | messages:
+ msg257172 |
2015-12-23 07:03:31 | Lita.Cho | set | files:
+ imaplib_after_silentghost_review.patch
messages:
+ msg256902 |
2015-12-23 00:05:28 | Lita.Cho | set | messages:
+ msg256885 |
2015-12-22 23:05:33 | maciej.szulik | set | messages:
+ msg256879 |
2015-12-20 07:27:17 | Lita.Cho | set | files:
+ imaplib_after_review.patch
messages:
+ msg256752 |
2015-12-17 12:05:17 | maciej.szulik | set | messages:
+ msg256599 |
2015-12-16 21:45:22 | Lita.Cho | set | messages:
+ msg256534 |
2015-12-08 20:33:21 | maciej.szulik | set | messages:
+ msg256126 |
2015-12-08 18:24:02 | Lita.Cho | set | messages:
+ msg256121 |
2015-12-07 21:58:19 | maciej.szulik | set | messages:
+ msg256077 |
2015-04-13 22:38:44 | maciej.szulik | set | messages:
+ msg240792 |
2015-04-13 19:32:38 | berker.peksag | set | nosy:
+ berker.peksag messages:
+ msg240712
|
2015-04-13 18:29:25 | Lita.Cho | set | messages:
+ msg240701 |
2015-04-13 16:50:07 | maciej.szulik | set | nosy:
+ maciej.szulik messages:
+ msg240657
|
2014-11-22 16:11:46 | berker.peksag | set | versions:
+ Python 3.4, Python 3.5 |
2014-11-22 10:34:12 | Lita.Cho | set | files:
+ imaplib_bracket_fix.patch
messages:
+ msg231516 |
2014-11-02 18:48:58 | Lita.Cho | set | messages:
+ msg230517 |
2014-11-01 22:32:23 | ezio.melotti | set | messages:
+ msg230467 stage: test needed -> patch review |
2014-08-14 23:18:42 | Lita.Cho | set | messages:
+ msg225322 |
2014-07-17 00:14:18 | Lita.Cho | set | files:
+ imap_regex.patch
messages:
+ msg223303 |
2014-07-17 00:13:35 | Lita.Cho | set | files:
+ imap_doc.patch
messages:
+ msg223302 versions:
- Python 3.4, Python 3.5 |
2014-07-16 23:29:27 | Lita.Cho | set | files:
+ test_imaplib.patch
messages:
+ msg223297 |
2014-07-16 20:23:42 | Lita.Cho | set | messages:
+ msg223265 |
2014-07-16 19:22:37 | r.david.murray | set | messages:
+ msg223254 versions:
+ Python 3.4, Python 3.5 |
2014-07-16 17:43:37 | Lita.Cho | set | messages:
+ msg223239 |
2014-07-16 14:18:04 | rafales | set | messages:
+ msg223207 |
2014-07-16 14:07:25 | r.david.murray | set | messages:
+ msg223204 |
2014-07-16 05:09:45 | Lita.Cho | set | messages:
+ msg223172 |
2014-07-16 04:53:32 | Lita.Cho | set | messages:
+ msg223170 |
2014-07-16 04:28:35 | ezio.melotti | set | nosy:
+ ezio.melotti
stage: test needed |
2014-07-16 03:55:39 | jesstess | set | messages:
+ msg223168 |
2014-07-16 01:06:02 | Lita.Cho | set | files:
+ imaplib_log_with_patch.txt
messages:
+ msg223164 |
2014-07-16 01:05:46 | Lita.Cho | set | files:
+ imap_regex.patch keywords:
+ patch messages:
+ msg223163
|
2014-07-15 00:41:47 | Lita.Cho | set | messages:
+ msg223070 |
2014-07-14 22:32:57 | Lita.Cho | set | nosy:
+ jesstess, Lita.Cho
|
2014-06-22 02:17:02 | r.david.murray | set | nosy:
+ barry, r.david.murray components:
+ email
|
2014-06-20 14:40:45 | rafales | set | files:
+ imaplib_log.txt
messages:
+ msg221092 |
2014-06-20 14:39:56 | rafales | create | |