pjfanning opened a new issue, #1245:
URL: https://github.com/apache/pekko-http/issues/1245
`ParsingErrorHandler` is the public extension point for customising the
response to a request that fails to parse, selected by
`pekko.http.server.parsing-error-handler`. It has one method:
```scala
def handle(status: StatusCode, error: ErrorInfo, log: LoggingAdapter,
settings: ServerSettings): HttpResponse
```
None of those four arguments describe the request that was rejected. A
custom handler can branch on the status code and on
`ErrorInfo.summary`/`detail`, but it cannot recover the method or the
request-target, so it cannot log which target was rejected, count rejections
per client, or return a target-dependent response. For an extension point whose
whole purpose is to react to a bad request, that seems like a gap worth naming.
Nothing upstream of the handler has the information either, so this is not a
case of reaching for the wrong hook:
| Symbol | Carries |
| --- | --- |
| `ParserOutput.MessageStartError` | `(status, info)` |
| `ControllerStage.finishWithIllegalRequestError` | `(status, info)` |
| `ControllerStage` constructor | `(settings, log)` — no remote address |
### The data does exist, one rung lower
`HttpRequestParser`'s `GraphStageLogic` still holds it at the moment of
failure:
```java
private HttpMethod method;
private Uri uri;
private ByteString uriBytes;
private String remoteAddressStr(); // from HttpAttributes.RemoteAddress
```
and the field writes happen before the throwing call. From
`parseRequestTarget` in 1.0.0 (unchanged through 1.4.0):
```
39: putfield uriBytes <- raw request-target
stored first
57: UriParser.reset
65: UriParser.parseHttpRequestTarget <- throws for e.g. GET /%%
HTTP/1.1
68: putfield uri
```
with `method` assigned earlier still, during method-token parsing. So for
`GET /%% HTTP/1.1` the parser knows the method is `GET` and the raw target is
`/%%` at the exact point it gives up. All of it is discarded when
`failMessageStart` constructs `MessageStartError(status, info)`.
Worth noting for anyone tempted to read those fields directly from outside:
they are never reset, and the parser instance is reused across a keep-alive
connection, so they can hold the *previous* request's values. Only the parser
itself knows which of them are valid for the request that just failed — which
is why this wants to be fixed in pekko-http rather than worked around
downstream.
### Suggested refactor
Additive, and binary/source compatible because `ParsingErrorHandler` is an
abstract class rather than a trait or interface:
```scala
final class IllegalRequestContext private[http] (
val method: Option[HttpMethod],
val rawRequestTarget: Option[String],
val protocol: Option[HttpProtocol])
abstract class ParsingErrorHandler {
def handle(status: StatusCode, error: ErrorInfo, log: LoggingAdapter,
settings: ServerSettings): HttpResponse
def handle(status: StatusCode, error: ErrorInfo, log: LoggingAdapter,
settings: ServerSettings, context: IllegalRequestContext):
HttpResponse =
handle(status, error, log, settings)
}
```
`ControllerStage` calls the five-argument overload. Existing handlers
override the four-argument one and keep working unchanged.
Internally this means `MessageStartError` carrying the context alongside
`(status, info)`. It is `@InternalApi`, and populating it at the failure site
is what lets the parser include only the fields that are actually valid for the
failed request instead of whatever the previous one left behind.
Two details that could be included or left out:
- the custom-method case fails inside method-token parsing, before `method`
is assigned, but the name is in a local `StringBuilder` at that point and could
be surfaced as the raw method
- `rawRequestTarget` is by definition malformed, attacker-controlled input.
Handing it to a handler is fine; it is worth a scaladoc line saying so, since
anything logging it unescaped inherits that
### Why I am asking
The OpenTelemetry Java agent currently emits no span at all for a request
rejected during parsing —
open-telemetry/opentelemetry-java-instrumentation#5139, reported in 2022 and
still open. open-telemetry/opentelemetry-java-instrumentation#19829 fixes that
by attaching advice to `ParsingErrorHandler.handle`, which is the earliest
public symbol that sees the rejection. Because of the above, the resulting span
reports the status code but is named `HTTP` and has no `http.request.method`,
no `url.path` and no `url.query` — `url.path` is Required in semconv, so it is
knowingly a partial span, on the grounds that it beats no telemetry for a
rejected request. The five-argument overload would let it report the method and
the raw target.
This is not a request to accommodate the agent specifically. Any custom
`ParsingErrorHandler` has the same blind spot; the agent is just a consumer
that makes it concrete.
### Compatibility note for whoever picks this up
Old pekko-http versions will never have the new overload, so the agent has
to support both paths for a long time. That is fine as long as the change stays
additive and detectable, with one caveat: if `DefaultParsingErrorHandler` were
changed to implement the five-argument method *instead of* the four-argument
one, advice matching the four-argument signature would silently stop firing.
Keeping the default implementation routed through the existing method, or
simply noting the change in release notes, avoids that. This is the same class
of silent-breakage risk as #1241.
Related: #1241 (internal APIs the OpenTelemetry agent instruments).
--
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]
