From 43bd47de6eff944f7f3bdc7d0b768502b9329d5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Robert=20Paj=C4=85k?= <pellared@hotmail.com>
Date: Tue, 19 Dec 2023 14:38:58 +0100
Subject: [PATCH] baggage: Fix Parse to validate member value before
 percent-decoding (#4755)

---
 CHANGELOG.md                |  4 +++
 baggage/baggage.go          | 24 ++++++++---------
 baggage/baggage_test.go     |  9 +++++--
 propagation/baggage_test.go | 51 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 71 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3c0afb9c957..d566dbf251a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,6 +26,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 `Parse` in `go.opentelemetry.io/otel/baggage` to validate member value before percent-decoding. (#4755)
+
 ## [1.21.0/0.44.0] 2023-11-16
 
 ### Removed
diff --git a/baggage/baggage.go b/baggage/baggage.go
index c1f06fab18c..12b9ca6f39c 100644
--- a/baggage/baggage.go
+++ b/baggage/baggage.go
@@ -254,11 +254,7 @@ func parseMember(member string) (Member, error) {
 		return newInvalidMember(), fmt.Errorf("%w: %d", errMemberBytes, n)
 	}
 
-	var (
-		key, value string
-		props      properties
-	)
-
+	var props properties
 	keyValue, properties, found := strings.Cut(member, propertyDelimiter)
 	if found {
 		// Parse the member properties.
@@ -279,19 +275,19 @@ func parseMember(member string) (Member, error) {
 	}
 	// "Leading and trailing whitespaces are allowed but MUST be trimmed
 	// when converting the header into a data structure."
-	key = strings.TrimSpace(k)
-	var err error
-	value, err = url.PathUnescape(strings.TrimSpace(v))
-	if err != nil {
-		return newInvalidMember(), fmt.Errorf("%w: %q", err, value)
-	}
+	key := strings.TrimSpace(k)
 	if !validateKey(key) {
 		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidKey, key)
 	}
-	if !validateValue(value) {
-		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, value)
-	}
 
+	val := strings.TrimSpace(v)
+	if !validateValue(val) {
+		return newInvalidMember(), fmt.Errorf("%w: %q", errInvalidValue, v)
+	}
+	value, err := url.PathUnescape(val)
+	if err != nil {
+		return newInvalidMember(), fmt.Errorf("%w: %v", errInvalidValue, err)
+	}
 	return Member{key: key, value: value, properties: props, hasData: true}, nil
 }
 
diff --git a/baggage/baggage_test.go b/baggage/baggage_test.go
index 04395efbca8..82c06949945 100644
--- a/baggage/baggage_test.go
+++ b/baggage/baggage_test.go
@@ -384,9 +384,9 @@ func TestBaggageParse(t *testing.T) {
 		},
 		{
 			name: "url encoded value",
-			in:   "key1=val%252",
+			in:   "key1=val%252%2C",
 			want: baggage.List{
-				"key1": {Value: "val%2"},
+				"key1": {Value: "val%2,"},
 			},
 		},
 		{
@@ -414,6 +414,11 @@ func TestBaggageParse(t *testing.T) {
 			in:   "foo=\\",
 			err:  errInvalidValue,
 		},
+		{
+			name: "invalid member: improper url encoded value",
+			in:   "key1=val%",
+			err:  errInvalidValue,
+		},
 		{
 			name: "invalid property: no key",
 			in:   "foo=1;=v",
diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go
index 359b899f7e4..a29e412b9d6 100644
--- a/propagation/baggage_test.go
+++ b/propagation/baggage_test.go
@@ -47,7 +47,7 @@ func (m member) Member(t *testing.T) baggage.Member {
 		}
 		props = append(props, p)
 	}
-	bMember, err := baggage.NewMember(m.Key, url.QueryEscape(m.Value), props...)
+	bMember, err := baggage.NewMember(m.Key, url.PathEscape(m.Value), props...)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -248,6 +248,55 @@ func TestInjectBaggageToHTTPReq(t *testing.T) {
 	}
 }
 
+func TestBaggageInjectExtractRoundtrip(t *testing.T) {
+	propagator := propagation.Baggage{}
+	tests := []struct {
+		name string
+		mems members
+	}{
+		{
+			name: "two simple values",
+			mems: members{
+				{Key: "key1", Value: "val1"},
+				{Key: "key2", Value: "val2"},
+			},
+		},
+		{
+			name: "values with escaped chars",
+			mems: members{
+				{Key: "key1", Value: "val3=4"},
+				{Key: "key2", Value: "mess,me%up"},
+			},
+		},
+		{
+			name: "with properties",
+			mems: members{
+				{Key: "key1", Value: "val1"},
+				{
+					Key:   "key2",
+					Value: "val2",
+					Properties: []property{
+						{Key: "prop", Value: "1"},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			b := tt.mems.Baggage(t)
+			req, _ := http.NewRequest("GET", "http://example.com", nil)
+			ctx := baggage.ContextWithBaggage(context.Background(), b)
+			propagator.Inject(ctx, propagation.HeaderCarrier(req.Header))
+
+			ctx = propagator.Extract(context.Background(), propagation.HeaderCarrier(req.Header))
+			got := baggage.FromContext(ctx)
+
+			assert.Equal(t, b, got)
+		})
+	}
+}
+
 func TestBaggagePropagatorGetAllKeys(t *testing.T) {
 	var propagator propagation.Baggage
 	want := []string{"baggage"}