diff --git a/http-core/src/main/mima-filters/1.4.x.backwards.excludes/bad-header-http2-response.backwards.excludes b/http-core/src/main/mima-filters/1.4.x.backwards.excludes/bad-header-http2-response.backwards.excludes new file mode 100644 index 000000000..fab687cc9 --- /dev/null +++ b/http-core/src/main/mima-filters/1.4.x.backwards.excludes/bad-header-http2-response.backwards.excludes @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Invalid HTTP/2 request headers return 400 Bad Request instead of GOAWAY +ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.copy") +ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.this") +ProblemFilters.exclude[MissingTypesProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent$ParsedHeadersFrame$") +ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.apply") +ProblemFilters.exclude[IncompatibleSignatureProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.unapply") diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameEvent.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameEvent.scala index 6c93916b4..9888ee0ea 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameEvent.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameEvent.scala @@ -18,6 +18,7 @@ import pekko.annotation.InternalApi import pekko.http.impl.engine.http2.Http2Protocol.ErrorCode import pekko.http.impl.engine.http2.Http2Protocol.FrameType import pekko.http.impl.engine.http2.Http2Protocol.SettingIdentifier +import pekko.http.scaladsl.model.ErrorInfo import pekko.util.ByteString import scala.collection.immutable @@ -112,11 +113,14 @@ private[http] object FrameEvent { /** * Convenience (logical) representation of a parsed HEADERS frame with zero, one or * many CONTINUATIONS Frames into a single, decompressed object. + * + * @param headerParseErrorDetails Only used server side, passes header errors from decompression into error response logic */ final case class ParsedHeadersFrame( streamId: Int, endStream: Boolean, keyValuePairs: Seq[(String, AnyRef)], - priorityInfo: Option[PriorityFrame]) extends StreamFrameEvent + priorityInfo: Option[PriorityFrame], + headerParseErrorDetails: Option[ErrorInfo]) extends StreamFrameEvent } diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameLogger.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameLogger.scala index 418cb3ac5..00caa0485 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameLogger.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameLogger.scala @@ -79,7 +79,7 @@ private[http2] object FrameLogger { case GoAwayFrame(lastStreamId, errorCode, debug) => LogEntry(0, "GOAY", s"lastStreamId = $lastStreamId, errorCode = $errorCode, debug = ${debug.utf8String}") - case ParsedHeadersFrame(streamId, endStream, kvPairs, prio) => + case ParsedHeadersFrame(streamId, endStream, kvPairs, prio, _) => val prioInfo = if (prio.isDefined) display(entryForFrame(prio.get)) + " " else "" val kvInfo = kvPairs.map { case (key, value) => s"$key -> $value" diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala index 3b14c0b86..576faa00d 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala @@ -274,7 +274,7 @@ private[http] object Http2Blueprint { * * To make use of parallelism requests and responses need to be associated (other than by ordering), suggestion * is to add a special (virtual) header containing the streamId (or any other kind of token) is added to the HttRequest - * that must be reproduced in an HttpResponse. This can be done automatically for the `bind`` API but for + * that must be reproduced in an HttpResponse. This can be done automatically for the `bind` API but for * `bindFlow` the user needs to take of this manually. */ def httpLayer(settings: ServerSettings, log: LoggingAdapter, dateHeaderRendering: DateHeaderRendering) @@ -284,12 +284,14 @@ private[http] object Http2Blueprint { // HttpHeaderParser is not thread safe and should not be called concurrently, // the internal trie, however, has built-in protection and will do copy-on-write val masterHttpHeaderParser = HttpHeaderParser(parserSettings, log) - BidiFlow.fromFlows( - Flow[HttpResponse].map(new ResponseRendering(settings, log, dateHeaderRendering)), - Flow[Http2SubStream].via(StreamUtils.statefulAttrsMap { attrs => - val headerParser = masterHttpHeaderParser.createShallowCopy() - RequestParsing.parseRequest(headerParser, settings, attrs) - })) + RequestErrorFlow().atop( + BidiFlow.fromFlows( + Flow[HttpResponse].map(new ResponseRendering(settings, log, dateHeaderRendering)), + Flow[Http2SubStream].via(StreamUtils.statefulAttrsMap { attrs => + val headerParser = masterHttpHeaderParser.createShallowCopy() + RequestParsing.parseRequest(headerParser, settings, attrs) + })) + ) } /** diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2StreamHandling.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2StreamHandling.scala index a09f4ef64..e17ca4dc3 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2StreamHandling.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2StreamHandling.scala @@ -299,7 +299,7 @@ private[http2] trait Http2StreamHandling extends GraphStageLogic with LogHelper nextStateStream: IncomingStreamBuffer => StreamState, correlationAttributes: Map[AttributeKey[_], _] = Map.empty): StreamState = event match { - case frame @ ParsedHeadersFrame(streamId, endStream, _, _) => + case frame @ ParsedHeadersFrame(streamId, endStream, _, _, _) => if (endStream) { dispatchSubstream(frame, Left(ByteString.empty), correlationAttributes) nextStateEmpty @@ -833,7 +833,7 @@ private[http2] trait Http2StreamHandling extends GraphStageLogic with LogHelper "Found both an attribute with trailing headers, and headers in the `LastChunk`. This is not supported.") trailer = OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true, HttpMessageRendering.renderHeaders(headers, log, isServer, shouldRenderAutoHeaders = false, - dateHeaderRendering = DateHeaderRendering.Unavailable), None)) + dateHeaderRendering = DateHeaderRendering.Unavailable), None, None)) } maybePull() diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRendering.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRendering.scala index e434b857e..d2dc3028e 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRendering.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRendering.scala @@ -94,11 +94,11 @@ private[http2] sealed abstract class MessageRendering[R <: HttpMessage] extends shouldRenderAutoHeaders = true, dateHeaderRendering) val streamId = nextStreamId(r) - val headersFrame = ParsedHeadersFrame(streamId, endStream = r.entity.isKnownEmpty, headerPairs.result(), None) + val headersFrame = ParsedHeadersFrame(streamId, endStream = r.entity.isKnownEmpty, headerPairs.result(), None, None) val trailingHeadersFrame = r.attribute(AttributeKeys.trailer) match { case Some(trailer) if trailer.headers.nonEmpty => - OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true, trailer.headers, None)) + OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true, trailer.headers, None, None)) case _ => OptionVal.None } diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestErrorFlow.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestErrorFlow.scala new file mode 100644 index 000000000..c37944992 --- /dev/null +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestErrorFlow.scala @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * license agreements; and to You under the Apache License, version 2.0: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * This file is part of the Apache Pekko project, which was derived from Akka. + */ + +/* + * Copyright (C) 2009-2023 Lightbend Inc. + */ + +package org.apache.pekko.http.impl.engine.http2 + +import org.apache.pekko +import pekko.NotUsed +import pekko.annotation.InternalApi +import pekko.http.impl.engine.http2.RequestParsing.ParseRequestResult +import pekko.http.scaladsl.model.{ HttpRequest, HttpResponse, StatusCodes } +import pekko.stream.{ Attributes, BidiShape, Inlet, Outlet } +import pekko.stream.scaladsl.BidiFlow +import pekko.stream.stage.{ GraphStage, GraphStageLogic, InHandler, OutHandler } + +/** + * INTERNAL API + */ +@InternalApi +private[http2] object RequestErrorFlow { + + private val _bidiFlow = BidiFlow.fromGraph(new RequestErrorFlow) + def apply(): BidiFlow[HttpResponse, HttpResponse, ParseRequestResult, HttpRequest, NotUsed] = _bidiFlow + +} + +/** + * INTERNAL API + */ +@InternalApi +private[http2] final class RequestErrorFlow + extends GraphStage[BidiShape[HttpResponse, HttpResponse, ParseRequestResult, HttpRequest]] { + + val requestIn = Inlet[ParseRequestResult]("requestIn") + val requestOut = Outlet[HttpRequest]("requestOut") + val responseIn = Inlet[HttpResponse]("responseIn") + val responseOut = Outlet[HttpResponse]("responseOut") + + override val shape: BidiShape[HttpResponse, HttpResponse, ParseRequestResult, HttpRequest] = + BidiShape(responseIn, responseOut, requestIn, requestOut) + + override def createLogic(inheritedAttributes: Attributes): GraphStageLogic = new GraphStageLogic(shape) { + setHandlers(requestIn, requestOut, + new InHandler with OutHandler { + override def onPush(): Unit = { + grab(requestIn) match { + case RequestParsing.OkRequest(request) => push(requestOut, request) + case notOk: RequestParsing.BadRequest => + emit(responseOut, + HttpResponse(StatusCodes.BadRequest, entity = notOk.info.summary).addAttribute(Http2.streamId, + notOk.streamId)) + pull(requestIn) + } + } + + override def onPull(): Unit = pull(requestIn) + }) + setHandlers(responseIn, responseOut, + new InHandler with OutHandler { + override def onPush(): Unit = push(responseOut, grab(responseIn)) + override def onPull(): Unit = if (!hasBeenPulled(responseIn)) pull(responseIn) + }) + + } +} diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala index e5f557ee7..964727440 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala @@ -16,7 +16,6 @@ package org.apache.pekko.http.impl.engine.http2 import javax.net.ssl.SSLSession import org.apache.pekko import pekko.annotation.InternalApi -import pekko.http.impl.engine.http2.Http2Compliance.Http2ProtocolException import pekko.http.impl.engine.parsing.HttpHeaderParser import pekko.http.impl.engine.server.HttpAttributes import pekko.http.scaladsl.model @@ -29,6 +28,7 @@ import pekko.util.OptionVal import scala.annotation.tailrec import scala.collection.immutable.VectorBuilder +import scala.util.control.NoStackTrace /** * INTERNAL API @@ -36,8 +36,12 @@ import scala.collection.immutable.VectorBuilder @InternalApi private[http2] object RequestParsing { + sealed trait ParseRequestResult + final case class OkRequest(request: HttpRequest) extends ParseRequestResult + final case class BadRequest(info: ErrorInfo, streamId: Int) extends ParseRequestResult + def parseRequest(httpHeaderParser: HttpHeaderParser, serverSettings: ServerSettings, streamAttributes: Attributes) - : Http2SubStream => HttpRequest = { + : Http2SubStream => ParseRequestResult = { val remoteAddressAttribute: Option[RemoteAddress] = if (serverSettings.remoteAddressAttribute) { @@ -151,19 +155,19 @@ private[http2] object RequestParsing { rec(incomingHeaders, offset + 1, method, scheme, authority, pathAndRawQuery, OptionVal.Some(ContentType.get(value)), contentLength, cookies, true, headers) else - malformedRequest("HTTP message must not contain more than one content-type header") + parseError("HTTP message must not contain more than one content-type header", "content-type") case ":status" => - malformedRequest("Pseudo-header ':status' is for responses only; it cannot appear in a request") + parseError("Pseudo-header ':status' is for responses only; it cannot appear in a request", ":status") case "content-length" => if (contentLength == -1) { val contentLengthValue = ContentLength.get(value).toLong if (contentLengthValue < 0) - malformedRequest("HTTP message must not contain a negative content-length header") + parseError("HTTP message must not contain a negative content-length header", "content-length") rec(incomingHeaders, offset + 1, method, scheme, authority, pathAndRawQuery, contentType, contentLengthValue, cookies, true, headers) - } else malformedRequest("HTTP message must not contain more than one content-length header") + } else parseError("HTTP message must not contain more than one content-length header", "content-length") case "cookie" => // Compress cookie headers as described here https://tools.ietf.org/html/rfc7540#section-8.1.2.5 @@ -182,11 +186,21 @@ private[http2] object RequestParsing { } } - val incomingHeaders = subStream.initialHeaders.keyValuePairs.toIndexedSeq - if (incomingHeaders.size > serverSettings.parserSettings.maxHeaderCount) - malformedRequest( - s"HTTP message contains more than the configured limit of ${serverSettings.parserSettings.maxHeaderCount} headers") - else rec(incomingHeaders, 0) + try { + subStream.initialHeaders.headerParseErrorDetails match { + case Some(details) => + // header errors already found in decompression + BadRequest(details, subStream.streamId) + case None => + val incomingHeaders = subStream.initialHeaders.keyValuePairs.toIndexedSeq + if (incomingHeaders.size > serverSettings.parserSettings.maxHeaderCount) + parseError( + s"HTTP message contains more than the configured limit of ${serverSettings.parserSettings.maxHeaderCount} headers") + else OkRequest(rec(incomingHeaders, 0)) + } + } catch { + case bre: ParsingException => BadRequest(bre.info, subStream.streamId) + } } } @@ -201,25 +215,34 @@ private[http2] object RequestParsing { } private[http2] def checkRequiredPseudoHeader(name: String, value: AnyRef): Unit = - if (value eq null) malformedRequest(s"Mandatory pseudo-header '$name' missing") + if (value eq null) protocolError(s"Mandatory pseudo-header '$name' missing") private[http2] def checkUniquePseudoHeader(name: String, value: AnyRef): Unit = - if (value ne null) malformedRequest(s"Pseudo-header '$name' must not occur more than once") + if (value ne null) protocolError(s"Pseudo-header '$name' must not occur more than once") private[http2] def checkNoRegularHeadersBeforePseudoHeader(name: String, seenRegularHeader: Boolean): Unit = - if (seenRegularHeader) malformedRequest(s"Pseudo-header field '$name' must not appear after a regular header") - private[http2] def malformedRequest(msg: String): Nothing = - throw new Http2ProtocolException(s"Malformed request: $msg") + if (seenRegularHeader) parseError(s"Pseudo-header field '$name' must not appear after a regular header", name) private[http2] def validateHeader(httpHeader: HttpHeader) = httpHeader.lowercaseName match { case "connection" => // https://tools.ietf.org/html/rfc7540#section-8.1.2.2 - malformedRequest("Header 'Connection' must not be used with HTTP/2") + parseError("Header 'Connection' must not be used with HTTP/2", "Connection") case "transfer-encoding" => // https://tools.ietf.org/html/rfc7540#section-8.1.2.2 - malformedRequest("Header 'Transfer-Encoding' must not be used with HTTP/2") + parseError("Header 'Transfer-Encoding' must not be used with HTTP/2", "Transfer-encoding") case "te" => // https://tools.ietf.org/html/rfc7540#section-8.1.2.2 if (httpHeader.value.compareToIgnoreCase("trailers") != 0) - malformedRequest(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}") + parseError(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}", "TE") case _ => // ok } + // parse errors lead to BadRequest response while Protocol + private[http2] def protocolError(summary: String): Nothing = + throw new Http2Compliance.Http2ProtocolException(s"Malformed request: $summary") + + private[http2] def parseError(summary: String, headerName: String): Nothing = + throw new ParsingException(ErrorInfo(s"Malformed request: $summary").withErrorHeaderName(headerName)) + with NoStackTrace + + private def parseError(summary: String): Nothing = + throw new ParsingException(ErrorInfo(s"Malformed request: $summary")) with NoStackTrace + } diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/client/ResponseParsing.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/client/ResponseParsing.scala index 80b0545e1..78868b2c6 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/client/ResponseParsing.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/client/ResponseParsing.scala @@ -16,6 +16,7 @@ package client import org.apache.pekko import pekko.annotation.InternalApi +import pekko.http.impl.engine.http2.Http2Compliance.Http2ProtocolException import pekko.http.impl.engine.http2.RequestParsing._ import pekko.http.impl.engine.parsing.HttpHeaderParser import pekko.http.impl.engine.server.HttpAttributes @@ -85,26 +86,26 @@ private[http2] object ResponseParsing { rec(remainingHeaders.tail, status, OptionVal.Some(contentTypeValue), contentLength, seenRegularHeader, headers) else - malformedRequest("HTTP message must not contain more than one content-type header") + malformedResponse("HTTP message must not contain more than one content-type header") case ("content-type", ct: String) => if (contentType.isEmpty) { val contentTypeValue = - ContentType.parse(ct).getOrElse(malformedRequest(s"Invalid content-type: '$ct'")) + ContentType.parse(ct).getOrElse(malformedResponse(s"Invalid content-type: '$ct'")) rec(remainingHeaders.tail, status, OptionVal.Some(contentTypeValue), contentLength, seenRegularHeader, headers) - } else malformedRequest("HTTP message must not contain more than one content-type header") + } else malformedResponse("HTTP message must not contain more than one content-type header") case ("content-length", length: String) => if (contentLength == -1) { val contentLengthValue = length.toLong if (contentLengthValue < 0) - malformedRequest("HTTP message must not contain a negative content-length header") + malformedResponse("HTTP message must not contain a negative content-length header") rec(remainingHeaders.tail, status, contentType, contentLengthValue, seenRegularHeader, headers) - } else malformedRequest("HTTP message must not contain more than one content-length header") + } else malformedResponse("HTTP message must not contain more than one content-length header") case (name, _) if name.startsWith(':') => - malformedRequest(s"Unexpected pseudo-header '$name' in response") + malformedResponse(s"Unexpected pseudo-header '$name' in response") case (_, httpHeader: HttpHeader) => rec(remainingHeaders.tail, status, contentType, contentLength, seenRegularHeader = true, @@ -119,4 +120,7 @@ private[http2] object ResponseParsing { rec(subStream.initialHeaders.keyValuePairs) } + + private def malformedResponse(msg: String): Nothing = + throw new Http2ProtocolException(s"Malformed response: $msg") } diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala index 2738a8f09..60fec77d8 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala @@ -48,7 +48,7 @@ private[http2] object HeaderCompression extends GraphStage[FlowShape[FrameEvent, case ack @ SettingsAckFrame(s) => applySettings(s) push(eventsOut, ack) - case ParsedHeadersFrame(streamId, endStream, kvs, prioInfo) => + case ParsedHeadersFrame(streamId, endStream, kvs, prioInfo, _) => // When ending the stream without any payload, use a DATA frame rather than // a HEADERS frame to work around https://github.com/golang/go/issues/47851. if (endStream && kvs.isEmpty) push(eventsOut, DataFrame(streamId, endStream, ByteString.empty)) diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala index 6ed39670a..4dfab40bb 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala @@ -21,6 +21,7 @@ import pekko.http.impl.engine.http2.Http2Protocol.ErrorCode import pekko.http.impl.engine.http2.RequestParsing.parseHeaderPair import pekko.http.impl.engine.http2._ import pekko.http.impl.engine.parsing.HttpHeaderParser +import pekko.http.scaladsl.model.ParsingException import pekko.http.scaladsl.settings.ParserSettings import pekko.http.shaded.com.twitter.hpack.HeaderListener import pekko.stream._ @@ -97,9 +98,12 @@ private[http2] final class HeaderDecompression(masterHeaderParser: HttpHeaderPar decoder.decode(stream, Receiver) // only compact ByteString supports InputStream with mark/reset decoder.endHeaderBlock() // TODO: do we have to check the result here? - push(eventsOut, ParsedHeadersFrame(streamId, endStream, headers.result(), prioInfo)) + push(eventsOut, ParsedHeadersFrame(streamId, endStream, headers.result(), prioInfo, None)) } catch { - case ex: IOException => + case ex: ParsingException => + // push details further and let RequestErrorFlow handle responding with bad request + push(eventsOut, ParsedHeadersFrame(streamId, endStream, Seq.empty, prioInfo, Some(ex.info))) + case _: IOException => // this is signalled by the decoder when it failed, we want to react to this by rendering a GOAWAY frame fail(eventsOut, new Http2Compliance.Http2ProtocolException(ErrorCode.COMPRESSION_ERROR, "Decompression failed.")) diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala index ae135cceb..7b2066065 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala @@ -15,7 +15,8 @@ package org.apache.pekko.http.impl.engine.http2.hpack import org.apache.pekko import pekko.annotation.InternalApi -import pekko.http.impl.engine.http2.RequestParsing.malformedRequest +import pekko.http.impl.engine.http2.RequestParsing +import pekko.http.impl.engine.http2.RequestParsing.parseError import pekko.http.scaladsl.model import pekko.http.scaladsl.model.HttpHeader.ParsingResult import model.{ HttpHeader, HttpMethod, HttpMethods, IllegalUriException, ParsingException, StatusCode, Uri } @@ -36,7 +37,7 @@ private[pekko] object Http2HeaderParsing { override def parse(name: String, value: String, parserSettings: ParserSettings): HttpMethod = HttpMethods.getForKey(value) .orElse(parserSettings.customMethods(value)) - .getOrElse(malformedRequest(s"Unknown HTTP method: '$value'")) + .getOrElse(RequestParsing.parseError(s"Unknown HTTP method: '$value'", ":method")) } object PathAndQuery extends HeaderParser[(Uri.Path, Option[String])](":path") { override def parse(name: String, value: String, parserSettings: ParserSettings): (Uri.Path, Option[String]) = @@ -60,7 +61,11 @@ private[pekko] object Http2HeaderParsing { } object ContentType extends HeaderParser[model.ContentType]("content-type") { override def parse(name: String, value: String, parserSettings: ParserSettings): model.ContentType = - model.ContentType.parse(value).getOrElse(malformedRequest(s"Invalid content-type: '$value'")) + model.ContentType.parse(value) match { + case Right(tpe) => tpe + case Left(_) => + parseError(s"Invalid content-type: '$value'", "content-type") + } } object ContentLength extends Verbatim("content-length") object Cookie extends Verbatim("cookie") diff --git a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala index 75c8d9f5f..60aa2edd3 100644 --- a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala +++ b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala @@ -30,6 +30,7 @@ import pekko.http.scaladsl.model.{ HttpHeader, HttpMethod, HttpMethods, + HttpProtocols, HttpRequest, HttpResponse, RequestResponseAssociation, @@ -133,6 +134,17 @@ class Http2ClientServerSpec extends PekkoSpecWithMaterializer( // expect idle timeout exception to propagate to user clientResponsesIn.expectError() shouldBe a[HttpIdleTimeoutException] } + + "return bad request response when header parsing fails" in new TestSetup { + val badRequest = HttpRequest( + // easiest valid way to cause parsing to fail - unknown method + method = HttpMethod.custom("UNKNOWN_TO_SERVER"), + uri = "http://www.example.com/test" + ) + sendClientRequest(badRequest) + val response = expectClientResponse() + response.status should be(StatusCodes.BadRequest) + } } case class ServerRequest(request: HttpRequest, promise: Promise[HttpResponse]) { diff --git a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerDemuxSpec.scala b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerDemuxSpec.scala index 8be03fa12..55f6adc39 100644 --- a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerDemuxSpec.scala +++ b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerDemuxSpec.scala @@ -54,7 +54,7 @@ class Http2ServerDemuxSpec extends PekkoSpecWithMaterializer(""" // The request is taken from the HTTP/1.1 request that had the Upgrade // header and is passed to the handler code 'directly', bypassing the demux stage, // so the first thing the demux stage sees of this request is the response: - val response = ParsedHeadersFrame(streamId = 1, endStream = true, Seq((":status", "200")), None) + val response = ParsedHeadersFrame(streamId = 1, endStream = true, Seq((":status", "200")), None, None) substreamProducer.sendNext(new Http2SubStream( response, OptionVal.None, diff --git a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala index a844bd762..e1ca5a235 100644 --- a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala +++ b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala @@ -21,6 +21,8 @@ import FrameEvent._ import org.apache.pekko import pekko.http.impl.engine.http2.hpack.HeaderDecompression import pekko.http.impl.engine.parsing.HttpHeaderParser +import pekko.http.impl.engine.http2.Http2Compliance.Http2ProtocolException +import pekko.http.impl.engine.http2.RequestParsing.ParseRequestResult import pekko.http.impl.engine.server.HttpAttributes import pekko.http.impl.util.PekkoSpecWithMaterializer import pekko.http.scaladsl.model._ @@ -31,6 +33,7 @@ import pekko.stream.scaladsl.{ Sink, Source } import pekko.util.{ ByteString, OptionVal } import org.scalatest.{ Inside, Inspectors } +import org.scalatest.exceptions.TestFailedException class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Inspectors { "RequestParsing" should { @@ -38,10 +41,10 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp /** Helper to test parsing */ def parse( keyValuePairs: Seq[(String, String)], - data: Source[ByteString, Any] = Source.empty, - attributes: Attributes = Attributes(), - uriParsingMode: Uri.ParsingMode = Uri.ParsingMode.Relaxed, - settings: ServerSettings = ServerSettings(system)): HttpRequest = { + data: Source[ByteString, Any], + attributes: Attributes, + uriParsingMode: Uri.ParsingMode, + settings: ServerSettings): RequestParsing.ParseRequestResult = { val (serverSettings, parserSettings) = { val ps = settings.parserSettings.withUriParsingMode(uriParsingMode) (settings.withParserSettings(ps), ps) @@ -52,30 +55,59 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp val frame = HeadersFrame(1, data == Source.empty, endHeaders = true, encoder.encodeHeaderPairs(keyValuePairs), None) - val parseRequest: Http2SubStream => HttpRequest = - RequestParsing.parseRequest(headerParser, serverSettings, attributes) - - try Source.single(frame) - .via(new HeaderDecompression(headerParser, parserSettings)) - .map { // emulate demux - case headers: ParsedHeadersFrame => - Http2SubStream( - initialHeaders = headers, - trailingHeaders = OptionVal.None, - data = Right(data), - correlationAttributes = Map.empty) - } - .map(parseRequest) - .runWith(Sink.head) - .futureValue - catch { case ex: Throwable => throw ex.getCause } // unpack futureValue exceptions + val parseRequest: Http2SubStream => ParseRequestResult = RequestParsing.parseRequest(headerParser, serverSettings, + attributes) + + Source.single(frame) + .via(new HeaderDecompression(headerParser, parserSettings)) + .map { // emulate demux + case headers: ParsedHeadersFrame => + Http2SubStream( + initialHeaders = headers, + trailingHeaders = OptionVal.None, + data = Right(data), + correlationAttributes = Map.empty) + } + .map(parseRequest) + .runWith(Sink.head) + .futureValue } - def shouldThrowMalformedRequest[T](block: => T): Exception = { - val thrown = the[RuntimeException] thrownBy block - thrown.getMessage should startWith("Malformed request: ") - thrown - } + def parseExpectOk( + keyValuePairs: Seq[(String, String)], + data: Source[ByteString, Any] = Source.empty, + attributes: Attributes = Attributes(), + uriParsingMode: Uri.ParsingMode = Uri.ParsingMode.Relaxed, + settings: ServerSettings = ServerSettings(system)): HttpRequest = + parse(keyValuePairs, data, attributes, uriParsingMode, settings) match { + case RequestParsing.OkRequest(req) => req + case RequestParsing.BadRequest(info, _) => fail(s"Failed parsing request: $info") + } + + def parseExpectError( + keyValuePairs: Seq[(String, String)], + data: Source[ByteString, Any] = Source.empty, + attributes: Attributes = Attributes(), + uriParsingMode: Uri.ParsingMode = Uri.ParsingMode.Relaxed, + settings: ServerSettings = ServerSettings(system)): ErrorInfo = + parse(keyValuePairs, data, attributes, uriParsingMode, settings) match { + case RequestParsing.OkRequest(req) => fail("Unexpectedly succeeded parsing request") + case RequestParsing.BadRequest(info, _) => info + } + + def parseExpectProtocolError( + keyValuePairs: Seq[(String, String)], + data: Source[ByteString, Any] = Source.empty, + attributes: Attributes = Attributes(), + uriParsingMode: Uri.ParsingMode = Uri.ParsingMode.Relaxed, + settings: ServerSettings = ServerSettings(system)): Http2ProtocolException = + try { + parse(keyValuePairs, data, attributes, uriParsingMode, settings) + fail("expected parsing to throw") + } catch { + case futureValueEx: TestFailedException if futureValueEx.getCause.isInstanceOf[Http2ProtocolException] => + futureValueEx.getCause.asInstanceOf[Http2ProtocolException] + } "follow RFC7540" should { @@ -85,13 +117,13 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // appear in requests. "not accept response pseudo-header fields in a request" in { - val thrown = shouldThrowMalformedRequest(parse( + val info = parseExpectError( keyValuePairs = Vector( ":scheme" -> "https", ":method" -> "GET", ":path" -> "/", - ":status" -> "200"))) - thrown.getMessage should ===( + ":status" -> "200")) + info.summary should ===( "Malformed request: Pseudo-header ':status' is for responses only; it cannot appear in a request") } @@ -109,7 +141,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // Insert the Foo header so it occurs before at least one pseudo-header val (before, after) = pseudoHeaders.splitAt(insertPoint) val modified = before ++ Vector("Foo" -> "bar") ++ after - shouldThrowMalformedRequest(parse(modified)) + parseExpectError(modified) } } @@ -119,32 +151,30 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // be treated as malformed... "not accept connection-specific headers" in { - shouldThrowMalformedRequest { - // Add Connection header to indicate that Foo is a connection-specific header - parse(Vector( - ":method" -> "GET", - ":scheme" -> "https", - ":path" -> "/", - "Connection" -> "foo", - "Foo" -> "bar")) - } + // Add Connection header to indicate that Foo is a connection-specific header + parseExpectError(Vector( + ":method" -> "GET", + ":scheme" -> "https", + ":path" -> "/", + "Connection" -> "foo", + "Foo" -> "bar")) } "not accept TE with other values than 'trailers'" in { - shouldThrowMalformedRequest { - // The only exception to this is the TE header field, which MAY be - // present in an HTTP/2 request; when it is, it MUST NOT contain any - // value other than "trailers". - parse(Vector( - ":method" -> "GET", - ":scheme" -> "https", - ":path" -> "/", - "TE" -> "chunked")) - } + + // The only exception to this is the TE header field, which MAY be + // present in an HTTP/2 request; when it is, it MUST NOT contain any + // value other than "trailers". + parseExpectError(Vector( + ":method" -> "GET", + ":scheme" -> "https", + ":path" -> "/", + "TE" -> "chunked")) + } "accept TE with 'trailers' as value" in { - parse(Vector( + parseExpectOk(Vector( ":method" -> "GET", ":scheme" -> "https", ":path" -> "/", @@ -159,7 +189,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp "parse the ':method' pseudo-header correctly" in { val methods = Seq("GET", "POST", "DELETE", "OPTIONS") forAll(methods) { (method: String) => - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> method, ":scheme" -> "https", @@ -181,7 +211,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // can't be constructed with any other schemes. val schemes = Seq("http", "https", "ws", "wss") forAll(schemes) { (scheme: String) => - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "POST", ":scheme" -> scheme, @@ -206,7 +236,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp ("example.com:8042", "example.com", Some(8042))) forAll(authorities) { case (authority, host, optPort) => - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "POST", ":scheme" -> "https", @@ -221,14 +251,13 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp val authorities = Seq("?", " ", "@", ":") forAll(authorities) { authority => - val thrown = the[ParsingException] thrownBy - (parse( - keyValuePairs = Vector( - ":method" -> "POST", - ":scheme" -> "https", - ":authority" -> authority, - ":path" -> "/"))) - thrown.getMessage should include("http2-authority-pseudo-header") + val info = parseExpectError( + keyValuePairs = Vector( + ":method" -> "POST", + ":scheme" -> "https", + ":authority" -> authority, + ":path" -> "/")) + info.summary should include("http2-authority-pseudo-header") } } } @@ -249,14 +278,14 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp val schemes = Seq("http", "https") forAll(schemes) { (scheme: String) => forAll(authorities) { (authority: String) => - val exception = the[Exception] thrownBy - (parse( - keyValuePairs = Vector( - ":method" -> "POST", - ":scheme" -> scheme, - ":authority" -> authority, - ":path" -> "/"))) - exception.getMessage should startWith("Illegal http2-authority-pseudo-header") + val info = parseExpectError( + keyValuePairs = Vector( + ":method" -> "POST", + ":scheme" -> scheme, + ":authority" -> authority, + ":path" -> "/" + )) + info.summary should startWith("Illegal http2-authority-pseudo-header") } } } @@ -269,9 +298,14 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp "follow RFC3986 for the ':path' pseudo-header" should { def parsePath(path: String, uriParsingMode: Uri.ParsingMode = Uri.ParsingMode.Relaxed): Uri = { - parse(Seq(":method" -> "GET", ":scheme" -> "https", ":path" -> path), uriParsingMode = uriParsingMode).uri + parseExpectOk(Seq(":method" -> "GET", ":scheme" -> "https", ":path" -> path), + uriParsingMode = uriParsingMode).uri } + def parsePathExpectError(path: String, uriParsingMode: Uri.ParsingMode = Uri.ParsingMode.Relaxed): ErrorInfo = + parseExpectError(Seq(":method" -> "GET", ":scheme" -> "https", ":path" -> path), + uriParsingMode = uriParsingMode) + // sub-delims = "!" / "$" / "&" / "'" / "(" / ")" // / "*" / "+" / "," / ";" / "=" @@ -333,8 +367,8 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp "?", "&", "=", "#", ":", "?", "#", "[", "]", "@", " ", "http://localhost/foo") forAll(invalidAbsolutePaths) { (absPath: String) => - val exception = the[ParsingException] thrownBy (parsePath(absPath)) - exception.getMessage should include("http2-path-pseudo-header") + val info = parsePathExpectError(absPath) + info.summary should include("http2-path-pseudo-header") } } @@ -343,8 +377,8 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // Illegal for path-absolute in RFC3986 to start with multiple slashes "//", "//x") forAll(invalidAbsolutePaths) { (absPath: String) => - val exception = the[ParsingException] thrownBy (parsePath(absPath, uriParsingMode = Uri.ParsingMode.Strict)) - exception.getMessage should include("http2-path-pseudo-header") + val info = parsePathExpectError(absPath, uriParsingMode = Uri.ParsingMode.Strict) + info.summary should include("http2-path-pseudo-header") } } @@ -386,10 +420,11 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp "reject a ':path' containing an invalid 'query'" in pendingUntilFixed { val invalidQueries: Seq[String] = Seq( ":", "/", "?", "#", "[", "]", "@", " ") + forAll(absolutePaths.take(3)) { case (inputPath, _) => forAll(invalidQueries) { (query: String) => - shouldThrowMalformedRequest(parsePath(inputPath + "?" + query, uriParsingMode = Uri.ParsingMode.Strict)) + parsePathExpectError(inputPath + "?" + query, uriParsingMode = Uri.ParsingMode.Strict) } } } @@ -400,7 +435,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // value '*' for the ":path" pseudo-header field. "handle a ':path' with an asterisk" in pendingUntilFixed { - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "OPTIONS", ":scheme" -> "http", @@ -411,14 +446,14 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // [The ":path"] pseudo-header field MUST NOT be empty for "http" or "https" // URIs... - "reject empty ':path' pseudo-headers for http and https" in pendingUntilFixed { + "reject empty ':path' pseudo-headers for http and https" in { val schemes = Seq("http", "https") forAll(schemes) { (scheme: String) => - shouldThrowMalformedRequest(parse( + parseExpectError( keyValuePairs = Vector( ":method" -> "POST", ":scheme" -> scheme, - ":path" -> ""))) + ":path" -> "")) } } @@ -439,12 +474,12 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp "reject requests without a mandatory pseudo-headers" in { val mandatoryPseudoHeaders = Seq(":method", ":scheme", ":path") forAll(mandatoryPseudoHeaders) { (name: String) => - val thrown = shouldThrowMalformedRequest(parse( + val ex = parseExpectProtocolError( keyValuePairs = Vector( ":scheme" -> "https", ":method" -> "GET", - ":path" -> "/").filter(_._1 != name))) - thrown.getMessage should ===(s"Malformed request: Mandatory pseudo-header '$name' missing") + ":path" -> "/").filter(_._1 != name)) + ex.getMessage should ===(s"Malformed request: Mandatory pseudo-header '$name' missing") } } @@ -453,13 +488,13 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp Seq(":method" -> "POST", ":scheme" -> "http", ":path" -> "/other", ":authority" -> "example.org") forAll(pseudoHeaders) { case (name: String, alternative: String) => - val thrown = shouldThrowMalformedRequest(parse( + val ex = parseExpectProtocolError( keyValuePairs = Vector( ":scheme" -> "https", ":method" -> "GET", ":authority" -> "pekko.apache.org", - ":path" -> "/") :+ (name -> alternative))) - thrown.getMessage should ===(s"Malformed request: Pseudo-header '$name' must not occur more than once") + ":path" -> "/") :+ (name -> alternative)) + ex.getMessage should ===(s"Malformed request: Pseudo-header '$name' must not occur more than once") } } @@ -479,7 +514,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp Seq("a=b", "c=d; e=f") -> "a=b; c=d; e=f") forAll(cookieHeaders) { case (inValues, outValue) => - val httpRequest: HttpRequest = parse( + val httpRequest: HttpRequest = parseExpectOk( Vector( ":method" -> "GET", ":scheme" -> "https", @@ -495,7 +530,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // 8.1.3. Examples "parse GET example" in { - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "GET", ":scheme" -> "https", @@ -518,7 +553,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp } "parse POST example" in { - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "POST", ":scheme" -> "https", @@ -552,7 +587,7 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp // Tests that don't come from an RFC document... "parse GET https://localhost:8000/ correctly" in { - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "GET", ":scheme" -> "https", @@ -571,42 +606,41 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp } "reject requests with multiple content length headers" in { - val thrown = shouldThrowMalformedRequest(parse( + val info = parseExpectError( keyValuePairs = Vector( ":method" -> "GET", ":scheme" -> "https", ":authority" -> "localhost:8000", ":path" -> "/", "content-length" -> "123", - "content-length" -> "124"))) - thrown.getMessage should ===( + "content-length" -> "124")) + info.summary should ===( s"Malformed request: HTTP message must not contain more than one content-length header") } "reject requests with multiple content type headers" in { - val thrown = shouldThrowMalformedRequest(parse( + val info = parseExpectError( keyValuePairs = Vector( ":method" -> "GET", ":scheme" -> "https", ":authority" -> "localhost:8000", ":path" -> "/", "content-type" -> "text/json", - "content-type" -> "text/json"))) - thrown.getMessage should ===( + "content-type" -> "text/json")) + info.summary should ===( s"Malformed request: HTTP message must not contain more than one content-type header") } "reject requests with too many headers" in { val maxHeaderCount = ServerSettings(system).parserSettings.maxHeaderCount - val thrown = shouldThrowMalformedRequest( - parse((0 to (maxHeaderCount + 1)).map(n => s"x-my-header-$n" -> n.toString).toVector)) - thrown.getMessage should ===( + val info = parseExpectError((0 to (maxHeaderCount + 1)).map(n => s"x-my-header-$n" -> n.toString).toVector) + info.summary should ===( s"Malformed request: HTTP message contains more than the configured limit of $maxHeaderCount headers") } "add remote address request attribute if enabled" in { val theAddress = InetAddress.getByName("127.5.2.1") - val request: HttpRequest = parse( + val request: HttpRequest = parseExpectOk( keyValuePairs = Vector( ":method" -> "GET", ":scheme" -> "https",