fix(baggage): do not percent-encode W3C baggage metadata - #8682
Conversation
Neither the W3C Baggage spec nor the OTel baggage API require percent encoding of metadata (W3C properties). Encoding breaks property key-value shape for other implementations on extract/inject round-trips. Only baggage entry values remain percent-encoded/decoded. Fixes open-telemetry#6771
|
|
Pull request dashboard statusWaiting on reviewers · refreshed 2026-09-02 14:35 UTC Review the latest changes. Status above doesn't look right?
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8682 +/- ##
============================================
- Coverage 91.48% 91.28% -0.21%
- Complexity 10467 10509 +42
============================================
Files 1021 1006 -15
Lines 27694 28344 +650
Branches 3247 3585 +338
============================================
+ Hits 25337 25874 +537
- Misses 1615 1675 +60
- Partials 742 795 +53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jack-berg
left a comment
There was a problem hiding this comment.
Curios if @SergeyKanzhelev or @basti1302 can confirm my understanding based on your involvement in w3c/baggage#145
| } | ||
| String decodedValue; | ||
| try { | ||
| // Only baggage entry values are percent-decoded. Metadata is an opaque string and must not |
There was a problem hiding this comment.
Metadata is an opaque string
Please provide a permalink reference for this claim
There was a problem hiding this comment.
Good call — added permanent references in the code comments in 3a146dd.
OTel (the “opaque string” claim):
- Set Value → Metadata: “This should be an opaque wrapper for a string with no semantic meaning. Left opaque to allow for future functionality.”
- Propagation: “On
extract, the propagator should store all metadata as a single metadata instance per entry. Oninject, the propagator should append the metadata per the W3C specification format.”
Also reflected in this repo’s BaggageEntryMetadata Javadoc (“opaque wrapper for a String metadata value”).
There was a problem hiding this comment.
The opaqueness in these is in reference to the metadata from the API standpoint. It doesn't have anything to do with its encoded representation.
| String decodedValue; | ||
| try { | ||
| // Only baggage entry values are percent-decoded. Metadata is an opaque string and must not | ||
| // be percent-decoded (W3C baggage properties / OTel metadata). |
There was a problem hiding this comment.
Added in w3c/baggage@bbaf396
With respect to encoding and decoding, the rules for baggage values
also apply to the value part of a properties that use the key-value pair form
(e.g.property = key OWS "=" OWS value), that is:
Any code points outside of thebaggage-octetrange in the property value MUST
be percent-encoded.
There was a problem hiding this comment.
Agreed the previous comment wording was too broad. Updated in 3a146dd.
What W3C actually requires (clarified in w3c/baggage@bbaf396 / w3c/baggage#148, from w3c/baggage#145):
- List-member
value: code points outsidebaggage-octetMUST be percent-encoded (§ value / editors draft). Java already does this on inject/extract for entry values — unchanged by this PR. - Property
value(key-value form only): the same encoding/decoding rules apply to the value part ofkey OWS "=" OWS value(§ property; blame L88–L100). - Encoding does not apply to the entire properties blob as one unit: property keys,
=,;, and OWS are structural. Percent-encoding them (old Java behavior) turns e.g.ValueProp \t = \t PropValintoValueProp%20%09%20%3D%20%09%20PropVal, which other implementations parse as a single flag-like property instead of key-value — the interop bug in W3C Baggage Propagator should not percent-encode metadata #6771.
Why pass-through is still the right OTel-Java behavior
OTel deliberately does not parse W3C property structure: metadata is one opaque string per entry (extract stores a single instance; inject appends it). So the propagator cannot correctly apply (2) without inventing a property parser and changing the API model. Pass-through preserves wire structure for compliant peers and matches opentelemetry-js.
Implication: callers who put non-baggage-octet characters into a property value must supply already wire-correct metadata (encode that value part themselves). That matches the opaque design.
Happy for @SergeyKanzhelev / @basti1302 to correct this if the intent of #145 was that OTel languages should parse properties and encode/decode property values specifically rather than treat metadata as opaque.
There was a problem hiding this comment.
I agree fully with this 👍 In particular, this:
Encoding does not apply to the entire properties blob as one unit: property keys, =, ;, and OWS are structural. Percent-encoding them (old Java behavior) turns e.g. ValueProp \t = \t PropVal into ValueProp%20%09%20%3D%20%09%20PropVal, which other implementations parse as a single flag-like property instead of key-value — the interop bug in #6771.
Exactly right.
Happy for @SergeyKanzhelev / @basti1302 to correct this if the intent of #145 was that OTel languages should parse properties and encode/decode property values specifically rather than treat metadata as opaque.
I personally think the better way to ultimately resolve this would be to fully align the OTel baggage spec with the W3C baggage spec, yes. That is: Remove the notion of treating metadata as one opaque string from the OTel baggage spec, and align with treating it as a list of individual items; then only percent-encode values of property key-value pairs. Having these two very closely related specs (W3C and OTel) using a different interpretation is bound to be surprising for users, and will lead to more interop problems down the road (as it already does).
I also think that the OTel spec diverged from the W3C spec is mostly a historical mishap. IIRC the OTel baggage spec made that decision when the W3C baggage spec was still somewhat unclear on how metadata/properties are meant to be handled, so OTel just went ahead and made what seemed like a safe bet at the time. Turns out, it wasn't a safe bet.
However: Making that spec change is a much bigger undertaking, and more impactful.
I think this changeset here is at the very least a good intermediate step to fix the very real interop breakage I pointed out in #6771. The only downside is that users now would need to percent-encode the values in metadata key-value pairs on their own. But I think that's better than what we currently have.
Address review: cite OTel opaque metadata + Propagation links, and note that W3C percent-encoding applies to list-member values and property *values* only — not to the entire OTel metadata blob as one unit.
|
Thanks @jack-berg — addressed both threads:
Comment-only follow-up: @SergeyKanzhelev @basti1302 — would appreciate a confirmation that this matches the #145 / #148 intent given OTel’s opaque metadata model. |
I responded here: #8682 (comment). |
| String metadataValue = baggageEntry.getMetadata().getValue(); | ||
| String encodedMetadata = | ||
| (metadataValue != null && !metadataValue.isEmpty()) | ||
| ? encodeValue(metadataValue) | ||
| : null; | ||
| String metadata = | ||
| (metadataValue != null && !metadataValue.isEmpty()) ? metadataValue : null; | ||
| // Exit early if adding this entry causes the total length to exceed the limit | ||
| // encodedEntryLength includes a trailing comma; the final string trims exactly one, | ||
| // so the net contribution to the final length is entryLength - 1. | ||
| if (headerContent.length() + encodedEntryLength(key, encodedValue, encodedMetadata) - 1 | ||
| if (headerContent.length() + encodedEntryLength(key, encodedValue, metadata) - 1 | ||
| > MAX_BAGGAGE_BYTES) { | ||
| return; | ||
| } | ||
| headerContent.append(key).append("=").append(encodedValue); | ||
| if (encodedMetadata != null) { | ||
| headerContent.append(";").append(encodedMetadata); | ||
| if (metadata != null) { | ||
| headerContent.append(";").append(metadata); | ||
| } | ||
| headerContent.append(","); |
There was a problem hiding this comment.
Backward-compat concern with pass-through: today's per-blob encoding makes inject defensive against arbitrary caller-supplied metadata. Removing it exposes new failure modes for existing callers:
- Embedded
,: downstream parser sees a phantom list-member. - Embedded
;/=in odd positions: property parses as a different shape at the peer. - CR/LF/NUL/controls: strict HTTP clients (Netty, JDK, OkHttp) throw when setting the header.
- Non-ASCII / obs-text: undefined per RFC 9110.
The spec allows producer discretion on non-conforming input ("the behavior is undefined... it MAY remove an offending list-member...").
Proposal: extend baggageIsInvalid to validate metadata against the W3C property charset (tchar / baggage-octet / OWS / = / ;) and skip the entry on failure. This matches the existing precedent of dropping the entry when key or value is invalid, keeps output spec-conformant, and covers HTTP safety as a side effect (W3C set is a strict subset of HTTP field-value).
There was a problem hiding this comment.
Done in 20df8ae. Invalid metadata charset skips the entry.
| // so W3C property structure (keys, '=', OWS) is preserved. Do not percent-encode the | ||
| // whole blob - that would break property key-value form (see #6771). W3C requires | ||
| // percent-encoding of list-member values and of property *values* only | ||
| // (https://w3c.github.io/baggage/#property); callers must supply wire-correct metadata |
There was a problem hiding this comment.
Only include comments if they're needed. When they are needed, keep them terse and avoid those AI tells like excessive em-dash, colon, and semi-colon use.
Validate metadata against the W3C property charset and drop the entry.
Checkstyle AvoidEscapedUnicodeCharacters rejects printable \u00e9.
Description
Fixes #6771.
The W3C Baggage propagator was percent-encoding and percent-decoding metadata (W3C properties) on inject/extract. That is not required by either the W3C Baggage spec or the OTel baggage API (metadata is an opaque string). Encoding breaks property key-value shape for other implementations after an extract→inject round-trip, e.g.:
Change: leave metadata untouched on inject and extract. Baggage entry values continue to be percent-encoded/decoded as before (aligned with the W3C requirement for list-member values).
This matches the approach used by opentelemetry-js (opaque pass-through).
Testing
W3CBaggagePropagatorTest.injectexpectation (metadata not encoded)inject_doesNotPercentEncodeMetadataextract_metadataNotPercentDecodedroundTrip_metadataPreservedOpaque,(safe without encoding)Local:
All green on Temurin 21.0.12 (macOS aarch64).
Compatibility note
This is a deliberate behavior change for interop with W3C-compliant / JS implementations. Callers that relied on Java-side percent-encoding of metadata will see raw metadata strings on the wire and on extract. Values are unchanged.
CNCF CLA: may need human sign-off if the CLA bot requests it.