fix(serializers): bound forged-entry error echoes; retire columnar dead code (LAB-3131) - #289
Merged
27Bslash6 merged 2 commits intoSep 14, 2026
Conversation
added 2 commits
September 13, 2026 22:56
…ad code (LAB-3131) Three trust-boundary hygiene fixes on the DataFrame/Series columnar decode path, all failing closed already — no change to what is accepted or rejected, and wire bytes are unchanged (byte-verified against the interop fixtures). 1. Bound untrusted error echoes at the read-path wrap sites. #276 capped the per-field marker/column-name echoes, but _dtype_from_untrusted still echoed the full forged dtype (numpy's own message + str(dtype)), so a poisoned entry drove log volume with payload size (measured pre-#276: a 1 MiB column name produced a 4.19 MB log line). Add base.bounded_error() and apply it once at each site that logs or re-wraps an untrusted-decode failure (cache_handler.handle_decrypt_failure, deserialize_data, _deserialize_interop; decorators/wrapper L1 guards) — a global O(1) bound, not per field. 2. Retire the dead bytes preamble in _deserialize_dataframe/_deserialize_series. Every production caller decodes under unpackb_bounded first, so the decoders now take a decoded document only; the "document is <type>, expected dict" shape gate is now reachable in production and covered. 3. Fold the writer trio into one _column_trio(series) beside its _column_values decoder mirror, so a third marker cannot be added to one side only. Tests: bounded-log assertion with a 1 MiB column name + 4 KB forged dtype; forged non-dict body cases; four direct-call tests updated to the decoded-doc contract. Full unit suite + interop wire vectors green.
- bounded_error now escapes every C0/C1 control char, DEL and U+2028/U+2029,
not just \n/\r — the helper enforces its own "one terminal-safe line"
invariant instead of relying on each caller's text being pre-escaped
(panel MAJ: ANSI/other separators could still split a log line).
- _column_trio docstring: state the object marker is 2 keys ({type, data}),
numeric is 3 ({type, data, dtype}); "trio" names the maximal numeric form.
- Tests: assert control-char neutralisation; tighten the bounded-log asserts
to ERROR_ECHO_MAX; add an end-to-end L2-read test (get_cached_value ->
_handle_l2_read_error -> handle_decrypt_failure) so a future unbounded log
added ahead of the bounded site is caught.
Rejected (stated for the record): bound-at-construction instead of at the wrap
sites — contradicts the ticket's explicit "global bound at the {e} wrap sites,
not per field" design; the re-raised messages are already born bounded as
belt-and-suspenders.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughThe change bounds exception text in cache and wrapper logs, centralises columnar marker encoding, and requires decoded documents for DataFrame and Series deserialisation. Tests cover corrupted payloads, safe error output, and updated decoding paths. The secret baseline metadata is refreshed. ChangesSerialization hardening
Secret baseline maintenance
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The bounded error handling remains within its documented contract, with no actionable merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
27Bslash6
deleted the
lab-3131-bound-forged-echoes-retire-columnar-preamble
branch
September 14, 2026 06:55
27Bslash6
pushed a commit
that referenced
this pull request
Sep 14, 2026
Resolves conflicts against main@284fa7e: - cache_handler.py / wrapper.py: main (#289) wraps echoed exception text in bounded_error() at the read-path log and re-raise sites; this branch renders exceptions at every log sink via redact_error_for_log(), which emits no exception text at all. Log lines keep redact_error_for_log (strictly tighter than the bound, and it also redacts the key); the two SerializationError re-raise sites take main's bounded_error(), which the branch had left as raw {e}. wrapper.py no longer needs the bounded_error import; bounded_error's docstring updated to match. - pyproject.toml: both sides bumped pip>=26.2; main's file is the superset. - .secrets.baseline: generated; took main's side, hook regenerated it. Merge (not rebase) so history stays append-only.
27Bslash6
pushed a commit
that referenced
this pull request
Sep 14, 2026
Resolve the LAB-3131 (#289) overlap in the columnar decode path: - src/cachekit/serializers/auto_serializer.py: both sides retired the dead isinstance(data, dict) preamble in _deserialize_dataframe/_deserialize_series; keep main's parameter naming and docstrings. The PR's deserialize() collapse and EnvelopeIntegrityError handling are unchanged. - tests/unit/test_auto_serializer_new_types.py: both sides adapted the four TestColumnarFallbackExtensionDtypes tests to the decoded-document contract; keep the PR's public serialize()/deserialize() round-trip, which routes through main's _decode_columnar.
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
1 participant
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.
Summary
This PR addresses a log-injection/denial-of-service vulnerability (LAB-3131) where forged cache entries could produce unbounded error log output, and removes dead code from the columnar serialization paths.
Problem
When a poisoned or corrupted cache entry is read, the resulting exception text is influenced by attacker-controlled bytes. Prior mitigations (#276) capped some per-field echoes (marker, column name), but
_dtype_from_untrustedstill echoed the full forged dtype string. As a result, a forged entry carrying a large dtype (or column name folded up through nested{e}wraps) could inflate a single log line to megabytes — measured at up to a 4.19 MB log line (505x amplification) from an 8.3 KB envelope. Attacker-controlled text could also inject newlines or ANSI/terminal control sequences into log records.Changes
New
bounded_error()helper (serializers/base.py)ERROR_ECHO_MAX(512 chars) and abounded_error()function that:U+2028/U+2029separators) so a poisoned read always produces exactly one terminal-safe log line.Applied at trust-boundary wrap sites
cache_handler.py: decrypt/integrity failure warnings, authentication failure errors, and deserialization/interop error messages now route exception text throughbounded_error().decorators/wrapper.py: L1 deserialization failure warnings (both sync and async paths) now usebounded_error().Retire columnar dead code (
auto_serializer.py)_column_trio()as a single shared writer for column/Series markers, used by both_serialize_dataframeand_serialize_series, keeping the marker set and key order in one place (on-wire byte compatibility preserved)._deserialize_dataframeand_deserialize_seriesnow accept an already-decoded document instead of raw bytes, removing the dead bytes-preamble branch that duplicated msgpack decoding. This makes the_expectshape gate reachable in production, so a forged non-dict body is properly refused.Tests
TestForgedEntryErrorEchoIsBoundedverifying clipping, control-char neutralization, and bounded log lines end-to-end (throughhandle_decrypt_failureand a full L2 read path)._decode_columnar(the new decoded-document contract).