Comply with W3C Baggage specification limits #7880
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7880 +/- ##
=======================================
- Coverage 81.7% 81.3% -0.4%
=======================================
Files 304 304
Lines 23260 23287 +27
=======================================
- Hits 19012 18947 -65
- Misses 3862 3870 +8
- Partials 386 470 +84
🚀 New features to boost your workflow:
|
XSAM
requested review from
MrAlias,
dashpole,
dmathieu,
flc1125 and
pellared
as code owners
I don't think dropping all baggage is the correct interpretation of this requirement. We cannot propagate any "partial list-members", but it doesn't mean we cannot propagate complete list-members. Infact, the specification specifically states:
While dropping all may indeed be a valid "selection" of list-members, I think we can do better and just take the first, or last |
Yeah. And, this will need to refactor |
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
Overall this looks good. It addresses the critical security issue, looks W3C specification compliant, and is well tested. The error handling of Parse can be improved, and the issue in extractMultiBaggage, where it silently drops headers, needs to be fixed.
| // Header failed parsing (e.g., invalid format). | ||
| // Stop processing to maintain "first N" semantics. | ||
| break |
There was a problem hiding this comment.
Failing to parse the header value does not necessarily mean we have reached the size limit. There are other reasons a parse can fail (i.e. invalid member definitions).
Breaking here means that valid baggage that follows this header will be silently dropped:
Request with multiple Baggage headers:
Baggage: key1=val1 <- Processed
Baggage: invalid <- Parse error: stops here
Baggage: key2=val2 <- Not processed
Result: Only key1 is in baggage (key2 is LOST)
Instead this should continue to the next header. The size validation can still be checked for the result below.
| // Header failed parsing (e.g., invalid format). | |
| // Stop processing to maintain "first N" semantics. | |
| break | |
| continue |
Or, the error returned from baggage.Parse could be compared with some known error exposed from that API to check what kind of error this is.
There was a problem hiding this comment.
This also indicates a missing test case where an invalid header is sandwiched between two valid headers.
| // Non-empty header produced no members (e.g., all members exceeded limits). | ||
| // Stop processing to maintain "first N" semantics. | ||
| break |
There was a problem hiding this comment.
Again, this may not always be the reason for the empty response. And it shouldn't mean all subsequent headers are dropped.
| // Non-empty header produced no members (e.g., all members exceeded limits). | |
| // Stop processing to maintain "first N" semantics. | |
| break | |
| continue |
| for memberStr := range strings.SplitSeq(bStr, listDelimiter) { | ||
| // Check member count limit. | ||
| if len(b) >= maxMembers { | ||
| break |
There was a problem hiding this comment.
Just breaking here means that the dropped members will be done silently.
Instead, can this return a partial result and an appropriate error describing the dropped member state?
| memberBytes++ // comma separator | ||
| } | ||
| if totalBytes+memberBytes > maxBytesPerBaggageString { | ||
| break |
| } | ||
|
|
||
| b, err := baggage.New(members...) | ||
| if err != nil || b.Len() == 0 { |
There was a problem hiding this comment.
Should this support a partial baggage if returned? Maybe send the error to the global error handler?
There was a problem hiding this comment.
Should this support a partial baggage if returned?
I am not sure it is necessary as the limits have been enforced before baggage.New. We just use baggage.New to construct baggage.
Maybe send the error to the global error handler?
This can't be done as importing go.opentelemetry.io/otel would cause an import cycle.
There was a problem hiding this comment.
Can you use
opentelemetry-go/internal/global/state.go
Line 55 in 9695067
There was a problem hiding this comment.
Regardless of the error handling, doesn't it make sense to return the partial baggage even in the error case? We are trying to comply with the baggage specification and just returning the parent context here when we may have truncated does not seem appropriate. It doesn't resolve the security issue trying to be resolved.
4 participants
Updates the baggage implementation to comply with https://www.w3.org/TR/baggage/#limits:
Changed maxMembers from 180 to 64 (the W3C compliance requirement)
Removed maxBytesPerMembers (4096) - this per-member limit was not part of the W3C spec
Added limit checking in extractMultiBaggage for multiple baggage headers: