Allow only valid trace ID characters when decoding

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] Ensure a few non-hex characters are caught before decoding trace IDs [#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,44 @@ 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
+	validChars := []string{
+		"a", "b", "c", "d", "e", "f",
+		"A", "B", "C", "D", "E", "F",
+		"0", "1", "2", "3", "4", "5", "6", "7", "8", "9",
+	}
+
+	var invalidCharPositions []int
+	idChars := strings.Split(id, "")
+
+	for pos, idChar := range idChars {
+		var valid bool
+
+		for _, validChar := range validChars {
+			if idChar == validChar {
+				valid = true
+				break
+			}
+		}
+
+		if !valid {
+			invalidCharPositions = append(invalidCharPositions, pos)
+		}
+	}
+
+	if len(invalidCharPositions) > 0 {
+		msg := "trace IDs can only contain hex characters: "
+		msgs := []string{}
+
+		for _, d := range invalidCharPositions {
+			msgs = append(msgs, fmt.Sprintf("invalid character '%s' at position %d", idChars[d], d+1))
+		}
+		msg += strings.Join(msgs, ", ")
+
+		return nil, errors.New(msg)
+	}
+
 	// the encoding/hex package does not like odd length strings.
 	// just append a bit here
 	if len(id)%2 == 1 {

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,30 @@ 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"),
+		},
+		{
+			id:          "_12345x7890abcdef ", // multiple invalid characters
+			expected:    nil,
+			expectError: errors.New("trace IDs can only contain hex characters: invalid character '_' at position 1, invalid character 'x' at position 7, invalid character ' ' at position 18"),
+		},
 	}
 
 	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
 			}