Don't log invalid details about a raw response #51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chris-martin
approved these changes
Jul 24, 2024
`wai` has a `RawResponse` constructor for `Response`[^1]. This is used by applications that need to send raw bytes on the wire, such as WebSockets. This constructor has a field that is also type `Response`; functions such as `responseStatus`[^2] or `responseHeaders` ultimately read from this value since there is no actual status or headers that we can access in the "raw" response itself. This is an obvious but bad thing to do in something like `requestLogger` because this inner response means nothing about what is actually happening. It's there as a "fallback" to be used when raw responses aren't supported. So it's almost always some kind of 500 error response[^3]. Ultimately, this causes any Blammo-using applications to report every WebSockets response as a 500 in the logs. By its nature, there's not much we can do instead, as we know nothing about the response, so this commit just changes things to omit response details entirely for such cases. This is the approach taken by the more official `Network.Wai.Middleware.RequestLogger` too. [^1]: https://hackage.haskell.org/package/wai-3.2.4/docs/src/Network.Wai.Internal.html#Response [^2]: https://hackage.haskell.org/package/wai-3.2.4/docs/src/Network.Wai.html#responseStatus [^3]: https://hackage.haskell.org/package/yesod-core-1.6.26.0/docs/src/Yesod.Core.Handler.html#sendRawResponseNoConduit
The KeyValue class changes in the Aeson version on nightly, so using it means we'd need CPP. We can be concrete.
This ensures we'll break and be intentional if the constructors change in the future.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
waihas aRawResponseconstructor forResponse1. This is usedby applications that need to send raw bytes on the wire, such as
WebSockets.
This constructor has a field that is also type
Response; functionssuch as
responseStatus2 orresponseHeadersultimately read fromthis value since there is no actual status or headers that we can access
in the "raw" response itself.
This is an obvious but bad thing to do in something like
requestLoggerbecause this inner response means nothing about what is actually
happening. It's there as a "fallback" to be used when raw responses
aren't supported. So it's almost always some kind of 500 error
response3. Ultimately, this causes any Blammo-using applications to
report every WebSockets response as a 500 in the logs.
By its nature, there's not much we can do instead, as we know nothing
about the response, so this commit just changes things to omit response
details entirely for such cases. This is the approach taken by the more
official
Network.Wai.Middleware.RequestLoggertoo4.I included a follow-up refactor separately, to (hopefully) make review
easier, but I plan to squash that in before merge.
Footnotes
https://hackage.haskell.org/package/wai-3.2.4/docs/src/Network.Wai.Internal.html#Response ↩
https://hackage.haskell.org/package/wai-3.2.4/docs/src/Network.Wai.html#responseStatus ↩
https://hackage.haskell.org/package/yesod-core-1.6.26.0/docs/src/Yesod.Core.Handler.html#sendRawResponseNoConduit ↩
https://hackage.haskell.org/package/wai-extra-3.1.15/docs/src/Network.Wai.Middleware.RequestLogger.html#detailedMiddleware%27 ↩