Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API)#102990
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedMar 24, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
ghost commentedMar 24, 2023 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
bedevere-bot commentedMar 24, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
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!
| for v in fieldvalues: | ||
| s = str(v).replace('\\(', '').replace('\\)', '') | ||
| if s.count('(') != s.count(')'): | ||
| fieldvalues.remove(v) |
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.
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= []forvinfieldvalues:s=v.replace('\\(','').replace('\\)','')ifs.count('(')!=s.count(')'):v= ('','')accepted_values.append(v)returnaccepted_values
| """Validate the parsed values are syntactically correct""" | ||
| for v in parsedvalues: | ||
| if '[' in v[1]: | ||
| parsedvalues.remove(v) |
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.
| n = 0 | ||
| for v in fieldvalues: | ||
| n += str(v).count(',') + 1 |
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)
| 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) |
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 callsstr(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)forvinfieldvalues]
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.
| parsedvalues.append(('', '')) | ||
| return parsedvalues | ||
| def getaddresses(fieldvalues): | ||
| """Return a list of (REALNAME, EMAIL) for each fieldvalue.""" |
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.
| def _pre_parse_validation(fieldvalues): | ||
| """Validate the field values are syntactically correct""" | ||
| for v in fieldvalues: | ||
| s = str(v).replace('\\(', '').replace('\\)', '') |
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 onstr(v) being bad, we can get rid of it here.
| @@ -106,12 +106,42 @@ def formataddr(pair, charset='utf-8'): | |||
| return address | |||
| def _pre_parse_validation(fieldvalues): | |||
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.
| def _post_parse_validation(parsedvalues): | ||
| """Validate the parsed values are syntactically correct""" | ||
| for v in parsedvalues: | ||
| if '[' in v[1]: |
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.
| return fieldvalues | ||
| def _post_parse_validation(parsedvalues): |
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?
bedevere-bot commentedApr 28, 2023
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 |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedMay 27, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
1 similar comment
bedevere-bot commentedMay 27, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
theta682 left a comment
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
| eq(utils.getaddresses(['alice@example.org(<bob@example.com>']), | ||
| [('', '')]) | ||
| eq(utils.getaddresses(['alice@example.org)<bob@example.com>']), | ||
| [('' ,'')]) |
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.
| [('' ,'')]) | |
| [('','')]) |
| eq(utils.getaddresses(['alice@example.org)<bob@example.com>']), | ||
| [('' ,'')]) | ||
| eq(utils.getaddresses(['alice@example.org<<bob@example.com>']), | ||
| [('' ,'')]) |
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.
| [('' ,'')]) | |
| [('','')]) |
| eq(utils.getaddresses(['alice@example.org<<bob@example.com>']), | ||
| [('' ,'')]) | ||
| eq(utils.getaddresses(['alice@example.org><bob@example.com>']), | ||
| [('' ,'')]) |
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.
| [('' ,'')]) | |
| [('','')]) |
| eq(utils.getaddresses(['alice@example.org><bob@example.com>']), | ||
| [('' ,'')]) | ||
| eq(utils.getaddresses(['alice@example.org@<bob@example.com>']), | ||
| [('' ,'')]) |
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.
| [('' ,'')]) | |
| [('','')]) |
| eq(utils.getaddresses(['alice@example.org,<bob@example.com>']), | ||
| [('', 'alice@example.org'), ('', 'bob@example.com')]) | ||
| eq(utils.getaddresses(['alice@example.org;<bob@example.com>']), | ||
| [('' ,'')]) |
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.
| [('' ,'')]) | |
| [('','')]) |
| eq(utils.parseaddr(['alice@example.org;<bob@example.com>']), | ||
| ('' ,'')) | ||
| eq(utils.parseaddr(['alice@example.org:<bob@example.com>']), | ||
| ('' ,'')) |
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.
| ('' ,'')) | |
| [('','')]) |
| eq(utils.parseaddr(['alice@example.org:<bob@example.com>']), | ||
| ('' ,'')) | ||
| eq(utils.parseaddr(['alice@example.org.<bob@example.com>']), | ||
| ('' ,'')) |
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.
| ('' ,'')) | |
| [('','')]) |
| eq(utils.parseaddr(['alice@example.org.<bob@example.com>']), | ||
| ('' ,'')) | ||
| eq(utils.parseaddr(['alice@example.org"<bob@example.com>']), | ||
| ('' ,'')) |
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.
| ('' ,'')) | |
| [('','')]) |
| eq(utils.parseaddr(['alice@example.org"<bob@example.com>']), | ||
| ('' ,'')) | ||
| eq(utils.parseaddr(['alice@example.org[<bob@example.com>']), | ||
| ('' ,'')) |
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.
| ('' ,'')) | |
| [('','')]) |
| eq(utils.parseaddr(['alice@example.org[<bob@example.com>']), | ||
| ('' ,'')) | ||
| eq(utils.parseaddr(['alice@example.org]<bob@example.com>']), | ||
| ('' ,'')) |
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.
| ('' ,'')) | |
| [('','')]) |
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.pyto 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.pyis a much simple file thanLib/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.