Skip to content

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

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.

Sign up for GitHub

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
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
Loading

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 September 30, 2019 22:03
@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 reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

(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 reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

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 type-bug An unexpected behavior, bug, or error labels Oct 5, 2019
@the-knights-who-say-ni
Copy link

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!

maxking
maxking approved these changes Oct 5, 2019
View reviewed changes
Copy link
Contributor

@maxking maxking left a comment

Choose a reason for hiding this comment

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

Thanks @aft90 for working on this.

It looks good to me!

@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

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

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 <[email protected]>
@bedevere-bot
Copy link

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 <[email protected]>
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 <[email protected]>
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.
@aft90 aft90 deleted the encoded_word_invalid_cte branch July 25, 2024 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxking maxking maxking approved these changes

@eamanu eamanu eamanu approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aft90 @eamanu @maxking @the-knights-who-say-ni @miss-islington @bedevere-bot