Add max baggage length as limitation#8222
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8222 +/- ##
=====================================
Coverage 82.9% 82.9%
=====================================
Files 314 314
Lines 24985 25019 +34
=====================================
+ Hits 20730 20760 +30
- Misses 3882 3884 +2
- Partials 373 375 +2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds stricter enforcement of W3C Baggage size limits across parsing (baggage) and extraction (propagation), and reduces overhead/error verbosity for malformed inputs.
Changes:
- Enforce an 8192-byte maximum baggage string size in
baggage.Parse, and cap joined parse errors to a fixed maximum. - Add an aggregate byte budget guard when extracting from multiple
baggageheader values in the propagator. - Update/add tests and a benchmark to cover oversized inputs and aggregate-budget behavior, plus a changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
propagation/baggage.go |
Adds aggregate byte budget limit when processing multiple baggage header values. |
propagation/baggage_test.go |
Updates expectations for aggregate-budget behavior and adds new extraction tests. |
baggage/baggage.go |
Adds max-size early rejection and caps how many parse errors are joined. |
baggage/baggage_test.go |
Updates parse tests for new oversize behavior; adds benchmark and error-cap test. |
CHANGELOG.md |
Adds a release note about the baggage parsing/extraction change. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…o into fix/baggage-parsing
MrAlias
left a comment
There was a problem hiding this comment.
Thanks for tightening the aggregate-size handling here. I think there is one edge case left around repeated baggage headers where the combined wire value can still exceed the intended limit. I left an inline note on the specific check with the concrete case and the adjustment that should make the enforcement consistent.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…o fix/baggage-parsing-review
285423e to
55e5b4f
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Can you add both benchmark and benchstat results to the PR description? |
| if totalBytes > maxBytesPerBaggageString { | ||
| parseErrors++ | ||
| if parseErrors <= maxParseErrors { |
| b, err := baggage.New(members...) | ||
| if err != nil { | ||
| errorhandler.GetErrorHandler().Handle(err) | ||
| truncateErr = errors.Join(truncateErr, err) |
There was a problem hiding this comment.
Loading additional members after the whole header has exceeded the limit requires more memory allocation. I think this is a trade-off: our last-one-wins is a best-effort promise when facing oversized baggage.
| // If the raw baggage-string exceeds the maximum allowed bytes (8192), an | ||
| // empty Baggage and an error are returned. If adding valid members would | ||
| // cause the parsed baggage to exceed the maximum allowed bytes (8192), or if | ||
| // the baggage-string exceeds the maximum allowed members (64), parsing stops | ||
| // and an error is returned along with the partial result. |
| if uw, ok := err.(interface{ Unwrap() []error }); ok { | ||
| parseErrors += len(uw.Unwrap()) | ||
| } else { | ||
| parseErrors++ | ||
| } | ||
| if parseErrors <= maxParseErrors { | ||
| truncateErr = errors.Join(truncateErr, err) | ||
| } |
There was a problem hiding this comment.
If one header value returns a capped joined error from baggage.Parse, extractMultiBaggage counts len(Unwrap()) and may drop that error entirely once it crosses maxParseErrors, then only emits and 1 more error(s).
The existing propagation test covers many invalid headers, not one header with many invalid members.
Uh oh!
There was an error while loading. Please reload this page.