Skip to content

Commit a389ca6

Browse files
tonghuaroottonghuaroot
authored
Merge commit from fork
* fix(sdk): enforce W3C Baggage limits in BaggagePropagator extract path BaggagePropagator::extract_with_context parsed each list-member of an inbound `baggage` header in full -- splitting on `;`, decoding the percent-encoded key/value, allocating Vec<String> for property segments, and constructing a KeyValueMetadata -- before passing the result to Baggage::insert_with_metadata. The storage-side limit (MAX_KEY_VALUE_PAIRS=64, MAX_LEN_OF_ALL_PAIRS=8192) silently dropped entries once full, but per-entry allocation work continued for every attacker-supplied member. This change applies the W3C Baggage limits at the propagator boundary: - Reject the header when its byte length exceeds MAX_BAGGAGE_LENGTH (8192). The header is logged at warn level and an empty Context is returned. - Cap the outer comma-split iterator with .take(MAX_BAGGAGE_ITEMS) so parsing stops after 64 list-members regardless of header content. The injection path is unchanged; the existing Baggage storage type already enforces both limits when entries are added. Two unit tests cover the new behavior: - extract_drops_header_exceeding_max_length verifies that an oversized header produces an empty Baggage. - extract_caps_entry_count_at_max_baggage_items verifies that more than 64 syntactically valid entries are truncated to 64. References: - W3C Baggage limits: https://www.w3.org/TR/baggage/#limits Signed-off-by: tonghuaroot <tonghuaroot@gmail.com> * Clarify CHANGELOG: 8192-byte over-limit drops, 64-entry over-limit truncates Address reviewer feedback that 'dropped at the propagator boundary' was ambiguous: the over-length header path drops the whole header, while the over-count path truncates to the first 64 list members. Split the wording into two clauses so the behavior is unambiguous. Signed-off-by: tonghuaroot <tonghuaroot@gmail.com> --------- Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
1 parent 1d3c54b commit a389ca6

2 files changed

Lines changed: 126 additions & 45 deletions

File tree

‎opentelemetry-sdk/CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,14 @@
22

33
## vNext
44

