refactor(webhooks): extract provider-specific logic into handler registry#3973
refactor(webhooks): extract provider-specific logic into handler registry#3973waleedlatif1 merged 18 commits intostagingfrom
Conversation
PR SummaryMedium Risk Overview Provider behaviors (auth verification, challenge handling, event matching/skipping, idempotency key extraction/header enrichment, success/error response formatting, subscription create/delete, polling configuration, and optional file/input processing) are now invoked via handler methods, and the legacy Also tightens typing ( Reviewed by Cursor Bugbot for commit 98b4586. Configure here. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
f580455 to
6e95981
Compare
|
@greptile |
|
@cursor review |
Greptile SummaryThis PR extracts 14+ provider-specific if-chains from Key changes:
One P2 finding: The Confidence Score: 5/5Safe to merge — clean structural refactoring with no user-facing behavior changes; all prior P0/P1 concerns from previous review threads are resolved All previous reviewer concerns (array-aware processed initializer, any types in the three new helper functions, duplicate generic file processing) have been addressed in subsequent commits. The Jira bug fix is correctly applied (issueEventTypeName passed as string). The new handler pattern is well-typed with no new any introductions. The only remaining finding is a P2 maintenance concern about the hardcoded CHALLENGE_PROVIDERS list, which does not affect correctness today. apps/sim/lib/webhooks/processor.ts (CHALLENGE_PROVIDERS list at lines 116–125) — P2 maintenance concern only Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Webhook Request] --> B[handleProviderChallenges\nslack / teams / whatsapp]
B -->|challenge| C[Return 200 challenge response]
B -->|no match| D[parseWebhookBody]
D --> E[handlePreLookupWebhookVerification]
E --> F[findWebhookAndWorkflow]
F --> G[handleProviderReachabilityTest\nhandler.handleReachabilityTest]
G --> H[verifyProviderAuth\nhandler.verifyAuth]
H -->|401/403| I[formatProviderErrorResponse\nhandler.formatErrorResponse]
H -->|pass| J[handler.matchEvent]
J -->|skip| K[Return 200 skip message]
J -->|match| L[handler.enrichHeaders\nidempotency key injection]
L --> M[Billing / idempotency check]
M --> N[Enqueue executeWebhookJob]
N --> O[handler.formatSuccessResponse\nor default JSON 200]
N -.->|async| Q[executeWebhookJobInternal]
Q --> R{handler.formatInput?}
R -->|yes| S[Call formatInput]
R -->|no| T[Use payload.body directly]
S --> U{input null?}
T --> U
U -->|yes + handleEmptyInput| V[Log skip and return]
U -->|no| W[processTriggerFileOutputs\n+ handler.processInputFiles]
W --> X[executeWorkflowCore]
X --> Y[handleExecutionResult\ntimeout / pause / resume]
Reviews (8): Last reviewed commit: "refactor(webhooks): remove remaining any..." | Re-trigger Greptile |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6e95981. Configure here.
|
@cursor review |
|
@greptile |
|
@greptile |
|
@cursor review |
|
@cursor review |
|
@greptile |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0d116fa. Configure here.
- Restore original fall-through behavior for generic requireAuth with no token - Replace `any` params with proper types in processor helper functions - Restore array-aware initializer in processTriggerFileOutputs
…ggerFileOutputs Cast array initializer to Record<string, unknown> to allow string indexing while preserving array runtime semantics for the return value.
…gured If a user explicitly sets requireAuth: true, they expect auth to be enforced. Returning 401 when no token is configured is the correct behavior — this is an intentional improvement over the original code which silently allowed unauthenticated access in this case.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0d116fa to
8bcf450
Compare
|
@greptile |
|
@cursor review |
…execution The generic provider's processInputFiles handler already handles file[] field processing via the handler.processInputFiles call. The hardcoded block from staging was incorrectly preserved during rebase, causing double processing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 60610b7. Configure here.
… at deploy time Rejects deployment with a clear error message if a generic webhook trigger has requireAuth enabled but no authentication token configured, rather than letting requests fail with 401 at runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…olling config The refactored IMAP handler added a rejectUnauthorized field that was not present in the original configureImapPolling function. This would default to true for all existing IMAP webhooks, potentially breaking connections to servers with self-signed certificates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… handler Per project coding standards, use generateId() from @/lib/core/utils/uuid instead of crypto.randomUUID() directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…m providers - Standardize logger names to WebhookProvider:X pattern across 6 providers (fathom, gmail, imap, lemlist, outlook, rss) - Replace all `any` types in airtable handler with proper types: - Add AirtableTableChanges interface for API response typing - Change function params from `any` to `Record<string, unknown>` - Change AirtableChange fields from Record<string, any> to Record<string, unknown> - Change all catch blocks from `error: any` to `error: unknown` - Change input object from `any` to `Record<string, unknown>` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
Replace 3 `catch (error: any)` with `catch (error: unknown)` and 1 `Record<string, any>` with `Record<string, unknown>`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 98b4586. Configure here.
Summary
lib/webhooks/providers/implementing only the methods it needscreateHmacVerifierfactory,verifyTokenAuthhelper,skipByEventTypesfilterverifyProviderWebhookfrom utils.server.ts (merged into per-provider auth)Type of Change
Testing
Checklist