Bugfix: OTLP exporters should not percent decode the key when parsing…

… HEADERS env vars (#5705) Bugfix #5623 As stated in the issue, we need to avoid parsing the key and instead implement a validation check for it. I've added some unit tests to verify this fix. However, I noticed a comment at the top of this file: ``` // Code created by gotmpl. DO NOT MODIFY. // source: internal/shared/otlp/envconfig/envconfig.go.tmpl ``` It seems that `internal/shared/otlp/envconfig/envconfig.go.tmpl` is the source template for this file. Since this template matches `exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go`, I updated the template to maintain consistency. I’m not entirely sure if this approach is correct, so please confirm if this is the right course of action. --------- Co-authored-by: Fools <[email protected]> Co-authored-by: Damien Mathieu <[email protected]> Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Chester Cheung <[email protected]> Co-authored-by: Tyler Yahn <[email protected]>
open-telemetry · Aug 21, 2024 · 83ae9bd · 83ae9bd
1 parent 083d03e
commit 83ae9bd
Show file tree

Hide file tree

Showing 11 changed files with 383 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,8 @@ The next release will require at least [Go 1.22].
 
 ### Fixed
 
+- Stop percent encoding header environment variables in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`, `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`, `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` (#5705)
+- Remove invalid environment variable header keys in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`, `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`, `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` (#5705)
 - Correct comments for the priority of the `WithEndpoint` and `WithEndpointURL` options and their corresponding environment variables in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#5584)
 - Correct the `Tracer`, `Meter`, and `Logger` names used in `go.opentelemetry.io/otel/example/dice`. (#5612)
 - Correct the `Tracer` names used in `go.opentelemetry.io/otel/example/namedtracer`. (#5612)

diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go
@@ -15,6 +15,7 @@ import (
 	"strconv"
 	"strings"
 	"time"
+	"unicode"
 
 	"go.opentelemetry.io/otel/internal/global"
 )
@@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.PathUnescape(n)
-		if err != nil {
-			global.Error(err, "escape header key", "key", n)
+
+		trimmedName := strings.TrimSpace(n)
+
+		// Validate the key.
+		if !isValidHeaderKey(trimmedName) {
+			global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName)
 			continue
 		}
-		trimmedName := strings.TrimSpace(name)
+
+		// Only decode the value.
 		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
@@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) {
 	}
 	return cp, nil
 }
+
+func isValidHeaderKey(key string) bool {
+	if key == "" {
+		return false
+	}
+	for _, c := range key {
+		if !isTokenChar(c) {
+			return false
+		}
+	}
+	return true
+}
+
+func isTokenChar(c rune) bool {
+	return c <= unicode.MaxASCII && (unicode.IsLetter(c) ||
+		unicode.IsDigit(c) ||
+		c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' ||
+		c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~')
+}
diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go
@@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "with percent-encoded headers",
+			reader: EnvOptionsReader{
+				GetEnv: func(n string) string {
+					if n == "HELLO" {
+						return "user%2Did=42,user%20name=alice%20smith"
+					}
+					return ""
+				},
+			},
+			configs: []ConfigFn{
+				WithHeaders("HELLO", func(v map[string]string) {
+					options = append(options, testOption{TestHeaders: v})
+				}),
+			},
+			expectedOptions: []testOption{
+				{
+					TestHeaders: map[string]string{
+						"user%2Did":   "42",
+						"user%20name": "alice smith",
+					},
+				},
+			},
+		},
+		{
+			name: "with invalid header key",
+			reader: EnvOptionsReader{
+				GetEnv: func(n string) string {
+					if n == "HELLO" {
+						return "valid-key=value,invalid key=value"
+					}
+					return ""
+				},
+			},
+			configs: []ConfigFn{
+				WithHeaders("HELLO", func(v map[string]string) {
+					options = append(options, testOption{TestHeaders: v})
+				}),
+			},
+			expectedOptions: []testOption{
+				{
+					TestHeaders: map[string]string{
+						"valid-key": "value",
+					},
+				},
+			},
+		},
 		{
 			name: "with URL",
 			reader: EnvOptionsReader{
@@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) {
 			name:  "invalid key",
 			value: "%XX=missing,userId=alice",
 			want: map[string]string{
+				"%XX":    "missing",
 				"userId": "alice",
 			},
 		},

diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go
@@ -15,6 +15,7 @@ import (
 	"strconv"
 	"strings"
 	"time"
+	"unicode"
 
 	"go.opentelemetry.io/otel/internal/global"
 )
@@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.PathUnescape(n)
-		if err != nil {
-			global.Error(err, "escape header key", "key", n)
+
+		trimmedName := strings.TrimSpace(n)
+
+		// Validate the key.
+		if !isValidHeaderKey(trimmedName) {
+			global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName)
 			continue
 		}
-		trimmedName := strings.TrimSpace(name)
+
+		// Only decode the value.
 		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
@@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) {
 	}
 	return cp, nil
 }
+
+func isValidHeaderKey(key string) bool {
+	if key == "" {
+		return false
+	}
+	for _, c := range key {
+		if !isTokenChar(c) {
+			return false
+		}
+	}
+	return true
+}
+
+func isTokenChar(c rune) bool {
+	return c <= unicode.MaxASCII && (unicode.IsLetter(c) ||
+		unicode.IsDigit(c) ||
+		c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' ||
+		c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~')
+}
diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go
@@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "with percent-encoded headers",
+			reader: EnvOptionsReader{
+				GetEnv: func(n string) string {
+					if n == "HELLO" {
+						return "user%2Did=42,user%20name=alice%20smith"
+					}
+					return ""
+				},
+			},
+			configs: []ConfigFn{
+				WithHeaders("HELLO", func(v map[string]string) {
+					options = append(options, testOption{TestHeaders: v})
+				}),
+			},
+			expectedOptions: []testOption{
+				{
+					TestHeaders: map[string]string{
+						"user%2Did":   "42",
+						"user%20name": "alice smith",
+					},
+				},
+			},
+		},
+		{
+			name: "with invalid header key",
+			reader: EnvOptionsReader{
+				GetEnv: func(n string) string {
+					if n == "HELLO" {
+						return "valid-key=value,invalid key=value"
+					}
+					return ""
+				},
+			},
+			configs: []ConfigFn{
+				WithHeaders("HELLO", func(v map[string]string) {
+					options = append(options, testOption{TestHeaders: v})
+				}),
+			},
+			expectedOptions: []testOption{
+				{
+					TestHeaders: map[string]string{
+						"valid-key": "value",
+					},
+				},
+			},
+		},
 		{
 			name: "with URL",
 			reader: EnvOptionsReader{
@@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) {
 			name:  "invalid key",
 			value: "%XX=missing,userId=alice",
 			want: map[string]string{
+				"%XX":    "missing",
 				"userId": "alice",
 			},
 		},

diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go
@@ -15,6 +15,7 @@ import (
 	"strconv"
 	"strings"
 	"time"
+	"unicode"
 
 	"go.opentelemetry.io/otel/internal/global"
 )
@@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string {
 			global.Error(errors.New("missing '="), "parse headers", "input", header)
 			continue
 		}
-		name, err := url.PathUnescape(n)
-		if err != nil {
-			global.Error(err, "escape header key", "key", n)
+
+		trimmedName := strings.TrimSpace(n)
+
+		// Validate the key.
+		if !isValidHeaderKey(trimmedName) {
+			global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName)
 			continue
 		}
-		trimmedName := strings.TrimSpace(name)
+
+		// Only decode the value.
 		value, err := url.PathUnescape(v)
 		if err != nil {
 			global.Error(err, "escape header value", "value", v)
@@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) {
 	}
 	return cp, nil
 }
+
+func isValidHeaderKey(key string) bool {
+	if key == "" {
+		return false
+	}
+	for _, c := range key {
+		if !isTokenChar(c) {
+			return false
+		}
+	}
+	return true
+}
+
+func isTokenChar(c rune) bool {
+	return c <= unicode.MaxASCII && (unicode.IsLetter(c) ||
+		unicode.IsDigit(c) ||
+		c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' ||
+		c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~')
+}
diff --git a/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go b/exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go
@@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "with percent-encoded headers",
+			reader: EnvOptionsReader{
+				GetEnv: func(n string) string {
+					if n == "HELLO" {
+						return "user%2Did=42,user%20name=alice%20smith"
+					}
+					return ""
+				},
+			},
+			configs: []ConfigFn{
+				WithHeaders("HELLO", func(v map[string]string) {
+					options = append(options, testOption{TestHeaders: v})
+				}),
+			},
+			expectedOptions: []testOption{
+				{
+					TestHeaders: map[string]string{
+						"user%2Did":   "42",
+						"user%20name": "alice smith",
+					},
+				},
+			},
+		},
+		{
+			name: "with invalid header key",
+			reader: EnvOptionsReader{
+				GetEnv: func(n string) string {
+					if n == "HELLO" {
+						return "valid-key=value,invalid key=value"
+					}
+					return ""
+				},
+			},
+			configs: []ConfigFn{
+				WithHeaders("HELLO", func(v map[string]string) {
+					options = append(options, testOption{TestHeaders: v})
+				}),
+			},
+			expectedOptions: []testOption{
+				{
+					TestHeaders: map[string]string{
+						"valid-key": "value",
+					},
+				},
+			},
+		},
 		{
 			name: "with URL",
 			reader: EnvOptionsReader{
@@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) {
 			name:  "invalid key",
 			value: "%XX=missing,userId=alice",
 			want: map[string]string{
+				"%XX":    "missing",
 				"userId": "alice",
 			},
 		},