Validate that ASCII Metadata values contain only printable ASCII Characters

api/src/main/java/io/grpc/Metadata.java

@@ -94,15 +94,19 @@ public byte[] parseBytes(byte[] serialized) {
    * Simple metadata marshaller that encodes strings as is.
    *
    * <p>This should be used with ASCII strings that only contain the characters listed in the class
-   * comment of {@link AsciiMarshaller}. Otherwise the output may be considered invalid and
-   * discarded by the transport, or the call may fail.
+   * comment of {@link AsciiMarshaller}. Otherwise an {@link IllegalArgumentException} will be
+   * thrown.
    */
   public static final AsciiMarshaller<String> ASCII_STRING_MARSHALLER =
       new AsciiMarshaller<String>() {
 
         @Override
         public String toAsciiString(String value) {
-          return value;
+          checkArgument(
+              value.chars().allMatch(c -> c >= 0x20 && c <= 0x7E),
+              "String \"%s\" contains non-printable ASCII characters",
+              value);
+          return value.trim();
         }
 
         @Override

api/src/test/java/io/grpc/MetadataTest.java

@@ -478,6 +478,17 @@ public void createFromPartial() {
     assertSame(anotherSalmon, h2.get(KEY_IMMUTABLE));
   }
 
+  @Test
+  public void failNonPrintableAsciiCharacters() {
+    String value = "José";
+
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("String \"" + value + "\" contains non-printable ASCII characters");
+
+    Metadata metadata = new Metadata();
+    metadata.put(Metadata.Key.of("test-non-printable", Metadata.ASCII_STRING_MARSHALLER), value);
+  }
+
   private static final class Fish {
     private String name;
 

core/src/main/java/io/grpc/internal/TransportFrameUtil.java

@@ -24,7 +24,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
-import java.util.logging.Logger;
 import javax.annotation.CheckReturnValue;
 
 /**
@@ -35,8 +34,6 @@
  */
 public final class TransportFrameUtil {
 
-  private static final Logger logger = Logger.getLogger(TransportFrameUtil.class.getName());
-
   private static final byte[] binaryHeaderSuffixBytes =
       Metadata.BINARY_HEADER_SUFFIX.getBytes(US_ASCII);
 
@@ -57,26 +54,14 @@ public static byte[][] toHttp2Headers(Metadata headers) {
     for (int i = 0; i < serializedHeaders.length; i += 2) {
       byte[] key = serializedHeaders[i];
       byte[] value = serializedHeaders[i + 1];
-      if (endsWith(key, binaryHeaderSuffixBytes)) {
-        // Binary header.
-        serializedHeaders[k] = key;
-        serializedHeaders[k + 1]
-            = InternalMetadata.BASE64_ENCODING_OMIT_PADDING.encode(value).getBytes(US_ASCII);
-        k += 2;
-      } else {
-        // Non-binary header.
-        // Filter out headers that contain non-spec-compliant ASCII characters.
-        // TODO(zhangkun83): only do such check in development mode since it's expensive
-        if (isSpecCompliantAscii(value)) {
-          serializedHeaders[k] = key;
-          serializedHeaders[k + 1] = value;
-          k += 2;
-        } else {
-          String keyString = new String(key, US_ASCII);
-          logger.warning("Metadata key=" + keyString + ", value=" + Arrays.toString(value)
-              + " contains invalid ASCII characters");
-        }
-      }
+      serializedHeaders[k] = key;
+      serializedHeaders[k + 1] =
+          endsWith(key, binaryHeaderSuffixBytes)
+              // Binary header.
+              ? InternalMetadata.BASE64_ENCODING_OMIT_PADDING.encode(value).getBytes(US_ASCII)
+              // Non-binary header.
+              : value;
+      k += 2;
     }
     // Fast path, everything worked out fine.
     if (k == serializedHeaders.length) {
@@ -163,18 +148,5 @@ private static boolean endsWith(byte[] subject, byte[] suffix) {
     return true;
   }
 
-  /**
-   * Returns {@code true} if {@code subject} contains only bytes that are spec-compliant ASCII
-   * characters and space.
-   */
-  private static boolean isSpecCompliantAscii(byte[] subject) {
-    for (byte b : subject) {
-      if (b < 32 || b > 126) {
-        return false;
-      }
-    }
-    return true;
-  }
-
   private TransportFrameUtil() {}
 }

core/src/test/java/io/grpc/internal/TransportFrameUtilTest.java

@@ -71,7 +71,6 @@ public void testToHttp2Headers() {
     Metadata headers = new Metadata();
     headers.put(PLAIN_STRING, COMPLIANT_ASCII_STRING);
     headers.put(BINARY_STRING, NONCOMPLIANT_ASCII_STRING);
-    headers.put(BINARY_STRING_WITHOUT_SUFFIX, NONCOMPLIANT_ASCII_STRING);
     byte[][] http2Headers = TransportFrameUtil.toHttp2Headers(headers);
     // BINARY_STRING_WITHOUT_SUFFIX should not get in because it contains non-compliant ASCII
     // characters but doesn't have "-bin" in the name.
@@ -96,7 +95,6 @@ public void testToAndFromHttp2Headers() {
     Metadata headers = new Metadata();
     headers.put(PLAIN_STRING, COMPLIANT_ASCII_STRING);
     headers.put(BINARY_STRING, NONCOMPLIANT_ASCII_STRING);
-    headers.put(BINARY_STRING_WITHOUT_SUFFIX, NONCOMPLIANT_ASCII_STRING);
     byte[][] http2Headers = TransportFrameUtil.toHttp2Headers(headers);
     byte[][] rawSerialized = TransportFrameUtil.toRawSerializedHeaders(http2Headers);
     Metadata recoveredHeaders = InternalMetadata.newMetadata(rawSerialized);