Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close #235: Add support for Jetty 12 to address CVE-2024-6763 #236

Open
wants to merge 1 commit into
base: series/0.24
Choose a base branch
from
Open

Close #235: Add support for Jetty 12 to address CVE-2024-6763 #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 9 additions & 66 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,7 @@ jobs:
matrix:
os: [ubuntu-latest]
scala: [2.13, 2.12, 3]
java: [temurin@11, temurin@17]
exclude:
- scala: 2.12
java: temurin@17
- scala: 3
java: temurin@17
java: [temurin@17]
runs-on: ${{ matrix.os }}
timeout-minutes: 60
steps:
Expand All @@ -47,19 +42,6 @@ jobs:
with:
fetch-depth: 0

- name: Setup Java (temurin@11)
id: setup-java-temurin-11
if: matrix.java == 'temurin@11'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 11
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@11' && steps.setup-java-temurin-11.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@17)
id: setup-java-temurin-17
if: matrix.java == 'temurin@17'
Expand All @@ -77,26 +59,26 @@ jobs:
run: sbt githubWorkflowCheck

- name: Check headers and formatting
if: matrix.java == 'temurin@11' && matrix.os == 'ubuntu-latest'
if: matrix.java == 'temurin@17' && matrix.os == 'ubuntu-latest'
run: sbt '++ ${{ matrix.scala }}' headerCheckAll scalafmtCheckAll 'project /' scalafmtSbtCheck

- name: Test
run: sbt '++ ${{ matrix.scala }}' test

- name: Check binary compatibility
if: matrix.java == 'temurin@11' && matrix.os == 'ubuntu-latest'
if: matrix.java == 'temurin@17' && matrix.os == 'ubuntu-latest'
run: sbt '++ ${{ matrix.scala }}' mimaReportBinaryIssues

- name: Generate API documentation
if: matrix.java == 'temurin@11' && matrix.os == 'ubuntu-latest'
if: matrix.java == 'temurin@17' && matrix.os == 'ubuntu-latest'
run: sbt '++ ${{ matrix.scala }}' doc

- name: Check scalafix lints
if: matrix.java == 'temurin@11' && !startsWith(matrix.scala, '3')
if: matrix.java == 'temurin@17' && !startsWith(matrix.scala, '3')
run: sbt '++ ${{ matrix.scala }}' 'scalafixAll --check'

- name: Check unused compile dependencies
if: matrix.java == 'temurin@11'
if: matrix.java == 'temurin@17'
run: sbt '++ ${{ matrix.scala }}' unusedCompileDependenciesTest

- name: Make target directories
Expand All @@ -121,7 +103,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
java: [temurin@11]
java: [temurin@17]
runs-on: ${{ matrix.os }}
steps:
- name: Install sbt
Expand All @@ -132,19 +114,6 @@ jobs:
with:
fetch-depth: 0

- name: Setup Java (temurin@11)
id: setup-java-temurin-11
if: matrix.java == 'temurin@11'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 11
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@11' && steps.setup-java-temurin-11.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@17)
id: setup-java-temurin-17
if: matrix.java == 'temurin@17'
Expand Down Expand Up @@ -218,7 +187,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
java: [temurin@11]
java: [temurin@17]
runs-on: ${{ matrix.os }}
steps:
- name: Install sbt
Expand All @@ -229,19 +198,6 @@ jobs:
with:
fetch-depth: 0

- name: Setup Java (temurin@11)
id: setup-java-temurin-11
if: matrix.java == 'temurin@11'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 11
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@11' && steps.setup-java-temurin-11.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@17)
id: setup-java-temurin-17
if: matrix.java == 'temurin@17'
Expand All @@ -266,7 +222,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
java: [temurin@11]
java: [temurin@17]
runs-on: ${{ matrix.os }}
steps:
- name: Install sbt
Expand All @@ -277,19 +233,6 @@ jobs:
with:
fetch-depth: 0

- name: Setup Java (temurin@11)
id: setup-java-temurin-11
if: matrix.java == 'temurin@11'
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: 11
cache: sbt

- name: sbt update
if: matrix.java == 'temurin@11' && steps.setup-java-temurin-11.outputs.cache-hit == 'false'
run: sbt +update

