From 1effe70e60e55aa3bd52ef227a0f1d4f37e61e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 29 Dec 2023 10:52:40 +0100 Subject: [PATCH] baggage: Precent-encoding and decoding is done only by Parse and String methods --- CHANGELOG.md | 3 ++ baggage/baggage.go | 33 +++++++----------- baggage/baggage_test.go | 58 ++++++++++++++----------------- bridge/opentracing/bridge_test.go | 24 ++++++++++--- propagation/baggage_test.go | 3 +- 5 files changed, 62 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 717a990bc7a..9d7502f346d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/baggage/baggage.go b/baggage/baggage.go index 89a0664201d..8b3dac3ffc5 100644 --- a/baggage/baggage.go +++ b/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 } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index e6664f0e0af..e4d006ecd26 100644 --- a/baggage/baggage_test.go +++ b/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") } diff --git a/bridge/opentracing/bridge_test.go b/bridge/opentracing/bridge_test.go index 7a3e384424b..11c21d79496 100644 --- a/bridge/opentracing/bridge_test.go +++ b/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. diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 8149fbaf833..3a08b0e562c 100644 --- a/propagation/baggage_test.go +++ b/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) }