Skip to content

Commit a99cc21

Browse files
ocelotlocelotl
authored
Fix validation of baggage values (#3058)
* Fix validation of baggage values Fixes #2934 * Remove test case
1 parent f879d38 commit a99cc21

File tree

6 files changed

+84
-57
lines changed

6 files changed

+84
-57
lines changed

‎CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1313
- Add db metric name to semantic conventions
1414
([#3115](https://github.com/open-telemetry/opentelemetry-python/pull/3115))
1515

16+
- Fix validation of baggage values
17+
([#3058](https://github.com/open-telemetry/opentelemetry-python/pull/3058))
18+
1619
## Version 1.15.0/0.36b0 (2022-12-09)
1720

1821
- Regenerate opentelemetry-proto to be compatible with protobuf 3 and 4

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

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,16 +81,7 @@ def set_baggage(
8181
A Context with the value updated
8282
"""
8383
baggage = dict(get_all(context=context))
84-
if not _is_valid_key(name):
85-
_logger.warning(
86-
"Baggage key `%s` does not match format, ignoring", name
87-
)
88-
elif not _is_valid_value(str(value)):
89-
_logger.warning(
90-
"Baggage value `%s` does not match format, ignoring", value
91-
)
92-
else:
93-
baggage[name] = value
84+
baggage[name] = value
9485
return set_value(_BAGGAGE_KEY, baggage, context=context)
9586

9687

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,12 +88,14 @@ def extract(
8888
"Baggage list-member `%s` doesn't match the format", entry
8989
)
9090
continue
91-
name = unquote_plus(name).strip().lower()
92-
value = unquote_plus(value).strip()
91+
9392
if not _is_valid_pair(name, value):
9493
_logger.warning("Invalid baggage entry: `%s`", entry)
9594
continue
9695

96+
name = unquote_plus(name).strip().lower()
97+
value = unquote_plus(value).strip()
98+
9799
context = set_baggage(
98100
name,
99101
value,

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@
1515
import logging
1616
import threading
1717
import typing
18-
import uuid
1918
from functools import wraps
2019
from os import environ
20+
from uuid import uuid4
2121

2222
from pkg_resources import iter_entry_points
2323

@@ -78,7 +78,7 @@ def create_key(keyname: str) -> str:
7878
Returns:
7979
A unique string representing the newly created key.
8080
"""
81-
return keyname + "-" + str(uuid.uuid4())
81+
return keyname + "-" + str(uuid4())
8282

8383

8484
def get_value(key: str, context: typing.Optional[Context] = None) -> "object":

‎opentelemetry-api/tests/baggage/test_baggage.py

Lines changed: 43 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -14,59 +14,70 @@
1414

1515
# type: ignore
1616

17-
import unittest
17+
from unittest import TestCase
1818

19-
from opentelemetry import baggage, context
19+
from opentelemetry.baggage import (
20+
_is_valid_value,
21+
clear,
22+
get_all,
23+
get_baggage,
24+
remove_baggage,
25+
set_baggage,
26+
)
27+
from opentelemetry.context import attach, detach
2028

2129

22-
class TestBaggageManager(unittest.TestCase):
30+
class TestBaggageManager(TestCase):
2331
def test_set_baggage(self):
24-
self.assertEqual({}, baggage.get_all())
32+
self.assertEqual({}, get_all())
2533

26-
ctx = baggage.set_baggage("test", "value")
27-
self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
34+
ctx = set_baggage("test", "value")
35+
self.assertEqual(get_baggage("test", context=ctx), "value")
2836

29-
ctx = baggage.set_baggage("test", "value2", context=ctx)
30-
self.assertEqual(baggage.get_baggage("test", context=ctx), "value2")
37+
ctx = set_baggage("test", "value2", context=ctx)
38+
self.assertEqual(get_baggage("test", context=ctx), "value2")
3139

3240
def test_baggages_current_context(self):
33-
token = context.attach(baggage.set_baggage("test", "value"))
34-
self.assertEqual(baggage.get_baggage("test"), "value")
35-
context.detach(token)
36-
self.assertEqual(baggage.get_baggage("test"), None)
41+
token = attach(set_baggage("test", "value"))
42+
self.assertEqual(get_baggage("test"), "value")
43+
detach(token)
44+
self.assertEqual(get_baggage("test"), None)
3745

3846
def test_set_multiple_baggage_entries(self):
39-
ctx = baggage.set_baggage("test", "value")
40-
ctx = baggage.set_baggage("test2", "value2", context=ctx)
41-
self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
42-
self.assertEqual(baggage.get_baggage("test2", context=ctx), "value2")
47+
ctx = set_baggage("test", "value")
48+
ctx = set_baggage("test2", "value2", context=ctx)
49+
self.assertEqual(get_baggage("test", context=ctx), "value")
50+
self.assertEqual(get_baggage("test2", context=ctx), "value2")
4351
self.assertEqual(
44-
baggage.get_all(context=ctx),
52+
get_all(context=ctx),
4553
{"test": "value", "test2": "value2"},
4654
)
4755

4856
def test_modifying_baggage(self):
49-
ctx = baggage.set_baggage("test", "value")
50-
self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
51-
baggage_entries = baggage.get_all(context=ctx)
57+
ctx = set_baggage("test", "value")
58+
self.assertEqual(get_baggage("test", context=ctx), "value")
59+
baggage_entries = get_all(context=ctx)
5260
with self.assertRaises(TypeError):
5361
baggage_entries["test"] = "mess-this-up"
54-
self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
62+
self.assertEqual(get_baggage("test", context=ctx), "value")
5563

5664
def test_remove_baggage_entry(self):
57-
self.assertEqual({}, baggage.get_all())
65+
self.assertEqual({}, get_all())
5866

59-
ctx = baggage.set_baggage("test", "value")
60-
ctx = baggage.set_baggage("test2", "value2", context=ctx)
61-
ctx = baggage.remove_baggage("test", context=ctx)
62-
self.assertEqual(baggage.get_baggage("test", context=ctx), None)
63-
self.assertEqual(baggage.get_baggage("test2", context=ctx), "value2")
67+
ctx = set_baggage("test", "value")
68+
ctx = set_baggage("test2", "value2", context=ctx)
69+
ctx = remove_baggage("test", context=ctx)
70+
self.assertEqual(get_baggage("test", context=ctx), None)
71+
self.assertEqual(get_baggage("test2", context=ctx), "value2")
6472

6573
def test_clear_baggage(self):
66-
self.assertEqual({}, baggage.get_all())
74+
self.assertEqual({}, get_all())
6775

68-
ctx = baggage.set_baggage("test", "value")
69-
self.assertEqual(baggage.get_baggage("test", context=ctx), "value")
76+
ctx = set_baggage("test", "value")
77+
self.assertEqual(get_baggage("test", context=ctx), "value")
7078

71-
ctx = baggage.clear(context=ctx)
72-
self.assertEqual(baggage.get_all(context=ctx), {})
79+
ctx = clear(context=ctx)
80+
self.assertEqual(get_all(context=ctx), {})
81+
82+
def test__is_valid_value(self):
83+
self.assertTrue(_is_valid_value("GET%20%2Fapi%2F%2Freport"))

‎opentelemetry-api/tests/baggage/test_baggage_propagation.py renamed to ‎opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,38 +14,38 @@
1414
#
1515
# type: ignore
1616

17-
import unittest
1817
from logging import WARNING
18+
from unittest import TestCase
1919
from unittest.mock import Mock, patch
2020

21-
from opentelemetry import baggage
21+
from opentelemetry.baggage import get_all, set_baggage
2222
from opentelemetry.baggage.propagation import (
2323
W3CBaggagePropagator,
2424
_format_baggage,
2525
)
2626
from opentelemetry.context import get_current
2727

2828

29-
class TestBaggagePropagation(unittest.TestCase):
29+
class TestW3CBaggagePropagator(TestCase):
3030
def setUp(self):
3131
self.propagator = W3CBaggagePropagator()
3232

3333
def _extract(self, header_value):
3434
"""Test helper"""
3535
header = {"baggage": [header_value]}
36-
return baggage.get_all(self.propagator.extract(header))
36+
return get_all(self.propagator.extract(header))
3737

3838
def _inject(self, values):
3939
"""Test helper"""
4040
ctx = get_current()
4141
for k, v in values.items():
42-
ctx = baggage.set_baggage(k, v, context=ctx)
42+
ctx = set_baggage(k, v, context=ctx)
4343
output = {}
4444
self.propagator.inject(output, context=ctx)
4545
return output.get("baggage")
4646

4747
def test_no_context_header(self):
48-
baggage_entries = baggage.get_all(self.propagator.extract({}))
48+
baggage_entries = get_all(self.propagator.extract({}))
4949
self.assertEqual(baggage_entries, {})
5050

5151
def test_empty_context_header(self):
@@ -57,10 +57,9 @@ def test_valid_header(self):
5757
expected = {"key1": "val1", "key2": "val2"}
5858
self.assertEqual(self._extract(header), expected)
5959

60-
def test_valid_header_with_space(self):
60+
def test_invalid_header_with_space(self):
6161
header = "key1 = val1, key2 =val2 "
62-
expected = {"key1": "val1", "key2": "val2"}
63-
self.assertEqual(self._extract(header), expected)
62+
self.assertEqual(self._extract(header), {})
6463

6564
def test_valid_header_with_properties(self):
6665
header = "key1=val1,key2=val2;prop=1;prop2;prop3=2"
@@ -188,8 +187,8 @@ def test_inject_no_baggage_entries(self):
188187
output = self._inject(values)
189188
self.assertEqual(None, output)
190189

191-
def test_inject_invalid_entries(self):
192-
self.assertEqual(None, self._inject({"key": "val ue"}))
190+
def test_inject_space_entries(self):
191+
self.assertEqual("key=val+ue", self._inject({"key": "val ue"}))
193192

194193
def test_inject(self):
195194
values = {
@@ -242,3 +241,24 @@ def test__format_baggage(self):
242241
_format_baggage({"key/key": "value/value"}),
243242
"key%2Fkey=value%2Fvalue",
244243
)
244+
245+
@patch("opentelemetry.baggage._BAGGAGE_KEY", new="abc")
246+
def test_inject_extract(self):
247+
248+
carrier = {}
249+
250+
context = set_baggage(
251+
"transaction", "string with spaces", context=get_current()
252+
)
253+
254+
self.propagator.inject(carrier, context)
255+
256+
context = self.propagator.extract(carrier)
257+
258+
self.assertEqual(
259+
carrier, {"baggage": "transaction=string+with+spaces"}
260+
)
261+
262+
self.assertEqual(
263+
context, {"abc": {"transaction": "string with spaces"}}
264+
)

0 commit comments

Comments
 (0)