- name: Setup Java (temurin@17)
id: setup-java-temurin-17
if: matrix.java == 'temurin@17'
Expand Down
12 changes: 7 additions & 5 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ ThisBuild / crossScalaVersions := Seq(Scala213, "2.12.20", "3.3.4")
ThisBuild / scalaVersion := Scala213 // the default Scala
ThisBuild / tlJdkRelease := Some(11)
ThisBuild / githubWorkflowJavaVersions ~= {
// Jetty 10 bumps the requirement to Java 11
_.filter { case JavaSpec(_, major) => major.toInt >= 11 }
// Jetty 12 bumps the requirement to Java 17
_.filter { case JavaSpec(_, major) => major.toInt >= 17 }
}

ThisBuild / resolvers +=
Expand All @@ -29,11 +29,12 @@ lazy val root = project
.enablePlugins(NoPublishPlugin)
.aggregate(jettyServer, jettyClient)

val jettyVersion = "10.0.24"
val jettyVersion = "12.0.15"
val http4sVersion = "0.23.29"
val http4sServletVersion = "0.24.0-RC1"
val munitCatsEffectVersion = "2.0.0"
val slf4jVersion = "1.7.25"
val scalaJava8Compat = "1.0.2"

lazy val jettyServer = project
.in(file("jetty-server"))
Expand All @@ -42,9 +43,9 @@ lazy val jettyServer = project
description := "Jetty implementation for http4s servers",
libraryDependencies ++= Seq(
"org.eclipse.jetty" % "jetty-client" % jettyVersion % Test,
"org.eclipse.jetty" % "jetty-servlet" % jettyVersion,
"org.eclipse.jetty" % "jetty-util" % jettyVersion,
"org.eclipse.jetty.http2" % "http2-server" % jettyVersion,
"org.eclipse.jetty.http2" % "jetty-http2-server" % jettyVersion,
"org.eclipse.jetty.ee8" % "jetty-ee8-servlet" % jettyVersion,
"org.http4s" %% "http4s-dsl" % http4sVersion % Test,
"org.http4s" %% "http4s-servlet" % http4sServletVersion,
"org.typelevel" %% "munit-cats-effect" % munitCatsEffectVersion % Test,
Expand Down Expand Up @@ -77,6 +78,7 @@ lazy val jettyClient = project
"org.eclipse.jetty" % "jetty-client" % jettyVersion,
"org.eclipse.jetty" % "jetty-http" % jettyVersion,
"org.eclipse.jetty" % "jetty-util" % jettyVersion,
"org.scala-lang.modules" %% "scala-java8-compat" % scalaJava8Compat,
"org.http4s" %% "http4s-client-testkit" % http4sVersion % Test,
),
)
Expand Down
2 changes: 1 addition & 1 deletion jetty-client/src/main/scala/org/http4s/jetty/client/JettyClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import cats.effect.std.Dispatcher
import cats.syntax.all._
import fs2._
import org.eclipse.jetty.client.HttpClient
import org.eclipse.jetty.client.api.{Request => JettyRequest}
import org.eclipse.jetty.client.{Request => JettyRequest}
import org.eclipse.jetty.http.{HttpVersion => JHttpVersion}
import org.http4s.client.Client
import org.log4s.Logger
Expand Down
39 changes: 26 additions & 13 deletions jetty-client/src/main/scala/org/http4s/jetty/client/ResponseListener.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,10 @@ import cats.effect.std.Queue
import cats.syntax.all._
import fs2.Stream._
import fs2._
import org.eclipse.jetty.client.api.Result
import org.eclipse.jetty.client.api.{Response => JettyResponse}
import org.eclipse.jetty.client.Result
import org.eclipse.jetty.client.{Response => JettyResponse}
import org.eclipse.jetty.http.HttpFields
import org.eclipse.jetty.http.{HttpVersion => JHttpVersion}
import org.eclipse.jetty.util.{Callback => JettyCallback}
import org.http4s.internal.CollectionCompat.CollectionConverters._
import org.http4s.jetty.client.ResponseListener.Item
import org.http4s.jetty.client.internal.loggingAsyncCallback
Expand All @@ -41,7 +40,7 @@ private[jetty] final case class ResponseListener[F[_]](
cb: Callback[Resource[F, Response[F]]],
dispatcher: Dispatcher[F],
)(implicit F: Async[F])
extends JettyResponse.Listener.Adapter {
extends JettyResponse.Listener {
import ResponseListener.logger

/* Needed to properly propagate client errors */
Expand Down Expand Up @@ -89,14 +88,14 @@ private[jetty] final case class ResponseListener[F[_]](
override def onContent(
response: JettyResponse,
content: ByteBuffer,
callback: JettyCallback,
): Unit = {
val copy = ByteBuffer.allocate(content.remaining())
copy.put(content).flip()
enqueue(Item.Buf(copy)) {
case Right(_) => F.delay(callback.succeeded())
enqueueSync(Item.Buf(copy)) {
case Right(_) =>
F.unit
case Left(e) =>
F.delay(logger.error(e)("Error in asynchronous callback")) >> F.delay(callback.failed(e))
F.delay(logger.error(e)("Error in asynchronous callback"))
}
}

Expand All @@ -115,17 +114,31 @@ private[jetty] final case class ResponseListener[F[_]](
// (the request might complete after the response has been entirely received)
override def onComplete(result: Result): Unit = ()

private def abort(t: Throwable, response: JettyResponse): Unit =
if (!response.abort(t)) // this also aborts the request
logger.error(t)("Failed to abort the response")
else
closeStream()
private def abort(t: Throwable, response: JettyResponse): Unit = {
import scala.compat.java8.FutureConverters._

dispatcher.unsafeRunAndForget(
Async[F]
.fromFuture(F.delay(response.abort(t).toScala))
.map { aborted =>
if (!aborted)
logger.error(t)("Failed to abort the response")
else
closeStream()
}
.attempt
.flatMap(loggingAsyncCallback[F, Unit](logger))
)
}

private def closeStream(): Unit =
enqueue(Item.Done)(loggingAsyncCallback[F, Unit](logger))

private def enqueue(item: Item)(cb: Either[Throwable, Unit] => F[Unit]): Unit =
dispatcher.unsafeRunAndForget(queue.offer(item.some).attempt.flatMap(cb))

private def enqueueSync(item: Item)(cb: Either[Throwable, Unit] => F[Unit]): Unit =
dispatcher.unsafeRunSync(queue.offer(item.some).attempt.flatMap(cb))
Comment on lines +140 to +141
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add it to use it for onContent. With fire-and-forget, some tests fail. My guess is:

  • There's no longer a callback to report success or failure in onContent(JettyResponse, ByteBuffer): Unit in ContentListener (Listener), which ResponseListener inherits from.
  • onContent without callback is used by onContent(Response response, Content.Chunk chunk, Runnable demander) in ContentListener.
  • If the body of onContent without callback is executed with fire-and-forget, there might be a chance that demander.run() in ContentListener.onContent(Response, Content.Chunk, Runnable) is executed in an incorrect state. Please have a look at the method body of ContentListener.onContent(Response, Content.Chunk, Runnable): 
    default void onContent(Response response, Content.Chunk chunk, Runnable demander) throws Exception
{
  onContent(response, chunk.getByteBuffer());
  demander.run();
}
  • So I had to make onContent(response, chunk.getByteBuffer()) sync with enqueueSync that I added.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the failed tests when enqueue is used instead of enqueueSync, they are as shown below. Sometimes three of them fail, and other times only two fail.

Click to expand 
org.http4s.jetty.client.JettyClientSuite:
==> X org.http4s.jetty.client.JettyClientSuite.JettyClient Repeat a simple request  0.332s munit.ComparisonFailException: value is not true
=> Obtained
false
=> Diff (- obtained, + expected)
-false
+true
    at munit.Assertions.failComparison(Assertions.scala:278)
    at apply @ munit.CatsEffectAssertions.$anonfun$assertIO$1(CatsEffectAssertions.scala:52)
    at unsafeToFuture @ munit.CatsEffectSuite$$anonfun$1.applyOrElse(CatsEffectSuite.scala:82)
    at parTraverse$extension @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$new$1(ClientRouteTestBattery.scala:81)
    at parTraverse$extension @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$new$1(ClientRouteTestBattery.scala:81)
    at parTraverse$extension @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$new$1(ClientRouteTestBattery.scala:81)
  + JettyClient POST an empty body 0.015s
  + JettyClient POST a normal body 0.009s
  + JettyClient POST a chunked body 0.009s
  + JettyClient POST a multipart body 0.048s
  + JettyClient Execute GET /chunked 0.012s
==> X org.http4s.jetty.client.JettyClientSuite.JettyClient Execute GET /large  0.036s munit.ComparisonFailException: value is not true
=> Obtained
false
=> Diff (- obtained, + expected)
-false
+true
    at munit.Assertions.failComparison(Assertions.scala:278)
    at apply @ munit.CatsEffectAssertions.$anonfun$assertIO$1(CatsEffectAssertions.scala:52)
    at apply @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$checkResponse$5(ClientRouteTestBattery.scala:190)
    at map @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$checkResponse$5(ClientRouteTestBattery.scala:190)
    at flatMap @ munit.CatsEffectAssertions.assertIO(CatsEffectAssertions.scala:52)
    at flatMap @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$checkResponse$5(ClientRouteTestBattery.scala:190)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at modify @ fs2.internal.Scope.close(Scope.scala:262)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at rethrow$extension @ fs2.Compiler$Target.$anonfun$compile$1(Compiler.scala:157)
==> X org.http4s.jetty.client.JettyClientSuite.JettyClient Execute GET /not-found  0.027s munit.ComparisonFailException: value is not true
=> Obtained
false
=> Diff (- obtained, + expected)
-false
+true
    at munit.Assertions.failComparison(Assertions.scala:278)
    at apply @ munit.CatsEffectAssertions.$anonfun$assertIO$1(CatsEffectAssertions.scala:52)
    at apply @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$checkResponse$5(ClientRouteTestBattery.scala:190)
    at map @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$checkResponse$5(ClientRouteTestBattery.scala:190)
    at flatMap @ munit.CatsEffectAssertions.assertIO(CatsEffectAssertions.scala:52)
    at flatMap @ org.http4s.client.testkit.ClientRouteTestBattery.$anonfun$checkResponse$5(ClientRouteTestBattery.scala:190)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at modify @ fs2.internal.Scope.close(Scope.scala:262)
    at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
    at rethrow$extension @ fs2.Compiler$Target.$anonfun$compile$1(Compiler.scala:157)

Screenshot 2024-11-13 at 3 55 06 pm

}

private[jetty] object ResponseListener {
Expand Down
8 changes: 3 additions & 5 deletions jetty-client/src/main/scala/org/http4s/jetty/client/StreamRequestContent.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import cats.effect._
import cats.effect.std._
import cats.syntax.all._
import fs2._
import org.eclipse.jetty.client.util.AsyncRequestContent
import org.eclipse.jetty.client.AsyncRequestContent
import org.eclipse.jetty.util.{Callback => JettyCallback}
import org.http4s.jetty.client.internal.loggingAsyncCallback
import org.log4s.getLogger
Expand All @@ -45,13 +45,11 @@ private[jetty] class StreamRequestContent[F[_]] private (
private val pipe: Pipe[F, Chunk[Byte], Unit] =
_.evalMap { c =>
write(c)
.ensure(new Exception("something terrible has happened"))(res => res)
.map(_ => ())
}

private def write(chunk: Chunk[Byte]): F[Boolean] =
private def write(chunk: Chunk[Byte]): F[Unit] =
s.acquire
.map(_ => super.offer(chunk.toByteBuffer, callback))
.map(_ => super.write(chunk.toByteBuffer, callback))

private val callback: JettyCallback = new JettyCallback {
override def succeeded(): Unit =
Expand Down
6 changes: 3 additions & 3 deletions jetty-server/src/main/scala/org/http4s/jetty/server/JettyBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ import cats.effect._
import cats.effect.kernel.Async
import cats.effect.std.Dispatcher
import cats.syntax.all._
import org.eclipse.jetty.ee8.servlet.FilterHolder
import org.eclipse.jetty.ee8.servlet.ServletContextHandler
import org.eclipse.jetty.ee8.servlet.ServletHolder
import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory
import org.eclipse.jetty.server.HttpConfiguration
import org.eclipse.jetty.server.HttpConnectionFactory
import org.eclipse.jetty.server.ServerConnector
import org.eclipse.jetty.server.handler.StatisticsHandler
import org.eclipse.jetty.server.{Server => JServer}
import org.eclipse.jetty.servlet.FilterHolder
import org.eclipse.jetty.servlet.ServletContextHandler
import org.eclipse.jetty.servlet.ServletHolder
import org.eclipse.jetty.util.ssl.SslContextFactory
import org.eclipse.jetty.util.thread.ThreadPool
import org.http4s.jetty.server.JettyBuilder._
Expand Down
4 changes: 2 additions & 2 deletions jetty-server/src/test/scala/org/http4s/jetty/server/JettyServerSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ import cats.effect.Temporal
import munit.CatsEffectSuite
import munit.catseffect.IOFixture
import org.eclipse.jetty.client.HttpClient
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.client.util.StringRequestContent
import org.eclipse.jetty.client.Request
import org.eclipse.jetty.client.StringRequestContent
import org.http4s.dsl.io._
import org.http4s.server.Server

Expand Down