Skip to content

fix: destroy sockets when errors occur after headers flush - #1395

Open
Slmpire wants to merge 3 commits into
CalloraOrg:mainfrom
Slmpire:security/issue-1277-destroy-sockets-when-errors-occur-after-headers
Open

Slmpire wants to merge 3 commits into
CalloraOrg:mainfrom
Slmpire:security/issue-1277-destroy-sockets-when-errors-occur-after-headers

Conversation

@Slmpire

@Slmpire Slmpire commented Sep 29, 2026

Copy link
Copy Markdown

Overview

This PR fixes a socket/connection leak in the error handling path. Previously, errorHandler only wrote a response when !res.headersSent; when an error occurred after headers had already been flushed (e.g. handleProxy failing mid-stream or an SSE handler throwing after flushHeaders), the handler returned without ending the response. This left the connection open with a truncated body, causing clients to hang until their own timeout and leaking sockets on the server.

The fix ensures that when headers are already sent, the error is logged once with the request ID and the response is destroyed so the client observes a terminated stream.

Related Issue

Changes

🛡️ Error Handling

  • [MODIFY] src/middleware/errorHandler.ts

    • When res.headersSent is true, log the error once (including requestId) and call res.destroy(err) to terminate the connection instead of returning silently.
    • Preserve existing behavior for the !res.headersSent path (unchanged response writing).
    • Guard against double-handling so the error is logged exactly once per request.
  • [MODIFY] src/routes/proxyRoutes.ts

    • Ensure mid-stream failures in the proxy path propagate to the error handler so the socket is destroyed rather than left open after a partial body.

🧪 Tests

  • [MODIFY] src/middleware/errorHandler.test.ts
    • Added coverage for the mid-stream case: an error thrown after res.write / headers flush results in the connection being destroyed.
    • Asserts the error is logged once with the request ID.
    • Asserts responses without headers sent are unchanged.

Verification Results

npm test -- src/middleware/errorHandler.test.ts src/__tests__/proxy.integration.test.ts
✅ passed
Acceptance Criteria Status
An error thrown after res.write closes the connection ✅ res.destroy(err) invoked when res.headersSent
The error is logged once with requestId ✅ Single log entry including requestId
Responses without headers sent are unchanged ✅ Existing !res.headersSent branch preserved
src/middleware/errorHandler.test.ts covers the mid-stream case ✅ Added mid-stream test

Security and Failure-Mode Handling

  • Destroying the socket on post-flush errors prevents clients from treating a truncated upstream body as complete, avoiding partial-data misinterpretation.
  • Logging once with requestId preserves observability without log spam on repeated error paths.
  • No safeguards or validation were weakened; the pre-flush response path is untouched.

Non-Goals

  • No typo-only, formatting-only, or cosmetic changes.
  • No unrelated refactors, dependency upgrades, or broad rewrites.

Closes #1277

Copilot AI balanced review requested due to automatic review settings September 29, 2026 18:01
@drips-wave

drips-wave Bot commented Sep 29, 2026

Copy link
Copy Markdown

@Slmpire Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Destroy sockets when errors occur after headers flush

2 participants