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 #3226

Merged
merged 16 commits into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from 9 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
MrAlias marked this conversation as resolved.
Show resolved Hide resolved

## [0.32.2] Metric SDK (Alpha) - 2022-10-11

### Added
Expand Down
16 changes: 11 additions & 5 deletions baggage/baggage.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,9 @@ 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
// 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{
Expand All @@ -263,7 +264,11 @@ func NewMember(key, value string, props ...Property) (Member, error) {
if err := m.validate(); err != nil {
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
return newInvalidMember(), err
}

decodedValue, err := url.QueryUnescape(value)
if err != nil {
return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
}
m.value = decodedValue
return m, nil
}

Expand Down Expand Up @@ -328,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)
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
4 changes: 2 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,7 @@ func (m member) Member(t *testing.T) baggage.Member {
}
props = append(props, p)
}

bMember, err := baggage.NewMember(m.Key, m.Value, props...)
bMember, err := baggage.NewMember(m.Key, url.QueryEscape(m.Value), props...)
if err != nil {
t.Fatal(err)
}
Expand Down