Fix/baggage propagator outbound limits#5163
Fix/baggage propagator outbound limits#5163lzchen wants to merge 10 commits intoopen-telemetry:mainfrom
Conversation
Assisted-by: Claude Opus 4.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: Claude Opus 4.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if not entry: | ||
| continue | ||
|
|
||
| if len(entry) > max_pair_length: |
There was a problem hiding this comment.
to count exact bytes I think you should encode the string to utf-8 first ? len(s.encode())
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't believe this would be a problem for just ASCII characters which the baggage spec restricts the key and values to be.
There was a problem hiding this comment.
Do we already validate that it's ASCII by the time this runs?
There was a problem hiding this comment.
We do not.
I've added encode for the header length check and is_ascii() in the check before per entry length check.
There was a problem hiding this comment.
I'm still slightly fuzzy on how we handle non-ascii. I see in the baggage spec that values should be url encoded
Any code points outside of the baggage-octet range MUST be percent-encoded. The percent code point (U+0025) MUST be percent-encoded.
So if we do that, does this code come after? That would change string length FWIW.
The _apply_baggage_limits helper already logs a warning when the maximum number of list-members is exceeded, making the early check redundant. Assisted-by: Claude Opus 4.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ) | ||
| continue | ||
|
|
||
| if count >= max_pairs: |
There was a problem hiding this comment.
Just curious, why do we limit both the amount of pairs and the total amount of bytes the pairs can have?
What matters isn't only the latter since it is the actual accurate limit of amount of information the pairs can have?
There was a problem hiding this comment.
It comes from the spec https://www.w3.org/TR/baggage/#limits, then OTel has its own max numbers
|
|
||
| baggage_entries: List[str] = split(_DELIMITER_PATTERN, header) | ||
| total_baggage_entries = self._MAX_PAIRS | ||
| baggage_entries = split(_DELIMITER_PATTERN, header) |
There was a problem hiding this comment.
I think it would be better to use _apply_baggage_limits here instead of doing it twice below.
There was a problem hiding this comment.
Sorry, I'm not sure what you mean by this? I've also refactored the logic so please take another look to see if the comment is still relevant.
ocelotl
left a comment
ocelotl
left a comment
There was a problem hiding this comment.
Just a few comments/questions ✌️
There was a problem hiding this comment.
Pull request overview
This PR enforces W3C Baggage outbound size limits in W3CBaggagePropagator.inject() and refactors limit enforcement into a shared helper used by both extraction and injection to consistently cap header size, pair size, and number of members.
Changes:
- Add shared
_apply_baggage_limits()helper and use it in bothextract()andinject(). - Enforce outbound inject limits (max pairs, max list-member length, max header length) and avoid injecting an empty baggage header when everything is dropped.
- Update/add tests and document the behavior change in the changelog (extract counting behavior).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| opentelemetry-api/src/opentelemetry/baggage/propagation/init.py | Adds shared size-limit helper; applies limits in extract() and inject(); refactors encoding helper. |
| opentelemetry-api/tests/propagators/test_w3cbaggagepropagator.py | Updates extraction log expectations; adds new injection limit tests; adjusts fields test mocking. |
| opentelemetry-api/tests/propagators/test__envcarrier.py | Updates fields test mocking to align with new injection implementation. |
| CHANGELOG.md | Notes outbound enforcement and extract counting behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…__.py Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
…y-python into fix/baggage-propagator-outbound-limits
Enforce header value and total size limits when injecting baggage entries per the W3C Baggage specification. Assisted-by: Claude Opus 4.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: Claude Opus 4.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: Claude Opus 4.6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address size limits for inject outbound calls in baggage. I refactored out length + validation logic out since we are using it for both extract and inject now. One behavioral breaking change is that previously, only VALID entries in extract counted towards the 180 limit but I believe we should count ALL entries so attackers won't be able to fill the header with a bunch of invalid entries. The spec is unclear about this.