Skip to content

Prevents setting headers multiple times #552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
w3nl merged 3 commits into from
Feb 18, 2026
Merged

Prevents setting headers multiple times #552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d52fa3
Prevents setting headers multiple times
pieterwigboldus-visma Feb 18, 2026
3cf3812
Prevents further processing after headers are sent
pieterwigboldus-visma Feb 18, 2026
e0ac0ac
Prevents further processing after headers sent
pieterwigboldus-visma Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/handlers/not-found.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export const notFound = (_context, request, response) => {
response.status(404)
if (!response.headersSent) {
response.status(404)
}
return {
status: 404,
timestamp: new Date(),
Expand Down
4 changes: 3 additions & 1 deletion src/handlers/request-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export const requestValidation = (context, request, response) => {
response.status(400)
if (!response.headersSent) {
response.status(400)
}
return {
errors: context.validation.errors,
status: 400,
Expand Down
20 changes: 14 additions & 6 deletions src/handlers/response-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
export default (logger, validateResponse) => (context, request, response) => {
// Prevent sending headers if they're already sent
if (response.headersSent) {
return response.end()
}
Comment on lines +3 to +5
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@w3nl w3nl Feb 18, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

const responseDoesntNeedValidation = response.statusCode >= 400
if (responseDoesntNeedValidation) {
return response.json(context.response)
Expand All @@ -22,12 +27,15 @@ export default (logger, validateResponse) => (context, request, response) => {
response: context.response
})
}
return response.status(502).json({
errors: valid.errors,
status: 502,
timestamp: new Date(),
message: 'Bad response'
})
if (!response.headersSent) {
return response.status(502).json({
errors: valid.errors,
status: 502,
timestamp: new Date(),
message: 'Bad response'
})
}
return response.end()
}
Comment on lines +30 to 39
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.

if (!context.response) {
Expand Down
4 changes: 3 additions & 1 deletion src/handlers/unauthorized.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export const unauthorized = async (context, request, response) => {
response.status(401)
if (!response.headersSent) {
response.status(401)
}
return {
status: 401,
timestamp: new Date(),
Expand Down
Loading