Skip to content

Commit

Permalink
Fix validation of baggage values
Browse files Browse the repository at this point in the history
  • Loading branch information
ocelotl committed Dec 13, 2022
1 parent 0f9cfdd commit 7b28f3f
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 57 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ 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))

## Version 1.15.0/0.36b0 (2022-12-09)

- Regenerate opentelemetry-proto to be compatible with protobuf 3 and 4
Expand Down
11 changes: 1 addition & 10 deletions opentelemetry-api/src/opentelemetry/baggage/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-api/src/opentelemetry/context/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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":
Expand Down
79 changes: 47 additions & 32 deletions opentelemetry-api/tests/baggage/test_baggage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Original file line number Diff line number Diff line change
Expand Up @@ -14,38 +14,38 @@
#
# type: ignore

import unittest
from logging import WARNING
from unittest import TestCase
from unittest.mock import Mock, patch

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):
Expand All @@ -57,10 +57,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"
Expand Down Expand Up @@ -188,8 +187,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 = {
Expand Down Expand Up @@ -242,3 +241,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"}}
)

0 comments on commit 7b28f3f

Please sign in to comment.