From e605bf5ff047e25b393ac9e2d790644b59cd89a2 Mon Sep 17 00:00:00 2001 From: "ryunosuke.ito" Date: Thu, 4 Jun 2026 13:48:05 +0900 Subject: [PATCH] fix: json ShapeDeserializer panic on union nested directly inside a union ReadUnion decided whether it was resuming an in-progress union read by checking whether the top of the context stack was ctxUnion. When a union member's value is itself a union, the inner ReadUnion call saw the parent's ctxUnion, wrongly resumed, skipped the inner '{' and passed the 1-byte token to memberFromToken, panicking with "slice bounds out of range [1:0]". Disambiguate by peeking: at a value position the next token can only be '{' (or null), while on resume it can only be a member name or '}' -- disjoint sets given the parser's grammar state machine. Also: - memberFromToken: validate the token shape instead of panicking. - smithy.ReadUnion helper: stop reading when the first ReadUnion call returns no member (union value was null/empty/all-null members); reading again consumed tokens past the union. Fixes #671 Co-Authored-By: Claude Opus 4.8 (1M context) --- serde.go | 13 ++- .../internal/json/shape_deserializer.go | 19 +++- .../json/shape_deserializer_union_test.go | 94 +++++++++++++++++++ 3 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 transport/http/protocol/internal/json/shape_deserializer_union_test.go diff --git a/serde.go b/serde.go index dacc4390..2b34f5e0 100644 --- a/serde.go +++ b/serde.go @@ -138,10 +138,15 @@ func ReadUnion(d ShapeDeserializer, schema *Schema, memberFn func(*Schema) error return err } - if ms != nil { - if err := memberFn(ms); err != nil { - return err - } + // A nil member on the first read means the union value was null, empty, + // or contained only null members -- the union has been fully consumed and + // reading again would read past it. + if ms == nil { + return nil + } + + if err := memberFn(ms); err != nil { + return err } for { diff --git a/transport/http/protocol/internal/json/shape_deserializer.go b/transport/http/protocol/internal/json/shape_deserializer.go index b1337833..53ab5565 100644 --- a/transport/http/protocol/internal/json/shape_deserializer.go +++ b/transport/http/protocol/internal/json/shape_deserializer.go @@ -11,11 +11,11 @@ import ( "time" "github.com/aws/smithy-go" - "github.com/aws/smithy-go/transport/http/protocol/internal/json/internal/stdlib" "github.com/aws/smithy-go/document" "github.com/aws/smithy-go/internal/serde" smithytime "github.com/aws/smithy-go/time" "github.com/aws/smithy-go/traits" + "github.com/aws/smithy-go/transport/http/protocol/internal/json/internal/stdlib" ) type ctxKind int8 @@ -500,7 +500,19 @@ func (d *ShapeDeserializer) ReadStructMember() (*smithy.Schema, error) { // ReadUnion implements [smithy.ShapeDeserializer]. func (d *ShapeDeserializer) ReadUnion(s *smithy.Schema) (*smithy.Schema, error) { - if top := d.head.Top(); top == nil || top.kind != ctxUnion { + resuming := false + if top := d.head.Top(); top != nil && top.kind == ctxUnion { + // The context on top of the stack may belong to a parent union when + // this union is itself the value of a union member. Disambiguate by + // peeking: at a value position the next token can only be '{' (or + // null), while on resume it can only be a member name or '}'. + tok, err := d.peek() + if err != nil { + return nil, err + } + resuming = !isLCB(tok) && !isN(tok) + } + if !resuming { if isNil, err := d.ReadNil(s); isNil || err != nil { return nil, err } @@ -588,6 +600,9 @@ func unquote(tok []byte) (string, error) { } func memberFromToken(s *smithy.Schema, tok []byte, escaped bool) (*smithy.Schema, error) { + if len(tok) < 2 || tok[0] != '"' { + return nil, fmt.Errorf("expected member name, got %s", tok) + } inner := tok[1 : len(tok)-1] if m := memberByBytes(s, inner); m != nil { return m, nil diff --git a/transport/http/protocol/internal/json/shape_deserializer_union_test.go b/transport/http/protocol/internal/json/shape_deserializer_union_test.go new file mode 100644 index 00000000..af6fbcbd --- /dev/null +++ b/transport/http/protocol/internal/json/shape_deserializer_union_test.go @@ -0,0 +1,94 @@ +package json + +import ( + "testing" + + "github.com/aws/smithy-go" +) + +// Schemas modeling a union member whose value is itself a union, e.g. +// bedrock-agentcore's TargetConfiguration -> mcp -> McpTargetConfiguration. +var ( + testSchemaString = smithy.NewSchema(smithy.ShapeID{ + Namespace: "smithy.api", Name: "String", + }, smithy.ShapeTypeString, 0) + + testSchemaInnerUnion = smithy.NewSchema(smithy.ShapeID{ + Namespace: "com.test", Name: "InnerUnion", + }, smithy.ShapeTypeUnion, 1) + + testSchemaOuterUnion = smithy.NewSchema(smithy.ShapeID{ + Namespace: "com.test", Name: "OuterUnion", + }, smithy.ShapeTypeUnion, 1) + + testSchemaInnerUnion_Lambda *smithy.Schema + testSchemaOuterUnion_Mcp *smithy.Schema +) + +func init() { + testSchemaInnerUnion_Lambda = testSchemaInnerUnion.AddMember("lambda", testSchemaString) + testSchemaOuterUnion_Mcp = testSchemaOuterUnion.AddMember("mcp", testSchemaInnerUnion) +} + +// readNestedUnion mimics the calling pattern of SDK-generated code +// (smithy.ReadUnion in serde.go): repeatedly call ReadUnion until it returns +// no member, deserializing each member value in between. +func readNestedUnion(d *ShapeDeserializer) (member string, value string, err error) { + err = smithy.ReadUnion(d, testSchemaOuterUnion, func(ms *smithy.Schema) error { + member = ms.MemberName() + return smithy.ReadUnion(d, testSchemaInnerUnion, func(inner *smithy.Schema) error { + member += "." + inner.MemberName() + return d.ReadString(inner, &value) + }) + }) + return member, value, err +} + +func TestReadUnion_NestedUnionValue(t *testing.T) { + // A union whose member value is itself a union. Before the fix, + // ReadUnion mistook the parent's union context for its own, skipped the + // inner '{' and panicked in memberFromToken (slice bounds [1:0]). + d := NewShapeDeserializer([]byte(`{"mcp":{"lambda":"arn:aws:lambda:fn"}}`)) + + member, value, err := readNestedUnion(d) + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + if member != "mcp.lambda" { + t.Errorf("expected member mcp.lambda, got %q", member) + } + if value != "arn:aws:lambda:fn" { + t.Errorf("expected value arn:aws:lambda:fn, got %q", value) + } +} + +func TestReadUnion_NestedUnionNullValue(t *testing.T) { + // A union member with a null value is skipped entirely; the member + // callback must not fire. + d := NewShapeDeserializer([]byte(`{"mcp":null}`)) + + member, _, err := readNestedUnion(d) + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + if member != "" { + t.Errorf("expected no member, got %q", member) + } +} + +func TestReadUnion_FlatUnionStillWorks(t *testing.T) { + // Regression guard: a plain (non-nested) union read. + d := NewShapeDeserializer([]byte(`{"lambda":"v"}`)) + + var member, value string + err := smithy.ReadUnion(d, testSchemaInnerUnion, func(ms *smithy.Schema) error { + member = ms.MemberName() + return d.ReadString(ms, &value) + }) + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + if member != "lambda" || value != "v" { + t.Errorf("expected lambda/v, got %q/%q", member, value) + } +}