Skip to content

Commit 9ecf032

Browse files
authored
fix: keep the last raw-byte truncations on a rune boundary (#1726)
# Pull Request ## Description Round 3 of the UTF-8-safe truncation sweep started in #1647 and continued in #1664 / #1665. Three production sites still cut text with a raw byte slice (`s[:n]`), so a non-ASCII value whose cut offset lands inside a multi-byte rune is published as invalid UTF-8, and JSON encoding rewrites the tail to U+FFFD. ### 1. `internal/storage` — the activity-log per-record cap `truncateResponse` is the only thing bounding a single activity record. It now goes through `truncateRuneSafe`, the helper the per-server tool-call records in the *same package* (`internal/storage/tool_call_bounds.go`) already use for exactly this reason. **Reachability.** An upstream tool response arrives as the `response` payload of an activity event, and `ActivityService.handleEvent` → `truncateForStorage` → `storage.TruncateActivityResponse` → `truncateResponse` applies `activity_max_response_size` (default 64 KB) on the write path (`internal/runtime/activity_service.go:267`). Any response over the cap whose byte at the cut offset is a UTF-8 continuation byte — ordinary CJK, Cyrillic, or emoji text — lands in `config.db` malformed, and from there in `GET /api/v1/activities`, `GET /api/v1/activities/{id}` and the CSV export. With a 3-byte rune, 2 of every 3 cut offsets are mid-rune. ### 2. `internal/outputvalidation` — the 500-byte violation `Reason` A `jsonschema` message quotes the offending instance location, so a non-ASCII property name or value sits astride the cut. **Reachability.** `Validator.Validate` calls `truncate(err.Error(), 500)` (`validator.go:157`) on any structured tool output that fails its `outputSchema`; the `Reason` goes back to the agent and into the audit line. Measured on the pre-fix code with a 200-rune non-ASCII property name, shifting the cut by one byte at a time: offsets 0 and 2 produce invalid UTF-8, offset 1 is valid — i.e. 2 of 3. ### 3. `internal/registries` — the 300-byte Pulse description cap Registry descriptions are upstream-authored free text. **Reachability.** `parsePulseWithoutGuesser` fills `ServerEntry.Description` from `short_description` / `EXPERIMENTAL_ai_generated_description` for every result of a Pulse registry search, which the REST API and the Web UI catalog then JSON-encode. Same 2-of-3 pattern in the test. The 300 literal is now a named constant (`pulseDescriptionMaxBytes`) since it appeared twice. ASCII behaviour is unchanged at all three sites, and the retained prefix never exceeds the cap. **Out of scope,** deliberately, so this stays reviewable: - The CLI table cells in `cmd/mcpproxy` that #1664 also left alone — sent separately. - `cmd/mcpproxy/security_cmd.go` and `cmd/mcpproxy/main.go` have the same shape but are under open PRs (#1714, #1720, #1725), so I have not touched them. - `internal/configimport.SanitizeServerName` and `internal/dockernaming.SanitizeServerName` also cut at a byte offset, but both strip everything outside `[A-Za-z0-9._-]` first, so no multi-byte rune can reach the cut. Left as they are. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass New tests, each failing by name at the base SHA (`14cb288`) and passing after: | Test | Package | |---|---| | `TestTruncateResponseKeepsRuneBoundary`, `TestTruncateResponseASCIIUnchanged` | `internal/storage` | | `TestViolationReasonIsValidUTF8`, `TestTruncateASCIIUnchanged` | `internal/outputvalidation` | | `TestPulseDescriptionTruncationIsValidUTF8` | `internal/registries` | Each boundary test sweeps every offset inside a 3-byte rune rather than asserting one fixed cap, so it cannot pass by luck. Commands run (exit status in brackets): ``` git diff --check [0] gofmt -l internal/storage internal/outputvalidation internal/registries [0, no output] go vet ./internal/storage ./internal/outputvalidation ./internal/registries [0] go test ./internal/storage ./internal/outputvalidation ./internal/registries [0] go test ./internal/runtime -run 'TestToolCallResponseTruncatedForStorage|TestInternalToolCallResponseTruncatedForStorage|TestStorageTruncatedBuiltinStillCountsDeliveredBytes|TestPromptGetResponseTruncatedForStorage|TestDefaultResponseCapAppliesWithoutConfig|TestEmitterTruncationFlagSurvivesUntruncatedRecord' [0, 6/6 pass] ``` The `internal/runtime` selection is the end-to-end activity-truncation suite that drives the storage cut through the real event path, included to show the write-path behaviour and the `ResponseTruncated` flag semantics are unchanged. Not run: `./scripts/run-all-tests.sh` and `-race` (local disk constraints on this machine). Nothing here is concurrent.
1 parent 92dc64e commit 9ecf032

6 files changed

Lines changed: 159 additions & 14 deletions

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package outputvalidation
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
"testing"
7+
"unicode/utf8"
8+
)
9+
10+
// A schema-violation Reason is capped at 500 bytes, and jsonschema's message
11+
// quotes the offending instance location — so a non-ASCII property name or
12+
// value puts multi-byte runes astride the cut. The raw byte slice left the
13+
// Reason invalid UTF-8, which JSON encoding rewrites to U+FFFD on the way to
14+
// the agent and the audit line (round 3 of #1647/#1664/#1665).
15+
//
16+
// The 1-byte padding shifts every later byte, so the loop covers all three
17+
// offsets inside a 3-byte rune; two of the three are mid-rune.
18+
func TestViolationReasonIsValidUTF8(t *testing.T) {
19+
v := New(0, 0, nil)
20+
for pad := 0; pad < 3; pad++ {
21+
name := strings.Repeat("a", pad) + strings.Repeat("語", 200)
22+
schema := fmt.Sprintf(`{"type":"object","properties":{%q:{"type":"integer"}}}`, name)
23+
24+
verdict := v.Validate(fmt.Sprintf("srv:tool%d", pad), schema, map[string]any{name: "not-an-integer"})
25+
if !verdict.IsViolation() {
26+
t.Fatalf("pad=%d: expected a violation", pad)
27+
}
28+
if len(verdict.Reason) > 500 {
29+
t.Errorf("pad=%d: reason is %d bytes, over the 500-byte cap", pad, len(verdict.Reason))
30+
}
31+
if !utf8.ValidString(verdict.Reason) {
32+
t.Errorf("pad=%d: reason is not valid UTF-8 (cut inside a rune)", pad)
33+
}
34+
}
35+
}
36+
37+
// ASCII behaviour must not move.
38+
func TestTruncateASCIIUnchanged(t *testing.T) {
39+
if got, want := truncate(strings.Repeat("x", 10), 20), strings.Repeat("x", 10); got != want {
40+
t.Errorf("under the cap: got %q, want %q", got, want)
41+
}
42+
if got, want := truncate(strings.Repeat("x", 30), 20), strings.Repeat("x", 17)+"..."; got != want {
43+
t.Errorf("over the cap: got %q, want %q", got, want)
44+
}
45+
}

