Skip to content

Commit 88ec1b1

Browse files
ignore entire baggage header if malformed (#6743)
## Summary of changes When parsing a `baggage` header, if it is malformed in any way, ignore the entire header instead of trying to extract the valid key/value pairs. ## Reason for change This is the defined behavior in the RFC and how we implement baggage in other tracing libraries. ## Implementation details Bail our as soon as we find any invalid format in the `baggage` header. ## Test coverage Fixed existing tests and added more test cases. ## Other details n/a <!-- Fixes #{issue} --> <!-- ⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
1 parent d70d287 commit 88ec1b1

File tree

2 files changed

+55
-16
lines changed

2 files changed

+55
-16
lines changed

tracer/src/Datadog.Trace/Propagators/W3CBaggagePropagator.cs

+19-10
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,15 @@ public bool TryExtract<TCarrier, TCarrierGetter>(
8686
}
8787

8888
var baggage = ParseHeader(header!);
89-
context = new PropagationContext(spanContext: null, baggage);
90-
TelemetryFactory.Metrics.RecordCountContextHeaderStyleExtracted(MetricTags.ContextHeaderStyle.Baggage);
9189

92-
return baggage is { Count: > 0 };
90+
if (baggage is { Count: > 0 })
91+
{
92+
context = new PropagationContext(spanContext: null, baggage);
93+
TelemetryFactory.Metrics.RecordCountContextHeaderStyleExtracted(MetricTags.ContextHeaderStyle.Baggage);
94+
return true;
95+
}
96+
97+
return false;
9398
}
9499

