fix(api): use linked Slack user identity for MCP auth#1447
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds subscription and account-link gating to Slack event handlers, extracts connect-URL generation to a new utility, passes caller-supplied userId and modelPreference into the Slack agent runtime, removes a DB connection lookup from run-agent, and updates billing page success/layout. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Slack User
participant Handler as Event Handler
participant DB as Database
participant Utils as URL Utility
participant Agent as Slack Agent
participant Slack as Slack API
User->>Handler: send mention / DM / app_home_opened
Handler->>DB: fetch active subscription & slackUserLink (concurrent)
DB-->>Handler: return subscription, slackUserLink
alt No Active Subscription
Handler->>Slack: post Pro-plan upgrade prompt
Slack-->>User: show upgrade prompt
else No Slack Link
Handler->>Utils: generateConnectUrl(slackUserId, teamId)
Utils-->>Handler: connectUrl
Handler->>Slack: post connect-account prompt with connectUrl
Slack-->>User: show connect prompt
else Subscription + Link present
Handler->>Agent: invoke runSlackAgent(params including userId & modelPreference)
Agent->>Slack: perform actions / post messages
Slack-->>User: deliver agent response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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-use-auth
branch
from
ef9e2a4 to
a77e978
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts`:
- Around line 101-106: Add a guard to ensure event.user is defined before
calling generateConnectUrl: if event.user is undefined, return early (same
pattern used in the process-mention flow). Locate the block that checks
slackUserLink and before calling generateConnectUrl({ slackUserId: event.user,
teamId }) add "if (!event.user) return;" so you never pass an undefined
slackUserId to generateConnectUrl or include "undefined" in the signed payload;
keep the existing slack.chat.postMessage usage unchanged.
In `@apps/web/src/app/`(dashboard)/settings/billing/page.tsx:
- Around line 36-41: Replace the plain anchor using the custom scheme with logic
that handles the missing desktop app: create a handler (e.g., handleOpenDesktop)
and change the Button/anchor usage (Button asChild + <a href="superset://...">)
to call that handler; in the handler attempt to open the superset:// URL (via
window.location or an iframe) and after a short timeout redirect or show a
prompt to the desktop download page if the scheme wasn't handled; additionally
render a small fallback note component under the Button (e.g., "Don't have the
app? Download it below.") and ensure the ExternalLink icon remains for visual
parity.
🧹 Nitpick comments (4)
apps/web/src/app/(dashboard)/settings/billing/page.tsx (1)
36-47: Download link only targets Mac ARM64; consider broader platform support.The "Download for Mac" button exclusively links to the ARM64
.dmg. Users on Intel Macs, Windows, or Linux have no download option. If this is intentional for now, a brief comment or a TODO would help clarify intent. If not, consider detecting the platform or linking to a general releases page.Also, the download
<a>should includetarget="_blank" rel="noopener noreferrer"so the current page isn't navigated away during download, and to follow secure linking best practices.Suggested fix for the download link
<Button variant="outline" size="lg" className="gap-2" asChild> - <a href={DOWNLOAD_URL_MAC_ARM64}> + <a href={DOWNLOAD_URL_MAC_ARM64} target="_blank" rel="noopener noreferrer"> Download for Mac <Download className="size-4" /> </a> </Button>apps/api/src/app/api/integrations/slack/events/utils/generate-connect-url/generate-connect-url.ts (2)
14-14: Extract the expiry duration to a named constant.The
10 * 60 * 1000magic number should be a named constant for clarity and single-point-of-change if the TTL needs adjustment.Proposed fix
+const CONNECT_URL_TTL_MS = 10 * 60 * 1000; // 10 minutes + export function generateConnectUrl({ slackUserId, teamId, }: { slackUserId: string; teamId: string; }): string { const payload = JSON.stringify({ slackUserId, teamId, - exp: Date.now() + 10 * 60 * 1000, + exp: Date.now() + CONNECT_URL_TTL_MS, });As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic".
16-18: Both link verification and signature generation are properly aligned and working correctly.The link verification endpoint (
link/route.ts) and the connect URL generation (generate-connect-url.ts) both useSLACK_SIGNING_SECRETwith the same HMAC-SHA256 algorithm to sign and validate the token payload. The signatures are correctly matched and validated.Note:
SLACK_SIGNING_SECRETis reused for two distinct purposes—verifying incoming Slack webhooks (verify-signature.tsusing thev0:timestamp:bodyformat) and signing internal connect URL tokens (using a plain JSON payload). While these use different payload formats which prevents signature confusion, the coupling of these two security domains to the same key material remains a design choice. As originally suggested, if the application grows significantly, consider dedicating a separate key for internal token signing to reduce blast radius if either security domain is compromised.apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts (1)
72-136: Duplicated Block Kit messages acrossprocess-assistant-messageandprocess-mention.The "Upgrade to Pro" and "Connect Account" Block Kit payloads are nearly identical in both files. Consider extracting them into shared helper functions in the
../utils/slack-blocksmodule to reduce duplication and ensure consistency.
| <Button size="lg" className="gap-2" asChild> | ||
| <a href="superset://settings/billing"> | ||
| Open in Desktop App | ||
| <ExternalLink className="size-4" /> | ||
| </a> | ||
| </Button> |
There was a problem hiding this comment.
Custom URL scheme superset:// will fail ungracefully if the desktop app is not installed.
When the desktop app isn't installed, clicking "Open in Desktop App" will either show a browser error or do nothing depending on the browser. Consider adding a fallback — e.g., a small note below the button ("Don't have the app? Download it below.") or using a mechanism that detects whether the scheme was handled.
🤖 Prompt for AI Agents
In `@apps/web/src/app/`(dashboard)/settings/billing/page.tsx around lines 36 - 41,
Replace the plain anchor using the custom scheme with logic that handles the
missing desktop app: create a handler (e.g., handleOpenDesktop) and change the
Button/anchor usage (Button asChild + <a href="superset://...">) to call that
handler; in the handler attempt to open the superset:// URL (via window.location
or an iframe) and after a short timeout redirect or show a prompt to the desktop
download page if the scheme wasn't handled; additionally render a small fallback
note component under the Button (e.g., "Don't have the app? Download it below.")
and ensure the ExternalLink icon remains for visual parity.
a77e978 to
601ea57
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/`(dashboard)/settings/billing/page.tsx:
- Around line 49-54: The "Download for Mac" Button currently links only to
DOWNLOAD_URL_MAC_ARM64 which serves an Apple Silicon (ARM64) binary; update the
UI to avoid misleading users by either implementing client-side platform
detection (use navigator.userAgent or navigator.platform in the component that
renders Button) to choose between DOWNLOAD_URL_MAC_ARM64 and a
DOWNLOAD_URL_MAC_INTEL (or other platform URLs) and render the appropriate link,
or at minimum change the label text to "Download for Mac (Apple Silicon)" and/or
add a secondary dropdown/menu offering other platform links; locate the Button
usage in page.tsx and the DOWNLOAD_URL_MAC_ARM64 symbol to apply the fix.
🧹 Nitpick comments (3)
apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts (2)
72-99: Consider extracting the billing URL to a shared constant.The URL
"https://app.superset.sh/settings/billing"is hardcoded here and likely duplicated inprocess-mention.ts. Extracting it to a named constant (e.g., in a sharedconstantsfile alongsidegenerateConnectUrl) would keep these in sync if the URL ever changes.
101-102: Silent early return whenevent.useris undefined.If
event.userisundefinedand we reach this point (subscription is active, but no link can be resolved), we exit silently. Adding a brief log here would help with debugging unexpected Slack events that lack a user field.Proposed improvement
if (!slackUserLink) { - if (!event.user) return; + if (!event.user) { + console.warn("[slack/process-assistant-message] event.user is undefined, skipping connect prompt"); + return; + } const connectUrl = generateConnectUrl({apps/api/src/app/api/integrations/slack/events/process-mention/process-mention.ts (1)
88-93: Hardcoded billing URL — consider using a shared constant or env variable.Line 91 uses a hardcoded
"https://app.superset.sh/settings/billing"while other URL construction in this PR (e.g.,generateConnectUrl) usesenv.NEXT_PUBLIC_API_URL. If the app domain varies by environment (staging, self-hosted), this URL will point to the wrong place.
| <Button variant="outline" size="lg" className="gap-2" asChild> | ||
| <a href={DOWNLOAD_URL_MAC_ARM64}> | ||
| Download for Mac | ||
| <Download className="size-4" /> | ||
| </a> | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'DOWNLOAD_URL' --type=ts --type=tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
cd apps/web && wc -l src/app/\(dashboard\)/settings/billing/page.tsxRepository: superset-sh/superset
Length of output: 112
🏁 Script executed:
cd apps/web && sed -n '1,70p' src/app/\(dashboard\)/settings/billing/page.tsxRepository: superset-sh/superset
Length of output: 1761
🏁 Script executed:
fd -t f constants -x grep -l DOWNLOAD_URL {}Repository: superset-sh/superset
Length of output: 98
🏁 Script executed:
rg -n 'DOWNLOAD_URL' --type tsRepository: superset-sh/superset
Length of output: 1765
🏁 Script executed:
cat -n packages/shared/src/constants.ts | head -50Repository: superset-sh/superset
Length of output: 2103
🏁 Script executed:
cd apps/web && grep -r "DOWNLOAD_URL\|platform\|Windows\|Intel" --type=ts --type=tsx src/ | head -20Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
rg 'Superset.*dmg|Superset.*exe|Superset.*appimage|Intel|x64' packages/shared/src/ apps/Repository: superset-sh/superset
Length of output: 50378
🏁 Script executed:
cat README.md | head -100Repository: superset-sh/superset
Length of output: 3633
🏁 Script executed:
find . -name "RELEASE*" -o -name "DOWNLOADS*" -o -name ".github" -type d | head -5Repository: superset-sh/superset
Length of output: 122
"Download for Mac" button only offers Apple Silicon (ARM64) binary — other platforms are excluded.
The button label "Download for Mac" links exclusively to the ARM64 .dmg. Users on Intel Macs will download an incompatible binary, and Windows/Linux users see a Mac-only option with no alternative. Consider either:
- Detecting the platform (via user-agent or a client-side check) and offering the appropriate binary, or
- At minimum, labeling the button more precisely (e.g., "Download for Mac (Apple Silicon)") to set correct expectations.
🤖 Prompt for AI Agents
In `@apps/web/src/app/`(dashboard)/settings/billing/page.tsx around lines 49 - 54,
The "Download for Mac" Button currently links only to DOWNLOAD_URL_MAC_ARM64
which serves an Apple Silicon (ARM64) binary; update the UI to avoid misleading
users by either implementing client-side platform detection (use
navigator.userAgent or navigator.platform in the component that renders Button)
to choose between DOWNLOAD_URL_MAC_ARM64 and a DOWNLOAD_URL_MAC_INTEL (or other
platform URLs) and render the appropriate link, or at minimum change the label
text to "Download for Mac (Apple Silicon)" and/or add a secondary dropdown/menu
offering other platform links; locate the Button usage in page.tsx and the
DOWNLOAD_URL_MAC_ARM64 symbol to apply the fix.
601ea57 to
e1c35a6
Compare
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/events/process-assistant-message/process-assistant-message.ts`:
- Around line 63-67: The subscription lookup currently only checks
eq(subscriptions.status, "active") which excludes trial users; update the where
clause used in db.query.subscriptions.findFirst (in
process-assistant-message.ts) to accept both "active" and "trialing" statuses by
replacing the single eq check with an or(eq(subscriptions.status, "active"),
eq(subscriptions.status, "trialing")) (and make the same change in the analogous
query in process-mention.ts) so both active and trialing subscriptions are
treated as valid.
In
`@apps/api/src/app/api/integrations/slack/events/utils/generate-connect-url/generate-connect-url.ts`:
- Around line 1-21: Replace the non-constant-time signature comparison (the
direct string equality check of sig vs expectedSig) with crypto.timingSafeEqual:
import timingSafeEqual from "node:crypto", convert both sig and expectedSig to
Buffers of the same encoding/length, and call timingSafeEqual(Buffer.from(sig),
Buffer.from(expectedSig)) inside a try/catch so mismatched lengths or errors
return a 401 (“Invalid signature”); update the request handler that validates
the query params (the code that reads token, sig and computes expectedSig) to
use timingSafeEqual instead of `sig !== expectedSig` while keeping the existing
expiry check unchanged.
In `@apps/web/src/app/`(dashboard)/settings/billing/page.tsx:
- Line 40: The link currently uses PROTOCOL_SCHEMES.PROD unconditionally; change
it to select PROTOCOL_SCHEMES.DEV when process.env.NODE_ENV !== 'production'
(otherwise use PROD). Update the anchor href construction in page.tsx to derive
the scheme from process.env.NODE_ENV and then build the URL with that scheme
(reference PROTOCOL_SCHEMES and the anchor generating code where
`${PROTOCOL_SCHEMES.PROD}://settings/billing` appears).
🧹 Nitpick comments (3)
apps/api/src/app/api/integrations/slack/events/utils/generate-connect-url/generate-connect-url.ts (2)
14-14: Extract the token TTL to a named constant.
10 * 60 * 1000is a magic number. A named constant improves readability and makes it easy to adjust in one place.Suggested change
+const CONNECT_TOKEN_TTL_MS = 10 * 60 * 1000; // 10 minutes + export function generateConnectUrl({ slackUserId, teamId, ... const payload = JSON.stringify({ slackUserId, teamId, - exp: Date.now() + 10 * 60 * 1000, + exp: Date.now() + CONNECT_TOKEN_TTL_MS, });As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic".
16-16: Verify thatSLACK_SIGNING_SECRETis appropriate for this use case.This reuses Slack's signing secret (intended for verifying requests from Slack) as an HMAC key for your own connect-URL tokens. If Slack rotates this secret, your outstanding connect URLs silently break. Consider whether a dedicated secret (e.g.,
SLACK_CONNECT_SIGNING_SECRET) would be more appropriate to decouple these concerns.apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts (1)
72-99: Consider extracting the subscription/link gating blocks into a shared helper.The subscription gate (Lines 72–99) and account-link gate (Lines 101–137) are nearly identical to the corresponding blocks in
process-mention.ts(Lines 72–137). This is a significant amount of duplicated Slack Block Kit markup and control flow.A shared utility (e.g.,
checkSlackPreconditions) could encapsulate the parallel fetch, subscription check, and link check, returning either the resolvedslackUserLinkor posting the appropriate Slack message and returningnull.Also applies to: 101-137
| db.query.subscriptions.findFirst({ | ||
| where: and( | ||
| eq(subscriptions.referenceId, connection.organizationId), | ||
| eq(subscriptions.status, "active"), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what subscription statuses exist and how trialing is handled elsewhere
rg -n "subscriptions.status" --type ts -C3 -g '!node_modules'
echo "---"
rg -n "trialing\|trial" --type ts -g '!node_modules' -g '!*.test.*' | head -40Repository: superset-sh/superset
Length of output: 4016
🏁 Script executed:
#!/bin/bash
# Find subscription status enum/schema definition
rg -n "status.*trialing|trialing.*status" --type ts -g '!node_modules' -A2 -B2
echo "---"
# Look for subscription schema definition
rg -n "subscriptions.*=.*pgTable|type.*Subscription" --type ts -g '!node_modules' -A10 | head -80
echo "---"
# Check if there's trial-related business logic
rg -n "plan.*trial|trial.*plan|isTrial" --type ts -g '!node_modules' -g '!*.test.*' | head -20Repository: superset-sh/superset
Length of output: 2457
🏁 Script executed:
#!/bin/bash
# Find all places where subscription status is set/updated
rg -n "status.*=.*[\"']" --type ts -g '!node_modules' | grep -i "subscription\|status" | head -30
echo "---"
# Look for Stripe subscription status handling (since status likely comes from Stripe)
rg -n "status.*complete|trial|active" --type ts -g '!node_modules' -g '!*.test.*' | grep -i "stripe\|subscription" | head -20
echo "---"
# Check the Slack integration files more closely
cat apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts | head -80Repository: superset-sh/superset
Length of output: 7742
🏁 Script executed:
#!/bin/bash
# Find Stripe webhook/event handling for subscriptions
rg -n "stripe.*subscription|webhook.*subscription" --type ts -g '!node_modules' -i | head -20
echo "---"
# Find where subscription status is actually SET (not just queried)
rg -n "subscriptions.*update|\.status.*=|status.*pending|status.*active|status.*trialing" --type ts -g '!node_modules' -g '!*.test.*' | head -30
echo "---"
# Check if there's a plan types enum
rg -n "type.*Plan|enum.*Plan|const.*PLAN" --type ts -g '!node_modules' | head -20Repository: superset-sh/superset
Length of output: 6778
🏁 Script executed:
#!/bin/bash
# Check the subscription schema more closely for trial-related fields
cat packages/db/src/schema/schema.ts | grep -A 30 "export const subscriptions"
echo "---"
# Check if there's any trial field in the subscription table
rg -n "trial|trialEnds\|trial_ends" --type ts -g '!node_modules' packages/db/src/schema/
echo "---"
# Look for plan handling in the subscription context
rg -n "subscription.*plan|plan.*subscription" --type ts -g '!node_modules' -B2 -A2 | head -50Repository: superset-sh/superset
Length of output: 4609
🏁 Script executed:
#!/bin/bash
# Find Stripe webhook handlers (likely in auth/server.ts based on earlier grep)
rg -n "events.data.object.status\|customer.subscription" --type ts -g '!node_modules' -B5 -A5 | head -80
echo "---"
# Check if trials are mentioned in any business logic or guards
rg -n "trialStart|trialEnd" --type ts -g '!node_modules' -B2 -A2Repository: superset-sh/superset
Length of output: 550
🏁 Script executed:
#!/bin/bash
# Find Stripe event type handlers - look for checkout.session.completed, invoice.payment_succeeded, etc.
rg -n "checkout\|invoice\|customer\\.subscription" --type ts packages/auth/src/server.ts -A10 -B2 | head -100
echo "---"
# Look for where subscription records are created with status values
rg -n "subscriptions\\.create\|INSERT.*subscriptions\|db\\.insert.*subscriptions" --type ts -g '!node_modules' -B3 -A10 | head -60Repository: superset-sh/superset
Length of output: 67
🏁 Script executed:
#!/bin/bash
# Search for subscription creation across entire codebase
rg -n "subscriptions.*insert\|insert.*subscriptions" --type ts -g '!node_modules' -B2 -A8
echo "---"
# Look at the full Stripe webhook file to understand event handling
find . -name "*stripe*" -type f -path "*/src/*" -not -path "*/node_modules/*" | grep -E "\.(ts|tsx)$"Repository: superset-sh/superset
Length of output: 278
🏁 Script executed:
#!/bin/bash
# Read the stripe.ts file to understand subscription handling
wc -l packages/auth/src/stripe.ts
echo "---"
cat packages/auth/src/stripe.tsRepository: superset-sh/superset
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Get the size of server.ts
wc -l packages/auth/src/server.ts
echo "---"
# Search for Stripe event handler patterns and subscription-related updates
rg -n "export.*function\|async.*function\|stripeSubscription\|subscription.*update\|subscription.*insert" packages/auth/src/server.ts | head -40Repository: superset-sh/superset
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Look at the structure of packages/auth/src/server.ts
head -100 packages/auth/src/server.ts
echo "---"
# Search for where subscriptions table is used (insert, update, upsert)
rg -n "db.*subscriptions\|subscriptions.*\(" packages/auth/src/server.ts | head -30Repository: superset-sh/superset
Length of output: 3382
🏁 Script executed:
#!/bin/bash
# Search for the stripe plugin configuration in server.ts
rg -n "stripe\(" packages/auth/src/server.ts -A30 | head -60
echo "---"
# Search for subscription status assignments
rg -n "status.*=\|update.*status" packages/auth/src/server.ts | grep -i subscriptionRepository: superset-sh/superset
Length of output: 981
🌐 Web query:
better-auth stripe plugin subscription status values trialing active
💡 Result:
In the Better Auth Stripe plugin, subscription.status is stored as a string and effectively follows Stripe’s subscription status field. Better Auth’s own examples treat a subscription as “currently valid” if status === "active" || status === "trialing" (and it enforces at most one active or trialing subscription per referenceId at a time). [1]
So, besides "trialing" and "active", you should expect the other Stripe status values as well: incomplete, incomplete_expired, past_due, canceled, unpaid, paused. [2]
Practical rule for access checks (per Better Auth docs/examples): grant access when status is active or trialing; treat the rest as not entitled unless you intentionally support grace periods (e.g., past_due). [1][2]
🏁 Script executed:
#!/bin/bash
# Check all Slack integration files for subscription status checks
rg -n "eq\(subscriptions.status" apps/api/src/app/api/integrations/slack -B5 -A5
echo "---"
# Check if there's any trial-aware logic elsewhere
rg -n "trialing\|trial" packages/auth/src/server.tsRepository: superset-sh/superset
Length of output: 2602
Update subscription status check to include trial subscriptions.
The query currently filters eq(subscriptions.status, "active"), which excludes trial subscriptions (status = "trialing"). According to Better Auth's Stripe plugin behavior, subscriptions can have status "trialing" during the trial period, and access logic should grant access for both "active" and "trialing" statuses.
This affects both Slack integration files:
apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts(line 66)apps/api/src/app/api/integrations/slack/events/process-mention/process-mention.ts(line 66)
Update the query to use or(eq(subscriptions.status, "active"), eq(subscriptions.status, "trialing")) or a similar pattern to include both valid statuses.
🤖 Prompt for AI Agents
In
`@apps/api/src/app/api/integrations/slack/events/process-assistant-message/process-assistant-message.ts`
around lines 63 - 67, The subscription lookup currently only checks
eq(subscriptions.status, "active") which excludes trial users; update the where
clause used in db.query.subscriptions.findFirst (in
process-assistant-message.ts) to accept both "active" and "trialing" statuses by
replacing the single eq check with an or(eq(subscriptions.status, "active"),
eq(subscriptions.status, "trialing")) (and make the same change in the analogous
query in process-mention.ts) so both active and trialing subscriptions are
treated as valid.
| import { createHmac } from "node:crypto"; | ||
| import { env } from "@/env"; | ||
|
|
||
| export function generateConnectUrl({ | ||
| slackUserId, | ||
| teamId, | ||
| }: { | ||
| slackUserId: string; | ||
| teamId: string; | ||
| }): string { | ||
| const payload = JSON.stringify({ | ||
| slackUserId, | ||
| teamId, | ||
| exp: Date.now() + 10 * 60 * 1000, | ||
| }); | ||
| const signature = createHmac("sha256", env.SLACK_SIGNING_SECRET) | ||
| .update(payload) | ||
| .digest("hex"); | ||
| const token = Buffer.from(payload).toString("base64url"); | ||
| return `${env.NEXT_PUBLIC_API_URL}/api/integrations/slack/link?token=${token}&sig=${signature}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the slack/link endpoint handler
fd -t f \( -name "*.ts" -o -name "*.js" \) | xargs grep -l "slack/link" | head -20Repository: superset-sh/superset
Length of output: 294
🏁 Script executed:
#!/bin/bash
# Search more broadly for slack integration route files
find . -type f \( -name "*.ts" -o -name "*.js" \) -path "*slack*" -path "*link*" 2>/dev/nullRepository: superset-sh/superset
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Search for signature validation and expiry checks in slack-related files
rg -n "timingSafeEqual|timingConstantEqual|exp.*Date|signature.*verify" --type ts -g "*slack*" 2>/dev/nullRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cat apps/api/src/app/api/integrations/slack/link/route.tsRepository: superset-sh/superset
Length of output: 2704
Fix timing attack vulnerability: use crypto.timingSafeEqual for signature comparison.
The endpoint validates both signature and expiry, but the signature comparison at line 27 uses non-constant-time string equality (sig !== expectedSig), which is vulnerable to timing attacks. Replace with:
Fix signature comparison
import { timingSafeEqual } from "node:crypto";
// Instead of:
if (sig !== expectedSig) {
// Use:
try {
timingSafeEqual(Buffer.from(sig), Buffer.from(expectedSig));
} catch {
return new Response("Invalid signature", { status: 401 });
}The expiry validation (lines 34–38) is correct.
🤖 Prompt for AI Agents
In
`@apps/api/src/app/api/integrations/slack/events/utils/generate-connect-url/generate-connect-url.ts`
around lines 1 - 21, Replace the non-constant-time signature comparison (the
direct string equality check of sig vs expectedSig) with crypto.timingSafeEqual:
import timingSafeEqual from "node:crypto", convert both sig and expectedSig to
Buffers of the same encoding/length, and call timingSafeEqual(Buffer.from(sig),
Buffer.from(expectedSig)) inside a try/catch so mismatched lengths or errors
return a 401 (“Invalid signature”); update the request handler that validates
the query params (the code that reads token, sig and computes expectedSig) to
use timingSafeEqual instead of `sig !== expectedSig` while keeping the existing
expiry check unchanged.
| </div> | ||
| <div className="flex flex-wrap justify-center gap-3"> | ||
| <Button size="lg" className="gap-2" asChild> | ||
| <a href={`${PROTOCOL_SCHEMES.PROD}://settings/billing`}> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where PROTOCOL_SCHEMES is defined
rg -n 'PROTOCOL_SCHEMES' --type ts --type tsx -B2 -A2 | head -100Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the specific file and line mentioned in the review
cat -n apps/web/src/app/\(dashboard\)/settings/billing/page.tsx | sed -n '35,45p'Repository: superset-sh/superset
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Look for how process.env.NODE_ENV is used for environment-specific logic in the codebase
rg -n 'process\.env\.NODE_ENV' --type ts --type tsx -C2 | head -80Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for other deep link usage patterns with PROTOCOL_SCHEMES
rg -n 'href.*PROTOCOL_SCHEMES' --type ts --type tsx -B1 -A1Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for PROTOCOL_SCHEMES definition without file type restriction
rg -n 'PROTOCOL_SCHEMES\s*=' -A5 | head -50Repository: superset-sh/superset
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Search for PROTOCOL_SCHEMES usage with context
rg -n 'PROTOCOL_SCHEMES\.' -B1 -A1 | head -100Repository: superset-sh/superset
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Look for process.env.NODE_ENV patterns in the codebase
rg -n 'process\.env\.NODE_ENV' -B2 -A2 | head -80Repository: superset-sh/superset
Length of output: 5195
🏁 Script executed:
#!/bin/bash
# Find constants or config files that might define PROTOCOL_SCHEMES
fd -e ts -e js -e tsx -e jsx | xargs grep -l 'PROTOCOL_SCHEMES' | head -20Repository: superset-sh/superset
Length of output: 188
Use the appropriate protocol scheme based on environment.
The PROTOCOL_SCHEMES.PROD is hardcoded, which will break deep links in development environments where the desktop app registers superset-dev://. Both DEV and PROD variants exist in PROTOCOL_SCHEMES; select the appropriate scheme based on process.env.NODE_ENV.
Proposed fix
+const protocolScheme =
+ process.env.NODE_ENV === "production"
+ ? PROTOCOL_SCHEMES.PROD
+ : PROTOCOL_SCHEMES.DEV;
+
...
- <a href={`${PROTOCOL_SCHEMES.PROD}://settings/billing`}>
+ <a href={`${protocolScheme}://settings/billing`}>🤖 Prompt for AI Agents
In `@apps/web/src/app/`(dashboard)/settings/billing/page.tsx at line 40, The link
currently uses PROTOCOL_SCHEMES.PROD unconditionally; change it to select
PROTOCOL_SCHEMES.DEV when process.env.NODE_ENV !== 'production' (otherwise use
PROD). Update the anchor href construction in page.tsx to derive the scheme from
process.env.NODE_ENV and then build the URL with that scheme (reference
PROTOCOL_SCHEMES and the anchor generating code where
`${PROTOCOL_SCHEMES.PROD}://settings/billing` appears).
…ller Previously, any Slack user could impersonate the person who installed the Slack integration because runSlackAgent always used connectedByUserId. Now each entry point resolves the actual Superset user via the usersSlackUsers link table and passes their userId to the agent. Unlinked users see a connect button; free orgs see an upgrade button. Also updates the billing page with deep link + download CTAs.
saddlepaddle
force-pushed
the
slack-use-auth
branch
from
e1c35a6 to
9229e03
Compare
1 participant
Summary
runSlackAgentalways usedconnectedByUserId(the person who installed the integration) as the identity for MCP tool calls. Any Slack user in the workspace could act as that person. Now each request resolves the actual Superset user via theusersSlackUserslink table.process-mentionandprocess-assistant-messagecheck for an active subscription before proceeding. Free orgs see an "Upgrade to Pro" button.generateConnectUrlto shared utility: Moved fromprocess-app-home-openedtoutils/generate-connect-url/so both entry points can reuse it.Test plan
/settings/billingon web → see deep link + download buttons/settings/billing?success=true→ see payment success message (unchanged)Summary by CodeRabbit
New Features
Improvements
Refactor