Prevent leaking stacktrace when unable to parse URI query params

misk/src/main/kotlin/misk/web/jetty/WebActionsServlet.kt

@@ -20,6 +20,7 @@ import okio.BufferedSink
 import okio.buffer
 import okio.sink
 import okio.source
+import org.eclipse.jetty.http.BadMessageException
 import org.eclipse.jetty.http.HttpMethod
 import org.eclipse.jetty.server.Request
 import org.eclipse.jetty.server.Response
@@ -70,7 +71,7 @@ internal class WebActionsServlet @Inject constructor(
     if (boundActions.any { it.action.dispatchMechanism == DispatchMechanism.GRPC }) {
       val isHttp2Enabled = config.http2
         || config.unix_domain_socket?.h2c ?: false
-        || config.unix_domain_sockets?.any { it.h2c?: false } ?: false
+        || config.unix_domain_sockets?.any { it.h2c ?: false } ?: false
       if (!isHttp2Enabled) {
         log.warn {
           "HTTP/2 must be enabled either via a unix domain socket or HTTP listener if any " +
@@ -86,12 +87,20 @@ internal class WebActionsServlet @Inject constructor(
     }
   }
 
-  override fun service(request: HttpServletRequest?, response: HttpServletResponse?) {
-    if (request?.method == "PATCH" && response != null) {
+  override fun service(request: HttpServletRequest, response: HttpServletResponse) {
+    if (request.method == "PATCH") {
       doPatch(request, response)
       return
     }
-    super.service(request, response)
+    try {
+      super.service(request, response)
+    } catch (e: Throwable) {
+      handleThrowable(
+        request,
+        response,
+        e,
+      )
+    }
   }
 
   override fun doGet(request: HttpServletRequest, response: HttpServletResponse) {
@@ -162,12 +171,37 @@ internal class WebActionsServlet @Inject constructor(
       // which are covered by the NotFoundAction.
       sendNotFound(request, response, responseBody)
     } catch (e: Throwable) {
-      log.error(e) { "Uncaught exception on ${request.dispatchMechanism()} ${request.httpUrl()}" }
+      handleThrowable(
+        request,
+        response,
+        e,
+      )
+    }
+  }
 
-      response.status = HttpURLConnection.HTTP_INTERNAL_ERROR
-      response.addHeader("Content-Type", MediaTypes.TEXT_PLAIN_UTF8)
-      response.writer.close()
+  private fun handleThrowable(
+    request: HttpServletRequest,
+    response: HttpServletResponse,
+    throwable: Throwable,
+  ) {
+    log.error(throwable) {
+      "Uncaught exception on ${request.dispatchMechanism()} ${request.httpUrl()}"
+    }
+
+    when (throwable) {
+      is BadMessageException -> {
+        response.status = HttpURLConnection.HTTP_BAD_REQUEST
+        if (throwable.message != null) {
+          response.writer.append(throwable.message)
+        }
+      }
+
+      else -> {
+        response.status = HttpURLConnection.HTTP_INTERNAL_ERROR
+      }
     }
+    response.addHeader("Content-Type", MediaTypes.TEXT_PLAIN_UTF8)
+    response.writer.close()
   }
 
   private fun sendNotFound(

misk/src/test/kotlin/misk/web/jetty/WebActionsServletTest.kt

@@ -61,6 +61,19 @@ class WebActionsServletTest {
     assertThat(response.code).isEqualTo(200)
   }
 
+  @Test
+  fun malformedUriQueryParamsResponseDoesNotContainStacktrace() {
+    val response = get(
+      path = "/potato",
+      viaUDS = false,
+      viaFileUDS = false,
+      encodedQuery = "test" to "%3C%a%3C",
+    )
+
+    assertThat(response.body.string()).isEqualTo("400: Unable to parse URI query")
+    assertThat(response.code).isEqualTo(400)
+  }
+
   @Test
   fun udsSocketSuccess() {
     val response = get("/potato", true)
@@ -123,23 +136,33 @@ class WebActionsServletTest {
     path: String,
     viaUDS: Boolean,
     viaFileUDS: Boolean = false,
-    headers: Headers = Headers.headersOf()
+    headers: Headers = Headers.headersOf(),
+    encodedQuery: Pair<String, String?>? = null,
   ): okhttp3.Response =
     with(
       Request.Builder()
         .headers(headers)
         .url(
           jettyService.httpServerUrl.newBuilder().encodedPath(path)
+            .run {
+              if (encodedQuery != null) {
+                addEncodedQueryParameter(encodedQuery.first, encodedQuery.second)
+              } else {
+                this
+              }
+            }
             .build()
         )
     ) {
       when {
         viaUDS -> {
           udsCall(get())
         }
+
         viaFileUDS -> {
           fileUdsCall(get())
         }
+
         else -> {
           call(get())
         }
@@ -178,7 +201,8 @@ class WebActionsServletTest {
           webConfig = WebServerTestingModule.TESTING_WEB_CONFIG.copy(
             unix_domain_sockets = listOf(
               WebUnixDomainSocketConfig(path = socketName),
-              WebUnixDomainSocketConfig(path = fileSocketName))
+              WebUnixDomainSocketConfig(path = fileSocketName)
+            )
           )
         )
       )