Uh oh!
There was an error while loading. Please reload this page.
Prevents setting headers multiple times - #552
Conversation
Ensures that response headers are not sent multiple times by checking if headers have already been sent before setting the status code or sending a JSON response. This prevents potential errors and unexpected behavior when dealing with response handling.
There was a problem hiding this comment.
Pull request overview
This PR adds protection against sending response headers multiple times by checking the response.headersSent property before setting status codes or sending responses. This prevents potential errors when response handlers are called after headers have already been sent by previous handlers or middleware.
Changes:
- Added headersSent checks before status code setting in error handlers (unauthorized, request-validation, not-found)
- Added early return and additional headersSent checks in response-validation handler
- Ensures defensive handling when headers may have already been sent by previous middleware
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/handlers/unauthorized.js | Added headersSent check before setting 401 status |
| src/handlers/response-validation.js | Added early return when headers sent and check before 502 error response |
| src/handlers/request-validation.js | Added headersSent check before setting 400 status |
| src/handlers/not-found.js | Added headersSent check before setting 404 status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!response.headersSent) { | ||
| return response.status(502).json({ | ||
| errors: valid.errors, | ||
| status: 502, | ||
| timestamp: new Date(), | ||
| message: 'Bad response' | ||
| }) | ||
| } | ||
| return undefined |
There was a problem hiding this comment.
There's an inconsistency in how headers-already-sent is handled. In other handlers (unauthorized.js, request-validation.js, not-found.js), the response object is always returned even if headers were already sent - only the status code setting is skipped. However, here the function returns undefined when headers are already sent. This inconsistency could lead to different behavior. Consider returning the error response object (without setting status) to match the pattern in other handlers, which would allow downstream code to see what the intended response was.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if (response.headersSent) { | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
This early return when headers are already sent is inconsistent with the pattern used in other handlers (unauthorized.js, request-validation.js, not-found.js), where the handler still returns a response object even if headers were already sent - they just skip setting the status code. Returning undefined here might prevent downstream code from understanding what should have been returned. Consider removing this early return and instead checking headersSent before each response.json/response.send/response.end call, which would allow the handler to still process and validate the response while avoiding sending headers twice.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Ensures that no further processing occurs after the response headers have already been sent, preventing potential errors. This change avoids attempting to modify headers or send additional data after the headers have been implicitly or explicitly sent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Prevent sending headers if they're already sent | ||
| if (response.headersSent) { | ||
| return response.end() | ||
| } |
There was a problem hiding this comment.
response.headersSent indicates headers were written, but not necessarily that the response should be terminated. Calling response.end() here can prematurely close streaming/SSE or other manually-written responses. Consider making this a no-op (e.g., return), or only calling end() when the response is not already finished (e.g., check response.writableEnded / response.finished).
| if (!response.headersSent) { | ||
| return response.status(502).json({ | ||
| errors: valid.errors, | ||
| status: 502, | ||
| timestamp: new Date(), | ||
| message: 'Bad response' | ||
| }) | ||
| } | ||
| return response.end() | ||
| } |
There was a problem hiding this comment.
In the validation-error path, falling back to response.end() when headersSent is true has the same risk of terminating an in-progress streamed/manual response. Prefer returning without writing, or only ending when the response hasn't already been ended (writableEnded/finished).
| // Prevent sending headers if they're already sent | ||
| if (response.headersSent) { | ||
| return response.end() | ||
| } |
There was a problem hiding this comment.
This change introduces new behavior for the headersSent cases, but there are no unit tests asserting what should happen when headersSent is true (e.g., ensuring no further writes occur, and that existing responses aren't unintentionally ended). Adding coverage would help prevent regressions in response handling.
Avoids potential errors by ensuring that no further actions are taken after the headers have already been sent in the response, such as after a redirect. This prevents issues like attempting to modify or send the response multiple times.
Uh oh!
There was an error while loading. Please reload this page.
Ensures that response headers are not sent multiple times by checking if headers have already been sent before setting the status code or sending a JSON response. This prevents potential errors and unexpected behavior when dealing with response handling.