-
Notifications
You must be signed in to change notification settings - Fork 570
fix: Cast message and detail attributes before appending exception notes #5114
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
Conversation
sentry_sdk/utils.py
Outdated
| message = str( | ||
| getattr(exc_value, "message", "") | ||
| or getattr(exc_value, "detail", "") | ||
| or safe_str(exc_value) |
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.
Bug: Applying str() to a bytes object within get_error_message() results in literal b'...' prefixes in Sentry error messages.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
If exc_value.message or exc_value.detail is a non-empty bytes object, the fix incorrectly wraps the entire or expression with str(). This converts the bytes object into its literal string representation (e.g., b"error" becomes "b'error'") instead of decoding it, resulting in malformed and unreadable error messages displayed in Sentry.
💡 Suggested Fix
Apply str() to each attribute individually before the or chain: str(getattr(exc_value, "message", "")) or str(getattr(exc_value, "detail", "")) or safe_str(exc_value).
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry_sdk/utils.py#L657-L660
Potential issue: If `exc_value.message` or `exc_value.detail` is a non-empty bytes
object, the fix incorrectly wraps the entire `or` expression with `str()`. This converts
the bytes object into its literal string representation (e.g., `b"error"` becomes
`"b'error'"`) instead of decoding it, resulting in malformed and unreadable error
messages displayed in Sentry.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2759871
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.
I guess we could special case for bytes to avoid the prefix, but I'm not sure if the bytes prefix is a bug.
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.
I think this is fine. But I'd use safe_str instead of pure str.
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.
Nice, changed!
Regarding Sentry's comment: previously, having bytes as the detail attribute always resulted in a TypeError as we try to append "\n" in all cases. Since the error was previously not captured, the case is not a breaking change.
sentry-python/sentry_sdk/utils.py
Line 669 in f89d77b
| message += "\n" + "\n".join(note for note in notes if isinstance(note, str)) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5114 +/- ##
==========================================
+ Coverage 84.00% 84.02% +0.01%
==========================================
Files 180 180
Lines 18030 18030
Branches 3209 3209
==========================================
+ Hits 15147 15149 +2
+ Misses 1906 1905 -1
+ Partials 977 976 -1
|
sentrivana
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.
LGTM, see comment.
sentry_sdk/utils.py
Outdated
| message = str( | ||
| getattr(exc_value, "message", "") | ||
| or getattr(exc_value, "detail", "") | ||
| or safe_str(exc_value) |
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.
I think this is fine. But I'd use safe_str instead of pure str.
Description
Cast message and detail attributes on exceptions to string when generating the value attribute on the Sentry exception.
Resolves an unhandled
TypeErrorwhen the message or detail attribute has type bytes andget_error_message()attempts to append string exception notes.Issues
Fixes the exception described in #5050.
Reminders
tox -e linters.feat:,fix:,ref:,meta:)