Bound jaeger-baggage parsing work by tokens rather than accepted entries - #8702
Merged
jack-berg merged 4 commits intoAug 7, 2026
Merged
Conversation
parseBaggageHeader only incremented entriesAdded for well-formed key=value tokens, so the maxEntries loop guard never engaged for a header consisting entirely of malformed tokens and the loop ran to the end of the input. Bound the loop on tokens parsed instead, and guard the malformed-token log message with isLoggable so its string concatenation is not evaluated when FINE is disabled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68223853-cb33-47c8-8486-b880f9f8b4e8
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68223853-cb33-47c8-8486-b880f9f8b4e8
There was a problem hiding this comment.
Pull request overview
This PR tightens JaegerPropagator baggage parsing so the per-header parse budget applies even when the header contains malformed tokens, and avoids unnecessary log-message string concatenation when FINE logging is disabled.
Changes:
- Bound
JaegerPropagator.parseBaggageHeaderby tokens parsed (not just successfully added entries) and guard malformed-token logging withisLoggable(Level.FINE). - Added tests covering behavior at the malformed-token boundary (63 vs 64 malformed tokens).
- Documented the user-visible behavior change in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java | Enforces token-based parsing budget and avoids unnecessary log string concatenation. |
| extensions/trace-propagators/src/test/java/io/opentelemetry/extension/trace/propagation/JaegerPropagatorTest.java | Adds coverage for malformed-token boundary behavior. |
| CHANGELOG.md | Notes the new token-based stop condition for jaeger-baggage parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8702 +/- ##
============================================
+ Coverage 91.63% 91.64% +0.01%
- Complexity 10349 10350 +1
============================================
Files 1003 1003
Lines 27207 27210 +3
Branches 3198 3199 +1
============================================
+ Hits 24931 24937 +6
+ Misses 1572 1566 -6
- Partials 704 707 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jack-berg
approved these changes
Aug 7, 2026
> extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java: The Javadoc says parsing stops once `maxBytes` is exceeded, but the implementation breaks before exceeding `maxBytes` (it stops when the next entry would exceed the budget). Also, `maxEntries` now represents a token budget rather than "entries added", so renaming the parameter would make the intent clearer for future readers/maintainers. > extensions/trace-propagators/src/test/java/io/opentelemetry/extension/trace/propagation/JaegerPropagatorTest.java: This parameterized-test case description says "entry limit", but the behavior being exercised is a token-parse budget (malformed tokens now count toward the limit). Updating the description avoids confusion when reading test failures. Analysis: Both comments have the same root cause: after switching the per-header bound from accepted entries to parsed tokens, the surrounding naming and prose still described the old entry-limit semantics. The Javadoc now says parsing stops once the next entry would exceed maxBytes, which is what the byte check inside the loop actually does. The parameter is renamed maxEntries -> maxTokens so its name matches the tokensParsed counter it bounds. The loop-guard clause bytesAdded > maxBytes was also dropped because it was unreachable: bytesAdded only grows through the guarded branch that breaks whenever the next entry would push it past maxBytes, so bytesAdded can never exceed maxBytes on a subsequent iteration. Removing it leaves exactly one byte-budget check and makes code and Javadoc describe the same rule. The test argument-set name now says "token budget" instead of "entry limit", matching the malformed-token behavior it exercises. Upsides: The Javadoc, the parameter name, the loop structure, and the test case name all describe one consistent rule, so a future reader does not have to reverse-engineer whether the bound counts entries or tokens. Dropping the unreachable clause removes a condition that contradicted the documented behavior. A failure of the malformed-token case now names the budget it actually tests. Downsides: No material downside identified. The changes are naming, documentation, and removal of an unreachable condition; extract behavior is unchanged and the existing JaegerPropagatorTest cases, including both malformed-token budget cases, still pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java:305
maxTokensis currently used as both a token-parse budget and an implicit “remaining entries” budget (since the only caller passesMAX_BAGGAGE_ENTRIES - entriesAdded). This can make the per-header token budget much smaller than 64 when someuberctx-entries were already accepted, and it can drop otherwise-eligible header entries that appear after malformed tokens (e.g., 63uberctx-entries +jaeger-baggageheader starting with one malformed token then a valid token would parse only 1 token and never reach the valid one). If the intent is a fixed per-header token budget of 64 (as described in the PR/changelog), consider separatingmaxTokens(work bound) frommaxEntriesRemaining(acceptance bound) and guarding both inside the loop; then the caller can pass a constant token budget while still enforcing the overall 64-entry limit.
/**
* Parses a single {@code jaeger-baggage} header, stopping after {@code maxTokens} tokens or once
* the next entry would exceed {@code maxBytes}. The token bound is per header and counts
* malformed tokens, so a header of entirely malformed tokens cannot keep the loop running to the
* end of the input.
CHANGELOG.md:10
- Changelog heading level looks inconsistent in the Unreleased section:
#### Extensionsnests Extensions under### API, but the surrounding top-level sections are### API,### SDK,### Shims, etc. If this entry is meant to be its own top-level section (consistent with the rest of the changelog), it should be### Extensions.
#### Extensions
> `maxTokens` is currently used as both a token-parse budget and an implicit "remaining entries" budget (since the only caller passes `MAX_BAGGAGE_ENTRIES - entriesAdded`). This can make the per-header token budget much smaller than 64 when some `uberctx-` entries were already accepted, and it can drop otherwise-eligible header entries that appear after malformed tokens (e.g., 63 `uberctx-` entries + `jaeger-baggage` header starting with one malformed token then a valid token would parse only 1 token and never reach the valid one). If the intent is a fixed per-header token budget of 64 (as described in the PR/changelog), consider separating `maxTokens` (work bound) from `maxEntriesRemaining` (acceptance bound) and guarding both inside the loop; then the caller can pass a constant token budget while still enforcing the overall 64-entry limit. Analysis: The comment is correct. Switching the loop guard from entriesAdded to tokensParsed while still passing MAX_BAGGAGE_ENTRIES - entriesAdded collapsed two distinct bounds into one argument, so already-accepted uberctx- entries shrank the per-header parse budget below the 64 tokens the changelog documents, and a single malformed token could permanently consume an entry slot that was still available. parseBaggageHeader now takes maxTokens and maxEntries separately and guards both at the top of the loop. The caller passes the new MAX_BAGGAGE_HEADER_TOKENS constant for the work bound and MAX_BAGGAGE_ENTRIES - entriesAdded for the acceptance bound, so the overall 64-entry and 8192-byte caps are still enforced exactly as before. A new parameterized case covers the reviewer's scenario: 63 uberctx- entries followed by a header of one malformed token and one valid token now yields 64 entries; it fails against the previous single-budget code. Upsides: The per-header token budget is now the fixed 64 the PR description and changelog promise, independent of how many prefix entries preceded it. Malformed tokens no longer discard entry slots that valid entries could have used, removing an extraction regression relative to the pre-PR behavior. The two bounds are named and guarded separately, so neither can silently change the other's meaning again. Parse work stays bounded at 64 tokens per header, which was the point of the original change. Downsides: parseBaggageHeader gains a fifth parameter, and the worst-case token count per header rises from "remaining entries" to a constant 64, so a carrier with 63 prefix entries plus a fully malformed header now parses up to 64 tokens instead of 1. That ceiling is the one the change set out to establish and is negligible next to the header length itself. Extraction results are unchanged for every previously covered case. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
marked this pull request as ready for review
August 7, 2026 14:50
Pull request dashboard statusMerged · refreshed 2026-08-07 19:17 UTC Status above doesn't look right?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JaegerPropagator.parseBaggageHeaderonly incrementedentriesAddedfor well-formedkey=valuetokens, so themaxEntriesloop guard never engaged for a header consisting entirely of malformed tokens (e.g.a,a,a,...) and the loop ran to the end of the input. This bounds the loop on tokens parsed instead, so the per-header parse budget applies regardless of token validity.Also guards the malformed-token log message with
isLoggable, since its string concatenation was previously evaluated on every malformed token whether or notFINEwas enabled.This is a user-visible behavior change for pathological input: a header of 64+ malformed tokens followed by a valid entry no longer extracts that entry. Tests cover both sides of the boundary (63 vs. 64 malformed tokens).
Found while analyzing GHSA-r3rj-5mf2-3rcg. That report claimed the
String.splitpre-tokenization in this method is a DoS vector; I don't think it is (allocation is linear in the input at a flat ~17x, bounded by the transport header limit, andJaegerPropagatoris deprecated and not a default propagator), so I've left thesplitin place and am closing that advisory separately. These two items are ordinary code quality fixes, not a security fix.