-
Notifications
You must be signed in to change notification settings - Fork 953
fix(api): use linked Slack user identity for MCP auth #1447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| 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}`; | ||
| } | ||
|
Comment on lines
+1
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 The endpoint validates both signature and expiry, but the signature comparison at line 27 uses non-constant-time string equality ( Fix signature comparisonimport { 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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { generateConnectUrl } from "./generate-connect-url"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 4016
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 2457
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 7742
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 6778
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 4609
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 550
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 67
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 278
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 219
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 99
🏁 Script executed:
Repository: superset-sh/superset
Length of output: 3382
🏁 Script executed:
Repository: 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.statusis stored as a string and effectively follows Stripe’s subscriptionstatusfield. Better Auth’s own examples treat a subscription as “currently valid” ifstatus === "active" || status === "trialing"(and it enforces at most one active or trialing subscription perreferenceIdat 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
activeortrialing; treat the rest as not entitled unless you intentionally support grace periods (e.g.,past_due). [1][2]🏁 Script executed:
Repository: 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