From 3c2bb7c93dd2e697636479a1882f49bb0c4a362e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= <pkiekowicz@splunk.com>
Date: Sat, 7 Oct 2023 01:37:44 +0200
Subject: [PATCH] =?UTF-8?q?Fix=20`TraceContextPropagator`=20behavior=20whe?=
 =?UTF-8?q?n=20trace=20parent=20flags=20contain=E2=80=A6=20(#4893)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src/OpenTelemetry.Api/CHANGELOG.md               |  4 ++++
 .../Propagation/TraceContextPropagator.cs        |  7 ++++---
 .../W3CTraceContextTests.cs                      |  4 ++--
 .../Propagation/TraceContextPropagatorTest.cs    | 16 ++++++++++------
 4 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md
index 36b39ea19bf..6d47071c7e3 100644
--- 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
index 4fe9c5503bc..16c2456ad4a 100644
--- a/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs
+++ b/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs
@@ -195,11 +195,12 @@ 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)
         {
@@ -207,7 +208,7 @@ internal static bool TryExtractTraceparent(string traceparent, out ActivityTrace
             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
index a40075bb1e5..58ec321d600 100644
--- 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
index e932f154ad0..f7dae35547f 100644
--- 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();