Adds '+' character to allowed characters in baggage value (#4898)

* Adds '+' character to allowed characters in baggade value

* Formatting fix

* Adda BaggageCodec implementation

* Additional cleanup

* Removal of Nullable method parameters.

* Additional tests for baggage decoding

Author: lmonkiewicz

api/all/src/main/java/io/opentelemetry/api/baggage/propagation/BaggageCodec.java

@@ -0,0 +1,88 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.api.baggage.propagation;
+
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+
+/**
+ * Note: This class is based on code from Apache Commons Codec. It is comprised of code from these
+ * classes:
+ *
+ * <ul>
+ *   <li><a
+ *       href="https://github.com/apache/commons-codec/blob/482df6cabfb288acb6ab3e4a732fdb93aecfa7c2/src/main/java/org/apache/commons/codec/net/URLCodec.java">org.apache.commons.codec.net.URLCodec</a>
+ *   <li><a
+ *       href="https://github.com/apache/commons-codec/blob/482df6cabfb288acb6ab3e4a732fdb93aecfa7c2/src/main/java/org/apache/commons/codec/net/Utils.java">org.apache.commons.codec.net.Utils</a>
+ * </ul>
+ *
+ * <p>Implements baggage-octet decoding in accordance with th <a
+ * href="https://w3c.github.io/baggage/#definition">Baggage header content</a> specification. All
+ * US-ASCII characters excluding CTLs, whitespace, DQUOTE, comma, semicolon and backslash are
+ * encoded in `www-form-urlencoded` encoding scheme.
+ */
+class BaggageCodec {
+
+  private static final byte ESCAPE_CHAR = '%';
+  private static final int RADIX = 16;
+
+  private BaggageCodec() {}
+
+  /**
+   * Decodes an array of URL safe 7-bit characters into an array of original bytes. Escaped
+   * characters are converted back to their original representation.
+   *
+   * @param bytes array of URL safe characters
+   * @return array of original bytes
+   */
+  private static byte[] decode(byte[] bytes) {
+    ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+    for (int i = 0; i < bytes.length; i++) {
+      int b = bytes[i];
+      if (b == ESCAPE_CHAR) {
+        try {
+          int u = digit16(bytes[++i]);
+          int l = digit16(bytes[++i]);
+          buffer.write((char) ((u << 4) + l));
+        } catch (ArrayIndexOutOfBoundsException e) { // FIXME
+          throw new IllegalArgumentException("Invalid URL encoding: ", e);
+        }
+      } else {
+        buffer.write(b);
+      }
+    }
+    return buffer.toByteArray();
+  }
+
+  /**
+   * Decodes an array of URL safe 7-bit characters into an array of original bytes. Escaped
+   * characters are converted back to their original representation.
+   *
+   * @param value string of URL safe characters
+   * @param charset encoding of given string
+   * @return decoded value
+   */
+  static String decode(String value, Charset charset) {
+    byte[] bytes = decode(value.getBytes(StandardCharsets.US_ASCII));
+    return new String(bytes, charset);
+  }
+
+  /**
+   * Returns the numeric value of the character {@code b} in radix 16.
+   *
+   * @param b The byte to be converted.
+   * @return The numeric value represented by the character in radix 16.
+   */
+  private static int digit16(byte b) {
+    int i = Character.digit((char) b, RADIX);
+    if (i == -1) {
+      throw new IllegalArgumentException( // FIXME
+          "Invalid URL encoding: not a valid digit (radix " + RADIX + "): " + b);
+    }
+    return i;
+  }
+}

api/all/src/main/java/io/opentelemetry/api/baggage/propagation/Parser.java

@@ -7,8 +7,6 @@
 
 import io.opentelemetry.api.baggage.BaggageBuilder;
 import io.opentelemetry.api.baggage.BaggageEntryMetadata;
-import java.io.UnsupportedEncodingException;
-import java.net.URLDecoder;
 import java.nio.charset.StandardCharsets;
 import javax.annotation.Nullable;
 
@@ -64,6 +62,8 @@ void parseInto(BaggageBuilder baggageBuilder) {
               } else {
                 skipToNext = true;
               }
+            } else if (state == State.VALUE) {
+              skipToNext = !value.tryNextChar(current, i);
             }
             break;
           }
@@ -146,11 +146,7 @@ private static String decodeValue(@Nullable String value) {
     if (value == null) {
       return null;
     }
-    try {
-      return URLDecoder.decode(value, StandardCharsets.UTF_8.name());
-    } catch (UnsupportedEncodingException e) {
-      return null;
-    }
+    return BaggageCodec.decode(value, StandardCharsets.UTF_8);
   }
 
   /**

api/all/src/test/java/io/opentelemetry/api/baggage/propagation/BaggageCodecTest.java

@@ -0,0 +1,42 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.api.baggage.propagation;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.nio.charset.StandardCharsets;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+class BaggageCodecTest {
+
+  @ParameterizedTest
+  @CsvSource(
+      quoteCharacter = ';', // default is a quote character "'", which collides with %27 character.
+      value = {
+        "%21,!", "%23,#", "%24,$", "%25,%", "%26,&", "%27,'", "%28,(", "%29,)", "%2A,*", "%2B,+",
+        "%2D,-", "%2E,.", "%2F,/", "%30,0", "%31,1", "%32,2", "%33,3", "%34,4", "%35,5", "%36,6",
+        "%37,7", "%38,8", "%39,9", "%3A,:", "%3C,<", "%3D,=", "%3E,>", "%3F,?", "%40,@", "%41,A",
+        "%42,B", "%43,C", "%44,D", "%45,E", "%46,F", "%47,G", "%48,H", "%49,I", "%4A,J", "%4B,K",
+        "%4C,L", "%4D,M", "%4E,N", "%4F,O", "%50,P", "%51,Q", "%52,R", "%53,S", "%54,T", "%55,U",
+        "%56,V", "%57,W", "%58,X", "%59,Y", "%5A,Z", "%5B,[", "%5D,]", "%5E,^", "%5F,_", "%60,`",
+        "%61,a", "%62,b", "%63,c", "%64,d", "%65,e", "%66,f", "%67,g", "%68,h", "%69,i", "%6A,j",
+        "%6B,k", "%6C,l", "%6D,m", "%6E,n", "%6F,o", "%70,p", "%71,q", "%72,r", "%73,s", "%74,t",
+        "%75,u", "%76,v", "%77,w", "%78,x", "%79,y", "%7A,z", "%7B,{", "%7C,|", "%7D,}", "%7E,~",
+      })
+  void shouldDecodePercentEncodedValues(String percentEncoded, String expectedDecoded) {
+    assertThat(BaggageCodec.decode(percentEncoded, StandardCharsets.UTF_8))
+        .isEqualTo(expectedDecoded);
+  }
+
+  @Test
+  void shouldThrowIfMalformedData() {
+    assertThatThrownBy(() -> BaggageCodec.decode("%1", StandardCharsets.UTF_8))
+        .isInstanceOf(IllegalArgumentException.class);
+  }
+}

api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorFuzzTest.java

@@ -18,8 +18,11 @@
 import io.opentelemetry.api.baggage.BaggageEntryMetadata;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapGetter;
+import java.util.Arrays;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import javax.annotation.Nullable;
 import org.junit.jupiter.api.Test;
 import org.junit.runner.Result;
@@ -62,6 +65,17 @@ public void roundTripAsciiValues(
       Baggage extractedBaggage = Baggage.fromContext(extractedContext);
       assertThat(extractedBaggage).isEqualTo(baggage);
     }
+
+    @Fuzz
+    public void baggageOctet(@From(BaggageOctetGenerator.class) String baggageValue) {
+      Map<String, String> carrier = new HashMap<>();
+      carrier.put("baggage", "key=" + baggageValue);
+      Context context =
+          baggagePropagator.extract(Context.current(), carrier, new MapTextMapGetter());
+      Baggage baggage = Baggage.fromContext(context);
+      String value = baggage.getEntryValue("key");
+      assertThat(value).isEqualTo(baggageValue);
+    }
   }
 
   // driver methods to avoid having to use the vintage junit engine, and to enable increasing the
@@ -79,9 +93,15 @@ void roundTripFuzzing() {
     assertThat(result.wasSuccessful()).isTrue();
   }
 
-  private static Result runTestCase(String roundTripRandomValues) {
+  @Test
+  void baggageOctetFuzzing() {
+    Result result = runTestCase("baggageOctet");
+    assertThat(result.wasSuccessful()).isTrue();
+  }
+
+  private static Result runTestCase(String testCaseName) {
     return GuidedFuzzing.run(
-        TestCases.class, roundTripRandomValues, new NoGuidance(10000, System.out), System.out);
+        TestCases.class, testCaseName, new NoGuidance(10000, System.out), System.out);
   }
 
   public static class AsciiGenerator extends AbstractStringGenerator {
@@ -109,4 +129,25 @@ public String get(Map<String, String> carrier, String key) {
       return carrier.get(key);
     }
   }
+
+  public static class BaggageOctetGenerator extends AbstractStringGenerator {
+
+    private static final Set<Character> excluded =
+        new HashSet<>(Arrays.asList(' ', '"', ',', ';', '\\', '%'));
+
+    @Override
+    protected int nextCodePoint(SourceOfRandomness random) {
+      while (true) {
+        char c = random.nextChar(' ', '~');
+        if (!excluded.contains(c)) {
+          return c;
+        }
+      }
+    }
+
+    @Override
+    protected boolean codePointInRange(int codePoint) {
+      return !excluded.contains((char) codePoint);
+    }
+  }
 }