bpo-13940: imaplib: All string arguments are now quoted when necessary. #6395
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
Additionally, raise an error when given CRLF in arguments. Add tests for the above behaviour.
|
My email address didn't match up with the one I used for the CLA. I've now updated the commit to have the correct email. |
|
Looks good, great help for |
|
Any chance this PR could get a review? |
I usually get best results when asking in the business hours on |
| @@ -110,7 +110,7 @@ | |||
| br'"') | |||
| # Literal is no longer used; kept for backward compatibility. | |||
| Literal = re.compile(br'.*{(?P<size>\d+)}$', re.ASCII) | |||
| MapCRLF = re.compile(br'\r\n|\r|\n') | |||
| MapCRLF = re.compile(br'[\r\n]') | |||
| # We no longer exclude the ']' character from the data portion of the response | |||
There was a problem hiding this comment.
I don't think this is correct. MapCRLF is only used in one line, as far as I can tell, to normalize all line endings:
literal = MapCRLF.sub(CRLF, message)
With the new pattern, b'ab\r\ncd\r\n' is replaced with b'ab\r\n\r\ncd\r\n\r\n'. The old pattern leaves that string unchanged and correctly replaces b'ab\ncd\n' with b'ab\r\ncd\r\n'.
Also, that "normalization" seems to be controversial: https://bugs.python.org/issue5430
There was a problem hiding this comment.
Thanks for pointing this out - I guess I should just take out the substitution, and add a test for it, so as to fix bpo5430 as well.
|
Any chance of getting some movement on this? The bug is a serious regression (2.x handles the quoting correctly) and it was reported over seven years ago. 🙁 We do want folks to migrate to Python 3, right? 😃 |
|
Also, I should point out that I believe the way the patch is currently implemented is a breaking change, beyond the bit about CRLF rejection. The documentation (before the patch is applied) says that the quoting will not be applied if the argument is already enclosed in double quotes, and that's how it was implemented in 2.7 when the quoting was actually done correctly. So anyone who has run into this bug in 3.x will have worked around it by enclosing (for example) folder names in double quotes. It's possible that I'm misreading the code in the patch (though that seems unlikely, given the corresponding change the patch applies to the documentation), but it seems that the patch will add a duplicate set of double quotes to an arg which is already quoted. |
|
@bearbin, I think you need to make some changes to get this pull request in.
|
|
This PR is stale because it has been open for 30 days with no activity. |
| if _Quoted_Invalid.search(arg): | ||
| raise self.error('illegal cr or lf in argument') | ||
| if len(arg) > 2 and [arg[0], arg[-1]] == ['(', ')']: | ||
| arg = arg[1:-1] |
There was a problem hiding this comment.
This looks like it is removing the brackets, which seems undesirable. Compare with the original _checkquote implementation removed in commit f241afa.
github-actions
bot
removed
the
stale
|
This PR is stale because it has been open for 30 days with no activity. |
Additionally, raise an error when given CRLF in arguments (this is invalid by the IMAP spec).
Add tests for the above behaviour.
https://bugs.python.org/issue13940