fix(logging): keep the record when JSON serialization fails (#1452) - #1491
Merged
Conversation
`_format_json` claimed `default=str` meant "a record is never lost to a
serialization error". It does not. `default` is consulted only for values
`json` cannot natively encode, and it is called unguarded, so two inputs
defeat it:
1. a circular container is rejected structurally, before `default` is
ever reached (`ValueError: Circular reference detected`);
2. a value whose `__str__` raises propagates straight out of `default`.
Either way `logging` swallows the raise via `Handler.handleError` and drops
the record entirely — the exact outcome the comment said was prevented.
Reproduced against main through a real handler: healthy → poisoned → healthy
emits 2 of 3 records for both inputs.
Reachable through the `correlation_id` / `performance_ms` enrichment loop
that #1439 added; the pre-#1439 template referenced neither field, so this
is a new failure mode rather than a pre-existing one. Not attacker-reachable
— every live call site passes a scalar, and an attacker-supplied header is a
string — so severity is low and the CWE-117 field-forgery fix is unaffected.
Wrap the dump and re-serialize with only the natively encodable fields, so a
bad enrichment costs its own value instead of the whole record, and record
why on the degraded record via `serialization_error`.
`except Exception` is deliberate, not a narrow tuple: the exploding-`__str__`
case walks straight through `(TypeError, ValueError, RecursionError)`.
The comment now states the guarantee the code actually provides.
Tests: +3 in the existing CWE-117 file. All three fail on the pre-fix
implementation and pass after it (3 failed, 20 passed → 23 passed), and one
of them pins that the degraded path still escapes attacker content, so the
fallback cannot become a hole in #1429.
Closes #1452
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LmZfJAVpuZqEzE5jc9Bmtw
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
This was referenced Aug 7, 2026
Closed
groupthinking
pushed a commit
that referenced
this pull request
Aug 7, 2026
#1491 closed #1452 by re-serializing from the scalar fields when `json.dumps` fails, so a bad enrichment costs its own value rather than the whole record. One residual path still costs the record. The exception the fallback catches is not `json`'s own error. In the exploding-`__str__` case it is *whatever that* `__str__` *raised*, which is arbitrary caller code. `f"{type(exc).__name__}: {exc}"` calls `str(exc)` on it, so an exception that also raises on `str()` fails inside the handler for the failure — losing the record for exactly the reason the fallback exists to prevent. Measured against main @ 8517bf8: 2 of 3 records reach the sink. `_describe_exception` falls back to the class name, an attribute lookup that runs no user code. Also covers `performance_ms`. The enrichment loop reads two attributes and #1491 tested only `correlation_id`, leaving half the reachable surface untested. That test passes on main — it closes a coverage gap rather than pinning a fix, and is marked as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013cGXYUfmU6yDSLZWe697eY
This was referenced Aug 7, 2026
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
9 tasks
This was referenced Aug 7, 2026
groupthinking
added a commit
that referenced
this pull request
Aug 13, 2026
#1515) #1491 wrapped the `json.dumps` call so a bad enrichment could not cost the whole record, and stated the guarantee as "never lose a record to a serialization error". The wrapper holds for the two inputs it was written against, but the *recovery* path it added can itself raise — so the guarantee covered the anticipated failures rather than the property. Measured against a real handler on 8517bf8, healthy -> poisoned -> healthy: | input | before | after | |------------------------------------|--------|-------| | circular container | 3/3 | 3/3 | | exploding `__str__` | 3/3 | 3/3 | | exception whose own `__str__` raises | 2/3 | 3/3 | | value forging `__class__ = str` | 2/3 | 3/3 | | int past the 4300-digit cap | 2/3 | 3/3 | | non-finite float | 3/3* | 3/3 | * emitted, but as a bare `NaN`/`Infinity` literal, which is not valid JSON — a strict downstream parser rejects the record, which is the same loss moved to the consumer. Each cause, and the fix: * The fallback built `f"{type(exc).__name__}: {exc}"` directly. The exception it catches may be one raised from a call site's own `__str__`, so describing the failure became the failure. Now `_describe_exception`, which falls back to the type name. * The scalar filter used `isinstance`, which consults `__class__` and can be forged with a property returning `str`. `json` dispatches on the real runtime type, so such a value passed the filter and then raised in the fallback's own dump. Now matched on exact runtime type. * `int` is a scalar by every type test, but `json` renders ints via `str` and CPython caps that at `sys.get_int_max_str_digits()` (4300). Now bounded by `bit_length`, which avoids performing the conversion being guarded against. * `allow_nan=False`, so a non-finite float routes to the fallback and the record stays valid JSON instead of carrying a JavaScript literal. A final constant-record tier keeps the guarantee a property of the code rather than of the failure modes anticipated here — the exact gap #1452 was about. It is not reachable through any input above, and the comment says so. Tests: +10 in the existing CWE-117 file. All 10 fail against 8517bf8 and pass after (7 failed / 26 passed -> 33 passed); the pre-existing 26 are unchanged, so this does not weaken what #1491 established. Full `tests/unit` is unchanged at 1637 failed / 76 errors (missing optional deps in this environment) with +10 passed, i.e. no regressions. ruff clean; mypy unchanged at its 17 pre-existing errors in this module. Refs #1452 Claude-Session: https://claude.ai/code/session_01DHLdfqAJcfL9LPWC7Dp9Gx Co-authored-by: Claude <noreply@anthropic.com>
groupthinking
pushed a commit
that referenced
this pull request
Aug 13, 2026
#1491 wrapped the `json.dumps` call so a bad enrichment could not cost the whole record, and stated the guarantee as "never lose a record to a serialization error". The wrapper holds for the two inputs it was written against, but the *recovery* path it added can itself raise — so the guarantee covered the anticipated failures rather than the property. Measured against a real handler on 8517bf8, healthy -> poisoned -> healthy: | input | before | after | |------------------------------------|--------|-------| | circular container | 3/3 | 3/3 | | exploding `__str__` | 3/3 | 3/3 | | exception whose own `__str__` raises | 2/3 | 3/3 | | value forging `__class__ = str` | 2/3 | 3/3 | | int past the 4300-digit cap | 2/3 | 3/3 | | non-finite float | 3/3* | 3/3 | * emitted, but as a bare `NaN`/`Infinity` literal, which is not valid JSON — a strict downstream parser rejects the record, which is the same loss moved to the consumer. Each cause, and the fix: * The fallback built `f"{type(exc).__name__}: {exc}"` directly. The exception it catches may be one raised from a call site's own `__str__`, so describing the failure became the failure. Now `_describe_exception`, which falls back to the type name. * The scalar filter used `isinstance`, which consults `__class__` and can be forged with a property returning `str`. `json` dispatches on the real runtime type, so such a value passed the filter and then raised in the fallback's own dump. Now matched on exact runtime type. * `int` is a scalar by every type test, but `json` renders ints via `str` and CPython caps that at `sys.get_int_max_str_digits()` (4300). Now bounded by `bit_length`, which avoids performing the conversion being guarded against. * `allow_nan=False`, so a non-finite float routes to the fallback and the record stays valid JSON instead of carrying a JavaScript literal. A final constant-record tier keeps the guarantee a property of the code rather than of the failure modes anticipated here — the exact gap #1452 was about. It is not reachable through any input above, and the comment says so. Tests: +10 in the existing CWE-117 file. All 10 fail against 8517bf8 and pass after (7 failed / 26 passed -> 33 passed); the pre-existing 26 are unchanged, so this does not weaken what #1491 established. Full `tests/unit` is unchanged at 1637 failed / 76 errors (missing optional deps in this environment) with +10 passed, i.e. no regressions. ruff clean; mypy unchanged at its 17 pre-existing errors in this module. Refs #1452 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHLdfqAJcfL9LPWC7Dp9Gx
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_format_jsonclaimeddefault=strmeant "a record is never lost to a serialization error". It does not.defaultis consulted only for valuesjsoncannot natively encode, and it is called unguarded, so two inputs defeat it:defaultis ever reached (ValueError: Circular reference detected);__str__raises propagates straight out ofdefault.Either way
loggingswallows the raise viaHandler.handleErrorand drops the record entirely — the exact outcome the comment said was prevented. Reproduced against main through a real handler: healthy → poisoned → healthy emits 2 of 3 records for both inputs.Reachable through the
correlation_id/performance_msenrichment loop that #1439 added; the pre-#1439 template referenced neither field, so this is a new failure mode rather than a pre-existing one. Not attacker-reachable — every live call site passes a scalar, and an attacker-supplied header is a string — so severity is low and the CWE-117 field-forgery fix is unaffected.Wrap the dump and re-serialize with only the natively encodable fields, so a bad enrichment costs its own value instead of the whole record, and record why on the degraded record via
serialization_error.except Exceptionis deliberate, not a narrow tuple: the exploding-__str__case walks straight through(TypeError, ValueError, RecursionError).The comment now states the guarantee the code actually provides.
Tests: +3 in the existing CWE-117 file. All three fail on the pre-fix implementation and pass after it (3 failed, 20 passed → 23 passed), and one of them pins that the degraded path still escapes attacker content, so the fallback cannot become a hole in #1429.
Closes #1452
Claude-Session: https://claude.ai/code/session_01LmZfJAVpuZqEzE5jc9Bmtw
Canonical issue
Closes #
Outcome
Describe the user or operational result this PR produces.
Scope
Risk
Verification
List exact automated and manual checks, tied to the current head SHA.
Production evidence
Provide the Vercel preview, production deployment, runtime evidence, or state why production evidence is not applicable.
Agent handoff