Skip to content

Commit 31b3e27

Browse files
pjfanningCopilot
pjfanning
and
Copilot
authored
Invalid HTTP/2 request headers return 400 Bad Request instead of GOAWAY (#961)
* Initial plan * Port akka-http PR #4227: fix invalid HTTP2 request headers leading to bad request responses Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com> * small changes * Fix post-merge issues: remove .right deprecation, remove duplicate import, remove unused @nowarn Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com> * scalafmt * Update RequestParsingSpec.scala * Add test for invalid percent-encoding in HTTP/2 path (pekko-http issue #59) Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com> * Revert RFC-violation errors to protocolError to fix h2spec acceptance tests, keep %_D as BadRequest Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com> * scalafmt --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pjfanning <11783444+pjfanning@users.noreply.github.com>
1 parent b458a6a commit 31b3e27

15 files changed

Lines changed: 343 additions & 147 deletions

File tree

‎http-core/src/main/mima-filters/1.4.x.backwards.excludes/bad-header-http2-response.backwards.excludes‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
18+
# Invalid HTTP/2 request headers return 400 Bad Request instead of GOAWAY
19+
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.copy")
20+
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.this")
21+
ProblemFilters.exclude[MissingTypesProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent$ParsedHeadersFrame$")
22+
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.apply")
23+
ProblemFilters.exclude[IncompatibleSignatureProblem]("org.apache.pekko.http.impl.engine.http2.FrameEvent#ParsedHeadersFrame.unapply")

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameEvent.scala‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import pekko.annotation.InternalApi
1818
import pekko.http.impl.engine.http2.Http2Protocol.ErrorCode
1919
import pekko.http.impl.engine.http2.Http2Protocol.FrameType
2020
import pekko.http.impl.engine.http2.Http2Protocol.SettingIdentifier
21+
import pekko.http.scaladsl.model.ErrorInfo
2122
import pekko.util.ByteString
2223

2324
import scala.collection.immutable
@@ -112,11 +113,14 @@ private[http] object FrameEvent {
112113
/**
113114
* Convenience (logical) representation of a parsed HEADERS frame with zero, one or
114115
* many CONTINUATIONS Frames into a single, decompressed object.
116+
*
117+
* @param headerParseErrorDetails Only used server side, passes header errors from decompression into error response logic
115118
*/
116119
final case class ParsedHeadersFrame(
117120
streamId: Int,
118121
endStream: Boolean,
119122
keyValuePairs: Seq[(String, AnyRef)],
120-
priorityInfo: Option[PriorityFrame]) extends StreamFrameEvent
123+
priorityInfo: Option[PriorityFrame],
124+
headerParseErrorDetails: Option[ErrorInfo]) extends StreamFrameEvent
121125

122126
}

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/FrameLogger.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ private[http2] object FrameLogger {
7979
case GoAwayFrame(lastStreamId, errorCode, debug) =>
8080
LogEntry(0, "GOAY", s"lastStreamId = $lastStreamId, errorCode = $errorCode, debug = ${debug.utf8String}")
8181

82-
case ParsedHeadersFrame(streamId, endStream, kvPairs, prio) =>
82+
case ParsedHeadersFrame(streamId, endStream, kvPairs, prio, _) =>
8383
val prioInfo = if (prio.isDefined) display(entryForFrame(prio.get)) + " " else ""
8484
val kvInfo = kvPairs.map {
8585
case (key, value) => s"$key -> $value"

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ private[http] object Http2Blueprint {
274274
*
275275
* To make use of parallelism requests and responses need to be associated (other than by ordering), suggestion
276276
* is to add a special (virtual) header containing the streamId (or any other kind of token) is added to the HttRequest
277-
* that must be reproduced in an HttpResponse. This can be done automatically for the `bind`` API but for
277+
* that must be reproduced in an HttpResponse. This can be done automatically for the `bind` API but for
278278
* `bindFlow` the user needs to take of this manually.
279279
*/
280280
def httpLayer(settings: ServerSettings, log: LoggingAdapter, dateHeaderRendering: DateHeaderRendering)
@@ -284,12 +284,14 @@ private[http] object Http2Blueprint {
284284
// HttpHeaderParser is not thread safe and should not be called concurrently,
285285
// the internal trie, however, has built-in protection and will do copy-on-write
286286
val masterHttpHeaderParser = HttpHeaderParser(parserSettings, log)
287-
BidiFlow.fromFlows(
288-
Flow[HttpResponse].map(new ResponseRendering(settings, log, dateHeaderRendering)),
289-
Flow[Http2SubStream].via(StreamUtils.statefulAttrsMap { attrs =>
290-
val headerParser = masterHttpHeaderParser.createShallowCopy()
291-
RequestParsing.parseRequest(headerParser, settings, attrs)
292-
}))
287+
RequestErrorFlow().atop(
288+
BidiFlow.fromFlows(
289+
Flow[HttpResponse].map(new ResponseRendering(settings, log, dateHeaderRendering)),
290+
Flow[Http2SubStream].via(StreamUtils.statefulAttrsMap { attrs =>
291+
val headerParser = masterHttpHeaderParser.createShallowCopy()
292+
RequestParsing.parseRequest(headerParser, settings, attrs)
293+
}))
294+
)
293295
}
294296

295297
/**

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2StreamHandling.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ private[http2] trait Http2StreamHandling extends GraphStageLogic with LogHelper
299299
nextStateStream: IncomingStreamBuffer => StreamState,
300300
correlationAttributes: Map[AttributeKey[_], _] = Map.empty): StreamState =
301301
event match {
302-
case frame @ ParsedHeadersFrame(streamId, endStream, _, _) =>
302+
case frame @ ParsedHeadersFrame(streamId, endStream, _, _, _) =>
303303
if (endStream) {
304304
dispatchSubstream(frame, Left(ByteString.empty), correlationAttributes)
305305
nextStateEmpty
@@ -833,7 +833,7 @@ private[http2] trait Http2StreamHandling extends GraphStageLogic with LogHelper
833833
"Found both an attribute with trailing headers, and headers in the `LastChunk`. This is not supported.")
834834
trailer = OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true,
835835
HttpMessageRendering.renderHeaders(headers, log, isServer, shouldRenderAutoHeaders = false,
836-
dateHeaderRendering = DateHeaderRendering.Unavailable), None))
836+
dateHeaderRendering = DateHeaderRendering.Unavailable), None, None))
837837
}
838838

839839
maybePull()

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRendering.scala‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,11 @@ private[http2] sealed abstract class MessageRendering[R <: HttpMessage] extends
9494
shouldRenderAutoHeaders = true, dateHeaderRendering)
9595

9696
val streamId = nextStreamId(r)
97-
val headersFrame = ParsedHeadersFrame(streamId, endStream = r.entity.isKnownEmpty, headerPairs.result(), None)
97+
val headersFrame = ParsedHeadersFrame(streamId, endStream = r.entity.isKnownEmpty, headerPairs.result(), None, None)
9898
val trailingHeadersFrame =
9999
r.attribute(AttributeKeys.trailer) match {
100100
case Some(trailer) if trailer.headers.nonEmpty =>
101-
OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true, trailer.headers, None))
101+
OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true, trailer.headers, None, None))
102102
case _ => OptionVal.None
103103
}
104104

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestErrorFlow.scala‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* license agreements; and to You under the Apache License, version 2.0:
4+
*
5+
* https://www.apache.org/licenses/LICENSE-2.0
6+
*
7+
* This file is part of the Apache Pekko project, which was derived from Akka.
8+
*/
9+
10+
/*
11+
* Copyright (C) 2009-2023 Lightbend Inc. <https://www.lightbend.com>
12+
*/
13+
14+
package org.apache.pekko.http.impl.engine.http2
15+
16+
import org.apache.pekko
17+
import pekko.NotUsed
18+
import pekko.annotation.InternalApi
19+
import pekko.http.impl.engine.http2.RequestParsing.ParseRequestResult
20+
import pekko.http.scaladsl.model.{ HttpRequest, HttpResponse, StatusCodes }
21+
import pekko.stream.{ Attributes, BidiShape, Inlet, Outlet }
22+
import pekko.stream.scaladsl.BidiFlow
23+
import pekko.stream.stage.{ GraphStage, GraphStageLogic, InHandler, OutHandler }
24+
25+
/**
26+
* INTERNAL API
27+
*/
28+
@InternalApi
29+
private[http2] object RequestErrorFlow {
30+
31+
private val _bidiFlow = BidiFlow.fromGraph(new RequestErrorFlow)
32+
def apply(): BidiFlow[HttpResponse, HttpResponse, ParseRequestResult, HttpRequest, NotUsed] = _bidiFlow
33+
34+
}
35+
36+
/**
37+
* INTERNAL API
38+
*/
39+
@InternalApi
40+
private[http2] final class RequestErrorFlow
41+
extends GraphStage[BidiShape[HttpResponse, HttpResponse, ParseRequestResult, HttpRequest]] {
42+
43+
val requestIn = Inlet[ParseRequestResult]("requestIn")
44+
val requestOut = Outlet[HttpRequest]("requestOut")
45+
val responseIn = Inlet[HttpResponse]("responseIn")
46+
val responseOut = Outlet[HttpResponse]("responseOut")
47+
48+
override val shape: BidiShape[HttpResponse, HttpResponse, ParseRequestResult, HttpRequest] =
49+
BidiShape(responseIn, responseOut, requestIn, requestOut)
50+
51+
override def createLogic(inheritedAttributes: Attributes): GraphStageLogic = new GraphStageLogic(shape) {
52+
setHandlers(requestIn, requestOut,
53+
new InHandler with OutHandler {
54+
override def onPush(): Unit = {
55+
grab(requestIn) match {
56+
case RequestParsing.OkRequest(request) => push(requestOut, request)
57+
case notOk: RequestParsing.BadRequest =>
58+
emit(responseOut,
59+
HttpResponse(StatusCodes.BadRequest, entity = notOk.info.summary).addAttribute(Http2.streamId,
60+
notOk.streamId))
61+
pull(requestIn)
62+
}
63+
}
64+
65+
override def onPull(): Unit = pull(requestIn)
66+
})
67+
setHandlers(responseIn, responseOut,
68+
new InHandler with OutHandler {
69+
override def onPush(): Unit = push(responseOut, grab(responseIn))
70+
override def onPull(): Unit = if (!hasBeenPulled(responseIn)) pull(responseIn)
71+
})
72+
73+
}
74+
}

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala‎

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ package org.apache.pekko.http.impl.engine.http2
1616
import javax.net.ssl.SSLSession
1717
import org.apache.pekko
1818
import pekko.annotation.InternalApi
19-
import pekko.http.impl.engine.http2.Http2Compliance.Http2ProtocolException
2019
import pekko.http.impl.engine.parsing.HttpHeaderParser
2120
import pekko.http.impl.engine.server.HttpAttributes
2221
import pekko.http.scaladsl.model
@@ -29,15 +28,20 @@ import pekko.util.OptionVal
2928

3029
import scala.annotation.tailrec
3130
import scala.collection.immutable.VectorBuilder
31+
import scala.util.control.NoStackTrace
3232

3333
/**
3434
* INTERNAL API
3535
*/
3636
@InternalApi
3737
private[http2] object RequestParsing {
3838

39+
sealed trait ParseRequestResult
40+
final case class OkRequest(request: HttpRequest) extends ParseRequestResult
41+
final case class BadRequest(info: ErrorInfo, streamId: Int) extends ParseRequestResult
42+
3943
def parseRequest(httpHeaderParser: HttpHeaderParser, serverSettings: ServerSettings, streamAttributes: Attributes)
40-
: Http2SubStream => HttpRequest = {
44+
: Http2SubStream => ParseRequestResult = {
4145

4246
val remoteAddressAttribute: Option[RemoteAddress] =
4347
if (serverSettings.remoteAddressAttribute) {
@@ -151,19 +155,19 @@ private[http2] object RequestParsing {
151155
rec(incomingHeaders, offset + 1, method, scheme, authority, pathAndRawQuery,
152156
OptionVal.Some(ContentType.get(value)), contentLength, cookies, true, headers)
153157
else
154-
malformedRequest("HTTP message must not contain more than one content-type header")
158+
parseError("HTTP message must not contain more than one content-type header", "content-type")
155159

156160
case ":status" =>
157-
malformedRequest("Pseudo-header ':status' is for responses only; it cannot appear in a request")
161+
protocolError("Pseudo-header ':status' is for responses only; it cannot appear in a request")
158162

159163
case "content-length" =>
160164
if (contentLength == -1) {
161165
val contentLengthValue = ContentLength.get(value).toLong
162166
if (contentLengthValue < 0)
163-
malformedRequest("HTTP message must not contain a negative content-length header")
167+
parseError("HTTP message must not contain a negative content-length header", "content-length")
164168
rec(incomingHeaders, offset + 1, method, scheme, authority, pathAndRawQuery, contentType,
165169
contentLengthValue, cookies, true, headers)
166-
} else malformedRequest("HTTP message must not contain more than one content-length header")
170+
} else parseError("HTTP message must not contain more than one content-length header", "content-length")
167171

168172
case "cookie" =>
169173
// 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 {
182186
}
183187
}
184188

185-
val incomingHeaders = subStream.initialHeaders.keyValuePairs.toIndexedSeq
186-
if (incomingHeaders.size > serverSettings.parserSettings.maxHeaderCount)
187-
malformedRequest(
188-
s"HTTP message contains more than the configured limit of ${serverSettings.parserSettings.maxHeaderCount} headers")
189-
else rec(incomingHeaders, 0)
189+
try {
190+
subStream.initialHeaders.headerParseErrorDetails match {
191+
case Some(details) =>
192+
// header errors already found in decompression
193+
BadRequest(details, subStream.streamId)
194+
case None =>
195+
val incomingHeaders = subStream.initialHeaders.keyValuePairs.toIndexedSeq
196+
if (incomingHeaders.size > serverSettings.parserSettings.maxHeaderCount)
197+
parseError(
198+
s"HTTP message contains more than the configured limit of ${serverSettings.parserSettings.maxHeaderCount} headers")
199+
else OkRequest(rec(incomingHeaders, 0))
200+
}
201+
} catch {
202+
case bre: ParsingException => BadRequest(bre.info, subStream.streamId)
203+
}
190204
}
191205
}
192206

@@ -201,25 +215,34 @@ private[http2] object RequestParsing {
201215
}
202216

203217
private[http2] def checkRequiredPseudoHeader(name: String, value: AnyRef): Unit =
204-
if (value eq null) malformedRequest(s"Mandatory pseudo-header '$name' missing")
218+
if (value eq null) protocolError(s"Mandatory pseudo-header '$name' missing")
205219
private[http2] def checkUniquePseudoHeader(name: String, value: AnyRef): Unit =
206-
if (value ne null) malformedRequest(s"Pseudo-header '$name' must not occur more than once")
220+
if (value ne null) protocolError(s"Pseudo-header '$name' must not occur more than once")
207221
private[http2] def checkNoRegularHeadersBeforePseudoHeader(name: String, seenRegularHeader: Boolean): Unit =
208-
if (seenRegularHeader) malformedRequest(s"Pseudo-header field '$name' must not appear after a regular header")
209-
private[http2] def malformedRequest(msg: String): Nothing =
210-
throw new Http2ProtocolException(s"Malformed request: $msg")
222+
if (seenRegularHeader) protocolError(s"Pseudo-header field '$name' must not appear after a regular header")
211223
private[http2] def validateHeader(httpHeader: HttpHeader) = httpHeader.lowercaseName match {
212224
case "connection" =>
213225
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2
214-
malformedRequest("Header 'Connection' must not be used with HTTP/2")
226+
protocolError("Header 'Connection' must not be used with HTTP/2")
215227
case "transfer-encoding" =>
216228
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2
217-
malformedRequest("Header 'Transfer-Encoding' must not be used with HTTP/2")
229+
protocolError("Header 'Transfer-Encoding' must not be used with HTTP/2")
218230
case "te" =>
219231
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2
220232
if (httpHeader.value.compareToIgnoreCase("trailers") != 0)
221-
malformedRequest(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}")
233+
protocolError(s"Header 'TE' must not contain value other than 'trailers', value was '${httpHeader.value}")
222234
case _ => // ok
223235
}
224236

237+
// parse errors lead to BadRequest response while Protocol
238+
private[http2] def protocolError(summary: String): Nothing =
239+
throw new Http2Compliance.Http2ProtocolException(s"Malformed request: $summary")
240+
241+
private[http2] def parseError(summary: String, headerName: String): Nothing =
242+
throw new ParsingException(ErrorInfo(s"Malformed request: $summary").withErrorHeaderName(headerName))
243+
with NoStackTrace
244+
245+
private def parseError(summary: String): Nothing =
246+
throw new ParsingException(ErrorInfo(s"Malformed request: $summary")) with NoStackTrace
247+
225248
}

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/client/ResponseParsing.scala‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package client
1616

1717
import org.apache.pekko
1818
import pekko.annotation.InternalApi
19+
import pekko.http.impl.engine.http2.Http2Compliance.Http2ProtocolException
1920
import pekko.http.impl.engine.http2.RequestParsing._
2021
import pekko.http.impl.engine.parsing.HttpHeaderParser
2122
import pekko.http.impl.engine.server.HttpAttributes
@@ -85,26 +86,26 @@ private[http2] object ResponseParsing {
8586
rec(remainingHeaders.tail, status, OptionVal.Some(contentTypeValue), contentLength, seenRegularHeader,
8687
headers)
8788
else
88-
malformedRequest("HTTP message must not contain more than one content-type header")
89+
malformedResponse("HTTP message must not contain more than one content-type header")
8990

9091
case ("content-type", ct: String) =>
9192
if (contentType.isEmpty) {
9293
val contentTypeValue =
93-
ContentType.parse(ct).getOrElse(malformedRequest(s"Invalid content-type: '$ct'"))
94+
ContentType.parse(ct).getOrElse(malformedResponse(s"Invalid content-type: '$ct'"))
9495
rec(remainingHeaders.tail, status, OptionVal.Some(contentTypeValue), contentLength, seenRegularHeader,
9596
headers)
96-
} else malformedRequest("HTTP message must not contain more than one content-type header")
97+
} else malformedResponse("HTTP message must not contain more than one content-type header")
9798

9899
case ("content-length", length: String) =>
99100
if (contentLength == -1) {
100101
val contentLengthValue = length.toLong
101102
if (contentLengthValue < 0)
102-
malformedRequest("HTTP message must not contain a negative content-length header")
103+
malformedResponse("HTTP message must not contain a negative content-length header")
103104
rec(remainingHeaders.tail, status, contentType, contentLengthValue, seenRegularHeader, headers)
104-
} else malformedRequest("HTTP message must not contain more than one content-length header")
105+
} else malformedResponse("HTTP message must not contain more than one content-length header")
105106

106107
case (name, _) if name.startsWith(':') =>
107-
malformedRequest(s"Unexpected pseudo-header '$name' in response")
108+
malformedResponse(s"Unexpected pseudo-header '$name' in response")
108109

109110
case (_, httpHeader: HttpHeader) =>
110111
rec(remainingHeaders.tail, status, contentType, contentLength, seenRegularHeader = true,
@@ -119,4 +120,7 @@ private[http2] object ResponseParsing {
119120

120121
rec(subStream.initialHeaders.keyValuePairs)
121122
}
123+
124+
private def malformedResponse(msg: String): Nothing =
125+
throw new Http2ProtocolException(s"Malformed response: $msg")
122126
}

‎http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ private[http2] object HeaderCompression extends GraphStage[FlowShape[FrameEvent,
4848
case ack @ SettingsAckFrame(s) =>
4949
applySettings(s)
5050
push(eventsOut, ack)
51-
case ParsedHeadersFrame(streamId, endStream, kvs, prioInfo) =>
51+
case ParsedHeadersFrame(streamId, endStream, kvs, prioInfo, _) =>
5252
// When ending the stream without any payload, use a DATA frame rather than
5353
// a HEADERS frame to work around https://github.com/golang/go/issues/47851.
5454
if (endStream && kvs.isEmpty) push(eventsOut, DataFrame(streamId, endStream, ByteString.empty))

0 commit comments

Comments
 (0)