Http4s: ResponseLogger prints raw request on cancellation

[sebbert]

On this line of code, we log the result of calling toString on the request:
https://github.com/http4s/http4s/blob/a7e2fc108de12f7b111d688d33288c20f6a5a085/server/src/main/scala/org/http4s/server/middleware/ResponseLogger.scala#L70

This is dangerous, as it may end up logging sensitive information, ignoring the logHeaders, logBody, and redactHeadersWhen options.

In our project, we've solved this by using Logger.logMessage to log the request in this case, re-using the same options as for responses and introducing a constraint on the request type, A <: Message[F].

However, this may not be a good solution for getting this patched upstream, since it technically breaks the API (introducing a new type constraint), and re-uses the response logging configuration for requests.

[rossabaker]

Ouch. Yeah, we need to get this fixed ASAP.

[rossabaker]

So if I'm reading the code right, the default will redact these headers, and your problem is that you have other sensitive headers?

val SensitiveHeaders = Set(
  "Authorization".ci,
  "Cookie".ci,
  "Set-Cookie".ci
)

This is tricky. Constraining A to be a Message[F] defeats the polymorphism of the middleware. We could pattern match on A and if it's a request, do something safer. But then we'd need to handle ContextRequest. And who's to say that people haven't invented their own A with a Request?

The only right answer here might be to be much more careful about what we put in .toString, since we can't pass the sensitive headers filter reliably in this context.

[rossabaker]

Also, much less gravely, while we're at it, canceled is one l. :smile:

[sebbert]

So if I'm reading the code right, the default will redact these headers, and your problem is that you have other sensitive headers?

Well, we actually disable logHeaders altogether, but we do have additional sensitive headers, yes.

[sebbert]

This might be a stupid question, but would it be possible to move this behavior to RequestLogger? I see there's a call to guaranteeCase there as well: https://github.com/http4s/http4s/blob/master/server/src/main/scala/org/http4s/server/middleware/RequestLogger.scala#L76

[rossabaker]

I think it's nice if there's a response log line for every request log line. I think I'd like to still log something here.

In several places, we refer to requests as ${req.method} ${req.pathInfo}. If we reduced the .toString implementation to that, we'd be safer from this. But we'd also be fighting against people who put a request ID in their headers and are using that to correlate the logs.