Fix baggage.NewMember to decode the accepted value (#3226)

* Fix baggage.NewMember to decode the accepted value

`value` is decoded and stored after validating the input parameters.

Corresponding test cases are modified so that we can make sure `value` is properly encoded before creating Member.

* fix md lint

* add function document to NewMember for value encoding and decoding

* remove redundant comments and fix CHANGELOG.md

* fix wrong PR number in the changelog

* fix wrong PR number

* fix md-lint

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>

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)

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)

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) {

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)
 	}