Skip to content
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.

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

Prevent sending response headers if already sent in default error han… #2006

Merged

Conversation

suhailgupta03
Copy link
Contributor

Summary

This pull request introduces a safeguard in the defaultProcessEventErrorHandler to prevent attempts to send HTTP response headers after they have already been transmitted. This situation can occur when ack() is invoked before an error is thrown during event processing, leading to potential conflicts in the response handling.

The goal of this PR is to enhance the robustness of the error handling mechanism by ensuring that the server does not encounter ERR_HTTP_HEADERS_SENT errors, which can disrupt the normal flow of Slack event processing. By checking the response.headersSent flag before setting headers or ending the response, we maintain the integrity of the HTTP transaction and provide a more reliable experience for developers using Bolt.js.

Example Scenario

Consider a custom event handler that acknowledges an event and then performs an asynchronous operation which fails:

app.event('custom_event', async ({ ack, say }) => {
  try {
    await ack();
    // An asynchronous operation that may fail
    await someAsyncOperation();
    await say('Event processed successfully!');
  } catch (error) {
    // Error handling logic that might inadvertently try to re-ack or send a response
    console.error('An error occurred:', error);
    // If the following statement crashes, it will cause a server crash
    // because the headers have already been sent when ack() was called.
    await performCleaning();
  }
});

Requirements (place an x in each [ ])

…dler

This change addresses a bug where the defaultProcessEventErrorHandler
could attempt to send HTTP response headers after they've already been
sent, leading to a server error. The fix adds a check for the
response.headersSent property before attempting to write headers or
end the response.

The update ensures that the error handler behaves correctly even if
ack() has been called previously during request processing.
Copy link

salesforce-cla bot commented Dec 1, 2023

Thanks for the contribution! Before we can merge this, we need @suhailgupta03 to sign the Salesforce Inc. Contributor License Agreement.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3142f9a) 82.21% compared to head (9a10a61) 82.00%.

Files Patch % Lines
src/receivers/HTTPModuleFunctions.ts 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2006      +/-   ##
==========================================
- Coverage   82.21%   82.00%   -0.22%     
==========================================
  Files          18       18              
  Lines        1524     1528       +4     
  Branches      438      439       +1     
==========================================
  Hits         1253     1253              
- Misses        175      178       +3     
- Partials       96       97       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality semver:patch labels Dec 1, 2023
@seratch seratch added this to the 3.16.0 milestone Dec 1, 2023
@seratch seratch self-assigned this Dec 1, 2023
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

This is a good catch! Thanks for sending this PR. Could you amend the logging part a little?


// Check if the response headers have already been sent
if (response.headersSent) {
logger.error('Headers already sent, cannot send another response');
Copy link
Member

Choose a reason for hiding this comment

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

Can you adjust the logging for consistency with the following logic?

Suggested change
logger.error('Headers already sent, cannot send another response');
logger.error('An unhandled error occurred after ack() called in a listener');
logger.debug(`Error details: ${error}, storedResponse: ${storedResponse}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seratch Updated.

@seratch
Copy link
Member

seratch commented Dec 1, 2023

Also, perhap adding unit tests for this pattern may not be an easy task. It's okay to skip the codecov error this time

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me 👍

@seratch seratch merged commit 5587d01 into slackapi:main Dec 1, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented cla:signed enhancement M-T: A feature request for new functionality semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants