Sanitize NUL bytes in operation and effect details before jsonb insert - #216
Open
tamirms wants to merge 5 commits into
Open
Sanitize NUL bytes in operation and effect details before jsonb insert#216tamirms wants to merge 5 commits into
tamirms wants to merge 5 commits into
Conversation
Strings derived from ledger data are not guaranteed to be representable in a Postgres jsonb column. Strip any NUL from the marshaled operation and effect details at the insert boundary so the write cannot fail on such values. Includes a unit test and DB-backed regression tests for both the history_operations and history_effects details columns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Sanitizes NUL bytes before storing operation and effect details in PostgreSQL jsonb.
Changes:
- Adds a JSONB details sanitizer.
- Applies sanitization to operation and effect batch inserts.
- Adds unit and database-backed regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
operation_batch_insert_builder.go |
Sanitizes operation details. |
effect_batch_insert_builder.go |
Sanitizes effect details. |
details.go |
Implements NUL escape removal. |
details_test.go |
Tests sanitizer and database writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback: - sanitizeJSONBDetails could match the six escape bytes when they appear as the tail of an escaped backslash, truncating an otherwise-valid document into invalid JSON. Walk the marshaled bytes and drop only escapes introduced by an unescaped backslash. Adds regression cases. - Use Require for query prerequisites before indexing the effects slice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the hand-rolled escape scan with a decode / strip-NUL / re-encode round trip, so JSON parsing and escaping are handled by encoding/json rather than by editing serialized bytes. UseNumber preserves numeric precision across the round trip. The no-NUL fast path is unchanged, so the common ingestion case pays nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The shadow vet in CI's check job flagged an err declaration in an inner Unmarshal block shadowing the function-scope err from a Marshal call. Use a small mustMarshal helper so each Unmarshal keeps its own scoped err with nothing to shadow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two DB-backed regression tests each provisioned their own database fixture, adding load to the history package which already runs close to the 10m per-package race timeout on the slower CI runners. Merge them into a single test that exercises both jsonb detail columns under one fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
What
Strings derived from ledger data are not guaranteed to be representable in a Postgres
jsonbcolumn. This makes the operation and effectdetailswrites robust to such values by removing any NUL character — whichjsonbcannot store — before the insert.How
detailsdocument by decoding it, stripping NUL from its string values, and re-encoding, so JSON parsing and escaping are handled byencoding/jsonrather than by editing serialized bytes. The common (no-NUL) case is returned untouched.history_operations.detailsandhistory_effects.detailscolumns.Tests
detailsdocument through both builders and confirm it stores correctly.