Allow only valid trace ID characters when decoding (#854)

Without this change, invalid characters received from the user input causes a
bad error message to the user.  Here we check that all of the characters that
are to be decoded are valid hex characters and improve the error to the user in
the event the trace ID submitted has invalid characters.

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## main / unreleased
 
+* [BUGFIX] Allow only valid trace ID characters when decoding [#854](https://github.com/grafana/tempo/pull/854) (@zalegrala)
 * [CHANGE] Upgrade Cortex from v1.9.0 to v1.9.0-131-ga4bf10354 [#841](https://github.com/grafana/tempo/pull/841) (@aknuds1)
 * [CHANGE] Change default tempo port from 3100 to 3200 [#770](https://github.com/grafana/tempo/pull/809) (@MurzNN)
 * [ENHANCEMENT] Added hedged request metric `tempodb_backend_hedged_roundtrips_total` and a new storage agnostic `tempodb_backend_request_duration_seconds` metric that

pkg/util/http.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/gorilla/mux"
 )
@@ -32,6 +33,18 @@ func ParseTraceID(r *http.Request) ([]byte, error) {
 }
 
 func hexStringToTraceID(id string) ([]byte, error) {
+	// The encoding/hex package does not handle non-hex characters.
+	// Ensure the ID has only the proper characters
+	for pos, idChar := range strings.Split(id, "") {
+		if (idChar >= "a" && idChar <= "f") ||
+			(idChar >= "A" && idChar <= "F") ||
+			(idChar >= "0" && idChar <= "9") {
+			continue
+		} else {
+			return nil, fmt.Errorf("trace IDs can only contain hex characters: invalid character '%s' at position %d", idChar, pos+1)
+		}
+	}
+
 	// the encoding/hex package does not like odd length strings.
 	// just append a bit here
 	if len(id)%2 == 1 {
@@ -45,7 +58,7 @@ func hexStringToTraceID(id string) ([]byte, error) {
 
 	size := len(byteID)
 	if size > 16 {
-		return nil, errors.New("trace ids can't be larger than 128 bits")
+		return nil, errors.New("trace IDs can't be larger than 128 bits")
 	}
 	if size < 16 {
 		byteID = append(make([]byte, 16-size), byteID...)

pkg/util/http_test.go

@@ -1,6 +1,7 @@
 package util
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/grafana/tempo/cmd/tempo-query/tempo"
@@ -13,7 +14,7 @@ func TestHexStringToTraceID(t *testing.T) {
 	tc := []struct {
 		id          string
 		expected    []byte
-		expectError bool
+		expectError error
 	}{
 		{
 			id:       "12",
@@ -30,20 +31,25 @@ func TestHexStringToTraceID(t *testing.T) {
 		{
 			id:          "121234567890abcdef1234567890abcdef", // value too long
 			expected:    []byte{0x12, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef, 0x12, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef},
-			expectError: true,
+			expectError: errors.New("trace IDs can't be larger than 128 bits"),
 		},
 		{
 			id:       "234567890abcdef", // odd length
 			expected: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x34, 0x56, 0x78, 0x90, 0xab, 0xcd, 0xef},
 		},
+		{
+			id:          "1234567890abcdef ", // trailing space
+			expected:    nil,
+			expectError: errors.New("trace IDs can only contain hex characters: invalid character ' ' at position 17"),
+		},
 	}
 
 	for _, tt := range tc {
 		t.Run(tt.id, func(t *testing.T) {
 			actual, err := hexStringToTraceID(tt.id)
 
-			if tt.expectError {
-				assert.Error(t, err)
+			if tt.expectError != nil {
+				assert.Equal(t, tt.expectError, err)
 				assert.Nil(t, actual)
 				return
 			}