From 5ed5c5b4d7a4b3b20dbe6d2bbba761fd5cd5df37 Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Fri, 23 Sep 2022 00:02:26 +0800 Subject: [PATCH 1/7] 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. --- CHANGELOG.md | 3 +++ baggage/baggage.go | 10 +++++++++- baggage/baggage_test.go | 28 ++++++++++++++++++++++++++++ propagation/baggage_test.go | 5 +++-- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89f7552df4b..8eef2b998d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixed +- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) + ### Added - The metric portion of the OpenCensus bridge (`go.opentelemetry.io/otel/bridge/opencensus`) has been reintroduced. (#3192) diff --git a/baggage/baggage.go b/baggage/baggage.go index 9869fa84304..1f2adf75b49 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -263,7 +263,15 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err := m.validate(); err != nil { return newInvalidMember(), err } - + // the accepted value of this function should be decoded + // according to the W3C Baggage value specification + // e.g. value "%3B" should be treated as its decoded form ";" + decodedValue, err := url.QueryUnescape(value) + if err != nil { + return newInvalidMember(), + fmt.Errorf("%w: %q", errInvalidValue, value) + } + m.value = decodedValue return m, nil } 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..6f6e834b462 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,8 @@ func (m member) Member(t *testing.T) baggage.Member { } props = append(props, p) } - - bMember, err := baggage.NewMember(m.Key, m.Value, props...) + // when creating a new Member, NewMember only accepted encoded string as value + bMember, err := baggage.NewMember(m.Key, url.QueryEscape(m.Value), props...) if err != nil { t.Fatal(err) } From d2ad216caa788d70bab5279b1c1e8cdacfeb046f Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Fri, 23 Sep 2022 01:12:19 +0800 Subject: [PATCH 2/7] fix md lint --- CHANGELOG.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8eef2b998d1..b7643d48907 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] -### Fixed -- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) - ### Added - The metric portion of the OpenCensus bridge (`go.opentelemetry.io/otel/bridge/opencensus`) has been reintroduced. (#3192) @@ -19,6 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Set the `MeterProvider` resource on all exported metric data. (#3218) +- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) ## [0.32.0] Revised Metric SDK (Alpha) - 2022-09-18 From e07d2d354885897f591690fa0013e9216b226281 Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Tue, 27 Sep 2022 14:28:33 +0800 Subject: [PATCH 3/7] add function document to NewMember for value encoding and decoding --- baggage/baggage.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 1f2adf75b49..3e40a93e079 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -252,7 +252,14 @@ type Member struct { // 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. +// Baggage specification and an error will be returned if any of the input +// parameters does not follow the specification. +// +// Note the character encoding of value string MUST be UTF-8[Encoding]. +// Any characters outside the baggage-octet range of characters +// MUST be percent-encoded. e.g. value ";" is not allowed. +// Instead, use its encoded form "%3B". After validation pass, value +// will be decoded and stored. "%3B" will be interpreted as ";". func NewMember(key, value string, props ...Property) (Member, error) { m := Member{ key: key, @@ -263,9 +270,8 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err := m.validate(); err != nil { return newInvalidMember(), err } - // the accepted value of this function should be decoded - // according to the W3C Baggage value specification - // e.g. value "%3B" should be treated as its decoded form ";" + // decode the input value so that it can be parsed + // and stored properly when used and propagated. decodedValue, err := url.QueryUnescape(value) if err != nil { return newInvalidMember(), From 150ea59bb5176e5f55168d2ee8506c5816030d42 Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Wed, 12 Oct 2022 17:17:27 +0800 Subject: [PATCH 4/7] remove redundant comments and fix CHANGELOG.md --- CHANGELOG.md | 5 ++++- baggage/baggage.go | 24 ++++++++---------------- propagation/baggage_test.go | 1 - 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af973a448d7..89a183430bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Upgrade `golang.org/x/sys/unix` from `v0.0.0-20210423185535-09eb48e85fd7` to `v0.0.0-20220919091848-fb04ddd9f9c8`. This addresses [GO-2022-0493](https://pkg.go.dev/vuln/GO-2022-0493). (#3235) +### Fixed + +- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) + ## [0.32.2] Metric SDK (Alpha) - 2022-10-11 ### Added @@ -54,7 +58,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Updated go.mods to point to valid versions of the sdk. (#3216) - Set the `MeterProvider` resource on all exported metric data. (#3218) -- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) ## [0.32.0] Revised Metric SDK (Alpha) - 2022-09-18 diff --git a/baggage/baggage.go b/baggage/baggage.go index 3e40a93e079..a36db8f8d85 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -250,16 +250,10 @@ 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 -// Baggage specification and an error will be returned if any of the input -// parameters does not follow the specification. -// -// Note the character encoding of value string MUST be UTF-8[Encoding]. -// Any characters outside the baggage-octet range of characters -// MUST be percent-encoded. e.g. value ";" is not allowed. -// Instead, use its encoded form "%3B". After validation pass, value -// will be decoded and stored. "%3B" will be interpreted as ";". +// 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{ key: key, @@ -270,12 +264,9 @@ func NewMember(key, value string, props ...Property) (Member, error) { if err := m.validate(); err != nil { return newInvalidMember(), err } - // decode the input value so that it can be parsed - // and stored properly when used and propagated. decodedValue, err := url.QueryUnescape(value) if err != nil { - return newInvalidMember(), - fmt.Errorf("%w: %q", errInvalidValue, value) + return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value) } m.value = decodedValue return m, nil @@ -342,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/propagation/baggage_test.go b/propagation/baggage_test.go index 6f6e834b462..359b899f7e4 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -47,7 +47,6 @@ func (m member) Member(t *testing.T) baggage.Member { } props = append(props, p) } - // when creating a new Member, NewMember only accepted encoded string as value bMember, err := baggage.NewMember(m.Key, url.QueryEscape(m.Value), props...) if err != nil { t.Fatal(err) From 184664cfe009105c3bb3d72af7764bd29c48c065 Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Thu, 13 Oct 2022 18:03:29 +0800 Subject: [PATCH 5/7] fix wrong PR number in the changelog --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be2e9d57acf..5252ad0774f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `sdktrace.TraceProvider.Shutdown` and `sdktrace.TraceProvider.ForceFlush` to not return error when no processor register. (#3268) +### Fixed + +- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) + ## [1.11.0/0.32.3] 2022-10-12 ### Added @@ -24,10 +28,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Upgrade `golang.org/x/sys/unix` from `v0.0.0-20210423185535-09eb48e85fd7` to `v0.0.0-20220919091848-fb04ddd9f9c8`. This addresses [GO-2022-0493](https://pkg.go.dev/vuln/GO-2022-0493). (#3235) -### Fixed - -- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3144) - ## [0.32.2] Metric SDK (Alpha) - 2022-10-11 ### Added From a86f56ea2920491d62a544f5c9992abc1e416632 Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Thu, 13 Oct 2022 23:39:07 +0800 Subject: [PATCH 6/7] fix wrong PR number --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5252ad0774f..129660bc80e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,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. (#3144) +- Fix function `baggage.NewMember` to decode the `value` parameter instead of directly use it according to the W3C specification. (#3226) ## [1.11.0/0.32.3] 2022-10-12 From 24f995d40d82ae89f8ed3d5925d9876702395e4a Mon Sep 17 00:00:00 2001 From: ReStartercc Date: Sat, 15 Oct 2022 19:53:28 +0800 Subject: [PATCH 7/7] fix md-lint --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75750a39e5c..c3dfe5ebef9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - 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) - ## [1.11.0/0.32.3] 2022-10-12 ### Added