‎internal/outputvalidation/validator.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import (
1111

1212
"github.com/santhosh-tekuri/jsonschema/v6"
1313
"go.uber.org/zap"
14+
15+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/stringutil"
1416
)
1517

1618
// compileCount counts how many times a schema is actually compiled (vs served
@@ -225,10 +227,15 @@ func hashSchema(schemaJSON string) uint64 {
225227
return h.Sum64()
226228
}
227229

228-
// truncate shortens s to at most maxLen characters, appending "..." if truncated.
230+
// truncate shortens s to at most maxLen bytes, appending "..." if truncated.
231+
//
232+
// The cut goes through stringutil.UTF8Prefix: a jsonschema message quotes the
233+
// offending instance location, so a non-ASCII property name or value puts
234+
// multi-byte runes astride the cut, and a raw byte slice leaves the Reason
235+
// invalid UTF-8 (U+FFFD once JSON-encoded for the agent and the audit line).
229236
func truncate(s string, maxLen int) string {
230237
if len(s) <= maxLen {
231238
return s
232239
}
233-
return s[:maxLen-3] + "..."
240+
return stringutil.UTF8Prefix(s, maxLen-3) + "..."
234241
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package registries
2+
3+
import (
4+
"encoding/json"
5+
"fmt"
6+
"strings"
7+
"testing"
8+
"unicode/utf8"
9+
)
10+
11+
// PulseMCP descriptions are upstream-authored free text, so a non-Latin
12+
// description longer than the 300-byte display cap put multi-byte runes
13+
// astride the cut. The raw byte slice left ServerEntry.Description invalid
14+
// UTF-8, which the REST/Web UI JSON encoder rewrites to U+FFFD (round 3 of
15+
// #1647/#1664/#1665).
16+
//
17+
// The 1-byte padding covers all three offsets inside a 3-byte rune.
18+
func TestPulseDescriptionTruncationIsValidUTF8(t *testing.T) {
19+
for _, field := range []string{"short_description", "EXPERIMENTAL_ai_generated_description"} {
20+
for pad := 0; pad < 3; pad++ {
21+
desc := strings.Repeat("a", pad) + strings.Repeat("語", 200)
22+
payload := fmt.Sprintf(`{"servers":[{"name":"srv",%q:%q}]}`, field, desc)
23+
24+
var raw map[string]interface{}
25+
if err := json.Unmarshal([]byte(payload), &raw); err != nil {
26+
t.Fatalf("fixture is not JSON: %v", err)
27+
}
28+
entries := parsePulseWithoutGuesser(raw)
29+
if len(entries) != 1 {
30+
t.Fatalf("%s pad=%d: parsed %d entries, want 1", field, pad, len(entries))
31+
}
32+
got := entries[0].Description
33+
if len(got) > 300 {
34+
t.Errorf("%s pad=%d: description is %d bytes, over the 300-byte cap", field, pad, len(got))
35+
}
36+
if !utf8.ValidString(got) {
37+
t.Errorf("%s pad=%d: description is not valid UTF-8 (cut inside a rune)", field, pad)
38+
}
39+
}
40+
}
41+
}

‎internal/registries/search.go‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,13 @@ import (
99
"strings"
1010

1111
"github.com/smart-mcp-proxy/mcpproxy-go/internal/experiments"
12+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/stringutil"
1213
)
1314

15+
// pulseDescriptionMaxBytes caps the description carried on a Pulse registry
16+
// search result.
17+
const pulseDescriptionMaxBytes = 300
18+
1419
// Sentinel errors for single-server lookup. Surfaces map these to stable error
1520
// codes (registry_not_found / server_not_found) for cross-surface consistency
1621
// (CN-004).
@@ -702,19 +707,15 @@ func parsePulseWithoutGuesser(rawData interface{}) []ServerEntry {
702707
Name: name,
703708
}
704709

705-
// Try to get description from multiple fields
710+
// Try to get description from multiple fields. The display cap goes
711+
// through stringutil.UTF8Prefix: a registry description is
712+
// upstream-authored free text, and a raw byte cut at 300 can land
713+
// inside a multi-byte rune, leaving the entry invalid UTF-8 (U+FFFD
714+
// once the REST/Web UI encoder sees it).
706715
if shortDesc, ok := itemMap["short_description"].(string); ok && shortDesc != "" {
707-
if len(shortDesc) > 300 {
708-
server.Description = shortDesc[:300]
709-
} else {
710-
server.Description = shortDesc
711-
}
716+
server.Description = stringutil.UTF8Prefix(shortDesc, pulseDescriptionMaxBytes)
712717
} else if aiDesc, ok := itemMap["EXPERIMENTAL_ai_generated_description"].(string); ok && aiDesc != "" {
713-
if len(aiDesc) > 300 {
714-
server.Description = aiDesc[:300]
715-
} else {
716-
server.Description = aiDesc
717-
}
718+
server.Description = stringutil.UTF8Prefix(aiDesc, pulseDescriptionMaxBytes)
718719
} else {
719720
server.Description = noDescAvailable
720721
}

‎internal/storage/activity.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,20 @@ func parseActivityKey(key []byte) string {
3232

3333
// truncateResponse truncates a response string if it exceeds maxSize.
3434
// Returns the (potentially truncated) string and whether truncation occurred.
35+
//
36+
// The cut goes through truncateRuneSafe, the same in-package helper the
37+
// per-server tool-call records use: an upstream response is arbitrary UTF-8,
38+
// and a raw byte cut at maxSize can land inside a multi-byte rune. The record
39+
// is then persisted and served over /api/v1/activities with invalid UTF-8,
40+
// which JSON encoding rewrites to U+FFFD.
3541
func truncateResponse(response string, maxSize int) (string, bool) {
3642
if maxSize <= 0 {
3743
maxSize = DefaultMaxResponseSize
3844
}
3945
if len(response) <= maxSize {
4046
return response, false
4147
}
42-
return response[:maxSize] + "...[truncated]", true
48+
return truncateRuneSafe(response, maxSize) + truncationMarker, true
4349
}
4450

4551
// SaveActivity stores an activity record in BBolt.
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package storage
2+
3+
import (
4+
"strings"
5+
"testing"
6+
"unicode/utf8"
7+
)
8+
9+
// Round 3 of the UTF-8 truncation sweep (#1647, #1664, #1665): the activity
10+
// log's own per-record cut was still a raw byte slice, so a response whose
11+
// cut offset lands inside a multi-byte rune was persisted — and served over
12+
// /api/v1/activities and the CSV export — as invalid UTF-8, which JSON
13+
// encoding rewrites to U+FFFD.
14+
//
15+
// Every offset inside a 3-byte rune is exercised, because only two of the
16+
// three are mid-rune: a single fixed cap would pass by luck.
17+
func TestTruncateResponseKeepsRuneBoundary(t *testing.T) {
18+
body := strings.Repeat("語", 200) // 3 bytes per rune
19+
20+
for _, maxSize := range []int{100, 101, 102, 103, 104, 105} {
21+
got, truncated := truncateResponse(body, maxSize)
22+
if !truncated {
23+
t.Fatalf("maxSize=%d: expected truncation of a %d-byte body", maxSize, len(body))
24+
}
25+
kept := strings.TrimSuffix(got, "...[truncated]")
26+
if !utf8.ValidString(kept) {
27+
t.Errorf("maxSize=%d: stored response ends in a partial rune: % x",
28+
maxSize, []byte(kept)[max(0, len(kept)-4):])
29+
}
30+
if len(kept) > maxSize {
31+
t.Errorf("maxSize=%d: kept %d bytes, which is over the cap", maxSize, len(kept))
32+
}
33+
}
34+
}
35+
36+
// ASCII behaviour must not move: the cap is still applied exactly.
37+
func TestTruncateResponseASCIIUnchanged(t *testing.T) {
38+
got, truncated := truncateResponse(strings.Repeat("x", 500), 100)
39+
if !truncated {
40+
t.Fatal("expected truncation")
41+
}
42+
if want := strings.Repeat("x", 100) + "...[truncated]"; got != want {
43+
t.Errorf("ASCII cut moved:\n got %q\nwant %q", got, want)
44+
}
45+
}

0 commit comments

Comments
 (0)