Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

baggage: Fix escaping in Member.String #4756

Merged
merged 11 commits into from
Dec 21, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### 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)

## [1.21.0/0.44.0] 2023-11-16

Expand Down
2 changes: 1 addition & 1 deletion baggage/baggage.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,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.
pellared marked this conversation as resolved.
Show resolved Hide resolved
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())
}
Expand Down
30 changes: 23 additions & 7 deletions baggage/baggage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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%2C",
Expand Down Expand Up @@ -487,18 +494,25 @@ func TestBaggageString(t *testing.T) {
},
{
name: "URL encoded value",
pellared marked this conversation as resolved.
Show resolved Hide resolved
out: "foo=1%3D1",
out: "foo=%20%22%2C%3B%5C%25%0D%0A%09",
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
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
pellared marked this conversation as resolved.
Show resolved Hide resolved
},
},
{
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",
Expand Down Expand Up @@ -561,8 +575,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)
})
}
}

Expand Down Expand Up @@ -863,9 +879,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
Expand Down
4 changes: 2 additions & 2 deletions propagation/baggage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down