feat: add Webhook resource to PBAC system with permission enforcement#23614
feat: add Webhook resource to PBAC system with permission enforcement#23614keithwillcode merged 19 commits intomainfrom
Conversation
- Add Webhook resource to PBAC permission registry with CRUD actions - Implement PBAC permission checks in webhook handlers (create, edit, delete) - Add webhook permission translations to common.json - Use PermissionCheckService with fallback roles [ADMIN, OWNER] for team webhooks - Maintain backward compatibility when PBAC is disabled - Follow same pattern as workflow PBAC implementation from PR #22845 Co-Authored-By: sean@cal.com <Sean@brydon.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change integrates PBAC for webhooks across the stack: adds a Webhook resource with CRUD entries to the permission registry and seeds role permissions; updates locales with webhook i18n keys; replaces membership-based checks with PermissionCheckService in the webhook repository (adds findByWebhookId, changes group metadata to canModify/canDelete, filters empty groups, and simplifies selected fields); refactors TRPC webhook procedures to use createWebhookPbacProcedure and adds per-endpoint PBAC checks; updates UI to use a permissions object (removes isAdmin); and adds unit tests for PBAC middleware. Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
- Add PermissionCheckService to filter team webhooks by webhook.read permission - Only show webhooks from teams where user has proper permissions - Maintain backward compatibility with fallback to all team memberships Co-Authored-By: sean@cal.com <Sean@brydon.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
32-52: Add PBAC check for eventType-scoped listing to prevent cross-team leakageWhen input.eventTypeId is provided, there’s no authorization. A user can pass an event type from another team and retrieve its webhooks. Gate this path: allow if (a) personal event type owned by the user, or (b) team event type where webhook.read is permitted (with ADMIN/OWNER fallback).
Apply this diff:
if (input?.eventTypeId) { + const permissionService = new PermissionCheckService(); + const evt = await prisma.eventType.findUnique({ + where: { id: input.eventTypeId }, + select: { teamId: true, userId: true }, + }); + if (evt?.teamId) { + const ok = await permissionService.checkPermission({ + userId: ctx.user.id, + teamId: evt.teamId, + permission: "webhook.read", + fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], + }); + if (!ok) return []; + } else if (evt?.userId && evt.userId !== ctx.user.id) { + return []; + } const managedParentEvt = await prisma.eventType.findFirst({ where: { id: input.eventTypeId, parentId: { not: null, }, }, select: { parentId: true, }, });
♻️ Duplicate comments (1)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
53-68: Fix PBAC filtering: don’t block personal webhooks and honor ADMIN/OWNER fallbacks per team
- Early return on Line 62 drops user-owned webhooks when no team permission exists.
- Using getTeamIdsWithPermission ignores fallback roles when PBAC is disabled.
- Switch to per-team checkPermission with fallbackRoles and build the OR clause dynamically.
Apply this diff:
- const permissionService = new PermissionCheckService(); - const teamsWithReadPermission = await permissionService.getTeamIdsWithPermission( - ctx.user.id, - "webhook.read" - ); - - const teamIds = user?.teams.map((membership) => membership.teamId) || []; - const authorizedTeamIds = teamIds.filter((teamId) => teamsWithReadPermission.includes(teamId)); - - if (authorizedTeamIds.length === 0) { - return []; - } - - where.AND?.push({ - OR: [{ userId: ctx.user.id }, { teamId: { in: authorizedTeamIds } }], - }); + const permissionService = new PermissionCheckService(); + const teamIds = user?.teams?.map((m) => m.teamId) ?? []; + const allowedTeamIds = ( + await Promise.all( + teamIds.map(async (teamId) => { + const ok = await permissionService.checkPermission({ + userId: ctx.user.id, + teamId, + permission: "webhook.read", + fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], + }); + return ok ? teamId : null; + }) + ) + ).filter((x): x is number => x !== null); + + where.AND?.push({ + OR: [{ userId: ctx.user.id }, ...(allowedTeamIds.length ? [{ teamId: { in: allowedTeamIds } }] : [])], + });
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (3)
3-3: Import fallback roles for legacy mode supportRequired by the per-team permission checks to honor ADMIN/OWNER when PBAC is disabled.
Apply this diff:
import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; +import { MembershipRole } from "@calcom/prisma/enums";
22-29: Narrow Prisma select to only required fieldsYou only need membership teamIds; selecting entire teams relation overfetches.
Apply this diff:
const user = await prisma.user.findUnique({ where: { id: ctx.user.id, }, select: { - teams: true, + teams: { select: { teamId: true } }, }, });
76-78: Return a minimal shape to avoid leaking sensitive columnsAlign with “select only what you need.” If Webhook has secrets (e.g., secret, signingKey), exclude them explicitly.
Proposed shape (adjust as per model):
- return await prisma.webhook.findMany({ - where, - }); + return await prisma.webhook.findMany({ + where, + select: { + id: true, + teamId: true, + userId: true, + eventTypeId: true, + appId: true, + eventTriggers: true, + active: true, + createdAt: true, + updatedAt: true, + // secret: false, // ensure any secret-like fields are excluded + }, + });If you confirm the exact Webhook schema, I can tailor the select to guarantee no secrets/PII are exposed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/trpc/server/routers/viewer/webhook/list.handler.ts(2 hunks)packages/trpc/server/routers/viewer/webhook/util.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/server/routers/viewer/webhook/util.ts
🧰 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/trpc/server/routers/viewer/webhook/list.handler.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/trpc/server/routers/viewer/webhook/list.handler.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/trpc/server/routers/viewer/webhook/list.handler.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#23614
File: packages/features/pbac/domain/types/permission-registry.ts:520-551
Timestamp: 2025-09-08T12:46:13.448Z
Learning: Webhooks in Cal.com support both team-level and organization-level scopes, so webhook PBAC permissions should not be constrained to Team scope only.
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.ts
📚 Learning: 2025-08-26T20:23:28.396Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:83-88
Timestamp: 2025-08-26T20:23:28.396Z
Learning: In calcom/cal.com PR #22995, the workflow update handler in packages/trpc/server/routers/viewer/workflows/update.handler.ts includes workflow-level authorization via isAuthorized(userWorkflow, ctx.user.id, "workflow.update") which validates the user can update the workflow before calling updateToolsFromAgentId (per maintainer Udit-takkar).
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.ts
📚 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/trpc/server/routers/viewer/webhook/list.handler.ts
📚 Learning: 2025-09-08T12:46:13.448Z
Learnt from: sean-brydon
PR: calcom/cal.com#23614
File: packages/features/pbac/domain/types/permission-registry.ts:520-551
Timestamp: 2025-09-08T12:46:13.448Z
Learning: Webhooks in Cal.com support both team-level and organization-level scopes, so webhook PBAC permissions should not be constrained to Team scope only.
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(21-366)
⏰ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
23-30: Minimize Prisma selection and exclude pending membershipsSelect only accepted teamIds to reduce payload and avoid evaluating permissions for pending invites.
- const user = await prisma.user.findUnique({ + const user = await prisma.user.findUnique({ where: { id: ctx.user.id, }, select: { - teams: true, + teams: { + where: { accepted: true }, + select: { teamId: true }, + }, }, });packages/lib/server/repository/webhook.ts (1)
164-181: Platform webhooks must exclude secret and unnecessary fieldsReturn only safe fields, and optionally filter out app-managed webhooks for consistency.
Apply this diff:
- if (userRole === "ADMIN") { - const platformWebhooks = await prisma.webhook.findMany({ - where: { platform: true }, - }); + if (userRole === "ADMIN") { + const platformWebhooks = await prisma.webhook.findMany({ + where: { platform: true, appId: { notIn: ["zapier", "make"] } }, + select: { + id: true, + subscriberUrl: true, + payloadTemplate: true, + active: true, + eventTriggers: true, + teamId: true, + userId: true, + platform: true, + time: true, + timeUnit: true, + appId: true, + }, + });
♻️ Duplicate comments (6)
packages/trpc/server/routers/viewer/webhook/getByViewer.handler.ts (1)
18-21: Align types with repository output: replace readOnly with canModify/canDeleteWebhookRepository.getFilteredWebhooksForUser returns metadata: { canModify, canDelete } and spreads these onto profiles. Update local types to match to avoid downstream type/runtime mismatches.
metadata?: { - readOnly: boolean; + canModify: boolean; + canDelete: boolean; }; @@ export type WebhooksByViewer = { webhookGroups: WebhookGroup[]; profiles: { - readOnly?: boolean | undefined; + canModify?: boolean | undefined; + canDelete?: boolean | undefined; slug: string | null; name: string | null; image?: string | undefined; teamId: number | null | undefined; }[]; };Also applies to: 24-33
packages/trpc/server/routers/viewer/webhook/delete.handler.ts (1)
26-28: Authorization for eventTypeId likely handled by PBAC middleware — confirm to avoid gapsIf the delete route is wrapped with createWebhookPbacProcedure("webhook.delete"), this branch is fine; otherwise add an ownership/team check for the event’s team.
#!/bin/bash # Verify delete route uses PBAC procedure rg -nP -C3 'createWebhookPbacProcedure\([^)]*webhook\.delete[^)]*\)' packages/trpc/server/routers/viewer/webhookpackages/trpc/server/routers/viewer/webhook/util.test.ts (1)
9-11: Move mocks above imports so they actually take effect
authedProcedureandprismaare imported before theirvi.mock(...)calls, so the real modules are loaded and mocks won’t apply. Import modules-under-test only after all mocks are defined. Also drop the unusedwebhookProcedureimport.Apply:
-import type { PermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; -import { prisma } from "@calcom/prisma"; -import type { MembershipRole } from "@calcom/prisma/enums"; - -import { TRPCError } from "@trpc/server"; - -import authedProcedure from "../../../procedures/authedProcedure"; -// Import after mocks are set up -import { createWebhookPbacProcedure, webhookProcedure } from "./util"; +import { TRPCError } from "@trpc/server"; + +// Mocks first ──────────────────────────────────────────────────────────────── +vi.mock("@calcom/prisma", () => ({ + prisma: { + webhook: { findUnique: vi.fn() }, + eventType: { findUnique: vi.fn() }, + user: { findUnique: vi.fn() }, + }, +})); +const mockCheckPermission = vi.fn(); +vi.mock("@calcom/features/pbac/services/permission-check.service", () => ({ + PermissionCheckService: vi.fn().mockImplementation(() => ({ + checkPermission: mockCheckPermission, + })), +})); +vi.mock("../../../procedures/authedProcedure", () => ({ + default: { + input: vi.fn().mockReturnThis(), + use: vi.fn(), + }, +})); + +// Imports after mocks ──────────────────────────────────────────────────────── +import type { PermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; +import { prisma } from "@calcom/prisma"; +import type { MembershipRole } from "@calcom/prisma/enums"; +import authedProcedure from "../../../procedures/authedProcedure"; +import { createWebhookPbacProcedure } from "./util";Also applies to: 36-41
packages/trpc/server/routers/viewer/webhook/util.ts (1)
70-74: Prisma: replaceincludewithselectto avoid overfetchingOnly
id,userId, andteamIdare used fromeventType. Replaceinclude: { team: true }with a minimalselect.Apply:
- const eventType = await prisma.eventType.findUnique({ - where: { id: webhook.eventTypeId }, - include: { team: true }, - }); + const eventType = await prisma.eventType.findUnique({ + where: { id: webhook.eventTypeId }, + select: { id: true, userId: true, teamId: true }, + });- const eventType = await prisma.eventType.findUnique({ - where: { id: eventTypeId }, - include: { team: true }, - }); + const eventType = await prisma.eventType.findUnique({ + where: { id: eventTypeId }, + select: { id: true, userId: true, teamId: true }, + });Also applies to: 126-130
packages/lib/server/repository/webhook.ts (2)
34-53: Block IDOR and stop exposing webhook.secret; require viewer context and authCurrent method:
- accepts optional webhookId (can be undefined → Prisma error),
- returns secret,
- performs no authorization.
Harden it to require viewerId/userRole, perform auth (team, personal, platform), and never return secret.
Apply this diff:
- static async findByWebhookId(webhookId?: string) { - return await prisma.webhook.findUniqueOrThrow({ - where: { - id: webhookId, - }, - select: { - id: true, - subscriberUrl: true, - payloadTemplate: true, - active: true, - eventTriggers: true, - secret: true, - teamId: true, - userId: true, - platform: true, - time: true, - timeUnit: true, - }, - }); - } + static async findByWebhookId({ + webhookId, + viewerId, + userRole, + }: { + webhookId: string; + viewerId: number; + userRole?: UserPermissionRole; + }) { + if (!webhookId) throw new Error("webhookId is required"); + + // Minimal fetch for auth context + const base = await prisma.webhook.findUniqueOrThrow({ + where: { id: webhookId }, + select: { id: true, teamId: true, userId: true, platform: true }, + }); + + // AuthZ + if (base.platform) { + if (userRole !== "ADMIN") throw new Error("Not authorized"); + } else if (base.teamId) { + const permissionService = new PermissionCheckService(); + const canRead = await permissionService.checkPermission({ + userId: viewerId, + teamId: base.teamId, + permission: "webhook.read", + fallbackRoles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], + }); + if (!canRead) throw new Error("Not authorized"); + } else if (base.userId !== viewerId) { + throw new Error("Not authorized"); + } + + // Safe fields only (no secret) + return prisma.webhook.findUniqueOrThrow({ + where: { id: webhookId }, + select: { + id: true, + subscriberUrl: true, + payloadTemplate: true, + active: true, + eventTriggers: true, + teamId: true, + userId: true, + platform: true, + time: true, + timeUnit: true, + appId: true, + }, + }); + }
186-191: Profiles should mirror filtered groupsprofiles is built from webhookGroups before filtering, causing orphan profiles for zero-webhook groups.
Apply this diff:
- return { - webhookGroups: webhookGroups.filter((group) => group.webhooks.length > 0), - profiles: webhookGroups.map((group) => ({ + const filteredGroups = webhookGroups.filter((group) => group.webhooks.length > 0); + return { + webhookGroups: filteredGroups, + profiles: filteredGroups.map((group) => ({ teamId: group.teamId, ...group.profile, ...group.metadata, })), };
🧹 Nitpick comments (21)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
54-69: Correct per-team permission check; remove debug loggingPer-team check via PermissionCheckService with [ADMIN, OWNER] fallback is good. Drop the console.log before merging.
- console.log("Allowed Team IDs:", allowedTeamIds);Also applies to: 72-74
packages/features/eventtypes/components/tabs/instant/InstantEventController.tsx (1)
358-373: Avoid dual prop patterns: use the new permissions prop onlyYou’re passing both canEditWebhook and permissions. Prefer the permissions object to prevent divergence.
- <WebhookListItem + <WebhookListItem key={webhook.id} webhook={webhook} lastItem={webhooks.length === index + 1} - canEditWebhook={!webhookLockedStatus.disabled} onEditWebhook={() => { setEditModalOpen(true); setWebhookToEdit(webhook); }} // TODO (SEAN): Implement Permissions here when we have event-types PR merged permissions={{ canEditWebhook: !webhookLockedStatus.disabled, canDeleteWebhook: !webhookLockedStatus.disabled, }} />packages/trpc/server/routers/viewer/webhook/create.handler.ts (1)
34-49: Remove manual PBAC check from handlerThe
createroute is already wrapped withcreateWebhookPbacProcedure("webhook.create")in packages/trpc/server/routers/viewer/webhook/_router.tsx (line 59), so you can remove the manual permission check in create.handler.ts to avoid duplication.packages/features/webhooks/components/CreateNewWebhookButton.tsx (3)
21-24: Avoid uppercasing localized stringstoUpperCase() can break locale-specific casing. Prefer styling (e.g., CSS uppercase) over string mutation.
12-18: Prefer absolute paths for router.push to avoid relative navigation issuesThese relative routes can misresolve depending on current path depth. Consider using absolute paths (e.g., with WEBAPP_URL/constants) for consistency.
35-35: Prefer named exports over default exportsStick to named exports for better tree-shaking/refactors.
Apply:
-export default CreateNewWebhookButton;packages/prisma/migrations/20250905115031_add_webhooks_permissions_default_roles/migration.sql (2)
2-12: Seed OWNER role too (if distinct from ADMIN)If OWNER is a distinct role in RolePermission, grant the same CRUD permissions to 'owner_role' for parity with UI fallbacks.
Proposed addition:
+-- Add webhook permissions for owner role +INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt") +SELECT gen_random_uuid(), 'owner_role', resource, action, NOW() +FROM ( + VALUES + ('webhook', 'create'), + ('webhook', 'read'), + ('webhook', 'update'), + ('webhook', 'delete') +) AS permissions(resource, action);
2-12: Make inserts idempotent to avoid duplicate rows on re-runsIf this migration might be replayed on environments with seeded data, guard against duplicates.
Example (Postgres):
-INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt") -SELECT - gen_random_uuid(), 'admin_role', resource, action, NOW() -FROM ( - VALUES - ('webhook', 'create'), - ('webhook', 'read'), - ('webhook', 'update'), - ('webhook', 'delete') -) AS permissions(resource, action); +INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt") +SELECT gen_random_uuid(), 'admin_role', v.resource, v.action, NOW() +FROM (VALUES + ('webhook', 'create'), + ('webhook', 'read'), + ('webhook', 'update'), + ('webhook', 'delete') +) AS v(resource, action) +LEFT JOIN "RolePermission" rp + ON rp."roleId" = 'admin_role' AND rp.resource = v.resource AND rp.action = v.action +WHERE rp.id IS NULL;Repeat similarly for 'member_role'.
Also applies to: 15-22
packages/features/webhooks/pages/webhooks-view.tsx (1)
96-96: Prefer named exports over default exportsFollow repo guideline to avoid default exports in components.
Apply:
-export default WebhooksView; +export { WebhooksView as default }; // or export named elsewhere and update importspackages/features/webhooks/components/WebhookListItem.tsx (3)
36-43: Remove legacy canEditWebhook prop; rely on permissions object onlyThe standalone canEditWebhook prop is now redundant and risks confusion.
Apply:
export default function WebhookListItem(props: { webhook: WebhookProps; - canEditWebhook?: boolean; onEditWebhook: () => void; lastItem: boolean; permissions: { canEditWebhook?: boolean; canDeleteWebhook?: boolean; }; }) {
112-126: Hide the toggle switch when user cannot editCurrently rendered but disabled; minor UX polish to only render when permitted.
Apply:
- {(props.permissions.canEditWebhook || props.permissions.canDeleteWebhook) && ( + {(props.permissions.canEditWebhook || props.permissions.canDeleteWebhook) && ( <div className="ml-2 flex items-center space-x-4"> - <Switch - defaultChecked={webhook.active} - data-testid="webhook-switch" - disabled={!props.permissions.canEditWebhook} - onCheckedChange={() => - toggleWebhook.mutate({ - id: webhook.id, - active: !webhook.active, - payloadTemplate: webhook.payloadTemplate, - eventTypeId: webhook.eventTypeId || undefined, - }) - } - /> + {props.permissions.canEditWebhook && ( + <Switch + defaultChecked={webhook.active} + data-testid="webhook-switch" + onCheckedChange={() => + toggleWebhook.mutate({ + id: webhook.id, + active: !webhook.active, + payloadTemplate: webhook.payloadTemplate, + eventTypeId: webhook.eventTypeId || undefined, + }) + } + /> + )}
70-77: Add a confirmation before destructive deletePrevent accidental deletions; localized strings already exist.
Apply:
const onDeleteWebhook = () => { - // TODO: Confimation dialog before deleting - deleteWebhook.mutate({ - id: webhook.id, - eventTypeId: webhook.eventTypeId || undefined, - teamId: webhook.teamId || undefined, - }); + if ( + window.confirm( + `${t("delete_webhook_confirmation_message")}\n\n${t("confirm_delete_webhook")}` + ) + ) { + deleteWebhook.mutate({ + id: webhook.id, + eventTypeId: webhook.eventTypeId || undefined, + teamId: webhook.teamId || undefined, + }); + } };packages/trpc/server/routers/viewer/webhook/util.test.ts (3)
54-62: Reset both.useand.inputspies to prevent cross-test leaksCurrently only
.useis cleared. Clear.inputtoo.Apply:
beforeEach(() => { vi.clearAllMocks(); // Reset the mock to ensure clean state mockCheckPermission.mockReset(); mockPrisma.webhook.findUnique.mockReset(); mockPrisma.eventType.findUnique.mockReset(); mockPrisma.user.findUnique.mockReset(); - mockAuthedProcedure.use.mockClear(); + mockAuthedProcedure.use.mockClear(); + mockAuthedProcedure.input?.mockClear?.(); });
132-144: Use robust TRPCError assertions; matching instances is brittle
rejects.toThrow(new TRPCError(...))can be flaky. Prefer matching by shape.Apply (example for each occurrence):
-).rejects.toThrow( - new TRPCError({ - code: "FORBIDDEN", - message: `Permission required: ${testPermission}`, - }) -); +).rejects.toMatchObject({ + code: "FORBIDDEN", + message: `Permission required: ${testPermission}`, +});-).rejects.toThrow(new TRPCError({ code: "NOT_FOUND" })); +).rejects.toMatchObject({ code: "NOT_FOUND" });-).rejects.toThrow(new TRPCError({ code: "UNAUTHORIZED" })); +).rejects.toMatchObject({ code: "UNAUTHORIZED" });Also applies to: 185-197, 301-313, 248-249, 272-273
382-388: This doesn't test the legacy wrapper; either import it or adjust the descriptionYou’re not importing or asserting anything about
webhookProcedure. Import it and assert it’s defined, or rename the test to reflect what’s actually asserted.Possible change:
-import { createWebhookPbacProcedure } from "./util"; +import { createWebhookPbacProcedure, webhookProcedure } from "./util"; ... - describe("webhookProcedure (legacy wrapper)", () => { + describe("webhookProcedure (legacy wrapper)", () => { it("should work as expected", () => { - // The legacy webhookProcedure uses createWebhookPbacProcedure internally - // This is verified by the functional tests above - expect(createWebhookPbacProcedure).toBeDefined(); + expect(webhookProcedure).toBeDefined(); }); });packages/trpc/server/routers/viewer/webhook/util.ts (1)
165-170: Legacy alias is fine for BC, but consider deprecating with JSDocAdd a
@deprecatedtag to steer new code towards the factory.Example:
/** * @deprecated Use createWebhookPbacProcedure(...) directly. */ export const webhookProcedure = createWebhookPbacProcedure("webhook.update", ["ADMIN", "OWNER"]);packages/trpc/server/routers/viewer/webhook/_router.tsx (2)
23-39: List route: enforce PBAC via filtering in the handlerSince the PBAC middleware skips checks when there’s no input, ensure
list.handlerfilters by teams the viewer haswebhook.readon (e.g., PermissionCheckService.getTeamIdsWithPermission) and scopes the query accordingly. This addresses the “missing permission filtering logic in list.handler.ts” noted in the PR objectives.
133-150: getByViewer route likely needs the same server-side filteringNo input means middleware won’t check permissions. Ensure
getByViewer.handlerrestricts results to teams/orgs where the viewer haswebhook.read.packages/lib/server/repository/webhook.ts (3)
20-21: Type safety: use a “safe webhook” type to prevent secret regressionsChange WebhookGroup.webhooks to a safe type (no secret) and let Prisma’s select enforce it.
Add this type outside the diffed ranges:
type SafeWebhook = Pick< Webhook, | "id" | "subscriberUrl" | "payloadTemplate" | "active" | "eventTriggers" | "teamId" | "userId" | "platform" | "time" | "timeUnit" | "appId" >; type WebhookGroup = { // ... webhooks: SafeWebhook[]; };
23-31: Minor: use a Set for appIdsTiny perf/readability nit.
Apply this diff:
-const filterWebhooks = (webhook: Webhook) => { - const appIds = [ - "zapier", - "make", - // Add more if needed - ]; - return !appIds.some((appId: string) => webhook.appId == appId); -}; +const filterWebhooks = (webhook: Webhook) => { + const appIds = new Set(["zapier", "make"]); + return !appIds.has(webhook.appId ?? ""); +};
33-53: Enforce CI guardrail against leakingsecret
Fail CI if anyselect: {...secret: true}or implicit webhook selects or standalone.secretusages slip in:#!/bin/bash rg -nP --type=ts -C2 'select\s*:\s*\{[^}]*\bsecret\s*:\s*true' rg -nP --type=ts -C3 'select\s*:\s*\{[^}]*\bwebhooks\s*:\s*true' rg -nP --type=ts -C2 '\.secret\b'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx(1 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/eventtypes/components/tabs/instant/InstantEventController.tsx(1 hunks)packages/features/pbac/domain/types/permission-registry.ts(2 hunks)packages/features/webhooks/components/CreateNewWebhookButton.tsx(2 hunks)packages/features/webhooks/components/WebhookListItem.tsx(4 hunks)packages/features/webhooks/pages/webhooks-view.tsx(4 hunks)packages/lib/server/repository/webhook.ts(6 hunks)packages/prisma/migrations/20250905115031_add_webhooks_permissions_default_roles/migration.sql(1 hunks)packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts(0 hunks)packages/trpc/server/routers/viewer/webhook/_router.tsx(2 hunks)packages/trpc/server/routers/viewer/webhook/create.handler.ts(2 hunks)packages/trpc/server/routers/viewer/webhook/delete.handler.ts(2 hunks)packages/trpc/server/routers/viewer/webhook/edit.handler.ts(2 hunks)packages/trpc/server/routers/viewer/webhook/getByViewer.handler.ts(1 hunks)packages/trpc/server/routers/viewer/webhook/list.handler.ts(2 hunks)packages/trpc/server/routers/viewer/webhook/util.test.ts(1 hunks)packages/trpc/server/routers/viewer/webhook/util.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/trpc/server/routers/loggedInViewer/teamsAndUserProfilesQuery.handler.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/trpc/server/routers/viewer/webhook/list.handler.tspackages/trpc/server/routers/viewer/webhook/getByViewer.handler.tspackages/trpc/server/routers/viewer/webhook/util.tspackages/trpc/server/routers/viewer/webhook/delete.handler.tspackages/trpc/server/routers/viewer/webhook/edit.handler.tspackages/trpc/server/routers/viewer/webhook/util.test.tspackages/trpc/server/routers/viewer/webhook/create.handler.tspackages/features/pbac/domain/types/permission-registry.tspackages/lib/server/repository/webhook.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/trpc/server/routers/viewer/webhook/list.handler.tspackages/trpc/server/routers/viewer/webhook/getByViewer.handler.tspackages/features/webhooks/components/CreateNewWebhookButton.tsxpackages/trpc/server/routers/viewer/webhook/util.tspackages/trpc/server/routers/viewer/webhook/delete.handler.tspackages/trpc/server/routers/viewer/webhook/edit.handler.tspackages/trpc/server/routers/viewer/webhook/util.test.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsxpackages/trpc/server/routers/viewer/webhook/create.handler.tspackages/features/eventtypes/components/tabs/instant/InstantEventController.tsxpackages/features/webhooks/components/WebhookListItem.tsxpackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/webhook/_router.tsxpackages/features/webhooks/pages/webhooks-view.tsxpackages/lib/server/repository/webhook.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/trpc/server/routers/viewer/webhook/list.handler.tspackages/trpc/server/routers/viewer/webhook/getByViewer.handler.tspackages/features/webhooks/components/CreateNewWebhookButton.tsxpackages/trpc/server/routers/viewer/webhook/util.tspackages/trpc/server/routers/viewer/webhook/delete.handler.tspackages/trpc/server/routers/viewer/webhook/edit.handler.tspackages/trpc/server/routers/viewer/webhook/util.test.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsxpackages/trpc/server/routers/viewer/webhook/create.handler.tspackages/features/eventtypes/components/tabs/instant/InstantEventController.tsxpackages/features/webhooks/components/WebhookListItem.tsxpackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/webhook/_router.tsxpackages/features/webhooks/pages/webhooks-view.tsxpackages/lib/server/repository/webhook.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/webhooks/components/CreateNewWebhookButton.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsxpackages/features/eventtypes/components/tabs/instant/InstantEventController.tsxpackages/features/webhooks/components/WebhookListItem.tsxpackages/trpc/server/routers/viewer/webhook/_router.tsxpackages/features/webhooks/pages/webhooks-view.tsx
🧠 Learnings (12)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#23614
File: packages/features/pbac/domain/types/permission-registry.ts:520-551
Timestamp: 2025-09-08T12:46:13.448Z
Learning: Webhooks in Cal.com support both team-level and organization-level scopes, so webhook PBAC permissions should not be constrained to Team scope only.
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.tspackages/trpc/server/routers/viewer/webhook/util.tspackages/trpc/server/routers/viewer/webhook/delete.handler.tspackages/trpc/server/routers/viewer/webhook/create.handler.ts
📚 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/trpc/server/routers/viewer/webhook/list.handler.tspackages/trpc/server/routers/viewer/webhook/delete.handler.ts
📚 Learning: 2025-09-08T12:46:13.448Z
Learnt from: sean-brydon
PR: calcom/cal.com#23614
File: packages/features/pbac/domain/types/permission-registry.ts:520-551
Timestamp: 2025-09-08T12:46:13.448Z
Learning: Webhooks in Cal.com support both team-level and organization-level scopes, so webhook PBAC permissions should not be constrained to Team scope only.
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.tspackages/trpc/server/routers/viewer/webhook/util.tspackages/features/pbac/domain/types/permission-registry.ts
📚 Learning: 2025-09-09T03:29:42.984Z
Learnt from: emrysal
PR: calcom/cal.com#23692
File: packages/lib/server/service/InsightsBookingBaseService.ts:16-16
Timestamp: 2025-09-09T03:29:42.984Z
Learning: In the Cal.com codebase, readonlyPrisma is still an instance of PrismaClient, making type changes from `typeof readonlyPrisma` to `PrismaClient` less critical since they are fundamentally compatible types.
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.ts
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`
Applied to files:
packages/trpc/server/routers/viewer/webhook/util.ts
📚 Learning: 2025-08-21T12:28:42.018Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#23247
File: packages/features/webhooks/lib/factory/WebhookPayloadFactory.ts:274-282
Timestamp: 2025-08-21T12:28:42.018Z
Learning: In webhook DTOs in packages/features/webhooks/lib/dto/types.ts, the booking fields are restricted structures containing only specific fields (id, eventTypeId, userId, and sometimes additional fields like startTime or smsReminderNumber) rather than full database booking objects, so there are no security or PII leakage concerns when using these booking objects in webhook payloads.
Applied to files:
packages/trpc/server/routers/viewer/webhook/util.ts
📚 Learning: 2025-08-21T13:44:06.805Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/trpc/server/routers/viewer/webhook/util.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma include uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.
Applied to files:
packages/trpc/server/routers/viewer/webhook/util.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma select uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.
Applied to files:
packages/trpc/server/routers/viewer/webhook/util.ts
📚 Learning: 2025-09-06T11:00:34.348Z
Learnt from: ShashwatPS
PR: calcom/cal.com#23638
File: packages/trpc/server/routers/viewer/calendars/setDestinationReminder.handler.test.ts:198-199
Timestamp: 2025-09-06T11:00:34.348Z
Learning: In calcom/cal.com PR #23638, the maintainer ShashwatPS determined that authorization checks in the setDestinationReminder handler are "not applicable" when CodeRabbit suggested adding user-scoped WHERE clauses to prevent users from modifying other users' destination calendar reminder settings.
Applied to files:
packages/trpc/server/routers/viewer/webhook/delete.handler.ts
📚 Learning: 2025-08-26T20:23:28.396Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:83-88
Timestamp: 2025-08-26T20:23:28.396Z
Learning: In calcom/cal.com PR #22995, the workflow update handler in packages/trpc/server/routers/viewer/workflows/update.handler.ts includes workflow-level authorization via isAuthorized(userWorkflow, ctx.user.id, "workflow.update") which validates the user can update the workflow before calling updateToolsFromAgentId (per maintainer Udit-takkar).
Applied to files:
packages/trpc/server/routers/viewer/webhook/delete.handler.tspackages/trpc/server/routers/viewer/webhook/edit.handler.tspackages/trpc/server/routers/viewer/webhook/create.handler.ts
🧬 Code graph analysis (12)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(21-366)
packages/trpc/server/routers/viewer/webhook/getByViewer.handler.ts (1)
packages/lib/server/repository/webhook.ts (1)
WebhookRepository(33-194)
packages/features/webhooks/components/CreateNewWebhookButton.tsx (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/trpc/server/routers/viewer/webhook/util.ts (3)
packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(61-61)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)
packages/trpc/server/routers/viewer/webhook/delete.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)packages/platform/libraries/index.ts (2)
MembershipRole(34-34)TRPCError(66-66)
packages/trpc/server/routers/viewer/webhook/edit.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)packages/platform/libraries/index.ts (2)
MembershipRole(34-34)TRPCError(66-66)
packages/trpc/server/routers/viewer/webhook/util.test.ts (2)
packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(61-61)packages/trpc/server/routers/viewer/webhook/util.ts (1)
createWebhookPbacProcedure(17-163)
packages/trpc/server/routers/viewer/webhook/create.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)
packages/features/webhooks/components/WebhookListItem.tsx (1)
packages/ui/components/dropdown/Dropdown.tsx (6)
Dropdown(12-12)DropdownMenuTrigger(15-26)DropdownMenuContent(34-51)DropdownMenuItem(63-71)DropdownItem(161-181)DropdownMenuSeparator(184-194)
packages/trpc/server/routers/viewer/webhook/_router.tsx (1)
packages/trpc/server/routers/viewer/webhook/util.ts (1)
createWebhookPbacProcedure(17-163)
packages/features/webhooks/pages/webhooks-view.tsx (3)
packages/trpc/server/routers/viewer/webhook/getByViewer.handler.ts (1)
WebhooksByViewer(24-33)packages/trpc/react/trpc.ts (1)
RouterOutputs(143-143)packages/features/webhooks/components/CreateNewWebhookButton.tsx (1)
CreateNewWebhookButton(9-33)
packages/lib/server/repository/webhook.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(21-366)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
🔇 Additional comments (14)
packages/trpc/server/routers/viewer/webhook/getByViewer.handler.ts (1)
36-41: LGTM: switched to PBAC-aware repository methodGood move to use getFilteredWebhooksForUser; it centralizes filtering and permission-derived metadata.
packages/trpc/server/routers/viewer/webhook/edit.handler.ts (1)
1-1: Router-level PBAC in place; no duplicate checks detected
Verified thatedit(and related) mutations inpackages/trpc/server/routers/viewer/webhook/_router.tsxare wrapped withcreateWebhookPbacProcedure("webhook.update")and no additional PBAC logic exists in handlers.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx (1)
31-31: Approve WebhooksView prop removal
WebhooksView’s Props no longer include isAdmin and all instances (including apps/web/.../page.tsx) now pass only data.packages/trpc/server/routers/viewer/webhook/delete.handler.ts (1)
29-43: Good: team-scoped delete permission with fallbacksThe check for webhook.delete with [ADMIN, OWNER] fallback is appropriate.
packages/features/webhooks/components/CreateNewWebhookButton.tsx (1)
27-30: PBAC gating and legacy fallbacks verified
Server‐sidecreateWebhookPbacProcedure("webhook.create")defaults to["ADMIN","OWNER"](confirmed by util tests andcreate.handler.ts), matching the client’s fallback roles.apps/web/public/static/locales/en/common.json (1)
3438-3442: Webhook PBAC i18n keys added — LGTMKeys align with the registry entries and existing action labels.
packages/features/pbac/domain/types/permission-registry.ts (2)
12-12: Resource.Webhook enum added — LGTM
520-551: Webhook CRUD permission entries look correct
- Depends on read for mutating actions.
- No explicit scope — supports both Team and Organization as intended.
packages/features/webhooks/components/WebhookListItem.tsx (1)
92-97: Readonly badge logic — LGTMBadge shows when edit not permitted; matches PBAC expectations.
packages/trpc/server/routers/viewer/webhook/util.ts (1)
53-68: Error semantics look good; keepFORBIDDENmessages consistentYou consistently return
FORBIDDENwithPermission required: ${permission}. Good for UX and tests.Also applies to: 95-108, 119-157
packages/trpc/server/routers/viewer/webhook/_router.tsx (1)
41-58: Fallback roles include MEMBER for get; confirm product intentUsing ["ADMIN", "OWNER", "MEMBER"] for read may be correct, but it widens legacy access when PBAC is disabled. Confirm with product/security that MEMBERs should read webhooks in legacy mode.
packages/lib/server/repository/webhook.ts (3)
1-1: Import looks goodRequired for permission checks.
17-19: Metadata shape change LGTMDownstream UI relying on readOnly should be updated; otherwise fine.
Please confirm no callers still expect metadata.readOnly.
100-113: Personal group OK; ensure user.webhooks schema is sanitized as aboveOnce the select is fixed, this block is fine.
| permissions={{ | ||
| canEditWebhook: group?.metadata?.canModify ?? false, | ||
| canDeleteWebhook: group?.metadata?.canDelete ?? false, | ||
| }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mapping server metadata to UI permissions — ensure backend populates these
UI derives canEdit/canDelete from group.metadata.canModify/canDelete. Please confirm list/get handlers set these via PBAC checks.
Run:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C3 'getByViewer\.handler|list\.handler' packages/trpc/server/routers/viewer/webhook
rg -nP --type=ts -C3 '\bmetadata\b.*(canModify|canDelete)' packages/trpc/server/routers/viewer/webhookLength of output: 2781
🏁 Script executed:
#!/bin/bash
rg -n -C3 'metadata' packages/trpc/server/routers/viewer/webhook/*.handler.ts
rg -n -C3 'canModify|canDelete' packages/trpc/server/routers/viewer/webhook/*.handler.tsLength of output: 1731
🏁 Script executed:
#!/bin/bash
ls -1 packages/trpc/server/routers/viewerLength of output: 350
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' packages/trpc/server/routers/viewer/webhook/list.handler.tsLength of output: 2417
Add canModify/canDelete metadata in list & getByViewer handlers
list.handler.ts & getByViewer.handler.ts don’t set metadata.canModify/metadata.canDelete—use PermissionCheckService to check "webhook.modify" and "webhook.delete" and include those booleans in each webhook’s metadata before returning.
🤖 Prompt for AI Agents
In packages/features/webhooks/pages/webhooks-view.tsx around lines 68 to 71, the
UI expects each webhook to have metadata.canModify and metadata.canDelete but
list.handler.ts and getByViewer.handler.ts do not populate those fields; update
both handlers to call PermissionCheckService (checking "webhook.modify" and
"webhook.delete") for each webhook and set metadata.canModify and
metadata.canDelete to the returned booleans on every webhook object before
returning the list/result so the front-end permissions prop is accurate.
| where: { id: userId }, | ||
| select: { | ||
| id: true, | ||
| username: true, | ||
| avatarUrl: true, | ||
| name: true, | ||
| avatarUrl: true, | ||
| webhooks: true, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not select full webhooks via relations; drop secret at the source
- user.webhooks: true and team.webhooks: true will include secret and fetch webhooks for teams even when canRead=false.
- Select only safe fields and avoid prefetching team.webhooks here.
Apply this diff:
const user = await prisma.user.findUnique({
where: { id: userId },
select: {
id: true,
username: true,
name: true,
- avatarUrl: true,
- webhooks: true,
+ avatarUrl: true,
+ webhooks: {
+ where: { appId: { notIn: ["zapier", "make"] } },
+ select: {
+ id: true,
+ subscriberUrl: true,
+ payloadTemplate: true,
+ active: true,
+ eventTriggers: true,
+ teamId: true,
+ userId: true,
+ platform: true,
+ time: true,
+ timeUnit: true,
+ appId: true,
+ },
+ },
teams: {
where: {
accepted: true,
},
select: {
role: true,
team: {
select: {
id: true,
name: true,
slug: true,
- logoUrl: true,
- webhooks: true,
+ logoUrl: true,
},
},
},
},
},
});Follow-up in the loop below replaces membership.team.webhooks usage.
Also applies to: 75-83
🤖 Prompt for AI Agents
In packages/lib/server/repository/webhook.ts around lines 63-69 (and similarly
lines 75-83), the code selects user.webhooks: true and team.webhooks: true which
prefetches full webhook objects including secrets and pulls team webhooks even
when canRead=false; change the select to avoid selecting full relations and
instead explicitly select only safe, non-secret webhook fields (e.g., id, name,
url, enabled, createdAt) via nested select objects like webhooks: { select: {
id: true, name: true, url: true, enabled: true, createdAt: true } } and remove
any team.webhooks: true selection; follow the follow-up note to replace any
later usage of membership.team.webhooks to reference the restricted/selective
fields or fetch team webhooks separately with proper authorization if secrets
are required.
| // Check permissions for each team | ||
| // The permission service handles PBAC when enabled and falls back to role-based permissions | ||
| for (const membership of user.teams) { | ||
| const teamId = membership.team.id; | ||
|
|
||
| // Check read permission (fallback: MEMBER, ADMIN, OWNER can read) | ||
| const canRead = await permissionService.checkPermission({ | ||
| userId, | ||
| teamId, | ||
| permission: "webhook.read", | ||
| fallbackRoles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], | ||
| }); | ||
|
|
||
| if (!canRead) { | ||
| // User doesn't have permission to view this team's webhooks | ||
| continue; | ||
| } | ||
|
|
||
| const teamWebhookGroups: WebhookGroup[] = user.teams.map((membership) => { | ||
| const orgMembership = teamMemberships.find( | ||
| (teamM) => teamM.teamId === membership.team.parentId | ||
| )?.membershipRole; | ||
| return { | ||
| // Check update/delete permissions in parallel (fallback: only ADMIN, OWNER can modify) | ||
| const [canUpdate, canDelete] = await Promise.all([ | ||
| permissionService.checkPermission({ | ||
| userId, | ||
| teamId, | ||
| permission: "webhook.update", | ||
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], | ||
| }), | ||
| permissionService.checkPermission({ | ||
| userId, | ||
| teamId, | ||
| permission: "webhook.delete", | ||
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], | ||
| }), | ||
| ]); | ||
|
|
||
| webhookGroups.push({ | ||
| teamId: membership.team.id, | ||
| profile: { | ||
| name: membership.team.name, | ||
| slug: membership.team.slug | ||
| ? !membership.team.parentId | ||
| ? `/team` | ||
| : `${membership.team.slug}` | ||
| : null, | ||
| slug: membership.team.slug || null, | ||
| image: getPlaceholderAvatar(membership.team.logoUrl, membership.team.name), | ||
| }, | ||
| webhooks: membership.team.webhooks.filter(filterWebhooks), | ||
| metadata: { | ||
| readOnly: | ||
| membership.role === | ||
| (membership.team.parentId | ||
| ? orgMembership && compareMembership(orgMembership, membership.role) | ||
| ? orgMembership | ||
| : MembershipRole.MEMBER | ||
| : MembershipRole.MEMBER), | ||
| canModify: canUpdate, | ||
| canDelete, | ||
| }, | ||
| webhooks: membership.team.webhooks.filter(filterWebhooks), | ||
| }; | ||
| }); | ||
|
|
||
| webhookGroups = webhookGroups.concat(teamWebhookGroups); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t rely on pre-fetched team.webhooks; fetch sanitized data only for teams the viewer can read
Avoid retaining or touching team.webhooks when canRead=false and ensure secrets never enter memory. Fetch per-team after permission check.
Apply this diff:
if (!canRead) {
// User doesn't have permission to view this team's webhooks
continue;
}
- // Check update/delete permissions in parallel (fallback: only ADMIN, OWNER can modify)
+ // Check update/delete permissions in parallel (fallback: only ADMIN, OWNER can modify)
const [canUpdate, canDelete] = await Promise.all([
permissionService.checkPermission({
userId,
teamId,
permission: "webhook.update",
fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
}),
permissionService.checkPermission({
userId,
teamId,
permission: "webhook.delete",
fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
}),
]);
+ // Fetch team webhooks now that we know the user can read
+ const teamWebhooks = await prisma.webhook.findMany({
+ where: { teamId, appId: { notIn: ["zapier", "make"] } },
+ select: {
+ id: true,
+ subscriberUrl: true,
+ payloadTemplate: true,
+ active: true,
+ eventTriggers: true,
+ teamId: true,
+ userId: true,
+ platform: true,
+ time: true,
+ timeUnit: true,
+ appId: true,
+ },
+ });
+
webhookGroups.push({
teamId: membership.team.id,
profile: {
name: membership.team.name,
slug: membership.team.slug || null,
image: getPlaceholderAvatar(membership.team.logoUrl, membership.team.name),
},
- webhooks: membership.team.webhooks.filter(filterWebhooks),
+ webhooks: teamWebhooks,
metadata: {
canModify: canUpdate,
canDelete,
},
});Optional optimization: batch via getTeamIdsWithPermission + single findMany and group in-memory.
📝 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.
| // Check permissions for each team | |
| // The permission service handles PBAC when enabled and falls back to role-based permissions | |
| for (const membership of user.teams) { | |
| const teamId = membership.team.id; | |
| // Check read permission (fallback: MEMBER, ADMIN, OWNER can read) | |
| const canRead = await permissionService.checkPermission({ | |
| userId, | |
| teamId, | |
| permission: "webhook.read", | |
| fallbackRoles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], | |
| }); | |
| if (!canRead) { | |
| // User doesn't have permission to view this team's webhooks | |
| continue; | |
| } | |
| const teamWebhookGroups: WebhookGroup[] = user.teams.map((membership) => { | |
| const orgMembership = teamMemberships.find( | |
| (teamM) => teamM.teamId === membership.team.parentId | |
| )?.membershipRole; | |
| return { | |
| // Check update/delete permissions in parallel (fallback: only ADMIN, OWNER can modify) | |
| const [canUpdate, canDelete] = await Promise.all([ | |
| permissionService.checkPermission({ | |
| userId, | |
| teamId, | |
| permission: "webhook.update", | |
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], | |
| }), | |
| permissionService.checkPermission({ | |
| userId, | |
| teamId, | |
| permission: "webhook.delete", | |
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], | |
| }), | |
| ]); | |
| webhookGroups.push({ | |
| teamId: membership.team.id, | |
| profile: { | |
| name: membership.team.name, | |
| slug: membership.team.slug | |
| ? !membership.team.parentId | |
| ? `/team` | |
| : `${membership.team.slug}` | |
| : null, | |
| slug: membership.team.slug || null, | |
| image: getPlaceholderAvatar(membership.team.logoUrl, membership.team.name), | |
| }, | |
| webhooks: membership.team.webhooks.filter(filterWebhooks), | |
| metadata: { | |
| readOnly: | |
| membership.role === | |
| (membership.team.parentId | |
| ? orgMembership && compareMembership(orgMembership, membership.role) | |
| ? orgMembership | |
| : MembershipRole.MEMBER | |
| : MembershipRole.MEMBER), | |
| canModify: canUpdate, | |
| canDelete, | |
| }, | |
| webhooks: membership.team.webhooks.filter(filterWebhooks), | |
| }; | |
| }); | |
| webhookGroups = webhookGroups.concat(teamWebhookGroups); | |
| }); | |
| } | |
| // Check permissions for each team | |
| // The permission service handles PBAC when enabled and falls back to role-based permissions | |
| for (const membership of user.teams) { | |
| const teamId = membership.team.id; | |
| // Check read permission (fallback: MEMBER, ADMIN, OWNER can read) | |
| const canRead = await permissionService.checkPermission({ | |
| userId, | |
| teamId, | |
| permission: "webhook.read", | |
| fallbackRoles: [MembershipRole.MEMBER, MembershipRole.ADMIN, MembershipRole.OWNER], | |
| }); | |
| if (!canRead) { | |
| // User doesn't have permission to view this team's webhooks | |
| continue; | |
| } | |
| // Check update/delete permissions in parallel (fallback: only ADMIN, OWNER can modify) | |
| const [canUpdate, canDelete] = await Promise.all([ | |
| permissionService.checkPermission({ | |
| userId, | |
| teamId, | |
| permission: "webhook.update", | |
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], | |
| }), | |
| permissionService.checkPermission({ | |
| userId, | |
| teamId, | |
| permission: "webhook.delete", | |
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], | |
| }), | |
| ]); | |
| // Fetch team webhooks now that we know the user can read | |
| const teamWebhooks = await prisma.webhook.findMany({ | |
| where: { teamId, appId: { notIn: ["zapier", "make"] } }, | |
| select: { | |
| id: true, | |
| subscriberUrl: true, | |
| payloadTemplate: true, | |
| active: true, | |
| eventTriggers: true, | |
| teamId: true, | |
| userId: true, | |
| platform: true, | |
| time: true, | |
| timeUnit: true, | |
| appId: true, | |
| }, | |
| }); | |
| webhookGroups.push({ | |
| teamId: membership.team.id, | |
| profile: { | |
| name: membership.team.name, | |
| slug: membership.team.slug || null, | |
| image: getPlaceholderAvatar(membership.team.logoUrl, membership.team.name), | |
| }, | |
| webhooks: teamWebhooks, | |
| metadata: { | |
| canModify: canUpdate, | |
| canDelete, | |
| }, | |
| }); | |
| } |
|
@sean-brydon if
then still the user should be able to perform CRUD on personal-scoped workflows, right? I'm testing that scenario and not seeing "+ new" button. (but RUD is working) |
| setEditModalOpen(true); | ||
| setWebhookToEdit(webhook); | ||
| }} | ||
| // TODO (SEAN): Implement Permissions here when we have event-types PR merged |
There was a problem hiding this comment.
need to fix this (leaving a comment for visibility)
eunjae-lee
dismissed
their stale review
my test was wrong. it's working well.
keithwillcode
left a comment
keithwillcode
left a comment
There was a problem hiding this comment.
Approving the migration from foundation side.
3 participants
What does this PR do?
Video Demo: https://cap.so/s/a9r9agv8yvqgt7z
This PR implements Permission-Based Access Control (PBAC) for webhooks, following the same pattern established in #22845 for workflows. The implementation adds webhook resource permissions to the PBAC system and enforces permission checks for team-level webhook operations.
Key Changes:
Webhookresource to PBAC permission registry with CRUD actionsScope: Only team-level webhooks are affected. User-level webhooks and EventType-level webhooks continue using existing authorization logic.
Requested by @sean-brydon
Link to Devin run: https://app.devin.ai/sessions/540f6ef60980462dbbc215c063b5081a
How should this be tested?
Test scenarios:
webhook.createpermission can create team webhookswebhook.updatepermission cannot edit team webhookswebhook.deletepermission cannot delete team webhooksCritical Review Points
🚨 Incomplete Implementation:
list.handler.tsonly has import added but missing actual permission filtering logic🔍 Validation Needed:
Mandatory Tasks
Checklist