diff --git a/.changelog/5561.fixed b/.changelog/5561.fixed new file mode 100644 index 00000000000..219c583a51d --- /dev/null +++ b/.changelog/5561.fixed @@ -0,0 +1 @@ +`opentelemetry-api`: percent-encode W3C Baggage instead of form-encoding it, so a literal `+` round trips and spaces are sent as `%20` diff --git a/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py b/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py index 37361ddc529..d4222f0a2fa 100644 --- a/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py +++ b/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py @@ -4,7 +4,7 @@ from collections.abc import Iterable, Iterator, Mapping from logging import getLogger from re import split -from urllib.parse import quote_plus, unquote_plus +from urllib.parse import quote, unquote from opentelemetry.baggage import _is_valid_pair, get_all, set_baggage from opentelemetry.context import get_current @@ -115,8 +115,8 @@ def extract( _logger.warning("Invalid baggage entry: `%s`", entry) continue - name = unquote_plus(name).strip() - value = unquote_plus(value).strip() + name = unquote(name).strip() + value = unquote(value).strip() context = set_baggage( name, @@ -162,9 +162,14 @@ def fields(self) -> set[str]: def _encode_baggage_pairs( baggage_entries: Mapping[str, object], ) -> Iterator[str]: - """Yield URL-encoded 'key=value' pairs from baggage entries.""" + """Yield percent-encoded 'key=value' pairs from baggage entries. + + Percent-encoding (RFC 3986), not form encoding: the W3C Baggage grammar + lists "+" as an ordinary baggage-octet, so it must not be used to stand in + for a space. https://www.w3.org/TR/baggage/ + """ for key, value in baggage_entries.items(): - yield quote_plus(str(key)) + "=" + quote_plus(str(value)) + yield quote(str(key), safe="") + "=" + quote(str(value), safe="") def _extract_first_element( diff --git a/opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py b/opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py index 5d02792cfeb..c90a39a33e0 100644 --- a/opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py +++ b/opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py @@ -6,10 +6,12 @@ from logging import WARNING from unittest import TestCase from unittest.mock import Mock, patch -from urllib.parse import quote_plus from opentelemetry.baggage import get_all, set_baggage -from opentelemetry.baggage.propagation import W3CBaggagePropagator +from opentelemetry.baggage.propagation import ( + W3CBaggagePropagator, + _encode_baggage_pairs, +) from opentelemetry.context import get_current @@ -186,7 +188,7 @@ def test_inject_no_baggage_entries(self): self.assertEqual(None, output) def test_inject_space_entries(self): - self.assertEqual("key=val+ue", self._inject({"key": "val ue"})) + self.assertEqual("key=val%20ue", self._inject({"key": "val ue"})) def test_inject(self): values = { @@ -232,14 +234,50 @@ def test_fields(self, mock_baggage): def test_encode_baggage_pairs(self): def _format_baggage(entries): - return ",".join(quote_plus(str(k)) + "=" + quote_plus(str(v)) for k, v in entries.items()) + return ",".join(_encode_baggage_pairs(entries)) - self.assertEqual(_format_baggage({"key key": "value value"}), "key+key=value+value") + self.assertEqual( + _format_baggage({"key key": "value value"}), + "key%20key=value%20value", + ) self.assertEqual( _format_baggage({"key/key": "value/value"}), "key%2Fkey=value%2Fvalue", ) + # The W3C Baggage grammar defines + # baggage-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E + # "+" is %x2B, an ordinary literal that must survive a round trip, and + # SP (%x20) is not a baggage-octet so it must be percent-encoded. + # https://www.w3.org/TR/baggage/ + + def test_extract_preserves_literal_plus(self): + """A "+" on the wire is a literal "+", never a space.""" + self.assertEqual(self._extract("key=a+b"), {"key": "a+b"}) + + def test_extract_preserves_plus_only_value(self): + """A value that is only "+" must not decode to an empty string.""" + self.assertEqual(self._extract("key=+"), {"key": "+"}) + + def test_extract_preserves_repeated_plus(self): + self.assertEqual(self._extract("key=c++"), {"key": "c++"}) + + def test_extract_preserves_plus_in_key(self): + self.assertEqual(self._extract("a+b=value"), {"a+b": "value"}) + + def test_extract_percent_encoded_space(self): + """%20 is the spec-correct wire form for a space.""" + self.assertEqual(self._extract("key=a%20b"), {"key": "a b"}) + + def test_inject_percent_encodes_space(self): + self.assertEqual("key=a%20b", self._inject({"key": "a b"})) + + def test_inject_extract_round_trips_plus(self): + self.assertEqual(self._extract(self._inject({"key": "a+b"})), {"key": "a+b"}) + + def test_inject_extract_round_trips_space(self): + self.assertEqual(self._extract(self._inject({"key": "a b"})), {"key": "a b"}) + def test_inject_too_many_entries(self): """Inject should drop entries exceeding _MAX_PAIRS.""" values = {f"key{i}": f"val{i}" for i in range(self.propagator._MAX_PAIRS + 10)} @@ -320,6 +358,6 @@ def test_inject_extract(self): context = self.propagator.extract(carrier) - self.assertEqual(carrier, {"baggage": "transaction=string+with+spaces"}) + self.assertEqual(carrier, {"baggage": "transaction=string%20with%20spaces"}) self.assertEqual(context, {"abc": {"transaction": "string with spaces"}})