Skip to content

feat: add CloudflareWorkerReceiver implementation and tests#2510

Draft
secret104278 wants to merge 1 commit into
slackapi:mainfrom
secret104278:feat/cloudflare-worker-receiver
Draft

feat: add CloudflareWorkerReceiver implementation and tests#2510
secret104278 wants to merge 1 commit into
slackapi:mainfrom
secret104278:feat/cloudflare-worker-receiver

Conversation

@secret104278

@secret104278 secret104278 commented Apr 28, 2025

Copy link
Copy Markdown

Summary

Address this issue #876

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

Requirements

- 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.
@secret104278
secret104278 force-pushed the feat/cloudflare-worker-receiver branch from c49927e to 1d1bea4 Compare April 29, 2025 01:42
@giautm

giautm commented Nov 10, 2025

Copy link
Copy Markdown

ping @secret104278, any update there? I also want to use bolt-js on Cloudflare. Thank you for your contribute.

@tsushanth tsushanth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 ⚠️ custom (see below) tsscmp
Interface name collision with AWS ⚠️ collides ✅ scoped name
Empty/null request body ⚠️ unchecked ✅ defensive request.body === null
Example app ❌ none examples/deploy-cloudflare-workers/
Test coverage ✅ 387 LOC ⚠️ 287 LOC + type-test

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 === b after the HMAC compare defeats the timing-safe property. a === b short-circuits character-by-character and leaks input length information. For Slack signatures specifically this isn't exploitable (signatures are fixed-length v0=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.getRandomValues per comparison is unnecessary work — the standard pattern is to use a single ephemeral key, or just skip the double-HMAC altogether and use crypto.timingSafeEqual on 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.

@michaelgruner

Copy link
Copy Markdown

Hey there, saw this PR a little too late. I like the ctx.waitUntil implementation here, and actually think it going to be needed for many (slow) implementations. Happy to port the example here, or port the async stuff over there, whatever is best. Mine is almost a literal copy of the aws lambda implementation they already have.

@tsushanth

Copy link
Copy Markdown
Contributor

@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:

  • ctx.waitUntil(processEventPromise) + the Promise.race([processEventPromise, ackPromise]) pattern. This is the most important runtime-correctness piece — without it, slow processEvent paths either block the response past Slack's 3s ack window or get cut off when the Worker exits. The AwsLambdaReceiver await processEvent model doesn't translate to Workers; you really do need the waitUntil handoff.
  • processBeforeResponse option for users who want to opt out of the async behavior (e.g. processing must complete before ack).
  • @cloudflare/workers-types dep + ExecutionContext typing on the handler signature — the unknown types in Add CloudFlare Workers receiver #2895 leave users without IDE support for the ctx parameter.

From #2895 worth preserving:

  • tsscmp instead of the custom timeSafeCompare. Less code, well-audited, consistent with the other receivers in the codebase.
  • Scoped interface name (CloudflareWorkerReceiverInvalidRequestSignatureHandlerArgs local, not exported) — avoids the AwsLambdaReceiver name collision I flagged.
  • getRawBody with the request.body === null defensive guard.
  • The full examples/deploy-cloudflare-workers/ directory with the AGENTS.md/manifest/wrangler.jsonc set — substantially lowers the adoption cost for new users.

The fastest path probably looks like: #2895 becomes the canonical receiver (it already has the example app + tighter library hygiene), and the ctx.waitUntil + ExecutionContext + processBeforeResponse changes get cherry-picked over from this PR. @secret104278's tests (387 LOC vs #2895's 287) are also worth porting selectively — particularly anything covering the waitUntil path that #2895's spec wouldn't have exercised.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants