From 32d3a017f1144dc1c8393d916ed1a2d1d5bef9e5 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 10 Jul 2025 17:42:40 -0700 Subject: [PATCH 1/3] Fix parsing trace parent in W3CPropagator --- .../src/System/Diagnostics/W3CPropagator.cs | 2 +- .../tests/W3CPropagatorTests.cs | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs index eb36be1d87f13f..17e13af6c8b3f1 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs @@ -606,7 +606,7 @@ private static bool IsInvalidTraceParent(string? traceParent) return true; } - if (traceParent[0] == 'f' || traceParent[1] == 'f' || IsInvalidTraceParentCharacter(traceParent[0]) || IsInvalidTraceParentCharacter(traceParent[1])) + if ((traceParent[0] == 'f' && traceParent[1] == 'f') || IsInvalidTraceParentCharacter(traceParent[0]) || IsInvalidTraceParentCharacter(traceParent[1])) { return true; } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs index d44ccae40dda6a..f41fed145b26ac 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs @@ -316,6 +316,60 @@ public void TestBaggagePropagationLimits() Assert.Equal(entryCount, decodedBaggage.Count()); } + // Trace ID format is defined in W3C Trace Context specification: + // HEXDIGLC = DIGIT / "a" / "b" / "c" / "d" / "e" / "f"; lowercase hex character + // value = version "-" version-format + // version = 2HEXDIGLC; this document assumes version 00. Version ff is forbidden + // version - format = trace - id "-" parent - id "-" trace - flags + // trace - id = 32HEXDIGLC; 16 bytes array identifier. All zeroes forbidden + // parent-id = 16HEXDIGLC ; 8 bytes array identifier. All zeroes forbidden + // trace-flags = 2HEXDIGLC ; 8 bit flags. + // . . . . . . + // Example 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01 + public static IEnumerable W3CTraceParentData() + { + // TraceId, valid data? + + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", true }; // Perfect trace parent + yield return new object[] { "0f-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", true }; // version is 0f, which is valid + yield return new object[] { "f0-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", true }; // version is f0, which is valid + yield return new object[] { "ff-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // version is ff, which is invalid + yield return new object[] { "f-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // version is one digit 'f', which is invalid + yield return new object[] { "0g-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // version is 0g, which is invalid + yield return new object[] { "0F-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // version is 0F, which is invalid, 'F' should be lower case + yield return new object[] { "00-af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // trace-id length is wrong + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e-01", false }; // parent id length is wrong + yield return new object[] { "00-00000000000000000000000000000000-b9c7c989f97918e1-01", false }; // all zeros trace-id is invalid + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-0000000000000000-01", false }; // all zeros parent id is valid + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319C-b9c7c989f97918e1-01", false }; // trace-id has upper case 'C', which is invalid + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-B9c7c989f97918e1-01", false }; // parent-id has upper case 'B', which is invalid + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-0", false }; // trace flags length is wrong + yield return new object[] { "00-0af7651916cd43dd8448ek211c80319c-b9c7c989f97918e1-01", false }; // trace-id has wrong character 'k', which is invalid + yield return new object[] { "00-0af7651916cd43dd8448ee211c80319c-b9c7c989f97918z1-01", false }; // parent-id has wrong character 'z', which is invalid + yield return new object[] { "00_0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // invalid separator, should be '-' + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c_b9c7c989f97918e1-01", false }; // invalid separator, should be '-' + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1_01", false }; // invalid separator, should be '-' + } + + [Theory] + [MemberData(nameof(W3CTraceParentData))] + public void ValidateTraceIdAndStateExtraction(string traceParent, bool isValid) + { + s_w3cPropagator.ExtractTraceIdAndState(null, (object carrier, string fieldName, out string? fieldValue, out IEnumerable? fieldValues) => + { + fieldValues = null; + fieldValue = null; + + if (fieldName == PropagatorTests.TraceParent) + { + fieldValue = traceParent; + return; + } + }, out string? traceId, out _); + + Assert.Equal(isValid, traceId is not null); + } + private static string? EncodeBaggage(IEnumerable> baggageEntries) { Activity? current = Activity.Current; From 1acfc6e3b61740475ae8cf8c9c436d798bfcc2b8 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 10 Jul 2025 17:47:11 -0700 Subject: [PATCH 2/3] Fix typo --- .../tests/W3CPropagatorTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs index f41fed145b26ac..3ed8a6aa49f351 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs @@ -340,7 +340,7 @@ public static IEnumerable W3CTraceParentData() yield return new object[] { "00-af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // trace-id length is wrong yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e-01", false }; // parent id length is wrong yield return new object[] { "00-00000000000000000000000000000000-b9c7c989f97918e1-01", false }; // all zeros trace-id is invalid - yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-0000000000000000-01", false }; // all zeros parent id is valid + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-0000000000000000-01", false }; // all zeros parent id is invalid yield return new object[] { "00-0af7651916cd43dd8448eb211c80319C-b9c7c989f97918e1-01", false }; // trace-id has upper case 'C', which is invalid yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-B9c7c989f97918e1-01", false }; // parent-id has upper case 'B', which is invalid yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-0", false }; // trace flags length is wrong From 5c4beba478a71c5b41cb7ee91ccafab60528b4be Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 11 Jul 2025 09:56:42 -0700 Subject: [PATCH 3/3] Support non-00 version --- .../src/System/Diagnostics/W3CPropagator.cs | 23 ++++++++++++++++++- .../tests/W3CPropagatorTests.cs | 10 ++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs index 17e13af6c8b3f1..239621061f19f2 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/W3CPropagator.cs @@ -601,7 +601,7 @@ private static bool NeedToEscapeBaggageValueCharacter(char c) // Example 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01 private static bool IsInvalidTraceParent(string? traceParent) { - if (string.IsNullOrEmpty(traceParent) || traceParent.Length != 55) + if (string.IsNullOrEmpty(traceParent) || traceParent.Length < 55) { return true; } @@ -611,6 +611,27 @@ private static bool IsInvalidTraceParent(string? traceParent) return true; } + if (traceParent[0] == '0' && traceParent[1] == '0') + { + // version 00 should have exactly 55 characters + if (traceParent.Length != 55) + { + return true; // invalid length for version 00 + } + } + else + { + // If a higher version is detected, the implementation SHOULD try to parse it by trying the following: + // o If the size of the header is shorter than 55 characters, the vendor should not parse the header and should restart the trace. + // o Parse trace-id (from the first dash through the next 32 characters). Vendors MUST check that the 32 characters are hex, and that they are followed by a dash (-). + // o Parse parent-id (from the second dash at the 35th position through the next 16 characters). Vendors MUST check that the 16 characters are hex and followed by a dash. + // o Parse the sampled bit of flags (2 characters from the third dash). Vendors MUST check that the 2 characters are either the end of the string or a dash. + if (traceParent.Length > 55 && traceParent[55] != '-') + { + return true; // invalid format for version other than 00 + } + } + if (traceParent[2] != '-' || traceParent[35] != '-' || traceParent[52] != '-') { return true; diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs index 3ed8a6aa49f351..21ffb24016f412 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/W3CPropagatorTests.cs @@ -326,6 +326,11 @@ public void TestBaggagePropagationLimits() // trace-flags = 2HEXDIGLC ; 8 bit flags. // . . . . . . // Example 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01 + // If a higher version is detected, the implementation SHOULD try to parse it by trying the following: + // o If the size of the header is shorter than 55 characters, the vendor should not parse the header and should restart the trace. + // o Parse trace-id (from the first dash through the next 32 characters). Vendors MUST check that the 32 characters are hex, and that they are followed by a dash (-). + // o Parse parent-id (from the second dash at the 35th position through the next 16 characters). Vendors MUST check that the 16 characters are hex and followed by a dash. + // o Parse the sampled bit of flags (2 characters from the third dash). Vendors MUST check that the 2 characters are either the end of the string or a dash. public static IEnumerable W3CTraceParentData() { // TraceId, valid data? @@ -349,6 +354,11 @@ public static IEnumerable W3CTraceParentData() yield return new object[] { "00_0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01", false }; // invalid separator, should be '-' yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c_b9c7c989f97918e1-01", false }; // invalid separator, should be '-' yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1_01", false }; // invalid separator, should be '-' + yield return new object[] { "00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-011", false }; // invalid trace parent length + yield return new object[] { "01-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-011", false }; // version higher than 00 but it supposes to have '-' after the sampling flags + + // version higher than 00, can have '-' after the sampling flags and more data. Vendors MUST NOT parse or assume anything about unknown fields for this version + yield return new object[] { "01-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01-00", true }; } [Theory]