Skip to content

Tolerate invalid UTF-8 and malformed percent-encoding in baggage headers#5689

Merged
marcotc merged 2 commits into
masterfrom
baggage-utf8-safety
May 6, 2026
Merged

Tolerate invalid UTF-8 and malformed percent-encoding in baggage headers#5689
marcotc merged 2 commits into
masterfrom
baggage-utf8-safety

Conversation

@p-datadog

Copy link
Copy Markdown
Member

What does this PR do?

Makes Datadog::Tracing::Distributed::Baggage#parse_baggage_header treat two classes of malformed input as "malformed header → empty baggage" rather than raising ArgumentError into the caller:

  1. baggage_header.valid_encoding? == false — UTF-8-tagged input containing invalid byte sequences. Affects String#split, String#strip, and URI.decode_www_form_component, all of which raise on invalid UTF-8.
  2. Malformed percent encoding (e.g. %XX, lone %) — raises from URI.decode_www_form_component.

Both paths now record the existing context_header_style.malformed telemetry metric and return {}, matching the shape of every other malformed-header branch in this method.

Motivation:

This PR was opened in response to review feedback on #5672 (the baggage size/item limits PR), to make the recommendations easier to evaluate as a self-contained change. The findings surfaced during a focused review of how the parser interacts with non-ASCII and malformed input.

Propagation#extract documents that it never raises — it logs and returns. That contract holds for every other distributed-trace propagator because the loop in Propagation#extract is wrapped in rescue => e. Baggage is the exception: propagate_baggage is invoked outside that rescue (lib/datadog/tracing/distributed/propagation.rb:146), so any exception raised inside Baggage#extract propagates to whoever called Propagation#extract — typically a Rack middleware in customer code.

A baggage header containing either invalid UTF-8 bytes (when the upstream hands the propagator a UTF-8-tagged string) or `%XX` is enough to trigger this. Both raises are pre-existing in the parser; this PR closes them at their source.

The change is intentionally narrow:

  • ASCII-8BIT input (Rack's default for HTTP headers) is unaffected: `valid_encoding?` is true for binary-encoded strings regardless of byte values.
  • Valid UTF-8 input is unaffected.
  • The existing malformed paths and telemetry are unchanged.

Change log entry

Yes. Stop raising on baggage headers containing invalid UTF-8 bytes or malformed percent-encoded sequences; treat them as malformed and return empty baggage.

Additional Notes:

Companion to #5672 — both touch `parse_baggage_header` but at different lines (size/item caps vs. encoding/decoding safety) and merge cleanly in either order.

How to test the change?

```
bundle exec rspec spec/datadog/tracing/distributed/baggage_spec.rb
```

Three new specs cover: invalid-UTF-8 input never raises, malformed percent-encoded input never raises, and ASCII-8BIT input continues to parse normally. Verified that the first two specs fail without the production change.

`Baggage#parse_baggage_header` raised `ArgumentError` on two classes of
malformed input, and the exception escaped `Propagation#extract` because
`propagate_baggage` runs outside the rescue block in `propagation.rb`,
contradicting `extract`'s "never raises" docstring contract.

Both paths now record the existing `context_header_style.malformed`
telemetry metric and return `{}`:

1. `baggage_header.valid_encoding? == false` — UTF-8-tagged input with
   invalid byte sequences. Triggers raises in `split`, `strip`, and
   `URI.decode_www_form_component`. Guard added at the top of the method.
2. Malformed percent encoding (`%XX`, lone `%`) — raises from
   `URI.decode_www_form_component`. Wrapped in `rescue ArgumentError`.

ASCII-8BIT input (Rack's default for HTTP headers) is unaffected:
`valid_encoding?` is true for binary regardless of byte values.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
@p-datadog p-datadog added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label May 6, 2026
@p-datadog
p-datadog requested review from a team as code owners May 6, 2026 14:44
@p-datadog
p-datadog requested a review from mabdinur May 6, 2026 14:44
@dd-octo-sts dd-octo-sts Bot added the tracing label May 6, 2026
@dd-octo-sts

dd-octo-sts Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 1 steep:ignore comment, and clears 1 steep:ignore comment.

steep:ignore comments (+1-1)Introduced:
lib/datadog/tracing/distributed/baggage.rb:234
Cleared:
lib/datadog/tracing/distributed/baggage.rb:216

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented May 6, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 97.23% (-0.00%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 84b6652 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented May 6, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-05-06 23:33:56

Comparing candidate commit 84b6652 in PR branch baggage-utf8-safety with baseline commit ea9c451 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

@marcotc
marcotc merged commit 5b3d916 into master May 6, 2026
585 checks passed
@marcotc
marcotc deleted the baggage-utf8-safety branch May 6, 2026 23:41
@dd-octo-sts dd-octo-sts Bot added this to the 2.32.0 milestone May 6, 2026
@marcotc marcotc mentioned this pull request May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants