pjfanning opened a new pull request, #1297: URL: https://github.com/apache/pekko-http/pull/1297
### Motivation Two things go wrong when an HTTP/2 peer sends a malformed header field. Probed against `main`: ``` :path "/a\r\nb" → 400 Illegal http2-path-pseudo-header :authority "a\r\nb" → 400 Illegal http2-authority-pseudo-header header value "foo\r\nx-b: bar" → 200 headers = [x-a=foo] ← accepted, truncated header value "foo\u0000bar" → stage failure, connection gone ← not a 400 ``` **Silent truncation.** `RequestParsing.parseHeaderPair` reuses the HTTP/1.1 line parser by building `name + ": " + value + "\r\nx"` (`RequestParsing.scala:220`). The parser stops at the first CRLF it meets — which is now the peer's — and the header that comes back is whatever preceded it. RFC 9113 §8.2.1 says a field name or value carrying NUL, CR or LF MUST be treated as malformed. Not an injection (the tail is dropped, not added), but silent acceptance where the spec wants rejection. **Connection teardown.** A field the HTTP/1.1 parser *does* reject — NUL in the value, an illegal character in the name, a value over `max-header-value-length` — is reported with that parser's own internal `pekko.http.impl.engine.parsing.ParsingException`. That is not the model `pekko.http.scaladsl.model.ParsingException` that `HeaderDecompression` catches (`HeaderDecompression.scala:24`), so it escapes the decompression stage and fails it, and with it the whole connection and every stream on it. The HTTP/2 engine answers other malformed fields with a 400 on the one stream — that was the point of #59. On a connection a proxy multiplexes for many users, one bad header from one of them takes the connection down for all of them. ### Modification - Check the name and the value for CR, LF and NUL at the top of the HPACK listener, before the field is dispatched on its name, reusing `HeaderCompression.hasIllegalChar` from the rendering side (#1258). Neither is echoed in the error, since either may be what is malformed. - Widen the internal `ParsingException` from `private[parsing]` to `private[http]` and rethrow it from `parseHeaderPair` as the model exception, so that every failure the HTTP/1.1 parser reports for an HTTP/2 field takes the existing 400 path. ### Result A header field carrying CR, LF or NUL is answered with a 400 instead of being accepted truncated, and a field the HTTP/1.1 parser rejects is answered with a 400 on its own stream instead of failing the connection. ### Tests - `RequestParsingSpec`: CR LF, bare LF and NUL in a value, CR LF in a name, and a value over `max-header-value-length` each produce a `BadRequest` with the expected summary. - `Http2ServerSpec`: the CR LF, NUL and over-long cases each get a 400 on their stream, **and the next stream on the same connection is served** — which is the part that fails today for the NUL and over-long cases. - Directional, in two steps: with both source changes reverted all eight fail; with only the CR/LF/NUL check in place the two over-long cases still fail, so the exception translation is covered on its own. - `sbt "http2-tests/test"` — 374 pass. - `sbt "http-core/mimaReportBinaryIssues"` — pass. - Native `scalafmt --list --mode diff-ref=upstream/main` — clean. ### References Refs #59 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
