GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) by tdwyer · Pull Request #102990 · python/cpython

Conversation

@tdwyer

@tdwyer tdwyer commented Mar 24, 2023

Copy link
Copy Markdown
Contributor

Pull Request title

gh-102988: This PR is designed to detect parsing errors and return an empty tuple to indicate the parsing error. Additionally, this PR updates the test_email.py to check for these bugs, as well as, adds some other wacky Address Headers that are in the examples of RFC 2822 and makes sure they are being parsed correctly.

I realize that this PR dose not actually track down the bug and fix it. It simply detects the error has happened and returns a parsing error. However, Lib/email/utils.py is a much simple file than Lib/email/_parseaddr.py, so it is much easier to review this change. Additionally, there are actually multiple bugs which are causing erroneous output. Tracing the code flow for each and fixing them would be prone to error considering all of the wacky stuff that RFC 2822 allows for in Address headers. Finally, this change is actually rather simple.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tdwyer
Detect parsing errors and return empty tuple to indicate the parsing …
d45802b 
…error
@tdwyer
tdwyer requested a review from a team as a code owner March 24, 2023 04:32
@bedevere-bot

bedevere-bot commented Mar 24, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting review label Mar 24, 2023
@ghost

ghost commented Mar 24, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@arhadthedev arhadthedev changed the title Detect parsing errors and return empty tuple to indicate the parsing error gh-102988: Detect parsing errors and return empty tuple to indicate the parsing error Mar 24, 2023
@arhadthedev arhadthedev added stdlib Standard Library Python modules in the Lib/ directory topic-email labels Mar 24, 2023
@bitdancer bitdancer changed the title gh-102988: Detect parsing errors and return empty tuple to indicate the parsing error gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) Mar 24, 2023
@tdwyer
Appened empty tuple for any tuple removed by def _post_parse_validati…
2ec41f1 
…on()
@bedevere-bot

bedevere-bot commented Mar 24, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tdwyer tdwyer mentioned this pull request Mar 24, 2023
[CVE-2023-27043] Parsing errors in email/_parseaddr.py lead to incorrect value in email address part of tuple #102988
Closed
@gpshead gpshead added the type-security A security issue label Mar 27, 2023
gpshead
gpshead requested changes Apr 28, 2023
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

thanks for the PR!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/email/utils.py
for v in fieldvalues:
s = str(v).replace('\\(', '').replace('\\)', '')
if s.count('(') != s.count(')'):
fieldvalues.remove(v)

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this loop is modifying the apparent list it is iterating over within the loop. that makes reasoning about its exact behavior hard. removing an item could mean you wind up skipping an item, appending an item could make the loop iterate over that. furthermore remove is O(n)... you're re-finding the item to remove to remove it. Also, this code modifys the passed in list in place before returning it.

A better code pattern for this is to build up a new list and return that. never modifying the input.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Something like:

accepted_values = []
for v in fieldvalues:
    s = v.replace('\\(', '').replace('\\)', '')
    if s.count('(') != s.count(')'):
        v = ('', '')
    accepted_values.append(v)

return accepted_values

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CharlieZhao95 and tdwyer reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Lib/email/utils.py
"""Validate the parsed values are syntactically correct"""
for v in parsedvalues:
if '[' in v[1]:
parsedvalues.remove(v)

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

same design comment as above, don't modify the list being iterated over and don't modify the argument in place.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py

n = 0
for v in fieldvalues:
n += str(v).count(',') + 1

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

get rid of this str(v)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py
def getaddresses(fieldvalues):
"""Return a list of (REALNAME, EMAIL) for each fieldvalue."""
fieldvalues = _pre_parse_validation(fieldvalues)
all = COMMASPACE.join(str(v) for v in fieldvalues)

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The existing code already calls str(v) here... this is a problem. It means the input can be anything. But we probably cannot change it in a patch release for a security fix.

To avoid propagating this mistake into more lines of code spread out, I suggest changing this function to do:

fieldvalues = [str(v) for v in fieldvalues]

on the first line and get rid of all subsequent str(v) calls on anything from fieldvalues in this function or in functions it calls.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py


def getaddresses(fieldvalues):
"""Return a list of (REALNAME, EMAIL) for each fieldvalue."""

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

add documentation mentioning that fieldvalues that could not be parsed may cause a ('', '') item to be returned in their place.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py
def _pre_parse_validation(fieldvalues):
"""Validate the field values are syntactically correct"""
for v in fieldvalues:
s = str(v).replace('\\(', '').replace('\\)', '')

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

See later comments on str(v) being bad, we can get rid of it here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py
return address


def _pre_parse_validation(fieldvalues):

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

the name "fieldvalues" is non-specific. i realize it comes from the name in getaddress() but we should make it more clear what these are. email_address_fields perhaps?

