Fix validation of baggage values

Fixes open-telemetry#2934

CHANGELOG.md

@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+- Fix validation of baggage values
+  ([#3058](https://github.com/open-telemetry/opentelemetry-python/pull/3058))
 - Fixed circular dependency issue with custom samplers
   ([#3026](https://github.com/open-telemetry/opentelemetry-python/pull/3026))
 - Add missing entry points for OTLP/HTTP exporter

opentelemetry-api/src/opentelemetry/baggage/__init__.py

@@ -81,16 +81,7 @@ def set_baggage(
         A Context with the value updated
     """
     baggage = dict(get_all(context=context))
-    if not _is_valid_key(name):
-        _logger.warning(
-            "Baggage key `%s` does not match format, ignoring", name
-        )
-    elif not _is_valid_value(str(value)):
-        _logger.warning(
-            "Baggage value `%s` does not match format, ignoring", value
-        )
-    else:
-        baggage[name] = value
+    baggage[name] = value
     return set_value(_BAGGAGE_KEY, baggage, context=context)
 
 

opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py

@@ -88,12 +88,14 @@ def extract(
                     "Baggage list-member `%s` doesn't match the format", entry
                 )
                 continue
-            name = unquote_plus(name).strip().lower()
-            value = unquote_plus(value).strip()
+
             if not _is_valid_pair(name, value):
                 _logger.warning("Invalid baggage entry: `%s`", entry)
                 continue
 
+            name = unquote_plus(name).strip().lower()
+            value = unquote_plus(value).strip()
+
             context = set_baggage(
                 name,
                 value,

opentelemetry-api/src/opentelemetry/context/__init__.py

@@ -15,9 +15,9 @@
 import logging
 import threading
 import typing
-import uuid
 from functools import wraps
 from os import environ
+from uuid import uuid4
 
 from pkg_resources import iter_entry_points
 
@@ -78,7 +78,7 @@ def create_key(keyname: str) -> str:
     Returns:
         A unique string representing the newly created key.
     """
-    return keyname + "-" + str(uuid.uuid4())
+    return keyname + "-" + str(uuid4())
 
 
 def get_value(key: str, context: typing.Optional[Context] = None) -> "object":

opentelemetry-api/tests/baggage/test_baggage.py

@@ -14,59 +14,74 @@
 
 # type: ignore
 
-import unittest
+from unittest import TestCase
 
-from opentelemetry import baggage, context
+from opentelemetry.baggage import (
+    _is_valid_key,
+    _is_valid_value,
+    clear,
+    get_all,
+    get_baggage,
+    remove_baggage,
+    set_baggage,
+)
+from opentelemetry.context import attach, detach
 
 
-class TestBaggageManager(unittest.TestCase):
+class TestBaggageManager(TestCase):
     def test_set_baggage(self):
-        self.assertEqual({}, baggage.get_all())
+        self.assertEqual({}, get_all())
 
-        ctx = baggage.set_baggage("test", "value")
-        self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
+        ctx = set_baggage("test", "value")
+        self.assertEqual(get_baggage("test", context=ctx), "value")
 
-        ctx = baggage.set_baggage("test", "value2", context=ctx)
-        self.assertEqual(baggage.get_baggage("test", context=ctx), "value2")
+        ctx = set_baggage("test", "value2", context=ctx)
+        self.assertEqual(get_baggage("test", context=ctx), "value2")
 
     def test_baggages_current_context(self):
-        token = context.attach(baggage.set_baggage("test", "value"))
-        self.assertEqual(baggage.get_baggage("test"), "value")
-        context.detach(token)
-        self.assertEqual(baggage.get_baggage("test"), None)
+        token = attach(set_baggage("test", "value"))
+        self.assertEqual(get_baggage("test"), "value")
+        detach(token)
+        self.assertEqual(get_baggage("test"), None)
 
     def test_set_multiple_baggage_entries(self):
-        ctx = baggage.set_baggage("test", "value")
-        ctx = baggage.set_baggage("test2", "value2", context=ctx)
-        self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
-        self.assertEqual(baggage.get_baggage("test2", context=ctx), "value2")
+        ctx = set_baggage("test", "value")
+        ctx = set_baggage("test2", "value2", context=ctx)
+        self.assertEqual(get_baggage("test", context=ctx), "value")
+        self.assertEqual(get_baggage("test2", context=ctx), "value2")
         self.assertEqual(
-            baggage.get_all(context=ctx),
+            get_all(context=ctx),
             {"test": "value", "test2": "value2"},
         )
 
     def test_modifying_baggage(self):
-        ctx = baggage.set_baggage("test", "value")
-        self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
-        baggage_entries = baggage.get_all(context=ctx)
+        ctx = set_baggage("test", "value")
+        self.assertEqual(get_baggage("test", context=ctx), "value")
+        baggage_entries = get_all(context=ctx)
         with self.assertRaises(TypeError):
             baggage_entries["test"] = "mess-this-up"
-        self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
+        self.assertEqual(get_baggage("test", context=ctx), "value")
 
     def test_remove_baggage_entry(self):
-        self.assertEqual({}, baggage.get_all())
+        self.assertEqual({}, get_all())
 
-        ctx = baggage.set_baggage("test", "value")
-        ctx = baggage.set_baggage("test2", "value2", context=ctx)
-        ctx = baggage.remove_baggage("test", context=ctx)
-        self.assertEqual(baggage.get_baggage("test", context=ctx), None)
-        self.assertEqual(baggage.get_baggage("test2", context=ctx), "value2")
+        ctx = set_baggage("test", "value")
+        ctx = set_baggage("test2", "value2", context=ctx)
+        ctx = remove_baggage("test", context=ctx)
+        self.assertEqual(get_baggage("test", context=ctx), None)
+        self.assertEqual(get_baggage("test2", context=ctx), "value2")
 
     def test_clear_baggage(self):
-        self.assertEqual({}, baggage.get_all())
+        self.assertEqual({}, get_all())
 
-        ctx = baggage.set_baggage("test", "value")
-        self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
+        ctx = set_baggage("test", "value")
+        self.assertEqual(get_baggage("test", context=ctx), "value")
 
-        ctx = baggage.clear(context=ctx)
-        self.assertEqual(baggage.get_all(context=ctx), {})
+        ctx = clear(context=ctx)
+        self.assertEqual(get_all(context=ctx), {})
+
+    def test__is_valid_key(self):
+        _is_valid_key
+
+    def test__is_valid_value(self):
+        self.assertTrue(_is_valid_value("GET%20%2Fapi%2F%2Freport"))

opentelemetry-api/tests/baggage/test_baggage_propagation.py → opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py

@@ -14,38 +14,40 @@
 #
 # type: ignore
 
-import unittest
 from logging import WARNING
+from random import Random
+from unittest import TestCase
 from unittest.mock import Mock, patch
+from uuid import UUID
 
-from opentelemetry import baggage
+from opentelemetry.baggage import get_all, set_baggage
 from opentelemetry.baggage.propagation import (
     W3CBaggagePropagator,
     _format_baggage,
 )
 from opentelemetry.context import get_current
 
 
-class TestBaggagePropagation(unittest.TestCase):
+class TestW3CBaggagePropagator(TestCase):
     def setUp(self):
         self.propagator = W3CBaggagePropagator()
 
     def _extract(self, header_value):
         """Test helper"""
         header = {"baggage": [header_value]}
-        return baggage.get_all(self.propagator.extract(header))
+        return get_all(self.propagator.extract(header))
 
     def _inject(self, values):
         """Test helper"""
         ctx = get_current()
         for k, v in values.items():
-            ctx = baggage.set_baggage(k, v, context=ctx)
+            ctx = set_baggage(k, v, context=ctx)
         output = {}
         self.propagator.inject(output, context=ctx)
         return output.get("baggage")
 
     def test_no_context_header(self):
-        baggage_entries = baggage.get_all(self.propagator.extract({}))
+        baggage_entries = get_all(self.propagator.extract({}))
         self.assertEqual(baggage_entries, {})
 
     def test_empty_context_header(self):
@@ -57,10 +59,9 @@ def test_valid_header(self):
         expected = {"key1": "val1", "key2": "val2"}
         self.assertEqual(self._extract(header), expected)
 
-    def test_valid_header_with_space(self):
+    def test_invalid_header_with_space(self):
         header = "key1 =   val1,  key2 =val2   "
-        expected = {"key1": "val1", "key2": "val2"}
-        self.assertEqual(self._extract(header), expected)
+        self.assertEqual(self._extract(header), {})
 
     def test_valid_header_with_properties(self):
         header = "key1=val1,key2=val2;prop=1;prop2;prop3=2"
@@ -188,8 +189,8 @@ def test_inject_no_baggage_entries(self):
         output = self._inject(values)
         self.assertEqual(None, output)
 
-    def test_inject_invalid_entries(self):
-        self.assertEqual(None, self._inject({"key": "val ue"}))
+    def test_inject_space_entries(self):
+        self.assertEqual("key=val+ue", self._inject({"key": "val ue"}))
 
     def test_inject(self):
         values = {
@@ -242,3 +243,24 @@ def test__format_baggage(self):
             _format_baggage({"key/key": "value/value"}),
             "key%2Fkey=value%2Fvalue",
         )
+
+    @patch("opentelemetry.baggage._BAGGAGE_KEY", new="abc")
+    def test_inject_extract(self):
+
+        carrier = {}
+
+        context = set_baggage(
+            "transaction", "string with spaces", context=get_current()
+        )
+
+        self.propagator.inject(carrier, context)
+
+        context = self.propagator.extract(carrier)
+
+        self.assertEqual(
+            carrier, {"baggage": "transaction=string+with+spaces"}
+        )
+
+        self.assertEqual(
+            context, {"abc": {"transaction": "string with spaces"}}
+        )