Repository navigation
gh-145824: email.utils: replace AttributeError with TypeError in collapse_rfc2231_value for non-string non-3-tuple input - #145825
Conversation
Passing a tuple of length != 3 fell through to unquote(value) which calls .startswith() on the tuple, raising AttributeError. Handle non-3-tuples explicitly before calling unquote.
…il_utils_collapse_non3_tuple
picnixz
left a comment
There was a problem hiding this comment.
I don't think we should do anything here. First, it's not a crash but rather a GIGO case (garbage in, garbage out). The documentation says:
For convenience, if the value passed to collapse_rfc2231_value() is not a tuple, it should be a string and it is returned unquoted.
So returning the stringification of the tuple may not necessarily be correct. Instead, a TypeError should be raised if AttributeError is too cryptic IMO.
|
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 |
Revised per reviewer feedback: instead of silently handling non-3-tuples, raise TypeError with a clear message. Updated docs to say 'not a 3-tuple' instead of 'not a tuple' to match the code's actual check.
…com/stefanzetzsche/cpython into fix/email_utils_collapse_non3_tuple
collapse_rfc2231_value crashes on non-3-tuplesAttributeError with TypeError in collapse_rfc2231_value for non-string non-3-tuple input
|
Thanks @picnixz, I have made the requested changes; please review again. See also #145824 (comment). |
…dant success test
|
This PR is stale because it has been open for 30 days with no activity. |
| def test_collapse_rfc2231_value_non_3_tuple(self): | ||
| for val in [(), ('a',), ('a', 'b'), ('a', 'b', 'c', 'd'), 42, None]: | ||
| with self.subTest(val=val): | ||
| with self.assertRaises(TypeError): | ||
| email.utils.collapse_rfc2231_value(val) |
There was a problem hiding this comment.
| def test_collapse_rfc2231_value_non_3_tuple(self): | |
| for val in [(), ('a',), ('a', 'b'), ('a', 'b', 'c', 'd'), 42, None]: | |
| with self.subTest(val=val): | |
| with self.assertRaises(TypeError): | |
| email.utils.collapse_rfc2231_value(val) | |
| @support.subTests("value", [(), ('a',), ('a', 'b'), ('a', 'b', 'c', 'd'), 42, None]) | |
| def test_collapse_rfc2231_value_non_3_tuple(self, value): | |
| self.assertRaises(TypeError, email.utils.collapse_rfc2231_value, value) |
and add from test import support at the top of the file if necessary. If subTests is already imported, use it as is.
picnixz
left a comment
There was a problem hiding this comment.
Overall LGTM except for the small test change.
|
This PR is stale because it has been open for 90 days with no activity. |
Issue
collapse_rfc2231_valuefalls through tounquote(value)for tuples whose length is not 3, butunquoteexpects a string, raisingAttributeError.Reproducer
Fix
Lib/email/utils.py, raiseTypeErrorfor values that are neither a string nor a 3-tuple:Doc/library/email.utils.rst, change "not a tuple" to "not a 3-tuple" to match the code's actual check.Tests
test_collapse_rfc2231_value_non_3_tupleinTestRFC2231(Lib/test/test_email/test_email.py) verifies that:TypeError('us-ascii', 'en', 'hello')decodes correctly'"hello"'passes throughunquote