Related: I don't this docstring adds meaningful value. naming the function and parameter right along with it being short code is sufficiently self explanatory for this internal function. get rid of the docstring.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py
def _post_parse_validation(parsedvalues):
"""Validate the parsed values are syntactically correct"""
for v in parsedvalues:
if '[' in v[1]:

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Add a comment explaining why only [ is not allowed. ideally with a link to the relevant RFC or similar.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/utils.py
return fieldvalues


def _post_parse_validation(parsedvalues):

@gpshead gpshead Apr 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

parsed_email_address_tuples perhaps?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Apr 28, 2023
@bedevere-bot

bedevere-bot commented Apr 28, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
tdwyer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead self-assigned this Apr 28, 2023
@gpshead
gpshead requested a review from bitdancer April 28, 2023 21:53
theta682
theta682 reviewed May 4, 2023
View reviewed changes
Comment thread Lib/test/test_email/test_email.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
CharlieZhao95
CharlieZhao95 reviewed May 6, 2023
View reviewed changes
Comment thread Lib/email/utils.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented May 27, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@bedevere-bot

bedevere-bot commented May 27, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
theta682
theta682 reviewed May 28, 2023
View reviewed changes

@theta682 theta682 left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

stylistic suggestions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.getaddresses(['[email protected](<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected])<[email protected]>']),
[('' ,'')])

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
[('' ,'')])
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.getaddresses(['[email protected])<[email protected]>']),
[('' ,'')])
eq(utils.getaddresses(['[email protected]<<[email protected]>']),
[('' ,'')])

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
[('' ,'')])
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.getaddresses(['[email protected]<<[email protected]>']),
[('' ,'')])
eq(utils.getaddresses(['[email protected]><[email protected]>']),
[('' ,'')])

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
[('' ,'')])
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.getaddresses(['[email protected]><[email protected]>']),
[('' ,'')])
eq(utils.getaddresses(['[email protected]@<[email protected]>']),
[('' ,'')])

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
[('' ,'')])
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.getaddresses(['[email protected],<[email protected]>']),
[('', '[email protected]'), ('', '[email protected]')])
eq(utils.getaddresses(['[email protected];<[email protected]>']),
[('' ,'')])

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
[('' ,'')])
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.parseaddr(['[email protected];<[email protected]>']),
('' ,''))
eq(utils.parseaddr(['[email protected]:<[email protected]>']),
('' ,''))

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
('' ,''))
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.parseaddr(['[email protected]:<[email protected]>']),
('' ,''))
eq(utils.parseaddr(['[email protected].<[email protected]>']),
('' ,''))

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
('' ,''))
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.parseaddr(['[email protected].<[email protected]>']),
('' ,''))
eq(utils.parseaddr(['[email protected]"<[email protected]>']),
('' ,''))

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
('' ,''))
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.parseaddr(['[email protected]"<[email protected]>']),
('' ,''))
eq(utils.parseaddr(['[email protected][<[email protected]>']),
('' ,''))

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
('' ,''))
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_email/test_email.py
eq(utils.parseaddr(['[email protected][<[email protected]>']),
('' ,''))
eq(utils.parseaddr(['[email protected]]<[email protected]>']),
('' ,''))

@theta682 theta682 May 28, 2023

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
('' ,''))
[('', '')])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tdwyer
tdwyer requested review from a team, Fidget-Spinner, barneygale, corona10, ethanfurman and gvanrossum as code owners May 31, 2023 01:01
@tdwyer
tdwyer requested review from asvetlov, ezio-melotti, hugovk, kumaraditya303 and willingc as code owners May 31, 2023 01:01
@CharlieZhao95

CharlieZhao95 commented May 31, 2023

Copy link
Copy Markdown
Contributor

You could use git merge, please see: https://devguide.python.org/getting-started/git-boot-camp/#updating-your-cpython-fork

Core developers can help to remove the redundant requests for review. Before that you may need to force push the correct commits, or close this PR and start a new one. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented May 31, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tdwyer

tdwyer commented May 31, 2023

Copy link
Copy Markdown
Contributor Author

I'm going to have to make a new PR... Git is all messed up now...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tdwyer tdwyer closed this by deleting the head repository May 31, 2023
@tdwyer

tdwyer commented May 31, 2023

Copy link
Copy Markdown
Contributor Author

@gpshead @CharlieZhao95 @theta682 I had to create a new PR because I messed up Git when trying to push the changes #105127
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tdwyer tdwyer mentioned this pull request May 31, 2023
gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) #105127
Merged
@hugovk
hugovk removed request for a team, barneygale, bitdancer, corona10, ericsnowcurrently, erlend-aasland, ethanfurman, gvanrossum, hugovk, iritkatriel, lysnikolaou, markshannon, pablogsal, pganssle, rhettinger, terryjreedy and willingc May 31, 2023 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead requested changes
+2 more reviewers
@theta682 theta682 theta682 left review comments
@CharlieZhao95 CharlieZhao95 CharlieZhao95 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

awaiting changes stdlib Standard Library Python modules in the Lib/ directory topic-email type-security A security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@tdwyer @bedevere-bot @CharlieZhao95 @gpshead @theta682 @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.