Conversation
…ent JSON - Add App.RegisterDispatcher(Dispatcher) for post-construction dispatcher wiring so services that need the trigger app at wire-up time can register themselves without a constructor cycle. No-op when dispatcher is nil. - Short-circuit the Slack url_verification handshake in AuthenticateWebhook so Slack's challenge can be echoed before a signing secret is necessarily configured (HandleWebhook already responds with the challenge). - Drop the thread_ts -> ts fallback in the Slack definition builder. Top-level channel/DM messages now correlate on the channel alone so a user's standalone messages merge into one Gram thread instead of spawning one per message; replies continue to correlate on thread_ts. - Add Task.EventJSON []byte populated from json.Marshal(envelope.Event) so downstream activities can route on structured event data, plus expose bot_id / app_id on slackEventRequestBody and slackTriggerEvent (with CEL tags on the latter) so the CEL filter surface can see them. ✻ Clauded...
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
🦋 Changeset detectedLatest commit: 2444058 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Slack's URL verification handshake must echo the challenge before | ||
| // any signing secret has necessarily been configured. Allow it | ||
| // through auth; HandleWebhook will respond with the challenge. | ||
| var probe slackEventRequest | ||
| if err := json.Unmarshal(body, &probe); err == nil && probe.Type == "url_verification" && probe.Challenge != "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🚩 url_verification auth bypass: comment premise doesn't hold, deviates from existing pattern
The new AuthenticateWebhook bypass at server/internal/background/triggers/definitions.go:314-317 skips Slack signature verification for url_verification requests, justified by the comment "before any signing secret has necessarily been configured." However, ProcessWebhook calls validateInstance at server/internal/background/triggers/app.go:361 before AuthenticateWebhook at line 374. Since the Slack definition declares SLACK_SIGNING_SECRET as Required: true (definitions.go:289), validateInstance enforces the secret exists in the environment (app.go:570-573). By the time AuthenticateWebhook runs, the secret IS necessarily configured — the comment's premise is incorrect.
This also deviates from the existing pattern in server/internal/thirdparty/slack/impl.go:226 where validateSlackEvent is called before the url_verification check at line 236, meaning signatures are always verified.
The practical impact is very low: HandleWebhook returns early for url_verification with Event: nil (no events created, no triggers dispatched). However, the bypass (1) allows unauthenticated confirmation that a trigger endpoint exists, and (2) masks signing-secret misconfiguration during Slack setup — the URL verification succeeds even if the secret value is wrong, but subsequent event deliveries will fail.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
this is incorrect: Slack probes this endpoint before it allows webhook registration, the endpoint needs to respond with 200, otherwise Slack won't allow adding the endpoint
| if dispatcher == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
why is this possible? we shouldn't have this type of logic in littering methods left and right.
| var probe slackEventRequest | ||
| if err := json.Unmarshal(body, &probe); err == nil && probe.Type == "url_verification" && probe.Challenge != "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🔴 Slack webhook HMAC signature verification bypassed for url_verification requests even when signing secret is available
The new short-circuit in AuthenticateWebhook at definitions.go:315-316 skips HMAC signature verification for any request where type == "url_verification" and challenge != "". The comment claims this is needed "before any signing secret has necessarily been configured", but that scenario is impossible in practice: ProcessWebhook calls validateInstance at server/internal/background/triggers/app.go:361 before reaching AuthenticateWebhook, and validateInstance already enforces that SLACK_SIGNING_SECRET exists (it's marked Required: true in the Slack definition's EnvRequirements at definitions.go:289). If the secret is missing, validateInstance returns an error and AuthenticateWebhook is never called.
Therefore, this bypass only takes effect when the signing secret is available — meaning it skips a legitimate HMAC check. An attacker can send {"type":"url_verification","challenge":"probe"} to any Slack trigger webhook endpoint without a valid Slack signature and receive a 200 OK response, allowing them to probe for valid trigger instance UUIDs without authentication. While no events are dispatched (HandleWebhook returns early with just the challenge echo), this is an unnecessary weakening of the authentication layer.
Prompt for agents
The url_verification bypass in AuthenticateWebhook skips HMAC signature verification even when the signing secret is available. The comment says this is for when the secret hasn't been configured yet, but validateInstance (called earlier in ProcessWebhook at app.go:361) already enforces that SLACK_SIGNING_SECRET exists before AuthenticateWebhook is ever reached.
Two possible fixes:
1. Remove the bypass entirely from AuthenticateWebhook and always verify the HMAC signature, even for url_verification requests. Slack sends a valid signature with the url_verification request, so this should work.
2. If there's a legitimate need to handle url_verification before the environment is fully configured, the short-circuit should be moved to HandleWebhook in the HTTP handler layer (server/internal/triggers/impl.go HandleWebhook method) BEFORE authentication is attempted, or the bypass should only activate when the signing secret is actually empty (i.e. check signingSecret == "" first, then allow url_verification through).
Option 1 is the simplest and most secure approach.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
App.RegisterDispatcher(Dispatcher)— register dispatchers post-construction (no-op on nil). Unblocks services that need the trigger app at wire-up time without a constructor cycle.AuthenticateWebhookshort-circuits theurl_verificationhandshake so the challenge can be echoed before a signing secret is necessarily configured.thread_ts → tsfallback for Slack correlation. Top-level channel/DM messages now correlate on the channel alone, so a user's standalone messages land in one Gram thread instead of spawning one per message.Task.EventJSON []byte(populated fromjson.Marshal(envelope.Event)), plusBotID/AppIDfields on bothslackEventRequestBodyandslackTriggerEvent(CEL tags on the latter).All four are pure additions — no removals or renames.
Linear: https://linear.app/speakeasy/issue/AGE-1897/feattriggers-dispatcher-registration-slack-url-verification-event-json
✻ Clauded...