Panic on duplicate prop register

propagators/autoprop/registry.go

@@ -15,6 +15,8 @@
 package autoprop // import "go.opentelemetry.io/contrib/propagators/autoprop"
 
 import (
+	"errors"
+	"fmt"
 	"sync"
 
 	"go.opentelemetry.io/contrib/propagators/aws/xray"
@@ -70,21 +72,46 @@ func (r *registry) load(key string) (p propagation.TextMapPropagator, ok bool) {
 	return p, ok
 }
 
-// store sets the value for a key.
-func (r *registry) store(key string, value propagation.TextMapPropagator) {
+var errDupReg = errors.New("duplicate registration")
+
+// store sets the value for a key if is not already in the registry. errDupReg
+// is returned if the registry already contains key.
+func (r *registry) store(key string, value propagation.TextMapPropagator) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	if r.names == nil {
 		r.names = map[string]propagation.TextMapPropagator{key: value}
-		return
+		return nil
+	}
+	if _, ok := r.names[key]; ok {
+		return fmt.Errorf("%w: %q", errDupReg, key)
 	}
 	r.names[key] = value
+	return nil
+}
+
+// drop removes key from the registry if it exists, otherwise nothing.
+func (r *registry) drop(key string) {
+	r.mu.Lock()
+	delete(r.names, key)
+	r.mu.Unlock()
 }
 
 // RegisterTextMapPropagator sets the TextMapPropagator p to be used when the
 // OTEL_PROPAGATORS environment variable contains the propagator name. This
-// allows adding additional environment TextMapPropagators, or replacing the
-// default TextMapPropagators with 3rd-party implementations.
+// will panic if name has already been registered or is a default
+// (tracecontext, baggage, b3, b3multi, jaeger, xray, or ottrace).
 func RegisterTextMapPropagator(name string, p propagation.TextMapPropagator) {
-	envRegistry.store(name, p)
+	if err := envRegistry.store(name, p); err != nil {
+		// envRegistry.store will return errDupReg if name is already
+		// registered. Panic here so the user is made aware of the duplicate
+		// registration, which could be done by malicious code trying to
+		// intercept cross-cutting concerns.
+		//
+		// Panic for all other errors as well. At this point there should not
+		// be any other errors returned from the store operation. If there
+		// are, alert the developer that adding them as soon as possible that
+		// they need to be handled here.
+		panic(err)
+	}
 }

propagators/autoprop/registry_test.go

@@ -15,6 +15,7 @@
 package autoprop
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -60,8 +61,20 @@ func TestRegistryConcurrentSafe(t *testing.T) {
 func TestRegisterTextMapPropagator(t *testing.T) {
 	const propName = "custom"
 	RegisterTextMapPropagator(propName, noop)
+	t.Cleanup(func() { envRegistry.drop(propName) })
 
 	v, ok := envRegistry.load(propName)
 	assert.True(t, ok, "missing propagator in envRegistry")
 	assert.Equal(t, noop, v, "wrong propagator stored")
 }
+
+func TestDuplicateRegisterTextMapPropagatorPanics(t *testing.T) {
+	const propName = "custom"
+	RegisterTextMapPropagator(propName, noop)
+	t.Cleanup(func() { envRegistry.drop(propName) })
+
+	errString := fmt.Sprintf("%s: %q", errDupReg, propName)
+	assert.PanicsWithError(t, errString, func() {
+		RegisterTextMapPropagator(propName, noop)
+	})
+}