Baggage::ToHeader/FromHeader use form-style `+` for space, which other W3C Baggage implementations read as a literal plus

オープン 初心者向け
#4,745 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

まだ誰も着手していません。

評価

難易度
2/5
見積もり時間
1〜3時間
初心者へのやさしさ
88/100
issue の種類
バグ
明瞭さ
明確に書かれている
活発さ
活発
技術スタック
cpp
領域
api

調査の方向性

api/include/opentelemetry/baggage/baggage.h の UrlEncode と UrlDecode から始め、次に api/test/baggage にある baggage エンコーディングのテストとその CMakeLists.txt を読みます。空白が %20 としてシリアライズされ、リテラルの + 文字がデコード時にそのまま保持されることを確認してください。影響を受ける期待値を更新し、baggage エンコーディング、baggage、および baggage propagator のテストを実行してください。

索引モデルが issue の本文から書いたものです。

説明

needs-triage

Describe your environment

  • opentelemetry-cpp main at e854d29, built with GCC 11.5.0, CMake 3.18.4, googletest 1.10.
  • api/include/opentelemetry/baggage/baggage.h is byte-identical on current main 3ee38aa.

Location (open-telemetry/opentelemetry-cpp @ default branch 3ee38aa):

      // 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 world is sent as hello+world and is read as hello+world. For example, opentelemetry-go decodes with url.PathUnescape (baggage.go L409), 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

環境構築

Codespaces で開く

このプロジェクトの開発コンテナを、あなたの GitHub アカウントでブラウザ上に起動します。

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

open-telemetry/opentelemetry-cpp のほかの issue

open-telemetry/opentelemetry-cpp の issue をすべて見る

似ている issue

C++ の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。