Conversation
… URLs from inventing credentials
Three defects in the same area, all found while answering whether the documented
environment variables were complete.
The Slack variables in .env.example were never read. SLACK_CLIENT_ID,
SLACK_CLIENT_SECRET, SLACK_BOT_TOKEN and SLACK_SIGNING_SECRET were documented as
the Slack configuration, but the only one any code touched was SLACK_CLIENT_ID,
read in the OAuth URL handler. The bot token and signing secret live on the
channel, so setting the documented variables did nothing at all. They are real
bindings now, following the same pattern Discord already had: a SlackConfig
section with deployment-wide defaults, and a channel credential that wins over
it. That matches how the Meta credentials resolve, so a deployment with one Slack
app has nothing to change and a deployment with several workspaces can still use
per-channel credentials.
Four OAuth authorization URLs fabricated a client identifier when none was
configured. Discord answered with "123456789012345678", Slack with
"123456789012.1234567890123", X with "x_oauth_client_id_placeholder" and TikTok
with "tiktok_client_key_placeholder". Each sent the operator to the provider's
own error page, which reads as an application bug, and the real cause was never
visible. They now answer which variable to set instead.
Slack webhook verification also hardened, matching the behaviour already
contributed upstream: a delivery with no signature headers is rejected rather
than waved through when a signing secret resolves for the channel, and request
timestamps outside a five minute window are rejected. Slack signs the timestamp
and the body but nothing in the signature expires, so without the check a
captured request replays indefinitely. A channel with no signing secret still
accepts deliveries, so this is not breaking for an existing installation.
Slack's OAuth install flow is implemented: POST
/api/dashboard/channel/slack_oauth_callback exchanges an installation code
through oauth.v2.access, verifies the resulting token with auth.test, reports the
workspace identity and the preselected default channel, and saves everything onto
the target channel while preserving its signing secret. The token type and the
chat:write scope are checked, because Slack does not treat a user token or a
missing scope as an install error and either would leave a bot that cannot reply.
Masking of channel credentials is shared between the WhatsApp and Slack flows
rather than duplicated.
Tests
internal/slack/client_test.go
oauth.v2.access is form-encoded, ok:false surfaces as an error rather than
being treated as success, a blank redirect_uri is omitted, auth.test failure
surfaces, threaded replies carry thread_ts and a top-level post omits it
internal/services/slack_oauth_service_test.go
connect persists the workspace identity and the bot token while preserving
the existing signing secret; missing client credentials is an error; a
Slack-rejected code carries its reason through; a user token and a missing
chat:write scope are warnings rather than silent installs; auth.test
failure does not discard a token Slack issued; non-Slack channels rejected
internal/services/slack_inbound_service_test.go
an unsigned delivery is rejected and stores nothing, a replayed timestamp
is rejected at ten minutes, one hour and ten minutes in the future, and a
fresh or four-minute-old signature is still accepted
internal/pkg/config
the resolvers return channel-level values when no configuration is loaded
.env.example gains sections for X and TikTok, whose client identifiers are read
by the OAuth URL handlers and were undocumented, and notes that LINE and Viber
store everything per channel.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7a53bf5b-3347-4acb-a39b-742aaa1a30c8) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Slack OAuth 2.0 integration, enabling the exchange of installation codes for workspace bot credentials, and adds deployment-wide Slack configuration support with channel-specific overrides. It also enhances webhook security by verifying Slack signatures with a five-minute timestamp tolerance to prevent replay attacks. Feedback on the changes highlights an inconsistency where the Connect callback service lacks the environment variable fallback for SLACK_CLIENT_ID and SLACK_CLIENT_SECRET present in the authorization URL handler, recommending that this fallback be added along with the necessary "os" import.
| app := config.ResolveSlack("", "") | ||
| if app.ClientID == "" || app.ClientSecret == "" { | ||
| return nil, errorsx.InvalidParamI18n("error.slack.oauth.clientCredentialsMissing") | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), slackOAuthTimeout) | ||
| defer cancel() | ||
|
|
||
| exchange, err := slackExchangeOAuthCode(ctx, slackOAuthBaseURL, app.ClientID, app.ClientSecret, code, req.RedirectURI) |
There was a problem hiding this comment.
The OAuth URL handler (ChannelGetSlackOAuthURL) falls back to reading SLACK_CLIENT_ID from the environment via os.Getenv if it is not resolved from the configuration. However, the Connect callback service does not implement this fallback, which can lead to a situation where the authorization flow is initiated successfully but the callback fails with a clientCredentialsMissing error.
We should add the same fallback to os.Getenv for both SLACK_CLIENT_ID and SLACK_CLIENT_SECRET in the Connect method to ensure consistent behavior.
app := config.ResolveSlack("", "")
clientID := app.ClientID
if clientID == "" {
clientID = strings.TrimSpace(os.Getenv("SLACK_CLIENT_ID"))
}
clientSecret := app.ClientSecret
if clientSecret == "" {
clientSecret = strings.TrimSpace(os.Getenv("SLACK_CLIENT_SECRET"))
}
if clientID == "" || clientSecret == "" {
return nil, errorsx.InvalidParamI18n("error.slack.oauth.clientCredentialsMissing")
}
ctx, cancel := context.WithTimeout(context.Background(), slackOAuthTimeout)
defer cancel()
exchange, err := slackExchangeOAuthCode(ctx, slackOAuthBaseURL, clientID, clientSecret, code, req.RedirectURI)| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "log/slog" | ||
| "strings" | ||
| "time" |
There was a problem hiding this comment.
Add the "os" package to the imports so we can access environment variables using os.Getenv.
| import ( | |
| "context" | |
| "encoding/json" | |
| "log/slog" | |
| "strings" | |
| "time" | |
| import ( | |
| "context" | |
| "encoding/json" | |
| "log/slog" | |
| "os" | |
| "strings" | |
| "time" |
1 participant
What this fixes
Three defects in the same area, all found while answering whether the documented environment variables were complete.
1. The Slack variables in
.env.examplewere never readSLACK_CLIENT_ID,SLACK_CLIENT_SECRET,SLACK_BOT_TOKENandSLACK_SIGNING_SECRETwere documented as the Slack configuration, but the only one any code touched wasSLACK_CLIENT_ID, read in the OAuth URL handler. The bot token and signing secret live on the channel, so setting the documented variables did nothing at all.They are real bindings now, following the pattern Discord already had: a
SlackConfigsection with deployment-wide defaults, and a channel credential that wins over it. That matches how the Meta credentials resolve (#8), so a deployment with one Slack app has nothing to change and a deployment with several workspaces can still use per-channel credentials.2. Four OAuth authorization URLs fabricated a client identifier
123456789012345678123456789012.1234567890123x_oauth_client_id_placeholdertiktok_client_key_placeholderEach sent the operator to the provider's own error page, which reads as an application bug, and the real cause was never visible. They now answer which variable to set instead.
3. Slack webhook verification hardened
Matches the behaviour already contributed upstream in huabeitech#42: a delivery with no signature headers is rejected rather than waved through when a signing secret resolves for the channel, and request timestamps outside a five minute window are rejected. Slack signs the timestamp and the body but nothing in the signature expires, so without the check a captured request replays indefinitely.
A channel with no signing secret still accepts deliveries, so this is not breaking for an existing installation.
Slack OAuth install flow
POST /api/dashboard/channel/slack_oauth_callbackexchanges an installation code throughoauth.v2.access, verifies the resulting token withauth.test, reports the workspace identity and the preselected default channel, and saves everything onto the target channel while preserving its signing secret.The token type and the
chat:writescope are checked, because Slack does not treat a user token or a missing scope as an install error and either would leave a bot that cannot reply.Tests
internal/slack/client_test.gooauth.v2.accessis form-encoded;ok:falsesurfaces as an error rather than being treated as success; blankredirect_uriomitted;auth.testfailure surfaces; threaded replies carrythread_ts; a top-level post omits itinternal/services/slack_oauth_service_test.gochat:writeare warnings rather than silent installs;auth.testfailure does not discard a token Slack issued; non-Slack channels rejectedinternal/services/slack_inbound_service_test.gointernal/pkg/configVerification
go build ./...,go veton every changed package: cleango test ./internal/services/ ./internal/slack/ ./internal/pkg/config/ ./internal/handlers/dashboard/: all green.env.examplegains X and TikTok sections, whose client identifiers are read by the OAuth URL handlers and were undocumented; notes that LINE and Viber store everything per channelen-USandzh-CNNot included
The Slack frontend install button and landing page. The backend endpoint and route exist; wiring the popup flow needs the WhatsApp callback page generalized first, and is left as the next step.
web/messages/vi-VN.jsonis deliberately untouched here — it belongs to #16.Note
Medium Risk
Changes channel OAuth error behavior, adds Slack token exchange and persistence, and tightens webhook signature checks when a signing secret is configured—security-sensitive but scoped and covered by tests.
Overview
Makes documented Slack deployment variables (
SLACK_CLIENT_ID,SLACK_CLIENT_SECRET,SLACK_BOT_TOKEN,SLACK_SIGNING_SECRET) load throughSlackConfigandResolveSlack, with per-channel bot token and signing secret overriding deployment defaults for inbound, outbound, and OAuth URL generation.Adds
POST /slack_oauth_callbackandSlackOAuthService.Connectto exchange an install code viaoauth.v2.access, optionally verify withauth.test, return workspace metadata for the channel form, and persist credentials on an existing channel while keeping the existing signing secret.Discord, Slack, X, and TikTok OAuth URL handlers no longer substitute placeholder client IDs; they return a clear i18n error naming the env var and require
redirect_uri..env.exampledocuments Slack, X, TikTok, and notes LINE/Viber are channel-only.Slack Events API verification is tightened when a signing secret resolves: missing signature headers are rejected, timestamps outside five minutes are rejected, and outbound sending can fall back to
SLACK_BOT_TOKENwhen the channel has no token.Reviewed by Cursor Bugbot for commit c25c573. Configure here.