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.

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

Already on GitHub? Sign in to your account

fix #4213: Content-Length when entity is unallowed #4214

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
# IntelliJ
/.idea

# VS Code
**/metals.sbt
**/.bloop
.metals
settings.json

.DS_Store
target

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,26 @@ private HttpMethods() {}

/**
* Create a custom method type.
* @deprecated The created method will compute the presence of Content-Length headers based on deprecated logic (before issue #4213).
*/
@Deprecated
public static HttpMethod custom(String value, boolean safe, boolean idempotent, akka.http.javadsl.model.RequestEntityAcceptance requestEntityAcceptance) {
//This cast is safe as implementation of RequestEntityAcceptance only exists in Scala
akka.http.scaladsl.model.RequestEntityAcceptance scalaRequestEntityAcceptance
= (akka.http.scaladsl.model.RequestEntityAcceptance) requestEntityAcceptance;
return akka.http.scaladsl.model.HttpMethod.custom(value, safe, idempotent, scalaRequestEntityAcceptance);
}

/**
* Create a custom method type.
*/
public static HttpMethod custom(String value, boolean safe, boolean idempotent, akka.http.javadsl.model.RequestEntityAcceptance requestEntityAcceptance, boolean contentLengthAllowed) {
bursauxa marked this conversation as resolved.
Show resolved Hide resolved
//This cast is safe as implementation of RequestEntityAcceptance only exists in Scala
akka.http.scaladsl.model.RequestEntityAcceptance scalaRequestEntityAcceptance
= (akka.http.scaladsl.model.RequestEntityAcceptance) requestEntityAcceptance;
return akka.http.scaladsl.model.HttpMethod.custom(value, safe, idempotent, scalaRequestEntityAcceptance, contentLengthAllowed);
}

/**
* Looks up a predefined HTTP method with the given name.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# HttpMethod is marked `private[http]` so can be excluded
# All changes are on auto-generated case-class methods, caused by adding a parameter to the case class
ProblemFilters.exclude[IncompatibleSignatureProblem]("akka.http.scaladsl.model.HttpMethod.unapply")
ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.HttpMethod.apply")
ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.HttpMethod.copy")
ProblemFilters.exclude[DirectMissingMethodProblem]("akka.http.scaladsl.model.HttpMethod.this")
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ private[http] class HttpResponseRendererFactory(
}

def renderContentLengthHeader(contentLength: Long) =
if (status.allowsEntity) r ~~ ContentLengthBytes ~~ contentLength ~~ CrLf else r
if (ctx.requestMethod.contentLengthAllowed(status)) r ~~ ContentLengthBytes ~~ contentLength ~~ CrLf else r

def headersAndEntity(entityBytes: => Source[ByteString, Any]): StrictOrStreamed =
if (noEntity) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import java.util.Locale
import akka.http.impl.util._
import akka.http.javadsl.{ model => jm }
import akka.http.scaladsl.model.RequestEntityAcceptance._
import akka.util.ConstantFun

sealed trait RequestEntityAcceptance extends jm.RequestEntityAcceptance {
def isEntityAccepted: Boolean
Expand All @@ -30,43 +31,77 @@ object RequestEntityAcceptance {
* @param isSafe true if the resource should not be altered on the server
* @param isIdempotent true if requests can be safely (& automatically) repeated
* @param requestEntityAcceptance Expected if meaning of request entities is properly defined
* @param contentLengthAllowed Function defining whether the method-statuscode combination should output the Content-Length header.
*/
final case class HttpMethod private[http] (
override val value: String,
isSafe: Boolean,
isIdempotent: Boolean,
requestEntityAcceptance: RequestEntityAcceptance) extends jm.HttpMethod with SingletonValueRenderable {
override val value: String,
isSafe: Boolean,
isIdempotent: Boolean,
requestEntityAcceptance: RequestEntityAcceptance,
val contentLengthAllowed: StatusCode => Boolean) extends jm.HttpMethod with SingletonValueRenderable {
override def isEntityAccepted: Boolean = requestEntityAcceptance.isEntityAccepted
override def toString: String = s"HttpMethod($value)"
}

