diff --git a/CHANGELOG.md b/CHANGELOG.md index baab4d165f4..c7d998065a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed +- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3226) - Slice attributes of `attribute` package are now comparable based on their value, not instance. (#3108 #3252) - Prometheus exporter will now cumulatively sum histogram buckets. (#3281) - Export the sum of each histogram datapoint uniquely with the `go.opentelemetry.io/otel/exporters/otlpmetric` exporters. (#3284, #3293) diff --git a/baggage/baggage.go b/baggage/baggage.go index 9869fa84304..a36db8f8d85 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -250,8 +250,9 @@ type Member struct { hasData bool } -// NewMember returns a new Member from the passed arguments. An error is -// returned if the created Member would be invalid according to the W3C +// NewMember returns a new Member from the passed arguments. The key will be +// used directly while the value will be url decoded after validation. An error +// is returned if the created Member would be invalid according to the W3C // Baggage specification. func NewMember(key, value string, props ...Property) (Member, error) { m := Member{ @@ -263,7 +264,11 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err := m.validate(); err != nil { return newInvalidMember(), err } - + decodedValue, err := url.QueryUnescape(value) + if err != nil { + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) + } + m.value = decodedValue return m, nil } @@ -328,8 +333,9 @@ func parseMember(member string) (Member, error) { return Member{key: key, value: value, properties: props, hasData: true}, nil } -// validate ensures m conforms to the W3C Baggage specification, returning an -// error otherwise. +// validate ensures m conforms to the W3C Baggage specification. +// A key is just an ASCII string, but a value must be URL encoded UTF-8, +// returning an error otherwise. func (m Member) validate() error { if !m.hasData { return fmt.Errorf("%w: %q", errInvalidMember, m) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 77b219e8599..46cf7b90ac7 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -768,6 +768,23 @@ func TestNewMember(t *testing.T) { } assert.Equal(t, expected, m) + // wrong value with wrong decoding + val = "%zzzzz" + _, err = NewMember(key, val, p) + assert.ErrorIs(t, err, errInvalidValue) + + // value should be decoded + val = "%3B" + m, err = NewMember(key, val, p) + expected = Member{ + key: key, + value: ";", + properties: properties{{key: "foo", hasData: true}}, + hasData: true, + } + assert.NoError(t, err) + assert.Equal(t, expected, m) + // Ensure new member is immutable. p.key = "bar" assert.Equal(t, expected, m) @@ -784,6 +801,17 @@ func TestPropertiesValidate(t *testing.T) { assert.NoError(t, p.validate()) } +func TestMemberString(t *testing.T) { + // normal key value pair + member, _ := NewMember("key", "value") + memberStr := member.String() + assert.Equal(t, memberStr, "key=value") + // encoded key + member, _ = NewMember("key", "%3B") + memberStr = member.String() + assert.Equal(t, memberStr, "key=%3B") +} + var benchBaggage Baggage func BenchmarkNew(b *testing.B) { diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index e98230a94ea..359b899f7e4 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -17,6 +17,7 @@ package propagation_test import ( "context" "net/http" + "net/url" "strings" "testing" @@ -46,8 +47,7 @@ func (m member) Member(t *testing.T) baggage.Member { } props = append(props, p) } - - bMember, err := baggage.NewMember(m.Key, m.Value, props...) + bMember, err := baggage.NewMember(m.Key, url.QueryEscape(m.Value), props...) if err != nil { t.Fatal(err) }