feat: add CloudflareWorkerReceiver implementation and tests#2510
feat: add CloudflareWorkerReceiver implementation and tests#2510secret104278 wants to merge 1 commit into
Conversation
- Introduced a new receiver for handling Slack events in Cloudflare Workers. - Implemented signature verification and request handling logic. - Added unit tests to validate functionality and edge cases. - Updated package.json to include @cloudflare/workers-types as a dependency.
c49927e to
1d1bea4
Compare
|
ping @secret104278, any update there? I also want to use bolt-js on Cloudflare. Thank you for your contribute. |
tsushanth
left a comment
There was a problem hiding this comment.
Thanks for getting this started 8 months ago — Cloudflare Workers support is a real gap (#876). A few pieces of context worth surfacing before the code feedback, then specific issues.
Context: there's a parallel PR (#2895) that's worth comparing
@michaelgruner opened #2895 two months ago as an alternative CloudflareWorkerReceiver implementation. It also stalled (no maintainer engagement, CLA unsigned), but the two PRs have complementary strengths that are worth merging rather than treating as alternatives. Both receivers diverge in non-trivial ways — listing the deltas here so a future maintainer (or you, if you want to take another swing at this) can pick the right shape:
| Concern | #2510 (this PR) | #2895 |
|---|---|---|
ctx.waitUntil for async processing |
✅ used | ❌ blocks response on processEvent |
ExecutionContext typing on handler |
✅ @cloudflare/workers-types import |
❌ unknown |
processBeforeResponse option |
✅ supported | ❌ not exposed |
| Timing-safe HMAC compare | ✅ tsscmp |
|
| Interface name collision with AWS | ✅ scoped name | |
| Empty/null request body | ✅ defensive request.body === null |
|
| Example app | ❌ none | ✅ examples/deploy-cloudflare-workers/ |
| Test coverage | ✅ 387 LOC |
If the goal is to land one receiver that's correct in production, the right merge would take #2510's Workers-runtime integration (the ctx.waitUntil + ExecutionContext + processBeforeResponse set) and #2895's library hygiene (tsscmp + scoped interface name + null-body guard + example app). Worth flagging in the maintainer thread.
Must-fix
1. The custom timeSafeCompare has a real security/timing smell.
function timeSafeCompare(a: string | number, b: string | number) {
const sa = String(a);
const sb = String(b);
const randomBytes = new Uint8Array(32);
const key = crypto.getRandomValues(randomBytes);
const ah = crypto.createHmac('sha256', key).update(sa).digest();
const bh = crypto.createHmac('sha256', key).update(sb).digest();
return bufferEqual(ah, bh) && a === b;
}Two concerns:
- The trailing
&& a === bafter the HMAC compare defeats the timing-safe property.a === bshort-circuits character-by-character and leaks input length information. For Slack signatures specifically this isn't exploitable (signatures are fixed-lengthv0=64hex), but the pattern is wrong and will be wrong the next time someone copies it for a non-fixed-length compare. The double-HMAC compare alone (bufferEqual(ah, bh)) is already collision-resistant up to the HMAC's strength — the trailing string-equality check adds nothing except the timing leak. - A fresh
crypto.getRandomValuesper comparison is unnecessary work — the standard pattern is to use a single ephemeral key, or just skip the double-HMAC altogether and usecrypto.timingSafeEqualon equal-length buffers. The buffers from a fixed-format hex signature are equal length by construction, so the wrapper isn't even needed.
The cleanest fix is to drop timeSafeCompare entirely and use the tsscmp library the same way HTTPReceiver / ExpressReceiver / #2895 does:
import tsscmp from 'tsscmp';
// ...
if (!tsscmp(hash, computedHash)) {
return false;
}Consistent with the rest of the codebase, well-audited, removes the per-call getRandomValues overhead.
2. Interface name collides with AwsLambdaReceiver.ts:52.
export interface ReceiverInvalidRequestSignatureHandlerArgs {
rawBody: string;
signature: string;
ts: number;
request: Request;
response: Promise<Response>;
}ReceiverInvalidRequestSignatureHandlerArgs is already defined on main at src/receivers/AwsLambdaReceiver.ts:52 with the AWS-specific shape (rawBody, signature: string, ts: number, awsEvent, awsResponse). This PR re-defines and exports the same name with a Cloudflare-specific shape. Same issue is present in another in-flight PR (#2937 against HTTPReceiver/ExpressReceiver, which I just left a review on).
Two clean options:
- (a) Rename to
CloudflareWorkerReceiverInvalidRequestSignatureHandlerArgs(what #2895 does — local, not exported). Avoids the collision. - (b) Make it actually shared — match the AWS shape across receivers, with optional protocol-specific fields. This was zimeg's direction on the related #2827; if it lands there, the same shape would apply here too.
If you don't want to coordinate cross-PR, (a) is the safe choice for this PR.
3. PR is 8 months stale.
Last author activity was the initial commit on 2025-04-28. @giautm pinged 2025-11-10 asking for updates — no response. If you're still interested in landing this, a rebase + a comment on the thread responding to the comparison with #2895 would unblock it. Otherwise the maintainer will likely close it in favor of #2895 (or vice versa) at next triage.
Worth discussing
A. ctx.waitUntil(processEventPromise) usage is correct for Workers and a genuine improvement over #2895 — surface this in the PR body so a maintainer reading both diffs sees it. Right now the PR body lists the change neutrally; the ctx.waitUntil choice is the reason to prefer this PR's runtime behavior. Make it visible.
B. parseRequestBody at src/receivers/CloudflareWorkerReceiver.ts:312 doesn't guard against null body (an edge case Workers exposes when a request lacks a body — e.g. a malformed health check). #2895's getRawBody returns '' when request.body === null. Worth porting that guard.
C. No import from src/index.ts — the new CloudflareWorkerReceiver class and CloudflareWorkerReceiverOptions type aren't re-exported, so consumers can't import { CloudflareWorkerReceiver } from '@slack/bolt' after this lands. Will need to add the export alongside the others (AwsLambdaReceiver, ExpressReceiver, HTTPReceiver, SocketModeReceiver).
D. unhandledRequestTimeoutMillis = 3001 — the off-by-one default (3001 vs 3000) matches the convention in other receivers, but worth a // matches HTTPReceiver convention to bias against the exact 3s threshold comment so the next person doesn't "fix" it to 3000.
Decision
Requesting changes. The timeSafeCompare issue is the most concrete — a custom crypto comparator is the kind of thing that shouldn't exist in a project that already has tsscmp as the established pattern in HTTPReceiver. The interface collision is the same merge-blocker class I flagged on #2937. And the staleness gap (8 months, unresponsive author) means a maintainer reading this triage today is going to have to make a call between the two PRs — making the comparison framing explicit gives them something to work with.
If you do come back to this, the path is probably: rename the interface, swap to tsscmp, add src/index.ts exports, port the null-body guard from #2895, and respond to the maintainer thread referencing the comparison. Happy to re-review then.
|
Hey there, saw this PR a little too late. I like the |
|
@michaelgruner thanks for jumping in — that's the outcome the comparison table was hoping for. The two PRs really do have complementary strengths and merging them is the right move. To be concrete about the deltas (in case it helps either of you decide which PR becomes the canonical home): From this PR (#2510) worth preserving:
From #2895 worth preserving:
The fastest path probably looks like: #2895 becomes the canonical receiver (it already has the example app + tighter library hygiene), and the cc @WilliamBergamin / @zimeg — does the coordinate-on-#2895-and-close-#2510 path work from the maintainer side, or would you rather keep both open until one author rebases into the other? (I don't have skin in either branch — left a review here yesterday on engineering grounds, and the response from @michaelgruner is the productive outcome. Happy to do another pass on whichever PR ends up being the canonical version once the merge lands.) |
Summary
Address this issue #876
Requirements