object HttpMethod {

// the allowsEntity condition was used to determine what responses provided the Content-Length header, before #4213 was fixed
private def oldContentLengthCondition(status: StatusCode) = status.allowsEntity

/**
* Create a custom method type.
* @deprecated The created method will compute the presence of Content-Length headers based on deprecated logic (before issue #4213).
*/
@Deprecated
def custom(name: String, safe: Boolean, idempotent: Boolean, requestEntityAcceptance: RequestEntityAcceptance): HttpMethod = {
require(name.nonEmpty, "value must be non-empty")
require(!safe || idempotent, "An HTTP method cannot be safe without being idempotent")
apply(name, safe, idempotent, requestEntityAcceptance)
apply(name, safe, idempotent, requestEntityAcceptance, oldContentLengthCondition)
}

/**
* Create a custom method type.
*/
def custom(name: String, safe: Boolean, idempotent: Boolean, requestEntityAcceptance: RequestEntityAcceptance, contentLengthAllowed: Boolean): HttpMethod = {
require(name.nonEmpty, "value must be non-empty")
require(!safe || idempotent, "An HTTP method cannot be safe without being idempotent")
// use constant functions so custom HttpMethod instances are equal (case class equality) when built with the same parameters
apply(name, safe, idempotent, requestEntityAcceptance, if (contentLengthAllowed) ConstantFun.anyToTrue else ConstantFun.anyToFalse)
}

/**
* Creates a custom method by name and assumes properties conservatively to be
* safe = false, idempotent = false and requestEntityAcceptance = Expected.
* safe = false, idempotent = false, requestEntityAcceptance = Expected and contentLengthAllowed always true.
*/
def custom(name: String): HttpMethod = custom(name, safe = false, idempotent = false, requestEntityAcceptance = Expected)
def custom(name: String): HttpMethod = custom(name, safe = false, idempotent = false, requestEntityAcceptance = Expected, contentLengthAllowed = true)
}

