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

Fix baggage.NewMember to decode the accepted value #3225

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 9 additions & 1 deletion baggage/baggage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
28 changes: 28 additions & 0 deletions baggage/baggage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand Down
5 changes: 3 additions & 2 deletions propagation/baggage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package propagation_test
import (
"context"
"net/http"
"net/url"
"strings"
"testing"

Expand Down Expand Up @@ -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)
}
Expand Down