Skip to content

Commit 0c3cf59

Browse files
committed
fix(mcp): guard decoder errors and null-body drain against unhandled crashes
- Attach the stream bridge's error listener before piping into the zlib decoder, so a synchronous zlib error (server mislabeling a non-gzip body as gzip) rejects the reader instead of crashing the process. - Attach an error listener before draining a null-body response, and wrap Response construction in try/catch that destroys the source (no socket leak) on an out-of-range status. Adds an invalid-gzip regression test.
1 parent cf8469e commit 0c3cf59

2 files changed

Lines changed: 43 additions & 18 deletions

File tree

apps/sim/lib/core/security/guarded-request-fetch.server.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,21 @@ describe('createSsrfGuardedFetchWithDispatcher (undici.request backed)', () => {
202202
expect(response.headers.get('content-length')).toBeNull()
203203
})
204204

205+
it('rejects the reader (not crash) when Content-Encoding is gzip but the body is not', async () => {
206+
const source = new Readable({ read() {} })
207+
source.push(Buffer.from('this is definitely not gzip'))
208+
source.push(null)
209+
mockUndiciRequest.mockResolvedValueOnce(
210+
undiciReply(200, { 'content-type': 'application/json', 'content-encoding': 'gzip' }, source)
211+
)
212+
const { fetch } = createSsrfGuardedFetchWithDispatcher()
213+
214+
const response = await fetch('https://mcp.example.com/bad', { method: 'GET' })
215+
216+
// The zlib error must surface as a rejected read, never an unhandled 'error' event.
217+
await expect(response.text()).rejects.toThrow()
218+
})
219+
205220
it('rejects the reader when the source is destroyed without an error (abort/reset)', async () => {
206221
const source = new Readable({ read() {} }) // stays open, never pushes
207222
mockUndiciRequest.mockResolvedValueOnce(undiciReply(200, {}, source))

apps/sim/lib/core/security/input-validation.server.ts

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -831,43 +831,53 @@ async function undiciRequestAsResponse(
831831
else if (value != null) responseHeaders.append(key, value)
832832
}
833833

834-
// 204/205/304 carry no body; `new Response` rejects a non-null body for them. Drain
835-
// undici's (empty) stream so its socket returns to the Agent pool instead of leaking.
834+
// Null-body statuses (204/205/304) can't carry a body; drain undici's (empty) stream so its
835+
// socket returns to the pool. Attach an error listener first so a socket reset mid-drain
836+
// surfaces as a handled event, not an unhandled 'error' that crashes the process.
836837
const isNullBody = statusCode === 204 || statusCode === 205 || statusCode === 304
837838
if (isNullBody) {
839+
body.on('error', () => {})
838840
body.resume()
839841
const response = new Response(null, { status: statusCode, headers: responseHeaders })
840842
Object.defineProperty(response, 'url', { value: url, configurable: true })
841843
return response
842844
}
843845

844846
// Decode Content-Encoding like `fetch` does (`request()` returns raw bytes). `maxResponseSize`
845-
// still caps the compressed wire bytes on `body`; forward its errors into the decoder so the
846-
// cap and any socket reset still surface as a stream error, and tear the source down when the
847-
// decoded stream ends or is cancelled so the socket can't leak.
847+
// still caps the compressed wire bytes on `body`.
848848
const contentEncoding = String(headers['content-encoding'] ?? '')
849849
.toLowerCase()
850850
.trim()
851851
const decoder = contentEncodingDecoder(contentEncoding)
852-
let bodyStream: Readable = body
853852
if (decoder) {
854-
body.once('error', (err) => decoder.destroy(err))
855-
decoder.once('close', () => body.destroy())
856-
bodyStream = body.pipe(decoder)
857853
// The bridged body is now decoded; drop framing headers that would misdescribe it.
858854
responseHeaders.delete('content-encoding')
859855
responseHeaders.delete('content-length')
860856
}
857+
// Build the bridge over the stream the consumer reads (the decoder when decoding).
858+
// `nodeReadableToWebStream` attaches its `error` listener synchronously, so wiring the pipe
859+
// AFTER it means a synchronous zlib error (e.g. a server mislabeling a non-gzip body as gzip)
860+
// is caught and rejects the reader instead of taking down the process.
861+
const webBody = nodeReadableToWebStream(decoder ?? body)
862+
if (decoder) {
863+
body.once('error', (err) => decoder.destroy(err)) // forward maxResponseSize / socket reset
864+
decoder.once('close', () => body.destroy()) // tear the source down so the socket can't leak
865+
body.pipe(decoder)
866+
}
861867

862-
const response = new Response(nodeReadableToWebStream(bodyStream), {
863-
status: statusCode,
864-
headers: responseHeaders,
865-
})
866-
// undici.request never sets `url`; `fetch` did, and consumers rely on it (the MCP
867-
// transport's response-cap wrapper copies it; the SDK resolves relative
868-
// auth-metadata URLs against it). Preserve parity.
869-
Object.defineProperty(response, 'url', { value: url, configurable: true })
870-
return response
868+
try {
869+
const response = new Response(webBody, { status: statusCode, headers: responseHeaders })
870+
// undici.request never sets `url`; `fetch` did, and consumers rely on it (the MCP
871+
// transport's response-cap wrapper copies it; the SDK resolves relative
872+
// auth-metadata URLs against it). Preserve parity.
873+
Object.defineProperty(response, 'url', { value: url, configurable: true })
874+
return response
875+
} catch (err) {
876+
// `new Response` rejects an out-of-range status (a 1xx undici shouldn't surface, but
877+
// defensively): destroy the source so its socket can't leak, then rethrow.
878+
body.destroy()
879+
throw err
880+
}
871881
}
872882

873883
/**

0 commit comments

Comments
 (0)