baggage: Precent-encoding and decoding is done only by Parse and Stri…

…ng methods

CHANGELOG.md

@@ -30,12 +30,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Improve `go.opentelemetry.io/otel/propagation.TraceContext`'s performance. (#4721)
 - Improve `go.opentelemetry.io/otel/baggage` performance. (#4743)
 - `Member.String` in `go.opentelemetry.io/otel/baggage` percent-encodes only when necessary. (#4775)
+- `NewMember` and `NewKeyValueProperty` in `go.opentelemetry.io/otel/baggage` does no longer require percent-encoding of values. Make sure to not precent-encode the value yourself (e.g. by using `url.PathEscape`) before passing it to `NewMember` and `NewKeyValueProperty`. `Member.String` and `Property.String` are encoding the value when necessary. (#4804)
+- `Property.Value` in `go.opentelemetry.io/otel/baggage` now returns a vanilla string instead of a percent-encoded value. (#4804)
 
 ### Fixed
 
 - Fix `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755)
 - Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#4756)
 - Fix baggage item key so that it is not canonicalized in `go.opentelemetry.io/otel/bridge/opentracing`. (#4776)
+- Fix `go.opentelemetry.io/otel/bridge/opentracing` to properly handle baggage values that requires escaping during propagation. (#4804)
 
 ## [1.21.0/0.44.0] 2023-11-16
 

baggage/baggage.go

@@ -71,9 +71,6 @@ func NewKeyValueProperty(key, value string) (Property, error) {
 	if !validateKey(key) {
 		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidKey, key)
 	}
-	if !validateValue(value) {
-		return newInvalidProperty(), fmt.Errorf("%w: %q", errInvalidValue, value)
-	}
 
 	p := Property{
 		key:      key,
@@ -113,9 +110,6 @@ func (p Property) validate() error {
 	if !validateKey(p.key) {
 		return errFunc(fmt.Errorf("%w: %q", errInvalidKey, p.key))
 	}
-	if p.hasValue && !validateValue(p.value) {
-		return errFunc(fmt.Errorf("%w: %q", errInvalidValue, p.value))
-	}
 	if !p.hasValue && p.value != "" {
 		return errFunc(errors.New("inconsistent value"))
 	}
@@ -138,7 +132,7 @@ func (p Property) Value() (string, bool) {
 // specification.
 func (p Property) String() string {
 	if p.hasValue {
-		return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, p.value)
+		return fmt.Sprintf("%s%s%v", p.key, keyValueDelimiter, valueEscape(p.value))
 	}
 	return p.key
 }
@@ -220,8 +214,7 @@ type Member struct {
 	hasData bool
 }
 
-// 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
+// NewMember returns a new Member from the passed arguments. 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) {
@@ -234,11 +227,6 @@ func NewMember(key, value string, props ...Property) (Member, error) {
 	if err := m.validate(); err != nil {
 		return newInvalidMember(), err
 	}
-	decodedValue, err := url.PathUnescape(value)
-	if err != nil {
-		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
-	}
-	m.value = decodedValue
 	return m, nil
 }
 
@@ -284,6 +272,8 @@ func parseMember(member string) (Member, error) {
 	if !validateValue(val) {
 		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v)
 	}
+
+	// Decode a precent-encoded value.
 	value, err := url.PathUnescape(val)
 	if err != nil {
 		return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidValue, err)
@@ -292,8 +282,7 @@ func parseMember(member string) (Member, error) {
 }
 
 // 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.
+// A key must be an ASCII string, returning an error otherwise.
 func (m Member) validate() error {
 	if !m.hasData {
 		return fmt.Errorf("%w: %q", errInvalidMember, m)
@@ -302,9 +291,6 @@ func (m Member) validate() error {
 	if !validateKey(m.key) {
 		return fmt.Errorf("%w: %q", errInvalidKey, m.key)
 	}
-	if !validateValue(m.value) {
-		return fmt.Errorf("%w: %q", errInvalidValue, m.value)
-	}
 	return m.properties.validate()
 }
 
@@ -595,10 +581,17 @@ func parsePropertyInternal(s string) (p Property, ok bool) {
 		return
 	}
 
+	// Decode a precent-encoded value.
+	value, err := url.PathUnescape(s[valueStart:valueEnd])
+	if err != nil {
+		return
+	}
+
 	ok = true
 	p.key = s[keyStart:keyEnd]
 	p.hasValue = true
-	p.value = s[valueStart:valueEnd]
+
+	p.value = value
 	return
 }
 

baggage/baggage_test.go

@@ -147,13 +147,9 @@ func TestNewKeyValueProperty(t *testing.T) {
 	assert.ErrorIs(t, err, errInvalidKey)
 	assert.Equal(t, Property{}, p)
 
-	p, err = NewKeyValueProperty("key", ";")
-	assert.ErrorIs(t, err, errInvalidValue)
-	assert.Equal(t, Property{}, p)
-
-	p, err = NewKeyValueProperty("key", "value")
+	p, err = NewKeyValueProperty("key", "Witaj Świecie!")
 	assert.NoError(t, err)
-	assert.Equal(t, Property{key: "key", value: "value", hasValue: true}, p)
+	assert.Equal(t, Property{key: "key", value: "Witaj Świecie!", hasValue: true}, p)
 }
 
 func TestPropertyValidate(t *testing.T) {
@@ -163,13 +159,10 @@ func TestPropertyValidate(t *testing.T) {
 	p.key = "k"
 	assert.NoError(t, p.validate())
 
-	p.value = ";"
+	p.value = "v"
 	assert.EqualError(t, p.validate(), "invalid property: inconsistent value")
 
 	p.hasValue = true
-	assert.ErrorIs(t, p.validate(), errInvalidValue)
-
-	p.value = "v"
 	assert.NoError(t, p.validate())
 }
 
@@ -397,6 +390,15 @@ func TestBaggageParse(t *testing.T) {
 				"key1": {Value: "val%2,"},
 			},
 		},
+		{
+			name: "encoded property",
+			in:   "key1=;bar=val%252%2C",
+			want: baggage.List{
+				"key1": {
+					Properties: []baggage.Property{{Key: "bar", HasValue: true, Value: "val%2,"}},
+				},
+			},
+		},
 		{
 			name: "encoded UTF-8 string",
 			in:   "foo=%C4%85%C5%9B%C4%87",
@@ -528,6 +530,17 @@ func TestBaggageString(t *testing.T) {
 				"foo": {Value: "ąść"},
 			},
 		},
+		{
+			name: "Encoded property value",
+			out:  "foo=;bar=%20",
+			baggage: baggage.List{
+				"foo": {
+					Properties: []baggage.Property{
+						{Key: "bar", Value: " ", HasValue: true},
+					},
+				},
+			},
+		},
 		{
 			name: "plus",
 			out:  "foo=1+1",
@@ -846,9 +859,6 @@ func TestMemberValidation(t *testing.T) {
 	assert.ErrorIs(t, m.validate(), errInvalidKey)
 
 	m.key, m.value = "k", "\\"
-	assert.ErrorIs(t, m.validate(), errInvalidValue)
-
-	m.value = "v"
 	assert.NoError(t, m.validate())
 }
 
@@ -869,23 +879,6 @@ 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,
-	}
-	assert.NoError(t, err)
-	assert.Equal(t, expected, m)
-
 	// Ensure new member is immutable.
 	p.key = "bar"
 	assert.Equal(t, expected, m)
@@ -907,8 +900,9 @@ func TestMemberString(t *testing.T) {
 	member, _ := NewMember("key", "value")
 	memberStr := member.String()
 	assert.Equal(t, memberStr, "key=value")
-	// encoded key
-	member, _ = NewMember("key", "%3B%20")
+
+	// encoded value
+	member, _ = NewMember("key", "; ")
 	memberStr = member.String()
 	assert.Equal(t, memberStr, "key=%3B%20")
 }

bridge/opentracing/bridge_test.go

@@ -578,17 +578,17 @@ func TestBridgeSpanContextPromotedMethods(t *testing.T) {
 func TestBridgeCarrierBaggagePropagation(t *testing.T) {
 	carriers := []struct {
 		name    string
-		carrier interface{}
+		factory func() interface{}
 		format  ot.BuiltinFormat
 	}{
 		{
 			name:    "TextMapCarrier",
-			carrier: ot.TextMapCarrier{},
+			factory: func() interface{} { return ot.TextMapCarrier{} },
 			format:  ot.TextMap,
 		},
 		{
 			name:    "HTTPHeadersCarrier",
-			carrier: ot.HTTPHeadersCarrier{},
+			factory: func() interface{} { return ot.HTTPHeadersCarrier{} },
 			format:  ot.HTTPHeaders,
 		},
 	}
@@ -619,6 +619,19 @@ func TestBridgeCarrierBaggagePropagation(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "with characters escaped by baggage propagator",
+			baggageItems: []bipBaggage{
+				{
+					key:   "space",
+					value: "Hello world!",
+				},
+				{
+					key:   "utf8",
+					value: "Świat",
+				},
+			},
+		},
 	}
 
 	for _, c := range carriers {
@@ -638,10 +651,11 @@ func TestBridgeCarrierBaggagePropagation(t *testing.T) {
 				}
 				defer span.Finish()
 
-				err := b.Inject(span.Context(), c.format, c.carrier)
+				carrier := c.factory()
+				err := b.Inject(span.Context(), c.format, carrier)
 				assert.NoError(t, err)
 
-				spanContext, err := b.Extract(c.format, c.carrier)
+				spanContext, err := b.Extract(c.format, carrier)
 				assert.NoError(t, err)
 
 				// Check baggage items.

propagation/baggage_test.go

@@ -17,7 +17,6 @@ package propagation_test
 import (
 	"context"
 	"net/http"
-	"net/url"
 	"strings"
 	"testing"
 
@@ -47,7 +46,7 @@ func (m member) Member(t *testing.T) baggage.Member {
 		}
 		props = append(props, p)
 	}
-	bMember, err := baggage.NewMember(m.Key, url.PathEscape(m.Value), props...)
+	bMember, err := baggage.NewMember(m.Key, m.Value, props...)
 	if err != nil {
 		t.Fatal(err)
 	}