Fix BaggageBuilder.put() silently accepting empty string keys - #8660
Conversation
Per the W3C Baggage spec (§3 definition), a baggage-name must be a
non-empty token. An empty string key is therefore invalid and should
be ignored, not stored and later propagated downstream.
The W3CBaggagePropagator.isValidBaggageKey() already correctly
rejects empty keys when *parsing* incoming headers. This change
closes the same gap on the *programmatic* builder path so that
calling Baggage.builder().put("", value).build() is a no-op,
consistent with how null keys are handled today.
- ImmutableBaggage.Builder.put(): add key.isEmpty() guard
- BaggageBuilder.java: document the empty-key contract in Javadoc
- ImmutableBaggageTest: correct put_keyEmpty to assert the right
behaviour; add put_keyEmpty_withMetadata variant
Fixes open-telemetry#8657
|
|
Pull request dashboard statusMerged · refreshed 2026-08-22 02:33 UTC Status above doesn't look right?
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8660 +/- ##
=========================================
Coverage 91.28% 91.28%
+ Complexity 10473 10472 -1
=========================================
Files 1006 1006
Lines 28277 28277
Branches 3569 3569
=========================================
Hits 25812 25812
Misses 1674 1674
Partials 791 791 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @Override | ||
| public BaggageBuilder put(String key, String value, BaggageEntryMetadata entryMetadata) { | ||
| if ((key == null) || (value == null) || (entryMetadata == null)) { | ||
| if ((key == null) || key.isEmpty() || (value == null) || (entryMetadata == null)) { |
There was a problem hiding this comment.
isEmpty() only checks for an empty string, but doesn't account for whitespace. According to the spec, a key cannot consist solely of whitespace, so maybe isBlank() should be used instead?
There was a problem hiding this comment.
isBlank was added in java 11. Would also need to go further and confirm there are printable chars as well. I'm going to merge without resolving this - feel free to open a separate issue / pr to track.
This comment has been minimized.
This comment has been minimized.
…y-java into fix/baggage-builder-empty-key-validation
|
Thank you for your contribution @itsmehotpants! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
3 participants
Problem
The W3C Baggage spec requires that a
baggage-namebe a non-empty token (§3 definition). An empty string""is therefore an invalid key and should be silently ignored, the same waynullkeys already are.Currently,
ImmutableBaggage.Builder.put()accepts empty string keys and stores them. When theW3CBaggagePropagatorlater serialises theBaggageinto a header, it includes the empty-key entry, which can produce malformedbaggageheaders like:This corrupts propagation for downstream services.
Inconsistency with the propagator
The parsing path (
W3CBaggagePropagator.isValidBaggageKey) already correctly rejects empty/blank keys when reading incoming headers:This PR closes the same gap on the programmatic builder path.
Change
ImmutableBaggage.Builder.put(): addkey.isEmpty()to the existing early-return null-guardBaggageBuilder.java: document the empty-key contract in Javadoc (links W3C spec)ImmutableBaggageTest: correct the existingput_keyEmptytest (it was asserting the buggy behaviour); addput_keyEmpty_withMetadatavariantFixes #8657