object HttpMethods extends ObjectRegistry[String, HttpMethod] {
private def register(method: HttpMethod): HttpMethod = register(method.value, method)

// define requirements for content-length according to https://httpwg.org/specs/rfc9110.html#field.content-length
// for CONNECT it is explicitly not allowed in the 2xx (Successful) range
private def contentLengthAllowedForConnect(forStatus: StatusCode): Boolean = forStatus.intValue < 200 || forStatus.intValue >= 300
// for HEAD it is technically allowed, but must match the content-length of hypothetical GET request, so can not be anticipated
private def contentLengthAllowedForHead(forStatus: StatusCode): Boolean = false
// for other methods there are common rules:
// - for 1xx (Informational) or 204 (No Content) it is explicitly not allowed
// - for 304 (Not Modified) it must match the content-length of hypothetical 200-accepted request, so can not be anticipated
private def contentLengthAllowedCommon(forStatus: StatusCode): Boolean = {
val code = forStatus.intValue
!(code < 200 || code == 204 || code == 304)
}

// format: OFF
val CONNECT = register(HttpMethod("CONNECT", isSafe = false, isIdempotent = false, requestEntityAcceptance = Disallowed))
val DELETE = register(HttpMethod("DELETE" , isSafe = false, isIdempotent = true , requestEntityAcceptance = Tolerated))
val GET = register(HttpMethod("GET" , isSafe = true , isIdempotent = true , requestEntityAcceptance = Tolerated))
val HEAD = register(HttpMethod("HEAD" , isSafe = true , isIdempotent = true , requestEntityAcceptance = Disallowed))
val OPTIONS = register(HttpMethod("OPTIONS", isSafe = true , isIdempotent = true , requestEntityAcceptance = Expected))
val PATCH = register(HttpMethod("PATCH" , isSafe = false, isIdempotent = false, requestEntityAcceptance = Expected))
val POST = register(HttpMethod("POST" , isSafe = false, isIdempotent = false, requestEntityAcceptance = Expected))
val PUT = register(HttpMethod("PUT" , isSafe = false, isIdempotent = true , requestEntityAcceptance = Expected))
val TRACE = register(HttpMethod("TRACE" , isSafe = true , isIdempotent = true , requestEntityAcceptance = Disallowed))
val CONNECT = register(HttpMethod("CONNECT", isSafe = false, isIdempotent = false, requestEntityAcceptance = Disallowed, contentLengthAllowed = contentLengthAllowedForConnect))
val DELETE = register(HttpMethod("DELETE" , isSafe = false, isIdempotent = true , requestEntityAcceptance = Tolerated, contentLengthAllowed = contentLengthAllowedCommon))
val GET = register(HttpMethod("GET" , isSafe = true , isIdempotent = true , requestEntityAcceptance = Tolerated, contentLengthAllowed = contentLengthAllowedCommon))
val HEAD = register(HttpMethod("HEAD" , isSafe = true , isIdempotent = true , requestEntityAcceptance = Disallowed, contentLengthAllowed = contentLengthAllowedForHead))
val OPTIONS = register(HttpMethod("OPTIONS", isSafe = true , isIdempotent = true , requestEntityAcceptance = Expected, contentLengthAllowed = contentLengthAllowedCommon))
val PATCH = register(HttpMethod("PATCH" , isSafe = false, isIdempotent = false, requestEntityAcceptance = Expected, contentLengthAllowed = contentLengthAllowedCommon))
val POST = register(HttpMethod("POST" , isSafe = false, isIdempotent = false, requestEntityAcceptance = Expected, contentLengthAllowed = contentLengthAllowedCommon))
val PUT = register(HttpMethod("PUT" , isSafe = false, isIdempotent = true , requestEntityAcceptance = Expected, contentLengthAllowed = contentLengthAllowedCommon))
val TRACE = register(HttpMethod("TRACE" , isSafe = true , isIdempotent = true , requestEntityAcceptance = Disallowed, contentLengthAllowed = contentLengthAllowedCommon))
// format: ON

override def getForKeyCaseInsensitive(key: String)(implicit conv: String <:< String): Option[HttpMethod] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ class HostConnectionPoolSpec extends AkkaSpecWithMaterializer(

conn1.pushResponse(HttpResponse(entity = HttpEntity.Default(ContentTypes.`application/octet-stream`, 100, Source.empty)))
val res = expectResponse()
res.entity.contentLengthOption.get shouldEqual 100
res.entity.contentLengthOption.get shouldEqual 0
bursauxa marked this conversation as resolved.
Show resolved Hide resolved

// HEAD requests do not require to consume entity

Expand All @@ -217,7 +217,7 @@ class HostConnectionPoolSpec extends AkkaSpecWithMaterializer(

conn1.pushResponse(HttpResponse(entity = HttpEntity.Default(ContentTypes.`application/octet-stream`, 100, Source.empty)))
val res = expectResponse()
res.entity.contentLengthOption.get shouldEqual 100
res.entity.contentLengthOption.get shouldEqual 0

// HEAD requests do not require consumption of entity but users might do anyway
res.entity.discardBytes()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ abstract class RequestParserSpec(mode: String, newLine: String) extends AnyFreeS
implicit val system: ActorSystem = ActorSystem(getClass.getSimpleName, testConf)
import system.dispatcher

val BOLT = HttpMethod.custom("BOLT", safe = false, idempotent = true, requestEntityAcceptance = Expected)
val BOLT = HttpMethod.custom("BOLT", safe = false, idempotent = true, requestEntityAcceptance = Expected, contentLengthAllowed = true)

s"The request parsing logic should (mode: $mode)" - {
"properly parse a request" - {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,32 @@ class ResponseRendererSpec extends AnyFreeSpec with Matchers with BeforeAndAfter
}
}

"status 304 and a few headers" in new TestSetup() {
"status 204 and a few headers (does not add content-length)" in new TestSetup() {
HttpResponse(204, List(RawHeader("X-Fancy", "of course"), Age(0))) should renderTo {
"""HTTP/1.1 204 No Content
|X-Fancy: of course
|Age: 0
|Server: akka-http/1.0.0
|Date: Thu, 25 Aug 2011 09:10:29 GMT
|
|"""
}
}

"status 205 and a few headers (adds content-length)" in new TestSetup() {
HttpResponse(205, List(RawHeader("X-Fancy", "of course"), Age(0))) should renderTo {
"""HTTP/1.1 205 Reset Content
|X-Fancy: of course
|Age: 0
|Server: akka-http/1.0.0
|Date: Thu, 25 Aug 2011 09:10:29 GMT
|Content-Length: 0
|
|"""
}
}

"status 304 and a few headers (does not add content-length)" in new TestSetup() {
HttpResponse(304, List(RawHeader("X-Fancy", "of course"), Age(0))) should renderTo {
"""HTTP/1.1 304 Not Modified
|X-Fancy: of course
Expand All @@ -69,6 +94,7 @@ class ResponseRendererSpec extends AnyFreeSpec with Matchers with BeforeAndAfter
|"""
}
}

"a custom status code and no headers" in new TestSetup() {
HttpResponse(ServerOnTheMove) should renderTo {
"""HTTP/1.1 330 Server on the move
Expand Down Expand Up @@ -97,6 +123,18 @@ class ResponseRendererSpec extends AnyFreeSpec with Matchers with BeforeAndAfter
override def currentTimeMillis() = initial + extraMillis
}

"no Content-Length on CONNECT method" in new TestSetup() {
ResponseRenderingContext(
requestMethod = HttpMethods.CONNECT,
response = HttpResponse(headers = List(Age(30), Connection("Keep-Alive")))) should renderTo(
"""HTTP/1.1 200 OK
|Age: 30
|Server: akka-http/1.0.0
|Date: Thu, 25 Aug 2011 09:10:29 GMT
|
|""", close = false)
}

"to a transparent HEAD request (Strict response entity)" in new TestSetup() {
ResponseRenderingContext(
requestMethod = HttpMethods.HEAD,
Expand All @@ -108,7 +146,6 @@ class ResponseRendererSpec extends AnyFreeSpec with Matchers with BeforeAndAfter
|Server: akka-http/1.0.0
|Date: Thu, 25 Aug 2011 09:10:29 GMT
|Content-Type: text/plain; charset=UTF-8
|Content-Length: 23
|
|""", close = false)
}
Expand Down Expand Up @@ -159,7 +196,6 @@ class ResponseRendererSpec extends AnyFreeSpec with Matchers with BeforeAndAfter
|Server: akka-http/1.0.0
|Date: Thu, 25 Aug 2011 09:10:29 GMT
|Content-Type: text/plain; charset=UTF-8
|Content-Length: 100
|
|""", close = false)
}
Expand Down Expand Up @@ -661,7 +697,7 @@ class ResponseRendererSpec extends AnyFreeSpec with Matchers with BeforeAndAfter
|Server: akka-http/1.0.0
|Date: Thu, 25 Aug 2011 09:10:29 GMT
|${renCH.fold("")(_.toString + "\n")}Content-Type: text/plain; charset=UTF-8
|${if (resCD) "" else "Content-Length: 6\n"}
|${if (headReq || resCD) "" else "Content-Length: 6\n"}
|${if (headReq) "" else "ENTITY"}""", close))
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,6 @@ class HttpServerSpec extends AkkaSpec(
|Server: akka-http/test
|Date: XXXX
|Content-Type: text/plain; charset=UTF-8
|Content-Length: 4
|
|""")
}
Expand All @@ -524,7 +523,6 @@ class HttpServerSpec extends AkkaSpec(
|Server: akka-http/test
|Date: XXXX
|Content-Type: text/plain; charset=UTF-8
|Content-Length: 4
|
|""")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class CustomHttpMethodSpec extends AkkaSpec with ScalaFutures

// define custom method type:
val BOLT = HttpMethod.custom("BOLT", safe = false,
idempotent = true, requestEntityAcceptance = Expected)
idempotent = true, requestEntityAcceptance = Expected, contentLengthAllowed = true)

// add custom method to parser settings:
val parserSettings = ParserSettings.forServer(system).withCustomMethods(BOLT)
Expand Down