From 4f56ea075c7c5bd18c31ab5eb5246dc0f5485a4e Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 17 Nov 2022 15:38:29 +0100 Subject: [PATCH] Fix validation of baggage values Fixes #2934 --- CHANGELOG.md | 2 + .../src/opentelemetry/baggage/__init__.py | 11 +-- .../baggage/propagation/__init__.py | 6 +- .../src/opentelemetry/context/__init__.py | 4 +- .../tests/baggage/test_baggage.py | 79 +++++++++++-------- .../test_w3cbaggagepropagator.py} | 44 ++++++++--- 6 files changed, 89 insertions(+), 57 deletions(-) rename opentelemetry-api/tests/{baggage/test_baggage_propagation.py => propagators/test_w3cbaggagepropagator.py} (87%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 624a41a467d..e7c70aa712c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/opentelemetry-api/src/opentelemetry/baggage/__init__.py b/opentelemetry-api/src/opentelemetry/baggage/__init__.py index 8dea6dbfb94..9a740200a6f 100644 --- a/opentelemetry-api/src/opentelemetry/baggage/__init__.py +++ b/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) diff --git a/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py b/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py index edba837b816..ecdc1eab43d 100644 --- a/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py +++ b/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, diff --git a/opentelemetry-api/src/opentelemetry/context/__init__.py b/opentelemetry-api/src/opentelemetry/context/__init__.py index 97ffcf8f728..306f0937d53 100644 --- a/opentelemetry-api/src/opentelemetry/context/__init__.py +++ b/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": diff --git a/opentelemetry-api/tests/baggage/test_baggage.py b/opentelemetry-api/tests/baggage/test_baggage.py index 223b9b2ff2a..81ca145196a 100644 --- a/opentelemetry-api/tests/baggage/test_baggage.py +++ b/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")) diff --git a/opentelemetry-api/tests/baggage/test_baggage_propagation.py b/opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py similarity index 87% rename from opentelemetry-api/tests/baggage/test_baggage_propagation.py rename to opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py index 466ff3be308..e44cd29301a 100644 --- a/opentelemetry-api/tests/baggage/test_baggage_propagation.py +++ b/opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py @@ -14,11 +14,13 @@ # # 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, @@ -26,26 +28,26 @@ 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"}} + )