FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-13940: imaplib: All string arguments are now quoted when necessary. by bearbin · Pull Request #6395 · python/cpython · GitHub

/ cpython Public

bpo-13940: imaplib: All string arguments are now quoted when necessary. - #6395

Closed
bearbin wants to merge 1 commit into
python:mainfrom
bearbin:fix-issue-13940
ConversationCommits (1)ChecksFiles changed
Closed

bpo-13940: imaplib: All string arguments are now quoted when necessary.#6395
bearbin wants to merge 1 commit into
python:mainfrom
bearbin:fix-issue-13940

Conversation

bearbin commented Apr 6, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown

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

bearbin requested a review from a team as a code owner April 6, 2018 09:38

the-knights-who-say-ni commented Apr 6, 2018

Copy link
Copy Markdown

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!

imaplib: All string arguments are now quoted when necessary.
e11c082 
Additionally, raise an error when given CRLF in arguments.
Add tests for the above behaviour.

bearbin commented Apr 6, 2018

Copy link
Copy Markdown
Author

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.

bearbin mentioned this pull request Apr 7, 2018
Open

mcepl commented Apr 21, 2018

Copy link
Copy Markdown
Contributor

Looks good, great help for imaplib! Thank you. +1

bearbin commented Jul 6, 2018

Copy link
Copy Markdown
Author

Any chance this PR could get a review?

mcepl commented Jul 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Any chance this PR could get a review?

I usually get best results when asking in the business hours on #python-dev on Freenode. And of course, fixing that problem with NEWS would help as well. Everybody prefers to review PRs in green.

andreasg123 reviewed Nov 3, 2018
View reviewed changes
Comment thread Lib/imaplib.py
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

andreasg123 Nov 3, 2018 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bearbin Nov 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

bkline commented May 23, 2019

Copy link
Copy Markdown
Contributor

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? 😃

bkline commented May 23, 2019

Copy link
Copy Markdown
Contributor

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.

rpuntaie mentioned this pull request Aug 29, 2020
Closed

rpuntaie commented Aug 29, 2020

Copy link
Copy Markdown

@bearbin, I think you need to make some changes to get this pull request in.
The contested parts seem to be

  • password should always be quoted
  • quote only if not quoted already (Python 2.7 does so)
  • can you do without the MapCRLF change?

github-actions Bot commented Aug 16, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 16, 2022
vadmium reviewed Apr 18, 2024
View reviewed changes
Comment thread Lib/imaplib.py
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]

vadmium Apr 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks like it is removing the brackets, which seems undesirable. Compare with the original _checkquote implementation removed in commit f241afa.

joebauer mannequin mentioned this pull request Apr 10, 2022
Closed
vadmium mentioned this pull request Apr 18, 2024
Closed
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Sep 21, 2024

github-actions Bot commented Nov 12, 2024

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Nov 12, 2024
serhiy-storchaka mentioned this pull request Dec 18, 2024
Closed
savagemechanic mentioned this pull request May 17, 2026
Closed
serhiy-storchaka mentioned this pull request Jun 30, 2026
Merged

serhiy-storchaka commented Jul 5, 2026

Copy link
Copy Markdown
Member

Thank you for working on this, and sorry that it never got a core review at the time.

Argument quoting has been restored in GH-152703, and CR and LF in arguments are rejected since gh-143921, so I am closing this PR as superseded.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vadmium vadmium left review comments
+1 more reviewer
andreasg123 andreasg123 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL