feat(slack): interactive Home tab with model selector and account linking#1418
Conversation
📝 WalkthroughWalkthroughAdds Slack user-workspace linking and per-user model preferences: DB migration and schema, signature verification, App Home UI and event handler, interactions and link routes, and propagation of stored model preferences into the Slack agent runner and event routing. Changes
Sequence DiagramssequenceDiagram
participant Slack as Slack API
participant EventSvc as App Home Handler
participant DB as Database
participant Agent as runSlackAgent
participant LinkSvc as Link Route
Slack->>EventSvc: app_home_opened event
EventSvc->>DB: Query Slack connection by teamId
DB-->>EventSvc: Connection record
EventSvc->>DB: Query users__slack_users by slackUserId+teamId
DB-->>EventSvc: Link state & modelPreference
EventSvc->>EventSvc: Build home view (initial model from preference)
EventSvc->>Slack: views.publish(home view)
Slack-->>EventSvc: views.publish response
Note over Slack,Agent: Later, assistant/mention triggers
Slack->>EventSvc: message or mention event
EventSvc->>DB: Read user modelPreference
DB-->>EventSvc: modelPreference
EventSvc->>Agent: runSlackAgent(..., model = modelPreference ?? DEFAULT_SLACK_MODEL)
Agent-->>Slack: Respond in thread
sequenceDiagram
participant User as Slack User
participant Slack as Slack API
participant LinkSvc as Link Route
participant Auth as Auth Service
participant DB as Database
User->>Slack: Click "Connect Account" (App Home)
Slack->>LinkSvc: GET /link?token=...&signature=...
LinkSvc->>LinkSvc: Verify token signature & expiry
LinkSvc->>Auth: Resolve session (redirect if unauthenticated)
Auth-->>LinkSvc: User session
LinkSvc->>DB: Find Slack connection by teamId
DB-->>LinkSvc: Connection record
LinkSvc->>DB: Verify org membership and upsert users__slack_users
DB-->>LinkSvc: Upsert result
LinkSvc-->>Slack: Redirect to linked page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
saddlepaddle
force-pushed
the
slack-home-forreal
branch
from
ff971f7 to
82eab1b
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
force-pushed
the
slack-home-forreal
branch
from
82eab1b to
15a32f8
Compare
saddlepaddle
force-pushed
the
slack-home-forreal
branch
from
15a32f8 to
b64a4c2
Compare
saddlepaddle
force-pushed
the
slack-home-forreal
branch
from
b64a4c2 to
3509968
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@apps/api/src/app/api/integrations/slack/events/process-app-home-opened/build-home-view.ts`:
- Around line 83-93: The code currently only renders the connected-account
context when both isUserLinked and userName are truthy, causing linked users
with empty names to fall into the "Connect Account" branch; update the logic in
build-home-view so that when isUserLinked is true but userName is falsy you
still render the connected-account block using a safe fallback (e.g., "your
account" or "Connected account") and keep externalOrgName appended as before;
locate the conditional around blocks.push for the context element (referencing
isUserLinked, userName, externalOrgName) and change it to check isUserLinked
first and derive displayName = userName || "your account" (or similar) before
pushing the block so linked users are never shown the Connect flow erroneously.
- Around line 119-142: The else branch may push a button with url: connectUrl
even though connectUrl is typed string | undefined; update the logic in
buildHomeView (or the function that constructs the blocks) to ensure connectUrl
is present before setting url: if isUserLinked is false guarantee connectUrl is
non-undefined by refining the function signature or adding a runtime guard:
either narrow the call site (processAppHomeOpened) to only call this branch with
a defined connectUrl, or before pushing the button assert/throw if connectUrl is
missing, or omit the url and use a fallback action_id (or disabled button) when
connectUrl is undefined so the Slack API never receives url: undefined. Ensure
the change references connectUrl and isUserLinked so the type-system and runtime
state stay consistent.
In `@apps/api/src/app/api/integrations/slack/interactions/route.ts`:
- Line 31: Wrap the JSON.parse call that builds payload from payloadRaw in a
try/catch to safely handle malformed Slack input: replace the direct const
payload = JSON.parse(payloadRaw); with parsing inside a try block and catch JSON
errors, log the error (including payloadRaw) and return a 400/400-like response
(or early exit) from the Slack interaction handler so the rest of the function
(e.g., code referencing payload, block_id, action_id) won't run on invalid
input; ensure the catch distinguishes syntax errors versus other failures and
provides a clear error message for observability.
In `@apps/api/src/app/api/integrations/slack/link/route.ts`:
- Around line 82-96: The upsert using usersSlackUsers.onConflictDoUpdate on
(slackUserId, teamId) allows one org member to overwrite another's link; before
performing the insert/update in route.ts, query usersSlackUsers for an existing
row with payload.slackUserId and payload.teamId and if a row exists and
row.userId !== session.user.id, reject the request (HTTP 409 or similar) asking
the existing user to disconnect first; if the row exists and row.userId ===
session.user.id, proceed to update organizationId only, otherwise perform the
insert. Use the unique symbols usersSlackUsers, payload.slackUserId,
payload.teamId, session.user.id and onConflictDoUpdate to locate and adjust the
logic.
- Around line 25-31: The current direct string comparison between sig and
expectedSig (created with createHmac and env.SLACK_SIGNING_SECRET) is vulnerable
to timing attacks; replace it with a timing-safe comparison like the
verifySlackSignature utility uses: convert both expectedSig and sig to Buffers
(ensuring equal length or use a constant-time fallback), then use
crypto.timingSafeEqual to compare and return 401 on mismatch. Update the block
that computes expectedSig and the subsequent check so it uses timingSafeEqual
(or call the existing verifySlackSignature) instead of !==, and handle length
differences safely before calling timingSafeEqual.
🧹 Nitpick comments (5)
apps/api/src/app/api/integrations/slack/constants.ts (1)
1-7: DeriveDEFAULT_SLACK_MODELfromSLACK_MODELSto avoid drift.The default is duplicated as a separate string literal. If
SLACK_MODELSis updated, the default could silently become stale.Proposed fix
export const SLACK_MODELS = [ { value: "claude-sonnet-4-5", label: "Sonnet 4.5" }, { value: "claude-opus-4-6", label: "Opus 4.6" }, { value: "claude-haiku-4-5", label: "Haiku 4.5" }, ] as const; -export const DEFAULT_SLACK_MODEL = "claude-sonnet-4-5"; +export const DEFAULT_SLACK_MODEL = SLACK_MODELS[0].value;apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts (1)
48-56: Consider extracting theslackUserLinklookup into a shared helper.This exact DB query (lines 48–56) is duplicated in
process-mention.ts. A small utility likegetSlackUserModelPreference({ slackUserId, teamId })would reduce duplication as more event handlers adopt model preferences.apps/api/src/app/api/integrations/slack/events/process-app-home-opened/process-app-home-opened.ts (1)
15-32: Consider extractinggenerateConnectUrlto a shared utility.This function implements the signing logic that
link/route.tsmust verify symmetrically. Co-locating the sign and verify in a shared module (or at least near each other) would make it easier to keep the two sides in sync and spot discrepancies (e.g., the timing-safe comparison issue flagged inlink/route.ts).apps/api/src/app/api/integrations/slack/interactions/route.ts (1)
42-52: Consider validatingselectedModelagainstSLACK_MODELS.Although the request is signature-verified, the
selected_option.valueis still attacker-influenceable (e.g., modified Slack client). A simple check against the known model values would prevent storing arbitrary strings inmodelPreference.♻️ Proposed validation
+import { SLACK_MODELS } from "../constants"; + +const VALID_MODELS = new Set(SLACK_MODELS.map((m) => m.value)); + if (action.action_id === "model_select") { const selectedModel = action.selected_option?.value ?? DEFAULT_SLACK_MODEL; + if (!VALID_MODELS.has(selectedModel)) { + console.warn("[slack/interactions] Invalid model selection:", selectedModel); + return; + } await handleModelSelect({ teamId, slackUserId, selectedModel }); }apps/api/src/app/api/integrations/slack/events/process-app-home-opened/build-home-view.ts (1)
173-173: Hardcoded app URL.
"https://app.superset.sh"should ideally useenv.NEXT_PUBLIC_WEB_URL(or a constant) for consistency and environment portability. As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic".
| if (isUserLinked && userName) { | ||
| blocks.push( | ||
| { | ||
| type: "context", | ||
| elements: [ | ||
| { | ||
| type: "mrkdwn", | ||
| text: `Connected as *${userName}*${externalOrgName ? ` in ${externalOrgName}` : ""}`, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Linked user with no userName silently skips the connected-account block.
When isUserLinked is true but userName is falsy (e.g., user record has an empty name), the entire account section falls through to the else branch showing "Connect Account" — even though the user is already linked. This would be confusing. Consider handling isUserLinked && !userName explicitly, or falling back to a generic label like "your account".
🤖 Prompt for AI Agents
In
`@apps/api/src/app/api/integrations/slack/events/process-app-home-opened/build-home-view.ts`
around lines 83 - 93, The code currently only renders the connected-account
context when both isUserLinked and userName are truthy, causing linked users
with empty names to fall into the "Connect Account" branch; update the logic in
build-home-view so that when isUserLinked is true but userName is falsy you
still render the connected-account block using a safe fallback (e.g., "your
account" or "Connected account") and keep externalOrgName appended as before;
locate the conditional around blocks.push for the context element (referencing
isUserLinked, userName, externalOrgName) and change it to check isUserLinked
first and derive displayName = userName || "your account" (or similar) before
pushing the block so linked users are never shown the Connect flow erroneously.
| } else { | ||
| blocks.push( | ||
| { | ||
| type: "section", | ||
| text: { | ||
| type: "mrkdwn", | ||
| text: "Link your Slack account to your Superset account to personalize your experience.", | ||
| }, | ||
| }, | ||
| { | ||
| type: "actions", | ||
| elements: [ | ||
| { | ||
| type: "button", | ||
| text: { | ||
| type: "plain_text", | ||
| text: "Connect Account", | ||
| emoji: true, | ||
| }, | ||
| url: connectUrl, | ||
| }, | ||
| ], | ||
| }, | ||
| ); |
There was a problem hiding this comment.
connectUrl may be undefined when rendered as a button URL.
When isUserLinked is false, the else branch renders a button with url: connectUrl. The type of connectUrl is string | undefined. While the caller (processAppHomeOpened) always provides it when the user is unlinked, the type system doesn't enforce this. A Slack button with url: undefined will likely be rejected by the API.
Consider tightening the types or adding a guard:
🛡️ Option A: Tighten types with a discriminated union
-interface BuildHomeViewParams {
- modelPreference?: string;
- externalOrgName?: string;
- isUserLinked: boolean;
- userName?: string;
- connectUrl?: string;
-}
+type BuildHomeViewParams = {
+ modelPreference?: string;
+ externalOrgName?: string;
+} & (
+ | { isUserLinked: true; userName: string; connectUrl?: undefined }
+ | { isUserLinked: false; userName?: undefined; connectUrl: string }
+);🤖 Prompt for AI Agents
In
`@apps/api/src/app/api/integrations/slack/events/process-app-home-opened/build-home-view.ts`
around lines 119 - 142, The else branch may push a button with url: connectUrl
even though connectUrl is typed string | undefined; update the logic in
buildHomeView (or the function that constructs the blocks) to ensure connectUrl
is present before setting url: if isUserLinked is false guarantee connectUrl is
non-undefined by refining the function signature or adding a runtime guard:
either narrow the call site (processAppHomeOpened) to only call this branch with
a defined connectUrl, or before pushing the button assert/throw if connectUrl is
missing, or omit the url and use a fallback action_id (or disabled button) when
connectUrl is undefined so the Slack API never receives url: undefined. Ensure
the change references connectUrl and isUserLinked so the type-system and runtime
state stay consistent.
| return new Response("ok", { status: 200 }); | ||
| } | ||
|
|
||
| const payload = JSON.parse(payloadRaw); |
There was a problem hiding this comment.
Wrap JSON.parse in try/catch to handle malformed payloads.
Slack payloads are external input. A malformed payload field will throw an unhandled exception, resulting in a 500. As per coding guidelines, "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks".
🛡️ Proposed fix
- const payload = JSON.parse(payloadRaw);
+ let payload: Record<string, unknown>;
+ try {
+ payload = JSON.parse(payloadRaw);
+ } catch {
+ console.error("[slack/interactions] Failed to parse payload");
+ return new Response("ok", { status: 200 });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload = JSON.parse(payloadRaw); | |
| let payload: Record<string, unknown>; | |
| try { | |
| payload = JSON.parse(payloadRaw); | |
| } catch { | |
| console.error("[slack/interactions] Failed to parse payload"); | |
| return new Response("ok", { status: 200 }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/interactions/route.ts` at line 31,
Wrap the JSON.parse call that builds payload from payloadRaw in a try/catch to
safely handle malformed Slack input: replace the direct const payload =
JSON.parse(payloadRaw); with parsing inside a try block and catch JSON errors,
log the error (including payloadRaw) and return a 400/400-like response (or
early exit) from the Slack interaction handler so the rest of the function
(e.g., code referencing payload, block_id, action_id) won't run on invalid
input; ensure the catch distinguishes syntax errors versus other failures and
provides a clear error message for observability.
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | ||
| .update(decoded) | ||
| .digest("hex"); | ||
|
|
||
| if (sig !== expectedSig) { | ||
| return new Response("Invalid signature", { status: 401 }); | ||
| } |
There was a problem hiding this comment.
Use timing-safe comparison for HMAC signature verification.
Line 29 uses !== for comparing the expected HMAC signature with the user-supplied sig. This is vulnerable to timing side-channel attacks. The extracted verifySlackSignature utility already uses timingSafeEqual for Slack webhook signatures — the same approach should be applied here.
🔒 Proposed fix
+import { createHmac, timingSafeEqual } from "node:crypto";
-import { createHmac } from "node:crypto"; const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET)
.update(decoded)
.digest("hex");
- if (sig !== expectedSig) {
+ if (
+ sig.length !== expectedSig.length ||
+ !timingSafeEqual(Buffer.from(sig), Buffer.from(expectedSig))
+ ) {
return new Response("Invalid signature", { status: 401 });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | |
| .update(decoded) | |
| .digest("hex"); | |
| if (sig !== expectedSig) { | |
| return new Response("Invalid signature", { status: 401 }); | |
| } | |
| import { createHmac, timingSafeEqual } from "node:crypto"; |
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | |
| .update(decoded) | |
| .digest("hex"); | |
| if (sig !== expectedSig) { | |
| return new Response("Invalid signature", { status: 401 }); | |
| } | |
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | |
| .update(decoded) | |
| .digest("hex"); | |
| if ( | |
| sig.length !== expectedSig.length || | |
| !timingSafeEqual(Buffer.from(sig), Buffer.from(expectedSig)) | |
| ) { | |
| return new Response("Invalid signature", { status: 401 }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/link/route.ts` around lines 25 - 31,
The current direct string comparison between sig and expectedSig (created with
createHmac and env.SLACK_SIGNING_SECRET) is vulnerable to timing attacks;
replace it with a timing-safe comparison like the verifySlackSignature utility
uses: convert both expectedSig and sig to Buffers (ensuring equal length or use
a constant-time fallback), then use crypto.timingSafeEqual to compare and return
401 on mismatch. Update the block that computes expectedSig and the subsequent
check so it uses timingSafeEqual (or call the existing verifySlackSignature)
instead of !==, and handle length differences safely before calling
timingSafeEqual.
| await db | ||
| .insert(usersSlackUsers) | ||
| .values({ | ||
| slackUserId: payload.slackUserId, | ||
| teamId: payload.teamId, | ||
| userId: session.user.id, | ||
| organizationId: connection.organizationId, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [usersSlackUsers.slackUserId, usersSlackUsers.teamId], | ||
| set: { | ||
| userId: session.user.id, | ||
| organizationId: connection.organizationId, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Upsert allows any authenticated org member to hijack another user's Slack link.
The onConflictDoUpdate on (slackUserId, teamId) will overwrite an existing mapping's userId. This means if User A has already linked their Slack account, User B (also an org member) could craft or replay a valid connect URL for User A's slackUserId and re-bind it to themselves. Consider checking whether a link already exists for a different userId and rejecting the request, or at least requiring the existing linked user to disconnect first.
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/link/route.ts` around lines 82 - 96,
The upsert using usersSlackUsers.onConflictDoUpdate on (slackUserId, teamId)
allows one org member to overwrite another's link; before performing the
insert/update in route.ts, query usersSlackUsers for an existing row with
payload.slackUserId and payload.teamId and if a row exists and row.userId !==
session.user.id, reject the request (HTTP 409 or similar) asking the existing
user to disconnect first; if the row exists and row.userId === session.user.id,
proceed to update organizationId only, otherwise perform the insert. Use the
unique symbols usersSlackUsers, payload.slackUserId, payload.teamId,
session.user.id and onConflictDoUpdate to locate and adjust the logic.
6f79d06 to
3e75a08
Compare
…king Add model dropdown, connect/disconnect account flows, and interactions endpoint to the Slack Home tab. Model preference is per-user via a new `users__slack_users` join table that maps Slack users to Superset users.
saddlepaddle
force-pushed
the
slack-home-forreal
branch
from
3e75a08 to
94a5ed1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/src/app/api/integrations/slack/manifest.json`:
- Around line 51-52: Remove the legacy "incoming-webhook" scope from the Slack
app bot scopes in the manifest
(apps/api/src/app/api/integrations/slack/manifest.json) so only modern scopes
like "chat:write" remain; locate the scopes array that currently contains
"links:write", "incoming-webhook" and delete the "incoming-webhook" entry,
leaving "links:write" and existing message-posting scopes intact, then run any
manifest/installation validation or linting to ensure the manifest is still
valid.
| "links:write", | ||
| "incoming-webhook" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if any code references incoming webhooks
rg -rn 'incoming.webhook' --type ts --type js -g '!manifest.json'
rg -rn 'webhook_url|webhook\.url' --type ts --type jsRepository: superset-sh/superset
Length of output: 46
Remove the incoming-webhook scope from bot scopes.
chat:write (already present) is the standard scope for posting messages in modern Slack apps. The incoming-webhook scope is legacy and has no usage in the codebase. Including it unnecessarily broadens the permission surface and adds an extra channel-selection step during OAuth installation that will confuse users.
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/manifest.json` around lines 51 - 52,
Remove the legacy "incoming-webhook" scope from the Slack app bot scopes in the
manifest (apps/api/src/app/api/integrations/slack/manifest.json) so only modern
scopes like "chat:write" remain; locate the scopes array that currently contains
"links:write", "incoming-webhook" and delete the "incoming-webhook" entry,
leaving "links:write" and existing message-posting scopes intact, then run any
manifest/installation validation or linting to ensure the manifest is still
valid.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/api/src/app/api/integrations/slack/interactions/route.ts`:
- Around line 42-52: The POST handler currently lets exceptions from the
payload.actions loop propagate (especially from handleModelSelect and
handleDisconnectAccount), causing 500 responses and Slack retries; wrap the
action processing in a try/catch around each action invocation (the loop over
payload.actions in the POST route), call processLogger.error or similar with
contextual info (teamId, slackUserId, action.action_id, and the error), and
ensure the route always returns a 200 to Slack even if an action fails so Slack
won’t retry — keep invoking remaining actions after logging the error to avoid
partial failures.
- Around line 44-45: The selectedModel value from the Slack action must be
validated against your known model list before persisting: replace the direct
assignment to selectedModel with logic that checks action.selected_option?.value
against the exported set/array (e.g., MODEL_OPTIONS or MODEL_IDS) from
constants.ts and only accepts it if it exists there, otherwise use
DEFAULT_SLACK_MODEL; update any code paths that persist or use selectedModel to
rely on this validated value (refer to the variable selectedModel, constant
DEFAULT_SLACK_MODEL, and the exported MODEL_OPTIONS symbol).
In `@apps/api/src/app/api/integrations/slack/link/route.ts`:
- Around line 22-33: The payload parsed from JSON must be runtime-validated with
a Zod schema before use: define a Zod schema (e.g., SlackLinkPayloadSchema) that
requires slackUserId:string, teamId:string, and exp:number, then replace the
unchecked JSON.parse result assigned to payload with
SlackLinkPayloadSchema.parse or safeParse and handle failures by returning a
400/401 response; update the code around the token decode logic (the payload
variable and its usage) to use the parsed/validated object and fail fast on
schema mismatch.
🧹 Nitpick comments (3)
packages/db/src/schema/schema.ts (1)
334-354: Consider adding anupdatedAtcolumn for audit trail consistency.Most other tables in this schema (repositories, tasks, integrationConnections, subscriptions, etc.) include an
updatedAtcolumn with$onUpdate(() => new Date()). This table supports updates (model preference changes, account re-links via upsert), so anupdatedAtwould aid debugging and maintain consistency with the rest of the schema.♻️ Suggested addition
modelPreference: text("model_preference"), createdAt: timestamp("created_at").notNull().defaultNow(), + updatedAt: timestamp("updated_at") + .notNull() + .defaultNow() + .$onUpdate(() => new Date()), },apps/api/src/app/api/integrations/slack/link/route.ts (1)
41-43: Log the error before returning a generic 400.The bare
catchdiscards useful diagnostic information (e.g., JSON parse failures, base64 decode errors). At minimum, log with context.♻️ Proposed fix
} catch (error) { + console.error("[slack/link] Invalid token:", error); return new Response("Invalid token", { status: 400 }); }As per coding guidelines, "Never swallow errors silently; at minimum log errors with context before rethrowing or handling them explicitly" and "Use prefixed console logging with consistent context pattern: [domain/operation] message".
apps/api/src/app/api/integrations/slack/interactions/route.ts (1)
34-35: Type annotations are incorrect — these values can beundefined.
payload.team?.idyieldsstring | undefined, but annotatingconst teamId: stringsilently lies to the compiler (no actual runtime cast). This works today only because of the guard on Line 37, but it masks the real type and can mislead future editors.Proposed fix
- const teamId: string = payload.team?.id; - const slackUserId: string = payload.user?.id; + const teamId: string | undefined = payload.team?.id; + const slackUserId: string | undefined = payload.user?.id;
| for (const action of payload.actions ?? []) { | ||
| if (action.action_id === "model_select") { | ||
| const selectedModel = | ||
| action.selected_option?.value ?? DEFAULT_SLACK_MODEL; | ||
| await handleModelSelect({ teamId, slackUserId, selectedModel }); | ||
| } | ||
|
|
||
| if (action.action_id === "disconnect_account") { | ||
| await handleDisconnectAccount({ teamId, slackUserId }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unhandled DB errors will cause a 500 and Slack retries.
If handleModelSelect or handleDisconnectAccount throws (e.g., a transient DB error), the exception propagates out of POST, returning a 500. Slack will then retry the interaction up to 3 times, potentially causing duplicate side-effects (especially for the delete in disconnect_account). Wrap the action processing in a try/catch, log the error, and still return 200 to Slack.
Proposed fix
for (const action of payload.actions ?? []) {
- if (action.action_id === "model_select") {
- const selectedModel =
- action.selected_option?.value ?? DEFAULT_SLACK_MODEL;
- await handleModelSelect({ teamId, slackUserId, selectedModel });
- }
-
- if (action.action_id === "disconnect_account") {
- await handleDisconnectAccount({ teamId, slackUserId });
+ try {
+ if (action.action_id === "model_select") {
+ const selectedModel =
+ action.selected_option?.value ?? DEFAULT_SLACK_MODEL;
+ await handleModelSelect({ teamId, slackUserId, selectedModel });
+ }
+
+ if (action.action_id === "disconnect_account") {
+ await handleDisconnectAccount({ teamId, slackUserId });
+ }
+ } catch (err) {
+ console.error("[slack/interactions] Action handling failed:", action.action_id, err);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const action of payload.actions ?? []) { | |
| if (action.action_id === "model_select") { | |
| const selectedModel = | |
| action.selected_option?.value ?? DEFAULT_SLACK_MODEL; | |
| await handleModelSelect({ teamId, slackUserId, selectedModel }); | |
| } | |
| if (action.action_id === "disconnect_account") { | |
| await handleDisconnectAccount({ teamId, slackUserId }); | |
| } | |
| } | |
| for (const action of payload.actions ?? []) { | |
| try { | |
| if (action.action_id === "model_select") { | |
| const selectedModel = | |
| action.selected_option?.value ?? DEFAULT_SLACK_MODEL; | |
| await handleModelSelect({ teamId, slackUserId, selectedModel }); | |
| } | |
| if (action.action_id === "disconnect_account") { | |
| await handleDisconnectAccount({ teamId, slackUserId }); | |
| } | |
| } catch (err) { | |
| console.error("[slack/interactions] Action handling failed:", action.action_id, err); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/interactions/route.ts` around lines
42 - 52, The POST handler currently lets exceptions from the payload.actions
loop propagate (especially from handleModelSelect and handleDisconnectAccount),
causing 500 responses and Slack retries; wrap the action processing in a
try/catch around each action invocation (the loop over payload.actions in the
POST route), call processLogger.error or similar with contextual info (teamId,
slackUserId, action.action_id, and the error), and ensure the route always
returns a 200 to Slack even if an action fails so Slack won’t retry — keep
invoking remaining actions after logging the error to avoid partial failures.
| const selectedModel = | ||
| action.selected_option?.value ?? DEFAULT_SLACK_MODEL; |
There was a problem hiding this comment.
selectedModel is not validated against allowed values.
Any arbitrary string from the Slack payload will be persisted to the database. Validate it against your known model options (from constants.ts) and fall back to DEFAULT_SLACK_MODEL if invalid. This is consistent with the coding guideline to validate external API data as untrusted. Based on learnings, "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks".
Proposed fix
Assuming MODEL_OPTIONS (or a similar set of valid model IDs) is exported from constants.ts:
+import { DEFAULT_SLACK_MODEL, MODEL_OPTIONS } from "../constants";
+
+const VALID_MODELS = new Set(MODEL_OPTIONS.map((m) => m.value));
+
...
- const selectedModel =
- action.selected_option?.value ?? DEFAULT_SLACK_MODEL;
+ const rawModel = action.selected_option?.value;
+ const selectedModel =
+ typeof rawModel === "string" && VALID_MODELS.has(rawModel)
+ ? rawModel
+ : DEFAULT_SLACK_MODEL;🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/interactions/route.ts` around lines
44 - 45, The selectedModel value from the Slack action must be validated against
your known model list before persisting: replace the direct assignment to
selectedModel with logic that checks action.selected_option?.value against the
exported set/array (e.g., MODEL_OPTIONS or MODEL_IDS) from constants.ts and only
accepts it if it exists there, otherwise use DEFAULT_SLACK_MODEL; update any
code paths that persist or use selectedModel to rely on this validated value
(refer to the variable selectedModel, constant DEFAULT_SLACK_MODEL, and the
exported MODEL_OPTIONS symbol).
| let payload: { slackUserId: string; teamId: string; exp: number }; | ||
| try { | ||
| const decoded = Buffer.from(token, "base64url").toString("utf-8"); | ||
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | ||
| .update(decoded) | ||
| .digest("hex"); | ||
|
|
||
| if (sig !== expectedSig) { | ||
| return new Response("Invalid signature", { status: 401 }); | ||
| } | ||
|
|
||
| payload = JSON.parse(decoded); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate the decoded payload with a Zod schema.
The payload is typed via a TS annotation but has no runtime validation after JSON.parse. If the token is tampered with (valid HMAC but unexpected shape after a secret rotation, or future schema changes), this will produce runtime errors or undefined behavior when accessing .slackUserId, .teamId, .exp.
♻️ Proposed fix
+import { z } from "zod";
+
+const linkPayloadSchema = z.object({
+ slackUserId: z.string(),
+ teamId: z.string(),
+ exp: z.number(),
+});
+
// Inside the try block:
- payload = JSON.parse(decoded);
+ const parsed = linkPayloadSchema.safeParse(JSON.parse(decoded));
+ if (!parsed.success) {
+ return new Response("Invalid token payload", { status: 400 });
+ }
+ payload = parsed.data;As per coding guidelines, "Use Zod schemas for validating tRPC inputs and API route bodies at boundaries".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let payload: { slackUserId: string; teamId: string; exp: number }; | |
| try { | |
| const decoded = Buffer.from(token, "base64url").toString("utf-8"); | |
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | |
| .update(decoded) | |
| .digest("hex"); | |
| if (sig !== expectedSig) { | |
| return new Response("Invalid signature", { status: 401 }); | |
| } | |
| payload = JSON.parse(decoded); | |
| import { z } from "zod"; | |
| // ... other imports | |
| const linkPayloadSchema = z.object({ | |
| slackUserId: z.string(), | |
| teamId: z.string(), | |
| exp: z.number(), | |
| }); | |
| // ... in the route handler: | |
| let payload: { slackUserId: string; teamId: string; exp: number }; | |
| try { | |
| const decoded = Buffer.from(token, "base64url").toString("utf-8"); | |
| const expectedSig = createHmac("sha256", env.SLACK_SIGNING_SECRET) | |
| .update(decoded) | |
| .digest("hex"); | |
| if (sig !== expectedSig) { | |
| return new Response("Invalid signature", { status: 401 }); | |
| } | |
| const parsed = linkPayloadSchema.safeParse(JSON.parse(decoded)); | |
| if (!parsed.success) { | |
| return new Response("Invalid token payload", { status: 400 }); | |
| } | |
| payload = parsed.data; |
🤖 Prompt for AI Agents
In `@apps/api/src/app/api/integrations/slack/link/route.ts` around lines 22 - 33,
The payload parsed from JSON must be runtime-validated with a Zod schema before
use: define a Zod schema (e.g., SlackLinkPayloadSchema) that requires
slackUserId:string, teamId:string, and exp:number, then replace the unchecked
JSON.parse result assigned to payload with SlackLinkPayloadSchema.parse or
safeParse and handle failures by returning a 400/401 response; update the code
around the token decode logic (the payload variable and its usage) to use the
parsed/validated object and fail fast on schema mismatch.
1 participant
Summary
users__slack_usersjoin tableblock_actionsfrom Slack (model select, disconnect)verifySlackSignature— extracted from events route, reused by interactions routerunSlackAgentacceptsmodelparam, falls back toDEFAULT_SLACK_MODELNew files
slack/constants.ts— model list + defaultslack/verify-signature.ts— shared signature verificationslack/events/process-app-home-opened/build-home-view.ts— reusable view builderslack/interactions/route.ts— interactions endpointslack/link/route.ts— account linking flowpackages/db/drizzle/0018_add_users_slack_users.sql— migrationTest plan
users__slack_usersusers__slack_users.model_preferenceSummary by CodeRabbit