5+
- `BaggagePropagator` now enforces the W3C Baggage maximum header length
6+
(8192 bytes) and maximum list-member count (64) when extracting an inbound
7+
`baggage` header. Headers exceeding 8192 bytes are dropped at the
8+
propagator boundary; headers with more than 64 list members are
9+
truncated to the first 64 entries. The change keeps the propagator from
10+
parsing attacker-controlled input beyond the W3C limits instead of doing
11+
per-entry parse, decode, and allocation work only to discard the excess
12+
on `Baggage` insert. See https://www.w3.org/TR/baggage/#limits.
513
- Reverted the `SimpleSpanProcessor` telemetry suppression added in 0.32.0
614
(see #3494), which caused a `RefCell already borrowed` panic when a span
715
was started and dropped inside a `get_active_span` (or `Context::map_current`)

‎opentelemetry-sdk/src/propagation/baggage.rs‎

Lines changed: 118 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,11 @@ use std::sync::OnceLock;
1111
static BAGGAGE_HEADER: &str = "baggage";
1212
const FRAGMENT: &AsciiSet = &CONTROLS.add(b' ').add(b'"').add(b';').add(b',').add(b'=');
1313

14+
// W3C Baggage specification limits.
15+
// See https://www.w3.org/TR/baggage/#limits
16+
const MAX_BAGGAGE_LENGTH: usize = 8192;
17+
const MAX_BAGGAGE_ITEMS: usize = 64;
18+
1419
// TODO Replace this with LazyLock once it is stable.
1520
static BAGGAGE_FIELDS: OnceLock<[String; 1]> = OnceLock::new();
1621
#[inline]
@@ -101,56 +106,74 @@ impl TextMapPropagator for BaggagePropagator {
101106
/// Extracts a `Context` with baggage values from a `Extractor`.
102107
fn extract_with_context(&self, cx: &Context, extractor: &dyn Extractor) -> Context {
103108
if let Some(header_value) = extractor.get(BAGGAGE_HEADER) {
104-
let baggage = header_value.split(',').filter_map(|context_value| {
105-
if let Some((name_and_value, props)) = context_value
106-
.split(';')
107-
.collect::<Vec<&str>>()
108-
.split_first()
109-
{
110-
let mut iter = name_and_value.split('=');
111-
if let (Some(name), Some(value)) = (iter.next(), iter.next()) {
112-
let decode_name = percent_decode_str(name).decode_utf8();
113-
let decode_value = percent_decode_str(value).decode_utf8();
114-
115-
if let (Ok(name), Ok(value)) = (decode_name, decode_value) {
116-
// Here we don't store the first ; into baggage since it should be treated
117-
// as separator rather part of metadata
118-
let decoded_props = props
119-
.iter()
120-
.flat_map(|prop| percent_decode_str(prop).decode_utf8())
121-
.map(|prop| prop.trim().to_string())
122-
.collect::<Vec<String>>()
123-
.join(";"); // join with ; because we deleted all ; when calling split above
124-
125-
Some(KeyValueMetadata::new(
126-
name.trim().to_owned(),
127-
value.trim().to_string(),
128-
decoded_props.as_str(),
129-
))
109+
// Enforce the W3C Baggage maximum header length up-front so a
110+
// single oversize header cannot drive per-entry allocation work
111+
// before the entries are dropped on insert. See
112+
// https://www.w3.org/TR/baggage/#limits.
113+
if header_value.len() > MAX_BAGGAGE_LENGTH {
114+
otel_warn!(
115+
name: "BaggagePropagator.Extract.HeaderTooLarge",
116+
message = "Baggage header exceeds W3C maximum length and was dropped",
117+
header_bytes = header_value.len() as i64,
118+
limit_bytes = MAX_BAGGAGE_LENGTH as i64,
119+
);
120+
return cx.clone();
121+
}
122+
123+
let baggage =
124+
header_value
125+
.split(',')
126+
.take(MAX_BAGGAGE_ITEMS)
127+
.filter_map(|context_value| {
128+
if let Some((name_and_value, props)) = context_value
129+
.split(';')
130+
.collect::<Vec<&str>>()
131+
.split_first()
132+
{
133+
let mut iter = name_and_value.split('=');
134+
if let (Some(name), Some(value)) = (iter.next(), iter.next()) {
135+
let decode_name = percent_decode_str(name).decode_utf8();
136+
let decode_value = percent_decode_str(value).decode_utf8();
137+
138+
if let (Ok(name), Ok(value)) = (decode_name, decode_value) {
139+
// Here we don't store the first ; into baggage since it should be treated
140+
// as separator rather part of metadata
141+
let decoded_props = props
142+
.iter()
143+
.flat_map(|prop| percent_decode_str(prop).decode_utf8())
144+
.map(|prop| prop.trim().to_string())
145+
.collect::<Vec<String>>()
146+
.join(";"); // join with ; because we deleted all ; when calling split above
147+
148+
Some(KeyValueMetadata::new(
149+
name.trim().to_owned(),
150+
value.trim().to_string(),
151+
decoded_props.as_str(),
152+
))
153+
} else {
154+
otel_warn!(
155+
name: "BaggagePropagator.Extract.InvalidUTF8",
156+
message = "Invalid UTF8 string in key values",
157+
baggage_header = header_value,
158+
);
159+
None
160+
}
161+
} else {
162+
otel_warn!(
163+
name: "BaggagePropagator.Extract.InvalidKeyValueFormat",
164+
message = "Invalid baggage key-value format",
165+
baggage_header = header_value,
166+
);
167+
None
168+
}
130169
} else {
131170
otel_warn!(
132-
name: "BaggagePropagator.Extract.InvalidUTF8",
133-
message = "Invalid UTF8 string in key values",
134-
baggage_header = header_value,
135-
);
136-
None
137-
}
138-
} else {
139-
otel_warn!(
140-
name: "BaggagePropagator.Extract.InvalidKeyValueFormat",
141-
message = "Invalid baggage key-value format",
142-
baggage_header = header_value,
143-
);
144-
None
145-
}
146-
} else {
147-
otel_warn!(
148171
name: "BaggagePropagator.Extract.InvalidFormat",
149172
message = "Invalid baggage format",
150173
baggage_header = header_value);
151-
None
152-
}
153-
});
174+
None
175+
}
176+
});
154177
cx.with_baggage(baggage)
155178
} else {
156179
cx.clone()
@@ -320,4 +343,54 @@ mod tests {
320343
}
321344
}
322345
}
346+
347+
#[test]
348+
fn extract_drops_header_exceeding_max_length() {
349+
let propagator = BaggagePropagator::new();
350+
351+
// Build a syntactically valid header longer than the W3C 8192-byte
352+
// limit. The header must be rejected outright; no entries are added.
353+
let mut header = String::with_capacity(MAX_BAGGAGE_LENGTH + 1024);
354+
let mut i = 0u32;
355+
while header.len() < MAX_BAGGAGE_LENGTH + 256 {
356+
if !header.is_empty() {
357+
header.push(',');
358+
}
359+
header.push_str(&format!("k{i}=v{i}"));
360+
i += 1;
361+
}
362+
assert!(header.len() > MAX_BAGGAGE_LENGTH);
363+
364+
let mut extractor: HashMap<String, String> = HashMap::new();
365+
extractor.insert(BAGGAGE_HEADER.to_string(), header);
366+
367+
let context = propagator.extract(&extractor);
368+
assert_eq!(context.baggage().len(), 0);
369+
}
370+
371+
#[test]
372+
fn extract_caps_entry_count_at_max_baggage_items() {
373+
let propagator = BaggagePropagator::new();
374+
375+
// Build a header whose total size stays under MAX_BAGGAGE_LENGTH but
376+
// contains more than MAX_BAGGAGE_ITEMS entries.
377+
let mut header = String::new();
378+
let entries = MAX_BAGGAGE_ITEMS + 32;
379+
for i in 0..entries {
380+
if i > 0 {
381+
header.push(',');
382+
}
383+
header.push_str(&format!("k{i:03}=v"));
384+
}
385+
assert!(header.len() <= MAX_BAGGAGE_LENGTH);
386+
387+
let mut extractor: HashMap<String, String> = HashMap::new();
388+
extractor.insert(BAGGAGE_HEADER.to_string(), header);
389+
390+
let context = propagator.extract(&extractor);
391+
// The storage type also caps at MAX_KEY_VALUE_PAIRS=64; the propagator
392+
// additionally stops parsing at MAX_BAGGAGE_ITEMS so allocator work
393+
// does not grow with attacker-supplied entry counts.
394+
assert_eq!(context.baggage().len(), MAX_BAGGAGE_ITEMS);
395+
}
323396
}

0 commit comments

Comments
 (0)