fix: Handle personal billing portal on backend#23521
Conversation
WalkthroughThe Stripe billing portal endpoint was changed to require authentication and to always issue an HTTP 302 redirect to a Stripe Billing Portal session URL. For requests without a teamId it looks up the Stripe customer by userId and redirects if found (404 if not). For requests with a teamId it verifies the user is an admin of the team, validates team metadata, ensures a subscription and subscription.customer exist, retrieves the Stripe customerId, and redirects to the portal. Errors return appropriate 400/401/404 statuses. The exported handler signature is unchanged. Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
keithwillcode
added
core
| if (safeRedirectUrl) return_url = safeRedirectUrl; | ||
| } | ||
|
|
||
| if (!team) { |
There was a problem hiding this comment.
Moved this further up in the function
dosubot
bot
added
billing
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app-store/stripepayment/api/portal.ts (1)
67-71: Handle string or expanded Customer forsubscription.customer
Use a runtime type‐check instead ofas stringto safely extractcustomerId:- const customerId = subscription.customer as string; - - if (!customerId) return res.status(400).json({ message: "CustomerId not found in stripe" }); + const customerId = + typeof subscription.customer === "string" + ? subscription.customer + : subscription.customer?.id; + if (!customerId) { + return res.status(400).json({ message: "Subscription customer not found" }); + }
🧹 Nitpick comments (5)
packages/app-store/stripepayment/api/portal.ts (5)
21-21: Normalize return URL handling and naming
- Use a consistent camelCase variable (e.g.,
returnUrl) and keepreturn_urlonly as the Stripe param key.- Compute the safe return URL before branching so both personal and team flows honor
returnTo.Apply this diff here and update usages below:
-let return_url = `${WEBAPP_URL}/settings/billing`; +let returnUrl = `${WEBAPP_URL}/settings/billing`;Then (outside this range) move the safe-redirect check above the
if (!teamId)branch:// just after reading query params if (typeof req.query.returnTo === "string") { const safe = getSafeRedirectUrl(req.query.returnTo); if (safe) returnUrl = safe; }And replace all
return_urlvariable references withreturnUrl, while keeping the Stripe field asreturn_url: returnUrl.
71-76: Team flow: prefer 303 on POST and add error handling symmetryMirror the personal-flow behavior: 303 for POST and catch Stripe errors.
Apply this diff:
- const stripeSession = await stripe.billingPortal.sessions.create({ - customer: customerId, - return_url, - }); - - res.redirect(302, stripeSession.url); + try { + const stripeSession = await stripe.billingPortal.sessions.create({ + customer: customerId, + return_url: returnUrl, + }); + const status = req.method === "POST" ? 303 : 302; + return res.redirect(status, stripeSession.url); + } catch (err) { + return res.status(502).json({ message: "Failed to create billing portal session" }); + }
14-15: SetAllowheader on 405 responsesStandards-compliant and helpful for clients.
Apply this diff:
- if (req.method !== "POST" && req.method !== "GET") - return res.status(405).json({ message: "Method not allowed" }); + if (req.method !== "POST" && req.method !== "GET") { + res.setHeader("Allow", "GET, POST"); + return res.status(405).json({ message: "Method not allowed" }); + }
21-31: Consistency: renamereturn_urlvariable to camelCaseMinor consistency nit: prefer
returnUrlfor the local variable and only usereturn_urlas the Stripe field key.Apply this diff (selected excerpts already updated in other suggestions):
-let return_url = `${WEBAPP_URL}/settings/billing`; +let returnUrl = `${WEBAPP_URL}/settings/billing`; ... - return_url: return_url, + return_url: returnUrl, ... - return_url, + return_url: returnUrl,Also applies to: 71-76
23-31: Observability (optional): log failures with minimal contextIf you use a logger (e.g.,
req.logor similar), log Stripe errors with sanitized messages to aid support while avoiding PII leakage.Happy to draft a minimal logging wrapper if you share your preferred logger.
Also applies to: 71-76
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app-store/stripepayment/api/portal.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/app-store/stripepayment/api/portal.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/app-store/stripepayment/api/portal.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/app-store/stripepayment/api/portal.ts
🧬 Code graph analysis (1)
packages/app-store/stripepayment/api/portal.ts (2)
packages/lib/constants.ts (1)
WEBAPP_URL(12-18)packages/app-store/_utils/stripe.ts (1)
stripe(72-74)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/app-store/stripepayment/api/portal.ts (4)
35-37: Harden teamId parsing; reject arrays/NaN; avoid falling into personal flow
parseInt(req.query.teamId as string)will coerce arrays to strings (e.g., "1,2") and silently parse to 1;NaNand0also fall into the personal flow. Validate and 400 on bad input.- const teamId = req.query.teamId ? parseInt(req.query.teamId as string) : null; + const teamIdRaw = req.query.teamId; + const teamId = + typeof teamIdRaw === "string" ? Number(teamIdRaw) : + typeof teamIdRaw === "undefined" ? null : + Number.NaN; + if (teamIdRaw !== undefined && (!Number.isInteger(teamId) || teamId <= 0)) { + return res.status(400).json({ message: "Invalid teamId" }); + }
47-54: Enforce admin/owner membership check for team portal access
getTeamByIdIfUserIsAdminreturns the team even whenmembersis empty; current code only checks existence, enabling access by any authenticated user who knows a teamId. Require at least one ADMIN/OWNER membership.- if (!team) return res.status(404).json({ message: "Team not found" }); + if (!team || team.members.length === 0) { + // Mask auth state to avoid leaking membership + return res.status(404).json({ message: "Team not found" }); + }
37-37: Honor returnTo for personal flow; normalize return URL oncePersonal flow ignores
returnTo. Normalize a singlereturnUrlup-front usinggetSafeRedirectUrland reuse it in both flows.- let return_url = `${WEBAPP_URL}/settings/billing`; + const returnTo = typeof req.query.returnTo === "string" ? req.query.returnTo : undefined; + let returnUrl = `${WEBAPP_URL}/settings/billing`; + if (returnTo) { + const safe = getSafeRedirectUrl(returnTo); + if (safe) returnUrl = safe; + } @@ - if (typeof req.query.returnTo === "string") { - const safeRedirectUrl = getSafeRedirectUrl(req.query.returnTo); - if (safeRedirectUrl) return_url = safeRedirectUrl; - } + // returnUrl already normalized above for both personal and team flowsAlso applies to: 55-58
42-45: Use 303 for POST redirects and surface Stripe errors as 502After non-idempotent POST, prefer
303 See Other. Also catch failures fromgetBillingPortalUrlto return a clean 502 JSON instead of a generic 500 HTML.- const billingPortalUrl = await getBillingPortalUrl(customerId, return_url); - - return res.redirect(302, billingPortalUrl); + try { + const billingPortalUrl = await getBillingPortalUrl(customerId, returnUrl); + const status = req.method === "POST" ? 303 : 302; + return res.redirect(status, billingPortalUrl); + } catch { + return res.status(502).json({ message: "Failed to create billing portal session" }); + } @@ - const billingPortalUrl = await getBillingPortalUrl(customerId, return_url); - - res.redirect(302, billingPortalUrl); + try { + const billingPortalUrl = await getBillingPortalUrl(customerId, returnUrl); + const status = req.method === "POST" ? 303 : 302; + return res.redirect(status, billingPortalUrl); + } catch { + return res.status(502).json({ message: "Failed to create billing portal session" }); + }Also applies to: 84-87
🧹 Nitpick comments (2)
packages/app-store/stripepayment/api/portal.ts (2)
14-27: Preserve stack via error cause and use structured loggingThrowing a new Error drops the original stack; include
causeand log as structured data to improve debuggability. Also consider usingreturnUrlnaming locally and map to Stripe’sreturn_urlfield.-const getBillingPortalUrl = async (customerId: string, return_url: string) => { +const getBillingPortalUrl = async (customerId: string, returnUrl: string) => { const log = logger.getSubLogger({ prefix: ["getBillingPortalUrl"] }); try { const portalSession = await stripe.billingPortal.sessions.create({ customer: customerId, - return_url, + return_url: returnUrl, }); return portalSession.url; } catch (e) { - log.error(`Failed to create billing portal session for ${customerId}: ${e}`); - throw new Error("Failed to create billing portal session"); + log.error({ err: e, customerId }, "Failed to create billing portal session"); + throw new Error("Failed to create billing portal session", { cause: e as Error }); } };
29-31: Minor: include Allow header on 405Add
Allow: GET, POSTfor better client compliance.- if (req.method !== "POST" && req.method !== "GET") - return res.status(405).json({ message: "Method not allowed" }); + if (req.method !== "POST" && req.method !== "GET") { + res.setHeader("Allow", "GET, POST"); + return res.status(405).json({ message: "Method not allowed" }); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app-store/stripepayment/api/portal.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/app-store/stripepayment/api/portal.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/app-store/stripepayment/api/portal.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/app-store/stripepayment/api/portal.ts
🧠 Learnings (1)
📚 Learning: 2025-08-27T12:15:43.830Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/testCall.handler.ts:41-44
Timestamp: 2025-08-27T12:15:43.830Z
Learning: In calcom/cal.com, the AgentService.getAgent() method in packages/features/calAIPhone/providers/retellAI/services/AgentService.ts does NOT include authorization checks - it only validates the agentId parameter and directly calls the repository without verifying user/team access. This contrasts with other methods like getAgentWithDetails() which properly use findByIdWithUserAccessAndDetails() for authorization. When reviewing updateToolsFromAgentId() calls, always verify both agent ownership and eventType ownership are checked.
Applied to files:
packages/app-store/stripepayment/api/portal.ts
🧬 Code graph analysis (1)
packages/app-store/stripepayment/api/portal.ts (2)
packages/app-store/_utils/stripe.ts (1)
stripe(72-74)packages/lib/constants.ts (1)
WEBAPP_URL(12-18)
🔇 Additional comments (1)
packages/app-store/stripepayment/api/portal.ts (1)
5-5: Good addition: route-level logging context is in placeImporting and using the shared logger is a solid move for observability.
E2E results are ready! |
3 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist