-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(webhooks): use next public app url instead of request origin for webhook registration #1596
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
Conversation
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.
Greptile Overview
Summary
This PR fixes webhook registration by replacing dynamic request origin detection with a strict requirement for NEXT_PUBLIC_APP_URL. Previously, the code used request.url.origin and fell back to the environment variable only for localhost, which could cause mismatches between registered webhook URLs and the actual notification URLs when external services try to reach the application.
Key Changes:
- Airtable webhook registration now requires
NEXT_PUBLIC_APP_URLand throws an error if not configured (route.ts:440-445) - Telegram webhook registration now requires
NEXT_PUBLIC_APP_URLand throws an error if not configured (route.ts:547-552) - Airtable webhook deletion now requires
NEXT_PUBLIC_APP_URLfor matching webhooks by notification URL ([id]/route.ts:285-290) - Test webhook endpoint uses
NEXT_PUBLIC_APP_URLwith fallback to request origin (test/route.ts:38) - Removed unnecessary code comments throughout
Benefits:
- Ensures consistent webhook URLs across all environments
- Prevents issues where external services can't reach webhooks due to URL mismatches
- Forces explicit configuration of the public-facing application URL
Confidence Score: 4/5
- This PR is safe to merge with low risk, addresses a real webhook configuration issue
- The changes are straightforward and improve webhook reliability by enforcing explicit URL configuration. However, there's a minor inconsistency where the test endpoint still has a fallback while the main endpoints don't, which could lead to confusion. The PR will require
NEXT_PUBLIC_APP_URLto be set in all environments, which is a breaking change if not already configured. - apps/sim/app/api/webhooks/test/route.ts - has inconsistent error handling compared to other webhook routes
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/app/api/webhooks/route.ts | 4/5 | Replaced dynamic request origin detection with strict NEXT_PUBLIC_APP_URL requirement for Airtable and Telegram webhook registration, adding validation to throw errors if not configured |
| apps/sim/app/api/webhooks/[id]/route.ts | 4/5 | Updated Airtable webhook deletion to require NEXT_PUBLIC_APP_URL for matching webhooks by notification URL, with strict validation |
| apps/sim/app/api/webhooks/test/route.ts | 5/5 | Changed webhook URL construction to prefer NEXT_PUBLIC_APP_URL with fallback to request origin (soft requirement), removed unnecessary comments |
Sequence Diagram
sequenceDiagram
participant Client
participant API as Webhook API
participant Env as Environment Config
participant External as External Service (Airtable/Telegram)
participant DB as Database
Note over Client,DB: Webhook Registration Flow (Before)
Client->>API: POST /api/webhooks
API->>API: Extract request.url.origin
API->>API: Check if localhost<br/>(fallback to NEXT_PUBLIC_APP_URL)
API->>External: Register webhook with notificationUrl
API->>DB: Save webhook config
API->>Client: Return webhook
Note over Client,DB: Webhook Registration Flow (After)
Client->>API: POST /api/webhooks
API->>Env: Check NEXT_PUBLIC_APP_URL
alt NEXT_PUBLIC_APP_URL missing
API->>Client: Error: NEXT_PUBLIC_APP_URL must be configured
else NEXT_PUBLIC_APP_URL present
API->>External: Register webhook with NEXT_PUBLIC_APP_URL
API->>DB: Save webhook config
API->>Client: Return webhook
end
Note over Client,DB: Webhook Deletion Flow (After)
Client->>API: DELETE /api/webhooks/[id]
API->>DB: Fetch webhook
API->>Env: Check NEXT_PUBLIC_APP_URL
alt NEXT_PUBLIC_APP_URL missing
API->>Client: Error: NEXT_PUBLIC_APP_URL must be configured
else NEXT_PUBLIC_APP_URL present
API->>API: Construct expected URL
API->>External: Match and delete webhook
API->>DB: Delete local record
API->>Client: Success
end
3 files reviewed, 1 comment
…webhook registration
32124e6 to
a726ee7
Compare
559ff0e to
9962d44
Compare
…webhook registration (#1596) * fix(webhooks): use next public app url instead of request origin for webhook registration * ack PR comments * ci: pin Bun to v1.2.22 to avoid Bun 1.3 breaking changes
Summary
use next public app url instead of request origin for webhook registration
Type of Change
Testing
Tested manually.
Checklist