finagle-http: make handling of invalid URI consistent across client i…

…mplementations

Problem

We have inconsistent behavior when faced with a an illegal URI
across our HTTP/1.x and HTTP/2 client implementations. For the
HTTP/2 client, this results in unhandled exceptions propogating
as `UnknownChannelExceptions` when the `java.net.URI` parser is
called under the covers on invalid input via the Netty pipeline.
For HTTP/1.x clients, we see the clients happily able to submit
a request with invalid input. We should really see a more consistent,
non-retryable exception get thrown in all cases, while still ensuring
that our server implementations consistently respond to these requests
with a `400 Bad Request` status.

Solution

Add better error handling to the `UriUtils`, which is used across
our implementations to handle invalid escape sequences. Additionally,
the error handling does not behave as expected for pure HTTP/2
clients, which do not have any validation/mapping installed in their
pipelines. Let's add tests and ensure that we install this handling
more consistently and correct this behavior through better test
coverage. We will now throw a `InvalidUriException` when attempting
to submit a request with an invalid URI and ensure this exception
is marked as non-retryable.

Result

More consistent behavior across HTTP client implementations.

JIRA Issues: CSL-10770

Differential Revision: https://phabricator.twitter.biz/D660069

CHANGELOG.rst

@@ -21,6 +21,21 @@ Breaking API Changes
 * finagle-core: `c.t.f.param.Logger` has been removed. Use external configuration supported by
   your logging backend to alter settings of `com.twitter.finagle` logger.  ``PHAB_ID=D618667``
 
+Runtime Behavior Changes
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+* finagle-http: Make handling of invalid URI consistent across client implementations. There are
+  behavioral inconsistencies amongst the current HTTP client implementations:
+
+  Our HTTP/1.x clients allow for submitting requests that contain non-ASCII characters and
+  invalid character encoded sequences, while our HTTP/2 clients will either mangle
+  the URI and strip out non-ASCII characters within the Netty pipeline or result in an
+  `UnknownChannelException` when attempting to parse invalid character encoded sequences.
+  With this change, we now consistently propagate an `InvalidUriException` result, which
+  is marked as NonRetryable for all HTTP client implementations. All HTTP server implementations
+  maintain behavior of returning a `400 Bad Request` response status, but now also correctly
+  handle invalid character encoded sequences. ``PHAB_ID=D660069``
+
 Bug Fixes
 ~~~~~~~~~~
 
@@ -34,6 +49,10 @@ Bug Fixes
 * finagle-core: `c.t.f.n.NameTreeFactory` will now discard empty elements in
   `c.t.f.NameTree.Union`s with zero weight. ``PHAB_ID=D666635``
 
+* finagle-http: All HTTP server implementations consistently return a `400 Bad Request`
+  response status when encountering a URI with invalid character encoded sequences.
+  ``PHAB_ID=D660069``
+
 21.4.0
 ------
 

finagle-base-http/src/main/scala/com/twitter/finagle/http/InvalidUriException.scala

@@ -0,0 +1,22 @@
+package com.twitter.finagle.http
+
+import com.twitter.finagle.{ChannelException, FailureFlags}
+import com.twitter.logging.{HasLogLevel, Level}
+
+private[finagle] object InvalidUriException {
+  def apply(uri: CharSequence): InvalidUriException =
+    new InvalidUriException(uri, FailureFlags.NonRetryable)
+}
+
+final class InvalidUriException private (
+  uri: CharSequence,
+  override val flags: Long)
+    extends ChannelException(new IllegalArgumentException(s"Invalid URI: $uri"))
+    with FailureFlags[InvalidUriException]
+    with HasLogLevel {
+
+  override protected def copyWithFlags(flags: Long): InvalidUriException =
+    new InvalidUriException(uri, flags)
+
+  override def logLevel: Level = Level.WARNING
+}

finagle-http/src/test/scala/com/twitter/finagle/http/AbstractEndToEndTest.scala

@@ -8,7 +8,7 @@ import com.twitter.finagle.builder.ClientBuilder
 import com.twitter.finagle.context.{Contexts, Deadline, Retries}
 import com.twitter.finagle.filter.ServerAdmissionControl
 import com.twitter.finagle.http.codec.context.LoadableHttpContext
-import com.twitter.finagle.http.service.{HttpResponseClassifier, NullService}
+import com.twitter.finagle.http.service.HttpResponseClassifier
 import com.twitter.finagle.http.{Status => HttpStatus}
 import com.twitter.finagle.http2.param.EncoderIgnoreMaxHeaderListSize
 import com.twitter.finagle.liveness.{FailureAccrualFactory, FailureDetector}
@@ -66,7 +66,6 @@ abstract class AbstractEndToEndTest
   object ClientAbort extends Feature
   object NoBodyMessage extends Feature
   object MaxHeaderSize extends Feature
-  object RequiresAsciiFilter extends Feature
 
   var saveBase: Dtab = Dtab.empty
   var statsRecv: InMemoryStatsReceiver = new InMemoryStatsReceiver()
@@ -1378,9 +1377,42 @@ abstract class AbstractEndToEndTest
     await(service.close())
   }
 
-  testIfImplemented(RequiresAsciiFilter)(
-    "server responds with 400 Bad Request if non-ascii character is present in uri") {
-    val service = NullService
+  test("client throws InvalidUriException with non-ascii character is present in uri") {
+    val expected = "/DSC02175拷貝.jpg"
+    // we shouldn't hit the service code, but if we do it is because something was incorrectly
+    // filtered out in the netty pipeline and this will help debug.
+    val service = Service.mk[Request, Response] {
+      case req if req.uri == expected =>
+        Future.value(Response())
+      case req =>
+        Future.exception(new Exception(s"Unexpected request URI: ${req.uri}"))
+    }
+    val server = serverImpl().withStatsReceiver(NullStatsReceiver).serve("localhost:*", service)
+    val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+
+    val client = clientImpl()
+      .withStatsReceiver(NullStatsReceiver)
+      .newService(s"${addr.getHostName}:${addr.getPort}", "client")
+
+    try {
+      intercept[InvalidUriException] {
+        await(client(Request(expected)))
+      }
+    } finally {
+      await(client.close())
+      await(server.close())
+    }
+  }
+
+  test("client throws InvalidUriException with illegal character encoding is present in uri") {
+    val expected = "/example/routing/json/1%%"
+
+    // we shouldn't hit the service code, but if we do it is because something was incorrectly
+    // filtered out in the netty pipeline and this will help debug.
+    val service = Service.mk[Request, Response] {
+      case req if req.uri == expected => Future.value(Response())
+      case req => Future.exception(new Exception(s"Unexpected request URI: ${req.uri}"))
+    }
     val server = serverImpl().withStatsReceiver(NullStatsReceiver).serve("localhost:*", service)
     val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
 
@@ -1389,8 +1421,9 @@ abstract class AbstractEndToEndTest
       .newService(s"${addr.getHostName}:${addr.getPort}", "client")
 
     try {
-      val rep = await(client(Request("/DSC02175拷貝.jpg")))
-      assert(rep.status == HttpStatus.BadRequest)
+      intercept[InvalidUriException] {
+        await(client(Request(expected)))
+      }
     } finally {
       await(client.close())
       await(server.close())

finagle-http/src/test/scala/com/twitter/finagle/http/Http2AlpnTest.scala

@@ -4,7 +4,6 @@ class Http2AlpnTest extends AbstractHttp2AlpnTest {
   def implName: String = "alpn http/2-multiplex"
 
   // MaxHeaderSize should be allowed when https://github.com/netty/netty/issues/8434 is fixed.
-  // The RequiresAsciiFilter is due to Netty filtering non-ASCII characters in the h2 pipeline.
   override def featureImplemented(feature: Feature): Boolean =
-    feature != MaxHeaderSize && feature != RequiresAsciiFilter && super.featureImplemented(feature)
+    feature != MaxHeaderSize && super.featureImplemented(feature)
 }

finagle-http/src/test/scala/com/twitter/finagle/http/Http2PriorKnowledgeTest.scala

@@ -4,7 +4,6 @@ class Http2PriorKnowledgeTest extends AbstractHttp2PriorKnowledgeTest {
   def implName: String = "prior knowledge http/2"
 
   // MaxHeaderSize should be allowed when https://github.com/netty/netty/issues/8434 is fixed.
-  // The RequiresAsciiFilter is due to Netty filtering non-ASCII characters in the h2 pipeline.
   override def featureImplemented(feature: Feature): Boolean =
-    feature != MaxHeaderSize && feature != RequiresAsciiFilter && super.featureImplemented(feature)
+    feature != MaxHeaderSize && super.featureImplemented(feature)
 }

finagle-http2/src/main/scala/com/twitter/finagle/http2/transport/server/H2UriValidatorHandler.scala

@@ -19,7 +19,7 @@ import io.netty.util.ReferenceCountUtil
 @Sharable
 final private[http2] object H2UriValidatorHandler extends ChannelInboundHandlerAdapter {
 
-  val HandlerName: String = "h2UriValidationHandler"
+  val HandlerName: String = "h2UriValidatorHandler"
 
   override def channelRead(ctx: ChannelHandlerContext, msg: Object): Unit = msg match {
     case headers: Http2HeadersFrame if !UriUtils.isValidUri(headers.headers().path()) =>

finagle-http2/src/test/scala/com/twitter/finagle/http2/Http2ListenerTest.scala

@@ -75,7 +75,7 @@ class Http2ListenerTest extends FunSuite {
     await(close())
   })
 
-  test("Http2Listener should not upgrade with an invalid URI")(new Ctx {
+  test("Http2Listener should not upgrade with an invalid URI (non-ASCII)")(new Ctx {
 
     await(write(s"""GET http:///DSC02175拷貝.jpg HTTP/1.1
                   |x-http2-stream-id: 1
@@ -95,4 +95,25 @@ class Http2ListenerTest extends FunSuite {
 
     await(close())
   })
+
+  test("Http2Listener should not upgrade with an invalid URI (encoding)")(new Ctx {
+
+    await(write(s"""GET http:///1%%.jpg HTTP/1.1
+                   |x-http2-stream-id: 1
+                   |upgrade: h2c
+                   |HTTP2-Settings: AAEAABAAAAIAAAABAAN_____AAQAAP__AAUAAEAAAAZ_____
+                   |connection: HTTP2-Settings,upgrade
+                   |content-length: 0
+                   |x-hello: world
+                   |
+                   |""".stripMargin.replaceAll("\n", "\r\n")))
+
+    assert(await(read()).get == """HTTP/1.0 400 Bad Request
+                                  |Connection: close
+                                  |Content-Length: 0
+                                  |
+                                  |""".stripMargin.replaceAll("\n", "\r\n"))
+
+    await(close())
+  })
 }

finagle-http2/src/test/scala/com/twitter/finagle/http2/transport/server/H2UriValidatorHandlerTest.scala

@@ -20,7 +20,7 @@ class H2UriValidatorHandlerTest extends FunSuite with MockitoSugar {
     assert(channel.readInbound[Http2HeadersFrame].headers().path() == "/abc.jpg")
   }
 
-  test("Rejects invalid URI") {
+  test("Rejects invalid URI (non-ascii chars)") {
     val channel = new EmbeddedChannel(H2UriValidatorHandler)
 
     val frame = mock[Http2HeadersFrame]
@@ -32,4 +32,16 @@ class H2UriValidatorHandlerTest extends FunSuite with MockitoSugar {
     assert(channel.readOutbound[Http2HeadersFrame].headers().status().toString == "400")
   }
 
+  test("Rejects invalid URI (encoding)") {
+    val channel = new EmbeddedChannel(H2UriValidatorHandler)
+
+    val frame = mock[Http2HeadersFrame]
+    val headers = mock[Http2Headers]
+    when(frame.headers()).thenReturn(headers)
+    when(headers.path()).thenReturn("/1%%.jpg")
+
+    assert(channel.writeInbound(frame) == false)
+    assert(channel.readOutbound[Http2HeadersFrame].headers().status().toString == "400")
+  }
+
 }

finagle-netty4-http/src/main/scala/com/twitter/finagle/netty4/http/handler/UriValidatorHandler.scala

@@ -1,11 +1,12 @@
 package com.twitter.finagle.netty4.http.handler
 
+import com.twitter.finagle.http.InvalidUriException
 import com.twitter.finagle.netty4.http.util.UriUtils
-import com.twitter.finagle.netty4.http.util.UriUtils.InvalidUriException
 import io.netty.channel.ChannelHandler.Sharable
-import io.netty.channel.{ChannelHandlerContext, ChannelInboundHandlerAdapter}
+import io.netty.channel.{ChannelDuplexHandler, ChannelHandlerContext, ChannelPromise}
 import io.netty.handler.codec.DecoderResult
 import io.netty.handler.codec.http.{HttpObject, HttpRequest}
+import io.netty.util.ReferenceCountUtil
 
 /**
  * All inbound URIs are validated in the Netty pipeline so we can
@@ -15,23 +16,36 @@ import io.netty.handler.codec.http.{HttpObject, HttpRequest}
  * to an exception if it's a client).
  */
 @Sharable
-private[finagle] object UriValidatorHandler extends ChannelInboundHandlerAdapter {
+private[finagle] object UriValidatorHandler extends ChannelDuplexHandler {
 
-  val HandlerName: String = "uriValidationHandler"
+  val HandlerName: String = "uriValidatorHandler"
 
   override def channelRead(ctx: ChannelHandlerContext, msg: scala.Any): Unit = {
     msg match {
       case req: HttpRequest =>
-        validateUri(ctx, req, req.uri())
+        validateUri(req, req.uri())
       case _ => ()
     }
 
     ctx.fireChannelRead(msg)
   }
 
-  private[this] def validateUri(ctx: ChannelHandlerContext, obj: HttpObject, uri: String): Unit =
+  override def write(
+    ctx: ChannelHandlerContext,
+    msg: Any,
+    promise: ChannelPromise
+  ): Unit =
+    msg match {
+      case req: HttpRequest if !UriUtils.isValidUri(req.uri()) =>
+        ReferenceCountUtil.release(msg)
+        ctx.fireExceptionCaught(InvalidUriException(req.uri()))
+      case _ =>
+        super.write(ctx, msg, promise)
+    }
+
+  private[this] def validateUri(obj: HttpObject, uri: String): Unit =
     if (!UriUtils.isValidUri(uri)) {
-      obj.setDecoderResult(DecoderResult.failure(new InvalidUriException(uri)))
+      obj.setDecoderResult(DecoderResult.failure(InvalidUriException(uri)))
     }
 
 }

finagle-netty4-http/src/main/scala/com/twitter/finagle/netty4/http/package.scala

@@ -111,6 +111,9 @@ package object http {
     // We're going to validate our headers right before the client exception mapper.
     fn(HeaderValidatorHandler.HandlerName, HeaderValidatorHandler)
 
+    // Let's see if we can filter out bad URI's before Netty starts handling them...
+    fn(UriValidatorHandler.HandlerName, UriValidatorHandler)
+
     // Map some client related channel exceptions to something meaningful to finagle
     fn("clientExceptionMapper", ClientExceptionMapper)
 

finagle-netty4-http/src/main/scala/com/twitter/finagle/netty4/http/util/UriUtils.scala

@@ -2,8 +2,6 @@ package com.twitter.finagle.netty4.http.util
 
 private[finagle] object UriUtils {
 
-  case class InvalidUriException(uri: CharSequence) extends Exception(s"Invalid URI found: $uri")
-
   /**
    * @return true if the uri is valid or null, false otherwise
    */
@@ -18,9 +16,14 @@ private[finagle] object UriUtils {
 
     // ex: "/1234f.jpg?query=123" should validate everything before ?query=123
 
-    while (idx <= size && ch != '?') { //the query string param will get encoded/decoded by Netty
+    while (idx < size && ch != '?') { //the query string param will get encoded/decoded by Netty
       if (isAscii(ch) && !ch.isWhitespace) {
         idx += 1
+        if (ch == '%') {
+          if (!isValidEncoding(uri, size, idx)) return false
+          idx += 2
+        }
+
         if (idx < size) ch = uri.charAt(idx)
       } else {
         return false
@@ -31,6 +34,11 @@ private[finagle] object UriUtils {
     true
   }
 
+  private[this] def isValidEncoding(uri: CharSequence, size: Int, offset: Int): Boolean =
+    (offset + 1 < size) && isHex(uri.charAt(offset)) && isHex(uri.charAt(offset + 1))
+
+  private[this] def isHex(ch: Char): Boolean = Character.digit(ch, 16) >= 0
+
   private[this] def isAscii(ch: Char): Boolean = ch < 128
 
 }

finagle-netty4-http/src/test/scala/com/twitter/finagle/netty4/http/handler/UriValidatorHandlerTest.scala

@@ -0,0 +1,54 @@
+package com.twitter.finagle.netty4.http.handler
+
+import com.twitter.finagle.http.InvalidUriException
+import io.netty.channel.embedded.EmbeddedChannel
+import io.netty.handler.codec.http.{
+  DefaultHttpRequest,
+  HttpMethod,
+  HttpRequest,
+  HttpResponse,
+  HttpVersion
+}
+import org.scalatest.FunSuite
+import org.scalatestplus.mockito.MockitoSugar
+
+class UriValidatorHandlerTest extends FunSuite with MockitoSugar {
+
+  test("Accepts valid URI") {
+    val channel = new EmbeddedChannel(UriValidatorHandler)
+    val msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/abc.jpg")
+    assert(channel.writeInbound(msg))
+    assert(channel.readInbound[HttpRequest].uri() == "/abc.jpg")
+  }
+
+  test("Marks decoder failure for invalid URI (non-ascii chars)") {
+    val channel = new EmbeddedChannel(UriValidatorHandler)
+    val msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/DSC02175拷貝.jpg")
+    assert(channel.writeInbound(msg))
+    assert(channel.readInbound[HttpResponse].decoderResult().isFailure)
+  }
+
+  test("Write exception for invalid URI (non-ascii chars)") {
+    val channel = new EmbeddedChannel(UriValidatorHandler)
+    val msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/DSC02175拷貝.jpg")
+    intercept[InvalidUriException] {
+      channel.writeOutbound(msg)
+    }
+  }
+
+  test("Marks decoder failure for invalid URI (encoding)") {
+    val channel = new EmbeddedChannel(UriValidatorHandler)
+    val msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/1%%.jpg")
+    assert(channel.writeInbound(msg))
+    assert(channel.readInbound[HttpResponse].decoderResult().isFailure)
+  }
+
+  test("Write exception for invalid URI (encoding)") {
+    val channel = new EmbeddedChannel(UriValidatorHandler)
+    val msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/1%%.jpg")
+    intercept[InvalidUriException] {
+      channel.writeOutbound(msg)
+    }
+  }
+
+}

finagle-netty4-http/src/test/scala/com/twitter/finagle/netty4/http/util/UriUtilsTest.scala

@@ -19,10 +19,21 @@ class UriUtilsTest extends FunSuite {
 
   test("accepts URI with URL encoded characters") {
     assert(UriUtils.isValidUri("/abc%20xyz%3D.jpg"))
+    assert(UriUtils.isValidUri("/abc+xyz%3D.jpg"))
+    assert(UriUtils.isValidUri("/abc/123/abc%2FKf23%2F4hQrHM%3D"))
+    assert(UriUtils.isValidUri("/%2F2F%3D"))
   }
 
   test("accepts URI with query string with non-encoded characters") {
     assert(UriUtils.isValidUri("/abc/xyz?query1=param 1234&query 2=891"))
   }
 
+  test("rejects URI with illegal escape characters") {
+    assert(UriUtils.isValidUri("/1%%") == false)
+    assert(UriUtils.isValidUri("/1%") == false)
+    assert(UriUtils.isValidUri("/1%F%F") == false)
+    assert(UriUtils.isValidUri("/%%%") == false)
+    assert(UriUtils.isValidUri("/%2%F") == false)
+  }
+
 }