fix: recover complete request bodies after client disconnect#375
fix: recover complete request bodies after client disconnect#375usualoma wants to merge 5 commits into
Conversation
When application code has called setEncoding() on the IncomingMessage, data events yield strings in that encoding. Converting those chunks back to bytes as UTF-8 corrupts bodies decoded with another encoding. Decode with incoming.readableEncoding and cover the streamed latin1 case.
A client can disconnect after Node has parsed the complete HTTP/1 request but before the application reads it. Recover the untouched buffered body for direct reads, native Request reads, formData(), and raw body streams. Share and cache the recovery result across paths, preserve application errors, reject lossy string encodings, and validate canonical content-length values. Add real-socket and synthetic coverage with bounded waits so regressions fail without leaking the test server.
Inspired by #373. Co-authored-by: BlankParticle <blankparticle@gmail.com>
Adopts the normalizeAbortError helper from #373. Co-authored-by: BlankParticle <blankparticle@gmail.com>
| const isRecoverableDisconnectedIncoming = ( | ||
| incoming: IncomingMessage | Http2ServerRequest | ||
| ): incoming is IncomingMessage => | ||
| !(incoming instanceof Http2ServerRequest) && |
There was a problem hiding this comment.
not a fan of instanceof checks
because the input is either a http/1 or http/2 request, I think incoming.httpVersionMajor === 1 is a much better check
There was a problem hiding this comment.
While I do understand your preference, since we've basically been using incoming instanceof Http2ServerRequest throughout this project, I think it would be more appropriate to use incoming instanceof Http2ServerRequest here as well.
There was a problem hiding this comment.
if it's a pattern already being used here, then I guess it's fine doing it here too
I just have a distrust of instanceof checks because libs including hono replaces the globals and these kind of checks fail in application code
| const recoverCompleteBodyAfterDisconnect = (error?: unknown): boolean => { | ||
| const streamError = incoming.errored ?? error | ||
| if ( | ||
| incoming instanceof Http2ServerRequest || |
There was a problem hiding this comment.
we should use the existing isRecoverableDisconnectedIncoming here and add the last condition with it
Co-authored-by: BlankParticle <blankparticle@gmail.com>
2 participants
fixes #370
A client disconnect marks the
IncomingMessageas disturbed even when Node's HTTP/1 parser has already received the complete request body. As a result, a later body read can incorrectly fail withBody is unusable.This follows the same core approach as #373:
incoming.readableDidReadinstead ofReadable.isDisturbed(incoming)to determine whether the body was actually consumed.The main difference is that the recovery is shared across the request body-reading paths rather than being limited to
readBodyDirect. This also coversformData(),c.req.raw.body, and paths that create or use the nativeRequestcache.Although the diff is relatively large, the core behavior is the same. Most of the additional code and tests ensure that recovery is applied consistently while rejecting partially consumed bodies, preserving application-provided stream errors, and avoiding byte corruption when
setEncoding()has been used.