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

Jump to bottom

bpo-38332: catch KeyError from unknown cte in encoded-word #16503

Merged
merged 2 commits into from Oct 5, 2019
Merged

bpo-38332: catch KeyError from unknown cte in encoded-word #16503

merged 2 commits into from Oct 5, 2019

Conversation

aft90
Copy link
Contributor

@aft90 aft90 commented Sep 30, 2019 •
edited by miss-islington

Given an encoded-word with an invalid content-transfer-encoding (something other than b or q, for instance: =?utf-8?X?somevalue?=), _encoded_words.decode() throws a KeyError which propagates all the way down to email.message.get().

Fix is to explicitly catch the KeyError in get_encoded_word and raise InvalidEwError which then causes get_unstructured to return the value unchanged.

If accepted, this should be backported to 3.5 through to 3.8

https://bugs.python.org/issue38332

Automerge-Triggered-By: @maxking

@aft90 aft90 requested a review from a team as a code owner Sep 30, 2019
@eamanu
Copy link
Contributor

eamanu commented Sep 30, 2019

Hi @aft90, thanks for the PR. Please open a BPO (https://bugs.python.org) to https://bugs.python.org

@aft90
Copy link
Contributor Author

aft90 commented Sep 30, 2019

I have: https://bugs.python.org/issue38332. BPO seems to be unresponsive at the moment, so I'll update it when it's back up.

eamanu
eamanu reviewed Oct 1, 2019
View changes
Copy link
Contributor

@eamanu eamanu left a comment

(copy here my bpo comments)

I am not a email expert, but according to RFC 1342 the enconding can be either "B" or "Q". So, I think is reasonable that when a not correct enconding is set, should be raise an exception

I think that we can improve the message raising a more specific Exception

eamanu
eamanu approved these changes Oct 2, 2019
View changes
Copy link
Contributor

@eamanu eamanu left a comment

LGTM. I test it and work successfully. IMO the NEW is not necessary. Good catch @aft90

@maxking
Copy link
Contributor

maxking commented Oct 5, 2019

I think the changes look good, although, it would be good to have a NEWS item too. This seems

It is good to have a changelog for bugfixes in case we need to refer to it later.

@maxking maxking added needs backport to 3.7 needs backport to 3.8 type-bug An unexpected behavior, bug, or error labels Oct 5, 2019
@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Oct 5, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@andreitroiebbc

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@aft90 aft90 force-pushed the encoded_word_invalid_cte branch from d5e55da to 1c17f54 Compare Oct 5, 2019
maxking
maxking approved these changes Oct 5, 2019
View changes
Copy link
Contributor

@maxking maxking left a comment

Thanks @aft90 for working on this.

It looks good to me!

@maxking maxking added the 🤖 automerge PR will be merged once it's been approved and all CI passed label Oct 5, 2019
@maxking
Copy link
Contributor

maxking commented Oct 5, 2019

Hmm, not sure why it wasn't merged automatically since it seems to have the right labels. Merging manually!

@maxking maxking merged commit 65dcc8a into python:master Oct 5, 2019
@miss-islington
Copy link
Contributor

miss-islington commented Oct 5, 2019

Thanks @aft90 for the PR, and @maxking for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Oct 5, 2019

GH-16596 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 5, 2019
@aft90 @miss-islington
bpo-38332: Catch KeyError from unknown cte in encoded-word. (pythonGH…
61cc90d 
…-16503)

KeyError should cause a failure in parsing the encoded word and should be caught and raised as a _InvalidEWError instead.
(cherry picked from commit 65dcc8a)

Co-authored-by: Andrei Troie <andreitroie90@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented Oct 5, 2019

GH-16597 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request Oct 12, 2019
@miss-islington @aft90
bpo-38332: Catch KeyError from unknown cte in encoded-word. (GH-16503)
febe359 
KeyError should cause a failure in parsing the encoded word and should be caught and raised as a _InvalidEWError instead.
(cherry picked from commit 65dcc8a)

Co-authored-by: Andrei Troie <andreitroie90@gmail.com>
miss-islington added a commit that referenced this pull request Oct 12, 2019
@miss-islington @aft90
bpo-38332: Catch KeyError from unknown cte in encoded-word. (GH-16503)
e540bb5 
KeyError should cause a failure in parsing the encoded word and should be caught and raised as a _InvalidEWError instead.
(cherry picked from commit 65dcc8a)

Co-authored-by: Andrei Troie <andreitroie90@gmail.com>
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@aft90
bpo-38332: Catch KeyError from unknown cte in encoded-word. (pythonGH…
f6ca80a 
…-16503)

KeyError should cause a failure in parsing the encoded word and should be caught and raised as a _InvalidEWError instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxking maxking

@eamanu eamanu

Assignees
No one assigned
Labels
🤖 automerge PR will be merged once it's been approved and all CI passed type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aft90 @eamanu @maxking @the-knights-who-say-ni @miss-islington @bedevere-bot