From f09e547824d29ab7cfdc10bd72397dc0a4f178f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 14 Dec 2023 21:36:46 +0100 Subject: [PATCH 1/6] baggage: Fix escaping in Member.String --- CHANGELOG.md | 4 ++++ baggage/baggage.go | 2 +- baggage/baggage_test.go | 30 +++++++++++++++++++++++------- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2376a26e50..97625e6c229 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ 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) +### Fixed + +- Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#????) + ## [1.21.0/0.44.0] 2023-11-16 ### Removed diff --git a/baggage/baggage.go b/baggage/baggage.go index c1f06fab18c..3bff2858fe7 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -325,7 +325,7 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // specification. func (m Member) String() string { // A key is just an ASCII string, but a value is URL encoded UTF-8. - s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.QueryEscape(m.value)) + s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.PathEscape(m.value)) if len(m.properties) > 0 { s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) } diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 04395efbca8..64d7e354a26 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -382,6 +382,13 @@ func TestBaggageParse(t *testing.T) { "foo": {Value: "2"}, }, }, + { + name: "= value", + in: "key==", + want: baggage.List{ + "key": {Value: "="}, + }, + }, { name: "url encoded value", in: "key1=val%252", @@ -482,18 +489,25 @@ func TestBaggageString(t *testing.T) { }, { name: "URL encoded value", - out: "foo=1%3D1", + out: "foo=%20%22%2C%3B%5C%25%0D%0A%09", baggage: baggage.List{ - "foo": {Value: "1=1"}, + "foo": {Value: " \",;\\%\r\n\t"}, // characters to be precent-escaped according to https://www.w3.org/TR/baggage/#definition }, }, { name: "plus", - out: "foo=1%2B1", + out: "foo=1+1", baggage: baggage.List{ "foo": {Value: "1+1"}, }, }, + { + name: "equal", + out: "foo=1=1", + baggage: baggage.List{ + "foo": {Value: "1=1"}, + }, + }, { name: "single member empty value with properties", out: "foo=;red;state=on", @@ -556,8 +570,10 @@ func TestBaggageString(t *testing.T) { } for _, tc := range testcases { - b := Baggage{tc.baggage} - assert.Equal(t, tc.out, orderer(b.String())) + t.Run(tc.name, func(t *testing.T) { + b := Baggage{tc.baggage} + assert.Equal(t, orderer(b.String()), tc.out) + }) } } @@ -858,9 +874,9 @@ func TestMemberString(t *testing.T) { memberStr := member.String() assert.Equal(t, memberStr, "key=value") // encoded key - member, _ = NewMember("key", "%3B") + member, _ = NewMember("key", "%3B%20") memberStr = member.String() - assert.Equal(t, memberStr, "key=%3B") + assert.Equal(t, memberStr, "key=%3B%20") } var benchBaggage Baggage From a6dc851d3d9eaed547ebcd0277c854f3e8135854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 14 Dec 2023 21:45:53 +0100 Subject: [PATCH 2/6] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97625e6c229..767cfa4cbc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#????) +- Fix whitespace encoding of `Member.String` in `go.opentelemetry.io/otel/baggage`. (#4756) ## [1.21.0/0.44.0] 2023-11-16 From 205f78cb49f17485457a4cb9ef33688798c93465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Thu, 14 Dec 2023 21:48:32 +0100 Subject: [PATCH 3/6] Fix TestInjectBaggageToHTTPReq --- propagation/baggage_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 359b899f7e4..2aad26b8595 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -214,9 +214,9 @@ func TestInjectBaggageToHTTPReq(t *testing.T) { { name: "values with escaped chars", mems: members{ - {Key: "key2", Value: "val3=4"}, + {Key: "key2", Value: "val3,4"}, }, - wantInHeader: []string{"key2=val3%3D4"}, + wantInHeader: []string{"key2=val3%2C4"}, }, { name: "with properties", From 1f98a59cdae90d602b0f3b1f632f2b2d7b860f4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Dec 2023 09:06:52 +0100 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Tyler Yahn --- baggage/baggage.go | 4 +++- baggage/baggage_test.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/baggage/baggage.go b/baggage/baggage.go index 9b15df1c26f..5869a4ed696 100644 --- a/baggage/baggage.go +++ b/baggage/baggage.go @@ -320,7 +320,9 @@ func (m Member) Properties() []Property { return m.properties.Copy() } // String encodes Member into a string compliant with the W3C Baggage // specification. func (m Member) String() string { - // A key is just an ASCII string, but a value is URL encoded UTF-8. + // A key is just an ASCII string. A value is restricted to be + // US-ASCII characters excluding CTLs, whitespace, + // DQUOTE, comma, semicolon, and backslash. s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.PathEscape(m.value)) if len(m.properties) > 0 { s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String()) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 8b0e83dacc3..02f037fdcdd 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -493,7 +493,7 @@ func TestBaggageString(t *testing.T) { }, }, { - name: "URL encoded value", + name: "Encoded value", out: "foo=%20%22%2C%3B%5C%25%0D%0A%09", baggage: baggage.List{ "foo": {Value: " \",;\\%\r\n\t"}, // characters to be precent-escaped according to https://www.w3.org/TR/baggage/#definition From f882da5d00e7111d07aa6ab4970dde44c5d72303 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Dec 2023 09:55:23 +0100 Subject: [PATCH 5/6] Update encoded value test case --- baggage/baggage_test.go | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 02f037fdcdd..4617bcb1c25 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -494,9 +494,28 @@ func TestBaggageString(t *testing.T) { }, { name: "Encoded value", - out: "foo=%20%22%2C%3B%5C%25%0D%0A%09", + // Allowed value characters are: + // + // %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E + // + // Meaning, US-ASCII characters excluding CTLs, whitespace, + // DQUOTE, comma, semicolon, and backslash. All excluded + // characters need to be percent encoded. + // Ideally, the want result is: + // out: "foo=%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20!%22#$%25&'()*+%2C-./0123456789:%3B<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[%5C]^_%60abcdefghijklmnopqrstuvwxyz{|}~%7F", + // However, the following characters are escaped: + // !#'()*/<>?[]^{|} + // It is not necessary, but still provides a correct result. + out: "foo=%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20%21%22%23$%25&%27%28%29%2A+%2C-.%2F0123456789:%3B%3C=%3E%3F@ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~%7F", baggage: baggage.List{ - "foo": {Value: " \",;\\%\r\n\t"}, // characters to be precent-escaped according to https://www.w3.org/TR/baggage/#definition + "foo": {Value: func() string { + // All US-ASCII characters. + b := [128]byte{} + for i := range b { + b[i] = byte(i) + } + return string(b[:]) + }()}, }, }, { @@ -577,7 +596,7 @@ func TestBaggageString(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { b := Baggage{tc.baggage} - assert.Equal(t, orderer(b.String()), tc.out) + assert.Equal(t, tc.out, orderer(b.String())) }) } } From bde4937aa17303b2ff0a3d56e854d7d293dc7cbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Wed, 20 Dec 2023 10:03:14 +0100 Subject: [PATCH 6/6] Add a line in comment for better readability --- baggage/baggage_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go index 4617bcb1c25..19b2c6e9459 100644 --- a/baggage/baggage_test.go +++ b/baggage/baggage_test.go @@ -501,6 +501,7 @@ func TestBaggageString(t *testing.T) { // Meaning, US-ASCII characters excluding CTLs, whitespace, // DQUOTE, comma, semicolon, and backslash. All excluded // characters need to be percent encoded. + // // Ideally, the want result is: // out: "foo=%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20!%22#$%25&'()*+%2C-./0123456789:%3B<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[%5C]^_%60abcdefghijklmnopqrstuvwxyz{|}~%7F", // However, the following characters are escaped: