Fix jsonable_encoder crash on invalid UTF‑8 bytes - #15641
SAURABHSALVE wants to merge 1 commit into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi! This PR is failing only due to the required label check. |
t0ugh-sys
left a comment
t0ugh-sys
left a comment
There was a problem hiding this comment.
Code Review: Fix jsonable_encoder crash on invalid UTF-8 bytes
Summary
This PR addresses a real issue — jsonable_encoder raising UnicodeDecodeError when encoding bytes values containing invalid UTF-8. The fix changes the bytes encoder from o.decode() to o.decode(errors="replace").
Observations
What's good:
- The bug is real and the fix is straightforward. Passing arbitrary binary payloads through
jsonable_encoderis a legitimate use case, and an unhandledUnicodeDecodeErroris surprising behavior. - A regression test is included, which is great. The test uses
b"\xff"(a common invalid UTF-8 byte) and asserts the replacement character\ufffdappears in the output.
Potential concerns and suggestions:
-
Lossy encoding is a design choice. Using
errors="replace"silently replaces invalid bytes with�(\ufffd). This is safe in the sense that it never crashes, but it also means the round-trip is lossy — you cannot reconstruct the original bytes from the encoded string. An alternative approach like base64 encoding (base64.b64encode(o).decode()) would be lossless, but that would be a larger behavioral change. The"replace"strategy is reasonable as a minimal fix, but it's worth being aware that this could mask data corruption in downstream usage. -
Documentation. It would be good to note this behavior somewhere (or at least in the changelog/release notes) so users know that invalid UTF-8 bytes are silently replaced rather than raising an error. Previously, a
UnicodeDecodeErrorat least signaled something was wrong. -
Test coverage. The test is minimal but adequate for the regression case. You might also consider testing a mixed string like
b"hello\xffworld"to verify that valid bytes surrounding invalid ones are preserved correctly (e.g.,"hello\ufffdworld"). -
No API surface change. The PR correctly notes this is not an API change — only the error handling behavior changes. This is a good, focused fix.
Verdict
This is a reasonable and minimal fix. The "replace" strategy is the least-invasive option and prevents unexpected crashes. LGTM with the minor suggestion to add one more test case for mixed valid/invalid byte sequences.
YuriiMotov
left a comment
YuriiMotov
left a comment
There was a problem hiding this comment.
Not sure it should be default behavior (it's lossy), but probably we can go this way.
Anyway, this behavior should be documented clearly.
|
|
||
| ENCODERS_BY_TYPE: dict[type[Any], Callable[[Any], Any]] = { | ||
| bytes: lambda o: o.decode(), | ||
| bytes: lambda o: o.decode(errors="replace"), |
There was a problem hiding this comment.
errors="replace" silently rewrites arbitrary bytes to U+FFFD, so any non-utf8 payload round-trips to garbage rather than erroring. was latin-1 or base64 considered? at least those keep the original bytes recoverable.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Late to this thread, but wanted to add one more angle to the replace vs. base64 discussion since it still seems open — just my two cents, happy to be told I'm missing something! Base64 has a cost of its own worth weighing. If bytes always got base64-encoded, that's a behavior change for the common case — something like content: bytes holding b"hello world" currently decodes to "hello world" today. Switching to always-base64 turns that into "aGVsbG8gd29ybGQ=" for anyone relying on current behavior — trading a rare crash for a much more common surprise. One option that might sidestep both the replace lossiness and the base64 format-change concern: decode as UTF-8 first (nothing changes for the common case), and only on failure raise a clear error instead of silently substituting anything: def _encode_bytes(o: bytes) -> str:
try:
return o.decode("utf-8")
except UnicodeDecodeError as e:
raise ValueError(
"Unable to serialize `bytes` value to JSON because it is "
"not valid UTF-8 text. JSON cannot represent raw binary "
"data. If this value is intended to be binary, encode it "
"yourself first, e.g. base64.b64encode(value).decode()."
) from eKeeps today's behavior identical for valid UTF-8, and turns the crash into something actionable instead of picking a lossy or format-changing default for the caller. Not attached to this at all if the team prefers replace — just wanted to put it on the table. Happy to open it as a separate PR if that's more useful than a comment here! |
|
hey guys, I am open to help with this issue, I think two last blocking bits are:
|
I think we should wait until Sebastian reviews this and decides the way to go |
|
Thanks for the feedback everyone. I agree that the lossy behavior of errors="replace" is worth considering carefully. I’m happy to adjust the implementation based on the maintainers’ preferred direction. I can also add the requested documentation and additional regression coverage once the behavior is decided. Thanks for reviewing! |
This branch has not been deployed
7 participants
jsonable_encoder currently decodes bytes using the default UTF‑8 settings, which can raise UnicodeDecodeError for arbitrary binary payloads.
This PR makes bytes encoding more robust by decoding with errors="replace", so encoding never crashes and instead produces a safe, JSON-serializable string.
Update bytes encoder to o.decode(errors="replace")
Add a regression test covering b"\xff" (invalid UTF‑8) to ensure it doesn’t raise and encodes to "\ufffd"
No API changes intended—this just prevents an unexpected exception during encoding.