From 8d6da7cb0bacf4ce092f4cab10dfaff21837353d Mon Sep 17 00:00:00 2001 From: tamirms Date: Wed, 5 Aug 2026 10:49:53 +0200 Subject: [PATCH 1/5] Sanitize NUL bytes in operation and effect details before jsonb insert 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 --- internal/db2/history/details.go | 30 ++++ internal/db2/history/details_test.go | 143 ++++++++++++++++++ .../history/effect_batch_insert_builder.go | 2 +- .../history/operation_batch_insert_builder.go | 2 +- 4 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 internal/db2/history/details.go create mode 100644 internal/db2/history/details_test.go diff --git a/internal/db2/history/details.go b/internal/db2/history/details.go new file mode 100644 index 00000000..001370cd --- /dev/null +++ b/internal/db2/history/details.go @@ -0,0 +1,30 @@ +package history + +import "bytes" + +// jsonNullEscape is how encoding/json renders a NUL character (U+0000): the six +// ASCII bytes backslash, 'u', '0', '0', '0', '0'. Postgres jsonb cannot store a +// NUL, because it decodes escapes into text and text cannot hold one (a plain +// json column would accept it), so this escape must be removed before a +// marshaled document is written to a jsonb column. +var jsonNullEscape = []byte{0x5c, 'u', '0', '0', '0', '0'} + +// sanitizeJSONBDetails makes a marshaled JSON document safe to store in a jsonb +// column by removing any NUL characters it contains. +// +// Strings derived from ledger data are not guaranteed to be free of bytes that +// jsonb cannot represent: asset codes are opaque 4/12-byte arrays and Soroban +// ScVal strings are arbitrary bytes. A NUL in particular would cause the insert +// to fail. +// +// encoding/json always renders a NUL as the jsonNullEscape sequence and always +// emits valid UTF-8, so stripping that escape from the already-marshaled bytes +// is sufficient to guarantee the document is jsonb-storable. Removing the +// self-contained six-byte escape from a JSON string literal cannot change JSON +// validity or affect any other value. +func sanitizeJSONBDetails(details []byte) []byte { + if !bytes.Contains(details, jsonNullEscape) { + return details + } + return bytes.ReplaceAll(details, jsonNullEscape, nil) +} diff --git a/internal/db2/history/details_test.go b/internal/db2/history/details_test.go new file mode 100644 index 00000000..cc20182e --- /dev/null +++ b/internal/db2/history/details_test.go @@ -0,0 +1,143 @@ +package history + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + + "github.com/guregu/null" + + "github.com/stellar/go-stellar-sdk/toid" + "github.com/stellar/go-stellar-sdk/xdr" + "github.com/stellar/stellar-horizon/internal/db2" + "github.com/stellar/stellar-horizon/internal/test" +) + +const nulByteTestIssuer = "GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ" + +// nulByteTestDetails returns marshaled details whose asset code contains an +// interior NUL — a value that can occur in ledger-derived data. encoding/json +// renders the NUL as jsonNullEscape, which a jsonb column cannot store, so this +// exercises the sanitizeJSONBDetails path. sanitizedAsset is what the asset must +// read as once the NUL has been stripped. +func nulByteTestDetails(t *testing.T) (marshaled []byte, sanitizedAsset string) { + t.Helper() + poisonedCode := string([]byte{'A', 0x00, 'B'}) // 'A' NUL 'B' + details, err := json.Marshal(map[string]string{ + "from": "asset", + "asset": poisonedCode + ":" + nulByteTestIssuer, + }) + if err != nil { + t.Fatal(err) + } + if !bytes.Contains(details, jsonNullEscape) { + t.Fatalf("expected marshaled details to contain the NUL escape, got %s", details) + } + return details, "AB:" + nulByteTestIssuer +} + +func TestSanitizeJSONBDetails(t *testing.T) { + poisoned, wantAsset := nulByteTestDetails(t) + + got := sanitizeJSONBDetails(poisoned) + + if bytes.Contains(got, jsonNullEscape) { + t.Errorf("sanitized details still contain the NUL escape: %s", got) + } + if strings.ContainsRune(string(got), rune(0)) { + t.Errorf("sanitized details still contain a raw NUL: %q", string(got)) + } + if !json.Valid(got) { + t.Errorf("sanitized details are not valid JSON: %s", got) + } + var out map[string]string + if err := json.Unmarshal(got, &out); err != nil { + t.Fatalf("cannot unmarshal sanitized details: %v", err) + } + if out["asset"] != wantAsset { + t.Errorf("asset = %q, want %q", out["asset"], wantAsset) + } + + // A document with no NUL escape must be returned unchanged. + clean := []byte(`{"asset": "AB:` + nulByteTestIssuer + `", "from": "asset"}`) + if got := sanitizeJSONBDetails(clean); !bytes.Equal(got, clean) { + t.Errorf("clean input was modified: got %s want %s", got, clean) + } +} + +// TestAddOperationSanitizesNulByteInDetails checks that a NUL-bearing details +// document inserts into the real jsonb column instead of failing. +func TestAddOperationSanitizesNulByteInDetails(t *testing.T) { + tt := test.Start(t) + defer tt.Finish() + test.ResetHorizonDB(t, tt.HorizonDB) + q := &Q{tt.HorizonSession()} + + tt.Assert.NoError(q.Begin(tt.Ctx)) + + details, wantAsset := nulByteTestDetails(t) + + builder := q.NewOperationBatchInsertBuilder() + sequence := int32(56) + opID := toid.New(sequence, 1, 1).ToInt64() + sourceAccount := "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY" + + // Without sanitizeJSONBDetails this insert fails because jsonb cannot store a NUL. + tt.Assert.NoError(builder.Add( + opID, + toid.New(sequence, 1, 0).ToInt64(), + 1, + xdr.OperationTypeInvokeHostFunction, + details, + sourceAccount, + null.String{}, + false, + )) + tt.Assert.NoError(builder.Exec(tt.Ctx, q)) + tt.Assert.NoError(q.Commit()) + + var stored string + tt.Assert.NoError(q.GetRaw(tt.Ctx, &stored, + "SELECT details FROM history_operations WHERE id = $1", opID)) + tt.Assert.False(strings.ContainsRune(stored, rune(0)), "stored details must not contain a NUL") + tt.Assert.Contains(stored, wantAsset) +} + +// TestAddEffectSanitizesNulByteInDetails guards the second jsonb sink, +// history_effects.details. +func TestAddEffectSanitizesNulByteInDetails(t *testing.T) { + tt := test.Start(t) + defer tt.Finish() + test.ResetHorizonDB(t, tt.HorizonDB) + q := &Q{tt.HorizonSession()} + + tt.Assert.NoError(q.Begin(tt.Ctx)) + + details, wantAsset := nulByteTestDetails(t) + + address := "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY" + accountLoader := NewAccountLoader(ConcurrentInserts) + builder := q.NewEffectBatchInsertBuilder() + sequence := int32(56) + + // Without sanitizeJSONBDetails this insert fails because jsonb cannot store a NUL. + tt.Assert.NoError(builder.Add( + accountLoader.GetFuture(address), + null.String{}, + toid.New(sequence, 1, 1).ToInt64(), + 1, + EffectType(3), + details, + )) + tt.Assert.NoError(accountLoader.Exec(tt.Ctx, q)) + tt.Assert.NoError(builder.Exec(tt.Ctx, q)) + tt.Assert.NoError(q.Commit()) + + effects, err := q.Effects(tt.Ctx, db2.PageQuery{Cursor: "0-0", Order: "asc", Limit: 200}, 0) + tt.Assert.NoError(err) + tt.Assert.Len(effects, 1) + stored := effects[0].DetailsString.String + tt.Assert.False(strings.ContainsRune(stored, rune(0)), "stored details must not contain a NUL") + tt.Assert.Contains(stored, wantAsset) +} diff --git a/internal/db2/history/effect_batch_insert_builder.go b/internal/db2/history/effect_batch_insert_builder.go index fc9b5ed6..a37619b9 100644 --- a/internal/db2/history/effect_batch_insert_builder.go +++ b/internal/db2/history/effect_batch_insert_builder.go @@ -51,7 +51,7 @@ func (i *effectBatchInsertBuilder) Add( "history_operation_id": operationID, "order": order, "type": effectType, - "details": string(details), + "details": string(sanitizeJSONBDetails(details)), }) } diff --git a/internal/db2/history/operation_batch_insert_builder.go b/internal/db2/history/operation_batch_insert_builder.go index 90b7b289..0947e63c 100644 --- a/internal/db2/history/operation_batch_insert_builder.go +++ b/internal/db2/history/operation_batch_insert_builder.go @@ -55,7 +55,7 @@ func (i *operationBatchInsertBuilder) Add( "transaction_id": transactionID, "application_order": applicationOrder, "type": operationType, - "details": string(details), + "details": string(sanitizeJSONBDetails(details)), "source_account": sourceAccount, "source_account_muxed": sourceAccountMuxed, "is_payment": nil, From d1325ce30f5f1548eb1e43bb6975ceb9fc5792bc Mon Sep 17 00:00:00 2001 From: tamirms Date: Wed, 5 Aug 2026 11:00:02 +0200 Subject: [PATCH 2/5] Only strip genuine NUL escapes; harden effect test 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 --- internal/db2/history/details.go | 37 ++++++++++++--- internal/db2/history/details_test.go | 69 +++++++++++++++++++++++----- 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/internal/db2/history/details.go b/internal/db2/history/details.go index 001370cd..4379901e 100644 --- a/internal/db2/history/details.go +++ b/internal/db2/history/details.go @@ -17,14 +17,39 @@ var jsonNullEscape = []byte{0x5c, 'u', '0', '0', '0', '0'} // ScVal strings are arbitrary bytes. A NUL in particular would cause the insert // to fail. // -// encoding/json always renders a NUL as the jsonNullEscape sequence and always -// emits valid UTF-8, so stripping that escape from the already-marshaled bytes -// is sufficient to guarantee the document is jsonb-storable. Removing the -// self-contained six-byte escape from a JSON string literal cannot change JSON -// validity or affect any other value. +// encoding/json renders a real NUL as the jsonNullEscape sequence, but those +// same six bytes can also occur as the tail of an escaped backslash followed by +// the literal text "u0000" (for example a string whose value is the six literal +// characters marshals with a doubled leading backslash). Only an escape +// introduced by an unescaped backslash represents an actual NUL, so we walk the +// document consuming each escape as a unit and drop only genuine NUL escapes; an +// escaped-backslash pair is copied verbatim. This never alters a legitimate +// value and always leaves valid JSON. func sanitizeJSONBDetails(details []byte) []byte { if !bytes.Contains(details, jsonNullEscape) { return details } - return bytes.ReplaceAll(details, jsonNullEscape, nil) + out := make([]byte, 0, len(details)) + for i := 0; i < len(details); { + if details[i] != '\\' { + out = append(out, details[i]) + i++ + continue + } + // An escape sequence starts here. If it is a genuine NUL escape, drop it. + if bytes.HasPrefix(details[i:], jsonNullEscape) { + i += len(jsonNullEscape) + continue + } + // Otherwise copy this backslash together with the character it escapes, + // so an escaped-backslash pair is consumed as a unit and its second + // backslash cannot be mistaken for the start of a NUL escape. + out = append(out, details[i]) + i++ + if i < len(details) { + out = append(out, details[i]) + i++ + } + } + return out } diff --git a/internal/db2/history/details_test.go b/internal/db2/history/details_test.go index cc20182e..838e9710 100644 --- a/internal/db2/history/details_test.go +++ b/internal/db2/history/details_test.go @@ -16,6 +16,11 @@ import ( const nulByteTestIssuer = "GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ" +// literalNulEscape is the six literal characters backslash, 'u', '0', '0', '0', +// '0' — the text that spells a NUL escape, but is NOT a NUL. It is built from +// bytes to avoid any real escape sequence in the source. +var literalNulEscape = string([]byte{0x5c, 'u', '0', '0', '0', '0'}) + // nulByteTestDetails returns marshaled details whose asset code contains an // interior NUL — a value that can occur in ledger-derived data. encoding/json // renders the NUL as jsonNullEscape, which a jsonb column cannot store, so this @@ -64,6 +69,46 @@ func TestSanitizeJSONBDetails(t *testing.T) { if got := sanitizeJSONBDetails(clean); !bytes.Equal(got, clean) { t.Errorf("clean input was modified: got %s want %s", got, clean) } + + // A value that legitimately contains the literal escape text (a backslash + // followed by u0000, not a NUL) marshals with a doubled leading backslash. It + // must be preserved, not mistaken for a NUL escape and truncated into invalid + // JSON. + withLiteral, err := json.Marshal(map[string]string{"asset": literalNulEscape}) + if err != nil { + t.Fatal(err) + } + sanitized := sanitizeJSONBDetails(withLiteral) + if !json.Valid(sanitized) { + t.Errorf("literal-escape input produced invalid JSON: %s", sanitized) + } + var lit map[string]string + if err := json.Unmarshal(sanitized, &lit); err != nil { + t.Fatalf("cannot unmarshal sanitized literal-escape details: %v", err) + } + if lit["asset"] != literalNulEscape { + t.Errorf("literal escape value corrupted: got %q want %q", lit["asset"], literalNulEscape) + } + + // A value with both a real NUL and the literal escape text: only the NUL is + // removed, the literal text survives. + mixed, err := json.Marshal(map[string]string{ + "asset": string([]byte{'A', 0x00}) + literalNulEscape, + }) + if err != nil { + t.Fatal(err) + } + sanitizedMixed := sanitizeJSONBDetails(mixed) + if !json.Valid(sanitizedMixed) { + t.Errorf("mixed input produced invalid JSON: %s", sanitizedMixed) + } + var mix map[string]string + if err := json.Unmarshal(sanitizedMixed, &mix); err != nil { + t.Fatalf("cannot unmarshal sanitized mixed details: %v", err) + } + if want := "A" + literalNulEscape; mix["asset"] != want { + t.Errorf("mixed value: got %q want %q", mix["asset"], want) + } } // TestAddOperationSanitizesNulByteInDetails checks that a NUL-bearing details @@ -74,7 +119,7 @@ func TestAddOperationSanitizesNulByteInDetails(t *testing.T) { test.ResetHorizonDB(t, tt.HorizonDB) q := &Q{tt.HorizonSession()} - tt.Assert.NoError(q.Begin(tt.Ctx)) + tt.Require.NoError(q.Begin(tt.Ctx)) details, wantAsset := nulByteTestDetails(t) @@ -84,7 +129,7 @@ func TestAddOperationSanitizesNulByteInDetails(t *testing.T) { sourceAccount := "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY" // Without sanitizeJSONBDetails this insert fails because jsonb cannot store a NUL. - tt.Assert.NoError(builder.Add( + tt.Require.NoError(builder.Add( opID, toid.New(sequence, 1, 0).ToInt64(), 1, @@ -94,11 +139,11 @@ func TestAddOperationSanitizesNulByteInDetails(t *testing.T) { null.String{}, false, )) - tt.Assert.NoError(builder.Exec(tt.Ctx, q)) - tt.Assert.NoError(q.Commit()) + tt.Require.NoError(builder.Exec(tt.Ctx, q)) + tt.Require.NoError(q.Commit()) var stored string - tt.Assert.NoError(q.GetRaw(tt.Ctx, &stored, + tt.Require.NoError(q.GetRaw(tt.Ctx, &stored, "SELECT details FROM history_operations WHERE id = $1", opID)) tt.Assert.False(strings.ContainsRune(stored, rune(0)), "stored details must not contain a NUL") tt.Assert.Contains(stored, wantAsset) @@ -112,7 +157,7 @@ func TestAddEffectSanitizesNulByteInDetails(t *testing.T) { test.ResetHorizonDB(t, tt.HorizonDB) q := &Q{tt.HorizonSession()} - tt.Assert.NoError(q.Begin(tt.Ctx)) + tt.Require.NoError(q.Begin(tt.Ctx)) details, wantAsset := nulByteTestDetails(t) @@ -122,7 +167,7 @@ func TestAddEffectSanitizesNulByteInDetails(t *testing.T) { sequence := int32(56) // Without sanitizeJSONBDetails this insert fails because jsonb cannot store a NUL. - tt.Assert.NoError(builder.Add( + tt.Require.NoError(builder.Add( accountLoader.GetFuture(address), null.String{}, toid.New(sequence, 1, 1).ToInt64(), @@ -130,13 +175,13 @@ func TestAddEffectSanitizesNulByteInDetails(t *testing.T) { EffectType(3), details, )) - tt.Assert.NoError(accountLoader.Exec(tt.Ctx, q)) - tt.Assert.NoError(builder.Exec(tt.Ctx, q)) - tt.Assert.NoError(q.Commit()) + tt.Require.NoError(accountLoader.Exec(tt.Ctx, q)) + tt.Require.NoError(builder.Exec(tt.Ctx, q)) + tt.Require.NoError(q.Commit()) effects, err := q.Effects(tt.Ctx, db2.PageQuery{Cursor: "0-0", Order: "asc", Limit: 200}, 0) - tt.Assert.NoError(err) - tt.Assert.Len(effects, 1) + tt.Require.NoError(err) + tt.Require.Len(effects, 1) stored := effects[0].DetailsString.String tt.Assert.False(strings.ContainsRune(stored, rune(0)), "stored details must not contain a NUL") tt.Assert.Contains(stored, wantAsset) From c4fc0722963856708c41f2e7e4ae947c141039be Mon Sep 17 00:00:00 2001 From: tamirms Date: Wed, 5 Aug 2026 11:19:27 +0200 Subject: [PATCH 3/5] Re-encode details instead of byte-editing to strip NUL 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 --- internal/db2/history/details.go | 96 +++++++++++++++++++++------------ 1 file changed, 62 insertions(+), 34 deletions(-) diff --git a/internal/db2/history/details.go b/internal/db2/history/details.go index 4379901e..0befc757 100644 --- a/internal/db2/history/details.go +++ b/internal/db2/history/details.go @@ -1,55 +1,83 @@ package history -import "bytes" +import ( + "bytes" + "encoding/json" + "strings" +) // jsonNullEscape is how encoding/json renders a NUL character (U+0000): the six // ASCII bytes backslash, 'u', '0', '0', '0', '0'. Postgres jsonb cannot store a // NUL, because it decodes escapes into text and text cannot hold one (a plain -// json column would accept it), so this escape must be removed before a -// marshaled document is written to a jsonb column. +// json column would accept it), so a marshaled document containing one cannot be +// written to a jsonb column as-is. It is used only as a cheap pre-check. var jsonNullEscape = []byte{0x5c, 'u', '0', '0', '0', '0'} // sanitizeJSONBDetails makes a marshaled JSON document safe to store in a jsonb -// column by removing any NUL characters it contains. +// column by removing NUL characters from every string it contains. // // Strings derived from ledger data are not guaranteed to be free of bytes that // jsonb cannot represent: asset codes are opaque 4/12-byte arrays and Soroban -// ScVal strings are arbitrary bytes. A NUL in particular would cause the insert -// to fail. +// ScVal strings are arbitrary bytes. A NUL would otherwise cause the insert to +// fail. // -// encoding/json renders a real NUL as the jsonNullEscape sequence, but those -// same six bytes can also occur as the tail of an escaped backslash followed by -// the literal text "u0000" (for example a string whose value is the six literal -// characters marshals with a doubled leading backslash). Only an escape -// introduced by an unescaped backslash represents an actual NUL, so we walk the -// document consuming each escape as a unit and drop only genuine NUL escapes; an -// escaped-backslash pair is copied verbatim. This never alters a legitimate -// value and always leaves valid JSON. +// The common case — no NUL — returns the input untouched. Otherwise the document +// is decoded, NUL is stripped from its string values (and any object keys), and +// it is re-encoded, leaving all JSON parsing and escaping to encoding/json +// rather than editing the serialized bytes by hand. UseNumber keeps numeric +// values exact across the round trip. func sanitizeJSONBDetails(details []byte) []byte { if !bytes.Contains(details, jsonNullEscape) { return details } - out := make([]byte, 0, len(details)) - for i := 0; i < len(details); { - if details[i] != '\\' { - out = append(out, details[i]) - i++ - continue - } - // An escape sequence starts here. If it is a genuine NUL escape, drop it. - if bytes.HasPrefix(details[i:], jsonNullEscape) { - i += len(jsonNullEscape) - continue + + decoder := json.NewDecoder(bytes.NewReader(details)) + decoder.UseNumber() + var decoded interface{} + if err := decoder.Decode(&decoded); err != nil { + return details + } + + sanitized, err := json.Marshal(stripNULFromJSON(decoded)) + if err != nil { + return details + } + return sanitized +} + +// stripNULFromJSON removes NUL characters from every string within a value +// decoded from JSON. Such a value only ever holds string, json.Number, bool, +// nil, []interface{} and map[string]interface{} — never a struct — so recursion +// over these cases reaches every string. +func stripNULFromJSON(value interface{}) interface{} { + switch v := value.(type) { + case string: + return stripNUL(v) + case []interface{}: + for i := range v { + v[i] = stripNULFromJSON(v[i]) } - // Otherwise copy this backslash together with the character it escapes, - // so an escaped-backslash pair is consumed as a unit and its second - // backslash cannot be mistaken for the start of a NUL escape. - out = append(out, details[i]) - i++ - if i < len(details) { - out = append(out, details[i]) - i++ + return v + case map[string]interface{}: + sanitized := make(map[string]interface{}, len(v)) + for key, val := range v { + sanitized[stripNUL(key)] = stripNULFromJSON(val) } + return sanitized + default: + return v + } +} + +// stripNUL removes every NUL character (U+0000) from s. +func stripNUL(s string) string { + if !strings.ContainsRune(s, 0) { + return s } - return out + return strings.Map(func(r rune) rune { + if r == 0 { + return -1 + } + return r + }, s) } From 79369b445946086f13fb7d4bceaf98b51b0261ce Mon Sep 17 00:00:00 2001 From: tamirms Date: Wed, 5 Aug 2026 11:36:16 +0200 Subject: [PATCH 4/5] Avoid shadowing err in sanitizer test 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 --- internal/db2/history/details_test.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/db2/history/details_test.go b/internal/db2/history/details_test.go index 838e9710..d708539b 100644 --- a/internal/db2/history/details_test.go +++ b/internal/db2/history/details_test.go @@ -43,6 +43,14 @@ func nulByteTestDetails(t *testing.T) (marshaled []byte, sanitizedAsset string) } func TestSanitizeJSONBDetails(t *testing.T) { + mustMarshal := func(v map[string]string) []byte { + b, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + return b + } + poisoned, wantAsset := nulByteTestDetails(t) got := sanitizeJSONBDetails(poisoned) @@ -74,10 +82,7 @@ func TestSanitizeJSONBDetails(t *testing.T) { // followed by u0000, not a NUL) marshals with a doubled leading backslash. It // must be preserved, not mistaken for a NUL escape and truncated into invalid // JSON. - withLiteral, err := json.Marshal(map[string]string{"asset": literalNulEscape}) - if err != nil { - t.Fatal(err) - } + withLiteral := mustMarshal(map[string]string{"asset": literalNulEscape}) sanitized := sanitizeJSONBDetails(withLiteral) if !json.Valid(sanitized) { t.Errorf("literal-escape input produced invalid JSON: %s", sanitized) @@ -92,12 +97,9 @@ func TestSanitizeJSONBDetails(t *testing.T) { // A value with both a real NUL and the literal escape text: only the NUL is // removed, the literal text survives. - mixed, err := json.Marshal(map[string]string{ + mixed := mustMarshal(map[string]string{ "asset": string([]byte{'A', 0x00}) + literalNulEscape, }) - if err != nil { - t.Fatal(err) - } sanitizedMixed := sanitizeJSONBDetails(mixed) if !json.Valid(sanitizedMixed) { t.Errorf("mixed input produced invalid JSON: %s", sanitizedMixed) From 66ed046a298f6e4cdc6d221f98e14c11a7e2e265 Mon Sep 17 00:00:00 2001 From: tamirms Date: Wed, 5 Aug 2026 11:44:58 +0200 Subject: [PATCH 5/5] Combine jsonb detail regression tests into one DB fixture 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 --- internal/db2/history/details_test.go | 66 +++++++++++----------------- 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/internal/db2/history/details_test.go b/internal/db2/history/details_test.go index d708539b..2808851b 100644 --- a/internal/db2/history/details_test.go +++ b/internal/db2/history/details_test.go @@ -113,64 +113,50 @@ func TestSanitizeJSONBDetails(t *testing.T) { } } -// TestAddOperationSanitizesNulByteInDetails checks that a NUL-bearing details -// document inserts into the real jsonb column instead of failing. -func TestAddOperationSanitizesNulByteInDetails(t *testing.T) { +// TestSanitizeJSONBDetailsWritesToDB is the end-to-end regression guard: a +// NUL-bearing details document must insert into the real jsonb columns +// (history_operations.details and history_effects.details) instead of failing. +// Both sinks share one database fixture to keep this DB-backed test cheap. +func TestSanitizeJSONBDetailsWritesToDB(t *testing.T) { tt := test.Start(t) defer tt.Finish() test.ResetHorizonDB(t, tt.HorizonDB) q := &Q{tt.HorizonSession()} - tt.Require.NoError(q.Begin(tt.Ctx)) - details, wantAsset := nulByteTestDetails(t) - - builder := q.NewOperationBatchInsertBuilder() sequence := int32(56) - opID := toid.New(sequence, 1, 1).ToInt64() - sourceAccount := "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY" + account := "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY" - // Without sanitizeJSONBDetails this insert fails because jsonb cannot store a NUL. - tt.Require.NoError(builder.Add( + // history_operations.details: without sanitizeJSONBDetails this insert fails + // because jsonb cannot store a NUL. + tt.Require.NoError(q.Begin(tt.Ctx)) + opBuilder := q.NewOperationBatchInsertBuilder() + opID := toid.New(sequence, 1, 1).ToInt64() + tt.Require.NoError(opBuilder.Add( opID, toid.New(sequence, 1, 0).ToInt64(), 1, xdr.OperationTypeInvokeHostFunction, details, - sourceAccount, + account, null.String{}, false, )) - tt.Require.NoError(builder.Exec(tt.Ctx, q)) + tt.Require.NoError(opBuilder.Exec(tt.Ctx, q)) tt.Require.NoError(q.Commit()) - var stored string - tt.Require.NoError(q.GetRaw(tt.Ctx, &stored, + var storedOp string + tt.Require.NoError(q.GetRaw(tt.Ctx, &storedOp, "SELECT details FROM history_operations WHERE id = $1", opID)) - tt.Assert.False(strings.ContainsRune(stored, rune(0)), "stored details must not contain a NUL") - tt.Assert.Contains(stored, wantAsset) -} - -// TestAddEffectSanitizesNulByteInDetails guards the second jsonb sink, -// history_effects.details. -func TestAddEffectSanitizesNulByteInDetails(t *testing.T) { - tt := test.Start(t) - defer tt.Finish() - test.ResetHorizonDB(t, tt.HorizonDB) - q := &Q{tt.HorizonSession()} + tt.Assert.False(strings.ContainsRune(storedOp, rune(0)), "operation details must not contain a NUL") + tt.Assert.Contains(storedOp, wantAsset) + // history_effects.details: the second jsonb sink. tt.Require.NoError(q.Begin(tt.Ctx)) - - details, wantAsset := nulByteTestDetails(t) - - address := "GAQAA5L65LSYH7CQ3VTJ7F3HHLGCL3DSLAR2Y47263D56MNNGHSQSTVY" accountLoader := NewAccountLoader(ConcurrentInserts) - builder := q.NewEffectBatchInsertBuilder() - sequence := int32(56) - - // Without sanitizeJSONBDetails this insert fails because jsonb cannot store a NUL. - tt.Require.NoError(builder.Add( - accountLoader.GetFuture(address), + effectBuilder := q.NewEffectBatchInsertBuilder() + tt.Require.NoError(effectBuilder.Add( + accountLoader.GetFuture(account), null.String{}, toid.New(sequence, 1, 1).ToInt64(), 1, @@ -178,13 +164,13 @@ func TestAddEffectSanitizesNulByteInDetails(t *testing.T) { details, )) tt.Require.NoError(accountLoader.Exec(tt.Ctx, q)) - tt.Require.NoError(builder.Exec(tt.Ctx, q)) + tt.Require.NoError(effectBuilder.Exec(tt.Ctx, q)) tt.Require.NoError(q.Commit()) effects, err := q.Effects(tt.Ctx, db2.PageQuery{Cursor: "0-0", Order: "asc", Limit: 200}, 0) tt.Require.NoError(err) tt.Require.Len(effects, 1) - stored := effects[0].DetailsString.String - tt.Assert.False(strings.ContainsRune(stored, rune(0)), "stored details must not contain a NUL") - tt.Assert.Contains(stored, wantAsset) + storedEffect := effects[0].DetailsString.String + tt.Assert.False(strings.ContainsRune(storedEffect, rune(0)), "effect details must not contain a NUL") + tt.Assert.Contains(storedEffect, wantAsset) }