Baggage::ToHeader/FromHeader use form-style `+` for space, which other W3C Baggage implementations read as a literal plus
メンテナーはふだん 1 日以内に返信
まだ誰も着手していません。
評価
調査の方向性
api/include/opentelemetry/baggage/baggage.h の UrlEncode と UrlDecode から始め、次に api/test/baggage にある baggage エンコーディングのテストとその CMakeLists.txt を読みます。空白が %20 としてシリアライズされ、リテラルの + 文字がデコード時にそのまま保持されることを確認してください。影響を受ける期待値を更新し、baggage エンコーディング、baggage、および baggage propagator のテストを実行してください。
索引モデルが issue の本文から書いたものです。
説明
Describe your environment
- opentelemetry-cpp
mainate854d29, built with GCC 11.5.0, CMake 3.18.4, googletest 1.10. api/include/opentelemetry/baggage/baggage.his byte-identical on currentmain3ee38aa.
Location (open-telemetry/opentelemetry-cpp @ default branch 3ee38aa):
api/include/opentelemetry/baggage/baggage.h:323-326:UrlEncodewrites a space as+.api/include/opentelemetry/baggage/baggage.h:358-361:UrlDecode, used byFromHeader, maps every+back to a space.
// UrlEncode
else if (c == ' ')
{
ret.push_back('+');
}
// UrlDecode
else if (str[i] == '+')
{
ret.push_back(' ');
}
Problem
C++ round-trips with itself, but the + convention is form encoding, which W3C Baggage does not use. A + is a legal baggage-octet, so a header containing hello+world is syntactically valid; the problem is interoperability, in both directions:
- C++ → spec-conforming parser:
hello worldis sent ashello+worldand is read ashello+world. For example, opentelemetry-go decodes withurl.PathUnescape(baggage.goL409), which leaves+as is. - Spec-conforming producer → C++: a literal
+, which the grammar allows without escaping (e.g.expr=1+1), is decoded by C++ as a space (1 1).
Scope: I am only reporting the space/+ mapping. C++ currently percent-encodes a literal + as %2B on output. That is allowed ("Code points which are not required to be percent-encoded MAY be percent-encoded"), and I am not reporting it as a bug.
Steps to reproduce
Reproduction
#include <gtest/gtest.h>
#include <string>
#include "opentelemetry/baggage/baggage.h"
#include "opentelemetry/nostd/shared_ptr.h"
using namespace opentelemetry::baggage;
TEST(BaggageEncodingTest, SpaceEncodedAsPercent20NotPlus)
{
opentelemetry::nostd::shared_ptr<Baggage> baggage(new Baggage{});
baggage = baggage->Set("key", "hello world");
EXPECT_EQ(baggage->ToHeader(), "key=hello%20world");
}
I added this as api/test/baggage/baggage_encoding_test.cc, registered it in api/test/baggage/CMakeLists.txt, and ran it with ctest (environment above).
What is the expected behavior?
The value hello world should be sent as hello%20world, so that any W3C Baggage parser decodes it back to hello world.
W3C Baggage (Working Draft 2024-05-30), §3.3.1.3 value: "Any code points outside of the baggage-octet range MUST be percent-encoded … Percent-encoding is defined in [RFC3986], Section 2.1". baggage-octet (%x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E) excludes space and includes + as an ordinary character. The spec's own example is serverNode=DF%2028. The spec does not use application/x-www-form-urlencoded, so + does not mean space.
What is the actual behavior?
api/test/baggage/baggage_encoding_test.cc:27: Failure
Expected equality of these values:
header
Which is: "key=hello+world"
"key=hello%20world"
[ FAILED ] BaggageEncodingTest.SpaceEncodedAsPercent20NotPlus (0 ms)
Additional context
Already fixed in open-telemetry/opentelemetry-go
PR: https://github.com/open-telemetry/opentelemetry-go/pull/4756 — "baggage: Fix escaping in Member.String" (merged 2023-12-21T08:16:13Z, merge commit 885210bf332). It switches Member.String from url.QueryEscape to url.PathEscape, so a space is written as %20 rather than +. The decode side of that SDK uses url.PathUnescape, which leaves + alone.
How opentelemetry-go fixed it:
--- a/baggage/baggage.go
+++ b/baggage/baggage.go
@@ -320,8 +320,10 @@ func (m Member) Properties() []Property { return m.properties.Copy() }
// String encodes Member into a string compliant with the W3C Baggage
// specification.
func (m Member) String() string {
- // A key is just an ASCII string, but a value is URL encoded UTF-8.
- s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.QueryEscape(m.value))
+ // A key is just an ASCII string. A value is restricted to be
+ // US-ASCII characters excluding CTLs, whitespace,
+ // DQUOTE, comma, semicolon, and backslash.
+ s := fmt.Sprintf("%s%s%s", m.key, keyValueDelimiter, url.PathEscape(m.value))
if len(m.properties) > 0 {
s = fmt.Sprintf("%s%s%s", s, propertyDelimiter, m.properties.String())
}
Suggested fix
Write a space as %20 in UrlEncode and treat + as a literal character in UrlDecode. Values that older C++ versions emitted with + are then read with a literal + by an updated C++ receiver, which is how other spec-conforming receivers already read them:
diff --git a/api/include/opentelemetry/baggage/baggage.h b/api/include/opentelemetry/baggage/baggage.h
--- a/api/include/opentelemetry/baggage/baggage.h
+++ b/api/include/opentelemetry/baggage/baggage.h
@@ -320,10 +320,6 @@ class OPENTELEMETRY_EXPORT Baggage
{
ret.push_back(c);
}
- else if (c == ' ')
- {
- ret.push_back('+');
- }
else
{
ret.push_back('%');
@@ -355,10 +351,6 @@ class OPENTELEMETRY_EXPORT Baggage
common::detail::HexDigitValue(str[i + 2])));
i += 2;
}
- else if (str[i] == '+')
- {
- ret.push_back(' ');
- }
// See https://www.w3.org/TR/baggage/#definition
else if ((str[i] >= '!') /* 21 */
&& (str[i] <= '~') /* 7E */
Verified on e854d29 (GCC 11.4.0, googletest 1.14.0, SDK and tests compiled directly with g++): the test above fails without this patch and passes with it; baggage_test and baggage_propagator_test pass with it, after changing the three baggage_test expectations that pinned the + form to %20.
I can submit a PR for this.
Checked: #3754/#3758 (values containing : rejected by the parser, fixed) and #1536/#4364 (percent-decoding OTEL_RESOURCE_ATTRIBUTES) do not cover the space/+ mapping.
- 主要言語
- C++
- スター
- 1.4k
- フォーク
- 658
- 平均マージ
- 2日 6時間
- マージ済み PR(30日)
- 55
環境構築
このプロジェクトの開発コンテナを、あなたの GitHub アカウントでブラウザ上に起動します。
- Dockerfile・Docker Compose ファイルなし
- プルリクエストのテンプレートあり
- コントリビューションガイドを読む
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
open-telemetry/opentelemetry-cpp のほかの issue
-
needs-triage
難易度 2/5 1〜3時間 初心者へのやさしさ 68/100
open-telemetry/opentelemetry-cpp#4697 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
-
TracerContext construction might result in a terminate再び着手できるかも @akshat-lakhera が 21 日前に担当しましたが、オープン中のプルリクエストはありません。 オープンtriage/accepted
難易度 2/5 1〜3時間 初心者へのやさしさ 62/100
open-telemetry/opentelemetry-cpp#4684 · コメント 2 件 · リアクション 2 件 ·
メンテナーはふだん 1 日以内に返信
-
[CI] Add Ubuntu 26.04 runners to the CI workflow対応中かも @deodattap が 34 日前に担当しました。 オープンtriage/accepted
難易度 2/5 1〜3時間 初心者へのやさしさ 72/100
open-telemetry/opentelemetry-cpp#4596 · コメント 2 件 · リアクション 1 件 ·
メンテナーはふだん 1 日以内に返信
-
[BUG] Resource::Create() throws bad_variant_access if process.executable.name isn't a string対応中かも @ryux1 が 55 日前に担当しました。 オープンbug help wanted triage/accepted
難易度 2/5 1〜3時間 初心者へのやさしさ 84/100
open-telemetry/opentelemetry-cpp#4535 · コメント 1 件 · リアクション 2 件 ·
メンテナーはふだん 1 日以内に返信
-
[BUG] OnResponse() can call std::terminate() when the response body fails to parse as JSON/protobuf対応中かも @YuEfSaEDU が 46 日前に担当しました。 オープンbug help wanted triage/accepted
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
open-telemetry/opentelemetry-cpp#4534 · コメント 2 件 · リアクション 1 件 ·
メンテナーはふだん 1 日以内に返信
open-telemetry/opentelemetry-cpp の issue をすべて見る
似ている issue
-
難易度 2/5 1〜3時間 初心者へのやさしさ 66/100
FujiNetWIFI/fujinet-firmware#2261 ·
メンテナーはふだん 1 日以内に返信
-
bug difficulty: easy good first issue help wanted
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
nagisa-kamishiro/rosehip#73 ·
メンテナーはふだん 1 日以内に返信
-
mode_has_nan gives MSVC warning C4702 (unreachable code)対応中かも @ibmibmibm が今日担当しました。 オープン
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
メンテナーはふだん 1 日以内に返信
-
難易度 2/5 1〜3時間 初心者へのやさしさ 72/100
メンテナーはふだん 1 日以内に返信
-
bug
難易度 2/5 1〜3時間 初心者へのやさしさ 62/100
pocoproject/poco#5626 ·
メンテナーはふだん 2 日以内に返信