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

Aperta Adatta ai principianti
  • Qualcosa non va?
#4,745 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
2/5
Tempo stimato
1-3 ore
Idoneità per principianti
88/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
cpp
Ambito
api

Direzione di ricerca

Inizia con UrlEncode e UrlDecode in api/include/opentelemetry/baggage/baggage.h, poi leggi i test di codifica di baggage in api/test/baggage e il relativo CMakeLists.txt. Verifica che gli spazi vengano serializzati come %20 e che i caratteri + letterali rimangano invariati durante la decodifica; aggiorna le aspettative interessate ed esegui i test di codifica di baggage, di baggage e del propagatore di baggage.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
C++
Stelle
1.4k
Fork
658
Merge medio
2g 6h
PR unite (30g)
55

Preparare l'ambiente

Apri in Codespaces

Avvia il container di sviluppo del progetto nel browser, con il tuo account GitHub.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di open-telemetry/opentelemetry-cpp

Tutte le issue di open-telemetry/opentelemetry-cpp

Issue simili

Altre issue su C++

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.