diff --git a/CHANGELOG.md b/CHANGELOG.md index 13251aed5a6..4cfd3d8feb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Fix race condition in `FixedSizeReservoir` in `go.opentelemetry.io/otel/sdk/metric/exemplar` by reverting #7447. (#8249) - Fix counting of spans and logs in self-observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`, `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`, `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`, and `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#8254) - Drop conflicting scope attributes named `name`, `version`, or `schema_url` from metric labels in `go.opentelemetry.io/otel/exporters/prometheus`, preserving the dedicated `otel_scope_name`, `otel_scope_version`, and `otel_scope_schema_url` labels. (#8264) +- Enforce the 8192-byte baggage size limit during extraction/parsing, changing behavior when the limit is exceeded in `go.opentelemetry.io/otel/baggage` and `go.opentelemetry.io/otel/propagation`. (#8222) diff --git a/baggage/baggage.go b/baggage/baggage.go index 878ffbe43a5..b290c6d6cc8 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -14,6 +14,10 @@ import ( ) const ( + maxParseErrors = 5 + + // W3C Baggage specification limits. + // https://www.w3.org/TR/baggage/#limits maxMembers = 64 maxBytesPerBaggageString = 8192 @@ -493,9 +497,15 @@ func New(members ...Member) (Baggage, error) { // from the W3C Baggage specification which allows duplicate list-members, but // conforms to the OpenTelemetry Baggage specification. // -// If the baggage-string exceeds the maximum allowed members (64) or bytes -// (8192), members are dropped until the limits are satisfied and an error is -// returned along with the partial result. +// If the raw baggage-string exceeds the maximum allowed bytes (8192), an +// empty Baggage and an error are returned. +// +// Otherwise, members are parsed left-to-right and accumulated until one of +// the following conditions is reached, at which point parsing stops and an +// error is returned alongside the partial result: +// - accepting the next member would cause the encoded baggage to exceed +// 8192 bytes, or +// - the baggage already contains 64 distinct keys. // // Invalid members are skipped and the error is returned along with the // partial result containing the valid members. @@ -504,9 +514,14 @@ func Parse(bStr string) (Baggage, error) { return Baggage{}, nil } + if n := len(bStr); n > maxBytesPerBaggageString { + return Baggage{}, fmt.Errorf("%w: %d", errBaggageBytes, n) + } + b := make(baggage.List) sizes := make(map[string]int) // Track per-key byte sizes var totalBytes int + var parseErrors int var truncateErr error for memberStr := range strings.SplitSeq(bStr, listDelimiter) { // Check member count limit. @@ -517,7 +532,10 @@ func Parse(bStr string) (Baggage, error) { m, err := parseMember(memberStr) if err != nil { - truncateErr = errors.Join(truncateErr, err) + parseErrors++ + if parseErrors <= maxParseErrors { + truncateErr = errors.Join(truncateErr, err) + } continue // skip invalid member, keep processing } @@ -553,6 +571,10 @@ func Parse(bStr string) (Baggage, error) { totalBytes = newTotalBytes } + if dropped := parseErrors - maxParseErrors; dropped > 0 { + truncateErr = errors.Join(truncateErr, fmt.Errorf("and %d more invalid member(s)", dropped)) + } + if len(b) == 0 { return Baggage{}, truncateErr } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index c8c3c98918a..cf1eb3091d5 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -528,8 +528,7 @@ func TestBaggageParse(t *testing.T) { { name: "invalid baggage string: too large", in: tooLarge, - // tooLarge is a single key without "=", so parseMember fails - err: errInvalidMember, + err: errBaggageBytes, }, { name: "baggage string with too many members keeps first 64", @@ -544,31 +543,26 @@ func TestBaggageParse(t *testing.T) { err: errMemberNumber, }, { - name: "baggage string exceeds byte limit returns partial result", + name: "baggage string at max size is accepted", in: func() string { - // Create members that collectively exceed maxBytesPerBaggageString. - // Each member: "kN=" + value. We use values large enough that - // a few members fit but the total exceeds 8192 bytes. + // Create a baggage string of exactly maxBytesPerBaggageString. + // 3 members: "k0=" + 2727v + "," + "k1=" + 2727v + "," + "k2=" + 2727v + // = 3*(3+2727) + 2 = 8192 + val := strings.Repeat("v", 2727) var parts []string - val := strings.Repeat("v", 2000) - for i := range 10 { + for i := range 3 { parts = append(parts, fmt.Sprintf("k%d=%s", i, val)) } return strings.Join(parts, ",") }(), want: func() baggage.List { - // Only members that fit within 8192 bytes should be kept. - // Each member is ~2003 bytes ("kN=" + 2000 "v"s), plus comma. - // 4 members = 4*2003 + 3 commas = 8015 bytes (fits). - // 5 members = 5*2003 + 4 commas = 10019 bytes (exceeds). b := make(baggage.List) - val := strings.Repeat("v", 2000) - for i := range 4 { + val := strings.Repeat("v", 2727) + for i := range 3 { b[fmt.Sprintf("k%d", i)] = baggage.Item{Value: val} } return b }(), - err: errBaggageBytes, }, { name: "percent-encoded octet sequences do not match the UTF-8 encoding scheme", @@ -1272,3 +1266,57 @@ func BenchmarkMemberString(b *testing.B) { _ = member.String() } } + +func BenchmarkParseOversized(b *testing.B) { + // 1MB oversized baggage string. + oversized := strings.Repeat("k=v,", 250000) + + b.ReportAllocs() + + for b.Loop() { + _, _ = Parse(oversized) + } +} + +func TestParseErrorCap(t *testing.T) { + // Build a baggage string with many invalid members (no '=' delimiter). + // All within the 8192 byte limit. + var parts []string + for i := range 20 { + parts = append(parts, fmt.Sprintf("bad%d", i)) + } + // Add one valid member so the baggage is not empty. + parts = append(parts, "good=val") + bStr := strings.Join(parts, ",") + + b, err := Parse(bStr) + assert.ErrorIs(t, err, errInvalidMember) + assert.Equal(t, 1, b.Len(), "should return the valid member") + + // Count the number of joined errors. + errs := err.Error() + invalidCount := strings.Count(errs, "invalid baggage list-member") + assert.Equal(t, maxParseErrors, invalidCount, + "should cap individual parse errors at maxParseErrors") + assert.Contains(t, errs, "and 15 more invalid member(s)") +} + +func TestParseErrorCapAllInvalid(t *testing.T) { + // All members invalid, no valid members. Exercises the len(b)==0 + // return path with a capped error message. + var parts []string + for i := range 20 { + parts = append(parts, fmt.Sprintf("bad%d", i)) + } + bStr := strings.Join(parts, ",") + + b, err := Parse(bStr) + assert.ErrorIs(t, err, errInvalidMember) + assert.Equal(t, 0, b.Len(), "should return empty baggage") + + errs := err.Error() + invalidCount := strings.Count(errs, "invalid baggage list-member") + assert.Equal(t, maxParseErrors, invalidCount, + "should cap individual parse errors at maxParseErrors") + assert.Contains(t, errs, "and 15 more invalid member(s)") +} diff --git a/propagation/baggage.go b/propagation/baggage.go index 2ecca3fed1e..04a1faa64fc 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -5,6 +5,8 @@ package propagation // import "go.opentelemetry.io/otel/propagation" import ( "context" + "errors" + "fmt" "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/internal/errorhandler" @@ -13,9 +15,12 @@ import ( const ( baggageHeader = "baggage" + maxParseErrors = 5 + // W3C Baggage specification limits. // https://www.w3.org/TR/baggage/#limits - maxMembers = 64 + maxMembers = 64 + maxBytesPerBaggageString = 8192 ) // Baggage is a propagator that supports the W3C Baggage format. @@ -72,10 +77,34 @@ func extractMultiBaggage(parent context.Context, carrier ValuesGetter) context.C } var members []baggage.Member - for _, bStr := range bVals { + var totalBytes int + var parseErrors int + var truncateErr error + for i, bStr := range bVals { + if i > 0 { + totalBytes++ // comma separator between combined header values + } + totalBytes += len(bStr) + if totalBytes > maxBytesPerBaggageString { + // Per the W3C Baggage spec, the byte limit applies to the + // combination of all baggage headers, not each header + // individually. Mirror the single-header behavior of + // reporting the error and returning the parent context + // with no baggage attached. + errorhandler.GetErrorHandler().Handle(fmt.Errorf( + "baggage: aggregate header size %d exceeds %d byte limit", + totalBytes, + maxBytesPerBaggageString, + )) + return parent + } + currBag, err := baggage.Parse(bStr) if err != nil { - errorhandler.GetErrorHandler().Handle(err) + parseErrors++ + if parseErrors <= maxParseErrors { + truncateErr = errors.Join(truncateErr, err) + } } if currBag.Len() == 0 { continue @@ -86,10 +115,18 @@ func extractMultiBaggage(parent context.Context, carrier ValuesGetter) context.C } } + if dropped := parseErrors - maxParseErrors; dropped > 0 { + truncateErr = errors.Join(truncateErr, fmt.Errorf("and %d more error(s)", dropped)) + } + b, err := baggage.New(members...) if err != nil { - errorhandler.GetErrorHandler().Handle(err) + truncateErr = errors.Join(truncateErr, err) } + if truncateErr != nil { + errorhandler.GetErrorHandler().Handle(truncateErr) + } + if b.Len() == 0 { return parent } diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 7666c73fdc0..a5f0871b063 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -12,10 +12,13 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/propagation" ) +const maxBytesPerBaggageString = 8192 + type property struct { Key, Value string } @@ -161,18 +164,29 @@ func generateMembers(n int, prefix string) members { return m } +// generateFixedWidthBaggageHeader creates a baggage header where every member +// has the same byte length, ensuring deterministic String() size regardless of +// which members baggage.New() keeps after map-order truncation. +// Each member is "prefixNN=vNN" with zero-padded indices. +func generateFixedWidthBaggageHeader(n int, prefix string) string { + parts := make([]string, n) + for i := range parts { + parts[i] = fmt.Sprintf("%s%02d=v%02d", prefix, i, i) + } + return strings.Join(parts, ",") +} + func TestExtractValidMultipleBaggageHeaders(t *testing.T) { // W3C Baggage spec limits: https://www.w3.org/TR/baggage/#limits const maxMembers = 64 - const maxBytesPerBaggageString = 8192 prop := propagation.TextMapPropagator(propagation.Baggage{}) tests := []struct { - name string - headers []string - want members - wantCount int // Used when want is nil and we only care about count. - wantMaxBytes int // Used to check that baggage size doesn't exceed limit. + name string + headers []string + want members + wantCount int // Used when want is nil and we only care about count. + wantBytes int // Used to check exact baggage size when want is nil. }{ { name: "non conflicting headers", @@ -239,13 +253,13 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { { name: "multiple headers exceeds total max members limit keeps 64", headers: []string{ - generateBaggageHeader(maxMembers/2, "a"), - generateBaggageHeader(maxMembers/2, "b"), - generateBaggageHeader(1, "c"), + generateFixedWidthBaggageHeader(maxMembers/2, "a"), + generateFixedWidthBaggageHeader(maxMembers/2, "b"), + generateFixedWidthBaggageHeader(1, "c"), }, - want: nil, // Non-deterministic truncation by baggage.New() - wantCount: maxMembers, - wantMaxBytes: maxBytesPerBaggageString, + want: nil, // Non-deterministic truncation by baggage.New() + wantCount: maxMembers, + wantBytes: maxMembers*7 + maxMembers - 1, // 64 uniform "xNN=vNN" members + 63 commas }, { name: "single header at max bytes limit", @@ -255,19 +269,17 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { }, }, { - name: "single header exceeds max bytes limit drops oversized member", + name: "single header exceeds max bytes limit will be dropped entirely", headers: []string{"k=" + strings.Repeat("v", maxBytesPerBaggageString-1)}, want: members{}, }, { - name: "multiple headers exceed total max bytes keeps one that fits", + name: "multiple headers exceed total max bytes drops everything", headers: []string{ "k=" + strings.Repeat("v", maxBytesPerBaggageString-2), "y=" + strings.Repeat("v", maxBytesPerBaggageString-2), }, - want: nil, // Non-deterministic: either k or y will be kept - wantCount: 1, // Only one member fits - wantMaxBytes: maxBytesPerBaggageString, + want: members{}, }, { name: "multiple headers within total max bytes", @@ -281,6 +293,39 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { {Key: "y", Value: strings.Repeat("v", maxBytesPerBaggageString/2-2-1)}, }, }, + { + name: "empty headers between non-empty do count comma separators", + headers: []string{ + "k=" + strings.Repeat("v", maxBytesPerBaggageString/2-2), + "", + "", + // The comma as the separator of member would take 1 byte. + "y=" + strings.Repeat("v", maxBytesPerBaggageString/2-2-1), + }, + want: members{}, + }, + { + name: "empty headers before non-empty do count comma separators", + headers: []string{ + // The comma as the separator of member would take 1 byte. + "", + "k=" + strings.Repeat("v", maxBytesPerBaggageString/2-2), + // The comma as the separator of member would take 1 byte. + "y=" + strings.Repeat("v", maxBytesPerBaggageString/2-2-1), + }, + want: members{}, + }, + { + name: "multiple headers exceed total max bytes due to comma separator", + headers: []string{ + // Two equal halves: each is exactly maxBytesPerBaggageString/2 bytes. + // Without the comma separator: 4096 + 4096 = 8192 (at limit). + // With the comma separator: 4096 + 1 + 4096 = 8193 (over limit). + "k=" + strings.Repeat("v", maxBytesPerBaggageString/2-2), + "y=" + strings.Repeat("v", maxBytesPerBaggageString/2-2), + }, + want: members{}, + }, { name: "many headers exceeding member limit caps collection early", headers: func() []string { @@ -289,25 +334,12 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { // New() truncates to exactly maxMembers. h := make([]string, 100) for i := range h { - h[i] = generateBaggageHeader(10, fmt.Sprintf("h%d_k", i)) + h[i] = generateFixedWidthBaggageHeader(10, fmt.Sprintf("h%02d_k", i)) } return h }(), - wantCount: maxMembers, - wantMaxBytes: maxBytesPerBaggageString, - }, - { - name: "skips large member that exceeds byte limit and continues", - headers: []string{ - "small1=v1,small2=v2", - "large=" + strings.Repeat("x", maxBytesPerBaggageString), - "small3=v3", - }, - want: members{ - {Key: "small1", Value: "v1"}, - {Key: "small2", Value: "v2"}, - {Key: "small3", Value: "v3"}, - }, + wantCount: maxMembers, + wantBytes: maxMembers*11 + maxMembers - 1, // 64 uniform "hNN_kNN=vNN" members + 63 commas }, } @@ -324,10 +356,9 @@ func TestExtractValidMultipleBaggageHeaders(t *testing.T) { if tt.want != nil { expected := tt.want.Baggage(t) assert.Equal(t, expected, got) - } else if tt.wantCount > 0 { - // If only count is specified, verify count and byte limit + } else { assert.Equal(t, tt.wantCount, got.Len(), "expected member count") - assert.LessOrEqual(t, len(got.String()), tt.wantMaxBytes, "baggage size exceeds limit") + assert.Len(t, got.String(), tt.wantBytes, "expected baggage size") } }) } @@ -498,3 +529,105 @@ func TestBaggagePropagatorGetAllKeys(t *testing.T) { t.Errorf("GetAllKeys: -got +want %s", diff) } } + +func TestExtractOversizedSingleBaggageHeader(t *testing.T) { + prop := propagation.Baggage{} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://example.com", http.NoBody) + // Set a single baggage header exceeding 8192 bytes. + req.Header.Set("baggage", "key="+strings.Repeat("v", maxBytesPerBaggageString)) + + ctx := prop.Extract(t.Context(), propagation.HeaderCarrier(req.Header)) + got := baggage.FromContext(ctx) + assert.Equal(t, 0, got.Len(), "oversized header should result in empty baggage") +} + +type errHandler struct { + err error +} + +func (e *errHandler) Handle(err error) { e.err = err } + +func TestExtractManyBaggageHeader(t *testing.T) { + tests := []struct { + name string + headers func() []string + want func() members + wantErrStr []string + }{ + { + name: "aggregate byte budget exceeded drops everything", + headers: func() []string { + // 100 headers, each ~195 bytes. Total: ~19.5KB, well over 8192. + h := make([]string, 100) + for i := range 100 { + h[i] = fmt.Sprintf("k%d=%s", i, strings.Repeat("v", 190)) + } + return h + }, + want: func() members { + return members{} + }, + wantErrStr: []string{"aggregate header size 8374 exceeds 8192 byte limit"}, + }, + { + name: "too many invalid headers triggers error cap", + headers: func() []string { + // 10 invalid headers (no '=' delimiter) followed by 1 valid. + // Each invalid header produces 1 parse error from baggage.Parse. + // maxParseErrors=5, so 5 errors are kept and 5 are summarized. + h := make([]string, 11) + for i := range 10 { + h[i] = fmt.Sprintf("bad%d", i) + } + h[10] = "good=val" + return h + }, + want: func() members { + return members{{Key: "good", Value: "val"}} + }, + wantErrStr: []string{"invalid baggage list-member", "and 5 more error(s)"}, + }, + { + name: "single header with many invalid members preserves inner error", + headers: func() []string { + // One header with 10 invalid members. baggage.Parse caps its own + // joined error at maxParseErrors=5 and appends its own summary. + // extractMultiBaggage must count this as a single failing header + // so the inner error is still attached to the outer error chain. + invalid := make([]string, 10) + for i := range invalid { + invalid[i] = fmt.Sprintf("bad%d", i) + } + return []string{strings.Join(invalid, ",") + ",good=val"} + }, + want: func() members { + return members{{Key: "good", Value: "val"}} + }, + wantErrStr: []string{"invalid baggage list-member", "and 5 more invalid member(s)"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + originalErrorHandler := otel.GetErrorHandler() + eh := &errHandler{} + otel.SetErrorHandler(eh) + t.Cleanup(func() { otel.SetErrorHandler(originalErrorHandler) }) + + prop := propagation.Baggage{} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://example.com", http.NoBody) + for _, h := range tt.headers() { + req.Header.Add("baggage", h) + } + + ctx := prop.Extract(t.Context(), propagation.HeaderCarrier(req.Header)) + got := baggage.FromContext(ctx) + + assert.Equal(t, tt.want().Baggage(t), got) + assert.Error(t, eh.err) + for _, s := range tt.wantErrStr { + assert.Contains(t, eh.err.Error(), s) + } + }) + } +}