Skip to content

fix(api): apply baggage size and entry caps on extract#2164

Open
tonghuaroot wants to merge 1 commit into
open-telemetry:mainfrom
tonghuaroot:fix-baggage-extract-cap
Open

fix(api): apply baggage size and entry caps on extract#2164
tonghuaroot wants to merge 1 commit into
open-telemetry:mainfrom
tonghuaroot:fix-baggage-extract-cap

Conversation

@tonghuaroot
Copy link
Copy Markdown

Fixes #2163.

Summary

OpenTelemetry::Baggage::Propagation::TextMapPropagator declares three private constants for the W3C Baggage spec limits (MAX_ENTRIES = 180, MAX_ENTRY_LENGTH = 4096, MAX_TOTAL_LENGTH = 8192). The private encode helper used on inject applies all three, and dedicated tests exist for each one. The extract path does not consult any of them, so the inject and extract directions enforce different rules.

This PR hoists the existing inject-side policy into extract so the two paths share the same rule, mirroring what opentelemetry-go, opentelemetry-dotnet, opentelemetry-cpp, and recent opentelemetry-java already do on their inbound side.

Changes

api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb:

  • extract returns the original context if the header byte size exceeds MAX_TOTAL_LENGTH.
  • A new private decode_entries helper runs the loop:
    • break once MAX_ENTRIES entries have been decoded
    • next past any entry whose byte size exceeds MAX_ENTRY_LENGTH
  • The loop body was factored into the helper to keep extract within the project's Metrics/CyclomaticComplexity and Metrics/PerceivedComplexity budgets.

api/test/opentelemetry/baggage/propagation/text_map_propagator_test.rb:

  • A new describe 'limits mirroring #inject' block adds three tests that parallel the existing inject-side cap tests:
    • returns the same context object when the header exceeds the total length of 8192 bytes
    • enforces max of 180 entries on extract
    • skips entries whose size exceeds the max entry length of 4096 bytes

Verification

$ cd api
$ bundle exec rake test
203 runs, 888 assertions, 0 failures, 0 errors, 0 skips

$ bundle exec rubocop lib/opentelemetry/baggage/propagation/text_map_propagator.rb \
                     test/opentelemetry/baggage/propagation/text_map_propagator_test.rb
2 files inspected, no offenses detected

Notes

This change was originally submitted via the private advisory channel (GHSA-g4r4-95p7-vp43, now closed) and the project responded that it is not considered a security vulnerability but is worth fixing, and asked for a standard issue + PR. The framing here is consistency between inject and extract, not security.

The behavior change is conservative:

  • A header within the spec limits is parsed exactly as before.
  • A header exceeding MAX_TOTAL_LENGTH becomes a no-op (returns the original context, same as the existing nil / empty? short-circuit).
  • An individual entry exceeding MAX_ENTRY_LENGTH is skipped while other entries continue to decode, matching what encode does on the inject side.
  • Entries beyond MAX_ENTRIES stop being decoded, matching the existing inject-side behaviour.

The W3C Baggage TextMapPropagator declares three constants
(MAX_ENTRIES=180, MAX_ENTRY_LENGTH=4096, MAX_TOTAL_LENGTH=8192) and
the private 'encode' helper applies all three on the inject path. The
'extract' path walks the inbound header through 'gsub' and 'split'
and iterates every entry with no corresponding gate, so the caps are
asymmetric between the two directions.

This change hoists the same policy into 'extract' so the two paths are
consistent:

  - reject the header outright when its byte size exceeds MAX_TOTAL_LENGTH
  - skip any individual entry whose byte size exceeds MAX_ENTRY_LENGTH
  - stop iterating after MAX_ENTRIES decoded entries

The loop body is factored into a private 'decode_entries' helper to
keep 'extract' within the project's complexity budget. Three new tests
cover each of the gates and mirror the existing inject-side limit
tests.

Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
Copy link
Copy Markdown
Contributor

@xuan-cao-swi xuan-cao-swi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Not related to this PR, but based on your cross-check (e.g. Cross-implementation consistency), it seems like otel spec doesn't restrict on single entry size anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

W3C Baggage extract does not enforce the MAX_ENTRIES / MAX_ENTRY_LENGTH / MAX_TOTAL_LENGTH constants the inject path enforces

2 participants