Skip to content

Commit 83ae9bd

Browse files
zhihalidmathieupellaredhanyuancheungMrAlias
authored
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]>
1 parent 083d03e commit 83ae9bd

File tree

11 files changed

+383
-21
lines changed

11 files changed

+383
-21
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ The next release will require at least [Go 1.22].
3434

3535
### Fixed
3636

37+
- 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)
38+
- 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)
3739
- 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)
3840
- Correct the `Tracer`, `Meter`, and `Logger` names used in `go.opentelemetry.io/otel/example/dice`. (#5612)
3941
- Correct the `Tracer` names used in `go.opentelemetry.io/otel/example/namedtracer`. (#5612)

‎exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig.go

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"strconv"
1616
"strings"
1717
"time"
18+
"unicode"
1819

1920
"go.opentelemetry.io/otel/internal/global"
2021
)
@@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string {
163164
global.Error(errors.New("missing '="), "parse headers", "input", header)
164165
continue
165166
}
166-
name, err := url.PathUnescape(n)
167-
if err != nil {
168-
global.Error(err, "escape header key", "key", n)
167+
168+
trimmedName := strings.TrimSpace(n)
169+
170+
// Validate the key.
171+
if !isValidHeaderKey(trimmedName) {
172+
global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName)
169173
continue
170174
}
171-
trimmedName := strings.TrimSpace(name)
175+
176+
// Only decode the value.
172177
value, err := url.PathUnescape(v)
173178
if err != nil {
174179
global.Error(err, "escape header value", "value", v)
@@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) {
189194
}
190195
return cp, nil
191196
}
197+
198+
func isValidHeaderKey(key string) bool {
199+
if key == "" {
200+
return false
201+
}
202+
for _, c := range key {
203+
if !isTokenChar(c) {
204+
return false
205+
}
206+
}
207+
return true
208+
}
209+
210+
func isTokenChar(c rune) bool {
211+
return c <= unicode.MaxASCII && (unicode.IsLetter(c) ||
212+
unicode.IsDigit(c) ||
213+
c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' ||
214+
c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~')
215+
}

‎exporters/otlp/otlpmetric/otlpmetricgrpc/internal/envconfig/envconfig_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) {
270270
},
271271
},
272272
},
273+
{
274+
name: "with percent-encoded headers",
275+
reader: EnvOptionsReader{
276+
GetEnv: func(n string) string {
277+
if n == "HELLO" {
278+
return "user%2Did=42,user%20name=alice%20smith"
279+
}
280+
return ""
281+
},
282+
},
283+
configs: []ConfigFn{
284+
WithHeaders("HELLO", func(v map[string]string) {
285+
options = append(options, testOption{TestHeaders: v})
286+
}),
287+
},
288+
expectedOptions: []testOption{
289+
{
290+
TestHeaders: map[string]string{
291+
"user%2Did": "42",
292+
"user%20name": "alice smith",
293+
},
294+
},
295+
},
296+
},
297+
{
298+
name: "with invalid header key",
299+
reader: EnvOptionsReader{
300+
GetEnv: func(n string) string {
301+
if n == "HELLO" {
302+
return "valid-key=value,invalid key=value"
303+
}
304+
return ""
305+
},
306+
},
307+
configs: []ConfigFn{
308+
WithHeaders("HELLO", func(v map[string]string) {
309+
options = append(options, testOption{TestHeaders: v})
310+
}),
311+
},
312+
expectedOptions: []testOption{
313+
{
314+
TestHeaders: map[string]string{
315+
"valid-key": "value",
316+
},
317+
},
318+
},
319+
},
273320
{
274321
name: "with URL",
275322
reader: EnvOptionsReader{
@@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) {
448495
name: "invalid key",
449496
value: "%XX=missing,userId=alice",
450497
want: map[string]string{
498+
"%XX": "missing",
451499
"userId": "alice",
452500
},
453501
},

‎exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig.go

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"strconv"
1616
"strings"
1717
"time"
18+
"unicode"
1819

1920
"go.opentelemetry.io/otel/internal/global"
2021
)
@@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string {
163164
global.Error(errors.New("missing '="), "parse headers", "input", header)
164165
continue
165166
}
166-
name, err := url.PathUnescape(n)
167-
if err != nil {
168-
global.Error(err, "escape header key", "key", n)
167+
168+
trimmedName := strings.TrimSpace(n)
169+
170+
// Validate the key.
171+
if !isValidHeaderKey(trimmedName) {
172+
global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName)
169173
continue
170174
}
171-
trimmedName := strings.TrimSpace(name)
175+
176+
// Only decode the value.
172177
value, err := url.PathUnescape(v)
173178
if err != nil {
174179
global.Error(err, "escape header value", "value", v)
@@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) {
189194
}
190195
return cp, nil
191196
}
197+
198+
func isValidHeaderKey(key string) bool {
199+
if key == "" {
200+
return false
201+
}
202+
for _, c := range key {
203+
if !isTokenChar(c) {
204+
return false
205+
}
206+
}
207+
return true
208+
}
209+
210+
func isTokenChar(c rune) bool {
211+
return c <= unicode.MaxASCII && (unicode.IsLetter(c) ||
212+
unicode.IsDigit(c) ||
213+
c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' ||
214+
c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~')
215+
}

‎exporters/otlp/otlpmetric/otlpmetrichttp/internal/envconfig/envconfig_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) {
270270
},
271271
},
272272
},
273+
{
274+
name: "with percent-encoded headers",
275+
reader: EnvOptionsReader{
276+
GetEnv: func(n string) string {
277+
if n == "HELLO" {
278+
return "user%2Did=42,user%20name=alice%20smith"
279+
}
280+
return ""
281+
},
282+
},
283+
configs: []ConfigFn{
284+
WithHeaders("HELLO", func(v map[string]string) {
285+
options = append(options, testOption{TestHeaders: v})
286+
}),
287+
},
288+
expectedOptions: []testOption{
289+
{
290+
TestHeaders: map[string]string{
291+
"user%2Did": "42",
292+
"user%20name": "alice smith",
293+
},
294+
},
295+
},
296+
},
297+
{
298+
name: "with invalid header key",
299+
reader: EnvOptionsReader{
300+
GetEnv: func(n string) string {
301+
if n == "HELLO" {
302+
return "valid-key=value,invalid key=value"
303+
}
304+
return ""
305+
},
306+
},
307+
configs: []ConfigFn{
308+
WithHeaders("HELLO", func(v map[string]string) {
309+
options = append(options, testOption{TestHeaders: v})
310+
}),
311+
},
312+
expectedOptions: []testOption{
313+
{
314+
TestHeaders: map[string]string{
315+
"valid-key": "value",
316+
},
317+
},
318+
},
319+
},
273320
{
274321
name: "with URL",
275322
reader: EnvOptionsReader{
@@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) {
448495
name: "invalid key",
449496
value: "%XX=missing,userId=alice",
450497
want: map[string]string{
498+
"%XX": "missing",
451499
"userId": "alice",
452500
},
453501
},

‎exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig.go

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"strconv"
1616
"strings"
1717
"time"
18+
"unicode"
1819

1920
"go.opentelemetry.io/otel/internal/global"
2021
)
@@ -163,12 +164,16 @@ func stringToHeader(value string) map[string]string {
163164
global.Error(errors.New("missing '="), "parse headers", "input", header)
164165
continue
165166
}
166-
name, err := url.PathUnescape(n)
167-
if err != nil {
168-
global.Error(err, "escape header key", "key", n)
167+
168+
trimmedName := strings.TrimSpace(n)
169+
170+
// Validate the key.
171+
if !isValidHeaderKey(trimmedName) {
172+
global.Error(errors.New("invalid header key"), "parse headers", "key", trimmedName)
169173
continue
170174
}
171-
trimmedName := strings.TrimSpace(name)
175+
176+
// Only decode the value.
172177
value, err := url.PathUnescape(v)
173178
if err != nil {
174179
global.Error(err, "escape header value", "value", v)
@@ -189,3 +194,22 @@ func createCertPool(certBytes []byte) (*x509.CertPool, error) {
189194
}
190195
return cp, nil
191196
}
197+
198+
func isValidHeaderKey(key string) bool {
199+
if key == "" {
200+
return false
201+
}
202+
for _, c := range key {
203+
if !isTokenChar(c) {
204+
return false
205+
}
206+
}
207+
return true
208+
}
209+
210+
func isTokenChar(c rune) bool {
211+
return c <= unicode.MaxASCII && (unicode.IsLetter(c) ||
212+
unicode.IsDigit(c) ||
213+
c == '!' || c == '#' || c == '$' || c == '%' || c == '&' || c == '\'' || c == '*' ||
214+
c == '+' || c == '-' || c == '.' || c == '^' || c == '_' || c == '`' || c == '|' || c == '~')
215+
}

‎exporters/otlp/otlptrace/otlptracegrpc/internal/envconfig/envconfig_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,53 @@ func TestEnvConfig(t *testing.T) {
270270
},
271271
},
272272
},
273+
{
274+
name: "with percent-encoded headers",
275+
reader: EnvOptionsReader{
276+
GetEnv: func(n string) string {
277+
if n == "HELLO" {
278+
return "user%2Did=42,user%20name=alice%20smith"
279+
}
280+
return ""
281+
},
282+
},
283+
configs: []ConfigFn{
284+
WithHeaders("HELLO", func(v map[string]string) {
285+
options = append(options, testOption{TestHeaders: v})
286+
}),
287+
},
288+
expectedOptions: []testOption{
289+
{
290+
TestHeaders: map[string]string{
291+
"user%2Did": "42",
292+
"user%20name": "alice smith",
293+
},
294+
},
295+
},
296+
},
297+
{
298+
name: "with invalid header key",
299+
reader: EnvOptionsReader{
300+
GetEnv: func(n string) string {
301+
if n == "HELLO" {
302+
return "valid-key=value,invalid key=value"
303+
}
304+
return ""
305+
},
306+
},
307+
configs: []ConfigFn{
308+
WithHeaders("HELLO", func(v map[string]string) {
309+
options = append(options, testOption{TestHeaders: v})
310+
}),
311+
},
312+
expectedOptions: []testOption{
313+
{
314+
TestHeaders: map[string]string{
315+
"valid-key": "value",
316+
},
317+
},
318+
},
319+
},
273320
{
274321
name: "with URL",
275322
reader: EnvOptionsReader{
@@ -448,6 +495,7 @@ func TestStringToHeader(t *testing.T) {
448495
name: "invalid key",
449496
value: "%XX=missing,userId=alice",
450497
want: map[string]string{
498+
"%XX": "missing",
451499
"userId": "alice",
452500
},
453501
},

0 commit comments

Comments
 (0)