Use url.PathUnescape rather than url.QueryUnescape when parsing OTLP …

…headers and resource attributes env vars (#4698) (#4699)
open-telemetry · Nov 9, 2023 · be5064a · be5064a
1 parent 0c5ebd5
commit be5064a
Show file tree

Hide file tree

Showing 13 changed files with 127 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -59,7 +59,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Fixed
 
-- Fix improper parsing of characters such us `+`, `\` by `Parse` in `go.opentelemetry.io/otel/baggage` as they were rendered as a whitespace. (#4667)
+- Fix improper parsing of characters such us `+`, `/` by `Parse` in `go.opentelemetry.io/otel/baggage` as they were rendered as a whitespace. (#4667)
+- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_RESOURCE_ATTRIBUTES` in `go.opentelemetry.io/otel/sdk/resource` as they were rendered as a whitespace. (#4699)
+- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_METRICS_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` as they were rendered as a whitespace. (#4699)
+- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_METRICS_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` as they were rendered as a whitespace. (#4699)
+- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_TRACES_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlptracegrpc` as they were rendered as a whitespace. (#4699)
+- Fix improper parsing of characters such us `+`, `/` passed via `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_EXPORTER_OTLP_TRACES_HEADERS` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlptracehttp` as they were rendered as a whitespace. (#4699)
 - In `go.opentelemetry.op/otel/exporters/prometheus`, the exporter no longer `Collect`s metrics after `Shutdown` is invoked. (#4648)
 - Fix documentation for `WithCompressor` in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`. (#4695)
 - Fix documentation for `WithCompressor` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc`. (#4695)

diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go
@@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.QueryUnescape(n)
+		name, err := url.PathUnescape(n)
 		if err != nil {
 			global.Error(err, "escape header key", "key", n)
 			continue
 		}
 		trimmedName := strings.TrimSpace(name)
-		value, err := url.QueryUnescape(v)
+		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
 			continue

diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go
@@ -427,14 +427,29 @@ func TestStringToHeader(t *testing.T) {
 			want:  map[string]string{"userId": "alice"},
 		},
 		{
-			name:  "multiples headers encoded",
+			name:  "simple header conforms to RFC 3986 spec",
+			value: " userId = alice+test ",
+			want:  map[string]string{"userId": "alice+test"},
+		},
+		{
+			name:  "multiple headers encoded",
 			value: "userId=alice,serverNode=DF%3A28,isProduction=false",
 			want: map[string]string{
 				"userId":       "alice",
 				"serverNode":   "DF:28",
 				"isProduction": "false",
 			},
 		},
+		{
+			name:  "multiple headers encoded per RFC 3986 spec",
+			value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
+			want: map[string]string{
+				"userId":       "alice+test",
+				"serverNode":   "DF:28",
+				"isProduction": "false",
+				"namespace":    "localhost/test",
+			},
+		},
 		{
 			name:  "invalid headers format",
 			value: "userId:alice",

diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go
@@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.QueryUnescape(n)
+		name, err := url.PathUnescape(n)
 		if err != nil {
 			global.Error(err, "escape header key", "key", n)
 			continue
 		}
 		trimmedName := strings.TrimSpace(name)
-		value, err := url.QueryUnescape(v)
+		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
 			continue

diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go
@@ -427,14 +427,29 @@ func TestStringToHeader(t *testing.T) {
 			want:  map[string]string{"userId": "alice"},
 		},
 		{
-			name:  "multiples headers encoded",
+			name:  "simple header conforms to RFC 3986 spec",
+			value: " userId = alice+test ",
+			want:  map[string]string{"userId": "alice+test"},
+		},
+		{
+			name:  "multiple headers encoded",
 			value: "userId=alice,serverNode=DF%3A28,isProduction=false",
 			want: map[string]string{
 				"userId":       "alice",
 				"serverNode":   "DF:28",
 				"isProduction": "false",
 			},
 		},
+		{
+			name:  "multiple headers encoded per RFC 3986 spec",
+			value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
+			want: map[string]string{
+				"userId":       "alice+test",
+				"serverNode":   "DF:28",
+				"isProduction": "false",
+				"namespace":    "localhost/test",
+			},
+		},
 		{
 			name:  "invalid headers format",
 			value: "userId:alice",

diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go
@@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.QueryUnescape(n)
+		name, err := url.PathUnescape(n)
 		if err != nil {
 			global.Error(err, "escape header key", "key", n)
 			continue
 		}
 		trimmedName := strings.TrimSpace(name)
-		value, err := url.QueryUnescape(v)
+		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
 			continue

diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go
@@ -427,14 +427,29 @@ func TestStringToHeader(t *testing.T) {
 			want:  map[string]string{"userId": "alice"},
 		},
 		{
-			name:  "multiples headers encoded",
+			name:  "simple header conforms to RFC 3986 spec",
+			value: " userId = alice+test ",
+			want:  map[string]string{"userId": "alice+test"},
+		},
+		{
+			name:  "multiple headers encoded",
 			value: "userId=alice,serverNode=DF%3A28,isProduction=false",
 			want: map[string]string{
 				"userId":       "alice",
 				"serverNode":   "DF:28",
 				"isProduction": "false",
 			},
 		},
+		{
+			name:  "multiple headers encoded per RFC 3986 spec",
+			value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
+			want: map[string]string{
+				"userId":       "alice+test",
+				"serverNode":   "DF:28",
+				"isProduction": "false",
+				"namespace":    "localhost/test",
+			},
+		},
 		{
 			name:  "invalid headers format",
 			value: "userId:alice",

diff --git a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig.go
@@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.QueryUnescape(n)
+		name, err := url.PathUnescape(n)
 		if err != nil {
 			global.Error(err, "escape header key", "key", n)
 			continue
 		}
 		trimmedName := strings.TrimSpace(name)
-		value, err := url.QueryUnescape(v)
+		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
 			continue

diff --git a/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracehttp/internal/envconfig/envconfig_test.go
@@ -427,14 +427,29 @@ func TestStringToHeader(t *testing.T) {
 			want:  map[string]string{"userId": "alice"},
 		},
 		{
-			name:  "multiples headers encoded",
+			name:  "simple header conforms to RFC 3986 spec",
+			value: " userId = alice+test ",
+			want:  map[string]string{"userId": "alice+test"},
+		},
+		{
+			name:  "multiple headers encoded",
 			value: "userId=alice,serverNode=DF%3A28,isProduction=false",
 			want: map[string]string{
 				"userId":       "alice",
 				"serverNode":   "DF:28",
 				"isProduction": "false",
 			},
 		},
+		{
+			name:  "multiple headers encoded per RFC 3986 spec",
+			value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
+			want: map[string]string{
+				"userId":       "alice+test",
+				"serverNode":   "DF:28",
+				"isProduction": "false",
+				"namespace":    "localhost/test",
+			},
+		},
 		{
 			name:  "invalid headers format",
 			value: "userId:alice",

diff --git a/internal/shared/otlp/envconfig/envconfig.go.tmpl b/internal/shared/otlp/envconfig/envconfig.go.tmpl
@@ -174,13 +174,13 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.QueryUnescape(n)
+		name, err := url.PathUnescape(n)
 		if err != nil {
 			global.Error(err, "escape header key", "key", n)
 			continue
 		}
 		trimmedName := strings.TrimSpace(name)
-		value, err := url.QueryUnescape(v)
+		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
 			continue

diff --git a/internal/shared/otlp/envconfig/envconfig_test.go.tmpl b/internal/shared/otlp/envconfig/envconfig_test.go.tmpl
@@ -427,14 +427,29 @@ func TestStringToHeader(t *testing.T) {
 			want:  map[string]string{"userId": "alice"},
 		},
 		{
-			name:  "multiples headers encoded",
+			name:  "simple header conforms to RFC 3986 spec",
+			value: " userId = alice+test ",
+			want:  map[string]string{"userId": "alice+test"},
+		},
+		{
+			name:  "multiple headers encoded",
 			value: "userId=alice,serverNode=DF%3A28,isProduction=false",
 			want: map[string]string{
 				"userId":       "alice",
 				"serverNode":   "DF:28",
 				"isProduction": "false",
 			},
 		},
+		{
+			name:  "multiple headers encoded per RFC 3986 spec",
+			value: "userId=alice+test,serverNode=DF%3A28,isProduction=false,namespace=localhost/test",
+			want: map[string]string{
+				"userId":       "alice+test",
+				"serverNode":   "DF:28",
+				"isProduction": "false",
+				"namespace":    "localhost/test",
+			},
+		},
 		{
 			name:  "invalid headers format",
 			value: "userId:alice",

diff --git a/sdk/resource/env.go b/sdk/resource/env.go
@@ -89,7 +89,7 @@ func constructOTResources(s string) (*Resource, error) {
 			continue
 		}
 		key := strings.TrimSpace(k)
-		val, err := url.QueryUnescape(strings.TrimSpace(v))
+		val, err := url.PathUnescape(strings.TrimSpace(v))
 		if err != nil {
 			// Retain original value if decoding fails, otherwise it will be
 			// an empty string.

diff --git a/sdk/resource/env_test.go b/sdk/resource/env_test.go
@@ -40,6 +40,19 @@ func TestDetectOnePair(t *testing.T) {
 	assert.Equal(t, NewSchemaless(attribute.String("key", "value")), res)
 }
 
+func TestDetectURIEncodingOnePair(t *testing.T) {
+	store, err := ottest.SetEnvVariables(map[string]string{
+		resourceAttrKey: "key=x+y+z?q=123",
+	})
+	require.NoError(t, err)
+	defer func() { require.NoError(t, store.Restore()) }()
+
+	detector := &fromEnv{}
+	res, err := detector.Detect(context.Background())
+	require.NoError(t, err)
+	assert.Equal(t, NewSchemaless(attribute.String("key", "x+y+z?q=123")), res)
+}
+
 func TestDetectMultiPairs(t *testing.T) {
 	store, err := ottest.SetEnvVariables(map[string]string{
 		"x":             "1",
@@ -60,6 +73,23 @@ func TestDetectMultiPairs(t *testing.T) {
 	), res)
 }
 
+func TestDetectURIEncodingMultiPairs(t *testing.T) {
+	store, err := ottest.SetEnvVariables(map[string]string{
+		"x":             "1",
+		resourceAttrKey: "key=x+y+z,namespace=localhost/test&verify",
+	})
+	require.NoError(t, err)
+	defer func() { require.NoError(t, store.Restore()) }()
+
+	detector := &fromEnv{}
+	res, err := detector.Detect(context.Background())
+	require.NoError(t, err)
+	assert.Equal(t, NewSchemaless(
+		attribute.String("key", "x+y+z"),
+		attribute.String("namespace", "localhost/test&verify"),
+	), res)
+}
+
 func TestEmpty(t *testing.T) {
 	store, err := ottest.SetEnvVariables(map[string]string{
 		resourceAttrKey: "   ",