Skip to content

fix: align webhook edit page with PBAC middleware#28769

Merged
pedroccastro merged 3 commits intomainfrom
fix/webhook-settings-page-data-loading
Apr 14, 2026
Merged

fix: align webhook edit page with PBAC middleware#28769
pedroccastro merged 3 commits intomainfrom
fix/webhook-settings-page-data-loading

Conversation

@pedroccastro
Copy link
Copy Markdown
Contributor

@pedroccastro pedroccastro commented Apr 6, 2026

What does this PR do?

Adds authentication and permission verification to the webhook edit page. The page was loading webhook data directly without verifying the current user's access, which could result in incorrect data being displayed.

Changes

  • Added session check with redirect to login (consistent with other settings pages)
  • Added permission verification for team webhooks using PermissionCheckService
  • Added direct ownership check for personal webhooks
  • Returns 404 when the user doesn't have access to the requested webhook

How should this be tested?

  1. Navigate to /settings/developer/webhooks/{id} for your own webhook → should load normally
  2. Navigate to a webhook URL you don't have access to → should show 404
  3. Verify team webhooks are accessible by team admins/owners

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have updated the developer docs in /docs if this PR makes changes that would require a documentation change.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

@pedroccastro pedroccastro marked this pull request as ready for review April 7, 2026 20:46
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 7, 2026

📝 Walkthrough

Walkthrough

The webhook edit page now includes server-side authentication and authorization validation. The page retrieves the current user session via getServerSession and redirects unauthenticated users to the login page. After fetching the webhook by id, the page performs ownership and permission checks: for team-scoped webhooks, it verifies read permissions using PermissionCheckService with ADMIN, OWNER, or MEMBER roles; for user-scoped webhooks, it confirms the webhook belongs to the current user. Unauthorized access results in a notFound() response. The control flow now includes these auth and permission validation branches before rendering the EditWebhookView component.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: align webhook edit page with PBAC middleware' clearly summarizes the main change—adding permission-based access control alignment to the webhook edit page.
Description check ✅ Passed The description is directly related to the changeset, explaining what authentication and permission verification were added, why they were needed, and providing testing instructions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/webhook-settings-page-data-loading

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot reviewed Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents 
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@apps/web/app/`(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsx:
- Around line 34-35: The call to
WebhookRepository.getInstance().findByWebhookId(id) can throw (it uses
findUniqueOrThrow), so wrap the await webhookRepository.findByWebhookId(id) in a
try-catch inside the page handler (the page component surrounding code) and
catch the not-found/prisma error; when caught, return the same 404/unauthorized
response you use for missing/unauthorized webhooks (instead of letting it bubble
to a 500). Ensure you only rethrow unexpected errors and map the specific
not-found error from findByWebhookId to the existing 404 response path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c8d6e6f6-e0bf-4c8c-b727-555891db2c59

📥 Commits

Reviewing files that changed from the base of the PR and between 3c52f57 and 5a11d17.

📒 Files selected for processing (1)
  • apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsx

Comment thread apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsx
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 7, 2026 •
edited
Loading

E2E results are ready!

@pedroccastro pedroccastro merged commit a657723 into main Apr 14, 2026
40 checks passed
@pedroccastro pedroccastro deleted the fix/webhook-settings-page-data-loading branch April 14, 2026 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@sahitya-chandra sahitya-chandra sahitya-chandra approved these changes

Assignees

No one assigned

Labels

ready-for-e2e size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pedroccastro @sahitya-chandra