95100
internal static void EncodeStringAndAppend(StringBuilder sb, string source, bool isKey)
@@ -264,17 +269,19 @@ internal static string CreateHeader(Baggage baggage, int maxBaggageItems, int ma
264269

265270
if (separatorPosition <= 0 || separatorPosition == span.Length - 1)
266271
{
267-
// -1: invalid, no '=' character found, e.g. "foo"
268-
// 0: invalid, key is empty, e.g. "=value" or "="
269-
// span.Length - 1: invalid, value is empty, e.g. "key=" or "="
270-
continue;
272+
// invalid format, ignore entire header.
273+
// separatorPosition == -1: no '=' character found, e.g. "foo"
274+
// separatorPosition == 0: key is empty, e.g. "=value" or "="
275+
// separatorPosition == span.Length - 1: value is empty, e.g. "key=" or "="
276+
return null;
271277
}
272278

273279
var key = Decode(span.Slice(0, separatorPosition).Trim());
274280

275281
if (key.Length == 0)
276282
{
277-
continue;
283+
// key was whitespace only. invalid format, ignore entire header.
284+
return null;
278285
}
279286

280287
// Ignore everything starting with the first `;` character after the value.
@@ -285,7 +292,8 @@ internal static string CreateHeader(Baggage baggage, int maxBaggageItems, int ma
285292
if (propertiesPosition >= 0 && propertiesPosition < separatorPosition)
286293
{
287294
// invalid, ';' character was found before the value token, e.g. "key;=value" or ";="
288-
continue;
295+
// invalid format, ignore entire header.
296+
return null;
289297
}
290298

291299
// If no ';' character is found, make sure we parse the remaining string, e.g. "key=value"
@@ -296,7 +304,8 @@ internal static string CreateHeader(Baggage baggage, int maxBaggageItems, int ma
296304

297305
if (value.Length == 0)
298306
{
299-
continue;
307+
// value was whitespace only. invalid format, ignore entire header.
308+
return null;
300309
}
301310

302311
baggage ??= new Baggage();

tracer/test/Datadog.Trace.Tests/Propagators/W3CBaggagePropagatorTests.cs

+36-6
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,11 @@ static W3CBaggagePropagatorTests()
4646
=> new()
4747
{
4848
// input header ➡️ expected key/value pairs after decoding
49+
// empty
4950
{ null, null },
5051
{ string.Empty, null },
5152
{ " ", null },
53+
// invalid
5254
{ "invalid", null },
5355
{ "invalid;", null },
5456
{ "invalid=", null },
@@ -59,18 +61,32 @@ static W3CBaggagePropagatorTests()
5961
{ "=", null },
6062
{ " = ", null },
6163
{ ";", null },
64+
// valid + invalid
65+
{ "key1=value1,", null },
66+
{ "key1=value1,key2", null },
67+
{ "key1=value1,key2=", null },
68+
{ "key1=value1,=value", null },
69+
{ "key1=value1,=", null },
70+
{ "key1=value1,key2;a=value2", null },
71+
// invalid + valid
72+
{ ",key2=value2", null },
73+
{ "key1,key2=value2", null },
74+
{ "key1=,key2=value2", null },
75+
{ "=value1,key2=value2", null },
76+
{ "=,key2=value2", null },
77+
{ "key1;a=value1,key2=value2", null },
78+
// valid
6279
{ "valid=%20", [("valid", " ")] },
6380
{ "%20=valid", [(" ", "valid")] },
6481
{ "%20=%20", [(" ", " ")] },
6582
{ "key1=value1,key2=value2", [("key1", "value1"), ("key2", "value2")] },
66-
{ "key1=value1,invalid", [("key1", "value1")] },
6783
{ "key1=value1, key2 = value2;property1;property2, key3=value3; propertyKey=propertyValue", [("key1", "value1"), ("key2", "value2"), ("key3", "value3")] }, // W3C metadata/property not currently supported so the values are discarded
6884
{ "key1=value1%2Cvalid", [("key1", "value1,valid")] },
6985
{ "key1=value1=valid", [("key1", "value1=valid")] },
7086
{ "%20key1%20=%20value%091", [(" key1 ", " value\t1")] }, // encoded whitespace
7187
{ "key1 = value1, key2 = value\t2 ", [("key1", "value1"), ("key2", "value\t2")] }, // whitespace not encoded
72-
{ "key%F0%9F%90%B6=value%E6%88%91", [("key🐶", "value我")] }, // encoded unicode
73-
{ "key🐶=value我", [("key🐶", "value我")] }, // unicode not encoded
88+
{ "key%F0%9F%90%B6=value%E6%88%91", [("key🐶", "value我")] }, // encoded unicode
89+
{ "key🐶=value我", [("key🐶", "value我")] }, // unicode not encoded
7490
};
7591

7692
[Theory]
@@ -162,16 +178,16 @@ public void CreateHeader_MaxLength(string value, int maxBaggageLength, string ex
162178
[MemberData(nameof(ExtractBaggageData))]
163179
public void ParseHeader(string inputHeader, (string Key, string Value)[] expectedPairs)
164180
{
165-
var baggage = W3CBaggagePropagator.ParseHeader(inputHeader);
181+
var baggage = W3CBaggagePropagator.ParseHeader(inputHeader)!;
166182

167-
if (expectedPairs is null || expectedPairs.Length == 0)
183+
if (expectedPairs is null)
168184
{
169185
baggage.Should().BeNull();
170186
return;
171187
}
172188

173189
baggage.Should().NotBeNull();
174-
baggage!.Count.Should().Be(expectedPairs.Length);
190+
baggage.Should().HaveCount(expectedPairs.Length);
175191

176192
foreach (var pair in expectedPairs)
177193
{
@@ -263,6 +279,20 @@ public void Extract_CarrierAndDelegate()
263279
[InlineData("")]
264280
[InlineData(" ")]
265281
[InlineData(null)]
282+
// valid + invalid
283+
[InlineData("key1=value1,")]
284+
[InlineData("key1=value1,key2")]
285+
[InlineData("key1=value1,key2=")]
286+
[InlineData("key1=value1,=value2")]
287+
[InlineData("key1=value1,=")]
288+
[InlineData("key1=value1,key2;a=value2")]
289+
// invalid + valid
290+
[InlineData(",key2=value2")]
291+
[InlineData("key1,key2=value2")]
292+
[InlineData("key1=,key2=value2")]
293+
[InlineData("=value1,key2=value2")]
294+
[InlineData("=,key2=value2")]
295+
[InlineData("key1;a=value1,key2=value2")]
266296
public void Extract_InvalidFormat(string header)
267297
{
268298
var headers = new Mock<IHeadersCollection>(MockBehavior.Strict);

0 commit comments

Comments
 (0)