Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imaplib truncates some untagged responses #66014

Closed
rafales mannequin opened this issue Jun 20, 2014 · 39 comments
Closed

imaplib truncates some untagged responses #66014

rafales mannequin opened this issue Jun 20, 2014 · 39 comments
Labels
expert-email stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@rafales
Copy link
Mannequin

rafales mannequin commented Jun 20, 2014

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:

    assignee = None
    closed_at = <Date 2016-01-02.22:24:50.004>
    created_at = <Date 2014-06-20.14:39:56.350>
    labels = ['type-bug', 'library', 'expert-email']
    title = 'imaplib truncates some untagged responses'
    updated_at = <Date 2016-01-02.22:26:16.496>
    user = 'https://bugs.python.org/rafales'

    bugs.python.org fields:

    activity = <Date 2016-01-02.22:26:16.496>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-01-02.22:24:50.004>
    closer = 'r.david.murray'
    components = ['Library (Lib)', 'email']
    creation = <Date 2014-06-20.14:39:56.350>
    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']

    @rafales
    Copy link
    Mannequin Author

    rafales mannequin commented Jun 20, 2014

    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

    @rafales rafales mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jun 20, 2014
    @rafales
    Copy link
    Mannequin Author

    rafales mannequin commented Jun 20, 2014

    Oops, forgot the file.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 15, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

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

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

    Here is a log of the output.

    @jesstess
    Copy link
    Member

    jesstess commented Jul 16, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

    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()

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

    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
    Copy link
    Member

    bitdancer commented Jul 16, 2014

    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).

    @rafales
    Copy link
    Mannequin Author

    rafales mannequin commented Jul 16, 2014

    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\>


    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

    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
    Copy link
    Member

    bitdancer commented Jul 16, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

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

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 16, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 17, 2014

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

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 17, 2014

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

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Aug 14, 2014

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

    @ezio-melotti
    Copy link
    Member

    ezio-melotti commented Nov 1, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Nov 2, 2014

    Sure, let me combine it into one change.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Nov 22, 2014

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

    @soltysh
    Copy link

    soltysh commented Apr 13, 2015

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Apr 13, 2015

    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
    Copy link
    Member

    berkerpeksag commented Apr 13, 2015

    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
    Copy link

    soltysh commented Apr 13, 2015

    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
    Copy link

    soltysh commented Dec 7, 2015

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Dec 8, 2015

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

    @soltysh
    Copy link

    soltysh commented Dec 8, 2015

    Perfect, thanks!

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Dec 16, 2015

    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
    Copy link

    soltysh commented Dec 17, 2015

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

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Dec 20, 2015

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

    @soltysh
    Copy link

    soltysh commented Dec 22, 2015

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Dec 23, 2015

    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\>


    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Dec 23, 2015

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

    @soltysh
    Copy link

    soltysh commented Dec 29, 2015

    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
    Copy link
    Member

    bitdancer commented Dec 29, 2015

    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
    Copy link

    soltysh commented Dec 30, 2015

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 2, 2016

    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
    Copy link
    Member

    bitdancer commented Jan 2, 2016

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 2, 2016

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    expert-email stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants