ack in ExpressReceiver ending response twice

[jarrodldavis]

Description

The ack method configured on events emitted by ExpressReceiver attempts to end the request twice for an empty response value.

What type of issue is this? (place an x in one of the [ ])

• bug
• enhancement (feature request)
• question
• documentation related
• testing related
• discussion

Requirements (place an x in each of the [ ])

• I've read and understood the Contributing guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Reproducible in:

package version: 1.4.1

node version: 12.12.0

OS version(s): macOS Catalina, Version 10.15.1 (19B88)

Steps to reproduce:

1. Setup a bolt app according to the getting started guide
2. Send a message to the app (e.g. a direct message)

Expected result:

The bot receives the messages, responds with an acknowledgement without error, and proceeds with the logic of the App instance (e.g. using say).

Actual result:

With an unaltered Express response object, the expected result occurs, but this is only because Express (currently) checks for an existing Content-Type header in its Request#json method.

Attachments:

I've tracked down the issue (and why it currently doesn't blow up on unaltered Express response objects) to this code path:

ExpressReceiver emits a message event in its requestHandler:

https://github.com/slackapi/bolt/blob/d2db40b85e80b95b83cf28dc30f63dd4d0e17b47/src/ExpressReceiver.ts#L72-L112

That comes through to App#onIncomingMessage, where it immediately acknowledges incoming Events API requests:

https://github.com/slackapi/bolt/blob/d2db40b85e80b95b83cf28dc30f63dd4d0e17b47/src/App.ts#L465-L471

ack is defined here:

https://github.com/slackapi/bolt/blob/d2db40b85e80b95b83cf28dc30f63dd4d0e17b47/src/ExpressReceiver.ts#L85-L98

For a falsy (e.g. undefined) response value, it first calls res#send, followed by res#json.

res#send, of course, sets appropriate response headers along with the response body and ends it:

https://github.com/expressjs/express/blob/e1b45ebd050b6f06aa38cda5aaf0c21708b0c71e/lib/response.js#L107-L225

At this point, no additional headers can be set on the response.

Similarly, res#json sets a JSON response along with response headers, and ends the response via res#send.

https://github.com/expressjs/express/blob/e1b45ebd050b6f06aa38cda5aaf0c21708b0c71e/lib/response.js#L239-L268

Thankfully, res#json in the current (v4.17.1) version of Express checks for an existing Content-Type header and doesn't set any additional headers, in part because the call to res#send has undefined as the argument.

So while this currently doesn't pose an issue, any change to how Express handles different values in regards to setting appropriate headers may cause surprising "headers can't be set after being sent to the client" errors in the future.

I believe the fix is to change the ack method:

diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts
index 1b036d6..9b52346 100644
--- a/src/ExpressReceiver.ts
+++ b/src/ExpressReceiver.ts
@@ -88,8 +88,9 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
           clearTimeout(timer);
           timer = undefined;
 
-          if (!response) res.send('');
-          if (typeof response === 'string') {
+          if (!response) {
+            res.send('');
+          } else if (typeof response === 'string') {
             res.send(response);
           } else {
             res.json(response);

But I'm not sure how to write a test for this since there currently isn't a failing test case outside of standing up a non-vanilla Express environment.

Of note, I found this issue due to using Bolt in an API route of Next.js, which modifies response objects to have custom send and json functions that aren't as judicious in their setting of response headers.

[shaydewael]

@jarrodldavis Good catch, I think you're right about changing it to a if/else if/else statement to future proof it against Express changes (and the logic should be correct anyway 😅)