Fix TraceContextPropagator behavior when trace parent flags contain… (

#4893)
open-telemetry · Oct 6, 2023 · 3c2bb7c · 3c2bb7c
1 parent eaf1d88
commit 3c2bb7c
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 11 deletions.
diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md
@@ -10,6 +10,10 @@
   `GetTracer` from leaking memory.
   ([#4906](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4906))
 
+* Fix `TraceContextPropagator` by validating the first digit of the hex-encoded
+  `trace-flags` field of the `traceparent` header.
+  ([#4893](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4893))
+
 ## 1.6.0
 
 Released 2023-Sep-05

diff --git a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs
@@ -195,19 +195,20 @@ internal static bool TryExtractTraceparent(string traceparent, out ActivityTrace
             return false;
         }
 
-        byte options1;
+        byte optionsLowByte;
         try
         {
             spanId = ActivitySpanId.CreateFromString(traceparent.AsSpan().Slice(VersionAndTraceIdLength, SpanIdLength));
-            options1 = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength + 1]);
+            _ = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength]); // to verify if there is no bad chars on options position
+            optionsLowByte = HexCharToByte(traceparent[VersionAndTraceIdAndSpanIdLength + 1]);
         }
         catch (ArgumentOutOfRangeException)
         {
             // it's ok to still parse tracestate
             return false;
         }
 
-        if ((options1 & 1) == 1)
+        if ((optionsLowByte & 1) == 1)
         {
             traceOptions |= ActivityTraceFlags.Recorded;
         }

diff --git a/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs b/test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/W3CTraceContextTests.cs
@@ -35,7 +35,7 @@ public class W3CTraceContextTests : IDisposable
      */
     private const string W3cTraceContextEnvVarName = "OTEL_W3CTRACECONTEXT";
     private static readonly Version AspNetCoreHostingVersion = typeof(Microsoft.AspNetCore.Hosting.Builder.IApplicationBuilderFactory).Assembly.GetName().Version;
-    private readonly HttpClient httpClient = new HttpClient();
+    private readonly HttpClient httpClient = new();
     private readonly ITestOutputHelper output;
 
     public W3CTraceContextTests(ITestOutputHelper output)
@@ -107,7 +107,7 @@ public void W3CTraceContextTestSuiteAsync(string value)
         }
         else
         {
-            Assert.StartsWith("FAILED (failures=3)", lastLine);
+            Assert.StartsWith("FAILED (failures=2)", lastLine);
         }
     }
 

diff --git a/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs b/test/OpenTelemetry.Tests/Trace/Propagation/TraceContextPropagatorTest.cs
@@ -103,12 +103,16 @@ public void IsBlankIfNoHeader()
         Assert.False(ctx.ActivityContext.IsValid());
     }
 
-    [Fact]
-    public void IsBlankIfInvalid()
+    [Theory]
+    [InlineData($"00-xyz7651916cd43dd8448eb211c80319c-{SpanId}-01")]
+    [InlineData($"00-{TraceId}-xyz7c989f97918e1-01")]
+    [InlineData($"00-{TraceId}-{SpanId}-x1")]
+    [InlineData($"00-{TraceId}-{SpanId}-1x")]
+    public void IsBlankIfInvalid(string invalidTraceParent)
     {
         var headers = new Dictionary<string, string>
         {
-            { TraceParent, $"00-xyz7651916cd43dd8448eb211c80319c-{SpanId}-01" },
+            { TraceParent, invalidTraceParent },
         };
 
         var f = new TraceContextPropagator();
@@ -191,8 +195,8 @@ public void DuplicateKeys()
         // test_tracestate_duplicated_keys
         Assert.Empty(CallTraceContextPropagator("foo=1,foo=1"));
         Assert.Empty(CallTraceContextPropagator("foo=1,foo=2"));
-        Assert.Empty(CallTraceContextPropagator(new string[] { "foo=1", "foo=1" }));
-        Assert.Empty(CallTraceContextPropagator(new string[] { "foo=1", "foo=2" }));
+        Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=1" }));
+        Assert.Empty(CallTraceContextPropagator(new[] { "foo=1", "foo=2" }));
     }
 
     [Fact]
@@ -318,7 +322,7 @@ private static string CallTraceContextPropagator(string[] tracestate)
     {
         var headers = new Dictionary<string, string[]>
         {
-            { TraceParent, new string[] { $"00-{TraceId}-{SpanId}-01" } },
+            { TraceParent, new[] { $"00-{TraceId}-{SpanId}-01" } },
             { TraceState, tracestate },
         };
         var f = new TraceContextPropagator();