Fix baggage parsing for invalid percent-encoded members#8480
Conversation
90736f6 to
f8d2245
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8480 +/- ##
=============================================
+ Coverage 78.54% 90.94% +12.40%
- Complexity 8468 10211 +1743
=============================================
Files 1008 1013 +5
Lines 28824 27180 -1644
Branches 3569 3184 -385
=============================================
+ Hits 22639 24719 +2080
+ Misses 5342 1735 -3607
+ Partials 843 726 -117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3cde33d to
2e7f52f
Compare
|
@Vcode2407 I want to echo @psx95's question:
Java's current implementation is: if anything in baggage is inproperly encoded, drop everything. I think this is a fine stance to take and doesn't violate any spec required behavior. If there's a known instance where a stable, widely used baggage injector has a bug where it improperly encodes, then we might want to rethink this stance to be more flexible. But if its just the case that a one-off internal baggage injector did something wrong, I don't think its strictly a good thing to be more accommodating since being more accommodating obscures a bug in the injector code. |
|
@jack-berg Thanks for the follow-up. I didn't come across a specific issue report, and this wasn't motivated by the Go or JS implementations. While looking through the baggage parsing flow, I noticed that a single invalid percent-encoded member caused the remaining valid members in the same header to be discarded. My thought was that preserving valid members while skipping only the malformed entry could make the parser more tolerant of malformed input. That said, I understand the concern that being more permissive could obscure bugs in baggage injectors, and I agree that's an important tradeoff to consider. |
jack-berg
left a comment
There was a problem hiding this comment.
We chatted about this at the 6/25 java sig. Agreed that this is a good change because:
- Alignment with otel go and js
- Slightly reduces attack surface area: an attacker with the ability to write http headers on request could add an additional baggage header with an invalid character and cause entries from a properly encoded header to be dropped
2e7f52f to
f092d09
Compare
|
Thank you for your contribution @Vcode2407! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Description
Currently, an invalid percent-encoded baggage member can cause parsing of the remaining members in the same baggage header to abort.
For example:
When parsing
bad=va%lue, percent decoding throws anIllegalArgumentException, preventing subsequent valid members from being processed.This change skips only the invalid member and continues processing the remaining members in the header.
A regression test has been added to verify that valid members are preserved when another member contains invalid percent encoding.
Testing