fix(api): apply baggage size and entry caps on extract#2164
Conversation
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>
mwear
left a comment
There was a problem hiding this comment.
I looked at the baggage specification for limits and found the following:
Condition 1: The resulting baggage-string contains 64 list-members or less.
Condition 2: The resulting baggage-string is of size 8192 bytes or less.
I didn't find a per-entry limit. So we can remove that on both inject and extract. MAX_ENTRIES should be reduced to 64. I realize some of these issues are inherited. Do you want to address these as part of this PR?
|
Hi @tonghuaroot! Have you had a chance to review @mwear's comment?
|
Fixes #2163.
Summary
OpenTelemetry::Baggage::Propagation::TextMapPropagatordeclares three private constants for the W3C Baggage spec limits (MAX_ENTRIES = 180,MAX_ENTRY_LENGTH = 4096,MAX_TOTAL_LENGTH = 8192). The privateencodehelper used on inject applies all three, and dedicated tests exist for each one. Theextractpath does not consult any of them, so the inject and extract directions enforce different rules.This PR hoists the existing inject-side policy into
extractso the two paths share the same rule, mirroring whatopentelemetry-go,opentelemetry-dotnet,opentelemetry-cpp, and recentopentelemetry-javaalready do on their inbound side.Changes
api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb:extractreturns the original context if the header byte size exceedsMAX_TOTAL_LENGTH.decode_entrieshelper runs the loop:breakonceMAX_ENTRIESentries have been decodednextpast any entry whose byte size exceedsMAX_ENTRY_LENGTHextractwithin the project'sMetrics/CyclomaticComplexityandMetrics/PerceivedComplexitybudgets.api/test/opentelemetry/baggage/propagation/text_map_propagator_test.rb: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 bytesenforces max of 180 entries on extractskips entries whose size exceeds the max entry length of 4096 bytesVerification
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:
MAX_TOTAL_LENGTHbecomes a no-op (returns the original context, same as the existingnil/empty?short-circuit).MAX_ENTRY_LENGTHis skipped while other entries continue to decode, matching whatencodedoes on the inject side.MAX_ENTRIESstop being decoded, matching the existing inject-side behaviour.