Add extra checks for baggage header parsing (#1010)

* Avoid IndexOutOfBoundsException

* Add extra checks for baggage header parsing + tests

---------

Co-authored-by: Jonatan Ivanov <[email protected]>

Author: songxiaosheng

micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/W3CPropagation.java

@@ -257,20 +257,27 @@ List<AbstractMap.SimpleEntry<Baggage, String>> addBaggageToContext(String baggag
                 entry = entry.substring(0, beginningOfMetadata);
             }
             String[] keyAndValue = entry.split("=");
-            for (int i = 0; i < keyAndValue.length; i += 2) {
-                try {
-                    String key = keyAndValue[i].trim();
-                    String value = keyAndValue[i + 1].trim();
-                    Baggage baggage = this.braveBaggageManager.createBaggage(key);
-                    pairs.add(new AbstractMap.SimpleEntry<>(baggage, value));
-                }
-                catch (Exception e) {
-                    if (log.isDebugEnabled()) {
-                        log.debug("Exception occurred while trying to parse baggage with key value ["
-                                + Arrays.toString(keyAndValue) + "]. Will ignore that entry.", e);
+            if (keyAndValue.length % 2 == 0) {
+                for (int i = 0; i < keyAndValue.length - 1; i += 2) {
+                    try {
+                        String key = keyAndValue[i].trim();
+                        String value = keyAndValue[i + 1].trim();
+                        Baggage baggage = this.braveBaggageManager.createBaggage(key);
+                        pairs.add(new AbstractMap.SimpleEntry<>(baggage, value));
+                    }
+                    catch (Exception e) {
+                        if (log.isDebugEnabled()) {
+                            log.debug("Exception occurred while trying to parse baggage with key value "
+                                    + Arrays.toString(keyAndValue) + ". Will ignore that entry.", e);
+                        }
                     }
                 }
             }
+            else if (log.isDebugEnabled()) {
+                log.debug(
+                        "Unable to to parse baggage with key value since it seems something is not in key=value format: "
+                                + Arrays.toString(keyAndValue) + ". Will ignore that entry.");
+            }
         }
         return pairs;
     }

micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/test/java/io/micrometer/tracing/brave/bridge/W3CBaggagePropagatorTest.java

@@ -27,7 +27,6 @@
 import io.micrometer.tracing.Tracer;
 import io.micrometer.tracing.handler.DefaultTracingObservationHandler;
 import io.micrometer.tracing.handler.PropagatingReceiverTracingObservationHandler;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.slf4j.MDC;
 
@@ -36,6 +35,7 @@
 
 import static java.util.Collections.singletonMap;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
 
 /**
  * Test taken from OpenTelemetry.
@@ -74,6 +74,46 @@ void extract_emptyBaggageHeader() {
         assertThat(contextWithBaggage).isEqualTo(contextWithBraveBaggageFields(context));
     }
 
+    @Test
+    void extract_metadataOnlyBaggageHeader() {
+        TraceContextOrSamplingFlags context = context();
+        Map<String, String> carrier = new HashMap<>();
+        carrier.put("baggage", ";metadata");
+
+        TraceContextOrSamplingFlags contextWithBaggage = propagator.contextWithBaggage(carrier, context, Map::get);
+        assertThat(baggageEntries(contextWithBaggage)).isEmpty();
+    }
+
+    @Test
+    void extract_noValueBaggageHeader() {
+        TraceContextOrSamplingFlags context = context();
+        Map<String, String> carrier = new HashMap<>();
+        carrier.put("baggage", "a=");
+
+        TraceContextOrSamplingFlags contextWithBaggage = propagator.contextWithBaggage(carrier, context, Map::get);
+        assertThat(baggageEntries(contextWithBaggage)).isEmpty();
+    }
+
+    @Test
+    void extract_noValueButMetadataBaggageHeader() {
+        TraceContextOrSamplingFlags context = context();
+        Map<String, String> carrier = new HashMap<>();
+        carrier.put("baggage", "a=;metadata");
+
+        TraceContextOrSamplingFlags contextWithBaggage = propagator.contextWithBaggage(carrier, context, Map::get);
+        assertThat(baggageEntries(contextWithBaggage)).isEmpty();
+    }
+
+    @Test
+    void extract_keyValuesNotinPairBaggageHeader() {
+        TraceContextOrSamplingFlags context = context();
+        Map<String, String> carrier = new HashMap<>();
+        carrier.put("baggage", "a=b,oops,c=,=d,=");
+
+        TraceContextOrSamplingFlags contextWithBaggage = propagator.contextWithBaggage(carrier, context, Map::get);
+        assertThat(baggageEntries(contextWithBaggage)).containsExactly(entry("a", "b"));
+    }
+
     @Test
     void extract_singleEntry() {
         TraceContextOrSamplingFlags context = context();
@@ -147,17 +187,15 @@ private Map<String, String> baggageEntries(TraceContextOrSamplingFlags flags) {
      * data, to make sure we don't blow up with it.
      */
     @Test
-    @Disabled("We don't support additional data")
     void extract_invalidHeader() {
         TraceContextOrSamplingFlags context = context();
         Map<String, String> carrier = new HashMap<>();
         carrier.put("baggage", "key1= v;alsdf;-asdflkjasdf===asdlfkjadsf ,,a sdf9asdf-alue1; metadata-key = "
                 + "value; othermetadata, key2 =value2 , key3 =\tvalue3 ; ");
 
         TraceContextOrSamplingFlags contextWithBaggage = propagator.contextWithBaggage(carrier, context, Map::get);
-
-        Map<String, String> baggageEntries = baggageEntries(contextWithBaggage);
-        assertThat(baggageEntries).isEmpty();
+        assertThat(baggageEntries(contextWithBaggage)).containsExactly(entry("key1", "v"), entry("key2", "value2"),
+                entry("key3", "value3"));
     }
 
     @Test